Refactor fetchIOUReportByID by luacmartins · Pull Request #13556 · Expensify/App

luacmartins · 2022-12-13T16:28:15Z

Details

Removes fetchIOUReportByID since that data is returned in OpenReport and ReadOldestAction

cc @Julesssss @mountiny

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/249195

Tests

Should be tested with the Web-E PR

From account A, send an IOU request to account B
In account B, pay the IOU request
Log into account A and verify that the paid IOU preview loads
From account A, send 50+ messages to account B
Send another IOU request
From account B, pay the latest IOU request
Log out and back into account A
Verify that you see the 2nd paid IOU preview without any loading spinners
Scroll back in the chat until you see the 1st IOU preview.
Verify that you see the 1st paid IOU preview without any loading spinners

Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

Same as test steps

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

chrome.mov

Mobile Web - Safari

safari.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

luacmartins · 2022-12-14T20:35:45Z

-            // This is expected and we return early as no further processing can be done.
-            return;
-        }
-        return getSimplifiedIOUReport(iouReportData, chatReportID);


This is still used in syncChatAndIOUReports. We can remove it once PayWithWallet is refactored

melvin-bot · 2022-12-14T21:50:28Z

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

luacmartins · 2022-12-14T21:51:18Z

@mollfpr please hold on testing this until the API changes are in staging!

mountiny

Nice! code looks good to me, gonna test tomorrow hopefully

mountiny · 2022-12-14T22:13:59Z

-            // This is expected and we return early as no further processing can be done.
-            return;
-        }
-        return getSimplifiedIOUReport(iouReportData, chatReportID);


mollfpr · 2022-12-15T02:10:50Z

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

13556.Web.mov

Mobile Web - Chrome

Mobile Web - Safari

Desktop

13556.Desktop.mov

iOS

13556.iOS.mov

Android

13556.Android.mov

mountiny

Tested well for me locally

Julesssss

Looks good, I just left a question because I can't figure out why THIS component needs the iouReport prop.

mollfpr · 2022-12-15T14:47:41Z

Can we clean this up? iou props are no longer used in IOUPreview and we get the IOU report data in the component.

App/src/pages/iou/IOUDetailsModal.js

Lines 149 to 150 in a414d1a

    
           <IOUPreview 
        
               iou={this.props.iouReport}

luacmartins · 2022-12-15T17:28:17Z

@mollfpr removed the iou prop!

mollfpr · 2022-12-15T17:28:41Z

Just a curiosity, in detail, mentioned that now we using OpenIOUPreview but I don't see any OpenIOUPreview is called.

luacmartins · 2022-12-15T17:34:29Z

@mollfpr nice catch! That was the first solution that I implemented and I forgot to update the description to reflect the new improved solution. I updated it now, so it's correct!

mollfpr

LGTM 👍

Will test it when the API hit the staging!

mountiny

Nice catch with the unused prop!

@mollfpr This is ready to be tested against Staging!

mollfpr · 2022-12-16T14:48:33Z

Updated checklist and test well on Web, Desktop, iOS, and Android.

I can't go through the mWeb testing because my IP is still not whitelisted. I believe the result will be the same...

mountiny · 2022-12-19T11:28:50Z

@mollfpr Thank you very much, I think that sounds fine to me.

@luacmartins There are some merge conflicts 😢

mountiny

I have tested this in mWeb locally and it was a pass, although even locally sending messages quickly is super slow for some reason.

Screen.Recording.2022-12-19.at.15.12.08.mp4

luacmartins · 2022-12-19T18:35:19Z

Since Web-E is not in prod yet and PayWithWallet might be merged today, I'll hold this PR so we can also remove this

mountiny

@luacmartins Thank you, this is no ton hold anymore as the Web PR hit production.

I think we can make that follow up clean up in a new PR just to also keep the concerns separate, maybe we can wait for the PayWithWallet to get to production and confirm there are no regressions.

github-actions · 2022-12-20T12:34:52Z

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
TTI	793.897 ms → 818.956 ms (+25.059 ms, +3.2%)
runJsBundle	178.931 ms → 188.258 ms (+9.327 ms, +5.2%)
regularAppStart	0.014 ms → 0.014 ms (+0.000 ms, +1.6%)
nativeLaunch	9.433 ms → 9.300 ms (-0.133 ms, -1.4%)

Show details

Name	Duration
TTI	Baseline Mean: 793.897 ms Stdev: 31.368 ms (4.0%) Runs: 741.659944999963 747.559591999976 757.4528679999057 761.2233549999073 762.3574390000431 764.8560940000461 767.5528820001055 767.6263699999545 773.0966890000273 776.3483120000456 777.8668219998945 778.415119000012 780.0740869999863 783.3622610000893 785.7879129999783 788.2109870000277 789.303814999992 789.6516710000578 791.6522160000168 792.471581999911 801.4535449999385 811.668508999981 813.0368129999842 818.6193979999516 828.9236910000909 829.2897330000997 829.7693389999913 833.9288769999985 850.5916150000412 853.6531750001013 863.3291579999495 Current Mean: 818.956 ms Stdev: 31.862 ms (3.9%) Runs: 769.1102529999334 769.2755249999464 772.0966700001154 775.3423339999281 784.2331389999017 790.2128280000761 793.1281119999476 793.240928000072 795.4902619998902 800.8987030000426 804.6731120001059 804.8056079999078 805.12126299995 812.9355570001062 814.7001030000392 818.5901299999095 819.7710470000748 820.0607330000494 820.5101870000362 826.1659019999206 827.0008199999575 836.4762949999422 837.4546690001152 839.0735609999392 841.3750209999271 851.3899370001163 862.6370570000727 870.1048620000947 872.1220299999695 874.9991599998903 884.6277620000765
runJsBundle	Baseline Mean: 178.931 ms Stdev: 13.393 ms (7.5%) Runs: 153 156 159 160 162 168 171 172 172 174 175 177 178 179 179 180 181 181 183 184 187 187 190 190 191 191 192 208 209 Current Mean: 188.258 ms Stdev: 15.392 ms (8.2%) Runs: 162 162 166 171 175 176 177 177 178 179 179 179 182 182 184 184 185 188 195 196 197 197 197 200 201 201 207 207 213 215 224
regularAppStart	Baseline Mean: 0.014 ms Stdev: 0.001 ms (9.5%) Runs: 0.012370000127702951 0.012653999961912632 0.012694999924860895 0.012695999932475388 0.012775999959558249 0.012898999964818358 0.01297999988310039 0.01297999988310039 0.012979999999515712 0.012980000115931034 0.012981000007130206 0.013019999954849482 0.01306200004182756 0.013184000039473176 0.013346000108867884 0.013428000034764409 0.013671999797224998 0.013672000030055642 0.013834999990649521 0.01407799986191094 0.014200999867171049 0.01432300009764731 0.014404000015929341 0.014567000092938542 0.014811000088229775 0.015178000088781118 0.01525900000706315 0.015461999922990799 0.015908999950625002 0.017496999818831682 0.01761899993289262 Current Mean: 0.014 ms Stdev: 0.001 ms (4.8%) Runs: 0.01310200011357665 0.013184000039473176 0.013306000037118793 0.013346000108867884 0.013428000034764409 0.013467999873682857 0.013508999953046441 0.013550000032410026 0.013672000030055642 0.013753999955952168 0.013833999866619706 0.01387499994598329 0.013916000025346875 0.013916000025346875 0.0139979999512434 0.014078999869525433 0.014200999867171049 0.014201000100001693 0.014282000018283725 0.014322999864816666 0.014362999936565757 0.014526000013574958 0.0147299999371171 0.014811000088229775 0.01485200016759336 0.014892000006511807 0.014933999860659242 0.015177000081166625 0.01521800016053021 0.015868999995291233
nativeLaunch	Baseline Mean: 9.433 ms Stdev: 1.146 ms (12.1%) Runs: 7 8 8 8 8 8 8 9 9 9 9 9 9 9 9 10 10 10 10 10 10 10 10 10 10 11 11 11 11 12 Current Mean: 9.300 ms Stdev: 0.936 ms (10.1%) Runs: 8 8 8 8 8 8 9 9 9 9 9 9 9 9 9 9 9 9 9 10 10 10 10 10 10 10 11 11 11 11

OSBotify · 2022-12-20T14:23:41Z

🚀 Deployed to staging by @mountiny in version: 1.2.42-1 🚀

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

OSBotify · 2022-12-21T22:57:00Z

🚀 Deployed to production by @yuwenmemon in version: 1.2.42-2 🚀

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

mollfpr · 2022-12-28T14:04:52Z

@luacmartins Could you pull the BZ of the issue to help wrap this up? Thank you!

mountiny · 2022-12-29T22:48:17Z

@mollfpr handling it in the issue, sorry realized it is internal, but we are on track cc @puneetlath

mallenexpensify · 2022-12-30T00:11:50Z

Handling payment via this E/app issue #13896

luacmartins added 3 commits December 13, 2022 09:24

rename fetchIOUReportByID to openIOUPreview

2be5d34

rm setLocalIOUReportData

3c60858

rm unused imports

f7bd614

luacmartins self-assigned this Dec 13, 2022

luacmartins added 3 commits December 13, 2022 09:48

rm fetchIOUReport

dc49401

convert ioupreview to class component

78ebc79

rm export of getSimplifiedIOUReport

a945e87

luacmartins commented Dec 13, 2022

View reviewed changes

Comment thread src/components/ReportActionItem/IOUPreview.js

luacmartins changed the title ~~Refactor fetchIOUReportByID~~ [Hold Web-E #35818] Refactor fetchIOUReportByID Dec 13, 2022

luacmartins added 6 commits December 13, 2022 18:09

rm openIOUPreview

934749e

revert class component

a263501

rm iouReportID prop

8081c50

rm isLoadingReport

e8a0e51

rm unused import

dca62bd

revert prop removal and suppress lint error

25660d0

luacmartins commented Dec 14, 2022

View reviewed changes

luacmartins marked this pull request as ready for review December 14, 2022 21:50

luacmartins requested a review from a team as a code owner December 14, 2022 21:50

melvin-bot Bot requested review from Julesssss and mollfpr and removed request for a team December 14, 2022 21:50

mountiny reviewed Dec 14, 2022

View reviewed changes

mountiny previously approved these changes Dec 15, 2022

View reviewed changes

Julesssss reviewed Dec 15, 2022

View reviewed changes

Comment thread src/components/ReportActionItem/IOUPreview.js

Julesssss previously approved these changes Dec 15, 2022

View reviewed changes

rm unused iou prop

4ba1852

luacmartins dismissed stale reviews from Julesssss and mountiny via 4ba1852 December 15, 2022 17:27

mollfpr previously approved these changes Dec 15, 2022

View reviewed changes

mountiny approved these changes Dec 16, 2022

View reviewed changes

mountiny previously approved these changes Dec 19, 2022

View reviewed changes

resolve conflicts

2623480

luacmartins dismissed stale reviews from mountiny and mollfpr via 2623480 December 19, 2022 18:33

mountiny changed the title ~~[Hold Web-E #35818] Refactor fetchIOUReportByID~~ Refactor fetchIOUReportByID Dec 20, 2022

mountiny approved these changes Dec 20, 2022

View reviewed changes

mountiny merged commit 3c776ee into main Dec 20, 2022

mountiny deleted the cmartins-refactorFetchIOUReportByID branch December 20, 2022 12:12

OSBotify mentioned this pull request Dec 20, 2022

Deploy Checklist: New Expensify 2022-12-20 #13723

Closed

34 tasks

mallenexpensify mentioned this pull request Dec 30, 2022

Payment for E/E #249195 [Refactor] Report.fetchIOUReportByID App API call and clean up syncChatAndIOUReports #13896

Closed

OSBotify added a commit that referenced this pull request Apr 29, 2025

Update Mobile-Expensify submodule to include cherry-picked PR #13556

2617540

Uh oh!

Conversation

luacmartins commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

Uh oh!

luacmartins Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

mountiny Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

melvin-bot Bot commented Dec 14, 2022

Uh oh!

luacmartins commented Dec 14, 2022

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

mountiny Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

mollfpr commented Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Julesssss left a comment

Choose a reason for hiding this comment

Uh oh!

mollfpr commented Dec 15, 2022

Uh oh!

luacmartins commented Dec 15, 2022

Uh oh!

mollfpr commented Dec 15, 2022

Uh oh!

luacmartins commented Dec 15, 2022

Uh oh!

mollfpr left a comment

Choose a reason for hiding this comment

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

mollfpr commented Dec 16, 2022

Uh oh!

mountiny commented Dec 19, 2022

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

luacmartins commented Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Dec 20, 2022

Performance Comparison Report 📊

Significant Changes To Duration

Meaningless Changes To Duration

Uh oh!

OSBotify commented Dec 20, 2022

Uh oh!

OSBotify commented Dec 21, 2022

Uh oh!

mollfpr commented Dec 28, 2022

Uh oh!

mountiny commented Dec 29, 2022

luacmartins commented Dec 13, 2022 •

edited

Loading

mollfpr commented Dec 15, 2022 •

edited

Loading

luacmartins commented Dec 19, 2022 •

edited

Loading