[HOLD for payment 2023-03-06] [$1000] The Terms of Service checkbox doesn't respond properly while navigating the connect bank account flow

[kavimuru]

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 workspace => connect bank account manually, enter valid information for first 3 steps
2. Go to Workspace => connect bank account manually again => Continue with setup => Go back to the first step, uncheck the check box then go back.
3. Click continue with setup again, now you are at Step 1, press Next.
4. Notice that the checkbox is checked, even though you unchecked it in step 2

Expected Result:

User should have to re-check the checkbox to continue

Actual Result:

The checkbox remains checked, incentivizing the user to try to continue with the flow. However, the form then shows the error "You must accept the Terms of Service to continue".

Workaround:

Just re-check it, but requires extra clicks

Platforms:

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

• MacOS / Chrome / Safari

Version Number: 1.2.67-6
Reproducible in staging?: y
Reproducible in production?: could not check
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:

Screen.Recording.2023-02-08.at.15.06.24.mov

Recording.1471.mp4

Expensify/Expensify Issue URL:
Issue reported by: @hungvu193
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1675843994636499

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0180c6f0674b5baa7b
• Upwork Job ID: 1623755142184652800
• Last Price Increase: 2023-02-09

[MelvinBot]

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

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)

• This bug is not a duplicate report (check E/App issues and #expensify-bugs)

• Similar to this, but definitely a different bug.

• This bug is reproducible using the reproduction steps in the OP. S/O

  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.

• This issue is filled out as thoroughly and clearly as possible

  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.

• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[greg-schroeder]

Hmm. I'm not sure I necessarily agree with the bug here - it seems to me that if you uncheck the Terms of Service checkbox and then return back to that page, it should remain unchecked, not show as checked again. I think the bug might actually be that the checkbox is showing checked when it shouldn't. Going to start a discussion in Slack

[greg-schroeder]

Discussion in Slack validates my thinking here - the bug is actually that the checkbox remains checked after unchecking it and later returning to the screen.

[greg-schroeder]

Updated issue title and adjusted the details in the OP to reflect the incorrect behavior

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~0180c6f0674b5baa7b

[MelvinBot]

Current assignee @greg-schroeder is eligible for the External assigner, not assigning anyone new.

[MelvinBot]

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

[MelvinBot]

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

[rushatgabhane]

Hi @esh-g, we recently made a change where all proposals have to follow this template.

Could you please edit your proposal to re-state the problem? Thanks

[esh-g]

Hey, sorry @rushatgabhane , I overlooked that. I have fixed it now, hope this proposal solves the problem
Thanks

[jayeshmangwani]

Proposal

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

In connect bank account page, The terms of Service checkbox does not reflect the checked/unchecked draft value correctly, when we open step 1 again then previously unchecked checkbox shows as checked

What is the root cause of that problem?

Problem is with default value of the accepted Terms chcekbox, default value is alway beinng passed true due to the naming issue, we are passing inputID="acceptedTerms" in checkbox but at the time of fetching the default value we are using acceptTerms

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

I would say we use the acceptedTerms id for gettinng default value

note adding a code for better understanding

Demo

14972.mov

[rushatgabhane]

@jayeshmangwani i get the intent behind adding a diff, but we recently changed our guidelines to strictly say no to diffs. Thanks for understanding

[rushatgabhane]

Terms of Service checkbox doesn't respond properly while navigating from connect bank account 2nd or 3rd step

@jayeshmangwani
nitpick: what do you mean by doesn't respond properly?

please update the proposal to exactly define the problem

[jayeshmangwani]

@rushatgabhane Thanks , I have removed diffs from proposal

[jayeshmangwani]

Terms of Service checkbox doesn't respond properly while navigating from connect bank account 2nd or 3rd step

@jayeshmangwani nitpick: what do you mean by doesn't respond properly?

@rushatgabhane

1. I have checked terms from step 1
2. completed step 2 in connect bank account flow
3. go back to step 1 and unchecked the checkbox
4. close the flow and again go to step 1, now I am able to see the checkbox is checked and thats wrong and second point is when I am pressing the continue I am getting the error You must accept the Terms of Service to continue but checkbox showing checked

[jayeshmangwani]

@rushatgabhane hey, I have updated the problem statement

[fedirjh]

Proposal

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

The checkbox remains checked, incentivizing the user to try to continue with the flow. However, the form then shows the error "You must accept the Terms of Service to continue".

What is the root cause of that problem?

This is a regression from #14866
We are passing draftValues to inputValues in Form initialisation , later when the form render it's input , it will ignore setting default value for input because of this line.

App/src/components/Form.js

Lines 239 to 242 in b113107

	// We want to initialize the input value if it's undefined
	if (_.isUndefined(this.state.inputValues[inputID])) {
	this.state.inputValues[inputID] = defaultValue;
	}

With the changes added in this PR #14866 , If we have a draftValue then the defaultValue will be ignored however the defaultValue is used to initiate the value of the input itself which make a conflict . here is an example with CheckboxWithLabel component

App/src/components/CheckboxWithLabel.js

Line 78 in b113107

this.isChecked = props.value || props.defaultValue || props.isChecked;

In conclusion input value is different from it's state value in the form. the input has default value and the form has draft value.

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

1. We should not initiate the Form's inputValues with draftValues . we should revert this PR fix/14784: Settings - Changed name is not saved under Display name page #14866
2. I have proposed a solution for this issue here
3. I think we should go with my propsed solution there [HOLD for payment 2023-02-15] [$1000] Settings - Changed name is not saved under Display name page #14784 (comment)

@rushatgabhane should I repost my solution here ?

What alternative solutions did you explore? (Optional)

I have noticed that when data of bank account is saved in the server , then routingNumber and accountNumber input fields are disabled , So i think, for consistency, we should also disable the checkbox field.

[greg-schroeder]

Assigned, contract extended

[tienifr]

Hi @NikkiWines @rushatgabhane , the PR is ready for review. Please help me check it. Thank you!

[tienifr]

Please help me review the PR when you're available @rushatgabhane. Thank you!

[MelvinBot]

📣 @tienifr! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[tienifr]

Contributor detail was already added in here

[tienifr]

Hi @greg-schroeder @NikkiWines @rushatgabhane, below is my regression test.

Regression Test Proposal

Bug: The terms of service checkbox does not respond properly while navigating the connect bank account flow

Proposed Test Steps:

Connecting bank account (language: English)

1, Open workspace => connect bank account manually, enter valid information for first 3 steps
2, Go to Workspace => connect bank account manually again => Continue with setup => Go back to the first step, uncheck the check box then go back.
3, Click continue with setup again, now you are at Step 1, press "Next".
4, Verify that the checkbox remains unchecked, because you unchecked it in step 2.

Adding debit card (language: English)

1, Open workspace => Payments => Add payment method => Debit card, enter valid information.
2, Verify that at this step, when you press "Save":

• If you don't check the check box, an error message will appear and prevent you from saving.
• If you check the check box, you will be allowed to proceed the flow.

Connecting bank account (language: English)

1, Open workspace => connect bank account manually, enter valid information for first 4 steps
2, Verify that at the fourth step, when you press "Save & continue":

• If you don't check the check box, an error message will appear and prevent you from saving and moving to the next step.
• If you check the check box, you will be allowed to proceed the flow.

Enabling payments (language: English)

1, Open Enable Payments
2, Verify that when you press "Enable Payments":

• If you don't check the check box, an error message will appear and prevent you from moving to the next step.
• If you check the check box, you will be allowed to proceed the flow.

To test in language: Spanish, please change language from English to Spanish then follow test steps for language: English because they are identical

Do we 👍 or 👎

[MelvinBot]

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

[MelvinBot]

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

• Fix/14972 the terms of service checkbox does not respond properly while navigating the connect bank account flow #15294

If no regressions arise, payment will be issued on 2023-03-06. 🎊

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.

• External issue reporter - @hungvu193
• Contributor that fixed the issue - @tienifr
• Contributor+ that helped on the issue and/or PR - @rushatgabhane

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

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[MelvinBot]

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:

• [@rushatgabhane / @NikkiWines] The PR that introduced the bug has been identified. Link to the PR: Form refactor bankaccountstep #10900 and Fix and refactor VBBA setup flow #13236
• [@rushatgabhane / @NikkiWines] 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: comment 1 and comment 2
• [@rushatgabhane / @NikkiWines] 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:
• [@tienifr] Propose regression test steps to ensure the same bug will not reach production again. - [HOLD for payment 2023-03-06] [$1000] The Terms of Service checkbox doesn't respond properly while navigating the connect bank account flow #14972 (comment)
• [@greg-schroeder] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[NikkiWines]

[@rushatgabhane / @NikkiWines] 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:

We can raise this in slack if necessary but I think this came about largely because QA steps weren't robust enough in this scenario. It's a bit of a niche flow for the user to go through to begin with, but hopefully with @tienifr's regression test proposal and more diligent testing for these type of changes, we won't see the same kind of bugs get to production.

[greg-schroeder]

@tienifr - paid
@hungvu193 @rushatgabhane - sent offer, please accept will pay today

[greg-schroeder]

Everyone paid, job closed

[greg-schroeder]

Regression test issue: https://github.com/Expensify/Expensify/issues/266422

[tienifr]

@greg-schroeder I think this is eligible for the PR timeliness bonus since the PR was merged within 3 business days of assignment

[tienifr]

@greg-schroeder I think this is eligible for the PR timeliness bonus since the PR was merged within 3 business days of assignment

@greg-schroeder what do you think?
cc @rushatgabhane

[greg-schroeder]

@tienifr hmmm - I don't think so? Based on what I saw:

Assigned: 2/17 3:20p PST
Merged: 2/22 1:54p PST

[tienifr]

@greg-schroeder It's less than 5 days, minus 2 days of weekends (2/18, 2/19), so I'd say it's less than 3 business days

[greg-schroeder]

Nice catch! Fixed for both of you.

[tienifr]

thanks!