[$1000] Not showing loader on add bank account page

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Open settings -> Payments.
2. Press on Add payment method -> Bank account.
3. Observe loader is not display.

Expected Result:

Loader should be display until preparing Plaid link.

Actual Result:

Loader not showing on add bank account

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.2.56-0
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

no-loader.1.mov

Recording.1334.mp4

Expensify/Expensify Issue URL:
Issue reported by: @jatinsonijs
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1674055957380549

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017a4fe2b93e051102
• Upwork Job ID: 1616549263858544640
• Last Price Increase: 2023-01-20

[arielgreen]

Bug0 Triage Checklist

Note: see this SO for more information.

• The "bug" is actually a bug
• The bug is not a duplicate report of an existing GH.
  • If it is, close the GH and add any novel details to the original GH instead
• The bug is reproducible, following the reproduction steps.
  • [ ] If the GH doesn’t have steps to reliably reproduce the bug and you figure it out, then please add them.
  • [ ] If you’re unable to reproduce the bug, add the Needs reproduction label.
  • [ ] Comment on the issue outlining the steps you took to try to reproduce the bug, your results and tag the issue reporter and the Applause QA member who created the issue. Ask them to clarify reproduction steps and/or to check the reproduction steps again. Close issue.
• The GH template is filled out as fully as possible
  • The GH body and title are clear (ie. could an external contributor understand it and work on it?)
• The GH was created by an Expensify employee or a QA tester
• [ ] If the bug is an OldDot issue, you should decide whether it is widespread enough to justify fixing or it is better to wait for reunification. If it’s better to wait, close the GH & provide this justification
• If there's a link to Slack, check the discussion to see if we decided not to fix it
• Decide if the GH should be resolved by an External contributor or Internal engineer, add the appropriate label

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~017a4fe2b93e051102

[melvin-bot]

Current assignee @arielgreen is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)

[melvin-bot]

Triggered auto assignment to @deetergp (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[abdulrahuman5196]

Proposal
Might be regression from #14264
As we don't check if the token is present or not, we just end up not showing the loader in the UI. We would be required to check if the token is present without any errors to avoid recreating the issue the above PR intended to solve and to solve this loader not showing issue as well.

--- a/src/components/AddPlaidBankAccount.js
+++ b/src/components/AddPlaidBankAccount.js
@@ -115,7 +115,7 @@ class AddPlaidBankAccount extends React.Component {
         if (!plaidBankAccounts.length) {
             return (
                 <FullPageOfflineBlockingView>
-                    {lodashGet(this.props.plaidData, 'isLoading') && (
+                    {(lodashGet(this.props.plaidData, 'isLoading') || (!token && !plaidDataErrorMessage)) && (
                         <View style={[styles.flex1, styles.alignItemsCenter, styles.justifyContentCenter]}>
                             <ActivityIndicator color={themeColors.spinner} size="large" />
                         </View>

We can also use lodash function on the above.

[aimane-chnaif]

We removed !token condition in #14264 in favor of #13236

I think this should be put on HOLD until #13236 is deployed to staging/production. Not reproducible in main.

• isLoading is set to true when call OpenPlaidBankLogin on AddPlaidBankAccount component mount
  https://github.com/Expensify/App/pull/13236/files#diff-bf2d7acc015c7abd6aa970a11d183001b238e66806abbcaaa6d010ce4439cfd6R19
• and !token is completely removed.

We can retest after #13236 deployed and close this issue if not reproducible.

cc: @ctkochan22 @nkuoch

[abdulrahuman5196]

Yup @aimane-chnaif, saw now that #13236 got merged only 3 hours ago. With latest main the issue is not reproducible.

[ctkochan22]

This should be fixed with the noted PRs. Can we close?

[Pujan92]

Proposal

Issue: On the read api call OpenPlaidBankLogin we don't set the loading state which seems to be issue here.

Soltuion: We need to update the plaidData isLoading onyx state on the read call OpenPlaidBankLogin which fetches the plaid token.

diff --git a/src/libs/actions/Plaid.js b/src/libs/actions/Plaid.js
index 393b5bbc9..ac463b6d0 100644
--- a/src/libs/actions/Plaid.js
+++ b/src/libs/actions/Plaid.js
@@ -12,7 +12,29 @@ function openPlaidBankLogin(allowDebit, bankAccountID) {
     const params = getPlaidLinkTokenParameters();
     params.allowDebit = allowDebit;
     params.bankAccountID = bankAccountID;
-    API.read('OpenPlaidBankLogin', params);
+    API.read('OpenPlaidBankLogin', params, {
+        optimisticData: [{
+            onyxMethod: CONST.ONYX.METHOD.MERGE,
+            key: ONYXKEYS.PLAID_DATA,
+            value: {
+                isLoading: true,
+            },
+        }],
+        successData: [{
+            onyxMethod: CONST.ONYX.METHOD.MERGE,
+            key: ONYXKEYS.PLAID_DATA,
+            value: {
+                isLoading: false,
+            },
+        }],
+        failureData: [{
+            onyxMethod: CONST.ONYX.METHOD.MERGE,
+            key: ONYXKEYS.PLAID_DATA,
+            value: {
+                isLoading: false,
+            },
+        }],
+    });
 }

Also like other pages(IOUDetails, ...), we can use FullScreenLoadingIndicator instead ActivityIndicator here.

diff --git a/src/components/AddPlaidBankAccount.js b/src/components/AddPlaidBankAccount.js
index 386eea4c3..3a6c9b557 100644
--- a/src/components/AddPlaidBankAccount.js
+++ b/src/components/AddPlaidBankAccount.js
@@ -120,8 +122,8 @@ class AddPlaidBankAccount extends React.Component {
             return (
                 <FullPageOfflineBlockingView>
                     {this.props.plaidData.isLoading && (
-                        <View style={[styles.flex1, styles.alignItemsCenter, styles.justifyContentCenter]}>
-                            <ActivityIndicator color={themeColors.spinner} size="large" />
+                        <View style={[styles.flex1]}>
+                            <FullScreenLoadingIndicator />

---

Other solution will be w.r.t to onyx key plaidLinkToken which we receives from backend, we need to modify the structure for onyx plaidLinkToken to object from string, which has 2 properties(1 for token, 1 for loading) and the same structure needs to receive from the api response with Merge operation.

[jatinsonijs]

@Pujan92 It is fixed by #13236, plz pull latest code and recheck

[Pujan92]

@jatinsonijs Thanks, I pulled and now it is not reproducible.

@aimane-chnaif @ctkochan22 Isn't it good practice to update the isLoading state on success and failure case, otherwise in the case of failure it keeps spinning and won't show the error.
We are making it to true here with the ONYXKEYS.PLAID_DATA optimisticData.

App/src/libs/actions/Plaid.js

Lines 16 to 20 in 28aab08

	const optimisticData = [{
	onyxMethod: CONST.ONYX.METHOD.SET,
	key: ONYXKEYS.PLAID_DATA,
	value: {...PlaidDataProps.plaidDataDefaultProps, isLoading: true},
	}, {

diff --git a/src/libs/actions/Plaid.js b/src/libs/actions/Plaid.js
index 0785a1fd9..76bbd2e0f 100644
--- a/src/libs/actions/Plaid.js
+++ b/src/libs/actions/Plaid.js
@@ -13,23 +13,42 @@ function openPlaidBankLogin(allowDebit, bankAccountID) {
     const params = getPlaidLinkTokenParameters();
     params.allowDebit = allowDebit;
     params.bankAccountID = bankAccountID;
-    const optimisticData = [{
-        onyxMethod: CONST.ONYX.METHOD.SET,
-        key: ONYXKEYS.PLAID_DATA,
-        value: {...PlaidDataProps.plaidDataDefaultProps, isLoading: true},
-    }, {
-        onyxMethod: CONST.ONYX.METHOD.SET,
-        key: ONYXKEYS.PLAID_LINK_TOKEN,
-        value: '',
-    }, {
-        onyxMethod: CONST.ONYX.METHOD.MERGE,
-        key: ONYXKEYS.REIMBURSEMENT_ACCOUNT_DRAFT,
-        value: {
-            plaidAccountID: '',
-        },
-    }];
 
-    API.read('OpenPlaidBankLogin', params, {optimisticData});
+    API.read('OpenPlaidBankLogin', params, {
+        optimisticData: [
+            {
+                onyxMethod: CONST.ONYX.METHOD.SET,
+                key: ONYXKEYS.PLAID_DATA,
+                value: {...PlaidDataProps.plaidDataDefaultProps, isLoading: true},
+            },
+        ],
+        successData: [
+            {
+                onyxMethod: CONST.ONYX.METHOD.MERGE,
+                key: ONYXKEYS.PLAID_LINK_TOKEN,
+                value: '',
+            },
+            {
+                onyxMethod: CONST.ONYX.METHOD.MERGE,
+                key: ONYXKEYS.PLAID_DATA,
+                value: {...PlaidDataProps.plaidDataDefaultProps},
+            },
+        ],
+        failureData: [
+            {
+                onyxMethod: CONST.ONYX.METHOD.MERGE,
+                key: ONYXKEYS.REIMBURSEMENT_ACCOUNT_DRAFT,
+                value: {
+                    plaidAccountID: '',
+                },
+            },
+            {
+                onyxMethod: CONST.ONYX.METHOD.MERGE,
+                key: ONYXKEYS.PLAID_DATA,
+                value: {...PlaidDataProps.plaidDataDefaultProps, errors: {['Error Message']}},
+            },
+        ],
+    });
 }

Issue with the hardcoded change for demo which restricts plaid redirection as added && false within PlaidLink

Screen.Recording.2023-01-21.at.2.36.08.PM.mp4

[eVoloshchak]

Not overdue, seems like the issue has been fixed by #13236, since it isn't reproducible anymore.

[arielgreen]

Closing since this is now resolved.

[Pujan92]

@eVoloshchak @ctkochan22 Can you plz check my comment as it says loader keeps on showing in case of the error.

[eVoloshchak]

@Pujan92, I've seen it, I think this is something worth reporting on Slack. It's different from our issue, since in your case spinner will be showing when it shouldn't