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

Fix: reset the wallet balance when transfer is successful #7681

Merged
merged 1 commit into from
Feb 14, 2022

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Feb 10, 2022

Details

Fixed Issues

$ #7626

Tests | QA Steps

  1. Go to Settings->Payment -> Transfer balance page.
  2. Select the Mode and account.
  3. Click Transfer balance.
  4. Confirm that you are taken back to the Payments Page, a confirmation modal is shown and the wallet balance is set to 0.

To test locally

  1. Apply this patch
diff --git a/src/libs/actions/PaymentMethods.js b/src/libs/actions/PaymentMethods.js
index fd9fa25be..59bff94b8 100644
--- a/src/libs/actions/PaymentMethods.js
+++ b/src/libs/actions/PaymentMethods.js
@@ -194,6 +194,10 @@ function transferWalletBalance(paymentMethod) {
         [paymentMethodIDKey]: paymentMethod.methodID,
     };
     Onyx.merge(ONYXKEYS.WALLET_TRANSFER, {loading: true});
+    Onyx.merge(ONYXKEYS.USER_WALLET, {currentBalance: 0});
+    Onyx.merge(ONYXKEYS.WALLET_TRANSFER, {shouldShowConfirmModal: true, loading: false});
+    Navigation.navigate(ROUTES.SETTINGS_PAYMENTS);
+    return;
 
     API.TransferWalletBalance(parameters)
         .then((response) => {
diff --git a/src/libs/actions/Wallet.js b/src/libs/actions/Wallet.js
index adb53b3e2..e25ef174c 100644
--- a/src/libs/actions/Wallet.js
+++ b/src/libs/actions/Wallet.js
@@ -178,7 +178,7 @@ function fetchUserWallet() {
                 return;
             }
 
-            Onyx.merge(ONYXKEYS.USER_WALLET, response.userWallet);
+            Onyx.merge(ONYXKEYS.USER_WALLET, {...response.userWallet, currentBalance: 12345});
         });
 }
  1. Go directly to http://localhost:8080/settings/payments/transfer-balance
  2. Follow the previous testing/QA steps.
  • Verify that no errors appear in the JS console

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web | Desktop

screen-2022-02-13_01.36.41.mp4

Mobile Web

screen-2022-02-13_01.34.08.mp4

iOS

Android

@parasharrajat parasharrajat requested a review from a team as a code owner February 10, 2022 19:47
@MelvinBot MelvinBot requested review from NikkiWines and removed request for a team February 10, 2022 19:47
@NikkiWines
Copy link
Contributor

@parasharrajat please include steps for how to get a non-zero balance as well so that this can be tested using a fresh account

@parasharrajat
Copy link
Member Author

parasharrajat commented Feb 11, 2022

@NikkiWines I have added the testing steps. I don't know the way to load the wallet.

@NikkiWines
Copy link
Contributor

@NikkiWines I have added the testing steps

Thank you!

I don't know the way to load the wallet.

No worries, I would just add a step 1 for QA steps that they should log into an account with a non-zero wallet balance.

@NikkiWines
Copy link
Contributor

Please let me know if You want me to attach clips for those tests.

If possible, I do think it's good to attach clips of the tests for mweb, android, ios

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.

Works well! Just missing some screenshots, will approve once those are added

@parasharrajat
Copy link
Member Author

I have added the test clips for web| Desktop| Mobile web. I can't open the transfer page directly on Android| IOS due to KYCWall. But I think they are not needed. The change is small and clear.

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.

👍

@NikkiWines NikkiWines merged commit 0b1ae30 into Expensify:main Feb 14, 2022
@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.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @NikkiWines in version: 1.1.39-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@mvtglobally
Copy link

@NikkiWines We are not able to QA Transfer Balance functionality due ty KYC wall. Can you validate this internally?

@NikkiWines
Copy link
Contributor

NikkiWines commented Feb 15, 2022

Asking @joekaufmanexpensify since it looks like his account is the one being used in the video shown in the issue

@joekaufmanexpensify
Copy link
Contributor

My balance was already transferred, but sent Nikki $1 to validate that this works when attempting a balance transfer.

@NikkiWines
Copy link
Contributor

Confirms the fix works as anticipated 🎉 Ty @joekaufmanexpensify for the help!

Screen.Recording.2022-02-15.at.14.55.04.mov

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @timszot in version: 1.1.39-3 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants