Add a Not Found page for IOU reports that we cannot load #14028

yuwenmemon · 2023-01-05T17:30:57Z

Details

As pointed out by @mollfpr here we were always showing a loading spinner if we didn't have an IOU report. Instead, we should be showing a report not found page and using the loading property in the IOU to show whether or not the report is loading.

Fixed Issues

$ #14011

Tests

Login with any account A
Send an IOU from account A to another account B
Click the IOU and copy the URL to it
(Via incognito window) From another account C, paste in the URL - make sure you see a page indicating the payment cannot be found

4a. **For iOS/Android/Desktop:** From account C paste the link into a chat message. Then click on it. Make sure you see a page indicating the payment cannot be found

Verify that no errors appear in the JS console

Offline tests

As account A/B from above, load the chat with the IOU report
Go offline
Click on the IOU report.
Make sure you can see the IOU report in the modal.

QA Steps

Perform the tests and offline tests sections above on all platforms.
Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

melvin-bot · 2023-01-05T18:00:54Z

@mollfpr @Julesssss 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]

yuwenmemon · 2023-01-05T18:06:08Z

src/pages/iou/IOUDetailsModal.js

@@ -146,47 +147,54 @@ class IOUDetailsModal extends Component {

    render() {
        const sessionEmail = lodashGet(this.props.session, 'email', null);
-        const reportIsLoading = _.isUndefined(this.props.iouReport);
+        const reportIsLoading = this.props.iou.loading;


@Julesssss I think this makes sense, right? This was the one part I was unsure about - the difference between iou and iouReport and if they're linked enough anyway that the loading property from iou would suffice.

I try to log the this.props.iou.loading inside the render function, but it returns false on both accounts (the account that has the IOU data and the account which doesn't have the IOU data).

So what are we saying? This prop is just useless anyway?

Could be, but this.props.iou.loading is also been used for disabling the settlement button. I will take a look more on the props.iou.

Hmm, yeah that one might be deprecated actually. it's probably better to check for the report itself in Onyx

Alternatively, we can create single Onyx data to keep the loading state for the OpenPaymentDetailsPage API similar to what we did with the OpenApp API and ONYXKEYS.IS_LOADING_REPORT_DATA.

I think I like this - what do you think @Julesssss ?

Yeah, I think that is pretty straight forward. Let's do it.

Actually, on second look, I think we don't need this since we already load all iouReports when opening the app, and if one is ever created for you it is pushed to you via Pusher. So there would never be a state where we would need to be "loading" the IOU report data? Right?

Ah nevermind no, this would be something that needs to wrap the OpenPaymentDetailsPage command.

Ah okay -

Could be, but this.props.iou.loading is also been used for disabling the settlement button. I will take a look more on the props.iou.

It's not useful anymore since we optimistically pay the IOU, so the settlement button's loading state is outdated. So, what I'll do is repurpose that loading property for loading the iou report itself.

Julesssss

Ran out of time, will need to finish up testing on Monday

yuwenmemon · 2023-01-10T19:58:06Z

Updated!

Julesssss

Tested well for me

Julesssss · 2023-01-11T11:58:07Z

@mollfpr over to you for the checklist review

mollfpr · 2023-01-11T12:00:21Z

@Julesssss Are we okay with showing the loading when the fetching happens while the data is already in Onyx?

Julesssss · 2023-01-11T12:11:54Z

@Julesssss Are we okay with showing the loading when the fetching happens while the data is already in Onyx?

Yeah, I don't see why not. It should be a very short period and we can improve this later by matching the regular chats:

mollfpr · 2023-01-11T12:19:06Z

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

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

mollfpr · 2023-01-11T13:27:57Z

Screen.Recording.2023-01-11.at.20.25.50.mov

@yuwenmemon @Julesssss When I open the IOU detail offline, it keeps showing the loading spinner.

Julesssss · 2023-01-11T14:49:57Z

Screen.Recording.2023-01-11.at.20.25.50.mov
@yuwenmemon @Julesssss When I open the IOU detail offline, it keeps showing the loading spinner.

Oh, I didn't see that. When offline I still saw the same UI 😕

mollfpr · 2023-01-11T14:59:53Z

Oh, I didn't see that. When offline I still saw the same UI 😕

@Julesssss Wait, I’m confused 😳 Is this mean your end working fine offline?

yuwenmemon · 2023-01-11T17:15:46Z

@mollfpr I see it - it's happening when you do have access to the IOU which is where I was getting confused as well. Looking into it!

yuwenmemon · 2023-01-11T17:30:30Z

Updated!

src/pages/iou/IOUDetailsModal.js

mollfpr · 2023-01-12T05:08:35Z

I think we should only show the back button only on the small screen.

Julesssss · 2023-01-12T12:47:45Z

I think we should only show the back button only on the small screen.

I disagree. I think it's totally fine as we show the back button in the RightDockedModal for other pages.

mollfpr · 2023-01-12T14:30:08Z

I can't test open the localhost link on native 😕

Any suggestion? @yuwenmemon @Julesssss

Julesssss · 2023-01-12T15:00:47Z

I can't test open the localhost link on native 😕

Any suggestion? @yuwenmemon @Julesssss

I manually edited the link to something like staging.new.expensify.com/r/1223456/12345678

mollfpr · 2023-01-12T15:34:21Z

Test well on offline too 👍

Here's the checklist

OSBotify · 2023-01-12T16:47:09Z

✋ 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 · 2023-01-12T17:17:33Z

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	664.583 ms → 677.011 ms (+12.428 ms, +1.9%)
App start regularAppStart	0.020 ms → 0.021 ms (+0.001 ms, +6.0%)
App start nativeLaunch	20.161 ms → 19.897 ms (-0.265 ms, -1.3%)
Open Search Page TTI	608.320 ms → 606.097 ms (-2.223 ms, ±0.0%)
App start runJsBundle	188.000 ms → 185.516 ms (-2.484 ms, -1.3%)

Show details

Name	Duration
App start TTI	Baseline Mean: 664.583 ms Stdev: 20.823 ms (3.1%) Runs: 624.1787369996309 627.4688870003447 630.6228740001097 639.536442999728 639.561999999918 646.3896009996533 649.46189600043 654.574032000266 656.2257049996406 656.7890269998461 659.8082550000399 663.1761800004169 663.8457519998774 663.9181840000674 664.1209629997611 664.2242149999365 664.7862729998305 665.6320780003443 669.4525490002707 670.8962759999558 673.7665069997311 676.932912000455 676.9558399999514 687.3068859996274 688.5303790001199 689.2297839997336 697.8043020004407 703.5312909996137 704.1807890003547 Current Mean: 677.011 ms Stdev: 30.809 ms (4.6%) Runs: 615.9887960003689 632.378023000434 635.2633490003645 636.7172010000795 640.2977740000933 644.9252479998395 648.8455280000344 654.5047019999474 655.0161650003865 656.2057689996436 663.9843610003591 664.7070660004392 666.2557110004127 667.2204010002315 668.3416430000216 669.0811569998041 681.5912039997056 686.6614049999043 690.2921059997752 691.7917379997671 693.6057040002197 696.6852139998227 697.318679000251 698.4598139999434 704.3844330003485 706.8637699997053 712.702085999772 721.4781149998307 723.2467809999362 723.5771559998393 738.952580999583
App start regularAppStart	Baseline Mean: 0.020 ms Stdev: 0.001 ms (6.7%) Runs: 0.01708999928086996 0.01769999973475933 0.01875900011509657 0.01896199956536293 0.019001999869942665 0.019003000110387802 0.019164999946951866 0.01916500087827444 0.019247000105679035 0.01945000048726797 0.01948999986052513 0.0195720000192523 0.0195720000192523 0.01961199939250946 0.020060000009834766 0.0201000003144145 0.020100999623537064 0.020182999782264233 0.020263000391423702 0.0204670000821352 0.020508000627160072 0.020548000000417233 0.0206709997728467 0.020711999386548996 0.021037000231444836 0.021403000690042973 0.022867999970912933 0.022909000515937805 0.023029999807476997 Current Mean: 0.021 ms Stdev: 0.002 ms (7.2%) Runs: 0.01729300059378147 0.01912400033324957 0.01948999986052513 0.019531000405550003 0.019612999632954597 0.01985699962824583 0.01985699962824583 0.019898000173270702 0.020060000009834766 0.020142000168561935 0.020142000168561935 0.02058899961411953 0.020711999386548996 0.02091400045901537 0.021037000231444836 0.0215659998357296 0.02164699975401163 0.021809999831020832 0.02185099944472313 0.022053999826312065 0.022216999903321266 0.022216999903321266 0.022297999821603298 0.02254199981689453 0.022542000748217106 0.02266399934887886 0.0227870000526309 0.022827000357210636 0.023357000201940536 0.02380300033837557 0.02380399964749813
App start nativeLaunch	Baseline Mean: 20.161 ms Stdev: 2.411 ms (12.0%) Runs: 17 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 19 19 20 20 20 21 21 21 22 22 22 25 25 26 26 Current Mean: 19.897 ms Stdev: 1.768 ms (8.9%) Runs: 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 19 20 20 20 20 21 21 21 21 22 22 22 24 25
Open Search Page TTI	Baseline Mean: 608.320 ms Stdev: 18.919 ms (3.1%) Runs: 574.8822839995846 578.4131680000573 579.8332520006225 583.5517180003226 586.6814370006323 588.9997149994597 592.5901289992034 595.9288330003619 598.6160069992766 602.34952800069 604.2237139996141 604.8581550000235 605.5653889998794 605.8985999999568 605.9761149995029 606.4313970003277 608.8160809995607 609.5792240006849 610.3715409999713 612.3376059997827 612.4656990002841 612.9126380002126 614.6207679994404 616.7090259995311 619.294882000424 621.8960769996047 622.9720869995654 640.2333990000188 641.8454189999029 647.7712810002267 651.2835289994255 Current Mean: 606.097 ms Stdev: 17.218 ms (2.8%) Runs: 572.3485519997776 576.6610920000821 582.4024260006845 583.1663820007816 583.7055260008201 587.5101319998503 588.7818610006943 591.3633230002597 595.2397460006177 597.2762859994546 599.6730960002169 599.9379070000723 600.8170159999281 604.9495040001348 606.3610429996625 606.5988770006225 607.0715749999508 608.8473720001057 609.1082360008731 609.1664220001549 613.8159590000287 614.4084069998935 615.0357259996235 615.994425999932 616.022257999517 616.931315000169 624.8659670008346 626.1416419995949 631.574421999976 634.3087170002982 636.0040690004826 639.0074869999662
App start runJsBundle	Baseline Mean: 188.000 ms Stdev: 18.240 ms (9.7%) Runs: 160 160 161 161 165 166 173 173 173 174 175 177 183 186 186 187 189 191 192 196 196 198 199 201 204 205 206 209 214 215 216 225 Current Mean: 185.516 ms Stdev: 16.553 ms (8.9%) Runs: 156 162 170 170 171 172 174 174 174 176 176 176 180 180 182 182 183 184 184 188 188 191 193 193 197 199 203 207 212 222 232

OSBotify · 2023-01-13T12:11:54Z

🚀 Deployed to staging by @Julesssss in version: 1.2.54-0 🚀

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

OSBotify · 2023-01-16T21:06:20Z

🚀 Deployed to production by @AndrewGable in version: 1.2.54-2 🚀

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

Add a not found page for iou reports that we cannot load)

7ef95f7

yuwenmemon self-assigned this Jan 5, 2023

Oops re-add in dismiss modal call

13edbdd

yuwenmemon marked this pull request as ready for review January 5, 2023 18:00

yuwenmemon requested a review from a team as a code owner January 5, 2023 18:00

melvin-bot bot requested review from Julesssss and mollfpr and removed request for a team January 5, 2023 18:00

yuwenmemon commented Jan 5, 2023

View reviewed changes

Julesssss reviewed Jan 6, 2023

View reviewed changes

yuwenmemon added 2 commits January 10, 2023 10:50

Merge branch 'main' of github.com:Expensify/App into yuwen-iouNotFound

c08ce1c

Repurpose unused loading property for iou report loading

ad13a22

Julesssss previously approved these changes Jan 11, 2023

View reviewed changes

Merge branch 'main' of github.com:Expensify/App into yuwen-iouNotFound

7b83aef

Make sure we don't try to fetch the report if we are offline

3ac0f2e

yuwenmemon dismissed Julesssss’s stale review via 3ac0f2e January 11, 2023 17:27

Julesssss approved these changes Jan 11, 2023

View reviewed changes

Julesssss reviewed Jan 11, 2023

View reviewed changes

src/pages/iou/IOUDetailsModal.js Show resolved Hide resolved

mollfpr approved these changes Jan 12, 2023

View reviewed changes

Julesssss merged commit 9a7b027 into main Jan 12, 2023

Julesssss deleted the yuwen-iouNotFound branch January 12, 2023 16:42

OSBotify mentioned this pull request Jan 13, 2023

Deploy Checklist: New Expensify 2023-01-13 #14277

Closed

13 tasks

melvin-bot bot mentioned this pull request Jan 16, 2023

[HOLD for Regression Test] Infinite loading while opening IOU request details deeplink #14011

Closed

6 tasks

fedirjh mentioned this pull request Jan 19, 2023

[HOLD Manual Requests][$1000] IOU Details page reload again after opening them same IOU #14399

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a Not Found page for IOU reports that we cannot load #14028

Add a Not Found page for IOU reports that we cannot load #14028

yuwenmemon commented Jan 5, 2023 •

edited by Julesssss

Loading

melvin-bot bot commented Jan 5, 2023

yuwenmemon Jan 5, 2023

mollfpr Jan 6, 2023

yuwenmemon Jan 6, 2023

mollfpr Jan 6, 2023

Julesssss Jan 6, 2023

yuwenmemon Jan 10, 2023

Julesssss Jan 10, 2023

yuwenmemon Jan 10, 2023

yuwenmemon Jan 10, 2023

yuwenmemon Jan 10, 2023

Julesssss left a comment

yuwenmemon commented Jan 10, 2023

Julesssss left a comment

Julesssss commented Jan 11, 2023

mollfpr commented Jan 11, 2023

Julesssss commented Jan 11, 2023

mollfpr commented Jan 11, 2023 •

edited

Loading

mollfpr commented Jan 11, 2023

Julesssss commented Jan 11, 2023

mollfpr commented Jan 11, 2023

yuwenmemon commented Jan 11, 2023

yuwenmemon commented Jan 11, 2023

mollfpr commented Jan 12, 2023

Julesssss commented Jan 12, 2023

mollfpr commented Jan 12, 2023

Julesssss commented Jan 12, 2023

mollfpr commented Jan 12, 2023 •

edited

Loading

OSBotify commented Jan 12, 2023

github-actions bot commented Jan 12, 2023

OSBotify commented Jan 13, 2023

OSBotify commented Jan 16, 2023

Add a Not Found page for IOU reports that we cannot load #14028

Add a Not Found page for IOU reports that we cannot load #14028

Conversation

yuwenmemon commented Jan 5, 2023 • edited by Julesssss Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Jan 5, 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

Choose a reason for hiding this comment

Julesssss left a comment

Choose a reason for hiding this comment

yuwenmemon commented Jan 10, 2023

Julesssss left a comment

Choose a reason for hiding this comment

Julesssss commented Jan 11, 2023

mollfpr commented Jan 11, 2023

Julesssss commented Jan 11, 2023

mollfpr commented Jan 11, 2023 • edited Loading

Reviewer Checklist

Screenshots/Videos

mollfpr commented Jan 11, 2023

Julesssss commented Jan 11, 2023

mollfpr commented Jan 11, 2023

yuwenmemon commented Jan 11, 2023

yuwenmemon commented Jan 11, 2023

mollfpr commented Jan 12, 2023

Julesssss commented Jan 12, 2023

mollfpr commented Jan 12, 2023

Julesssss commented Jan 12, 2023

mollfpr commented Jan 12, 2023 • edited Loading

OSBotify commented Jan 12, 2023

github-actions bot commented Jan 12, 2023

Performance Comparison Report 📊

Significant Changes To Duration

Meaningless Changes To Duration

OSBotify commented Jan 13, 2023

OSBotify commented Jan 16, 2023

yuwenmemon commented Jan 5, 2023 •

edited by Julesssss

Loading

mollfpr commented Jan 11, 2023 •

edited

Loading

mollfpr commented Jan 12, 2023 •

edited

Loading