-
Notifications
You must be signed in to change notification settings - Fork 9
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
Conversation
5a64bb7
to
339b5b6
Compare
c1beab1
to
75a9037
Compare
3aeecad
to
7c2ed43
Compare
Preview url: https://benefits-2291--cal-itp-previews.netlify.app |
99485ff
to
c5130db
Compare
benefits/core/models.py
Outdated
@property | ||
def supports_claims_verification(self): | ||
return bool(self.claims_scope) and bool(self.claims_claim) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
There was a problem hiding this 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.
c5130db
to
27f8ad8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting closer
27f8ad8
to
163054e
Compare
There was a problem hiding this 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 ✔️
Closes #2237
Testing locally
main
branchlocal_fixtures.json
(or whatever fixture you are using) has aClaimsProvider
and anEligiblityVerifier
that has a foreign key reference to that claims provider.bin/reset_db.sh
(now your local DB mirrors the structure of thedev
environment)bin/init.sh
(running this PR's migration on top of existing DB structure)EligiblityVerifier
s that have aClaimsProvider
, confirm that the data inEligiblityVerifier.claims_provider.scope
andEligiblityVerifier.claims_provider.claim
was migrated toEligiblityVerifier.claims_scope
andEligiblityVerifier.claims_claim
, respectively.EligiblityVerifier.claims_scheme
instance, the value ofclaims_scheme
equals the value of the associatedClaimsProvider.scheme
ifclaims_scheme
is blank in the database. Otherwise,claims_scheme
should equal the value set forEligiblityVerifier
in the database.Claims scheme
under a selectedEligibility verifiers
is read-only.Notes/Questions
claims_scheme
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?