Refactor: field defaults, optional fields, templates #2471

thekaveman · 2024-10-22T19:17:30Z

This PR is another step along the way of making it easier and faster to configure new TransitAgency and EnrollmentFlow instances, while also ensuring a valid configuration for instances that are going live on the app.

Changes

`TransitAgency`

Make the slug field a Django SlugField to enforce the slugness; this field is now used to calculate template paths as well
Make all Eligibility API fields, Transit Processor API fields optional
Defaults for info fields e.g. long_name, phone from CST defaults
Calculate sane defaults for templates from the slug following our established pattern, e.g. core/index--{slug}.html and eligibility/index--{slug}.html
Retain ability to override templates if needed
Setting active=True and saving validates some fields (e.g. info fields, templates must exist, all associated flows must be valid)
Removes a field that was moved to EnrollmentFlow and no longer needed on TransitAgency

`EnrollmentFlow`

Make the system_name field a Django SlugField to enforce slugness; this field is now used to calculate template paths as well
Make all template fields optional
Calculate sane defaults for templates from the system_name or slug following our established pattern, e.g. eligibility/start--{system_name}.html and enrollment/success--{slug}.html
Remove the specific name of the agency card from templates, like courtesy-card
Calculate defaults specific to Agency Card flows establishing a new pattern, e.g. eligibility/start--{slug}-agency-card.html
Retain ability to override templates if needed
When the EnrollmentFlow has an active TransitAgency, saving validates that templates exist and that either claims verification or Eligibility API verification is configured correctly.

Reviewing

Check out this branch locally
Delete your database
Run bin/reset_db.sh with default fixtures
Launch the app
Run through various flows, ensure no regressions
# to the Admin
Verify that new TransitAgency instances can be created with only the slug field provided
Verify that saving an active TransitAgency requires certain info fields to have values
Verify that saving an active TransitAgency requires templates to exist, and the defaults are calculated from the slug
Verify that saving an active TransitAgency requires all of its associated EnrollmentFlows to be valid.
Verify that saving an EnrollmentFlow with an active TransitAgency requires a valid claims or Eligibility API verification configuration.
Verify that saving an EnrollmentFlow with an active TransitAgency requires templates to exist, and defaults are calculated from the system_name or TransitAgency.slug.

github-actions · 2024-10-22T19:18:46Z

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
benefits/core
admin.py
context_processors.py
models.py
benefits/enrollment
views.py
Project Total

_{This report was generated by python-coverage-comment-action}

angela-tran

Lots of nice refactors in this PR @thekaveman!

I thought these were really great changes:

removing null=True for fields where it was not truly needed and default empty string worked just as well / is better
making more fields optional (basically allowing for saving an empty, inactive object that can be filled out later)
fixing things like group_id and label not allowing blank

I have two pieces of feedback:

a requested change for the default values for TransitAgency's info fields
a question on validation behavior

Let me know if you want to talk through any of these. Thanks!

benefits/core/models.py

benefits/eligibility/verify.py

benefits/core/models.py

thekaveman · 2024-11-27T02:12:26Z

Rebased with new commit a8c40bb to address @angela-tran's comments: #2471 (comment)

TransitAgency.clean() calls clean() on each of its EnrollmentFlow, catches any ValidationError, and rethrows with a message indicating an invalid flow.

This makes it less likely that an agency could go live with a misconfigured flow.

Thanks for continuing to push on this one Angela, I think this is definitely the better outcome.

Test this scenario

Login to the Admin
Create a TransitAgency with all fields necessary to be active=True, but don't make it active yet. Save.
Create an EnrollmentFlow with only a claims_provider; select the TransitAgency from above. Save.
Go back to the TransitAgency and check to make it active=True. Save.
💥

slug field is a SlugField for validation

- compute template fields by default - allow overriding templates

- compute some template fields by default - allow overriding templates

further simplify template requirements for flows rename agency card templates into standard naming pattern: {prefix}--{agency.slug}--agency-card.html where {prefix} is e.g. eligibility/start or enrollment/success

when active=True, validate that: - there are values for user-facing info fields like names, logos, phone, etc. - transit processor is configured correctly, if any - templates exist

when the flow's agency.active=True, validate that templates exist consolidate other EnrollmentFlow.clean() tests

