Skip to content
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

Legal last name field gives error when clicking on the save btn, but don't give us an error when click on and out of it #24218

Closed
1 of 6 tasks
kavimuru opened this issue Aug 7, 2023 · 10 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2

Comments

@kavimuru
Copy link

kavimuru commented Aug 7, 2023

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 website and Settings > Profile > Personal details > Legal name
  2. When you click on and out of the dirst field you are given an error but not given with second field. But there is error for second field when clicking on the save btn

Expected Result:

Legal last name field should give us an error when click on and out of it

Actual Result:

Legal last name field gives error when clicking on the save btn, but don't give us an error when click on and out of it

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.50-0
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
Notes/Photos/Videos: Any additional supporting documentation

Recording.5754.mp4
screen-capture.71.webm

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

View all open jobs on GitHub

@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 7, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 7, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 7, 2023

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)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • 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

@hungvu193
Copy link
Contributor

Correct me if I'm wrong, but I feel like this is another regressions from this #21838

@alphaboss1104
Copy link
Contributor

I agree with @hungvu193 's opinion.

@alphaboss1104
Copy link
Contributor

alphaboss1104 commented Aug 7, 2023

Proposal

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

Legal last name field gives error when clicking on the save btn, but don't give us an error when click on and out of it

What is the root cause of that problem?

App/src/components/Form.js

Lines 310 to 318 in 7b13f11

setTimeout(() => {
setTouchedInput(inputID);
// To prevent server errors from being cleared inadvertently, we only run validation on blur if any form values have changed since the last validation/submit
const shouldValidate = !_.isEqual(inputValues, lastValidatedValues.current);
if (shouldValidate) {
onValidate(inputValues);
}
}, 200);

In the above code, invoking the onValidate function depends on the shouldValidate. And shouldValidate is related to lastValidatedValues.

When click on and out legalFirstName TextInput field firstly, onValidate function is invoked and lastValidatedValues will be set as like this by following code:
{legalFirstName:'', legalLastName:''}

App/src/components/Form.js

Lines 151 to 153 in 7b13f11

if (isAtLeastOneInputTouched) {
lastValidatedValues.current = values;
}

And then when we click on and out legalLastName TextInput field, inputValues and lastValidatedValues.current has same value and shouldValidate is fasle.

App/src/components/Form.js

Lines 310 to 318 in 7b13f11

setTimeout(() => {
setTouchedInput(inputID);
// To prevent server errors from being cleared inadvertently, we only run validation on blur if any form values have changed since the last validation/submit
const shouldValidate = !_.isEqual(inputValues, lastValidatedValues.current);
if (shouldValidate) {
onValidate(inputValues);
}
}, 200);

Because of shouldValidate is fasle, onValidate function isn't invoked and error message isn't appeared.
This is the root cause of this problem.

It is independent of which TextInput field is clicked first.

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

To solve this issue we should set the lastValidatedValues with only clicked TextInput field.

            if (isAtLeastOneInputTouched) {
-                lastValidatedValues.current = values;
+                lastValidatedValues.current = values.inputID;
            }

What alternative solutions did you explore? (Optional)

None

@lschurr
Copy link
Contributor

lschurr commented Aug 7, 2023

@eVoloshchak @techievivek - Do you agree this is a regression and should be placed on hold?

@techievivek
Copy link
Contributor

Yes, this looks like a regression from the PR #23306 @joh42 @eVoloshchak), I think we can put this on HOLD for now.

@chiragxarora
Copy link
Contributor

chiragxarora commented Aug 8, 2023

This is basically duplicate, same error can be seen on all the forms, we should close all of these except the oldest resport.

Bug: error on blurring present only on first field which has autofocus, not the others

Or we could go on creating GHs for all the forms

@lschurr
Copy link
Contributor

lschurr commented Aug 8, 2023

Cool. Should we close or just put on hold?

@techievivek
Copy link
Contributor

We have reverted the original fix #21838 (comment) so we can close this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2
Projects
None yet
Development

No branches or pull requests

6 participants