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-3814] NPQ Post Separation Cleanup - Remove NPQ specific tables from ECF #5374

Merged
merged 19 commits into from
Jan 8, 2025
Merged

[CPDLP-3814] NPQ Post Separation Cleanup - Remove NPQ specific tables from ECF #5374

merged 19 commits into from
Jan 8, 2025

Conversation

mooktakim
Copy link
Contributor

@mooktakim mooktakim commented Dec 18, 2024 •
edited
Loading

Context

Changes proposed in this pull request

  • Remove the following models and tables:
    • NPQ applications
    • NPQ contracts
    • NPQ courses
    • NPQ lead providers
    • NPQ application exports

Guidance to review

@mooktakim mooktakim changed the base branch from main to CPDLP-3840 December 18, 2024 10:33
@github-actions GitHub Actions
Copy link

Review app deployed to https://cpd-ecf-review-5374-web.test.teacherservices.cloud

@mooktakim mooktakim marked this pull request as ready for review December 18, 2024 17:39
@leandroalemao leandroalemao force-pushed the CPDLP-3840 branch 4 times, most recently from fde1522 to c0f9ecc Compare December 19, 2024 11:31
leandroalemao
leandroalemao reviewed Dec 19, 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.

Looks good 👍 but maybe we could wait for the other PR to be tested first? as many changes are included there as well..

@mooktakim mooktakim force-pushed the CPDLP-3840 branch 2 times, most recently from 4579553 to c0f9ecc Compare December 23, 2024 10:47
Base automatically changed from CPDLP-3840 to main December 23, 2024 14:38
@mooktakim mooktakim force-pushed the 3814-npq-post-separation-cleanup-remove-npq-specific-tables-from-ecf branch from 7eac355 to 1a01ad7 Compare December 23, 2024 16:22
@mooktakim mooktakim force-pushed the 3814-npq-post-separation-cleanup-remove-npq-specific-tables-from-ecf branch from 1a01ad7 to 7b08491 Compare December 24, 2024 10:02
cwrw
cwrw reviewed Dec 27, 2024
View reviewed changes
spec/services/record_declaration_spec.rb Outdated Show resolved Hide resolved
spec/models/participant_outcome/npq_spec.rb Outdated Show resolved Hide resolved
spec/requests/api/v1/participant_declarations_spec.rb Outdated Show resolved Hide resolved
spec/requests/api/v2/participant_declarations_spec.rb Outdated Show resolved Hide resolved
spec/requests/api/v3/participant_declarations_spec.rb Outdated Show resolved Hide resolved
spec/forms/choose_role_form_spec.rb Show resolved Hide resolved
app/policies/npq_application_policy.rb Show resolved Hide resolved
spec/forms/nominate_induction_tutor_form_spec.rb Outdated Show resolved Hide resolved
spec/forms/schools/add_participants/add_wizard_spec.rb Outdated Show resolved Hide resolved
db/migrate/20220530141418_add_national_institute_of_teaching_npq_lead_provider.rb Show resolved Hide resolved
@mooktakim mooktakim force-pushed the 3814-npq-post-separation-cleanup-remove-npq-specific-tables-from-ecf branch from 4a177b9 to 9f8e1ed Compare December 27, 2024 16:33
@mooktakim mooktakim force-pushed the 3814-npq-post-separation-cleanup-remove-npq-specific-tables-from-ecf branch from 9f8e1ed to 6769222 Compare December 30, 2024 10:20
@mooktakim mooktakim mentioned this pull request Jan 3, 2025
Open
ethax-ross
ethax-ross approved these changes Jan 6, 2025
View reviewed changes
Copy link
Contributor

@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 good, I'm not sure if we care too much, but if you apply the migration and then roll it back and diff against main there are a few foreign keys missing:

-  add_foreign_key "npq_application_exports", "users"
-  add_foreign_key "npq_applications", "npq_courses"
-  add_foreign_key "npq_applications", "npq_lead_providers"
-  add_foreign_key "npq_applications", "participant_identities"
-  add_foreign_key "npq_applications", "users", column: "eligible_for_funding_updated_by_id"

Has this been tested on the snapshot environment?

@mooktakim
Copy link
Contributor Author

Looks good, I'm not sure if we care too much, but if you apply the migration and then roll it back and diff against main there are a few foreign keys missing:

-  add_foreign_key "npq_application_exports", "users"
-  add_foreign_key "npq_applications", "npq_courses"
-  add_foreign_key "npq_applications", "npq_lead_providers"
-  add_foreign_key "npq_applications", "participant_identities"
-  add_foreign_key "npq_applications", "users", column: "eligible_for_funding_updated_by_id"

Has this been tested on the snapshot environment?

Migration has been run on snapshot 👍

I'm thinking we ignore the foreign keys as we won't need it

@mooktakim mooktakim force-pushed the 3814-npq-post-separation-cleanup-remove-npq-specific-tables-from-ecf branch from 00aafc5 to 856360e Compare January 8, 2025 15:51
leandroalemao and others added 19 commits January 8, 2025 15:53
@leandroalemao @mooktakim
[CPDLP-3840] Remove NPQ logic from RecordDeclaration
2dc2df6
@leandroalemao @mooktakim
[CPDLP-3840] Clean up services that uses NPQ declaration factory
2b426f4
@mooktakim
[CPDLP-3814] removed NPQContract model and everything related
e42a9bf
@mooktakim
[CPDLP-3814] removed npq_application_exports table
f5ec3c5
@mooktakim
[CPDLP-3814] remove npq_applications table, model and related stuff
a7b440d
@mooktakim
[CPDLP-3814] remove NPQCourse model and table
ca6b0f1
@mooktakim
[CPDLP-3814] remove NPQLeadProvider model and table
3739610
@mooktakim
[CPDLP-3814] rubocop fix
52fa0bb
@mooktakim
[CPDLP-3814] check if foreign key exists
e627b16
@mooktakim
[CPDLP-3814] remove 'NPQLeadProvider' from old migration files
2385f18
@mooktakim
[CPDLP-3814] reverted npq declaration request test removal
56c662b
@mooktakim
[CPDLP-3814] reverted and brought back npq_participant_profile to avo…
2d2b495 
…id too many changes
@mooktakim
[CPDLP-3814] removed tables that dont exist anymore from analytics_bl…
d9f35ed 
…ocklist.yml
@mooktakim
[CPDLP-3814] remove Importers::NPQManualValidation instructions from …
e2bd58d 
…README
@mooktakim
[CPDLP-3814] removed seed quantities that are not needed anymore
66970df
@mooktakim
[CPDLP-3814] removed 'data_cleanup:fix_applications_in_ecf_not_in_npq…
52a3d22 
…' rake task
@mooktakim
[CPDLP-3814] removed methods to keep skeleton model
29dee80
@mooktakim
[CPDLP-3814] Leave ParticipantProfile::NPQ with minimal changes
1ccf5a2
@mooktakim
[CPDLP-3814] fixture api spec cleanup
673893e
@mooktakim mooktakim force-pushed the 3814-npq-post-separation-cleanup-remove-npq-specific-tables-from-ecf branch from 856360e to 673893e Compare January 8, 2025 15:53
@mooktakim mooktakim added this pull request to the merge queue Jan 8, 2025
Merged via the queue into main with commit 47cc672 Jan 8, 2025
29 checks passed
@mooktakim mooktakim deleted the 3814-npq-post-separation-cleanup-remove-npq-specific-tables-from-ecf branch January 8, 2025 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@leandroalemao leandroalemao leandroalemao approved these changes

@cwrw cwrw cwrw 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