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

Add supported_methods field to EnrollmentFlow #2383

Merged
merged 4 commits into from
Sep 23, 2024

Conversation

machikoyasuda
Copy link
Member

@machikoyasuda machikoyasuda commented Sep 19, 2024

closes #2287

What this PR does

  • Note: This PR currently does not add any checks throughout the app to check if a flow is supported in the in-person or digital apps.
  • Adds https://pypi.org/project/django-multiselectfield/ package
  • Adds supported_enrollment_methods field to EnrollmentFlow, as radio buttons that can be both selected
  • The default for a new flow is having both selected: http://localhost:11369/admin/core/enrollmentflow/add/
  • Updates local fixtures: All existing enrollment flows are supported both digitally and in-person, adds a sample In-Person Only and Digital Only --- but does not add those flows to CST.
  • Add spec
image

@machikoyasuda machikoyasuda requested a review from a team as a code owner September 19, 2024 21:01
@machikoyasuda machikoyasuda self-assigned this Sep 19, 2024
@github-actions github-actions bot added tests Related to automated testing (unit, UI, integration, etc.) back-end Django views, sessions, middleware, models, migrations etc. deployment-dev [auto] Changes that will trigger a deploy if merged to dev migrations [auto] Review for potential model changes/needed data migrations updates labels Sep 19, 2024
@machikoyasuda machikoyasuda marked this pull request as draft September 19, 2024 21:21
@machikoyasuda machikoyasuda force-pushed the feat/2287-enrollment-flow-supported-methods-again branch from 7bbe713 to 2834a2f Compare September 19, 2024 21:35
@machikoyasuda
Copy link
Member Author

Not sure why both Pytest and Cypress tests are failing here. Both suites pass for me locally. 🤨

Copy link

github-actions bot commented Sep 23, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  benefits/core
  models.py
Project Total  

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

@machikoyasuda
Copy link
Member Author

machikoyasuda commented Sep 23, 2024

@angela-tran and I learnt something new today:

A GitHub repo for a PyPI package might not reflect the latest release! The release page for https://github.com/goinnn/django-multiselectfield shows that the latest version is v0.1.12. But we noticed that when I had installed django-multiselectfield with the command, pip install django-multiselectfield, version 0.1.13 was installed -- and, I didn't get any failing testes locally. So we went to the PyPI page for the package directly and saw a 0.1.13 release: https://pypi.org/project/django-multiselectfield/0.1.13/, which fixed the bug that was breaking my tests.

As a result, I simply updated the version to 0.1.13, and all the tests were fixed 🎉 No hacky fixes required.

Lesson learnt: Check both the GitHub and PyPI page for the package you are using.

@machikoyasuda machikoyasuda force-pushed the feat/2287-enrollment-flow-supported-methods-again branch from 90566b0 to f066e34 Compare September 23, 2024 19:25
@machikoyasuda machikoyasuda marked this pull request as ready for review September 23, 2024 19:26
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 looks great!

This PR currently does not update existing enrollment flows in the fixture data, nor does it add any checks throughout the app to check if a flow is supported in the in-person or digital apps.

I think for existing fixtures e.g. in dev, test, prod -- this is fine. Since the field defines a default value, we'll be good there and the flows will be updated in the database next time they are edited.

I do think it would be worth it to update our local fixtures in this PR though, just for completeness of the data when starting from scratch. And to show a full example of how the config works together.

The other minor change, can we change the label shown to admin users for the new field to: Supported enrollment methods? This is to align more closely with @indexing's comment on the analytics side: #2245 (comment)

@machikoyasuda
Copy link
Member Author

@thekaveman That makes sense. Should I set all Enrollment Flows to accept both methods?

@thekaveman
Copy link
Member

@thekaveman That makes sense. Should I set all Enrollment Flows to accept both methods?

Let's set all our existing fixtures to both 👍

Maybe add one basic sample each of a digital only and in_person only, if that feels reasonable?

@machikoyasuda
Copy link
Member Author

machikoyasuda commented Sep 23, 2024

@thekaveman Updated all:

  • Existing flows are supported by both enrollment methods
  • Adds a sample Digital only and In-Person only method -- but does not add them to the CST Agency. 9b2e3f6
  • Renames the field to supported_enrollment_method -- this is the cleanest way to change the display name, plus it makes for more readable code, I think. a12349b

@machikoyasuda machikoyasuda force-pushed the feat/2287-enrollment-flow-supported-methods-again branch from a12349b to 0162e64 Compare September 23, 2024 21:48
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.

Sorry, I just realized one more thing, but it can be left for a follow-up.

We need to actually use this field! In the views that get the list of flows for the user to select from.

@machikoyasuda machikoyasuda merged commit 2051a48 into main Sep 23, 2024
15 checks passed
@machikoyasuda machikoyasuda deleted the feat/2287-enrollment-flow-supported-methods-again branch September 23, 2024 23:24
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. deployment-dev [auto] Changes that will trigger a deploy if merged to dev migrations [auto] Review for potential model changes/needed data migrations updates tests Related to automated testing (unit, UI, integration, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance EnrollmentFlow model with field to indicate it is a pathway that can only be done in-person
2 participants