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

[HOLD for payment 2022-03-02] After you transfer your balance the wallet page doesn't automatically update #7626

Closed
mvtglobally opened this issue Feb 8, 2022 · 21 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@mvtglobally
Copy link

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


Action Performed:

  1. From account A transfer balance to Account B
  2. Login to account B and check balance
  3. Refresh

Expected Result:

Account balance should update after transfer is made

Actual Result:

After you transfer your balance the wallet page doesn't automatically update

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.1.36-0
Reproducible in staging?:
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

2022-02-07_15-16-21.mp4

Expensify/Expensify Issue URL:
Issue reported by: @puneetlath
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1643563169531119

View all open jobs on GitHub

@mvtglobally mvtglobally added AutoAssignerTriage Auto assign issues for triage to an available triage team member Engineering Daily KSv2 labels Feb 8, 2022
@MelvinBot
Copy link

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

@MelvinBot
Copy link

Triggered auto assignment to @zanyrenney (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Feb 8, 2022
@zanyrenney zanyrenney removed their assignment Feb 8, 2022
@zanyrenney
Copy link
Contributor

triage complete -- looks like they already tagged with eng, so not reapplying the label again now.

@bondydaa
Copy link
Contributor

bondydaa commented Feb 8, 2022

Hmm I'm not super familiar with this code to know off hand if this is a client issue (maybe onyx isn't being updated properly) or an API issue (maybe we're not returning the updated balance properly) so I can't tell if this is external or not. I'll take a look and see what I can find out.

@parasharrajat
Copy link
Member

There are a couple of issues here.

  1. First, no confirmation modal is shown after completion.
  2. Second, balance is not updated.

@bondydaa
Copy link
Contributor

bondydaa commented Feb 8, 2022

alrighty looks like the API doesn't return anything back to the client https://github.com/Expensify/Web-Expensify/blob/5988e0dc06223c766c5f57b4eea4773963802654/api.php#L2279-L2280

Personally I think we probably should just update the API to return the remaining balance so that we don't have to then just make another call to the API to get the balance but 🤷 seems like it was built this way on purpose.

I think the bug is that here:

Onyx.merge(ONYXKEYS.USER_WALLET, {balance: 0});
we set the balance key on the USER_WALLET but looking at the rest of the code we refer to currentBalance when trying to render the balance:

Expensidev/App (main) $ ack -Q .currentBalance
src/components/CurrentWalletBalance.js
43:        props.userWallet.currentBalance / 100, // Divide by 100 because balance is in cents

src/pages/settings/Payments/TransferBalancePage.js
177:        const calculatedFee = PaymentUtils.calculateWalletTransferBalanceFee(this.props.userWallet.currentBalance, selectedPaymentType);
178:        const transferAmount = this.props.userWallet.currentBalance - calculatedFee;

src/pages/settings/InitialSettingsPage.js
125:        props.userWallet.currentBalance / 100, // Divide by 100 because balance is in cents

@bondydaa
Copy link
Contributor

bondydaa commented Feb 8, 2022

looks like it was added in this PR #6878

@parasharrajat
Copy link
Member

Oh, yeah. You are correct. I added this code and this should be currentBalance.

@bondydaa bondydaa removed their assignment Feb 8, 2022
@bondydaa bondydaa added the External Added to denote the issue can be worked on by a contributor label Feb 8, 2022
@MelvinBot
Copy link

Triggered auto assignment to @kadiealexander (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@bondydaa
Copy link
Contributor

bondydaa commented Feb 8, 2022

coolio throwing the external label and un-assigning myself, fix seems simple enough 👍

@parasharrajat
Copy link
Member

parasharrajat commented Feb 8, 2022

Proposal

  1. Change the
    Onyx.merge(ONYXKEYS.USER_WALLET, {balance: 0});
    to set currentBalance to 0.

Confirmation Modal is shown correctly. The linked video was recorded on the web info

This fixes the linked issue.

@kadiealexander
Copy link
Contributor

Posted to Upwork! https://www.upwork.com/jobs/~012f263a77525f0ae0

@botify botify removed the Daily KSv2 label Feb 8, 2022
@MelvinBot MelvinBot added the Weekly KSv2 label Feb 8, 2022
@MelvinBot
Copy link

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (Exported)

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 8, 2022
@MelvinBot
Copy link

Triggered auto assignment to @NikkiWines (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@parasharrajat
Copy link
Member

I would like to fix this via proposal @NikkiWines

@NikkiWines
Copy link
Contributor

Yep, @parasharrajat's proposal looks good! 👍

@parasharrajat
Copy link
Member

PR will be live shortly.

@NikkiWines NikkiWines removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 10, 2022
@NikkiWines
Copy link
Contributor

@kadiealexander can you hire @parasharrajat for this job please. TY!

@NikkiWines NikkiWines added the Reviewing Has a PR in review label Feb 10, 2022
@kadiealexander
Copy link
Contributor

Hired! Thanks @parasharrajat :)

@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Feb 23, 2022
@botify botify changed the title After you transfer your balance the wallet page doesn't automatically update [HOLD for payment 2022-03-02] After you transfer your balance the wallet page doesn't automatically update Feb 23, 2022
@botify
Copy link

botify commented Feb 23, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.39-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2022-03-02. 🎊

@kadiealexander
Copy link
Contributor

Paid! Thanks @parasharrajat 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants