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: in-person eligibility policies #2689

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented Feb 13, 2025

Closes #2632

This PR implements the functionality where, when verifying eligibility in-person, the user must agree they have used the specific eligibility criteria that is shown.

Screen recordings of behavior

(GIFs play only once. You can refresh the page to replay I think.)

User-facing behavior (for transit agency staff user)

Checkbox changes based on selected flow

benefits-in-person-dynamic-checkbox-edited

Checkbox resets if user changes selected flow

benefits-in-person-checkbox-reset-edited

Browser validation messages

benefits-in-person-browser-validation-edited

User-facing behavior (for superusers and Cal-ITP staff users)

  • We currently only have in-person policies for the Older Adult, Medicare Cardholder, and Courtesy Card flows, so any other flows that were marked as supporting In-person enrollment will be automatically updated to have that option unchecked.
  • When attempting to add In-person as a supported enrollment method, that will only work for the three flows mentioned above. For any other flows, an error message will be shown:
Error message when attempting to add `In-person` to a flow without a policy

Screenshot from 2025-02-21 15-12-59

Testing locally

Data migration

  • Start on main branch, load sample fixtures with ./bin/reset_db.sh
    • Launch app, go to /admin, and log in as cst-user. At /in_person/eligibility, you'll see the 5 in-person flows for CST.
    • Log out and log back in as a superuser. Look over your EnrollmentFlows and notice the In-person checkbox is checked.
  • Checkout this branch (git checkout feat/in-person-policies) and run the migration with ./bin/init.sh
    • Launch app, go to /admin, and log in as cst-user. At /in_person/eligibility, you'll see only "Older Adult" and "Medicare Cardholder".
    • Log out and log back in as a superuser. Looking at the EnrollmentFlows, only the "Older Adult" and "Medicare Cardholder" flows have In-person checked.

EnrollmentFlow validation enforces that in-person flows have a policy

  • Try to check In-person for a flow and save it. If it is not one of the flows with an in-person eligibility policy, you will get an error message.

a11y

I tested going through the form with only keyboard interaction, and the tab order / interactions worked as I expected.

I also inspected the accessibility tree and observed that when the checkboxes go from hidden to shown, the accessibility tree makes sense, i.e. it only shows the checkbox that is currently displayed.

@angela-tran angela-tran self-assigned this Feb 13, 2025
@github-actions github-actions bot added deployment-dev [auto] Changes that will trigger a deploy if merged to dev migrations [auto] Review for potential model changes/needed data migrations updates back-end Django views, sessions, middleware, models, migrations etc. front-end HTML/CSS/JavaScript and Django templates and removed migrations [auto] Review for potential model changes/needed data migrations updates labels Feb 13, 2025
Copy link

github-actions bot commented Feb 13, 2025

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  benefits/core/models
  enrollment.py
  benefits/in_person
  forms.py
  benefits/in_person/context
  __init__.py
  eligibility.py
Project Total  

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

the mocked agency used by the test did not have any flows, and the test
was passing as a false positive.

use the `mocked_session_flow` fixture so that the agency has a flow.

this is similar to the fix in 3bf128c.
@angela-tran angela-tran force-pushed the feat/in-person-policies branch 2 times, most recently from 5dccd3e to 10f0355 Compare February 21, 2025 19:21
@angela-tran angela-tran force-pushed the feat/in-person-policies branch 3 times, most recently from c9e8c25 to 785d3b6 Compare February 21, 2025 19:50
change the form to have a checkbox for each flow. when the user selects
the radio button for a flow, the corresponding checkbox is shown, and
the other checkboxes are hidden.

implement a template specifically for this form.
also fix spelling typo on copy for checkboxes.
@angela-tran angela-tran force-pushed the feat/in-person-policies branch from 785d3b6 to c818c77 Compare February 21, 2025 20:13
previously we could rely on Django's default handling of the single
`BooleanField`, but since we have multiple checkboxes now, and we don't
want to require all of them, we need to implement our own logic to
require that the checkbox that matches the selected flow was checked.

also added Javascript to the template to show the checkbox for the case
where the form didn't pass back-end validation and therefore we show
the form again with what the user had selected.
consumer is responsible for handling any error that might occur from
the key not being found.
@angela-tran angela-tran force-pushed the feat/in-person-policies branch from c818c77 to 21e0f69 Compare February 21, 2025 20:17
data migration handles removing "in_person" from flows that do not have
a policy in the `in_person.context.eligibility_index` dict.

`EnrollmentFlow.clean` handles enforcing this rule going forward when
creating or editing flows.
@angela-tran angela-tran force-pushed the feat/in-person-policies branch from 1f8f25e to d27a53e Compare February 21, 2025 21:10
@angela-tran angela-tran marked this pull request as ready for review February 21, 2025 22:07
@angela-tran angela-tran requested a review from a team as a code owner February 21, 2025 22:07
@@ -257,8 +258,12 @@ def enrollment_success_template(self):
else:
return self.enrollment_success_template_override or f"{prefix}--{self.transit_agency.slug}.html"

@property
def in_person_eligibility_context(self):
return in_person_context.eligibility_index[self.system_name].dict()
Copy link
Member Author

@angela-tran angela-tran Feb 21, 2025

Choose a reason for hiding this comment

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

@thekaveman - to follow up on #2698 (comment):

I rewrote this so that it is non-defensive and simply looks up the key. Consumers of this property are responsible for knowing what to do, whether that's to let the exception bubble up or to handle it.

Here's the diff: 21e0f69

That commit shows how InPersonEligibilityForm consumes the property. EnrollmentFlow.clean consumes it in a similar way.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see... yeah I don't think this is the specific use-case I was thinking about, and I didn't understand the difference in what we were talking about. I agree this change isn't as clean.

The use-case I was thinking about (related to #2698 (comment)): Why was it possible to load a fixture with a bad system_name? I expected this to have failed on import: #2698 (comment)

But I think you described why this is: #2698 (comment). And then it was fixed anyway by adding agency_card to the enum...

Sorry, I mixed up a few different things in that comment. But really I was wondering: why should we even be able to import bad data like that? And does that mean it is somehow possible to get a bad value in there from the Admin? I think both of those are probably answered by your explanation.

in_person_eligibility_context = None

if not in_person_eligibility_context:
errors.append(ValidationError(f"In-person eligibility context not found for: {self.system_name}"))
Copy link
Member Author

Choose a reason for hiding this comment

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

Open to suggestions on this error message 😅

I guess it would help if it told the user some sort of action to take, like to contact a dev or to uncheck In-person.

Copy link
Member

@thekaveman thekaveman Feb 21, 2025

Choose a reason for hiding this comment

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

Hmmm... yeah, this is a technically accurate error message! But since we show it to end-users, I think we probably want something more friendly here 😅

I agree with your action-oriented take, maybe something like:

{system_name} not configured for In-person. Please uncheck to continue."

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 front-end HTML/CSS/JavaScript and Django templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admin: In-Person - View policy
2 participants