search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.5k stars 2.85k forks source link

IOU - Pay button is visible for a moment after cancel requested amount. #12781

Closed kbecciv closed 1 year ago

kbecciv commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Issue found when executing PR https://github.com/Expensify/App/pull/12488

Action Performed:

  1. Launch the app
  2. Log in with any account
  3. Send a money request in USD while being online, e.g. $100.
  4. Send a second money request in a different currency, e.g. 100 AED.
  5. Go offline, cancel the request in USD
  6. Go online.
  7. Cancel the request in AED

Expected Result:

Pay button is not visible for a moment after cancel requested amount.

Actual Result:

Pay button is visible for a moment after cancel requested amount.

Workaround:

Unknown

Platform:

Where is this issue occurring?

Version Number: 1.2.28.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/93399543/202238511-64437469-d74d-475e-9f69-326382ffeb6c.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @sophiepintoraetz (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @techievivek (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

sophiepintoraetz commented 1 year ago

Though cc @youssef-lr as you're in the linked PR this stemmed from.

youssef-lr commented 1 year ago

Thanks for the ping @sophiepintoraetz - I've reverted my PR locally and I can still reproduce this, so I don't think this is a regression bug, but just something we didn't notice before. The steps to reproduce this is simply to request money while online using a different currency than the report's, then cancel the request. I may have a fix for this one, looking into it today.

youssef-lr commented 1 year ago

I think we may need to cancel updating the IOU if the IOU's currency is different than the report's currency all together, regardless of whether or not the user is offline.

The issue here is that say we request 100 MAD from a user, and the default report's currency is USD, that's about $11. If we cancel the 100 MAD, in the code this amount is not converted to USD, so we're subtracting 100 from 11 which becomes -$89, then the IOU manager/owner is switched here and we see the "Pay" button. Until new data comes from the backend and the IOU gets updated properly.

Since this is affecting only the online behavior, where we will always have data coming back from the backend, I'd say we can cancel making updates to the IOU in the client and rely on the backend only.

~If we absolutely need this for offline behavior, another solution I can think of is to store currency exchange values in Onyx and use them when we're offline.~

@JmillsExpensify what do you think?

JmillsExpensify commented 1 year ago

Gotcha, so I think what we're essentially discovering here is that we can't reliably Cancel in scenarios where an IOU has multiple currencies – just like we recently discovered for Pay. The pickle with the "Pay" is that it's possible to accidentally pay something for an amount you didn't expect. That said, a couple of questions:

P.S. We looked into storing currencies in the App briefly and there was a huge performance it, so I tend to agree that it's not a realistic solution for now.

youssef-lr commented 1 year ago

Isn't Cancel only an issue when both currencies were added online, but then one of them is canceled offline

The issue here is cancelling a request while online, and when the request's currency is different from the report's. e.g. cancelling 100 AED from a report that shows $27, it will actually subtract 100 from 27 which will flip the IOU status and show a Pay button temporarily. If you notice in the screen recording in this issue, around the 1:32 second mark, the request was cancelled online.

Related, if you Cancel offline, I thought we already agreed that if an IOU has multiple currencies that the total would not be updated offline? So maybe I'm missing why Cancel should also be disabled while offline?

Correct, cancelling offline works fine and there is no need to disable it.

youssef-lr commented 1 year ago

Here's a recording of the bug:

https://user-images.githubusercontent.com/9680864/202515139-1d188c8a-7723-4172-839a-46ed24b58dbf.mov

trjExpensify commented 1 year ago

I'm kind of confused. The steps in the OP are not the same as the steps in the OP video. Here's the flow following the video:

  1. Launch the app
  2. Log in with any account
  3. Send a money request in USD while being online, e.g. $100. (no prior balance owed between either parties on the report)
  4. Go offline
  5. Send a second money request in a different currency, e.g. 100 AED. (The report total doesn't update and shows pending) - ✅ correct
  6. Go online, the report total updates with the value of the second request (e.g $127.23 is owed by the other user) - ✅ correct
  7. Go offline again
  8. Cancel the $100 request in USD. The report total updates and shows pending. (e.g $27.23 is owed by the other user) - ✅ correct
  9. Go online again, the report total remains as $27.23 owed by the other user - ✅ correct
  10. Cancel the 100 AED request online
  11. The report total briefly shows you now owe $72.77 with the Pay button - ❌ incorrect
  12. Then updates (well, reverts?) to $27.23 owed - ❌ incorrect
  13. Then $0 and the report preview is removed as the balance is equal again. - ✅ correct

So the question I have is what is happening to make the report total go from $27.23 to $72.77 when cancelling a request online? Before reverting back to $27.23, before cancelling it to bring the report total to $0.

youssef-lr commented 1 year ago

The report total briefly shows you now owe $72.77 with the Pay button - ❌ incorrect

That's the code in the client (incorrectly) subtracting 100 AED from $27.23 = -$72.77

Then updates (well, reverts?) to $27.23 owed - ❌ incorrect

Not sure what's happening here either. I didn't see this in my testing.

Then $0 and the report preview is removed as the balance is equal again. - ✅ correct

Correct data was fetched from the backend.

trjExpensify commented 1 year ago

That's the code in the client (incorrectly) subtracting 100 AED from $27.23 = -$72.77

Why is it doing that? That must be a regression we introduced somewhere?

Do we agree, this is the expectation?

JmillsExpensify commented 1 year ago

Can we bring this discussion to Slack instead please.

JmillsExpensify commented 1 year ago

Let's create a thread in open source.

youssef-lr commented 1 year ago

Let's create a thread in open source.

Sounds good, I will just quickly answer Tom's question here.

Why is it doing that? That must be a regression we introduced somewhere?

For clarification, everything is saved correctly in the backend, issue here is just the temporary visual glitch related to how we instantly update the IOU in the client before fresh data is fetched from the backend, which displays the IOU incorrectly.

sophiepintoraetz commented 1 year ago

Pulled out here

techievivek commented 1 year ago

I was ooo catching up now. Thanks

techievivek commented 1 year ago

This issue seems to have been solved by the PR https://github.com/Expensify/App/pull/12960 @youssef-lr Can you confirm it? So I guess we can focus on displaying the calculating state wherever required.

youssef-lr commented 1 year ago

@techievivek Correct, this has been fixed and I think we can close this out and create an issue to work on the design and copy of the pending/calculating state.

sophiepintoraetz commented 1 year ago

Happy to close this one out and open up a design ticket - just point me in the right direction - which part are we thinking about showing a loading screen? We kind of already do (when looking back on the video)

image
techievivek commented 1 year ago

@sophiepintoraetz The screenshot that you have shared is when the user is offline. This one would be when we added a new IOU request where the currency is different from the report's currency in that case we wait for the response from the backend. And during this period of waiting, we would like to show a calculating state.

techievivek commented 1 year ago

Closing this issue as I have created a new issue to add the calculating state for IOU requests.