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

[CPDLP-3631] Ensure we migrate all relevant timestamps from ECF #1848

Merged
merged 9 commits into from
Oct 25, 2024
Merged

[CPDLP-3631] Ensure we migrate all relevant timestamps from ECF #1848

merged 9 commits into from
Oct 25, 2024

Conversation

mooktakim
Copy link
Contributor

@mooktakim mooktakim commented Oct 23, 2024 •
edited
Loading

Context

Ticket: https://dfedigital.atlassian.net/browse/CPDLP-3631

Changes proposed in this pull request

  • Cohort, Schedule, ParticipantIdChange and Statement migrations will use timestamps from ECF
  • Updated paper_trail
    • object field type is now json
    • object_changes added
    • added note field
  • Application & User
    • Can now include version_note which adds versions.note entry into versions
    • Migration will use ECF timestamps and include a version_note
    • If there has been any changes from ECF attrs, we set updated_at to Time.zone.now

@mooktakim mooktakim requested a review from a team as a code owner October 23, 2024 15:39
@mooktakim mooktakim force-pushed the 3631-ensure-we-migrate-all-relevant-timestamps-from-ecf branch from 3e704bc to feef997 Compare October 23, 2024 15:41
@mooktakim mooktakim force-pushed the 3631-ensure-we-migrate-all-relevant-timestamps-from-ecf branch from cf977b7 to 68693b2 Compare October 24, 2024 09:56
ethax-ross
ethax-ross reviewed Oct 24, 2024
View reviewed changes
Copy link
Collaborator

@ethax-ross ethax-ross left a comment

Choose a reason for hiding this comment

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

Looks great just one question

db/migrate/20241023112339_change_object_type_to_json_for_versions.rb Show resolved Hide resolved
cwrw
cwrw reviewed Oct 24, 2024
View reviewed changes
app/services/migration/migrators/application.rb
@@ -25,6 +25,8 @@ class Application < Base
teacher_catchment_country
notes
funded_place
created_at
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here and in user, are we making sure this is covered?

Overwrite updated_at with the one from ECF, or set to the date of migration if anything changed from ECF

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated_at will be set to Time.now when:

  • Application ukprn is changed from ECF
  • User trn and email is changed from ECF

cwrw
cwrw reviewed Oct 24, 2024
View reviewed changes
Copy link
Collaborator

@cwrw cwrw left a comment

Choose a reason for hiding this comment

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

looking good with a few comments, can we also run this in migration keen to see how the parity checks look like and as discussed and how long it will take with papertrail now :)

@github-actions GitHub Actions
Copy link

Review app deployed to https://npq-registration-review-1848-web.test.teacherservices.cloud/

cwrw
cwrw approved these changes Oct 24, 2024
View reviewed changes
Copy link
Collaborator

@cwrw cwrw left a comment

Choose a reason for hiding this comment

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

🙌 thanks mook

leandroalemao
leandroalemao approved these changes Oct 24, 2024
View reviewed changes
Copy link
Contributor

@leandroalemao leandroalemao left a comment

Choose a reason for hiding this comment

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

Looking very good tks @mooktakim 👍

cwrw
cwrw reviewed Oct 25, 2024
View reviewed changes
app/models/user.rb Outdated Show resolved Hide resolved
@mooktakim mooktakim force-pushed the 3631-ensure-we-migrate-all-relevant-timestamps-from-ecf branch from d29fec4 to ae65ecc Compare October 25, 2024 08:58
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Oct 25, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@mooktakim mooktakim added this pull request to the merge queue Oct 25, 2024
Merged via the queue into main with commit e6b4ca0 Oct 25, 2024
18 checks passed
@mooktakim mooktakim deleted the 3631-ensure-we-migrate-all-relevant-timestamps-from-ecf branch October 25, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwrw cwrw cwrw approved these changes

@leandroalemao leandroalemao leandroalemao approved these changes

@ethax-ross ethax-ross ethax-ross approved these changes

Assignees

@leandroalemao leandroalemao

@ethax-ross ethax-ross

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mooktakim @cwrw @leandroalemao @ethax-ross