-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Allow first and last name in send money additional details to be completely deleted and refactor to use Form #13170
Conversation
@NikkiWines Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
…itionalDetailsDraft prop
…efault values that aren't necessary
@Beamanator fixed it and tested to make sure empty values in the Address Street box show an error: |
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.
Looking great!
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-01-06.at.9.55.26.AM.movMobile Web - ChromeScreen.Recording.2023-01-06.at.10.06.04.AM.movMobile Web - SafariScreen.Recording.2023-01-06.at.10.20.55.AM.movDesktopScreen.Recording.2023-01-06.at.10.08.46.AM.moviOSScreen.Recording.2023-01-06.at.10.15.29.AM.movAndroidScreen.Recording.2023-01-06.at.10.03.12.AM.mov |
@NikkiWines @roryabraham let me know if you're able to take a look at this. It would be great to get this one wrapped up. |
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 good, once question about the error message for the 9-digit SSN entry. Should we see the error on the SSN field as soon as 9-digits are required? Or only after they click on-and-away from the field? Currently it's the latter. If that's the expected behavior lemme know and I'll approve
Screen.Recording.2023-01-09.at.14.44.48.mov
@NikkiWines I believe what you are seeing is the desired behavior. The error message first comes up over the submit button letting the user know to add a 9 digit SSN, and the SSN field changes to say "Full 9 digits of SSN". Then, after the user clicks in and out of the SSN box without adding a 9 digit SSN we show the error on the SSN box. |
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.
I haven't tested this so I'll leave it to @NikkiWines to formally approve, but I looked through this code and didn't notice anything that looked wrong. Overall it looks like a great simplification.
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.
Actually I guess I have to approve so it doesn't appear as Requested Changes
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 PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
|
🚀 Deployed to staging by @NikkiWines in version: 1.2.53-0 🚀
|
@NikkiWines @joelbettner @Beamanator Also, We are not able to navigate to these screens on IOS as https://staging.new.expensify.com/enable-payments leads to mWeb only. It works on Android |
@mvtglobally I'm a bit confused...
Are you able to go through the "Send Money" flow? Really all you need to do is go through the Send Money flow until you hit the page asking for the additional details (i.e. first name, last name, address, etc.). |
There is a really old issue on Native apps, we get the connectivity issue. so we were trying to use the URL directly to bypass it. It works on Android, but not on IOS |
@joelbettner and additionally on Web when trying with card, ew were getting an error like this and blocked from proceeding to Additional details page. Team is doublechecking with Bank account too Recording.1269.mp4bank flow Recording.1271.mp4 |
From the videos above it looks like the bank account option for Send Money flow is getting you to the correct screen. |
@mvtglobally to answer that question^, yes you can. |
@joelbettner Yes, we did it and here is the issue found |
@mvtglobally That linked issue shows the desired behavior for the form (see the screenshot in step 10). I went ahead and closed it. |
Just finished testing it on iOS. I was able to send the https://staging.new.expensify.com/enable-payments link as a comment in a chat, and then click on that link which took me to the correct page to test. |
🚀 Deployed to production by @Julesssss in version: 1.2.53-0 🚀
|
@Beamanator @Expensify/pullerbear will you please review?
Details
This PR allows the user to delete the entire default legal first name and last name in the additional details form for sending money. It also updates the AdditionalDetails form to use our Form functionality.
Fixed Issues
$ #12807
$ #12807 (comment)
Tests
Click the "fix the errors" link above the "Save & continue" button and confirm it takes you to the first error in the form:
Start filling in every input and confirm the errors go away as you do, AND that only the last 4 of the SSN are requested:
Click the "x" in the upper right corner to close the form.
Navigate back to the form through the send money flow and confirm it is pre-filled out with the exception of the SSN:
Add 4 fake digits for the SSN and click "Save & continue":
Confirm you are prompted to set all 9 digits of the SSN:
Add 9 fake numbers for the SSN and confirm the error goes away:
Click "Save & continue" and confirm you see the following prompt:
Offline tests
I confirmed that I could not submit the form when offline...
...but that while offline I could still save values to the Onyx object that will populate the form.
QA Steps
Repeat the steps from the "Tests" section above
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
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 reviewer will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
have been tested & I retested again)/** comment above it */
this
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)Screenshots/Videos
Web
See all screenshots above
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android
High Traffic Account on Prod