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

Veterans - Eligibility Verifier + Eligibility Type #1436

Merged
merged 17 commits into from Jun 26, 2023

Conversation

machikoyasuda
Copy link
Member

@machikoyasuda machikoyasuda commented Jun 21, 2023

closes #1427
closes #1426

What this PR does

  • Create EligibilityVerifier for MST Veterans
  • Create EligibilityType for MST Veterans
  • Create Terraform values stubs for: "MST_VETERAN_GROUP_ID", "MST_VETERAN_VERIFIER_NAME"
  • Spec: Update benefit-select.cy.js spec from looking for 2 radio inputs, to 3 inputs. Update Cypress helper method so it correctly chooses Courtesy Card radio button.
  • Copy: Eligibility Index page and add VA.gov url as well (but not the new external link icon design).
  • Copy: Enrollment Success page - Same as Login.gov
  • Copy: Eligibility Start page: 2 paragraphs (except for the required_items bullet list - that's not part of EligibilityVerifier)
  • UI: Eligibility Index page - Make sure US Veteran option is the 2nd choice.
  • Define EligibilityType for veterans #1426

What configuration has been done

  • Update KeyVault development for EligibilityVerifier: Create and set these key/values
  • Update KeyVault test for EligibilityVerifier: Create and set these key/values
  • Create Veteran group in LittlePay
  • Update KeyVault development for EligibilityType
  • Update KeyVault test for EligibilityVerifier

Screenshots

image image

Documentation

EligibilityVerifier model

  • Adds start_item_secondary_details, an optional TextField(), to EligibilityVerifier model

MediaItem viewmodel

  • Adds secondary_details, an optional string, to MediaItem viewmodel class

The secondary_details is this details paragraph that comes after the bullets, on the Eligibility Start page.

image

Notes

On Production, after going through the Login.gov flow and verifying eligibility, the user sees the enrollment screen with this copy, consisting of 2 media items:
image

In the new design, however, there is only 1 media item for all enrollment screens, and the remaining media item for Little Pay is also shortened from 2 paragraphs to 1:
image

@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. and removed deployment-dev [auto] Changes that will trigger a deploy if merged to dev migrations [auto] Review for potential model changes/needed data migrations updates labels Jun 21, 2023
@machikoyasuda
Copy link
Member Author

machikoyasuda commented Jun 21, 2023

@thekaveman @angela-tran I'm blocked here: #1437 Do you see any syntax error or something I'm doing wrong?

Update: Not blocked anymore.

@machikoyasuda machikoyasuda added this to the Veterans milestone Jun 21, 2023
@machikoyasuda machikoyasuda self-assigned this Jun 21, 2023
@machikoyasuda machikoyasuda force-pushed the feat/1427-veterans--eligibility-verifier branch from bd8b260 to 0fd040d Compare June 21, 2023 22:50
@machikoyasuda machikoyasuda marked this pull request as ready for review June 21, 2023 22:50
@machikoyasuda machikoyasuda requested a review from a team as a code owner June 21, 2023 22:50
@machikoyasuda machikoyasuda changed the title WIP: Veterans - Eligibility Verifier Veterans - Eligibility Verifier Jun 21, 2023
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.

These changes look good to me @machikoyasuda! I'm inclined to approve, but would like to get others' feedback about the PO entry naming / consolidation. Maybe at the very least we can remove login_gov from that PO entry msgid and leave the Courtesy Card verifier alone for now.

@thekaveman thekaveman mentioned this pull request Jun 22, 2023
20 tasks
@machikoyasuda
Copy link
Member Author

machikoyasuda commented Jun 22, 2023

@thekaveman @angela-tran Ready for re-review, hopefully final review. I added a few changes since the last review from Angela:

  1. The copy on Eligibility Start was off by one word.
  2. Removed login_gov from msgids for Unverified page, b/c all the Unverified pages have the same copy now.
  3. Added VA.gov link (which required adding format_html() code)

I also added these secrets to KeyVault dev:
image
image stubbed with fake value

thekaveman
thekaveman previously approved these changes Jun 22, 2023
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.

Please update the description to also note that this PR closes #1426

Otherwise this looks good and can be approved once the Test KeyVault secrets are created!

@thekaveman thekaveman dismissed their stale review June 22, 2023 19:58

Accidental approval

@machikoyasuda machikoyasuda changed the title Veterans - Eligibility Verifier Veterans - Eligibility Verifier + Eligibility Type Jun 22, 2023
@angela-tran
Copy link
Member

@machikoyasuda To clarify on this PR also closing #1426, have you created the LP group yet?

@machikoyasuda
Copy link
Member Author

@angela-tran No not yet.

This is my current status:

image

@machikoyasuda
Copy link
Member Author

machikoyasuda commented Jun 22, 2023

@angela-tran @thekaveman

Ready for final...final 😅 ...review, to confirm that this PR can be merged to close 2 issues (#1427 #1426).

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.

Looks good to me @machikoyasuda 👍

@machikoyasuda machikoyasuda merged commit ba67a6c into dev Jun 26, 2023
13 checks passed
@machikoyasuda machikoyasuda deleted the feat/1427-veterans--eligibility-verifier branch June 26, 2023 18:52
@thekaveman
Copy link
Member

Ah @machikoyasuda I was still reviewing this!

@@ -5,8 +5,8 @@
#, fuzzy
msgid ""
msgstr ""
"Report-Msgid-Bugs-To: https://github.com/cal-itp/benefits/issues \n"
"POT-Creation-Date: 2023-01-26 16:08+0000\n"
"Report-Msgid-Bugs-To: \n"
Copy link
Member

Choose a reason for hiding this comment

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

Why did we lose our Issues link here?

Copy link
Member

Choose a reason for hiding this comment

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

It looks like the helper bin/makemessages.sh was not run, it is supposed to put this line back.

@machikoyasuda
Copy link
Member Author

@thekaveman Oops sorry!!! 😓

I just noticed I get this error after running bin/makemessages.sh now:

calitp@cdbe8faf8c05:~/app$ bin/makemessages.sh
processing locale es
processing locale en
sed: couldn't open temporary file benefits/locale/en/LC_MESSAGES/sed3E0pRF: Permission denied

which fails when running sed -i 's/Report-Msgid-Bugs-To\:/Report-Msgid-Bugs-To\: https\:\/\/github.com\/cal-itp\/benefits\/issues/g' benefits/locale/*/*/*.po - so that's why that line failed to get changed.

@machikoyasuda
Copy link
Member Author

Reported here: #1455

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

Successfully merging this pull request may close these issues.

Define EligibilityVerifier for veterans Define EligibilityType for veterans
3 participants