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: analytics for EnrollmentFlow #2379

Merged
merged 6 commits into from
Sep 23, 2024
Merged

Refactor: analytics for EnrollmentFlow #2379

merged 6 commits into from
Sep 23, 2024

Conversation

thekaveman
Copy link
Member

@thekaveman thekaveman commented Sep 19, 2024 •
edited
Loading

Part of #2248, update new events that are sent.

  • Renames event/user property eligibility_types to enrollment_flows
  • Renames event selected eligibility verifier to selected enrollment flow
  • Updates the value sent for event/user property eligibility_verifier to the same logic as from Model and capture local enrollment events #2267

@thekaveman thekaveman self-assigned this Sep 19, 2024
@thekaveman thekaveman added this to the Admin tool: agency users milestone Sep 19, 2024
@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.) documentation [auto] Improvements or additions to documentation and removed tests Related to automated testing (unit, UI, integration, etc.) labels Sep 19, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 19, 2024 •
edited
Loading

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)		Lines missing
  benefits/core
  analytics.py
  models.py
  benefits/eligibility
  analytics.py 21, 43, 53, 58
  views.py
  benefits/enrollment
  views.py
Project Total  

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

@github-actions GitHub Actions
Copy link

Preview url: https://benefits-2379--cal-itp-previews.netlify.app

@thekaveman thekaveman mentioned this pull request Sep 19, 2024
Merged
10 tasks
@thekaveman thekaveman marked this pull request as ready for review September 19, 2024 21:49
@thekaveman thekaveman requested a review from a team as a code owner September 19, 2024 21:49
@thekaveman thekaveman force-pushed the refactor/analytics-enrollmentflow branch 2 times, most recently from b3d0be1 to bf2632a Compare September 19, 2024 22:30
angela-tran
angela-tran reviewed Sep 20, 2024
View reviewed changes
Copy link
Member

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

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

The code changes look good to me! Just left a suggestion on naming.

benefits/eligibility/analytics.py Outdated Show resolved Hide resolved
benefits/eligibility/analytics.py Outdated Show resolved Hide resolved
benefits/eligibility/analytics.py Outdated Show resolved Hide resolved
benefits/eligibility/analytics.py Outdated Show resolved Hide resolved
benefits/eligibility/analytics.py Outdated Show resolved Hide resolved
benefits/eligibility/analytics.py Outdated Show resolved Hide resolved
benefits/eligibility/analytics.py Outdated Show resolved Hide resolved
benefits/eligibility/analytics.py Outdated Show resolved Hide resolved
benefits/eligibility/analytics.py Outdated Show resolved Hide resolved
@thekaveman thekaveman force-pushed the refactor/analytics-enrollmentflow branch from e3de3c5 to 455b011 Compare September 21, 2024 00:23
@thekaveman
refactor(analytics): pass the flow in call chain
b7d9993 
hide the usage of flow.system_name from callers, this is an
implementation detail of the analytics code
@thekaveman thekaveman force-pushed the refactor/analytics-enrollmentflow branch from 455b011 to b7d9993 Compare September 21, 2024 00:58
@thekaveman
Copy link
Member Author

I refactored this in b7d9993, good call!

The logic for getting the value for analytics is now in one place, and hidden away from callers.

angela-tran
angela-tran reviewed Sep 23, 2024
View reviewed changes
benefits/eligibility/analytics.py
Comment on lines -42 to -43
if status == "success":
self.update_user_properties(eligibility_types=eligibility_types)
Copy link
Member

Choose a reason for hiding this comment

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

Just noting for future reference that now we'll capture the enrollment flow as a user property for all ReturnedEligibilityEvents, not just successful ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

This logic is (was) already present ... i.e. creating a new EligibilityEvent always updated the event/user properties with the flow (eligibility type in the past).

I couldn't find a reason for this specific case of also updating it on success... it would have already been done by the user selecting their flow.

Good call out though.

angela-tran
angela-tran approved these changes Sep 23, 2024
View reviewed changes
Copy link
Member

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

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

Looks good!

@thekaveman thekaveman merged commit cff69d4 into main Sep 23, 2024
17 checks passed
@thekaveman thekaveman deleted the refactor/analytics-enrollmentflow branch September 23, 2024 23:12
@thekaveman thekaveman mentioned this pull request Sep 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@angela-tran angela-tran angela-tran approved these changes

Assignees

@thekaveman thekaveman

Labels
back-end Django views, sessions, middleware, models, migrations etc. deployment-dev [auto] Changes that will trigger a deploy if merged to dev documentation [auto] Improvements or additions to documentation
Projects
None yet
Milestone
Benefits admin tool (agency users)
Development

Successfully merging this pull request may close these issues.
2 participants
@thekaveman @angela-tran