Update reimbursement account page to function component #29166

keisyrzk · 2023-10-10T10:00:25Z

Details

Updated the component so it is now built with a function.

Fixed Issues

$ #16245

Tests

Click on avatar on the main view
Go to Workspaces
Choose a workspace
Go to Reimbursements
Under "Unlock next-day reimbursements" section click Connect Bank Account
On the next view click Connect Manually and when loaded go back

Verify that no errors appear in the JS console

Offline tests

NA

QA Steps

Same steps as the Tests section

[:white_check_mark:] Verify that no errors appear in the JS console

PR Author Checklist

I linked the correct issue in the ### Fixed Issues section above
I wrote clear testing steps that cover the changes made in this PR
- I added steps for local testing in the Tests section
- I added steps for the expected offline behavior in the Offline steps section
- I added steps for Staging and/or Production testing in the QA steps section
- I added steps to cover 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
I included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (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
  - If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
- 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 followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
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
I verified that if a function's arguments changed that all usages have also been updated correctly
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
- If we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes
- 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 author checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

android.mp4

Android: mWeb Chrome

android_chrome.mp4

iOS: Native

ios.mp4

iOS: mWeb Safari

ios_safari.mp4

MacOS: Chrome / Safari

macos_chrome.mp4

macos_safari.mp4

MacOS: Desktop

macos_desktop.mp4

rezkiy37

Also, please run lint script to make sure that everything is correct, because I see that some deps. arrays don't have all dependencies and no lint ignores.

src/pages/ReimbursementAccount/ReimbursementAccountPage.js

keisyrzk · 2023-11-07T12:07:15Z

@ntdiary @marcaaron just want to let you know the changes have been pushed yesterday

ntdiary · 2023-11-08T05:32:07Z

@ntdiary @marcaaron just want to let you know the changes have been pushed yesterday

@keisyrzk, thank you, I have tested, looks like it's working well, except there's a small difference when opening from bill option, but personally I don't feel it's a blocking problem, it would be better to complete the migration first, then optimize the business logic separately.

test.mp4

marcaaron · 2023-11-08T05:56:28Z

Sorry for the delay here - code LGTM. We must run prettier and then it is ready for

keisyrzk · 2023-11-08T06:22:51Z

@marcaaron I ran prettier and pushed the update.

marcaaron

LGTM, but the reassure performance test is failing. I am not sure how to resolve that and don't quite understand why it fails.

ntdiary · 2023-11-09T04:04:53Z

LGTM, but the reassure performance test is failing. I am not sure how to resolve that and don't quite understand why it fails.

@marcaaron, the failed jest test on the main branch are causing the reassure job to fail, So there's probably not much we can do on this branch. : )

job error code:
reassure source code:
failed jest test:

I see other branches have successful runs, maybe we could also try rerunning the reassure job, or merging this branch first, and then improving the stability of the jest tests in a separate issue.

mountiny · 2023-11-09T23:06:21Z

That failure is just a timeout in those tests which we have fixed in a recent PR, I am going to merge this over the failing tests as its flakey

OSBotify · 2023-11-09T23:12:57Z

✋ 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-11-10T15:59:06Z

🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.98-0 🚀

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

NikkiWines · 2023-11-10T19:06:37Z

src/pages/ReimbursementAccount/ReimbursementAccountPage.js

 import ReimbursementAccountLoadingIndicator from '@components/ReimbursementAccountLoadingIndicator';
 import ScreenWrapper from '@components/ScreenWrapper';
-import Text from '@components/Text';
-import withLocalize, {withLocalizePropTypes} from '@components/withLocalize';
+import Text from '@components/TextInput';


This import is tied to a crash on staging here. Fix is here

Should have been

import Text from '@components/Text';

johnmlee101 · 2023-11-10T20:59:12Z

This caused a regression IRT the resuming bank account flows here: #31212

…ccountPage" This reverts commit 2addd69, reversing changes made to b9643cd.

johnmlee101 · 2023-11-10T21:17:26Z

This is staged to be reverted and CP'd

[CP Staging] Revert #31201 and #29166

[CP Staging] Revert #31201 and #29166 (cherry picked from commit ab4a148)

OSBotify · 2023-11-14T21:52:34Z

🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.98-5 🚀

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

ntdiary · 2023-11-15T06:19:26Z

This caused a regression IRT the resuming bank account flows here: #31212

Ah, I didn't realize it was a regression. I initially thought it wasn't a blocking problem, so I left only a comment there. 😅

OSBotify · 2023-11-15T21:10:12Z

🚀 Deployed to staging by https://github.com/mountiny in version: 1.4.0-0 🚀

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

keisyrzk added 4 commits October 6, 2023 12:33

migration

3b50dca

testing logs

7dd1c89

test logging

bdf950f

updates

9747361

rezkiy37 suggested changes Oct 10, 2023

View reviewed changes

keisyrzk added 5 commits October 10, 2023 14:15

white spaces

66e5db9

useLocalize

9cb601c

updated useLocalize path

a4b210d

useNetwork

6afd415

network attribute clean up

1d0a428

rezkiy37 suggested changes Oct 11, 2023

View reviewed changes

keisyrzk added 2 commits October 11, 2023 14:43

remove prevProps and use useRef instead

67bc8ad

some code styling

164ee64

marcaaron self-requested a review October 13, 2023 20:33

BeeMargarida reviewed Oct 16, 2023

View reviewed changes

code arrangement updates

c3b85d7

keisyrzk requested review from rezkiy37, adhorodyski and BeeMargarida October 17, 2023 09:26

rezkiy37 approved these changes Oct 17, 2023

View reviewed changes

koko57 reviewed Oct 19, 2023

View reviewed changes

src/pages/ReimbursementAccount/ReimbursementAccountPage.js Outdated Show resolved Hide resolved

arrow function update

ffe65cc

keisyrzk force-pushed the update_ReimbursementAccountPage branch from fa28b86 to ffe65cc Compare October 19, 2023 09:47

Merge branch 'main' into update_ReimbursementAccountPage

b29d612

keisyrzk requested a review from koko57 October 19, 2023 09:55

update all functions to be without =>

2c821db

barttom approved these changes Oct 19, 2023

View reviewed changes

src/pages/ReimbursementAccount/ReimbursementAccountPage.js Outdated Show resolved Hide resolved

src/pages/ReimbursementAccount/ReimbursementAccountPage.js Outdated Show resolved Hide resolved

take getStepToOpenFromRouteParams function out of the main component

c32d081

keisyrzk marked this pull request as ready for review October 19, 2023 11:34

keisyrzk requested a review from a team as a code owner October 19, 2023 11:34

move code back insoide the useEffect

02bc688

prettier ran

1df1ae8

marcaaron approved these changes Nov 8, 2023

View reviewed changes

mountiny merged commit 2addd69 into Expensify:main Nov 9, 2023

OSBotify mentioned this pull request Nov 10, 2023

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

Closed

kbecciv mentioned this pull request Nov 10, 2023

[$500] [HOLD for payment 2023-11-22] [HOLD for payment 2023-11-21] Workspace - App crashes when opening Bank account page #31188

Closed

6 tasks

NikkiWines reviewed Nov 10, 2023

View reviewed changes

johnmlee101 mentioned this pull request Nov 10, 2023

[HOLD for payment 2023-11-22] [HOLD for payment 2023-11-21] Workspace - Start over page is not shown when user quits bank account setup and revisits it #31212

Closed

6 tasks

johnmlee101 added a commit that referenced this pull request Nov 10, 2023

Revert "Merge pull request #29166 from keisyrzk/update_ReimbursementA…

01b05ce

…ccountPage" This reverts commit 2addd69, reversing changes made to b9643cd.

johnmlee101 mentioned this pull request Nov 10, 2023

[CP Staging] Revert https://github.com/Expensify/App/pull/31201 and https://github.com/Expensify/App/pull/29166 #31214

Merged

59 tasks

marcaaron added a commit that referenced this pull request Nov 10, 2023

Merge pull request #31214 from Expensify/john-revert

ab4a148

[CP Staging] Revert #31201 and #29166

OSBotify pushed a commit that referenced this pull request Nov 10, 2023

Merge pull request #31214 from Expensify/john-revert

d50319b

[CP Staging] Revert #31201 and #29166 (cherry picked from commit ab4a148)

melvin-bot bot mentioned this pull request Nov 14, 2023

[HOLD for payment 2023-11-22] [HOLD for payment 2023-11-21] [$250] Migrate ReimbursementAccountPage.js to function component #16245

Closed

1 task

OSBotify mentioned this pull request Nov 15, 2023

Deploy Checklist: New Expensify 2023-11-15 #31404

Closed

37 tasks

keisyrzk mentioned this pull request Nov 16, 2023

[fix] show continue or start over widget on form filling exit from step 3 #31439

Merged

47 tasks

ntdiary mentioned this pull request Dec 5, 2023

[$500] Workspace - Bank account setup page appears briefly when opening BA when BA is already set up #31207

Closed

6 tasks

barttom mentioned this pull request Jan 18, 2024

fix: editing issue after coming back barttom/App-expensify#3

Merged

50 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update reimbursement account page to function component #29166

Update reimbursement account page to function component #29166

keisyrzk commented Oct 10, 2023 •

edited

Loading

rezkiy37 left a comment

keisyrzk commented Nov 7, 2023

ntdiary commented Nov 8, 2023

marcaaron commented Nov 8, 2023

keisyrzk commented Nov 8, 2023

marcaaron left a comment

ntdiary commented Nov 9, 2023 •

edited

Loading

mountiny commented Nov 9, 2023 •

edited

Loading

OSBotify commented Nov 9, 2023

OSBotify commented Nov 10, 2023

NikkiWines Nov 10, 2023

johnmlee101 commented Nov 10, 2023

johnmlee101 commented Nov 10, 2023

OSBotify commented Nov 14, 2023

ntdiary commented Nov 15, 2023

OSBotify commented Nov 15, 2023

Update reimbursement account page to function component #29166

Update reimbursement account page to function component #29166

Conversation

keisyrzk commented Oct 10, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

rezkiy37 left a comment

Choose a reason for hiding this comment

keisyrzk commented Nov 7, 2023

ntdiary commented Nov 8, 2023

marcaaron commented Nov 8, 2023

keisyrzk commented Nov 8, 2023

marcaaron left a comment

Choose a reason for hiding this comment

ntdiary commented Nov 9, 2023 • edited Loading

mountiny commented Nov 9, 2023 • edited Loading

OSBotify commented Nov 9, 2023

OSBotify commented Nov 10, 2023

NikkiWines Nov 10, 2023

Choose a reason for hiding this comment

johnmlee101 commented Nov 10, 2023

johnmlee101 commented Nov 10, 2023

OSBotify commented Nov 14, 2023

ntdiary commented Nov 15, 2023

OSBotify commented Nov 15, 2023

keisyrzk commented Oct 10, 2023 •

edited

Loading

ntdiary commented Nov 9, 2023 •

edited

Loading

mountiny commented Nov 9, 2023 •

edited

Loading