[HOLD for payment 2023-01-25] Date inputs allow 5 to 6 digits as year in Manual journey of Connect Bank Account

[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 the app
2. Go to any workspace
3. Choose Connect Bank Account
4. Choose Connect manually
5. Go to step 2 of the Form
6. Enter random values of "Incorporation date" and you'll notice the year part of the value can go upto 6 digits

Expected Result:

Year value should only allow 4 digits

Actual Result:

User can enter upto 6 digits

Workaround:

unknown

Platforms:

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

• MacOS / Chrome / Safari

Version Number: 1.2.43-1
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:

Expensify/Expensify Issue URL:
Issue reported by: @daraksha-dk
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1672076005803659

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a092ee76a867ed09
• Upwork Job ID: 1613242559970676736
• Last Price Increase: 2023-01-11

[yuwenmemon]

Oh yes, I've seen this one! Will take a look.

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @mollfpr (Internal)

[yuwenmemon]

Hmmm... I'm not sure what's happening here but it seems like we'd need a maxLength on our DatePicker component somewhere. Although I could be wrong.

Anyway, it's consistently reproducible and is purely front-end so will flip this External.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[Pujan92]

Proposal

It seems moment will allow the 6 digits for the year and it considers it as a valid, if we pass strict to true in moment constructor then 5/6 digits in the year will be considered invalid but still it can takes 6 digit internally to overwrite year in the input text.
We can provide a max date to the datepicker which will resolve this issue.

diff --git a/src/CONST.js b/src/CONST.js
index ef02585f6..277c4daac 100755
--- a/src/CONST.js
+++ b/src/CONST.js
@@ -41,6 +41,7 @@ const CONST = {
     DATE: {
         MOMENT_FORMAT_STRING: 'YYYY-MM-DD',
         UNIX_EPOCH: '1970-01-01 00:00:00.000',
+        MAX_DATE: '9999-12-31',
     },
     SMS: {
         DOMAIN: '@expensify.sms',
diff --git a/src/components/DatePicker/index.js b/src/components/DatePicker/index.js
index 2983ce762..29e2ee93c 100644
--- a/src/components/DatePicker/index.js
+++ b/src/components/DatePicker/index.js
@@ -30,6 +30,7 @@ class DatePicker extends React.Component {
     componentDidMount() {
         // Adds nice native datepicker on web/desktop. Not possible to set this through props
         this.inputRef.setAttribute('type', 'date');
+        this.inputRef.setAttribute('max', CONST.DATE.MAX_DATE);
         this.inputRef.classList.add('expensify-datepicker');
     }

Screen.Recording.2023-01-12.at.8.00.04.AM.mov

[yuwenmemon]

Ah so simple! This makes sense to me. @mollfpr thoughts?

[fedirjh]

Proposal

we can pass the expected format parameter to moment when parsing the date, this will make asMoment.isValid validates the format too before trigger onInputChange

diff --git a/src/components/DatePicker/index.js b/src/components/DatePicker/index.js
index 2983ce7620..cd06957c4e 100644
--- a/src/components/DatePicker/index.js
+++ b/src/components/DatePicker/index.js
@@ -43,7 +43,7 @@ class DatePicker extends React.Component {
             return;
         }
 
-        const asMoment = moment(text);
+        const asMoment = moment(text, CONST.DATE.MOMENT_FORMAT_STRING);
         if (asMoment.isValid()) {
             this.props.onInputChange(asMoment.format(CONST.DATE.MOMENT_FORMAT_STRING));
         }

[tienifr]

@arielgreen I think the split is fine. However I think the bonus for the timely PR should be split as well. Since any of the 3 could have been able to do the timely PR if he's chosen to implement the PR. And @Pujan92 already got the (2x) bigger original split for the added effort of implementing the PR.

If the bonus only goes to the one implementing the PR, then the original split should be equal (a.k.a 1/3 for each).

Just my 2 cents.

[Pujan92]

Proposal

It seems moment will allow the 6 digits for the year and it considers it as a valid, if we pass strict to true in moment constructor then 5/6 digits in the year will be considered invalid but still it can takes 6 digit internally to overwrite year in the input text. We can provide a max date to the datepicker which will resolve this issue.

@tienifr just to point out I already mentioned your solution in my initial proposal with the drawback. Only thing I wasn't aware of the safari/firefox date support issue for which we would need to consider that as a partial solution. So I believe if your proposal differs and smoothly fixes the issue then definitely you get the job.
@yuwenmemon

[tienifr]

@Pujan92 I interpret it as dismissing that solution, much like when I point out your proposal's potential issue here, rather than you proposing an alternate proposal (which should have accompanied code change and clear indication of such, ...). So I assume my proposal is the first one recommending that solution, which, along with your actual solution, goes into the final code change.

Anyway, let's leave the team to make the call here.

Thanks!

[arielgreen]

I think the bonus for the timely PR should be split as well. Since any of the 3 could have been able to do the timely PR if he's chosen to implement the PR.

Thanks for your thoughts. Seems fair.

Offers sent to all.

[melvin-bot]

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

• year restriction to 4 digit in date inputs #14272

If no regressions arise, payment will be issued on 2023-01-25. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

[melvin-bot]

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:

• [@mollfpr / @yuwenmemon] The PR that introduced the bug has been identified. Link to the PR: Allow future dates to be entered in DatePicker #13702
• [@mollfpr / @yuwenmemon] 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: Allow future dates to be entered in DatePicker #13702 (comment)
• [@mollfpr / @yuwenmemon] 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
• [@arielgreen] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here: https://github.com/Expensify/Expensify/issues/260470

[arielgreen]

Not overdue, this is in its regression period.

[arielgreen]

@mollfpr @yuwenmemon please be sure to complete the checklist here.

[mollfpr]

@arielgreen Thanks for the ping! Will finish the checklist today.

[yuwenmemon]

Thanks @mollfpr - let me know if I can be of assistance.

[mollfpr]

Previously we have maximumDate props to DatePicker where it's set to new Date() on the CompanyStep form. But before this PR, the props are removed to allow users to input any future dates and the Form will validate the value.

Here's the PR #13702 that removes the maximumDate.

cc @yuwenmemon

[yuwenmemon]

@arielgreen last item in the checklist is all yours!

[mollfpr]

Bump @arielgreen

[arielgreen]

All payments have been issued. Discussing a test here, then we can close this one out.

[arielgreen]

Bumped thread for consensus