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

Refactor: move IdG config to EligibilityVerifier #2291

Merged
merged 4 commits into from
Aug 12, 2024

Conversation

lalver1
Copy link
Member

@lalver1 lalver1 commented Aug 7, 2024

Closes #2237

Testing locally

  1. Checkout the main branch
  2. Make sure that your local_fixtures.json (or whatever fixture you are using) has a ClaimsProvider and an EligiblityVerifier that has a foreign key reference to that claims provider.
  3. Run bin/reset_db.sh (now your local DB mirrors the structure of the dev environment)
  4. Checkout this PR's branch
  5. Run bin/init.sh (running this PR's migration on top of existing DB structure)
  • For all EligiblityVerifiers that have a ClaimsProvider, confirm that the data in EligiblityVerifier.claims_provider.scope and EligiblityVerifier.claims_provider.claim was migrated to EligiblityVerifier.claims_scope and EligiblityVerifier.claims_claim, respectively.
  • Confirm that when the code calls anEligiblityVerifier.claims_scheme instance, the value of claims_scheme equals the value of the associated ClaimsProvider.scheme if claims_scheme is blank in the database. Otherwise, claims_scheme should equal the value set for EligiblityVerifier in the database.
  • Flows using scopes/claims from IdG should still work
  • In the admin panel, when not logged in as a super user, check that Claims scheme under a selected Eligibility verifiers is read-only.

Notes/Questions

@claims_scheme.setter
def _set_claims_scheme(self, value):
    self._claims_scheme = value

actually needed? I included it because I thought that it would be required for setting the value using the admin interface, but I did some testing (with it removed) and it doesn't seem to be needed. We also don't set claims_scheme in the code, so maybe I should remove it?

@lalver1 lalver1 self-assigned this Aug 7, 2024
@github-actions github-actions bot added back-end Django views, sessions, middleware, models, migrations etc. migrations [auto] Review for potential model changes/needed data migrations updates deployment-dev [auto] Changes that will trigger a deploy if merged to dev and removed back-end Django views, sessions, middleware, models, migrations etc. labels Aug 7, 2024
Copy link

github-actions bot commented Aug 8, 2024

Coverage report

Click to see where and how coverage changed

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

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

@thekaveman thekaveman added this to the Admin tool: agency users milestone Aug 8, 2024
@lalver1 lalver1 force-pushed the refactor/move-IdG-config branch 4 times, most recently from 3aeecad to 7c2ed43 Compare August 9, 2024 14:24
Copy link

github-actions bot commented Aug 9, 2024

@lalver1 lalver1 marked this pull request as ready for review August 9, 2024 16:34
@lalver1 lalver1 requested a review from a team as a code owner August 9, 2024 16:34
Comment on lines 212 to 214
@property
def supports_claims_verification(self):
return bool(self.claims_scope) and bool(self.claims_claim)
Copy link
Member

Choose a reason for hiding this comment

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

I think we can put this logic inline in uses_claims_verification and remove this method.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, removing this method made the model a bit simpler 👍

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.

This is largely looking good 👍

I think the claims_scheme field needs a little more work though.

benefits/core/models.py Outdated Show resolved Hide resolved
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.

Getting closer

benefits/core/models.py Outdated Show resolved Hide resolved
benefits/core/migrations/local_fixtures.json Show resolved Hide resolved
benefits/core/models.py Outdated Show resolved Hide resolved
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.

This LGTM! And I tested it locally with dev IdG and the older adults flow, worked ✔️

@lalver1 lalver1 merged commit bcb06cf into main Aug 12, 2024
17 checks passed
@lalver1 lalver1 deleted the refactor/move-IdG-config branch August 12, 2024 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Model refactor: move IdG config to EligibilityVerifier
3 participants