-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[HOLD for payment 2024-06-28] [$250] Extra spaces are not trimmed in the next page for the routing number and Account number #43135
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
Comments
Triggered auto assignment to @Christinadobrzyn ( |
@Christinadobrzyn Can i be the C+ here? as i reported this issue and have more context about the same. |
ProposalPlease re-state the problem that we are trying to solve in this issue.Extra spaces are not trimmed in the next page for the routing number and Account number What is the root cause of that problem?We are not using
What changes do you think we should make in order to solve the problem?Use App/src/pages/ReimbursementAccount/PersonalInfo/substeps/FullName.tsx Lines 52 to 56 in 088939b
const handleSubmit = useReimbursementAccountStepFormSubmit({
fieldIds: [BANK_INFO_STEP_KEYS.ROUTING_NUMBER, BANK_INFO_STEP_KEYS.ACCOUNT_NUMBER],
onNext,
shouldSaveDraft: isEditing,
}); We will also check for other pages like this, which uses substeps. What alternative solutions did you explore? (Optional) |
ProposalPlease re-state the problem that we are trying to solve in this issue.Extra spaces are not trimmed in the next page for the routing number and Account number What is the root cause of that problem?Values saved from Manual step with spaces and used on the next step. App/src/pages/ReimbursementAccount/BankInfo/substeps/Manual.tsx Lines 73 to 76 in 952d844
What changes do you think we should make in order to solve the problem?The best way to solve this issue id to make sure we don't go to the next step with wrong values (with spaces). I suggest adding a more check on Manual step to make sure we don't accept values with spaces. This will make sure all values sent to the next steps and saved on the Onyx data are correct and safe
1- The values sent to the
for this we need to add
2- We need to add a new regex for spaces before and after REGEX: {
US_ACCOUNT_NUMBER: /^[0-9]{4,17}$/,
MASKED_US_ACCOUNT_NUMBER: /^[X]{0,13}[0-9]{4}$/,
SWIFT_BIC: /^[A-Za-z0-9]{8,11}$/,
+ START_OR_END_WITH_SPACE: /^\s|\s$/,
}, 3- We update the validate function to check if the values has spaces at the start or at the end if (values.accountNumber && CONST.BANK_ACCOUNT.REGEX.START_OR_END_WITH_SPACE.test(values.accountNumber)) {
errors.accountNumber = 'bankAccount.error.accountNumber';
} We do the same for the What alternative solutions did you explore? |
@allgandalf I'm going to add the label so it's fair share but we can ask the assigned C+ if you can take it. |
Job added to Upwork: https://www.upwork.com/jobs/~0134151ac2072ab6de |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @ahmedGaber93 ( |
Hey @ahmedGaber93 - would you mind if @allgandalf took over as C+ since they reported this issue and have some more context? |
@Christinadobrzyn fair point, i’m okay with @ahmedGaber93 taking this over, willing to help them if they need any help on the PR phase 👍 |
@Christinadobrzyn Yeah, that's fair. Please unassign me and assign @allgandalf. |
📣 @allgandalf 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
Thanks for your proposals @Krishna2323 @dragnoir @dragnoir , In @Krishna2323 , your solution would work fine for this bug, but before moving forward can you update your proposal to highlight the following points:
Approach wise your proposal LGTM, but can you please address the above points once :) thanks |
@Krishna2323 , @Krishna2323's proposal, looks good to me, initial proposal summary can be found here 🎀👀🎀 C+ reviewed |
Triggered auto assignment to @iwiznia, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
I don't see this component trimming the values, @Krishna2323 can you point out where that component would trim the value? |
@iwiznia, currently we pass
App/src/hooks/useStepFormSubmit.ts Lines 24 to 35 in 8d11d0b
App/src/components/Form/FormProvider.tsx Lines 197 to 198 in 8d11d0b
|
Got it. |
📣 @Krishna2323 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
@allgandalf, PR ready for review. |
Thanks for the PR @Krishna2323 , will review today 👍 |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.85-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2024-06-28. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
Payment summary:
|
There are no regressions, so I'm paying this out based on this payment summary - #43135 (comment) @allgandalf can you let us know about a regression test? |
Regression Test Proposal
Do we agree 👍 or 👎 |
Regression test here - https://github.com/Expensify/Expensify/issues/409420 Payment summary here - #43135 (comment) Closing as complete! |
Uh oh!
There was an error while loading. Please reload this page.
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 1.4.79-6
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @allgandalf
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1717537508312169
Action Performed:
Expected Result:
Spaces are trimmed in the confirmation page
Actual Result:
Spaces are present on the confirmation page, we should trim them while saving as a account number cannot have spaces before or after.
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Screen.Recording.2024-06-05.at.3.11.25.AM.mov
Recording.176.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @MitchExpensify / @ChristinadobrzynThe text was updated successfully, but these errors were encountered: