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: add CalFresh option #1958

Merged
merged 7 commits into from
Mar 20, 2024
Merged

Feat: add CalFresh option #1958

merged 7 commits into from
Mar 20, 2024

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented Mar 15, 2024

Closes #1915 and #1918

This PR adds the radio button option for "CalFresh Cardholders" to the eligibility index. The description contains a link that opens a help modal about CalFresh Cardholder eligibility.

Screenshots

To test verifier ordering

Steps for setting up to test

From existing database

./bin/init.sh

will run migrations, including a migration that sets the initial display order for your existing verifiers.

From scratch

./bin/reset_db.sh
./bin/init.sh

will recreate your database, run migrations, and load fixtures which define the values for display_order (since fixtures are loaded after migrations).

Steps for testing

Start your local app, and log into your local admin interface to edit the order of your Eligibility verifiers by drag-and-drop.

The radio buttons on the eligibility index should be in the order you specified.

@angela-tran angela-tran self-assigned this Mar 15, 2024
Copy link

github-actions bot commented Mar 15, 2024

Coverage report

Click to see where and how coverage changed

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

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

@github-actions github-actions bot added migrations [auto] Review for potential model changes/needed data migrations updates deployment-dev [auto] Changes that will trigger a deploy if merged to dev front-end HTML/CSS/JavaScript and Django templates labels Mar 15, 2024
@angela-tran angela-tran force-pushed the feat/add-calfresh-option branch 4 times, most recently from be7b2a4 to 55fd9bb Compare March 15, 2024 18:20
@angela-tran
Copy link
Member Author

angela-tran commented Mar 15, 2024

resolved discussion: Unexpected behavior with the ordering of benefit options

In dc62f35, I added the CalFresh eligibility type and verifier as new fixtures and set their ids by incrementing the greatest id in their respective tables. This mimics how it would work if one were to use the admin interface to add CalFresh to the Benefits app database since the id is an AutoField.

When setting the references on a transit agency, I added the CalFresh eligibility type and verifier to be second in the list assuming / hoping that somehow Django would preserve the declaration order. We want CalFresh to be the second item when the app retrieves the verifiers for the eligibility selection form.

However, launching the app locally, I see that is not the case:
image

The eligibility selection form queries for the options using verifiers = agency.eligibility_verifiers.filter(active=True), and the result seems to be ordered by id. We can use order_by to specify an order, but we are limited to ordering by the model's fields, i.e. the declaration order is no longer available or stored anywhere.

This makes sense because ultimately what gets put in the database are just rows in the junction tables core_transit_agency_eligibility_types and core_transit_agency_eligibility_verifiers that capture the relationship but not any ordering.
image

The admin interface makes it even more obvious that there's no "declaration order"; you can multi-select, but the order is clearly fixed.
image

I propose we add a display_order field to EligibilityVerifier (PositiveSmallIntegerField I guess?) but am also curious to hear any other ideas.

the CalFresh objects were added to our fixtures in a way similar to how
they would be added to the database via the admin interface.

namely, they were added as objects that have an incremented ID in their
respective tables.
use `django-admin-sortable2` to handle updating / editing.
@angela-tran angela-tran force-pushed the feat/add-calfresh-option branch from 55fd9bb to 09be83e Compare March 19, 2024 20:28
@angela-tran
Copy link
Member Author

@thekaveman and I discussed #1958 (comment) and agreed that the solution must include a good user experience for editing the order.

This led to finding jrief/django-admin-sortable2, which allows for maintaining ordering on whatever models you choose and for editing that order using drag-and-drop in the Django admin site.

We are following the approach described by these sections:

Alternatives considered

The approach described by Make stacked and tabular inlines sortable is for a one-to-many relationship and therefore not applicable to our TransitAgency and EligibilityVerifier models.

The approach described by Sortable Many-to-Many Relations with Sortable Inlines is applicable, but we decided against it because it was getting complex: we'd need to use a custom through model to add display_order to the TransitAgency<>EligibilityVerifier association, write a data migration to avoid losing any existing TransitAgency<>EligibilityVerifier associations, and add a way to retrieve display_order from the through model when getting the EligibilityVerifiers since the ordering doesn't propagate all the way through to the original models.

@angela-tran
Copy link
Member Author

angela-tran commented Mar 19, 2024

Re: new ordering functionality, I don't think there are any unit tests to add here since all we're doing is adding a PositiveSmallIntegerField to a model and letting django-admin-sortable2 do the rest of the logic.

@angela-tran angela-tran marked this pull request as ready for review March 19, 2024 21:42
@angela-tran angela-tran requested a review from a team as a code owner March 19, 2024 21:42
@thekaveman thekaveman linked an issue Mar 19, 2024 that may be closed by this pull request
this is to prepare for Spanish translations that are coming later
@angela-tran
Copy link
Member Author

Ready for re-review

@angela-tran angela-tran requested a review from thekaveman March 20, 2024 18:22
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.

Looks great, I love the graphical re-ordering capability within the Admin!

@angela-tran angela-tran merged commit 23d3da9 into dev Mar 20, 2024
11 checks passed
@angela-tran angela-tran deleted the feat/add-calfresh-option branch March 20, 2024 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployment-dev [auto] Changes that will trigger a deploy if merged to dev front-end HTML/CSS/JavaScript and Django templates migrations [auto] Review for potential model changes/needed data migrations updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create new CalFresh Cardholder informational modal Eligibility index: Add CalFresh Cardholder option
2 participants