-
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 Wallet_Activate into VerifyIdentity #10053
Conversation
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
…-identity # Conflicts: # src/libs/actions/Wallet.js
Conflicts fixed. On hold till the Web PR goes out to prod but this is ready for reviews @marcochavezf @marcaaron @francoisl! |
Off hold! |
The code LGTM but I can't test it locally because for some reason I'm not getting the
Not sure it's related to the problem, but I got only two addresses in the pre-requisite steps. Also I found an issue that I think is not related to the PR: When I try to fill out the |
I only had two addresses as well, and it worked. Is it possible you didn't mock the response from IDology here, and it failed at that step? Alternatively, you might be able to skip directly to the Onfido step by running this query (replace the bankAccountID with your wallet's ID): UPDATE bankAccounts SET additionalData = JSON_SET(COALESCE(NULLIF(additionalData, ''), '{}'), '$.currentStep', 'OnfidoStep') WHERE bankAccountID = XXX; |
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.
Not sure it's related to the problem, but I got only two addresses in the pre-requisite steps.
Nope, should be all good. I got two addresses as well :)
I can't test it locally because for some reason I'm not getting the Verify Identity page yet.
As @francoisl said, you'll need to mock the response from IDology here and you should be good to go.
Also I found an issue that I think is not related to the PR: When I try to fill out the Additional details (which is the first page that appears when I navigate to /enable-payments) the server error is not displayed in the front-end:
Thanks for catching that! Its probably related to the other refactor PR: https://github.com/Expensify/Web-Expensify/pull/34326. Working on a separate PR to fix that.
src/components/FormAlertWrapper.js
Outdated
<View style={[styles.flexRow, styles.ml2, styles.flexWrap, styles.flex1]}> | ||
{!_.isEmpty(props.message) && props.isMessageHtml && <RenderHTML html={`<muted-text>${props.message}</muted-text>`} />} | ||
class FormAlertWrapper extends React.Component { | ||
renderMessage() { |
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.
This is missing a JSDoc with the @returns
, but would recommend reverting most of these changes. They are unrelated to the current changes (I'm having trouble telling exactly what changed about this file otherwise) and I think we normally recommend to keep as much JSX inline as possible (though I'm not sure if this is established somewhere in the style guide or just clash of personal preference).
Thanks this helped! Also, I found in this SO that I had to enter |
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.
Looks great!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by @francoisl in version: 1.1.89-0 🚀
|
Hold on: https://github.com/Expensify/Web-Expensify/pull/34343
Details
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/218501
Tests
Pre-requisites:
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:
Tests:
/enable-payments
, fill out the additional details formTo mock fixable errors in Onyx:
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
Screenshots
Web
Added in test steps
Mobile Web