add “Report virtual card fraud” page and route #28312

pasyukevich · 2023-09-27T11:27:30Z

Details

Fixed Issues

Tests

Add useEffect with test data to the InitialSettingsPage

 useEffect(() => {
        window.Onyx.merge(`cardList`, {
            'virtual': {
                cardID: 234523452345,
                state: 3,
                bank: 'Expensify',
                availableSpend:10000,
        domainName: 'Expensify',
        lastFourPAN: '',
        isVirtual: true,
        cardholderFirstName: "Test",
        cardholderLastName: "Test",
            }, 'physical': {
                cardID: 234523452345,
                state: 3,
                bank: '1000',
                availableSpend:10000,
        domainName: 'Expensify',
        lastFourPAN: '2345',
        isVirtual: false,
            },
        });

        Navigation.navigate(ROUTES.getWalletCardRoute('expensify'));
    }, []);

Press report fraud
Observe redirect to the next page
Press deactivate button
Verify that no errors appear in the JS console

Offline tests

Observe that the 'Deactivate card' button is inactive

QA Steps

Log in with an account that has an active virtual card
Navigate to the wallet page /settings/wallet and wait for all API commands to finish
Navigate directly to /settings/wallet/cards/expensify.com/
Verify the UI looks correct
Tap the "Report virtual card fraud" button
Verify you are redirected to the page to report fraud
Tap "Deactivate card"
Verify that you are redirected back to the Expensify Card page
And a new card should have been issued.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

android.web.webm

Mobile Web - Safari

ios-web.mp4

Desktop

desktop.app.mov

iOS

ios.native.mp4

Android

android.native.webm

src/pages/settings/Wallet/ReportFraudPage.js

src/libs/actions/Card.js

melvin-bot · 2023-09-28T12:41:16Z

@s77rt Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

s77rt · 2023-09-29T01:27:16Z

src/pages/settings/Wallet/ReportFraudPage.js

+    const submitReportFraud = () => {
+        Card.reportDigitalExpensifyCardFraud(virtualCard.cardID);
+    };
+
+    const onBackButtonPress = () => Navigation.goBack(ROUTES.SETTINGS_WALLET_DOMAINCARDS.getRoute(domain));


NAB. Let's inline those functions

Updated, let me know if that's what you meant

I meant to pass the function directly to HeaderWithBackButton

onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS_WALLET_DOMAINCARDS.getRoute(domain))}

s77rt · 2023-09-29T01:49:29Z

src/pages/settings/Wallet/ReportFraudPage.js

+            return;
+        }
+
+        FormActions.setErrors(ONYXKEYS.FORMS.REPORT_FRAUD_FORM, virtualCard.errors);


I don't think this is a pattern that we follow. The backend errors are not form errors

+1 the errors should either come from the backend (in that case they are already there on the virtualCard object) or the form itself should set them. In this case though, we have no form fields so there is no validation that could fail.

Basically the options are:

Use the FormAlertWithSubmitButton directly (I think this might be the way I would go here) and then pass the errors that are on the card object.

Use the Form - but don't manually set the errors. Just trust that the backend will set them if anything goes wrong.

All right, so let's go with number 1! What exactly do you mean by "pass the errors that are on the card object."?

I think I get it now, let me know if that's what you meant 😄 CC: @marcaaron

s77rt · 2023-09-29T01:51:22Z

This PR does not seem to be ready for test yet. The backend changes are not live yet, I'm getting 404 errors trying to access /api?command=ReportDigitalExpensifyCardFraud

pasyukevich · 2023-09-29T08:23:27Z

@s77rt I'm not sure that we need to wait BE

pasyukevich · 2023-09-29T08:30:13Z

@marcaaron can you confirm this and take a look at the Error handling?

JKobrynski · 2023-10-03T12:08:18Z

I am not the owner of this PR, so I can't edit the description, but I think the useEffect step described above won't work anymore, what worked for me was this:

useEffect(() => {
    window.Onyx.merge(`cardList`, {
        'virtual': {
            cardID: 234523452345,
            state: 3,
            bank: 'Expensify',
            availableSpend:10000,
    domainName: 'Expensify',
    lastFourPAN: '',
    isVirtual: true,
    cardholderFirstName: "Test",
    cardholderLastName: "Test",
        }, 'physical': {
            cardID: 234523452345,
            state: 3,
            bank: '1000',
            availableSpend:10000,
    domainName: 'Expensify',
    lastFourPAN: '2345',
    isVirtual: false,
        },
    });

    Navigation.navigate(ROUTES.SETTINGS_WALLET_DOMAINCARDS.getRoute('expensify'));
}, []);

marcaaron

Overall this is looking great. I don't have many comments, but will ask for some annoying name changes.

marcaaron · 2023-10-04T00:20:13Z

src/libs/Navigation/AppNavigator/ModalStackNavigators.js

@@ -141,6 +141,7 @@ const SettingsModalStackNavigator = createModalStackNavigator({
    Settings_Lounge_Access: () => require('../../../pages/settings/Profile/LoungeAccessPage').default,
    Settings_Wallet: () => require('../../../pages/settings/Wallet/WalletPage').default,
    Settings_Wallet_DomainCards: () => require('../../../pages/settings/Wallet/ExpensifyCardPage').default,
+    Settings_Report_Fraud: () => require('../../../pages/settings/Wallet/ReportFraudPage').default,


Could we make this Settings_Wallet_ReportVirtualCardFraud for consistency?

Sure, done!

marcaaron · 2023-10-04T00:26:35Z

src/libs/actions/Card.js

+/**
+ * @param {Number} cardID
+ */
+function reportDigitalExpensifyCardFraud(cardID) {


If I am not mistaken, we decided to rebrand these as "virtual cards". Could we update that for consistency? So...

reportDigitalExpensifyCardFraud -> reportVirtualExpensifyCardFraud

ReportDigitalExpensifyCardFraud -> ReportVirtualExpensifyCardFraud

report-digital-fraud -> report-virtual-fraud

etc

marcaaron · 2023-10-04T00:27:26Z

src/ONYXKEYS.ts

@@ -291,6 +291,7 @@ const ONYXKEYS = {
        PRIVATE_NOTES_FORM: 'privateNotesForm',
        I_KNOW_A_TEACHER_FORM: 'iKnowTeacherForm',
        INTRO_SCHOOL_PRINCIPAL_FORM: 'introSchoolPrincipalForm',
+        REPORT_FRAUD_FORM: 'reportFraudForm',


Maybe also could make this: REPORT_VIRTUAL_CARD_FRAUD? Not sure if we will have other kinds of fraud reporting, but doesn't hurt to get specific here.

marcaaron · 2023-10-04T00:43:17Z

src/pages/settings/Wallet/ReportFraudPage.js

+            return;
+        }
+
+        FormActions.setErrors(ONYXKEYS.FORMS.REPORT_FRAUD_FORM, virtualCard.errors);


+1 the errors should either come from the backend (in that case they are already there on the virtualCard object) or the form itself should set them. In this case though, we have no form fields so there is no validation that could fail.

marcaaron · 2023-10-04T01:00:51Z

btw tested locally with a real virtual card and it's looking nice..

s77rt · 2023-10-04T12:14:12Z

I'm not sure I can test this well. Request to /api?command=ReportDigitalExpensifyCardFraud fail with 404 and is stuck in persisted requests. Don't we need to wait for backend first? cc @marcaaron

JKobrynski · 2023-10-04T14:03:49Z

I pushed a couple commits, I think all comments have been addressed in some way, let me know if there is anything else you'd like me to change!

CC: @marcaaron @s77rt

JKobrynski · 2023-10-06T12:15:37Z

PR updated

CC: @marcaaron @s77rt

marcaaron · 2023-10-06T23:58:58Z

@marcaaron I can initiate the deactivate card request (after setting Onyx as in the PR description). But once I do (and click the submit button), all I would see is an infinite loader due to the request not getting completed.

@s77rt Test what you can. Like I said, I will verify that part.

s77rt · 2023-10-07T14:24:38Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

mweb-chrome.mov

Mobile Web - Safari

mweb-safari.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

marcaaron · 2023-10-10T23:29:59Z

I think this one looks ready to merge now. As for testing... this isn't something that QA will be able to test as the flows aren't really accessible and I am not sure if they have Expensify Cards.

I updated the test steps and I think either myself or @grgia should be able to do them on staging.

OSBotify · 2023-10-10T23:38:38Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2023-10-11T05:07:47Z

🚀 Deployed to staging by https://github.com/marcaaron in version: 1.3.81-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

marcaaron · 2023-10-11T23:59:26Z

This pretty much failed the internal QA for me, but since this flow isn't really "accessible" yet I think we can deploy anyway and continue testing as we go and create some follow up issues. We will want to E2E test everything before we release the changes to the wallet page.

cc @grgia but this is what I ran into:

Able to access the Expensify card page
Navigating to the report virtual card page works
Request to deactivate the card
Doesn't work and fails with these weird things

Expired physical card case

First interesting thing is that it says I have no physical card. This is correct, mine has expired 😅

@kevinksullivan @joekaufmanexpensify what should the behavior be in this case?

Error message says MISSING TRANSLATION

This comes from here. So it means we are taking the error message and assuming it is a translation key. Maybe something with this logic is off @fedirjh. Because it would not throw on staging if I am looking at it correctly 🤔

App/src/libs/Localize/index.js

Lines 118 to 137 in cb883e8

    
           function translateIfPhraseKey(message) { 
        
               if (_.isEmpty(message)) { 
        
                   return ''; 
        
               } 
        
               try { 
        
                   // check if error message has a variable parameter 
        
                   const [phrase, variables] = _.isArray(message) ? message : [message]; 
        
                   // This condition checks if the error is already translated. For example, if there are multiple errors per input, we handle translation in ErrorUtils.addErrorMessage due to the inability to concatenate error keys. 
        
                   if (variables && variables.isTranslated) { 
        
                       return phrase; 
        
                   } 
        
                   return translateLocal(phrase, variables); 
        
               } catch (error) { 
        
                   return message; 
        
               } 
        
           }

I think on production this error would end up showing to the user Auth RequestReplacementExpensifyCard returned an error which we probably don't want.

But what do we want to say here? cc @kevinksullivan & @joekaufmanexpensify again here as it probably depends on what we decide on first issue.

Separately, maybe someone with a physical + virtual card is daring enough to try this on NewDot staging?

OSBotify · 2023-10-12T04:49:11Z

🚀 Deployed to staging by https://github.com/marcaaron in version: 1.3.83-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

fedirjh · 2023-10-12T17:25:50Z

Because it would not throw on staging if I am looking at it correctly

@marcaaron I believe this is a regression from #28377, which appears to have altered the error-handling mechanism previously in use.

I think on production this error would end up showing to the user Auth RequestReplacementExpensifyCard returned an error which we probably don't want.

I expect that production will behave similarly to staging and also display MISSING TRANSLATION.

I think prior to that PR it will display the server error: Auth RequestReplacementExpensifyCard returned an error which we probably don't want.

marcaaron · 2023-10-12T19:58:56Z

I expect that production will behave similarly to staging and also display MISSING TRANSLATION.

Hmm ok I am not really sure what the expected behavior is supposed to be here. Pretty confused on those changes overall actually. Maybe @mountiny can explain what we should do.

mountiny · 2023-10-13T16:11:32Z

This is bug in the form error, we are passing the message which wants to be translated into translate method which we should not be doing. That will be handled as a fix of the form component, dont have link right now, sorry

marcaaron · 2023-10-13T18:50:47Z

ah so all error messages from the backend that the Form component was previously displaying are broken atm?

mountiny · 2023-10-15T12:15:14Z

ah so all error messages from the backend that the Form component was previously displaying are broken atm?

If they are being passed as what should be a translate key to translate method, then yeah, this feels like incorrect usage of the translate utility.

If we want to keep this behaviour in Form, ie get translated error if the key exist.

Other option which I dont think we really want to do now, would be to catch the error in PHP and then pass down the translation key and have a nice and translated error.

mountiny · 2023-10-15T12:18:59Z

This was added as a futureproof concept for violations, which will be passed from a server and should be translated so the MISSING TRANSLATIONS is shown Only to Expensify emails account https://github.com/Expensify/App/pull/28377/files#diff-2f79c1e80b9fe28f0d8f5487095f8219dce80539b231f41190e509b8d5d3f8d4R93

So cutomers should still just see the Error from the server @marcaaron

marcaaron · 2023-10-16T20:38:59Z

Interesting. I missed that convo. Still not sure what it means. So if someone with an "expensify email" sees the MISSING TRANSLATIONS then they should do what exactly? 😅

mountiny · 2023-10-16T21:38:04Z

It should just be easier to spot that there is a bug and raise it as a bug 😄 we did not really expect this would be happening until violations come in wave9 but the current usage of the form validation error does not look ideal

pasyukevich force-pushed the feature/report-fraud branch 4 times, most recently from 1be03aa to 8cbb012 Compare September 27, 2023 11:32

fabioh8010 reviewed Sep 27, 2023

View reviewed changes

pasyukevich force-pushed the feature/report-fraud branch 2 times, most recently from 7533f27 to 1441d4c Compare September 28, 2023 11:13

add “Report virtual card fraud” page and route

0274f11

pasyukevich force-pushed the feature/report-fraud branch from 1441d4c to 0274f11 Compare September 28, 2023 12:30

pasyukevich marked this pull request as ready for review September 28, 2023 12:41

pasyukevich requested a review from a team as a code owner September 28, 2023 12:41

melvin-bot bot requested review from s77rt and removed request for a team September 28, 2023 12:41

s77rt suggested changes Sep 29, 2023

View reviewed changes

grgia self-requested a review September 29, 2023 09:22

marcaaron requested changes Oct 4, 2023

View reviewed changes

inline function

46ecfe1

JKobrynski added 5 commits October 4, 2023 14:16

inline function correctly

c6f6791

replace Form with FormAlertWithSubmitButton

716775c

rename route

a40ddf6

rename digital to virtual

48528c9

rename onyx key

963509f

JKobrynski mentioned this pull request Oct 6, 2023

Add report card lost or damaged #28141

Merged

59 tasks

JKobrynski added 2 commits October 6, 2023 14:07

apply minor changes

f6a4166

fix linting errors, remove unnecessary prop

fd13bba

s77rt approved these changes Oct 7, 2023

View reviewed changes

JKobrynski added 2 commits October 10, 2023 16:07

Merge branch 'main' into feature/report-fraud

04ede94

fix linting

00666f5

marcaaron added the InternalQA This pull request required internal QA label Oct 10, 2023

marcaaron self-requested a review October 10, 2023 23:30

marcaaron approved these changes Oct 10, 2023

View reviewed changes

marcaaron merged commit cb883e8 into Expensify:main Oct 10, 2023
12 checks passed

OSBotify mentioned this pull request Oct 11, 2023

Deploy Checklist: New Expensify 2023-10-11 #29252

Closed

OSBotify mentioned this pull request Oct 12, 2023

Deploy Checklist: New Expensify 2023-10-12 #29417

Closed

97 tasks

marcaaron mentioned this pull request Oct 12, 2023

[NoQA] Safely catch translation key when it's wrong or missing #28377

Merged

59 tasks

melvin-bot bot mentioned this pull request Oct 13, 2023

[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [ECard Settings] - Add “Report virtual card fraud” page and route #22880

Closed

add “Report virtual card fraud” page and route #28312

add “Report virtual card fraud” page and route #28312

Conversation

pasyukevich commented Sep 27, 2023 • edited by marcaaron Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Sep 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s77rt commented Sep 29, 2023

pasyukevich commented Sep 29, 2023 • edited Loading

pasyukevich commented Sep 29, 2023

JKobrynski commented Oct 3, 2023

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented Oct 4, 2023

s77rt commented Oct 4, 2023

JKobrynski commented Oct 4, 2023

JKobrynski commented Oct 6, 2023

marcaaron commented Oct 6, 2023 • edited Loading

s77rt commented Oct 7, 2023

Reviewer Checklist

Screenshots/Videos

marcaaron commented Oct 10, 2023

OSBotify commented Oct 10, 2023

OSBotify commented Oct 11, 2023

marcaaron commented Oct 11, 2023

OSBotify commented Oct 12, 2023

fedirjh commented Oct 12, 2023

marcaaron commented Oct 12, 2023

mountiny commented Oct 13, 2023

marcaaron commented Oct 13, 2023

mountiny commented Oct 15, 2023

mountiny commented Oct 15, 2023

marcaaron commented Oct 16, 2023

mountiny commented Oct 16, 2023

pasyukevich commented Sep 27, 2023 •

edited by marcaaron

Loading

pasyukevich commented Sep 29, 2023 •

edited

Loading

marcaaron commented Oct 6, 2023 •

edited

Loading