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: Cal-ITP staff can add EnrollmentFlow #2654

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented Jan 30, 2025

Part of #2201

Allows Cal-ITP staff members to add new EnrollmentFlow objects.

@angela-tran angela-tran self-assigned this Jan 30, 2025
Copy link

github-actions bot commented Jan 30, 2025

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  benefits/core/admin
  enrollment.py 96
Project Total  

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

@github-actions github-actions bot added back-end Django views, sessions, middleware, models, migrations etc. tests Related to automated testing (unit, UI, integration, etc.) and removed back-end Django views, sessions, middleware, models, migrations etc. labels Jan 30, 2025
@angela-tran angela-tran force-pushed the feat/staff-add-enrollmentflow branch 2 times, most recently from 42ec34f to c28fdab Compare January 30, 2025 21:43
Base automatically changed from feat/users-helper-functions to main January 30, 2025 22:13
@angela-tran angela-tran force-pushed the feat/staff-add-enrollmentflow branch from e41efcf to 7bc6687 Compare January 30, 2025 22:36
angela-tran and others added 5 commits February 6, 2025 00:39
even if the EnrollmentFlow is not fully configured, we want the staff
member to be able to save whatever they have so far.

the test uses the same `ModelForm` class that the
`SortableEnrollmentFlowAdmin` will provide to the admin interface.
we would want the form to show the user helpful error messages, telling
them what is missing.

these tests are asserting the desired behavior, but they're currently
failing and show that our validation logic will raise a ValueError when
it tries to add field-level ValidationErrors.

this is because the validation is trying to tell the form about
field-level errors, but the staff member's form doesn't even show some
of those fields.
prevents an error in the admin for the following use-case:

* non-superuser, member of the Cal-ITP Staff group
* create a new EnrollmentFlow
* assign a TransitAgency
* save

before this commit, clean() threw a validation error as the
eligibility_api_url and eligibility_form_class fields were both:

* required
* read-only for this user type

this commit changes the clean() validation to only check for API fields
when the flow actually uses API verification
inverts the logic on a number of checks, where the special case is
using Eligibility API verification, and the base case is using claims
refine the tests to show that there is still incorrect behavior, namely
that as long as there are no template errors, the model validation
allows an incomplete EnrollmentFlow to be saved with a transit agency.
@angela-tran angela-tran force-pushed the feat/staff-add-enrollmentflow branch from 5229140 to 05d9ad5 Compare February 6, 2025 00:41
this is solely a code refactor. this doesn't change the behavior of the
bug.

remove tests that were for model validation logic that's been moved
to the form.
the EnrollmentFlowForm will raise an error if a staff member tries to
save an incomplete EnrollmentFlow with an agency
show them the field name for the fields they can't edit and are missing
@angela-tran
Copy link
Member Author

I just have one more test to fix 💪 😤

It's test_TransitAgency_clean_dirty_flow which asserts that saving a TransitAgency linked to an incomplete EnrollmentFlow will fail validation.

The test needs to be refactored to go through forms rather than models so that it more accurately captures the validation process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Related to automated testing (unit, UI, integration, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants