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

Refactor: use ClaimsVerificationRequest model from cdt_identity #2782

Merged
merged 5 commits into from
Mar 27, 2025
Merged

Refactor: use ClaimsVerificationRequest model from cdt_identity #2782

merged 5 commits into from
Mar 27, 2025
+145 −118

Conversation

lalver1
Copy link
Member

@lalver1 lalver1 commented Mar 25, 2025 •
edited
Loading

Closes #2721

This PR implements the ClaimsVerificationRequest refactor. We first add a claims_request field (which is a foreign key to ClaimsVerificationRequest, a new model from cdt_identity) to EnrollmentFlow, migrate data from EnrollmentFlow to ClaimsVerificationRequest, and finally remove from EnrollmentFlow

  • claims_scope
  • claims_eligibility_claim
  • claims_extra_claims
  • claims_scheme_override

since they now reside in ClaimsVerificationRequest. This schematic shows how the fields for EnrollmentFlow ended up:

EnrollmentFlow ClaimsVerificationRequest
id id
system_name system_name
claims_scope scopes
claims_eligibility_claim eligibility_claim
claims_extra_claims extra_claims
claims_scheme_override scheme
claims_request (ClaimsVerificationRequest fk)
...

To review

  • Checkout main
  • Set DJANGO_DB_FIXTURES to local_fixtures.json and run bin/reset_db.sh to reset your database
  • Switch to this PR's branch
  • Run bin/init.sh to run the migrations in this branch and test that the data was migrated successfully from EnrollmentFlow to ClaimsVerificationRequest and that claims_request in EnrollmentFlow points to the correct ClaimsVerificationRequest
  • Start a debugger
  • Log in to the admin with your Google SSO account and enter valid values for any flow that uses the IdG (you can use Older Adult):
    • Identity gateway configs -> Client id
    • Identity gateway configs -> Authority
    • Claims verification requests -> Scopes
    • Claims verification requests -> Eligibility claim
  • Test the user facing flow that you configured and also test the admin using calitp-user
  • Test the local developer environment, ensure that the fixtures have been updated and that they successfully load by running bin/reset_db.sh

@lalver1 lalver1 self-assigned this Mar 25, 2025
@github-actions github-actions bot added deployment-dev [auto] Changes that will trigger a deploy if merged to dev back-end Django views, sessions, middleware, models, migrations etc. tests Related to automated testing (unit, UI, integration, etc.) migrations [auto] Review for potential model changes/needed data migrations updates and removed back-end Django views, sessions, middleware, models, migrations etc. labels Mar 25, 2025
@lalver1 lalver1 changed the title Refactor: use ClaimsVerificationRequest model from cdt_identity Refactor: use ClaimsVerificationRequest model from cdt_identity Mar 25, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 25, 2025 •
edited
Loading

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)		Lines missing
  benefits/core
  session.py
  benefits/core/admin
  enrollment.py
  benefits/core/models
  enrollment.py
  benefits/eligibility
  verify.py
  benefits/oauth
  views.py
Project Total  

This report was generated by python-coverage-comment-action

@lalver1 lalver1 force-pushed the refactor/model-claimsverificationrequest branch from 1335c1b to 4937247 Compare March 26, 2025 13:23
@angela-tran angela-tran mentioned this pull request Mar 26, 2025
Merged
@lalver1 lalver1 force-pushed the refactor/model-claimsverificationrequest branch from 4937247 to 15c72d9 Compare March 26, 2025 23:38
@lalver1 lalver1 marked this pull request as ready for review March 26, 2025 23:47
@lalver1 lalver1 requested a review from a team as a code owner March 26, 2025 23:47
@thekaveman
Copy link
Member

thekaveman commented Mar 27, 2025 •
edited
Loading

You can squash 23b4c07 into fce3bad. Git is smart enough to figure out the file rename for the rest of the commits.

thekaveman
thekaveman reviewed Mar 27, 2025
View reviewed changes
Copy link
Member

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

Code changes look good 👍

Tested the data migration locally 👍

lalver1 added 5 commits March 27, 2025 13:18
@lalver1
refactor(models): relate ClaimsVerificationRequest with EnrollmentFlows
a552ba7
@lalver1
chore: migrate data related to ClaimsVerificationRequest
6a1deee 
creates ClaimsVerificationRequests for IdG systems and
sets up the relationship between EnrollmentFlows and
ClaimsVerificationRequest.
@lalver1
refactor(models): remove redundant EnrollmentFlow fields
4a83ea0 
claims_scope, claims_eligibility_claim, claims_extra_claims, and
claims_scheme_override are all redundant fields in EnrollmentFlow.
They will all be derived from the ClaimsVerificationRequest model.
@lalver1
chore: update fixtures for claimsverificationrequest and enrollmentflow
64b3d77
@lalver1
refactor(models): remove redundant claims_all_claims managed property
d56e0d6 
EnrollmentFlow.claims_all_claims is a managed property that is made
redundant by the ClaimsVerificationRequest.claims_list property.
Some tests were simplified since there is no need to test the case
when extra_claims=None since ClaimsVerificationRequest.extra_claims
is a string ("") when it is not set.
@lalver1 lalver1 force-pushed the refactor/model-claimsverificationrequest branch from 15c72d9 to d56e0d6 Compare March 27, 2025 13:29
@lalver1
Copy link
Member Author

lalver1 commented Mar 27, 2025

Thanks for the suggestion to remove @property claims_all_claims and squashing 23b4c07 into fce3bad! 👍

thekaveman
thekaveman approved these changes Mar 27, 2025
View reviewed changes
Copy link
Member

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

🚀

@lalver1 lalver1 merged commit 75ab4cf into main Mar 27, 2025
14 checks passed
@lalver1 lalver1 deleted the refactor/model-claimsverificationrequest branch March 27, 2025 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thekaveman thekaveman thekaveman approved these changes

Assignees

@lalver1 lalver1

Labels
deployment-dev [auto] Changes that will trigger a deploy if merged to dev migrations [auto] Review for potential model changes/needed data migrations updates tests Related to automated testing (unit, UI, integration, etc.)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OAuth refactor: update models
2 participants
@lalver1 @thekaveman