search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.34k stars 2.77k forks source link

[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [$1000] Bank account - Phone number field is not trimmed for spaces in connect bank account #25996

Closed lanitochka17 closed 10 months ago

lanitochka17 commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

  1. Open the app
  2. Open settings->workspaces->any workspace->bank account->connect manually
  3. Fill in fields in step 1 and continue
  4. Fill in fields in step 2, before legal name, phone number, tax ID add some spaces and continue
  5. Go back to step 2 and observe that app trim spaces before value for legal name, tax ID but doesn't trim space for phone number

Expected Result:

App should trim spaces before phone number value as it does for tax ID or legal name

Actual Result:

App should trim spaces before phone number value as it does for tax ID or legal name

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: 1.3.57-5

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/78819774/a9458e04-f657-40cb-a701-1e37730fbfc5

https://github.com/Expensify/App/assets/78819774/d31d1ade-5f60-40af-a389-9ff3ef7897fa

Expensify/Expensify Issue URL:

Issue reported by: @dhanashree-sawant

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1692249462405549

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~017eadfab4995c4b33
  • Upwork Job ID: 1696513864176926720
  • Last Price Increase: 2023-09-19
melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @garrettmknight (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

hungvu193 commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Bank account - Phone number field is not trimmed for spaces in connect bank account

What is the root cause of that problem?

We're not trimming the space here: https://github.com/Expensify/App/blob/c16995257c738761206ea07645e863ee717ecbc0/src/pages/ReimbursementAccount/CompanyStep.js#L141

What changes do you think we should make in order to solve the problem?

We should trim the space before submitting

companyPhone: parsePhoneNumber(values.companyPhone.trim(), {regionCode: CONST.COUNTRY.US}).number.significant,

What alternative solutions did you explore? (Optional)

N/A

neonbhai commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Bank account - Phone number field is not trimmed for spaces in connect bank account

What is the root cause of that problem?

This happens because we are not sanitizing the phone number input here;

What changes do you think we should make in order to solve the problem?

We should be using the replace function and sanitizing using the NON_NUMERIC regex const:

companyPhone: parsePhoneNumber(values.companyPhone.replace(CONST.REGEX.NON_NUMERIC, ''), {regionCode: CONST.COUNTRY.US}).number.significant,

This is better than just trimming with trim() as we are remove all Non Numeric characters. Note: this is also the approach taken when sanitizing taxID here.

What alternative solutions did you explore? (Optional)

xx

dukenv0307 commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Phone number field is not trimmed for spaces in connect bank account

What is the root cause of that problem?

When we save the value of company, it's trimmed before saving. We use shouldUseDefaultValue prop for companyTaxID and companyName so after saving and going back companyTaxID and companyName are trimmed

https://github.com/Expensify/App/blob/79ea92345e48599d6663ef20213f8c64bb0568c1/src/pages/ReimbursementAccount/CompanyStep.js#L179

https://github.com/Expensify/App/blob/79ea92345e48599d6663ef20213f8c64bb0568c1/src/pages/ReimbursementAccount/CompanyStep.js#L231

companyPhone field doesn't use this prop so it's not trimmed after going back

What changes do you think we should make in order to solve the problem?

We can pass shouldUseDefaultValue into TextInput of companyPhone by the same way we use for companyTaxID

const shouldDisableCompanyPhone = Boolean(bankAccountID && this.props.getDefaultStateForField('companyPhone'))
<TextInput
    inputID="companyPhone"
    label={this.props.translate('common.phoneNumber')}
    accessibilityLabel={this.props.translate('common.phoneNumber')}
    accessibilityRole={CONST.ACCESSIBILITY_ROLE.TEXT}
    containerStyles={[styles.mt4]}
    keyboardType={CONST.KEYBOARD_TYPE.PHONE_PAD}
    placeholder={this.props.translate('common.phoneNumberPlaceholder')}
    defaultValue={this.props.getDefaultStateForField('companyPhone')}
    shouldSaveDraft
    disabled={shouldDisableCompanyPhone}
    shouldUseDefaultValue={shouldDisableCompanyPhone}
/>

https://github.com/Expensify/App/blob/79ea92345e48599d6663ef20213f8c64bb0568c1/src/pages/ReimbursementAccount/CompanyStep.js#L206

What alternative solutions did you explore? (Optional)

We can use a state in CompanyStep for phone number text input

const defaultPhoneValue = getDefaultStateForField('companyPhone');
const [companyPhone, setCompanyPhone] = useState(defaultPhoneValue);

<TextInput
    inputID="companyPhone"
    label={translate('common.phoneNumber')}
    accessibilityLabel={translate('common.phoneNumber')}
    accessibilityRole={CONST.ACCESSIBILITY_ROLE.TEXT}
    containerStyles={[styles.mt4]}
    keyboardType={CONST.KEYBOARD_TYPE.PHONE_PAD}
    placeholder={translate('common.phoneNumberPlaceholder')}
    value={companyPhone}
    onValueChange={(value) => setCompanyPhone(value)}
    shouldSaveDraft
/>

We can do the same way this these other fields that are editable.

Result

Screencast from 11-09-2023 17:18:11.webm

dhanashree-sawant commented 1 year ago

I think issue reporter was mistakenly marked to @dhanashree instead of @dhanashree-sawant, can we correct that if possible? It will help me to recieve emails about it

garrettmknight commented 1 year ago

@dhanashree-sawant updated the OP. Checking whether this is an issue for us on the backend or not before opening up.

garrettmknight commented 1 year ago

It looks like we're parsing the phone number into the correct format in the App here and then sending it to the API. So the API is currently receiving and storing phone numbers in the backend without any leading spaces but I think the display of the phone number in the App isn't being updated.

Confirmed that the expected behavior is that we'd update the app to clear leading spaces since we're cleaning that up on the backend.

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~017eadfab4995c4b33

melvin-bot[bot] commented 1 year ago

Current assignee @garrettmknight is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Ollyws (External)

Ollyws commented 1 year ago

Thanks for the proposals everyone.

It does seem like Awesomephonenumber removes the spaces when using parsePhoneNumber already and there are no spaces included in the Onyx data.

@dukenv0307 Your proposal seems promising but it's making the phone number field uneditable for me when we return to it.

dukenv0307 commented 1 year ago

@Ollyws For taxID we also use this way and make this field uneditable so I think if we decided to display phoneNumber value with trimming, it will be fine.

melvin-bot[bot] commented 1 year ago

@garrettmknight, @Ollyws Whoops! This issue is 2 days overdue. Let's get this updated quick!

Ollyws commented 1 year ago

Asked for more clarification around which fields should be disabled in the slack thread.

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 year ago

@garrettmknight, @Ollyws Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 1 year ago

@garrettmknight, @Ollyws Eep! 4 days overdue now. Issues have feelings too...

garrettmknight commented 1 year ago

Just bumped the thread for clarification.

Ollyws commented 1 year ago

@dukenv0307 Consensus in the slack thread is that the phone number field should remain editable. Is there any way we can utilise the data from Onyx while allowing the field to remain editable?

dukenv0307 commented 1 year ago

@Ollyws I think we can use a state to control companyPhone TextInput in CompanyStep.

dukenv0307 commented 1 year ago

@Ollyws Updated alternative solution and the video demo for this comment above.

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 year ago

@garrettmknight, @Ollyws Whoops! This issue is 2 days overdue. Let's get this updated quick!

garrettmknight commented 1 year ago

@Ollyws can you check out that udpated proposal?

melvin-bot[bot] commented 1 year ago

@garrettmknight @Ollyws this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

garrettmknight commented 1 year ago

Bumped in slack.

Ollyws commented 1 year ago

@dukenv0307 Thanks for the update, it's going in the right direction but I've noticed this problem is not just limited to the phone number field, it also happens on all the other enabled fields so we should implement a solution for all editable fields. Thanks!

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

garrettmknight commented 12 months ago

@dukenv0307 can you update your solution when you get a chance?

dukenv0307 commented 12 months ago

@Ollyws If we want to fix this for all fields we should use the same way in my alternative proposal for other fields. When we go back, the value still is a text that we entered because we only use defaultValue prop and the component doesn't unmount. For companyName and taxID, these are updated because shouldUseDefaultValue is updated. So I think using state to control the text input is the only possible solution now to make the input updated and editable.

We also use this solution in NewTaskDetailPage

melvin-bot[bot] commented 12 months ago

@garrettmknight @Ollyws this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Thanks!

melvin-bot[bot] commented 12 months ago

Current assignee @Ollyws is eligible for the Internal assigner, not assigning anyone new.

neonbhai commented 12 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Fields in the Connect Bank Account form are not trimmed for spaces when values are retrived from localstorage in the frontend.

What is the root cause of that problem?

This happens because we are not sanitizing the drafts before saving them. We have no logic for the Form Component to trim whitespace before saving to ONYX.

What changes do you think we should make in order to solve the problem?

We can add an optional prop shouldTrimDraftValues that would remove whitespaces from Form inputs before saving them under draft key in ONYX.

We should remove whitespaces from Form inputs before saving them under draft key in ONYX. This should be done conditionally using an optional prop shouldTrimDraftValues.

The trim functionality will be added here: https://github.com/Expensify/App/blob/71db96d8a77068220fac73ca0dc984796ff862f9/src/components/Form.js#L384-L386

if(props.shouldTrimDraftValues) {
    value = value.trim();
}

Since, it is customary for any and all Form input elements to trim whitespaces before sending it to the backend.We can also turn this on by default.

This will have the additional benefit of reflecting the behavior of trimming values in the frontend.

We trim the strings all over the App. This functionality was missing here. [Screencast from 24-09-23 07:06:26 AM IST.webm](https://github.com/Expensify/App/assets/64629613/935182ea-1903-4eac-b38c-50177774934e)

Result:

Screencast from 24-09-23 07:03:24 AM IST.webm

This will solve this issue for all pages in the flow.

What alternative solutions did you explore? (Optional)

xx

garrettmknight commented 12 months ago

@Ollyws before we recruit internally, can you comment on @dukenv0307 alternative solution suggestion and if that's not working for you have a look at the most recent proposal? Thanks!

Ollyws commented 12 months ago

@neonbhai Thanks for the proposal but Onyx data is already sanitized the problem is we are not retreiving it when we navigate back.

@dukenv0307's solution, similar to what we do in NewTaskDetailsPage looks good to me.

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 12 months ago

Triggered auto assignment to @flodnv, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

dukenv0307 commented 11 months ago

@Ollyws The PR is ready for review.

melvin-bot[bot] commented 11 months ago

Current assignee @Ollyws is eligible for the External assigner, not assigning anyone new.

garrettmknight commented 11 months ago

PR is on Staging - will update to the awaiting payment hold when it goes to prod if Melv misses it.

melvin-bot[bot] commented 11 months ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 11 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.84-10 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-10-23. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

For reference, here are some details about the assignees on this issue:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

melvin-bot[bot] commented 11 months ago

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:

melvin-bot[bot] commented 11 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.84-10 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-10-23. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

For reference, here are some details about the assignees on this issue:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

melvin-bot[bot] commented 11 months ago

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:

Ollyws commented 11 months ago

BugZero Checklist:

  • [x] The PR that introduced the bug has been identified. Link to the PR:

We can't really call this a regression from any PR, we just never considered to sanitize/trim the draft values after the form was submitted.

  • [x] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

N/A

  • [x] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

N/A

  • [x] Determine if we should create a regression test for this bug.

I don't think a regression test is helpful for this one as it's a minor aesthetic feature that doesn't affect the flow in any meaningful way.

garrettmknight commented 11 months ago

Summary of Payments:

New Upwork issue: https://www.upwork.com/en-gb/ab/applicants/1716919006499426304/job-details

New offers out since the other job went stale - I'll pay when y'all accept.

garrettmknight commented 10 months ago

All paid up - closing.