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.3k stars 2.74k forks source link

[Payment card / Subscription] make backend return relevant onyxData when changing currency for billing card #45124

Closed MitchExpensify closed 1 month ago

MitchExpensify commented 2 months 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!


Version Number: v9.0.5-10 Reproducible in staging?: Staging Reproducible in production?: If this was caught during regression testing, add the test name, ID and link from TestRail: NA Email or phone of affected tester (no customers): Logs: NA Expensify/Expensify Issue URL: NA Issue reported by: mitch@expensify.com Slack conversation: Internal: https://expensify.slack.com/archives/C036QM0SLJK/p1720560965872139?thread_ts=1720544914.651789&cid=C036QM0SLJK

Action Performed:

  1. Open new.expensify.com/settings/subscription
  2. Add a payment card
  3. Click the 3 dot menu
  4. Click "Change payment currency"
  5. Choose a different currency to the one already saved
  6. Enter the card on file's CVV number
  7. Hit Save

Expected Result:

You should see you payment currency update to the choice made at step 5

Actual Result:

The updated payment currency does not show without hard refresh and a small lag

Workaround:

Wait

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

https://github.com/Expensify/App/assets/36425901/abf5dd75-135e-44ad-b298-a0af513f9613

View all open jobs on GitHub

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @puneetlath (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

MitchExpensify commented 2 months ago

Do you think this will solve the delay here @blimpich ? https://github.com/Expensify/App/issues/44904

dominictb commented 2 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

to the PaymentCardChangeCurrencyForm.

dominictb commented 2 months ago

Proposal updated

dominictb commented 2 months ago

Proposal updated

trjExpensify commented 2 months ago

For sure let's handle this in the same way as https://github.com/Expensify/App/issues/44904 when aligned.

blimpich commented 1 month ago

Do you think this will solve the delay here @blimpich ? https://github.com/Expensify/App/issues/44904

It won't fix it but it'll be fixed in the same way, so we might as well handle them in 1 PR. See this comment.

blimpich commented 1 month ago

Okay so looks like this is a backend bug. I briefly looked into this and its one of those problems that looks fairly straightforward at first but is more complicated than it seems.

When we call UpdateBillingCardCurrency we aren't passing back any onyx data to the frontend, so the currency doesn't update unless the client does a full refresh, which then triggers a OpenSubscriptionPage call which gets the updated fund. So naturally we could just add the onyx data to the auth command SetFundCurrency, which is the Auth command that is triggered here, but its really just using CreateFund under the hood. So fixing this "correctly" means moving Onyx updates from UserApi::addPaymentCard in Web to CreateFund.cpp, and then making sure that both adding a new payment card and changing the currency correctly update the onyx data and return it. Not insane stuff, but slightly more complicated than you would expect.

Marking this as an internal hotpick so that someone can pick up soon.

dominictb commented 1 month ago

@blimpich Beside of the BE bug, this issue still need to implement solution to fix the original issue: The updated payment currency does not show without hard refresh and a small lag.

blimpich commented 1 month ago

I think yes we will still need some frontend work, but lets come back to that once an Expensify engineer has fixed the backend bug. We'll handle this separately since this we have to wait for an Expensify engineer to have time to fix this.

dominictb commented 1 month ago

@blimpich Can you re-assign me to this issue for easier to self keep track? Then I will create a PR to fix once the BE is fixed. Thanks

blimpich commented 1 month ago

@blimpich Can you re-assign me to this issue for easier to self keep track? Then I will create a PR to fix once the BE is fixed. Thanks

Well actually this doesn't make sense really. This issue is now a backend issue. I'll create a separate frontend issue for making it wait for the request to finish.

blimpich commented 1 month ago

Got an auth PR up to fix this. Will also need to get a web PR out to clean up onyx updates that are no longer needed in web. In review.

melvin-bot[bot] commented 1 month ago

@blimpich Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 1 month ago

@blimpich Still overdue 6 days?! Let's take care of this!

blimpich commented 1 month ago

Looks like web and auth PRs have been deployed. Closing.