-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IOU Details Modal #2394
IOU Details Modal #2394
Conversation
5e73888
to
3533bd5
Compare
ccf9b7f
to
f15262e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retested and didn't experience either of the errors that @marcaaron ran into. Looks good.
FWIW, I've experienced this sporadically as well but it seems to be tied to VM slowness from what I could tell - though I didn't investigate further than to retry several times to see if I could repro consistently (which I couldn't) |
I'm certain this is the case. During development I would wait until all network requests had completed before creating a transaction or settling a report. Even then, they would fail 50% of the time 😩 |
@marcaaron I've fixed the LoadingIndicator issue, so you should now see a loading icon when a paid report is loaded. Also, I've rewritten the tests to simplify them and added screenshots for reference. Would you mind retesting, thanks 🙏 When testing for the sporadic issue where Preview component wouldn't display -- it would always be associated with a failed network request, usually 555. |
So the network request returned 200 but failed in some other way? In the cases where I observed this the network requests eventually returned 200. So it made me think that the UI is not updating correctly or dependent on something happening "fast". But maybe that's not the case? |
I'll look for this when re-testing. Didn't see anything like that in the first go. |
As you can imagine, it has been an absolute nightmare to develop against an API that failed 30% of the time |
Sorry, I know it's been a long review @Julesssss. To clarify, I am OK with merging this after we fix the incorrect text that shows who owes/paid who. If the inconsistency surfaces in production we can address it. Seems like I am the only one experiencing that particular bug. |
@marcaaron I've spent some time looking into the Preview issue you mentioned. I can reproduce it with new conversations (the creator will not see the Preview Component, but the receiver will). I'm unable to make any further progress so have had to separate this out to a new issue. I've reversed the user 'paid' user message (I accidentally reversed this when implementing translations 😩) |
* | ||
* @param {Number} iouReportID - ID of the report we are fetching | ||
* @param {Number} chatReportID - associated chatReportID, set as an iouReport field | ||
* @returns {Promise} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm definitely not a fan of this promise being returned, and I think there are several other patterns that we have which can remove the need to return the promise here. For sake of #urgency, I am fine merging this as-is and then cleaning it up later. Here are some of the solutions that I think would ultimately make the code cleaner (in order of preference).
- Solve more of the issues in the API layer rather than forcing the front-end to do a lot of work to keep all the data happy. For example, rather than expecting the front-end to try and keep the reportIDs in sync, the server should push data to e.cash that is just put straight into Onyx using pusher (this was the original dream).
- Rather than having two methods (
fetchIOUReportByID
andfetchIOUReportByIDAndUpdateChatReport
), add a third parameter to this method calledshouldUpdateChatReport
and move the logic into this method. - Have a callback connected to Onyx which looks for changes to the IOU report (eg. like after it's fetched from the server) and then it does the appropriate actions to keep the reportIDs in sync. This is similar to the pattern that we have for fetching new report data (here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair points. The original PR used suggestion #2, but this led to other issues as the logic became quite complex and I switched to two functions in an attempt to make the code simpler to understand.
I am already planning to do #1, but I have added #3 to the post-launch improvements on the IOU tracker as this seems a worthy improvement.
Thanks for the review!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
White screen displayed after clicking 'View Details"Expected Result:The Details modal should be displayed Actual Result:White screen displayed after clicking 'View Details" Actions Performed:
Platform:Web ✔️ Build:1.0.49-0 Notes/Images/Video:Bug5076215_Screen_Recording_20210519-140913_Expensifycash.mp4 |
👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open
|
Details
ReportActionItemIOUPreview
into two distinct components so that we can reuse them individually within IOU Details.ReportActionItemIOUAction
now sits above...IOUPreview
and...IOUQuote
and selectively displays one or both of these componentsNot included in this PR
Fixed Issues
Fixes https://github.com/Expensify/Expensify/issues/158811
Fixes https://github.com/Expensify/Expensify/issues/163036
Tests
NOTE: If you are unable to build to iOS and are on the latest OSX version, you should disable Flipper locally
NOTE: You should test against the latest web and Auth commits
NOTE: IOU Report logic has recently changed, you should create new IOUs for testing
NOTE: This can occasionally be flakey on dev, due to requests timing out -- before creating a transaction, or paying an iouReport, please clear the network tab in devtools and ensure that the request succeeds_
A) Test the View Details and Pay opens the IOU
B) Test that paid reports are displayed correctly
10. Log out, and back into the app as either user
11. To avoid an external issue, refresh the page
12. Navigate to a chatReport with a paid IOU, you should see a 'Settled up elsewhere' message
C) Test that View Details is not displayed for group Bill Splits
16. Create a New Group from the global menu
17. WIthin this new group, click the + icon and select 'Split Bill'
18. Create an IOU Split of any amount
19. Locate the message that is generated ('Split $2.00 with Jules... User and User10')
(notice there is no View Details link)
QA Steps
Tested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android