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.

Sign up for GitHub

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

Jump to bottom

Don't dismiss the Reimbursement Account Page after validating a withdrawal account #5803

Merged
merged 4 commits into from
Oct 13, 2021
Merged

Don't dismiss the Reimbursement Account Page after validating a withdrawal account #5803

merged 4 commits into from
Oct 13, 2021

Conversation

yuwenmemon
Copy link
Contributor

@yuwenmemon yuwenmemon commented Oct 13, 2021 •
edited
Loading

@luacmartins please review

Details

Make sure we don't close the Reimbursement Account Page after validating a withdrawal account, regardless if you have the Expensify Card or not.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/180851

Tests

  1. Create workspace
  2. Go to connect bank account page
  3. Select Connect bank account and use steps in option (1) of this SO
  4. Enter validation amounts to finalize setup
  5. Make sure that the right docked modal stays open after you finish the setup.
    Kapture 2021-10-13 at 11 32 21

@yuwenmemon yuwenmemon self-assigned this Oct 13, 2021
@yuwenmemon yuwenmemon requested a review from a team as a code owner October 13, 2021 06:44
@MelvinBot MelvinBot requested review from MariaHCD and removed request for a team October 13, 2021 06:45
MariaHCD
MariaHCD previously approved these changes Oct 13, 2021
View reviewed changes
@trjExpensify
Copy link
Contributor

Can we add the CP Staging label to this, as it's for the N6 launch in a couple of days?

luacmartins
luacmartins requested changes Oct 13, 2021
View reviewed changes
Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM and it tested well. Could we just address some of the later comments in the issue?

  1. Remove growl
  2. Change Save & continue button to Finish setup

@github-actions
Copy link
Contributor

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

@yuwenmemon
Copy link
Contributor Author

Updated with the new changes. Observable in new animation.

luacmartins
luacmartins approved these changes Oct 13, 2021
View reviewed changes
Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for this!

@luacmartins luacmartins merged commit 27b23b5 into main Oct 13, 2021
@luacmartins luacmartins deleted the yuwen-betterSteps branch October 13, 2021 19:23
github-actions bot pushed a commit that referenced this pull request Oct 13, 2021
@luacmartins
Merge pull request #5803 from Expensify/yuwen-betterSteps
4a89a32 
Don't dismiss the Reimbursement Account Page after validating a withdrawal account

(cherry picked from commit 27b23b5)
This was referenced Oct 13, 2021
Merged
Closed
@mvtglobally
Copy link

mvtglobally commented Oct 13, 2021 •
edited
Loading

@yuwenmemon @MariaHCD @luacmartins
we will not be able to QA this on our side as it requires validation amounts from account. Can you please run it internally?

@yuwenmemon
Copy link
Contributor Author

Ah correct this is internalQA, apologies about that!

@kevinksullivan
Copy link
Contributor

kevinksullivan commented Oct 13, 2021 •
edited
Loading

Worked on staging! Selected Finish setup and then landed on the All set page after the account was added.

image

@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by @luacmartins in version: 1.1.7-10 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@OSBotify OSBotify mentioned this pull request Oct 15, 2021
Closed
@OSBotify
Copy link
Contributor

🚀 Deployed to production by @AndrewGable in version: 1.1.7-24 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @luacmartins in version: 1.1.7-25 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.8-9 🚀

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

@OSBotify OSBotify mentioned this pull request Aug 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luacmartins luacmartins luacmartins approved these changes

@MariaHCD MariaHCD MariaHCD left review comments

Assignees

@yuwenmemon yuwenmemon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@yuwenmemon @trjExpensify @mvtglobally @kevinksullivan @OSBotify @MariaHCD @luacmartins