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: use EligibilityVerifierSelectionForm #337

Merged
merged 5 commits into from
Mar 25, 2022
Merged

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented Mar 22, 2022

Closes #322

This is dependent on changes from #335

Changes in this PR:

@angela-tran angela-tran changed the base branch from dev to feat/eligibility-start March 22, 2022 21:47
@github-actions github-actions bot added the deployment-dev [auto] Changes that will trigger a deploy if merged to dev label Mar 22, 2022
@angela-tran angela-tran force-pushed the feat/selection-form branch 3 times, most recently from 7a31ad9 to 06028f3 Compare March 22, 2022 22:18
@thekaveman
Copy link
Member

@angela-tran I know this is still WIP, but I'm wondering if it would be possible to avoid the bulk of the changes that happened in the *.po files here? It's going to make for a more difficult merge of the eventual #338 PR.

Maybe just have this PR focus on the specific adds/changes needed for the selection page, and not all the line-number, reorganization, and timestamp changes?

@angela-tran
Copy link
Member Author

@thekaveman I think that's a really good idea. I will do some rebasing and editing to make as minimal of changes to the *.po files. I think what that'll look like is just the addition of an entry for the submit button (which the Figma mockup has as saying "Continue"/"Continuar").

@angela-tran
Copy link
Member Author

@thekaveman As I've been looking into the remaining work to finish out the selection form, I'm starting to feel like we should split it off into a separate issue. The remaining work is all frontend and might require bigger changes or decisions. Here are the details:

In order to get from the current state in this PR to what's in the Figma mockup, the questions to answer are:
image

  • How should we get that paragraph under the form?
    • Currently, paragraphs from viewmodels.Page is rendered above the form. See page.html
  • RadioSelect in Django 3.2 renders as <li> elements. We probably don't want that? Should we add something to page.html so that we have more granular control over how radio buttons are shown? (Django docs have an example of looping over the widgets.)
  • Rendering the selection_label_description may involve adding some HTML to the display value in our choices tuple. My first step would be to try out the approach in https://stackoverflow.com/questions/2204696/setting-help-text-for-each-choice-in-a-radioselect.

@thekaveman
Copy link
Member

@angela-tran I think you're right, a separate issue to split out the frontend work makes a lot of sense here. I have some ideas so I'll write up a starter issue and you can edit with what I miss.

For this PR, rendering the unstyled radio button list with the intended functionality is good 👍

benefits/locale/en/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
benefits/eligibility/forms.py Outdated Show resolved Hide resolved
benefits/eligibility/views.py Outdated Show resolved Hide resolved
benefits/locale/en/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
benefits/locale/es/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
@angela-tran angela-tran force-pushed the feat/selection-form branch from adcd072 to 0d45e29 Compare March 24, 2022 18:55
@angela-tran angela-tran changed the base branch from feat/eligibility-start to dev March 24, 2022 18:55
@angela-tran
Copy link
Member Author

Rebased my commits onto dev, and changed the target branch to dev

@angela-tran
Copy link
Member Author

The eligibility UI test won't pass until #335 is merged in. (reverse("eligibility:start") throws an exception, so the tests can't find the "Continue" button.)

@thekaveman
Copy link
Member

@angela-tran doh! forgot about that. #335 is done and deployed 👍

@angela-tran
Copy link
Member Author

Closing and re-opening to rerun the checks

@angela-tran angela-tran reopened this Mar 24, 2022
@thekaveman
Copy link
Member

You can also re-run from the failed action page:

image

@thekaveman
Copy link
Member

thekaveman commented Mar 24, 2022

@angela-tran I think we can just skip the failing tests for now - we know we'll have to update them in #339 anyway.

Edit to add: I mean by using .skip() on the ones that fail now with the new pages/flow.

@angela-tran
Copy link
Member Author

Thank you @thekaveman! I added .skip() to those tests.

@angela-tran angela-tran requested a review from thekaveman March 25, 2022 17:56
Make eligibility index handle the EligibilityVerifierSelectionForm, and
remove previous hard-coded logic in core/views.py for setting the
verifier.
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.

I found out what the issue is, and if you rebase on dev again you can make the tests pass:

Since there is only 1 verifier, the eligibility:index view is forwarding directly to the eligibility:start view. This view is using the eligibility.pages.start.button msgid for the button, which still has the old "Ready to continue" value.

@angela-tran angela-tran force-pushed the feat/selection-form branch from d46c3c1 to 2c7137b Compare March 25, 2022 18:36
@thekaveman
Copy link
Member

I'll clean up these msgids a bit more in #344 once this is merged 🙏

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Django form to capture user's EligibilityVerifier selection
2 participants