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

ROE-2313 Trim leading zeros from date fields #995

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

markpit
Copy link
Contributor

@markpit markpit commented Jul 26, 2023

JIRA link

https://companieshouse.atlassian.net/browse/ROE-2313

Change description

Trimming any leading zeros from date values to prevent errors when saving to the Api

Work checklist

  • Tests added where applicable
  • UI changes meet accessibility criteria

Merge instructions

We are committed to keeping commit history clean, consistent and linear. To achieve this, this commit should be structured as follows:

<type>[optional scope]: <description>

and contain the following structural elements:

  • fix: a commit that patches a bug in your codebase (this correlates with PATCH in semantic versioning),
  • feat: a commit that introduces a new feature to the codebase (this correlates with MINOR in semantic versioning),
  • BREAKING CHANGE: a commit that has a footer BREAKING CHANGE: introduces a breaking API change (correlating with MAJOR in semantic versioning). A BREAKING CHANGE can be part of commits of any type,
  • types other than fix: and feat: are allowed, for example build:, chore:, ci:, docs:, style:, refactor:, perf:, test:, and others,
  • footers other than BREAKING CHANGE: <description> may be provided.

@markpit markpit marked this pull request as draft July 26, 2023 08:53
mwestacott

This comment was marked as resolved.

@markpit

This comment was marked as resolved.

@mwestacott

This comment was marked as resolved.

@markpit
Copy link
Contributor Author

markpit commented Jul 26, 2023

As a general comment, I can't see any unit tests for the particular scenarios described in the JIRA ticket - valid days and months but with leading zeroes. Are these missing or due to come later?

I thought the 'missing x when x is zero' would cover that scenario but I can look at adding tests or improve existing ones to check the zero is in fact removed.

So yes, the 'when x is zero' part is not the scenario in the ticket, it's 'when x has leading zeroes'.

Added extra tests that check the leading zeros are removed by capturing the req.body when it is passed into a mock and checking the date fields in that compared to what we originally sent in the post request.

@chsonarqubeprchecks
Copy link

@markpit markpit marked this pull request as ready for review July 28, 2023 09:12
@markpit markpit marked this pull request as draft July 28, 2023 09:14
Copy link
Contributor

@mwestacott mwestacott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@markpit markpit marked this pull request as ready for review July 31, 2023 12:54
@markpit markpit marked this pull request as draft July 31, 2023 12:56
@markpit markpit changed the title ROE-2313 Trim leading zeros from some date fields ROE-2313 Trim leading zeros from date fields Jul 31, 2023
@dlloyd1-ch
Copy link
Contributor

Could we use the 'maxlength' attribute in the input field to prevent the issue happening in the first place?

@markpit
Copy link
Contributor Author

markpit commented Aug 2, 2023

Could we use the 'maxlength' attribute in the input field to prevent the issue happening in the first place?

that could be a better solution, I can try that out

@markpit
Copy link
Contributor Author

markpit commented Aug 14, 2023

Could we use the 'maxlength' attribute in the input field to prevent the issue happening in the first place?

Looking into using maxlength, the govuk nunjucks date component doesn't seem to support it and further digging led me to this question on the alphagov github site, where they don't recommend maxlength, but instead suggest displaying an error to the user. This PR doesn't match this approach (it auto trims the leading zeros) so perhaps solution might need to be re-done to display message to user instead - alphagov/govuk-design-system#1977

@dlloyd1-ch
Copy link
Contributor

Displaying an error sounds good, particularly if it's the recommend approach. It gives immediate feedback to the user and prevents us having to manipulate data that's been entered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants