Skip to content

[CP staging] Revert "Allow negative response to be reprompted after 30 days" #96229

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jasperhuangg wants to merge 1 commit into from
Closed

[CP staging] Revert "Allow negative response to be reprompted after 30 days" #96229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions src/components/ProactiveAppReviewModalManager.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import useActivePolicy from '@hooks/useActivePolicy';
import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails';
import useDelegateAccountID from '@hooks/useDelegateAccountID';
import useOnyx from '@hooks/useOnyx';
Expand All @@ -7,7 +6,6 @@ import useProactiveAppReview from '@hooks/useProactiveAppReview';
import requestStoreReview from '@libs/actions/StoreReview';
import {respondToProactiveAppReview} from '@libs/actions/User';

import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {AppReviewResponse} from '@src/types/onyx/AppReview';

Expand All @@ -22,9 +20,6 @@ const CONCIERGE_NEGATIVE_MESSAGE = "Hi there! I'm sorry to hear you aren't fully
function ProactiveAppReviewModalManager() {
const {shouldShowModal, proactiveAppReview} = useProactiveAppReview();
const [conciergeReportID] = useOnyx(ONYXKEYS.CONCIERGE_REPORT_ID);
const activePolicy = useActivePolicy();
// Only send a policyID for real group workspaces, not personal policies.
const policyID = activePolicy && activePolicy.type !== CONST.POLICY.TYPE.PERSONAL ? activePolicy.id : undefined;
const [isAnyOtherModalActive] = useOnyx(ONYXKEYS.MODAL, {
selector: isModalActiveSelector,
});
Expand All @@ -50,7 +45,7 @@ function ProactiveAppReviewModalManager() {
}

const handleResponse = (response: AppReviewResponse, message?: string) => {
respondToProactiveAppReview(response, proactiveAppReview, currentUserEmail, currentUserAccountID, delegateAccountID, policyID, message, conciergeReportID);
respondToProactiveAppReview(response, proactiveAppReview, currentUserEmail, currentUserAccountID, delegateAccountID, message, conciergeReportID);
};

const handlePositive = () => {
Expand Down
5 changes: 3 additions & 2 deletions src/hooks/useProactiveAppReview.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,9 @@ function useProactiveAppReview(): UseProactiveAppReviewReturn {
return false;
}

// Don't show again after user gave a positive response
if (proactiveAppReview.response && proactiveAppReview.response === 'positive') {
// Don't show if user gave a definitive answer (positive/negative)
// Only allow re-prompting if they skipped
if (proactiveAppReview.response && proactiveAppReview.response !== 'skip') {
return false;
}

Expand Down
1 change: 0 additions & 1 deletion src/libs/API/parameters/RespondToProactiveAppReviewParams.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ type RespondToProactiveAppReviewParams = {
response: 'positive' | 'negative' | 'skip';
optimisticReportActionID?: string;
conciergeChatReportID?: string;
policyID?: string;
};

export default RespondToProactiveAppReviewParams;
4 changes: 1 addition & 3 deletions src/libs/actions/User.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1571,7 +1571,6 @@ function requestUnlockAccount(accountID: number) {
type RespondToProactiveAppReviewParams = {
response: 'positive' | 'negative' | 'skip';
optimisticReportActionID?: string;
policyID: string | undefined;
};

/**
Expand All @@ -1583,11 +1582,10 @@ function respondToProactiveAppReview(
userEmail: string | undefined,
userAccountID: number,
delegateAccountID: number | undefined,
policyID: string | undefined,
message?: string,
conciergeChatReportID?: string,
) {
const params: RespondToProactiveAppReviewParams = {response, policyID};
const params: RespondToProactiveAppReviewParams = {response};
const optimisticData: Array<OnyxUpdate<typeof ONYXKEYS.COLLECTION.REPORT_ACTIONS | typeof ONYXKEYS.COLLECTION.REPORT | typeof ONYXKEYS.NVP_APP_REVIEW>> = [];
const successData: Array<OnyxUpdate<typeof ONYXKEYS.COLLECTION.REPORT_ACTIONS>> = [];
const failureData: Array<OnyxUpdate<typeof ONYXKEYS.COLLECTION.REPORT_ACTIONS | typeof ONYXKEYS.NVP_APP_REVIEW>> = [];
Expand Down
13 changes: 6 additions & 7 deletions tests/actions/UserTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,24 +814,23 @@ describe('actions/User', () => {
const TEST_USER_ACCOUNT_ID = 42;
const TEST_USER_EMAIL = 'user@example.com';
const TEST_CONCIERGE_REPORT_ID = '999';
const TEST_POLICY_ID = '123';

it('should call RESPOND_TO_PROACTIVE_APP_REVIEW API with the given response', async () => {
UserActions.respondToProactiveAppReview('positive', undefined, TEST_USER_EMAIL, TEST_USER_ACCOUNT_ID, undefined, TEST_POLICY_ID);
UserActions.respondToProactiveAppReview('positive', undefined, TEST_USER_EMAIL, TEST_USER_ACCOUNT_ID, undefined);
await waitForBatchedUpdates();

expect(mockAPI.write).toHaveBeenCalledWith(WRITE_COMMANDS.RESPOND_TO_PROACTIVE_APP_REVIEW, expect.objectContaining({response: 'positive'}), expect.anything());
});

it('should call RESPOND_TO_PROACTIVE_APP_REVIEW API with skip response', async () => {
UserActions.respondToProactiveAppReview('skip', undefined, TEST_USER_EMAIL, TEST_USER_ACCOUNT_ID, undefined, TEST_POLICY_ID);
UserActions.respondToProactiveAppReview('skip', undefined, TEST_USER_EMAIL, TEST_USER_ACCOUNT_ID, undefined);
await waitForBatchedUpdates();

expect(mockAPI.write).toHaveBeenCalledWith(WRITE_COMMANDS.RESPOND_TO_PROACTIVE_APP_REVIEW, expect.objectContaining({response: 'skip'}), expect.anything());
});

it('should include optimisticReportActionID when message and conciergeChatReportID are provided for non-skip response', async () => {
UserActions.respondToProactiveAppReview('positive', undefined, TEST_USER_EMAIL, TEST_USER_ACCOUNT_ID, undefined, TEST_POLICY_ID, 'Great app!', TEST_CONCIERGE_REPORT_ID);
UserActions.respondToProactiveAppReview('positive', undefined, TEST_USER_EMAIL, TEST_USER_ACCOUNT_ID, undefined, 'Great app!', TEST_CONCIERGE_REPORT_ID);
await waitForBatchedUpdates();

expect(mockAPI.write).toHaveBeenCalledWith(
Expand All @@ -845,7 +844,7 @@ describe('actions/User', () => {
});

it('should NOT include optimisticReportActionID when response is skip even with message', async () => {
UserActions.respondToProactiveAppReview('skip', undefined, TEST_USER_EMAIL, TEST_USER_ACCOUNT_ID, undefined, TEST_POLICY_ID, 'Some message', TEST_CONCIERGE_REPORT_ID);
UserActions.respondToProactiveAppReview('skip', undefined, TEST_USER_EMAIL, TEST_USER_ACCOUNT_ID, undefined, 'Some message', TEST_CONCIERGE_REPORT_ID);
await waitForBatchedUpdates();

expect(mockAPI.write).toHaveBeenCalledWith(
Expand All @@ -860,12 +859,12 @@ describe('actions/User', () => {
await Onyx.merge(ONYXKEYS.NVP_APP_REVIEW, currentReview);
await waitForBatchedUpdates();

UserActions.respondToProactiveAppReview('negative', currentReview, TEST_USER_EMAIL, TEST_USER_ACCOUNT_ID, undefined, TEST_POLICY_ID);
UserActions.respondToProactiveAppReview('negative', currentReview, TEST_USER_EMAIL, TEST_USER_ACCOUNT_ID, undefined);
await waitForBatchedUpdates();

expect(mockAPI.write).toHaveBeenCalledWith(
WRITE_COMMANDS.RESPOND_TO_PROACTIVE_APP_REVIEW,
expect.objectContaining({response: 'negative', policyID: TEST_POLICY_ID}),
expect.objectContaining({response: 'negative'}),
expect.objectContaining({
optimisticData: expect.arrayContaining([
expect.objectContaining({
Expand Down
Loading