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

Refactor: eligibility forms #1657

Merged
merged 4 commits into from
Aug 15, 2023
Merged

Refactor: eligibility forms #1657

merged 4 commits into from
Aug 15, 2023

Conversation

thekaveman
Copy link
Member

@thekaveman thekaveman commented Aug 15, 2023

Closes #1564

Gets us closer to #1525

What this PR does

  • Removes all the form-specific fields from EligibilityVerifier model: form_blurb, form_sub_label, etc.
  • Adds a single field form_class that holds the name of a Python form class, adds a method to create an instance of this class
  • Creates a form class for MSTCourtesyCard verification, moves all config there (out of migration)

Testing

  • Run bin/init.sh
  • Verify with MST Courtesy Card flow
  • Ensure no functional regressions
  • Ensure no design regressions
  • Ensure no copy regressions

@thekaveman thekaveman requested a review from a team as a code owner August 15, 2023 19:20
@github-actions github-actions bot added i18n Copy: Language files or Django i18n framework tests Related to automated testing (unit, UI, integration, etc.) back-end Django views, sessions, middleware, models, migrations etc. deployment-dev [auto] Changes that will trigger a deploy if merged to dev migrations [auto] Review for potential model changes/needed data migrations updates and removed i18n Copy: Language files or Django i18n framework labels Aug 15, 2023
@thekaveman thekaveman self-assigned this Aug 15, 2023
@thekaveman thekaveman marked this pull request as draft August 15, 2023 19:22
@thekaveman thekaveman force-pushed the refactor/eligibility-forms branch 2 times, most recently from 80723e3 to 3544819 Compare August 15, 2023 21:21
@thekaveman thekaveman marked this pull request as ready for review August 15, 2023 21:23
@machikoyasuda
Copy link
Member

Reviewing now

Comment on lines +67 to +70
name_max_length=None,
sub_input_mode=None,
sub_max_length=None,
sub_pattern=None,
Copy link
Member

@machikoyasuda machikoyasuda Aug 15, 2023

Choose a reason for hiding this comment

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

These deleted comments from the data file could become docstring or comments on this file to describe these:

    # A regular expression used to validate the 'sub' API field before sending to this verifier
    form_sub_pattern = models.TextField(null=True)
    # Input mode can be "numeric", "tel", "search", etc. to override default "text" keyboard on mobile devices
    form_input_mode = models.TextField(null=True)
    # The maximum length accepted for the 'sub' API field before sending to this verifier
    form_max_length = models.PositiveSmallIntegerField(null=True)
    # The maximum length accepted for the 'name' API field before sending to this verifier
    form_name_max_length = models.PositiveSmallIntegerField(null=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.

Thanks for pasting here 🙏 I'll add those.

Copy link
Member Author

@thekaveman thekaveman Aug 15, 2023

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

🎉🎉🎉

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh, typo... one more rebase incoming

machikoyasuda
machikoyasuda previously approved these changes Aug 15, 2023
Copy link
Member

@machikoyasuda machikoyasuda left a comment

Choose a reason for hiding this comment

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

Tested:

  • Desktop design: col-lg-6 is correct ✅
  • English MST CC flow / Good user, bad user - Desktop ✅
  • Spanish MST CC flow / Good user, bad user - Desktop ✅
  • English MST CC flow / Good user, bad user - iPhone simulator - Number keypad appears properly ✅
image

@angela-tran
Copy link
Member

👀

machikoyasuda
machikoyasuda previously approved these changes Aug 15, 2023
@thekaveman
Copy link
Member Author

thekaveman commented Aug 15, 2023

Merging based on @machikoyasuda's prior approval. Thanks both!

@thekaveman thekaveman merged commit 7bfe439 into dev Aug 15, 2023
12 checks passed
@thekaveman thekaveman deleted the refactor/eligibility-forms branch August 15, 2023 23:40
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 migrations [auto] Review for potential model changes/needed data migrations updates tests Related to automated testing (unit, UI, integration, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Eligibility form fields away from model
3 participants