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

Replace the legacy format functions with formatDate #7387

Merged
merged 16 commits into from
Dec 11, 2024

Conversation

paulgain
Copy link
Contributor

@paulgain paulgain commented Dec 9, 2024

Description of change

This PR replaces the following format functions with the dateFormat function:

  • formatMediumDate
  • formatMediumDateTime
  • formatMediumDateTimeWithoutParsing
  • formatLongDate
  • formatShortDate
  • formatMediumDateParsed

Checklist

  • Has the branch been rebased to main?
  • Automated tests (Any of the following when applicable: Unit, Functional or End-to-End)
  • Manual compatibility testing (Browsers: Chrome, Firefox, Edge, Safari)

Copy link

cypress bot commented Dec 9, 2024

data-hub-frontend    Run #57910

Run Properties:  status check passed Passed #57910  •  git commit a376ecc7ed: Fix typo
Project data-hub-frontend
Branch Review fix/remove-format-functions
Run status status check passed Passed #57910
Run duration 01m 39s
Commit git commit a376ecc7ed: Fix typo
Committer Paul Gain
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 15
View all changes introduced in this branch ↗︎

Copy link

codecov bot commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 95.94595% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.80%. Comparing base (b77df74) to head (a376ecc).
Report is 36 commits behind head on main.

Files with missing lines Patch % Lines
...dules/Contacts/ContactAuditHistory/transformers.js 0.00% 1 Missing ⚠️
...s/Investments/Projects/EditHistory/transformers.js 0.00% 1 Missing ⚠️
...ent/modules/Tasks/TaskDetails/TaskDetailsTable.jsx 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7387      +/-   ##
==========================================
- Coverage   87.98%   87.80%   -0.18%     
==========================================
  Files        1168     1168              
  Lines       18171    18163       -8     
  Branches     5138     5136       -2     
==========================================
- Hits        15987    15948      -39     
- Misses       2184     2215      +31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@paulgain paulgain marked this pull request as ready for review December 10, 2024 16:20
@paulgain paulgain requested a review from a team as a code owner December 10, 2024 16:20
Copy link
Contributor

@baarkerlounger baarkerlounger left a comment

Choose a reason for hiding this comment

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

Worth adding tests for the missing lines?

Copy link
Contributor

@oliverjwroberts oliverjwroberts 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. Had a couple of comments, but nothing blocking the PR.

src/client/utils/__test__/date-utils.test.js Outdated Show resolved Hide resolved
src/client/utils/date-utils.js Show resolved Hide resolved
@peterhudec
Copy link
Contributor

peterhudec commented Dec 11, 2024

Worth adding tests for the missing lines?

We turned off the coverage as a required check so I'm not sure whether we really need to make all those lines covered. But the coverage report reveals some fundamental issues with our code:

How can created_on ever be null? Was it never created? Or has it not been created yet?
Screenshot 2024-12-11 at 10 35 52

Does it make sense for interaction.date to be empty? What kind of interaction would that be?
.../modules/Companies/CompanyActivity/transformers.js
Screenshot 2024-12-11 at 10 37 31

Should we really validate date strings in ad hoc places and just fall back to a not set placeholder if it is invalid? All the dates come data-hub-api, don't we trust that service? If the API gives us invalid dates, this is a problem and the FE should not brush it under the carpet as a not set value.
...dules/Contacts/ContactAuditHistory/transformers.js
Screenshot 2024-12-11 at 10 41 11

@baarkerlounger
Copy link
Contributor

baarkerlounger commented Dec 11, 2024

@peterhudec it looks like created_on is a nullable field at the backend database level in most cases. There might be cases where it's not set if the record hasn't been created through the Django ORM - though that probably shouldn't really be happening.

@paulgain
Copy link
Contributor Author

paulgain commented Dec 11, 2024

Worth adding tests for the missing lines?

@baarkerlounger I've fixed 3 of these, however, there are still 3 outstanding, the top 2 will be addressed in the next PR as I need to remove the isUnparsedDateValid function. The last one is covered by component tests (see the links below), component tests have not been instrumented by nyc so they do not show up in the Codecov report.
https://github.com/uktrade/data-hub-frontend/blob/main/test/component/cypress/specs/Tasks/TaskDetails/TaskDetailsTable.cy.jsx#L45
https://github.com/uktrade/data-hub-frontend/blob/main/test/component/cypress/specs/Tasks/TaskDetails/TaskDetailsTable.cy.jsx#L88

@paulgain
Copy link
Contributor Author

@peterhudec and I jumped on a call to discuss his comments in detail 😄 .

@paulgain paulgain merged commit 6d5a9a2 into main Dec 11, 2024
16 checks passed
@paulgain paulgain deleted the fix/remove-format-functions branch December 11, 2024 17:48
chopkinsmade pushed a commit that referenced this pull request Dec 11, 2024
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.

4 participants