[Hold for payment 2022-12-06] [$250] Terms of Service link has to be clicked twice to open in bank account setup page reported by @mdneyazahmad

[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. Visit https://staging.new.expensify.com/bank-account/BankAccountStep
2. Click on routing number field
3. Click on Expensify Terms of Service

Expected result:

It should open the link on the first click

Actual result:

It takes two click to open the link

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web

Version Number: 1.2.27-3

Reproducible in staging?: y

Reproducible in production?: y

Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Recording.929.mp4

Screen.Recording.2022-11-14.at.4.53.19.PM.mov

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

View all open jobs on GitHub

[melvin-bot]

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

[strepanier03]

Aaaah, I had a hard time reproducing this one but got to it in the end. The trick is you have to have a completely blank bank setup before trying to test.

In this case, I was able to reproduce it after clearing out the info that was already there. If anything is in the fields, the link works the first time. This makes me believe the error (of not having a routing number) is taking precedence over the TOS click or that when clicking the TOS link, we're trying to validate the fields unnecessarily.

My steps to reproduce:

1. Visit https://staging.new.expensify.com/bank-account/BankAccountStep,
2. If you have a partial setup and numbers in the account or routing fields, delete them, close the setup, and then open it up again so it's blank,
3. Click on routing number field,
4. Click on Expensify Terms of Service,

I'll add the External label and make a job for it.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[strepanier03]

Internal job post: https://www.upwork.com/ab/applicants/1592262964342812672/suggested

---

External job post: https://www.upwork.com/jobs/~0199da084980259e9d

[strepanier03]

@mdneyazahmad - As the issue reporter, feel free to apply for the job and share your Upwork profile with me so I can hire you for reporting.

[Delgee]

Proposal

Root cause

In the form component, input is validated on 'blur' event and error text is added on the bottom of the element which changes the link's position before the mouse clicks release. So link click is not counted.

Solution

Validating input on 'blur' event and adding error text bottom of input both of them are correct logic. I think changing these logic or jumping to the link on mouse press is the wrong way to fix this issue. So the simplest way to fix this is just moving checkbox to the bottom.

Screen.Recording.2022-11-15.at.07.05.58.mov

Solution 2

If changing the design is a bad idea. The link can be jumped to on mouse press instead of mouse release.

Screen.Recording.2022-11-15.at.07.32.10.mov

[peterklamka]

Proposal

Root cause

Agree with @Delgee

Solution

https://stackoverflow.com/questions/17769005/onclick-and-onblur-ordering-issue
Because OnMouseDown Event will be triggerred earlier than onBlur, we added onMouseDown event on Text component .

-Code
51f7464

-Working Video

2022-11-14-working_link.mp4

[mollfpr]

Thank you guys, I'll review the proposal today!

[mdneyazahmad]

@strepanier03 I applied for the job. Thanks!

[Puneet-here]

I think we should do nothing because we will be moving away from the validation at onBlur event after the Form refactors, so the issue will be resolved. Discussion here.

[mollfpr]

we will be moving away from the validation at onBlur event

Thanks for the info @Puneet-here! I do not see any detail on what the Form refactor will look like in the thread, so I'm not a hundred percent sure about what you say.

I'll circle back to the team and get some thoughts about it.

cc @Gonals @strepanier03

[peterklamka]

@mollfpr so my solution is not acceptable?

[mollfpr]

@peterklamka Your proposal still valid.

[peterklamka]

@mollfpr Thanks, I already applied jobs.@strepanier03

[strepanier03]

I'll hold on hiring anyone until we have confirming that we're going to fix this and that the proposal is officially being accepted. Will check tomorrow.

[mollfpr]

@strepanier03 We can continue to receive proposals now, we don't have a plan to remove the onBlur validation in the refactor form.

@Delgee Can you provide the code for solution 2?

@peterklamka I don't think using onMouseDown is a great idea, I feel lost click feeling with onMouseDown. Do you have any solution?

[strepanier03]

Great, thank you for the heads up @mollfpr. I'll continue to follow along and when we're ready to hire I'll be here to take the next steps. If I can help with anything else, just let me know.

[Delgee]

@mollfpr My solution is changing Text component inside TextLink component to Pressable component so we can use onPressIn props. (onPressIn will called before blur event) I think It will be much better if we create new component similar to TextLink instead of changing TextLink.

-        <Text
+        <Pressable
             style={[styles.link, ...additionalStyles]}
             accessibilityRole="link"
             href={props.href}
-            onPress={openLink}
+
+            onPressIn={openLink}
             onKeyDown={openLinkIfEnterKeyPressed}
         >
             {props.children}
-        </Text>
+        </Pressable>

[mollfpr]

Thanks @Delgee, Is using Pressable will render as anchor tag on the web?

[joelbettner]

I will not work on it, then.

[strepanier03]

@Delgee and @mdneyazahmad I've hired you for this job in Upwork. @mollfpr - Please apply for this job in Upwork here and if possible link me to your profile if your Github name isn't in the profile already.

---

I am out of the office Wednesday-Sunday and will return Monday. If urgent help is needed for this GH please post your request and the URL in #expensify-open-source.

[mollfpr]

@strepanier03 Applied!

[melvin-bot]

@Delgee, @strepanier03, @Gonals, @mollfpr Eep! 4 days overdue now. Issues have feelings too...

[strepanier03]

PR is on staging. Will continue following along as it progresses through QA and production merge.

[strepanier03]

On staging still. Will check tomorrow.

[strepanier03]

Still on staging.

---

I am OoO for jury duty tomorrow. If immediate action is needed on this GH please post in #expensify-open-source for assistance, thank you.

[mollfpr]

The PR is already in production since six days ago, but Melvin misses that 🤷‍♂️

https://github.com/Expensify/App/releases/tag/1.2.31-8

---

Btw I think we are eligible for a 50% bonus since the PR got merged within two days after being assigned.

cc @strepanier03 @Gonals

[melvin-bot]

@Delgee, @strepanier03, @Gonals, @mollfpr Whoops! This issue is 2 days overdue. Let's get this updated quick!

[mollfpr]

PR is in production...

[strepanier03]

Sorry about the issues with the GH update everyone. I'm taking care of this now.

[strepanier03]

BugZero checklist:

• A regression test has been added or updated so that the same bug will not reach production again. Link to the updated test here: Not making a TR for this edge case.
• The PR that introduced the bug has been identified. Link to the PR: Form refactor bankaccountstep #10900
• 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: https://expensify.slack.com/archives/C01GTK53T8Q/p1659440463665489 there's a long conversation about the Form refactoring a request to remove the onBlur event was causing this issue and other issues. The conversation leaned to keep the onBlur event so the issues caused by that should be fixed with other solutions.
• A discussion in #contributor-plus 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: Link to discussion: https://expensify.slack.com/archives/C01GTK53T8Q/p1659440463665489
• Pay @Delgee $250+$125 bonus
• Pay @mollfpr $250+$125 bonus
• Pay @mdneyazahmad $250

[strepanier03]

@mollfpr I have accepted the application, sorry I thought I'd already done that. I'll check back in to pay you out as well.

[mollfpr]

• The PR that introduced the bug has been identified.
  Link to the PR: Form refactor bankaccountstep #10900

• 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: https://expensify.slack.com/archives/C01GTK53T8Q/p1659440463665489 there's a long conversation about the Form refactoring a request to remove the onBlur event was causing this issue and other issues. The conversation leaned to keep the onBlur event so the issues caused by that should be fixed with other solutions.

• A discussion in #contributor-plus 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: https://expensify.slack.com/archives/C01GTK53T8Q/p1659440463665489

[strepanier03]

All BZ checklist items are complete and all parties have been paid as well. Going to close this one out, please reopen as needed and ping me if I missed something.