Fetch the bank account for reimburse expenses with offline and loading support #13368

neil-marcellini · 2022-12-06T16:48:30Z

cc @robertjchen @yuwenmemon @arosiclair since they are reviewing the corresponding Web PR https://github.com/Expensify/Web-Expensify/pull/35727

Details

Previously, if you added a reimbursement account, signed out, signed back in and then navigated to a workspace's "Reimburse expenses" page, the page would show that you need to connect a bank account. The problem is that we did not fetch the reimbursement account on that page so the data was stale.

I have modified Policy.openWorkspaceReimburseView to fetch the reimbursement account and to set it as loading while the request is in progress. I debounced updates to the loading state so that the loading spinner will not flash onto the screen briefly. I added a new component WorkspaceReimburseSection that will show a loading spinner, or a section indicating that the feature is not available while offline, or the appropriate reimburse expenses section depending on the reimbursement account state.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/244855
PROPOSAL: N/A

Tests

If testing locally, first check out the branch from the Web-Expensify PR, neil-fix-hasVBA.

Log in with any account that does not have a reimbursement account
Create a workspace (if needed)
Go to Settings > any workspace > Reimburse expenses
Verify that you see a brief loading spinner and then a section at the bottom which says "Unlock next-day reimbursements" and has a button to connect a bank account
Go back and then go to Reimburse expenses again
Verify that you see a brief loading spinner and then a section at the bottom which says "Unlock next-day reimbursements" and has a button to connect a bank account
Click to connect a bank account and verify that you are taken to a page where you can connect manually or with Plaid.
Sign out and sign in with an account that is set up with a reimbursement account following the instructions in this SO.
Go to Settings > any workspace > Reimburse expenses
Verify that you see a brief loading spinner and then a section at the bottom which says "Fast reimbursements = happy members!".
Go back and then go to Reimburse expenses again
Verify that you see a brief loading spinner and then a section at the bottom which says "Fast reimbursements = happy members!".

Verify that no errors appear in the JS console

Offline tests

Log in with any account
Create a workspace (if needed)
Set up a reimbursement account following the instructions in this SO (if needed).
Go to Settings > any workspace > Reimburse expenses
Verify that you see a brief loading spinner and then a section at the bottom which says "Fast reimbursements = happy members!".
Go offline
Verify that you see a section at the bottom which says "Reimburse expenses" and says that it's not available while offline
Go back online
Verify that you see a brief loading spinner and then a section at the bottom which says "Fast reimbursements = happy members!".

QA Steps

Same as the tests.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Online

web.mov

Offline

web-offline.mov

Mobile Web - Chrome

Online

android-Chrome.mov

Offline

android-Chrome-offline.mov

Mobile Web - Safari

Online

iOS-Safari.mp4

Offline

iOS-Safari-offline.mp4

Desktop

Online

desktop.mov

Offline

desktop-offline.mov

iOS

Online

iOS.mp4

Offline

iOS-offline.mp4

Android

Online

android.mov

Offline

android-offline.mov

neil-marcellini · 2022-12-07T22:49:58Z

I still need to test on all platforms, but I'm going to open this up for an initial review. @Expensify/design would you please take a look at the offline and loading states from the web testing video and let me know if that looks good?

melvin-bot · 2022-12-07T22:50:59Z

@aimane-chnaif @grgia One of you needs to 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]

shawnborton · 2022-12-07T23:33:04Z

I don't think we should be replacing the branded illustration for offline.

shawnborton · 2022-12-07T23:33:28Z

Rather, let's just keep the original illustration there and then display the offline message below the header + illustration

neil-marcellini · 2022-12-08T00:00:48Z

Rather, let's just keep the original illustration there and then display the offline message below the header + illustration

Ok I can do that, but which illustration should I use? The one for when you have a bank account connected or when it's not connected? I didn't want the illustration to give any indication of whether you had a bank account connected, since you are offline. Do we have an illustration for when you are offline?

arosiclair

Looks pretty good other than that isLoading flag

arosiclair · 2022-12-08T16:13:42Z

src/libs/actions/BankAccounts.js

+ * @return {Promise}
+ */
+function setReimbursementAccountLoading(isLoading) {
+    return Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {isLoading});


Instead of setting isLoading with this function we can just set it using optimisticData in the API command

At first I set it using the optimisticData, however that presented a problem when going offline and then back online on the reimburse expenses page. The isOffline prop updates to false before the API command is actually processed, which means that the page briefly shows stale data, and then a loading spinner, and then the correct data. Using this action I'm able to set isLoading right away.

Hmmmm... The only spot I see this function being used is right before we call openWorkspaceReimburseView - so how would using optimisticData in that API command not work? 🤔

I think it's because the Network request queue doesn't process the command right away. I'll make that change and record a video tomorrow, or you could try it if you want.

Oh hmmmm.... cause it's a read. So I guess we're getting blocked by a WRITE request that's hanging? Looks like that was added here: c653d1a

Oh interesting, good investigation. Given that, are you happy with the current approach?

I have no clue 😅 - maybe it's fixable or there's a way around it. I'll ask in Slack.

I added a comment to provide some more explanation.

shawnborton · 2022-12-08T19:35:45Z

Maybe an icon that better matches the "Reimburse receipts" text you are using? So something like this (which is already in the App repo):

neil-marcellini · 2022-12-08T19:37:04Z

Perfect I'll go ahead and use that, thanks.

neil-marcellini · 2022-12-08T23:59:35Z

Updated and ready for another review!

neil-marcellini · 2022-12-12T19:22:03Z

Ready for another review. The Web-PR has been merged so we only need to wait until it's deployed to production to merge this PR. @aimane-chnaif would you please fill out the PR reviewer checklist?

aimane-chnaif · 2022-12-12T19:23:21Z

Ready for another review. The Web-PR has been merged so we only need to wait until it's deployed to production to merge this PR. @aimane-chnaif would you please fill out the PR reviewer checklist?

Sure, I will review, test and complete checklist

aimane-chnaif · 2022-12-13T01:42:49Z

Been waiting for web dev error here gone but seems not fixed yet.
For now, tests done on iOS, android, desktop and adding videos only for them.

aimane-chnaif · 2022-12-13T01:48:04Z

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:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- 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 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 correct English and 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 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 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.
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

mchrome.mp4

Mobile Web - Safari

msafari.mp4

Desktop

desktop-old.mov

iOS

ios.mp4

Android

android.mp4

aimane-chnaif · 2022-12-13T07:26:06Z

@neil-marcellini please pull from latest main. This branch crashes on mWeb so not able to test on mWeb.

neil-marcellini · 2022-12-13T18:39:34Z

Ok @aimane-chnaif I merged main and verified that mWeb is working. From what I read on Slack here contributors are not able to test mWeb and Web right now. @arosiclair would you please run those tests for me? Or any of the other reviewers can do that.

aimane-chnaif · 2022-12-15T06:07:07Z

I managed to get my ip whitelisted and test web/mWeb successfully. Added those videos and updated checklist.

aimane-chnaif

LGTM 🎉

grgia

LGTM! I added a comment about adding a 'to' to a comment, but it's not necessary

grgia · 2022-12-15T16:20:03Z

src/libs/actions/BankAccounts.js

@@ -363,6 +363,15 @@ function openWorkspaceView() {
    API.read('OpenWorkspaceView');
 }

+/**
+ * Set the reimbursement account loading so that it happens right away, instead of when the API command is processed.


NAB

Suggested change

* Set the reimbursement account loading so that it happens right away, instead of when the API command is processed.

* Set the reimbursement account to loading so that it happens right away, instead of when the API command is processed.

In the interest of getting this merged I'm going to pass on that change for now.

OSBotify · 2022-12-15T16:56:53Z

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

github-actions · 2022-12-15T17:17:04Z

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
TTI	815.066 ms → 833.674 ms (+18.609 ms, +2.3%)
runJsBundle	189.645 ms → 193.406 ms (+3.761 ms, +2.0%)
regularAppStart	0.014 ms → 0.018 ms (+0.004 ms, +27.0%) 🔴
nativeLaunch	10.219 ms → 9.800 ms (-0.419 ms, -4.1%)

Show details

Name	Duration
TTI	Baseline Mean: 815.066 ms Stdev: 31.820 ms (3.9%) Runs: 751.5016709999181 761.2188389999792 770.4088869998232 776.9038329999894 779.9730170001276 782.0472510000691 787.9821250000969 789.177201999817 793.7277409997769 794.7087820000015 799.0010029999539 808.9704559999518 809.1942150001414 812.611359000206 813.0547549999319 815.2793189999647 815.4944389997981 820.1320569999516 823.8045689999126 824.3410950000398 830.6672419998795 833.5473039997742 836.6735229999758 837.2284740000032 838.6393070002086 838.6690019997768 853.1392529997975 854.5907589998096 859.2531249998137 862.6205179998651 892.4728069999255 Current Mean: 833.674 ms Stdev: 37.519 ms (4.5%) Runs: 779.8086589998566 781.5703150001355 781.8241730001755 790.5424959999509 790.5582200000063 792.8807279998437 795.434605000075 798.7990939998999 805.1003789999522 817.6651189997792 821.8315699999221 823.4917660001665 823.9036510000005 825.7654690002091 825.9968949998729 826.8073080000468 829.0621879999526 829.9240649999119 834.527854999993 841.3581960001029 851.2847670000046 853.0361509998329 853.3633929998614 858.7566729998216 868.9113050000742 870.9459520000964 873.800385999959 876.3132569999434 887.5544810001738 892.068826999981 941.0135699999519
runJsBundle	Baseline Mean: 189.645 ms Stdev: 18.993 ms (10.0%) Runs: 163 167 168 169 170 173 173 175 175 177 178 180 180 180 180 182 183 185 194 195 199 199 200 202 207 212 214 218 222 229 230 Current Mean: 193.406 ms Stdev: 20.808 ms (10.8%) Runs: 159 166 167 170 170 171 172 174 175 177 184 184 185 185 186 187 189 194 195 195 200 207 208 210 212 217 218 220 225 227 228 232
regularAppStart	Baseline Mean: 0.014 ms Stdev: 0.001 ms (6.5%) Runs: 0.012532999739050865 0.012817999813705683 0.012898999731987715 0.0129399998113513 0.0131029998883605 0.013183000031858683 0.013265000190585852 0.013305999804288149 0.013345999643206596 0.013630999717861414 0.013794000260531902 0.013915999792516232 0.013996999710798264 0.014037999790161848 0.014078999869525433 0.014119999948889017 0.014200999867171049 0.014241999946534634 0.014444999862462282 0.014525999780744314 0.014648000244051218 0.014892000239342451 0.014973999932408333 0.014973999932408333 0.014973999932408333 0.015015000011771917 0.015178000088781118 0.015217999927699566 0.016030999831855297 0.016032000072300434 Current Mean: 0.018 ms Stdev: 0.001 ms (8.3%) Runs: 0.01631700014695525 0.016358000226318836 0.016478999983519316 0.016478999983519316 0.016478999983519316 0.016600999981164932 0.016601999755948782 0.016642000060528517 0.0167239997535944 0.016764000058174133 0.01680499967187643 0.016927000135183334 0.017171999905258417 0.017293999902904034 0.017821999732404947 0.017821999732404947 0.01814799988642335 0.018148000352084637 0.01831099996343255 0.01843199972063303 0.018554999958723783 0.018635999877005816 0.018920999951660633 0.019001999869942665 0.01904299994930625 0.0194089999422431 0.01973400032147765 0.019979000091552734 0.020629999693483114 0.022745000198483467
nativeLaunch	Baseline Mean: 10.219 ms Stdev: 2.190 ms (21.4%) Runs: 8 8 8 8 8 8 8 8 8 8 9 9 9 9 10 10 10 10 10 11 11 11 11 11 11 12 12 12 14 14 15 16 Current Mean: 9.800 ms Stdev: 1.470 ms (15.0%) Runs: 8 8 8 8 8 9 9 9 9 9 9 9 9 9 9 9 10 10 10 10 10 10 11 11 11 12 12 12 12 14

OSBotify · 2022-12-16T04:00:39Z

🚀 Deployed to staging by @neil-marcellini in version: 1.2.41-0 🚀

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

OSBotify · 2022-12-16T04:32:21Z

🚀 Deployed to staging by @neil-marcellini in version: 1.2.41-0 🚀

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

OSBotify · 2022-12-20T06:05:56Z

🚀 Deployed to production by @yuwenmemon in version: 1.2.41-4 🚀

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

rushatgabhane · 2023-07-23T10:56:20Z

src/pages/workspace/reimburse/WorkspaceReimburseSection.js

+        const achState = lodashGet(this.props.reimbursementAccount, 'achData.state', '');
+        const hasVBA = achState === BankAccount.STATE.OPEN;
+
+        if (this.props.network.isOffline) {


hi all
we forgot to add offline behavior for other pages that had connect bank account button
eg: Cards’, ‘Bills’, ‘Invoices’, ‘Travel’ etc.

because of the missed case, we created an issue here - #21186

ntdiary · 2024-01-18T01:41:09Z

src/pages/workspace/reimburse/WorkspaceReimburseSection.js

+            shouldShowLoadingSpinner: false,
+        };
+
+        this.debounceSetShouldShowLoadingSpinner = _.debounce(this.setShouldShowLoadingSpinner.bind(this), CONST.TIMING.SHOW_LOADING_SPINNER_DEBOUNCE_TIME);


Coming from #32841.
We removed the debounce logic here, because it was not working as expected and caused the "unlock" section to blink.

neil-marcellini added 4 commits December 2, 2022 16:55

Set the reimbursement account loading

66df5d7

Add extra params to openWorkspaceReimburseView

bafb324

Reimburse section with loading and offline states

2ea9da4

Use workspace reimburse section

89ab681

neil-marcellini self-assigned this Dec 6, 2022

neil-marcellini added 9 commits December 6, 2022 12:05

Debounce the loading state to prevent flashing

0dacb1e

Fix prop types

c353af2

Match text style for the offline section

df0d06f

Pass the policy to WorkspaceReimburseSection

8647dcd

Render nothing if the VBA is briefly loading

ce92ef1

Reduce loading debounce for a more responsive feel

211e5d3

Default the VBA to loading on the reimburse view

dde6907

Always debounce the loading spinner

2da2fec

More descriptive name for state variable

4292962

neil-marcellini changed the title ~~Fetch the bank account for reimburse expenses with offline and loading support~~ [HOLD Web 35727] Fetch the bank account for reimburse expenses with offline and loading support Dec 7, 2022

neil-marcellini marked this pull request as ready for review December 7, 2022 22:50

neil-marcellini requested a review from a team as a code owner December 7, 2022 22:50

melvin-bot bot requested review from aimane-chnaif and grgia and removed request for a team December 7, 2022 22:50

arosiclair requested changes Dec 8, 2022

View reviewed changes

neil-marcellini added 2 commits December 8, 2022 12:56

Use money wings for the offline illustration

04db048

Remove useless promise return

6d18d26

Explain setting isLoading with a separate action

9db3ada

neil-marcellini requested a review from yuwenmemon December 12, 2022 19:22

arosiclair approved these changes Dec 12, 2022

View reviewed changes

Merge branch 'main' into neil-fix-reimburse

a6ef5b9

neil-marcellini changed the title ~~[HOLD Web 35727] Fetch the bank account for reimburse expenses with offline and loading support~~ Fetch the bank account for reimburse expenses with offline and loading support Dec 13, 2022

Merge branch 'main' to fix lint

0f02541

aimane-chnaif approved these changes Dec 15, 2022

View reviewed changes

grgia approved these changes Dec 15, 2022

View reviewed changes

neil-marcellini merged commit f15f96e into main Dec 15, 2022

neil-marcellini deleted the neil-fix-reimburse branch December 15, 2022 16:52

OSBotify mentioned this pull request Dec 16, 2022

Deploy Checklist: New Expensify 2022-12-16 #13654

Closed

25 tasks

mallenexpensify mentioned this pull request Dec 28, 2022

Payment for E/E PR review - User with a Workspace cannot add/select bank account for reimbursement E/E #244855 #13864

Closed

neil-marcellini mentioned this pull request Jan 10, 2023

[No QA] Prevent componentDidUpdate loop on WorkspaceReimburseSection #14167

Merged

53 tasks

rushatgabhane reviewed Jul 23, 2023

View reviewed changes

rushatgabhane mentioned this pull request Jul 23, 2023

[HOLD for payment 2023-07-26] [$1000] IOS - Offline Mode: Appropriate feedback is not shown on workspace menus that require an internet connection. #21186

Closed

6 tasks

bernhardoj mentioned this pull request Dec 11, 2023

[Waiting on BZ checklist] [$500] Reimbursements - "Unlock next-day reimbursements" section blinks #32841

Closed

6 tasks

ntdiary reviewed Jan 18, 2024

View reviewed changes

	* Set the reimbursement account loading so that it happens right away, instead of when the API command is processed.
	* Set the reimbursement account to loading so that it happens right away, instead of when the API command is processed.

Fetch the bank account for reimburse expenses with offline and loading support #13368

Fetch the bank account for reimburse expenses with offline and loading support #13368

Conversation

neil-marcellini commented Dec 6, 2022 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

neil-marcellini commented Dec 7, 2022

melvin-bot bot commented Dec 7, 2022

shawnborton commented Dec 7, 2022

shawnborton commented Dec 7, 2022

neil-marcellini commented Dec 8, 2022

arosiclair 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnborton commented Dec 8, 2022

neil-marcellini commented Dec 8, 2022

neil-marcellini commented Dec 8, 2022

neil-marcellini commented Dec 12, 2022

aimane-chnaif commented Dec 12, 2022

aimane-chnaif commented Dec 13, 2022

aimane-chnaif commented Dec 13, 2022 • edited Loading

Reviewer Checklist

Screenshots/Videos

aimane-chnaif commented Dec 13, 2022

neil-marcellini commented Dec 13, 2022

aimane-chnaif commented Dec 15, 2022

aimane-chnaif left a comment

Choose a reason for hiding this comment

grgia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

Performance Comparison Report 📊

Significant Changes To Duration

Meaningless Changes To Duration

OSBotify commented Dec 16, 2022

OSBotify commented Dec 16, 2022

OSBotify commented Dec 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-marcellini commented Dec 6, 2022 •

edited

Loading

aimane-chnaif commented Dec 13, 2022 •

edited

Loading