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.

Sign up for GitHub

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

Jump to bottom

[bugfix] Stricter single digit date parsing #5592

Closed
wants to merge 1 commit into from
Closed

[bugfix] Stricter single digit date parsing #5592

wants to merge 1 commit into from
+153 −18

Conversation

Alanscut
Copy link
Contributor

Changes

@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 88.528% when pulling aa19bbc on Alanscut:issue_5314 into 528ac2b on moment:develop.

@Alanscut Alanscut changed the title [bugfix]Fix the issue of isValid() in strict mode [bugfix] Fix the issue of isValid() in strict mode Jun 11, 2020
@ichernev
Copy link
Contributor

Merged in cbcd0c5

@ichernev ichernev closed this Dec 23, 2023
ichernev added a commit that referenced this pull request Dec 23, 2023
@ichernev
Merge pull request #5592 from Alanscut:issue_5314
cbcd0c5 
[bugfix] Fix the issue of  `isValid()` in strict mode
@ichernev ichernev changed the title [bugfix] Fix the issue of isValid() in strict mode [bugfix] Stricter single digit date parsing Dec 26, 2023
@lasselindqvist
Copy link

@ichernev while I agree that this is a correct change and makes the stricter parsing more correct, it might have been nice to make it clearer in release notes. This might break many applications that have previously parsed 01.01.2000 and 1.1.2000 by just using DD.MM.YYYY

@lehmat
Copy link

lehmat commented Jan 3, 2024 •
edited

strict mode offers other functionality too that is very useful. I think it would be beneficial to have an option to accept both versions together with strict mode.

I think this change complicates the required application logic - We are letting users input dates as text and validate that it is valid. It is up-to user preference if they type dates with zeros or not.

Previously

return moment(userValue, 'D.M.YYYY', true).isValid()

After

for (const format in ['D.M.YYYY', 'DD.MM.YYYY', 'D.MM.YYYY', 'DD.M.YYYY']) {
  if (moment(userVaue, format, true).isValid() {
    return true
  }
}
return false

EDITED: Added missing cases so someone who finds this does not make same mistake

@lasselindqvist
Copy link

@lehmat there is also the case of "1.02.2020" or "01.2.2020" which someone might input. Those can be caught with 'D.MM.YYYY', 'DD.M.YYYY'

Cruikshanks added a commit to DEFRA/water-abstraction-service that referenced this pull request Jan 3, 2024
@Cruikshanks
Lock version of moment due to breaking changes
1c9c569 
> See [Bump moment from 2.29.4 to 2.30.1](#2375) for Dependabot bump that exposed this issue

**Dependabot** recently tried to bump [Moment](https://github.com/moment/moment) to a new minor version. However, when it did Ci started failing with breaking tests.

When we dug in we discovered the reason was any dates we tested where the day had a leading zero, for example, `03 August 2019`. One of the changes between **Moment** 2.29.4 and 2.30 was [[bugfix] Stricter single digit date parsing](moment/moment#5592).

[The issue](moment/moment#5314) that got fixed was that if you specify a date format of `['D MMMM YYYY']` then test if `03 August 2019` is valid **Moment** should say no. Prior to the 'fix' **Moment** was allowing this through and return true.

It turns out we are dependent on this behaviour. In `src/modules/returns/lib/csv-adapter/date-parser.js` we have a set of formats we use when parsing dates (the comments were copied from the source code).

```javascript
// preferred format for dates is D MMMM YYYY, but some applications
// may output the CSV in another format. This list attempts to deal
// with the unexpected formats without risking potential
// crossovers between formats such at DD/MM/YYYY and MM/DD/YYYY
// by using a sample of potential separators.
const GDS_DATE_FORMATS = [
  ['D', 'MMMM', 'YYYY'],
  ['D', 'MMM', 'YYYY'],
  ['D', 'MM', 'YYYY'],
  ['D', 'MMMM', 'YY'],
  ['D', 'MMM', 'YY'],
  ['D', 'MM', 'YY']
].flatMap(date => ([
  date.join(' '),
  date.join('/'),
  date.join('-')
]))
```

In the proper interpretation of the rules we're telling **Moment** the only valid date formats are the ones where 'day' does not have a leading zero. **Moment** v2.30 is saying "Sir, yes sir!" and rejecting dates we expect to be valid. **Moment** v2.29 is saying "it's cool daddy-o, we're all days here".

We don't fully understand the implications of updating `GDS_DATE_FORMATS` and have to take the tests at face value and that they reflect what the module needs to handle. This means we're very reluctant to start messing with the code.

So, until we do or even better, we've archived this project we're restricting the version of **Moment** we can use.
@Cruikshanks Cruikshanks mentioned this pull request Jan 3, 2024
Merged
Cruikshanks added a commit to DEFRA/water-abstraction-service that referenced this pull request Jan 3, 2024
@Cruikshanks
Lock version of moment due to breaking changes (#2379)
8af5b12 
> See [Bump moment from 2.29.4 to 2.30.1](#2375) for Dependabot bump that exposed this issue

**Dependabot** recently tried to bump [Moment](https://github.com/moment/moment) to a new minor version. However, when it did Ci started failing with breaking tests.

When we dug in we discovered the reason was any dates we tested where the day had a leading zero, for example, `03 August 2019`. One of the changes between **Moment** 2.29.4 and 2.30 was [[bugfix] Stricter single digit date parsing](moment/moment#5592).

[The issue](moment/moment#5314) that got fixed was that if you specify a date format of `['D MMMM YYYY']` then test if `03 August 2019` is valid **Moment** should say no. Before the 'fix' **Moment** was allowing this through and returning true.

It turns out we are dependent on this behaviour. In `src/modules/returns/lib/csv-adapter/date-parser.js`, we use a set of formats when parsing dates (the comments were copied from the source code).

```javascript
// preferred format for dates is D MMMM YYYY, but some applications
// may output the CSV in another format. This list attempts to deal
// with the unexpected formats without risking potential
// crossovers between formats such at DD/MM/YYYY and MM/DD/YYYY
// by using a sample of potential separators.
const GDS_DATE_FORMATS = [
  ['D', 'MMMM', 'YYYY'],
  ['D', 'MMM', 'YYYY'],
  ['D', 'MM', 'YYYY'],
  ['D', 'MMMM', 'YY'],
  ['D', 'MMM', 'YY'],
  ['D', 'MM', 'YY']
].flatMap(date => ([
  date.join(' '),
  date.join('/'),
  date.join('-')
]))
```

In properly interpreting the rules, we're telling **Moment** that the only valid date formats are the ones where 'day' does not have a leading zero. **Moment** v2.30 is saying "Sir, yes sir!" and rejecting dates we expect to be valid. **Moment** v2.29 says, "It's cool daddy-o, we're all days here".

We don't fully understand the implications of updating `GDS_DATE_FORMATS` and have to take the tests at face value and that they reflect what the module needs to handle. This means we're very reluctant to start messing with the code.

So, until we do or even better, we've archived this project we're restricting the version of **Moment** we can use.
@JohnC-80 JohnC-80 mentioned this pull request Jan 4, 2024
Merged
@a6software a6software mentioned this pull request Jan 18, 2024
Merged
@ghiscoding ghiscoding mentioned this pull request Apr 16, 2024
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Alanscut @coveralls @ichernev @lasselindqvist @lehmat