-
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
Feat: distinguish between Older Adult and Disabled riders #2463
base: main
Are you sure you want to change the base?
Conversation
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
021b4bc
to
57ba9b1
Compare
57ba9b1
to
632d610
Compare
@@ -148,17 +148,17 @@ def logged_in(request): | |||
|
|||
def logout(request): | |||
"""Reset the session claims and tokens.""" | |||
update(request, oauth_claim=False, oauth_token=False, enrollment_token=False) | |||
update(request, oauth_claims=False, oauth_token=False, enrollment_token=False) |
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 know you didn't introduce this, but I'm wondering if False
makes sense here, especially now with the list approach? Should we set this to an empty list []
instead?
stored_claims.append(claim) | ||
elif claim_value >= 10 and claim == flow.claims_eligibility_claim: | ||
# error_claim is only set if claim is the eligibility claim | ||
error_claim = claim_value |
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 can be saved for the follow-up PR that implements analytics with the new extra_claims
...
I'm thinking we should send all the errors with the analytics event, instead of just the error on the eligibility_claim
.
E.g. make error_claim
a dict
for {"claim": "error_code"}
and send that with the analytics.
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.
Yeah, I agree we haven't discussed how and which error codes should be sent, so we can save it for the other PR. Good to note this though
@property | ||
def claims_all_claims(self): | ||
claims = [self.claims_eligibility_claim] | ||
claims.extend(self.claims_extra_claims.split()) |
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 like this helper to calculate the full list in the model.
However this call to .split()
will throw an error if self.claims_extra_claims
is None
(the default).
We should either set the default to an empty string, or handle when this is None
.
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 and want to add that we should have a unit test that covers this.
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 noted some minor fixes to make to some unit tests. Other than that and what @thekaveman pointed out, the main code changes here look good!
@@ -199,16 +199,16 @@ def test_logged_in_True(app_request): | |||
|
|||
@pytest.mark.django_db | |||
def test_logout(app_request): | |||
session.update(app_request, oauth_claim="oauth_claim", oauth_token="oauth_token", enrollment_token="enrollment_token") | |||
session.update(app_request, oauth_claims="oauth_claim", oauth_token="oauth_token", enrollment_token="enrollment_token") |
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 should pass in a list like oauth_claims=["oauth_claim"]
to be more aligned with what application code is doing.
@@ -269,12 +269,12 @@ def test_reset_enrollment(app_request): | |||
@pytest.mark.django_db | |||
def test_reset_oauth(app_request): | |||
app_request.session[session._OAUTH_TOKEN] = "oauthtoken456" | |||
app_request.session[session._OAUTH_CLAIM] = "claim" | |||
app_request.session[session._OAUTH_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.
Similar comment as above: I think this should be a list like app_request.session[session._OAUTH_CLAIMS] = ["claim"]
to be more aligned with what the value would actually be set to.
Part of #2355
This PR implements a way to distinguish between older adult and disabled Medicare sessions.
EnrollmentFlow
is modified so that it now stores a "main" claim and an optional set of "extra" claims. An@property
method on the model takes care of arranging them into one list of claims. The code now handles this list of claims everywhere that multiple claims are involved.session
has been modified to store and process this list of claims to make it available to verify and analytics. Updating analytics will probably be part of a separate PR.