-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Refactor part of ActivateWallet into UpdatePersonalDetailsForWallet #9914
Conversation
Please explain why the PR is on HOLD in the PR description or a comment 🙂 |
Yep! I'll have that PR ready tomorrow, hopefully! |
is it ready for review or are you still working on the PR? |
Ready! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments. Rest LGTM i think.
I think you may be right, yeah. Good catch! |
Comments addressed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Triggered auto assignment to @youssef-lr ( |
@chiragsalian looks like this was merged without passing tests. Please add a note explaining why this was done and remove the |
Hmm, melvin derping. Not emergency and travis tests had passed. Looks like internalQA assignment is firing too early. Removing Youssef. |
@Gonals @chiragsalian @MariaHCD Can this PR be QAed internally? We do not have a valid account with wallet to run it due to PROD KYC wall |
It should be! I set the |
@francoisl Hmm, I'm assuming that you're on step 4 in the test steps? I think we can just navigate to |
Ok yeah good idea, though in this case I'm hitting this error (which makes sense since the account is not supposed to have a wallet...) For the record, here's the
@Gonals can you double-check the QA steps are accurate please? |
Hmm. Does the account have a debit card added from oldDot? That's what enabled the option in dev for mine |
I'm not sure this is right, every account has a wallet. In this case, I think we want a wallet that's in the pending state (i.e. not yet KYC'd and activated) |
Updated the wording! |
@francoisl how is the QA going, just keeping track for the release here |
Still getting the same That said though, do we really need to make a money request to someone first? Or to simplify, can we just ask someone to navigate to |
Details
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/218499
Tests
Ready? This is a pain to test! Gotta be tested with https://github.com/Expensify/Web-Expensify/pull/34326
Run the following commands:
Each one will return one or more
address
fields. For each of those, run:For the three addresses I got back, this means the actual commands I run are:
Now, you'll need to mockup a successful response from Idology.
Now you can start testing! From an account with a Wallet, on NewDot, request money from another account that does NOT have a wallet yet.
Log into the walletless account and try to pay (you may need to add a debit card first for the "Pay with Expensify" button to show up). You'll be taken to the additional details page:
https://github.com/Expensify/Web-Expensify/pull/34326/files#diff-e4dfb4153823fd1c467ff673d4bfb99adca4b88ee20e20ba73ca86fc2fcc8433R1069
Fill the details with mockup stuff. Chances are the SSN number will fail and it will ask for a full number:
!
here for$didPass
to mockup the questions behavior. If you got through the additional details steps again, you should get a few questions like these:!
for the question count here. Repeat the steps again and it should take you to:And I think that should be enough!
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)PR Reviewer Checklist
The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
have been tested & I retested again)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)QA Steps
This needs to be QAed from the US, I believe.
From an account with a fully authenticated Wallet, on NewDot, request money from another account that does NOT have a verified wallet yet (so the wallet is in a pending state). All accounts on expensify/expensifail should have a pending wallet. You'll need both of them to be in the
wallet
beta and thepayWithExpensify
betaOn OldDot, log into the account with the pending wallet and go to Settings>Account>Payments and add a debit card. You can use one of these test cards.
Log into the account with the pending wallet on NewDot and try to pay (you may need to add a debit card first for the "Pay with Expensify" button to show up). You'll be taken to the additional details page:
https://github.com/Expensify/Web-Expensify/pull/34326/files#diff-e4dfb4153823fd1c467ff673d4bfb99adca4b88ee20e20ba73ca86fc2fcc8433R1069
Fill the details with mockup stuff. Chances are the SSN number will fail and it will ask for a full number:
Screenshots
Web
Mobile Web
Desktop
iOS
Android