-
Notifications
You must be signed in to change notification settings - Fork 0
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
Fix broken return version start date page #1469
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
https://eaflood.atlassian.net/browse/WATER-4687 In [Decouple start date logic](#1461) we made a change to support adding quarterly returns. It was determined the logic for working out the start date from the values entered by the user was being duplicated in other places. The change ensured the logic stays in just the `SubmitStartDateService`, and added a new property `returnVersionStartDate` that other services could use instead. Unfortunately, an error was introduced. ```javascript // app/services/return-versions/setup/submit-start-date.service.js // Reminder! Because of the unique qualities of Javascript, Year and Day are literal values, month is an index! So, // January is actually 0, February is 1 etc. This is why we deduct 1 from the month. session.returnVersionStartDate = new Date(`${payload['start-date-year']}-${payload['start-date-month'] - 1}-${payload['start-date-day']}`).toISOString().split('T')[0] ``` This way of creating the date was added. It uses a [template literal](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals) to combine the values provided by the user and pass it as an argument o JavaScript's `new Date()`. But it also applies a common fix for the fact JavaScript expects month to be provided as an index, not literally! So, January = 0, February = 1 etc. The problem is `new Date('2024-04-01')` is not the same as `new Date(2024, 4, 1)`. The first _will_ return you 1 April 2024, the second will return 1 March 2024. JavaScript understands that 'month' is literal when you provide a string argument to `new Date()`. It is only when passing month as a separate value that you have to treat it as an index. The result for the return versions setup journey is that a user is entering, for example, `13 May 2019` on the start page, but we're storing (and would eventually persist!) `13 April 2019`. This change fixes the issue.
We don't need to call `toISOString()` and `split()`. It works perfectly, as it has done in other places in the code without these additions.
The user has entered '26 11 2023' in the UI. Therefore, they should expect to see `26 November 2023` when `returnVersionStartDate` is used or displayed anywhere in the UI. The test wasn't highlighting something was wrong because it was asserting this fact.
Cruikshanks
force-pushed
the
fix-broken-ret-verion-start-date
branch
from
November 8, 2024 09:19
7986599
to
eef022b
Compare
jonathangoulding
approved these changes
Nov 8, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did notice this and fixed on the PR branch for the other implementation of the code.
Should have fixed separately.
apologize.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://eaflood.atlassian.net/browse/WATER-4687
In Decouple start date logic we made a change to support adding quarterly returns.
It was determined the logic for working out the start date from the values entered by the user was being duplicated in other places. The change ensured the logic stays in just the
SubmitStartDateService
, and added a new propertyreturnVersionStartDate
that other services could use instead.Unfortunately, an error was introduced.
This way of creating the date was added. It uses a template literal to combine the values provided by the user and pass it as an argument o JavaScript's
new Date()
.But it also applies a common fix for the fact JavaScript expects month to be provided as an index, not literally! So, January = 0, February = 1 etc.
The problem is
new Date('2024-04-01')
is not the same asnew Date(2024, 4, 1)
. The first will return you 1 April 2024, the second will return 1 March 2024.JavaScript understands that 'month' is literal when you provide a string argument to
new Date()
. It is only when passing month as a separate value that you have to treat it as an index.The result for the return versions setup journey is that a user is entering, for example,
13 May 2019
on the start page, but we're storing (and would eventually persist!)13 April 2019
.This change fixes the issue.