Skip to content
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

Merged
merged 153 commits into from
May 18, 2021
Merged

IOU Details Modal #2394

merged 153 commits into from
May 18, 2021

Conversation

Julesssss
Copy link
Contributor

@Julesssss Julesssss commented Apr 14, 2021

Details

  • Introduces the IOU Details Modal, which allows users to view both active and settled IOU Reports and to settle them. Users can open IOU reports by tapping 'Pay' or 'View Details' from a chat with an outstanding IOU report
  • I have also split 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 components

Not included in this PR

  • Rejecting transactions
  • Displaying the transaction canceled/declined message
  • Displaying the IOU settled message

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

  1. Create an IOU Request of any amount, you should see the following, with 'user owes user' message in the Preview Component

Screenshot 2021-05-18 at 18 42 48

  1. As the creator, click 'View Details' on the Preview component that is displayed
  2. The Details modal should be displayed

Screenshot 2021-05-18 at 18 46 36

  1. You should not see a green 'I'll settle up elsewhere' button
  2. Now login to the receiving user, navigate to the same chat report
  3. you should see the following Preview Component

Screenshot 2021-05-18 at 18 43 27

  1. click on 'Pay' AND 'View Details', both should launch the Details Modal

Screenshot 2021-05-18 at 18 45 44

  1. Tap the I'll settle up elsewhere button, the button should show a loading indicator, and eventually disappear

Screenshot 2021-05-18 at 18 54 03

  1. The Preview message should now read 'user paid user' and the 'Ill settle up elsewhere' button should not be displayed

Screenshot 2021-05-18 at 18 54 12

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
Screenshot 2021-05-18 at 19 00 47

  1. Tap the 'View Details' button
  2. You should see a loading indicator while the paid report is retrieved

Screenshot 2021-05-18 at 19 10 41

  1. It should then display, without the green 'I'll settle up elsewhere' button

Screenshot 2021-05-18 at 19 11 52

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')
Screenshot 2021-05-18 at 19 12 51

  1. You SHOULD NOT see a 'View Details' link beneath the message
    (notice there is no View Details link)

QA Steps

  • Run the above tests

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screenshot 2021-05-10 at 15 13 01

Mobile Web

Simulator Screen Shot - iPhone 11 - 2021-05-10 at 15 46 38

Desktop

Screenshot 2021-05-10 at 15 15 24

iOS

Simulator Screen Shot - iPhone 11 - 2021-05-10 at 16 51 46

Android

Screenshot_1620662061

@Julesssss Julesssss self-assigned this Apr 14, 2021
@Julesssss Julesssss force-pushed the jules-iouDetailsModal branch 2 times, most recently from 5e73888 to 3533bd5 Compare April 20, 2021 13:50
@Julesssss Julesssss force-pushed the jules-iouDetailsModal branch from ccf9b7f to f15262e Compare April 26, 2021 12:44
NikkiWines
NikkiWines previously approved these changes May 18, 2021
Copy link
Contributor

@NikkiWines NikkiWines left a 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.

@NikkiWines
Copy link
Contributor

Actual: Sometimes only the report action quoted text is displayed and the preview component does not display at all.

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)

@Julesssss
Copy link
Contributor Author

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 😩

@Julesssss
Copy link
Contributor Author

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

@marcaaron
Copy link
Contributor

Even then, they would fail 50% of the time

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?

@marcaaron
Copy link
Contributor

When testing for the sporadic issue where Preview component wouldn't display -- it would always be associated with a failed network request, usually 555.

I'll look for this when re-testing. Didn't see anything like that in the first go.

@Julesssss
Copy link
Contributor Author

Screen Shot 2021-05-18 at 7 35 41 AM

In this example, the IOU was sent by test account to Marcus and then paid by Marcus but the preview component says that the tester paid Marcus.

Finally, I also believe this issue is due to a failed request, as the report hasn't updated locally. Could you please try this again?

@Julesssss
Copy link
Contributor Author

Julesssss commented May 18, 2021

As you can imagine, it has been an absolute nightmare to develop against an API that failed 30% of the time

@marcaaron
Copy link
Contributor

Sorry I re-tested and it's still not correct

Screen Shot 2021-05-18 at 8 43 37 AM

Screen Shot 2021-05-18 at 8 43 21 AM

@marcaaron
Copy link
Contributor

Also observed the inconsistency again during re-test + no errors at all in the networking tab

CreateIOUTransaction succeeded

Screen Shot 2021-05-18 at 8 47 12 AM

Get&rvl=reportStuff succeeded
Screen Shot 2021-05-18 at 8 47 48 AM

No preview component appears.

@marcaaron
Copy link
Contributor

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.

@Julesssss
Copy link
Contributor Author

@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}
Copy link
Contributor

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

  1. 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).
  2. Rather than having two methods (fetchIOUReportByID and fetchIOUReportByIDAndUpdateChatReport), add a third parameter to this method called shouldUpdateChatReport and move the logic into this method.
  3. 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).

Copy link
Contributor Author

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!

@marcaaron marcaaron requested review from NikkiWines and marcaaron May 18, 2021 23:56
@marcaaron marcaaron merged commit fc080b7 into main May 18, 2021
@marcaaron marcaaron deleted the jules-iouDetailsModal branch May 18, 2021 23:56
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@kavimuru
Copy link

kavimuru commented May 19, 2021

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:

  1. Lunch the app
  2. Log in with expensifail account
  3. Select any user
  4. Click on plus button and click Request Money
  5. Put amount and click next
  6. As the creator, click 'View Details' on the Preview component that is displayed.

Platform:

Web ✔️
Desktop ❌
iOS ✔️
mWeb ✔
Android ✔️

Build:

1.0.49-0

Notes/Images/Video:

Bug5076215_Screen_Recording_20210519-140913_Expensifycash.mp4

@isagoico isagoico added the DeployBlockerCash This issue or pull request should block deployment label May 20, 2021
@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@Julesssss
Copy link
Contributor Author

Julesssss commented May 20, 2021

Created a new issue for this bug, but it was already fixed as part of this IOU PR

I'm removing the deploy blocker label, as this feature is behind a beta and was not working prior to this PR - so nothing has really changed.

@Julesssss Julesssss removed the DeployBlockerCash This issue or pull request should block deployment label May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants