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(eligibility/form): custom validation message for index and confirm fields #2045

Merged
merged 9 commits into from
Apr 25, 2024

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented Apr 19, 2024

Closes #2012

Reviewing

Eligibility Index form

  1. Don't select any transit benefit
  2. Click "Choose this benefit"
  3. You should get a browser validation message that says "Please choose a transit benefit."

Eligibility Confirm form

Cases to try with sub field (card number field):

  • Enter not enough digits
  • Enter non-numeric characters
  • Leave the field blank

In all those cases, you should get a browser validation message that says "Please enter a X-digit number." where X is the number specific for the transit benefit (5 for MST Courtesy Card, 4 for SBMTD Reduced Fare Mobility ID). See #2012 (comment) for more context.

Cases to try with name field (last name field):

  • Enter more than 255 characters
  • Leave the field blank

In all those cases, you should get a browser validation message that says "Please enter your last name."

Screenshots

Expand

image

image

@angela-tran angela-tran self-assigned this Apr 19, 2024
@github-actions github-actions bot added deployment-dev [auto] Changes that will trigger a deploy if merged to dev back-end Django views, sessions, middleware, models, migrations etc. i18n Copy: Language files or Django i18n framework front-end HTML/CSS/JavaScript and Django templates labels Apr 19, 2024
Copy link

github-actions bot commented Apr 19, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  benefits/eligibility
  forms.py
Project Total  

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

@angela-tran angela-tran force-pushed the feat/custom-validity branch 5 times, most recently from 2058dbd to 00c9e86 Compare April 23, 2024 19:18
@angela-tran
Copy link
Member Author

Side note: our Cypress tests were actually really helpful for this PR. They fail tests that encounter syntax errors, which caught regressions on pages that contain forms but don't need custom validation messages. 🎉

See GitHub Action runs:
https://github.com/cal-itp/benefits/actions/runs/8760017173/job/24044225079
https://github.com/cal-itp/benefits/actions/runs/8790692524/job/24123260719
https://github.com/cal-itp/benefits/actions/runs/8791452896/job/24125714216

@angela-tran angela-tran marked this pull request as ready for review April 23, 2024 22:43
@angela-tran angela-tran requested a review from a team as a code owner April 23, 2024 22:43
@angela-tran angela-tran changed the title Feat(form): set custom validation message for Eligibility Confirm fields Feat(eligibility/form): set custom validation message for index and confirm fields Apr 24, 2024
@angela-tran angela-tran changed the title Feat(eligibility/form): set custom validation message for index and confirm fields Feat(eligibility/form): custom validation message for index and confirm fields Apr 24, 2024
@angela-tran angela-tran requested a review from thekaveman April 25, 2024 16:17
@angela-tran angela-tran force-pushed the feat/custom-validity branch from 59c4808 to e0f296e Compare April 25, 2024 16:28
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.

Yes that looks good, just one more small change.

benefits/eligibility/forms.py Show resolved Hide resolved
@angela-tran
Copy link
Member Author

Need to resolve conflicts

@angela-tran angela-tran force-pushed the feat/custom-validity branch from f6ebcd1 to eb2264a Compare April 25, 2024 17:12
@angela-tran angela-tran requested a review from thekaveman April 25, 2024 17:15
@angela-tran angela-tran marked this pull request as draft April 25, 2024 17:36
event listeners on the inputs and form check validity and set a custom
message if the input is not valid.

the custom message is defined as a data attribute on the input element.
this should've been removed in #1022
the data is such that would pass browser validation
this behavior is not included in product requirements
query for elements with the 'data-custom-validity' attribute instead of
trying to use form fields.

the eligibility index for example only has 1 field with 4 inputs.
@angela-tran angela-tran force-pushed the feat/custom-validity branch from eb2264a to ccff754 Compare April 25, 2024 19:30
@angela-tran
Copy link
Member Author

Rebased on top of #2042 and waiting for it to be merged to minimize merge conflicts

@angela-tran angela-tran marked this pull request as ready for review April 25, 2024 19:52
@angela-tran angela-tran requested a review from thekaveman April 25, 2024 19:52
@angela-tran angela-tran changed the base branch from dev to test April 25, 2024 20:04
@angela-tran angela-tran changed the base branch from test to dev April 25, 2024 20:04
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.

Eligibility index

Confirming custom error messages:

Expand
English (mobile) Spanish (desktop - w/local edits to .po)
image image

Eligibility confirm

Confirming custom error messages:

Expand
Format English (mobile) Spanish (desktop w/local edits to .po)
sub missing MST image image
sub missing SBMTD image image
sub length MST image image
sub length SBMTD image image
sub mismatch MST image image
sub mismatch SBMTD image image
name missing MST image image
name missing SBMTD image image

@angela-tran angela-tran merged commit dd3bd7a into dev Apr 25, 2024
13 checks passed
@angela-tran angela-tran deleted the feat/custom-validity branch April 25, 2024 21:22
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 i18n Copy: Language files or Django i18n framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Form field validation: update copy to be more specific on the problem
2 participants