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

Feat: analytics for auth claims flow #2125

Merged
merged 4 commits into from
Jun 6, 2024
Merged

Conversation

lalver1
Copy link
Member

@lalver1 lalver1 commented May 29, 2024

Closes #2049

How to test

To test the error condition, insert something like claim_value = "20" after line 73 in benefits.oauth.views.authorize. Then check that "error_code": 20 appears as a property of "event_properties" in the log.

Notes

The behavior originally described in these notes was fixed by #2127.

When running the test test_authorize_success_without_verifier_claim, the oauth middleware VerifierUsesAuthVerificationSessionRequired that decorates the authorize view seems to behave differently compared to debugging the application.

Specifically, line 19 in benefits/oauth/middleware.py evaluates to a <Mock> object if running the test, but it evaluates to a boolean if debugging the application.

To see this behavior, debug the test and notice that line 19 evaluates to a<Mock> object, so the if condition evaluates to True and returns None instead of returning the user error page.
However, when debugging the application and setting claim in AuthProvider to blank (to simulate a missing verifier claim in the database), line 19 will evaluate to False and the conditional will return the user error page.

Also, seems like verifier.auth_provider.claim = "" in the test only sets verifier_claim = verifier.auth_provider.claim (line 64 in the view). For a test closer to the behavior being tested, we may need to set the claim to blank in a fixture similar to model_AuthProvider_with_verification.

@lalver1 lalver1 self-assigned this May 29, 2024
@github-actions github-actions bot added back-end Django views, sessions, middleware, models, migrations etc. deployment-dev [auto] Changes that will trigger a deploy if merged to dev tests Related to automated testing (unit, UI, integration, etc.) and removed deployment-dev [auto] Changes that will trigger a deploy if merged to dev labels May 29, 2024
@lalver1 lalver1 force-pushed the feat/analytics-oauth-claims branch from 36ca2eb to 477edcb Compare May 29, 2024 22:37
@lalver1 lalver1 force-pushed the feat/analytics-oauth-claims branch from 477edcb to b67a7b1 Compare June 3, 2024 20:05
Copy link

github-actions bot commented Jun 3, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  benefits/oauth
  analytics.py 68
  views.py
Project Total  

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

@lalver1 lalver1 force-pushed the feat/analytics-oauth-claims branch from a10bafe to f3cba88 Compare June 4, 2024 15:50
@lalver1 lalver1 marked this pull request as ready for review June 4, 2024 16:39
@lalver1 lalver1 requested a review from a team as a code owner June 4, 2024 16:39
@lalver1 lalver1 force-pushed the feat/analytics-oauth-claims branch from f3cba88 to 4b253c2 Compare June 4, 2024 17:35
@lalver1
Copy link
Member Author

lalver1 commented Jun 4, 2024

I'm unsure what to do about missing tests for lines 68-71 in benefits.oauth.analytics.finished_sign_in since finished_sign_in doesn't return a value; I likely need a mocker.
We addressed this during our dev workshop 👍

@lalver1 lalver1 marked this pull request as draft June 5, 2024 19:49
@lalver1 lalver1 marked this pull request as draft June 5, 2024 19:49
@lalver1 lalver1 force-pushed the feat/analytics-oauth-claims branch from 4b253c2 to 3efdc5b Compare June 5, 2024 20:07
@lalver1 lalver1 marked this pull request as ready for review June 5, 2024 20:26
@lalver1 lalver1 force-pushed the feat/analytics-oauth-claims branch from 3efdc5b to 75cfeb0 Compare June 6, 2024 15:57
@lalver1 lalver1 merged commit 55c8905 into dev Jun 6, 2024
10 checks passed
@lalver1 lalver1 deleted the feat/analytics-oauth-claims branch June 6, 2024 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
back-end Django views, sessions, middleware, models, migrations etc. tests Related to automated testing (unit, UI, integration, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve analytics for auth claims flow
3 participants