change default to empty string these are stored as the empty string when blank=True Django recommends against null=True on TextFields: https://docs.djangoproject.com/en/5.1/ref/models/fields/#null

keep track of all field and template errors separately raise a ValidationError from either when present

this field is now captured on EnrollmentFlow with the other Eligibility API fields

TransitAgency cleans all of its associated EnrollmentFlows, to ensure that an agency doesn't go live with partially configured flows since TransitAgency won't have fields from EnrollmentFlow, any ValidationError that bubbles would raise a new error in Django Admin instead, catch and rethrow a ValidationError indicating the problem flow

lalver1 · 2024-12-02T17:12:42Z

I really like these latest changes where TransitAgency.clean() calls clean() on each of its EnrollmentFlow 👍
One thing I'm wondering is, if we should remove and self.transit_agency.active from

benefits/benefits/core/models.py

Line 633 in a8c40bb

if self.transit_agency and self.transit_agency.active:

so that the user can still see the particular validation errors when an EnrollmentFlow is being configured.

Following the testing steps, the validity of an EnrollmentFlow is only checked when setting a TransitAgency to active and attempting to save, and if there is an error, the message is only that the flow is invalid. So then we would go back to the flow, but we'd have no hint of what is invalid on the flow since the TransitAgency is inactive (because we were not allowed to activate it from the previous step) and the field and template validations are not ran because of the condition referenced above.

The only downside I guess of making this change is that if the EnrollmentFlow is associated with a TransitAgency, the user needs to have all the information for an EnrollmentFlow before being able to save it, regardless of the active status of the TransitAgency. But maybe this isn't too bad since if you can choose to not associate a TransitAgency with an EnrollmentFlow, and this would allow you to temporarily save an EnrollmentFlow that is not completely configured yet.

thekaveman · 2024-12-02T17:21:01Z

Ahh @lalver1 that's a really good point!! 😭

Yes, I agree with you -- I'm going to update that commit one more time 🤞

whether or not the agency is marked as Active since TransitAgency.clean() will call .clean() on each of its EnrollmentFlows and raise a generic error, we want to be able to see the specific problem on the EnrollmentFlow if any

thekaveman · 2024-12-02T17:34:31Z

@lalver1 great suggestion! b48b7ac

angela-tran · 2024-12-02T17:56:40Z

Agreed, great suggestion @lalver1! It makes the scenario described in #2471 (comment), where invalid flows are linked to transit agencies, pretty much impossible, which is great!

angela-tran

Nice work on all this @thekaveman!

lalver1

This looks great @thekaveman and @angela-tran!

thekaveman added this to the Admin tool: agency configuration milestone Oct 22, 2024

thekaveman added the deployment-dev [auto] Changes that will trigger a deploy if merged to dev label Oct 22, 2024

thekaveman self-assigned this Oct 22, 2024

thekaveman force-pushed the refactor/field-defaults branch from c14e109 to 8d1cd32 Compare October 29, 2024 15:11

thekaveman marked this pull request as ready for review October 29, 2024 15:11

thekaveman requested a review from a team as a code owner October 29, 2024 15:11

thekaveman marked this pull request as draft October 29, 2024 15:23

thekaveman force-pushed the refactor/field-defaults branch 2 times, most recently from a088943 to 54b2042 Compare October 29, 2024 15:30

thekaveman marked this pull request as ready for review October 29, 2024 15:40

thekaveman marked this pull request as draft October 29, 2024 18:22

thekaveman force-pushed the refactor/field-defaults branch 4 times, most recently from 56165d4 to 6012bff Compare November 6, 2024 00:44

thekaveman force-pushed the refactor/field-defaults branch 2 times, most recently from 321538f to 7759cc7 Compare November 12, 2024 23:16

thekaveman changed the base branch from main to feat/admin-agency-logos November 12, 2024 23:17

lalver1 force-pushed the feat/admin-agency-logos branch 5 times, most recently from 6f4337f to e255665 Compare November 14, 2024 21:52

angela-tran requested changes Nov 26, 2024

View reviewed changes

thekaveman force-pushed the refactor/field-defaults branch from dfe4ab9 to 034bcaf Compare November 26, 2024 22:37

thekaveman requested a review from angela-tran November 26, 2024 22:39

thekaveman force-pushed the refactor/field-defaults branch from 034bcaf to 6f0ed51 Compare November 27, 2024 02:06

