Refactor ReportScreenWrapper to functional component #22260

kosmydel · 2023-07-05T11:54:54Z

Details

This PR addresses two issues:

Refactor ReportScreenWrapper to functional component,
Explore using a skeleton for loading an app after sign in.

Fixed Issues

Related to #19263 (comment)
$ #20320

Tests

iOS/Android/mWeb iOS:

Log in to the account.
Enter the report by selecting it from the list, and check if the report is displayed correctly.
Leave the report by pressing the back button.
Enter another report.
Leave the report.

Web/desktop:

Log in to the account.
Check if while loading the skeleton is displayed instead of the loading circle.
Check if the last report is opened.
Navigate between reports several times.
3 Navigate to http://localhost:8080/r/ from the address bar.
Check if the last selected chat is opened.
Log out.
Create a new account.
Check if the user is forwarded to the Concierge report.

Offline tests

Checked on the web version with "Slow 3G" option if the flow works.

QA Steps

Verify that no errors appear in the JS console
Log in to the account with several reports, and navigate between reports.
In the address bar type (or with a different prefix) http://localhost:8080/r/, and check if the last report is automatically selected and loaded.
On the web/desktop: Create a new account, log in, and verify if the user can see the Concierge report.
On the web/desktop:
- Log in to the existing account.
- Select a random report and remember which report you have chosen.
- Log out from the account.
- Log in again to the same account.
- Check if the previously chosen report is displayed.
On the web/desktop:
- Log in to the account.
- Check if the skeleton is displayed in the report screen while loading.
- Check if the report is loaded successfully.

PR Author Checklist

Screenshots/Videos

Web

web.mov

Previous versions:

web.mov

web-newaccount.mov

Mobile Web - Chrome

Mobile Web - Safari

ios_web.mp4

Previous versions:

web-safari.mp4

Desktop

desktop.mov

Previous versions:

desktop.mov

iOS

ios.mp4

Previous versions:

ios.mp4

Android

android.mov

Previous versions:

android.mov

melvin-bot · 2023-07-05T11:54:59Z

@parasharrajat 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]

github-actions · 2023-07-05T11:55:11Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

kosmydel · 2023-07-05T11:56:13Z

I have read the CLA Document and I hereby sign the CLA

kosmydel · 2023-07-05T11:56:39Z

recheck

parasharrajat · 2023-07-05T12:27:35Z

src/libs/Navigation/AppNavigator/ReportScreenWrapper.js

Let's use useBetas hook.

Bump @kosmydel

Hey, sorry for the delay in the response.

I'm not sure what you mean by that. I couldn't find the useBetas hook in our codebase. Could you explain it a little more in detail?

Where should I get from the useBetas hook?

For what should I use it (instead of betas in withOnyx?)

Oops sorry! This is named usePermissions. Yes we can use this and remove betas Onyx subscription.

parasharrajat · 2023-07-10T16:46:12Z

There is only one change needed here. Let's get this done quickly so that we can merge it.

parasharrajat · 2023-07-11T00:05:45Z

Bump @kosmydel @mountiny

mountiny · 2023-07-11T02:05:57Z

I have bumped SWM i think @kosmydel is out for uni exams

kosmydel · 2023-07-11T12:54:18Z

Changed to draft, because I am going to add to this PR fix to a related issue (using skeleton instead of FullScreenLoadingIndicator).

parasharrajat · 2023-07-11T13:02:25Z

Ok. Let me know when ready.

kosmydel · 2023-07-11T13:28:22Z

I've updated the PR description and added a fix for displaying the skeleton when logging in.

parasharrajat · 2023-07-11T17:20:23Z

src/pages/home/ReportScreen.js

@@ -113,7 +113,7 @@ const defaultProps = {
 * @returns {String}
 */
 function getReportID(route) {
-    return route.params.reportID.toString();
+    return lodashGet(route, 'params.reportID', null);


Why is the change needed? It might have a side effect on the ReportScreen. As we are using this function to get a collection item on ReportScreen. If this returns null then we will be getting ${ONYXKEYS.COLLECTION.REPORT_ACTIONS}{null} which does not exists. It might lead to confusion and bugs in the future. Also we can not pass '' as defaultvalue, this will result in the whole collection being fetched.

The main reason is to show the skeleton while the reportID is not loaded yet (in the wrapper).
I made this change to allow the ReportScreen to use null as reportID, which then is used to display the skeleton.

This approach is very convenient because ReportScreen already implements a skeleton. But, as you say, it may lead to some bugs with Onyx.

I created a stripped-down version of ReportScreen when we don't know the reportID yet. It is used in ReportScreenWrapper.

Another solution might be, to add skeletons (when reportID is still loading) to the ReportScreenWrapper. But I think the first approach is better.

Thanks, I understood it later on after reviewing the code but forget to comment here. After looking at the code, I think we can just move back to the original logic of passing undefined or null which will not pass any data from onyx to ReportScreen. Because ReportScreen is already handling the skeleton logic, we can do this.

Current changes are good but do not give a lot of advantage over the previous code. So I will suggest we revert to what did originally and add a comment in ReportScreenWrapper where ReportScreen is rendered to explain this undefined report ID situation.

In the future, we can also think of breaking down the skeleton logic and reusing it.

src/pages/home/ReportScreen.js

parasharrajat · 2023-07-13T15:43:01Z

I don't know if this is an issue coming from this PR or present on main but every time I load the app via http://localhost:8080/r/ or http://localhost:8080. App is loading the same report which was not the last report I opened.

Screen.Recording.2023-07-13.at.9.13.18.PM.mov

@mountiny

parasharrajat · 2023-07-13T15:47:37Z

So I tested with a different account and the last report is not opening all the time. It opens the second last report.

So it seems that it works sometimes. Going to test it on main.

parasharrajat · 2023-07-13T16:01:04Z

Ok, not related to this PR.

parasharrajat · 2023-07-13T16:02:27Z

Screenshots

🔲 iOS / native

Screen.Recording.2023-07-14.at.1.10.55.AM.mov

🔲 iOS / Safari

Screen.Recording.2023-07-14.at.1.04.11.AM.mov

🔲 MacOS / Desktop

Screen.Recording.2023-07-14.at.1.05.40.AM.mov

🔲 MacOS / Chrome

Screen.Recording.2023-07-14.at.1.00.09.AM.mov

🔲 Android / Chrome

Screen.Recording.2023-07-14.at.1.02.26.AM.mov

🔲 Android / native

Screen.Recording.2023-07-14.at.1.36.46.AM.mov

mountiny · 2023-07-13T17:18:25Z

Thanks @parasharrajat lets get this one wrapped soon

parasharrajat

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 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.

🎀 👀 🎀 C+ reviewed

src/pages/home/ReportScreen.js

mountiny

Thanks @kosmydel and @parasharrajat

OSBotify · 2023-07-13T21:48:40Z

✋ 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-07-14T15:01:02Z

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

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

OSBotify · 2023-07-17T19:38:47Z

🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.41-3 🚀

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

OSBotify · 2023-07-17T19:39:50Z

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

platform	result
🤖 android 🤖	cancelled 🔪
🖥 desktop 🖥	cancelled 🔪
🍎 iOS 🍎	cancelled 🔪
🕸 web 🕸	cancelled 🔪

OSBotify · 2023-07-17T22:04:39Z

🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.41-3 🚀

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

OSBotify · 2023-07-19T18:01:35Z

🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.42-26 🚀

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

Refactor ReportScreenWrapper to functional component

d445cd8

kosmydel requested a review from a team as a code owner July 5, 2023 11:54

melvin-bot bot requested review from parasharrajat and removed request for a team July 5, 2023 11:54

parasharrajat reviewed Jul 5, 2023

View reviewed changes

mountiny assigned kosmydel Jul 6, 2023

mountiny self-requested a review July 6, 2023 03:05

kosmydel added 6 commits July 11, 2023 10:38

Merge branch 'Expensify:main' into kuba/refactor-report-screen-wrapper2

940c607

Prettier

86f4578

Merge branch 'Expensify:main' into kuba/refactor-report-screen-wrapper2

620608b

Use usePermissions hook

2cf2ca0

Fix useEffect dependencies

e119742

Fix skeleton

60dd0ef

kosmydel marked this pull request as draft July 11, 2023 12:51

kosmydel marked this pull request as ready for review July 11, 2023 13:27

parasharrajat suggested changes Jul 11, 2023

View reviewed changes

kosmydel added 4 commits July 12, 2023 07:52

Merge branch 'Expensify:main' into kuba/refactor-report-screen-wrapper2

7373174

Don't use Onyx when there is no reportID

f6e5f2a

Merge branch 'Expensify:main' into kuba/refactor-report-screen-wrapper2

0bfbbe4

Revert changes, add explanation

c56caf4

parasharrajat suggested changes Jul 13, 2023

View reviewed changes

src/pages/home/ReportScreen.js Outdated Show resolved Hide resolved

Convert returned reportID to string

9f15869

parasharrajat approved these changes Jul 13, 2023

View reviewed changes

mountiny reviewed Jul 13, 2023

View reviewed changes

src/pages/home/ReportScreen.js Show resolved Hide resolved

mountiny approved these changes Jul 13, 2023

View reviewed changes

mountiny merged commit fc3905b into Expensify:main Jul 13, 2023

OSBotify mentioned this pull request Jul 14, 2023

Deploy Checklist: New Expensify 2023-07-14 #22896

Closed

45 tasks

lanitochka17 mentioned this pull request Jul 14, 2023

[$1000] Chat - User is not navigated to last open chat from staging.new.expensify.com/r/ #22918

Closed

6 tasks

OSBotify mentioned this pull request Jul 17, 2023

Deploy Checklist: New Expensify 2023-07-17 #23033

Closed

melvin-bot bot mentioned this pull request Jul 17, 2023

[HOLD for payment 2023-07-24] [Navigation Refactor] Follow-ups from the migration PR #20320

Closed

16 tasks

fedirjh mentioned this pull request Oct 11, 2023

[HOLD for payment 2023-10-10] [$250] App crashing when clicked send message from Fub menu quickly after sign in from google #28504

Closed

6 tasks

erquhart mentioned this pull request Nov 11, 2023

[HOLD for payment 2023-11-30] [$500] Composer is very large on first render #31089

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ReportScreenWrapper to functional component #22260

Refactor ReportScreenWrapper to functional component #22260

kosmydel commented Jul 5, 2023 •

edited by mountiny

Loading

melvin-bot bot commented Jul 5, 2023

github-actions bot commented Jul 5, 2023 •

edited

Loading

kosmydel commented Jul 5, 2023

kosmydel commented Jul 5, 2023

parasharrajat Jul 5, 2023

parasharrajat Jul 6, 2023

kosmydel Jul 11, 2023

parasharrajat Jul 11, 2023

parasharrajat commented Jul 10, 2023

parasharrajat commented Jul 11, 2023 •

edited

Loading

mountiny commented Jul 11, 2023

kosmydel commented Jul 11, 2023

parasharrajat commented Jul 11, 2023

kosmydel commented Jul 11, 2023

parasharrajat Jul 11, 2023

kosmydel Jul 12, 2023 •

edited

Loading

parasharrajat Jul 13, 2023

parasharrajat Jul 13, 2023

parasharrajat commented Jul 13, 2023 •

edited

Loading

parasharrajat commented Jul 13, 2023 •

edited

Loading

parasharrajat commented Jul 13, 2023

parasharrajat commented Jul 13, 2023 •

edited

Loading

mountiny commented Jul 13, 2023

parasharrajat left a comment

mountiny left a comment

OSBotify commented Jul 13, 2023

OSBotify commented Jul 14, 2023

OSBotify commented Jul 17, 2023

OSBotify commented Jul 17, 2023

OSBotify commented Jul 17, 2023

OSBotify commented Jul 19, 2023

Refactor ReportScreenWrapper to functional component #22260

Refactor ReportScreenWrapper to functional component #22260

Conversation

kosmydel commented Jul 5, 2023 • edited by mountiny Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Jul 5, 2023

github-actions bot commented Jul 5, 2023 • edited Loading

kosmydel commented Jul 5, 2023

kosmydel commented Jul 5, 2023

parasharrajat Jul 5, 2023

Choose a reason for hiding this comment

parasharrajat Jul 6, 2023

Choose a reason for hiding this comment

kosmydel Jul 11, 2023

Choose a reason for hiding this comment

parasharrajat Jul 11, 2023

Choose a reason for hiding this comment

parasharrajat commented Jul 10, 2023

parasharrajat commented Jul 11, 2023 • edited Loading

mountiny commented Jul 11, 2023

kosmydel commented Jul 11, 2023

parasharrajat commented Jul 11, 2023

kosmydel commented Jul 11, 2023

parasharrajat Jul 11, 2023

Choose a reason for hiding this comment

kosmydel Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

parasharrajat Jul 13, 2023

Choose a reason for hiding this comment

parasharrajat Jul 13, 2023

Choose a reason for hiding this comment

parasharrajat commented Jul 13, 2023 • edited Loading

parasharrajat commented Jul 13, 2023 • edited Loading

parasharrajat commented Jul 13, 2023

parasharrajat commented Jul 13, 2023 • edited Loading

Screenshots

🔲 iOS / native

🔲 iOS / Safari

🔲 MacOS / Desktop

🔲 MacOS / Chrome

🔲 Android / Chrome

🔲 Android / native

mountiny commented Jul 13, 2023

parasharrajat left a comment

Choose a reason for hiding this comment

Reviewer Checklist

mountiny left a comment

Choose a reason for hiding this comment

OSBotify commented Jul 13, 2023

OSBotify commented Jul 14, 2023

OSBotify commented Jul 17, 2023

OSBotify commented Jul 17, 2023

OSBotify commented Jul 17, 2023

OSBotify commented Jul 19, 2023

kosmydel commented Jul 5, 2023 •

edited by mountiny

Loading

github-actions bot commented Jul 5, 2023 •

edited

Loading

parasharrajat commented Jul 11, 2023 •

edited

Loading

kosmydel Jul 12, 2023 •

edited

Loading

parasharrajat commented Jul 13, 2023 •

edited

Loading

parasharrajat commented Jul 13, 2023 •

edited

Loading

parasharrajat commented Jul 13, 2023 •

edited

Loading