Skip to content
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

Require validateCode when requesting replacement cards #51147

Merged
merged 29 commits into from
Nov 7, 2024
Merged

Require validateCode when requesting replacement cards #51147

Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
2df4f36
Sort command alphabetically
youssef-lr Oct 21, 2024
e0af600
Move commands to side effect commands list
youssef-lr Oct 21, 2024
4e26e41
Use a side effect request when report virtual card fraud
youssef-lr Oct 21, 2024
e631c0a
Navigate back to the card page on error
youssef-lr Oct 21, 2024
172e11f
Set description of ValidateCodeActionModal
youssef-lr Oct 21, 2024
85a9dd7
Cleanup code
youssef-lr Oct 21, 2024
f60a62f
Make sure all validateCode modals have confirmation button docked
youssef-lr Oct 22, 2024
f53f47f
Handle loading state and display validetCode error
youssef-lr Oct 23, 2024
5845c87
Navigate to new cardID page and fix brief display of 404 page
youssef-lr Oct 24, 2024
2c3e090
Cleanup unused stuff
youssef-lr Oct 25, 2024
d774a7f
Merge branch 'main' into youssef_validateCode_replacement_cards
youssef-lr Oct 25, 2024
00ae18e
Merge branch 'main' into youssef_validateCode_replacement_cards
youssef-lr Oct 25, 2024
de7bf8a
Show validateCode error in the valideCode modal
youssef-lr Oct 26, 2024
9feb1cd
Use makeRequestWithSideEffects when requesting physical replacement card
youssef-lr Oct 26, 2024
0bfa399
Require validate code when replacing physical cards
youssef-lr Oct 26, 2024
e777ee5
Lint
youssef-lr Oct 26, 2024
419167a
More lint
youssef-lr Oct 26, 2024
73e89c1
Don't break existing code
youssef-lr Oct 26, 2024
4cec967
Remove usage of withOnyx from ReportCardLostPage
youssef-lr Oct 27, 2024
37edf51
Merge branch 'main' into youssef_validateCode_replacement_cards
youssef-lr Oct 29, 2024
dc649ea
Fix spinner not showing
youssef-lr Oct 29, 2024
1d830fc
Use ACCOUNT key for handling loading state
youssef-lr Oct 29, 2024
c7b2059
Display validateCode error above submit button
youssef-lr Oct 29, 2024
62b7fea
Merge branch 'main' into youssef_validateCode_replacement_cards
youssef-lr Nov 3, 2024
7ed59ee
Merge branch 'main' into youssef_validateCode_replacement_cards
youssef-lr Nov 6, 2024
e4af3c8
Keep offline pattern
youssef-lr Nov 6, 2024
1868fcb
Remove usage of withOnyx and other lint fixes
youssef-lr Nov 7, 2024
ff17b0e
Merge branch 'main' into youssef_validateCode_replacement_cards
youssef-lr Nov 7, 2024
cc4c172
Lint
youssef-lr Nov 7, 2024
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
1 change: 1 addition & 0 deletions src/components/ValidateCodeActionModal/ValidateCodeForm/BaseValidateCodeForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ function BaseValidateCodeForm({
pendingAction={validatePendingAction}
errors={validateError}
errorRowStyles={[styles.mt2]}
shouldDisplayErrorAbove
onClose={() => clearError()}
style={buttonStyles}
>
Expand Down
5 changes: 4 additions & 1 deletion src/components/ValidateCodeActionModal/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import HeaderWithBackButton from '@components/HeaderWithBackButton';
import Modal from '@components/Modal';
import ScreenWrapper from '@components/ScreenWrapper';
import Text from '@components/Text';
import useSafePaddingBottomStyle from '@hooks/useSafePaddingBottomStyle';
import useThemeStyles from '@hooks/useThemeStyles';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
Expand All @@ -27,6 +28,7 @@ function ValidateCodeActionModal({
hasMagicCodeBeenSent,
}: ValidateCodeActionModalProps) {
const themeStyles = useThemeStyles();
const safePaddingBottomStyle = useSafePaddingBottomStyle();
const firstRenderRef = useRef(true);
const validateCodeFormRef = useRef<ValidateCodeFormHandle>(null);

Expand Down Expand Up @@ -67,7 +69,7 @@ function ValidateCodeActionModal({
onBackButtonPress={hide}
/>

<View style={[themeStyles.ph5, themeStyles.mt3, themeStyles.mb7]}>
<View style={[themeStyles.ph5, themeStyles.mt3, themeStyles.mb7, themeStyles.flex1]}>
<Text style={[themeStyles.mb3]}>{description}</Text>
<ValidateCodeForm
validateCodeAction={validateCodeAction}
Expand All @@ -76,6 +78,7 @@ function ValidateCodeActionModal({
handleSubmitForm={handleSubmitForm}
sendValidateCode={sendValidateCode}
clearError={clearError}
buttonStyles={[themeStyles.justifyContentEnd, themeStyles.flex1, safePaddingBottomStyle]}
ref={validateCodeFormRef}
hasMagicCodeBeenSent={hasMagicCodeBeenSent}
/>
Expand Down
1 change: 1 addition & 0 deletions src/libs/API/parameters/ReportVirtualExpensifyCardFraudParams.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
type ReportVirtualExpensifyCardFraudParams = {
cardID: number;
validateCode: string;
};
export default ReportVirtualExpensifyCardFraudParams;
1 change: 1 addition & 0 deletions src/libs/API/parameters/RequestReplacementExpensifyCardParams.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
type RequestReplacementExpensifyCardParams = {
cardID: number;
reason: string;
validateCode: string;
};

export default RequestReplacementExpensifyCardParams;
8 changes: 4 additions & 4 deletions src/libs/API/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@ const WRITE_COMMANDS = {
CONNECT_BANK_ACCOUNT_MANUALLY: 'ConnectBankAccountManually',
VERIFY_IDENTITY_FOR_BANK_ACCOUNT: 'VerifyIdentityForBankAccount',
BANK_ACCOUNT_HANDLE_PLAID_ERROR: 'BankAccount_HandlePlaidError',
REPORT_VIRTUAL_EXPENSIFY_CARD_FRAUD: 'ReportVirtualExpensifyCardFraud',
REQUEST_REPLACEMENT_EXPENSIFY_CARD: 'RequestReplacementExpensifyCard',
ACTIVATE_PHYSICAL_EXPENSIFY_CARD: 'ActivatePhysicalExpensifyCard',
UPDATE_EXPENSIFY_CARD_LIMIT: 'UpdateExpensifyCardLimit',
UPDATE_EXPENSIFY_CARD_TITLE: 'UpdateExpensifyCardTitle',
Expand Down Expand Up @@ -453,8 +451,6 @@ type WriteCommandParameters = {
[WRITE_COMMANDS.CONNECT_BANK_ACCOUNT_MANUALLY]: Parameters.ConnectBankAccountParams;
[WRITE_COMMANDS.VERIFY_IDENTITY_FOR_BANK_ACCOUNT]: Parameters.VerifyIdentityForBankAccountParams;
[WRITE_COMMANDS.BANK_ACCOUNT_HANDLE_PLAID_ERROR]: Parameters.BankAccountHandlePlaidErrorParams;
[WRITE_COMMANDS.REPORT_VIRTUAL_EXPENSIFY_CARD_FRAUD]: Parameters.ReportVirtualExpensifyCardFraudParams;
[WRITE_COMMANDS.REQUEST_REPLACEMENT_EXPENSIFY_CARD]: Parameters.RequestReplacementExpensifyCardParams;
[WRITE_COMMANDS.ACTIVATE_PHYSICAL_EXPENSIFY_CARD]: Parameters.ActivatePhysicalExpensifyCardParams;
[WRITE_COMMANDS.UPDATE_EXPENSIFY_CARD_LIMIT]: Parameters.UpdateExpensifyCardLimitParams;
[WRITE_COMMANDS.UPDATE_EXPENSIFY_CARD_TITLE]: Parameters.UpdateExpensifyCardTitleParams;
Expand Down Expand Up @@ -1010,6 +1006,8 @@ const SIDE_EFFECT_REQUEST_COMMANDS = {
RECONNECT_APP: 'ReconnectApp',
ADD_PAYMENT_CARD_GBP: 'AddPaymentCardGBP',
REVEAL_EXPENSIFY_CARD_DETAILS: 'RevealExpensifyCardDetails',
REPORT_VIRTUAL_EXPENSIFY_CARD_FRAUD: 'ReportVirtualExpensifyCardFraud',
REQUEST_REPLACEMENT_EXPENSIFY_CARD: 'RequestReplacementExpensifyCard',
TWO_FACTOR_AUTH_VALIDATE: 'TwoFactorAuth_Validate',
CONNECT_AS_DELEGATE: 'ConnectAsDelegate',
DISCONNECT_AS_DELEGATE: 'DisconnectAsDelegate',
Expand All @@ -1024,6 +1022,8 @@ type SideEffectRequestCommandParameters = {
[SIDE_EFFECT_REQUEST_COMMANDS.OPEN_REPORT]: Parameters.OpenReportParams;
[SIDE_EFFECT_REQUEST_COMMANDS.OPEN_OLD_DOT_LINK]: Parameters.OpenOldDotLinkParams;
[SIDE_EFFECT_REQUEST_COMMANDS.REVEAL_EXPENSIFY_CARD_DETAILS]: Parameters.RevealExpensifyCardDetailsParams;
[SIDE_EFFECT_REQUEST_COMMANDS.REPORT_VIRTUAL_EXPENSIFY_CARD_FRAUD]: Parameters.ReportVirtualExpensifyCardFraudParams;
[SIDE_EFFECT_REQUEST_COMMANDS.REQUEST_REPLACEMENT_EXPENSIFY_CARD]: Parameters.RequestReplacementExpensifyCardParams;
[SIDE_EFFECT_REQUEST_COMMANDS.GET_MISSING_ONYX_MESSAGES]: Parameters.GetMissingOnyxMessagesParams;
[SIDE_EFFECT_REQUEST_COMMANDS.JOIN_POLICY_VIA_INVITE_LINK]: Parameters.JoinPolicyInviteLinkParams;
[SIDE_EFFECT_REQUEST_COMMANDS.RECONNECT_APP]: Parameters.ReconnectAppParams;
Expand Down
147 changes: 80 additions & 67 deletions src/libs/actions/Card.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import * as PolicyUtils from '@libs/PolicyUtils';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {Card, CompanyCardFeed} from '@src/types/onyx';
import type {CardLimitType, ExpensifyCardDetails, IssueNewCardData, IssueNewCardStep} from '@src/types/onyx/Card';
import type {CardLimitType, ExpensifyCardDetails, ExpensifyCardID, IssueNewCardData, IssueNewCardStep} from '@src/types/onyx/Card';
import type {ConnectionName} from '@src/types/onyx/Policy';

type ReplacementReason = 'damaged' | 'stolen';
Expand All @@ -37,45 +37,52 @@ type IssueNewCardFlowData = {
data?: Partial<IssueNewCardData>;
};

function reportVirtualExpensifyCardFraud(cardID: number) {
const optimisticData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.FORMS.REPORT_VIRTUAL_CARD_FRAUD,
value: {
isLoading: true,
function reportVirtualExpensifyCardFraud(cardID: number, validateCode: string): Promise<string> {
return new Promise((resolve, reject) => {
const optimisticData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.ACCOUNT,
value: {
isLoading: true,
},
},
},
];
];

const successData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.FORMS.REPORT_VIRTUAL_CARD_FRAUD,
value: {
isLoading: false,
const successData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.ACCOUNT,
value: {
isLoading: false,
},
},
},
];
];

const failureData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.FORMS.REPORT_VIRTUAL_CARD_FRAUD,
value: {
isLoading: false,
const failureData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.ACCOUNT,
value: {
isLoading: false,
},
},
},
];
];

const parameters: ReportVirtualExpensifyCardFraudParams = {
cardID,
};
const parameters: ReportVirtualExpensifyCardFraudParams = {
cardID,
validateCode,
};

API.write(WRITE_COMMANDS.REPORT_VIRTUAL_EXPENSIFY_CARD_FRAUD, parameters, {
optimisticData,
successData,
failureData,
// eslint-disable-next-line rulesdir/no-api-side-effects-method
API.makeRequestWithSideEffects(SIDE_EFFECT_REQUEST_COMMANDS.REPORT_VIRTUAL_EXPENSIFY_CARD_FRAUD, parameters, {optimisticData, successData, failureData})
.then((response): string | undefined => {
if (response?.jsonCode !== CONST.JSON_CODE.SUCCESS) {
return;
}
resolve((response as ExpensifyCardID).newCardID.toString());
})
.catch(() => reject());
});
}

Expand All @@ -84,47 +91,53 @@ function reportVirtualExpensifyCardFraud(cardID: number) {
* @param cardID - id of the card that is going to be replaced
* @param reason - reason for replacement
*/
function requestReplacementExpensifyCard(cardID: number, reason: ReplacementReason) {
const optimisticData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.FORMS.REPORT_PHYSICAL_CARD_FORM,
value: {
isLoading: true,
errors: null,
function requestReplacementExpensifyCard(cardID: number, reason: ReplacementReason, validateCode: string): Promise<string> {
return new Promise((resolve, reject) => {
const optimisticData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.ACCOUNT,
value: {
isLoading: true,
},
},
},
];
];

const successData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.FORMS.REPORT_PHYSICAL_CARD_FORM,
value: {
isLoading: false,
const successData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.ACCOUNT,
value: {
isLoading: false,
},
},
},
];
];

const failureData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.FORMS.REPORT_PHYSICAL_CARD_FORM,
value: {
isLoading: false,
const failureData: OnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.ACCOUNT,
value: {
isLoading: false,
},
},
},
];
];

const parameters: RequestReplacementExpensifyCardParams = {
cardID,
reason,
};
const parameters: RequestReplacementExpensifyCardParams = {
cardID,
reason,
validateCode,
};

API.write(WRITE_COMMANDS.REQUEST_REPLACEMENT_EXPENSIFY_CARD, parameters, {
optimisticData,
successData,
failureData,
// eslint-disable-next-line rulesdir/no-api-side-effects-method
API.makeRequestWithSideEffects(SIDE_EFFECT_REQUEST_COMMANDS.REQUEST_REPLACEMENT_EXPENSIFY_CARD, parameters, {optimisticData, successData, failureData})
.then((response): string | undefined => {
if (response?.jsonCode !== CONST.JSON_CODE.SUCCESS) {
return;
}
resolve((response as ExpensifyCardID).newCardID.toString());
})
.catch(() => reject());
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning promises from action methods is a discouraged pattern - it looks like we can do this without the side effects.

We just want the new cardID here, so we could probably just have it update an onyx key and only re-direct if we have that key present and are done loading. Alternatively, it seems like we didn't have a redirect before so maybe we don't need to have it still

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thienlnam without the side effects we used to redirect to the old card page and we display a "$0" limit which looks like a bug to the user

Screen_Recording_2024-10-23_at_22.04.03-2024-10-30.17_36_55.334.mov

We also use side effects for reveal virtual card details here

App/src/libs/actions/Card.ts

Line 197 in 084b104

function revealVirtualCardDetails(cardID: number, validateCode: string): Promise<ExpensifyCardDetails> {
. We also already display a loading spinner when requesting a new card.

Copy link
Contributor

@thienlnam thienlnam Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use side effects there because we can't store those details in onyx since they're sensitive - but for this which is just a cardID is fine to store in onyx

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm let me see how I can make this work without side effects and also prevent the bug above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 lmk if you need some ideas - usually the way we get around this is by adding a loading indicator on a loading key on the onyx key for the page

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this, wdyt?

Screenshot 2024-11-01 at 18 40 40

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thienlnam IMO I think we can keep the side effect call here, you can see in the comment that our use case here is allowed, we are both calling Marqeta and redirecting based on the response

Screenshot 2024-11-01 at 19 10 59

Also given that we display a spinner and make the user wait, I don't see how is using the optimistic pattern is helpful?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup something along those lines would work - you'd need to have the API command send an onyx update for the newCardID if you wanted to redirect to the new card.

Though thinking about this more though, if you only added the redirect so that the card doesn't show with a $0 limit we could also just not do the redirect because then it forces this flow to happen online.

In App, in the optimistic data you could set a new key called isReplacing or something on the cardID and so when you open that cardID you would see some note that it's been replaced and then when the API completes it would remove the card and have the new card in the list. That's probably easier and then allows the user to take other actions offline with the command queued - what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hear you. However I think that this stuff is out of scope, so it's best if I kept this PR focused on validateCode using the same offline pattern we have now, which means not fixing the $0 bug, I think we can open an issue to fix separately. Sounds good?

Copy link
Contributor

@thienlnam thienlnam Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that works for me - the main concern for me here is to remove the side effect so if we can do that here and have an external contributor handle the UI update seperately


Expand Down
Loading
Loading