thekaveman added 12 commits November 27, 2024 02:12

chore(models): TransitAgency eligibility api fields are optional

f15a2c9

slug field is a SlugField for validation

refactor(models): simplify template fields on TransitAgency

84e96cc

- compute template fields by default - allow overriding templates

refactor(models): simplify template fields on EnrollmentFlow

33e3d56

- compute some template fields by default - allow overriding templates

refactor(models): default agency card templates

5f24c8f

further simplify template requirements for flows rename agency card templates into standard naming pattern: {prefix}--{agency.slug}--agency-card.html where {prefix} is e.g. eligibility/start or enrollment/success

feat(models): validate active TransitAgency

7964d78

when active=True, validate that: - there are values for user-facing info fields like names, logos, phone, etc. - transit processor is configured correctly, if any - templates exist

feat(models): improve EnrollmentFlow validation

19dc656

when the flow's agency.active=True, validate that templates exist consolidate other EnrollmentFlow.clean() tests

chore(models): remove null=True from TextFields

e932376

change default to empty string these are stored as the empty string when blank=True Django recommends against null=True on TextFields: https://docs.djangoproject.com/en/5.1/ref/models/fields/#null

refactor(models): separate clean() errors types

01a82cb

keep track of all field and template errors separately raise a ValidationError from either when present

feat(models): clean() validates claims vs. eligibility API flows

2ec3839

chore(models): reorder EnrollmentFlow fields for Admin

0a6f9fb

chore(models): remove dupe field on TransitAgency

75454ba

this field is now captured on EnrollmentFlow with the other Eligibility API fields

thekaveman force-pushed the refactor/field-defaults branch from 6f0ed51 to a8c40bb Compare November 27, 2024 02:12

fix(models): validate EnrollmentFlows with assigned agency

b48b7ac

whether or not the agency is marked as Active since TransitAgency.clean() will call .clean() on each of its EnrollmentFlows and raise a generic error, we want to be able to see the specific problem on the EnrollmentFlow if any

angela-tran approved these changes Dec 2, 2024

View reviewed changes

lalver1 approved these changes Dec 2, 2024

View reviewed changes

thekaveman merged commit 8299355 into main Dec 2, 2024
14 checks passed

thekaveman deleted the refactor/field-defaults branch December 2, 2024 18:02

thekaveman mentioned this pull request Dec 3, 2024

Admin: Cal-ITP Benefits staff user can add new models #2201

Open

7 tasks

angela-tran mentioned this pull request Dec 4, 2024

Chore: add back values for CST local fixture #2563

Merged

thekaveman mentioned this pull request Jan 9, 2025

Create new group in QA environment for MST Courtesy Cards #2574

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: field defaults, optional fields, templates #2471

Refactor: field defaults, optional fields, templates #2471

thekaveman commented Oct 22, 2024 •

edited

Loading

github-actions bot commented Oct 22, 2024 •

edited

Loading

angela-tran left a comment

thekaveman commented Nov 27, 2024 •

edited

Loading

lalver1 commented Dec 2, 2024

thekaveman commented Dec 2, 2024

thekaveman commented Dec 2, 2024

angela-tran commented Dec 2, 2024

angela-tran left a comment

lalver1 left a comment

Refactor: field defaults, optional fields, templates #2471

Refactor: field defaults, optional fields, templates #2471

Conversation

thekaveman commented Oct 22, 2024 • edited Loading

Changes

TransitAgency

EnrollmentFlow

Reviewing

github-actions bot commented Oct 22, 2024 • edited Loading

Coverage report

angela-tran left a comment

Choose a reason for hiding this comment

thekaveman commented Nov 27, 2024 • edited Loading

Test this scenario

lalver1 commented Dec 2, 2024

thekaveman commented Dec 2, 2024

thekaveman commented Dec 2, 2024

angela-tran commented Dec 2, 2024

angela-tran left a comment

Choose a reason for hiding this comment

lalver1 left a comment

Choose a reason for hiding this comment

thekaveman commented Oct 22, 2024 •

edited

Loading

`TransitAgency`

`EnrollmentFlow`

github-actions bot commented Oct 22, 2024 •

edited

Loading

thekaveman commented Nov 27, 2024 •

edited

Loading