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

Admin: Eligibility View #2317

Merged
merged 4 commits into from
Sep 10, 2024
Merged

Admin: Eligibility View #2317

merged 4 commits into from
Sep 10, 2024

Conversation

machikoyasuda
Copy link
Member

@machikoyasuda machikoyasuda commented Aug 22, 2024

closes #2269

http://localhost:11369/in_person/eligibility/

Light mode
image

Dark mode
image

How to test

  • Follow Feat: automatically associate user to agency #2325 (comment) to get into the dashboard
  • Click New Enrollment button
  • Test clicking submit without choosing anything
  • Test clicking submit with only checkbox
  • Test clicking submit with only radio button
  • Use enter or spacebar and tab for form completion and submission
  • Use tab to skip to main content correctly
  • Accessibility: Passes WCAG AA (except for 1 known ARIA form issue coming from form.html - which isn't going to be changed in this PR)
  • Check that the Console warning area is empty

What this PR does

Known issues/questions

  • The log out user tools area does not get rendered.

Form

  • Has jQuery library loaded
  • Does not have recaptcha
  • Uses Django base form inputs (RadioSelect and BooleanField) on top of form.html from core
  • Uses HTML5 validation and Django default back-end validation
  • Has some hacky CSS written for styles, instead of adding utility classes through Django or adding logic to the Django template.
  • This PR aims to not make any changes to forms.html and just go with defaults. The required * is black because the forms.html utility classes make it so, and overriding a style that already has an !important style declaration is kinda messy. So we're keeping it black for now.

Copy

  • Is not internationalized.

Tests

  • Write tests for form submission

CSS

  • Adds an Outline Button style, use with "btn btn-lg btn-outline-primary" (On view and hover)
image image

@github-actions github-actions bot added deployment-dev [auto] Changes that will trigger a deploy if merged to dev front-end HTML/CSS/JavaScript and Django templates labels Aug 22, 2024
Copy link

github-actions bot commented Aug 22, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  benefits
  routes.py
  benefits/in_person
  forms.py
  views.py
Project Total  

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

@machikoyasuda machikoyasuda self-assigned this Aug 22, 2024
@machikoyasuda machikoyasuda force-pushed the feat/2269-admin-elig-form branch from 1fb5427 to 05249a2 Compare August 28, 2024 20:22
Comment on lines +77 to +91
.checkbox-parent .form-group:last-of-type .col-12 {
display: flex;
flex-direction: row-reverse;
justify-content: start;
column-gap: 0.5rem;
margin-top: 2rem;
}

.checkbox-parent,
.checkbox-parent .form-group .col-12,
.checkbox-parent .form-group .col-12 #id_flow {
display: flex;
flex-direction: column;
row-gap: 1rem;
}
Copy link
Member Author

@machikoyasuda machikoyasuda Aug 29, 2024

Choose a reason for hiding this comment

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

As utility classes: d-flex flex-row-reverse justify-content-start gap-0 row-gap-1 mt-4

d-flex flex-column gap-0 row-gap-3 - adds equal spacing between the radio buttons and checkboxes.

But just decided to do it like this for now. Named checkbox-parent because the main thing this does is flip the checkbox rendering to show the checkbox input first, then label, which is the opposite of the rest of the pages.


self.classes = "checkbox-parent"
flow_field = self.fields["flow"]
flow_field.choices = [(f.id, f.label) for f in flows]
Copy link
Member Author

Choose a reason for hiding this comment

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

This sets the form field's label as the label, which are currently named like this:

(CST) Senior Discount
CalFresh
(CST) Veteran Discount
(CST) Agency Card Discount

It's slightly different from what we want (Older adult, CalFresh card holder, Courtesy Card holder, etc.) - which are specified in these files for the Benefits app: https://github.com/cal-itp/benefits/blob/1e2ff89cfcdbcddc6895701399aaf1702b3b2fc1/benefits/eligibility/templates/eligibility/includes/selection-label--senior.html

But didn't want to bog down this PR with implementing all of that.

Copy link
Member

Choose a reason for hiding this comment

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

Created separate issue #2336 to cover this

method = "POST"

flow = forms.ChoiceField(label="Choose an eligibility type to qualify this rider.", widget=forms.widgets.RadioSelect)
verified = forms.BooleanField(label="I have verified this person’s eligibility for a transit benefit.", required=True)
Copy link
Member Author

Choose a reason for hiding this comment

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

@machikoyasuda machikoyasuda force-pushed the feat/2269-admin-elig-form branch from 7412a21 to 20fc8fc Compare August 29, 2024 22:28
@machikoyasuda
Copy link
Member Author

Status update: Everything in this PR is finished, commented/documented and tested except I have not yet written specs for the form. The one other "unknown" I haven't yet gotten around to figuring out is why the admin user's name and the sign out link doesn't appear, as noted above.

@angela-tran
Copy link
Member

angela-tran commented Aug 30, 2024

@machikoyasuda You can do something similar to e9614fe to get the usertools to show up.

The Django base template expects to have has_permission in the context to decide if it will show the usertools, and we can see that Django admin's view functions follow this pattern of adding each_context to their own contexts - e.g.index, login.

@angela-tran
Copy link
Member

I think another unknown is why the custom validity messages are not working

@angela-tran
Copy link
Member

I think another unknown is why the custom validity messages are not working

Created separate issue #2337 to cover this

@machikoyasuda machikoyasuda force-pushed the feat/2269-admin-elig-form branch from 15ad6d2 to 428d63d Compare September 10, 2024 03:43
@machikoyasuda machikoyasuda marked this pull request as ready for review September 10, 2024 03:44
@machikoyasuda machikoyasuda requested a review from a team as a code owner September 10, 2024 03:44
@machikoyasuda machikoyasuda force-pushed the feat/2269-admin-elig-form branch from 428d63d to 40048cb Compare September 10, 2024 03:46
Copy link
Member

@angela-tran angela-tran 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 @machikoyasuda! I think we can remove an unused fixture, so after that, this is ready to merge

tests/pytest/in_person/test_views.py Outdated Show resolved Hide resolved
@machikoyasuda machikoyasuda force-pushed the feat/2269-admin-elig-form branch from b6043eb to 19cc65b Compare September 10, 2024 18:13
Copy link
Member

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

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

🎉

@machikoyasuda machikoyasuda merged commit 977dfd4 into main Sep 10, 2024
15 checks passed
@machikoyasuda machikoyasuda deleted the feat/2269-admin-elig-form branch September 10, 2024 18:28
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement eligibility view and template
2 participants