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

[HOLD for payment 2023-03-09] [$1000] Wrong validation error message on choose date of birth. #15228

Closed
6 tasks
kavimuru opened this issue Feb 16, 2023 · 42 comments
Closed
6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@kavimuru
Copy link

kavimuru commented Feb 16, 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 Settings -> Profile -> Personal Details.
  2. Click on Date of birth.
  3. Choose date 17/2/2018.( Testing this scenario on 02/16)
  4. Press on save button.
  5. Error message will be display "Date should be before 2018-02-16."
  6. Now select 16/2/2018, it will works.

Expected Result:

Either message should be "Date should be on or before 2018-02-16.". Or don't allow to select 16/2/2018

Actual Result:

Incorrect error message allowing to select 16/2/2018 but error message says date should be before 16/2/2018

Workaround:

unknown

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.2.72-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:

date-validation.mov
Recording.1538.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d2cc56f9c3202414
  • Upwork Job ID: 1626535149478653952
  • Last Price Increase: 2023-02-17
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Feb 16, 2023
@MelvinBot
Copy link

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

@melvin-bot melvin-bot bot locked and limited conversation to collaborators Feb 16, 2023
@MelvinBot
Copy link

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

@dylanexpensify
Copy link
Contributor

Cannot reproduce on staging, @kavimuru can you share what platforms this occurs on and if still occurring? 🙇

@dylanexpensify dylanexpensify added the Needs Reproduction Reproducible steps needed label Feb 17, 2023
@dylanexpensify
Copy link
Contributor

Was able to reproduce, but we need to update steps to say set to today's date not Feb 17 cc @kavimuru

@dylanexpensify
Copy link
Contributor

Thread

@dylanexpensify dylanexpensify added External Added to denote the issue can be worked on by a contributor and removed Needs Reproduction Reproducible steps needed labels Feb 17, 2023
@melvin-bot melvin-bot bot unlocked this conversation Feb 17, 2023
@melvin-bot melvin-bot bot changed the title Wrong validation error message on choose date of birth. [$1000] Wrong validation error message on choose date of birth. Feb 17, 2023
@MelvinBot
Copy link

Job added to Upwork: https://www.upwork.com/jobs/~01d2cc56f9c3202414

@MelvinBot
Copy link

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

@MelvinBot
Copy link

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 17, 2023
@MelvinBot
Copy link

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

@jatinsonijs
Copy link
Contributor

jatinsonijs commented Feb 17, 2023

Proposal

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

Validation message is incorrect or we can say incomplete

What is the root cause of that problem?

We are allowing ages between 5 to 150 so we are doing some validation to check age is between these numbers or not. but message is incorrect in translation:

dateShouldBeBefore: ({dateString}) => `Date should be before ${dateString}.`,

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

We have to just change message in translation to indicate before or equal to date

@situchan
Copy link
Contributor

Should we put this on HOLD for New Date Picker Design?

@allroundexperts
Copy link
Contributor

allroundexperts commented Feb 17, 2023

Proposal

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

The validation error message says that we can not select a particular date during DOB selection but in actual, we can select that date without any error.

What is the root cause of that problem?

The root cause of this problem is that current date that we use for comparison includes hours of the day as well. The selected date however, does not include the hours of the day. eg 18-01-01 translates to 01 Jan 2018 00:00:00. This causes the check to pass as this date is before the current date.

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

We can solve the problem by making sure that we're comparing against the current date with start of the day selected. The following needs to be changed:
https://github.com/Expensify/App/blob/main/src/libs/ValidationUtils.js#L218-L219
to

    const recentDate = moment().startOf('day').subtract(minimumAge, 'years');
    const longAgoDate = moment().startOf('day').subtract(maximumAge, 'years');

We also need to change the following:
https://github.com/Expensify/App/blob/main/src/libs/ValidationUtils.js#L227
to

    if (testDate.isSameOrAfter(recentDate)) {

What alternative solutions did you explore? (Optional)

  • Changing the validation message. This however does not consider the root cause.
  • We can also change the testDate to not start from the beginning of the day. Instead, it can include hours. However, doing this might still cause problems as the testDate might be evaluated before currentDate and the difference between the two might be some milliseconds which will effect the result.

@melvin-bot melvin-bot bot added the Overdue label Feb 20, 2023
@dylanexpensify
Copy link
Contributor

@rushatgabhane can we get a review of @allroundexperts proposal please?

@melvin-bot melvin-bot bot removed the Overdue label Feb 20, 2023
@rushatgabhane
Copy link
Member

@danieldoglas i prefer @allroundexperts' proposal because it fixes the root cause #15228 (comment)

C+ reviewed 🎀 👀 🎀

@danieldoglas
Copy link
Contributor

Makes sense to me. Assigning @allroundexperts

@dylanexpensify dylanexpensify removed their assignment Mar 14, 2023
@dylanexpensify dylanexpensify added the Bug Something is broken. Auto assigns a BugZero manager. label Mar 14, 2023
@MelvinBot
Copy link

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

@dylanexpensify
Copy link
Contributor

lol

@dylanexpensify dylanexpensify removed their assignment Mar 14, 2023
@dylanexpensify dylanexpensify added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Mar 14, 2023
@Expensify Expensify deleted a comment from MelvinBot Mar 14, 2023
@MelvinBot
Copy link

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

@dylanexpensify
Copy link
Contributor

@mallenexpensify I'm heading OOO! Would you mind finishing the last bits for me?? 🙇

@Expensify Expensify deleted a comment from MelvinBot Mar 14, 2023
@mallenexpensify
Copy link
Contributor

@jatinsonijs @rushatgabhane @allroundexperts can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~01d2cc56f9c3202414

This bug is specific to the date being exactly 5 years from the current date, right?

@allroundexperts
Copy link
Contributor

@jatinsonijs @rushatgabhane @allroundexperts can you please accept the job and reply here once you have? https://www.upwork.com/jobs/~01d2cc56f9c3202414

This bug is specific to the date being exactly 5 years from the current date, right?

Currently we have it set up for 5 years.

@allroundexperts
Copy link
Contributor

I've applied @mallenexpensify.

@mallenexpensify
Copy link
Contributor

mallenexpensify commented Mar 14, 2023

Currently we have it set up for 5 years.

Thanks @allroundexperts . @isagoico , when you're back from ECX, can you take a look at this? I'm unsure it should be added to TestRail because it's an extreme edge case that anyone (outside of @jatinsonijs , ha) would add a birthday of exactly 5 years.

@jatinsonijs
Copy link
Contributor

@mallenexpensify applied

@isabelastisser
Copy link
Contributor

@mallenexpensify , you meant to tag @isagoico . :)

@mallenexpensify
Copy link
Contributor

@isabelastisser I swear I updated it right after I posted it! (honestly, I probably have done it more than once this week, sorry)

@mallenexpensify mallenexpensify added Weekly KSv2 and removed Daily KSv2 labels Mar 15, 2023
@mallenexpensify
Copy link
Contributor

Everyone is paid!! Leaving open and bumping to weekly til @isagoico is back and can review.

@allroundexperts
Copy link
Contributor

@mallenexpensify Thanks for the payment. The PR for this was merged within 3 days. Am I eligible for the bonus here?

@allroundexperts
Copy link
Contributor

@mallenexpensify Thanks for the payment. The PR for this was merged within 3 days. Am I eligible for the bonus here?

I take my comment back. This was merged ~12 hours late.

@rushatgabhane
Copy link
Member

i have a checklist todo here

@isagoico
Copy link

Currently we have it set up for 5 years.

Thanks @allroundexperts . @isagoico , when you're back from ECX, can you take a look at this? I'm unsure it should be added to TestRail because it's an extreme edge case that anyone (outside of @jatinsonijs , ha) would add a birthday of exactly 5 years.

Lol agree here, I don't think this would be a valuable addition since there's a extremely low chance of this happening to any user. So I would say :donothing: in TestRail for this case.

@mallenexpensify
Copy link
Contributor

Thanks for confirming @isagoico , closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests