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: EligibilityVerifier custom text #331

Merged
merged 8 commits into from
Mar 22, 2022
Merged

Conversation

angela-tran
Copy link
Member

Closes #319

@angela-tran angela-tran added this to the Courtesy Cards milestone Mar 17, 2022
@angela-tran angela-tran self-assigned this Mar 17, 2022
@angela-tran angela-tran marked this pull request as draft March 17, 2022 20:12
@angela-tran angela-tran force-pushed the feat/verifier-custom-text branch from 7781e29 to 450417d Compare March 17, 2022 20:24
these will store the msgid for translation.
these will store the msgid for translation.
these will store the msgid for translation.
This is because the selection description is optional.
@angela-tran angela-tran force-pushed the feat/verifier-custom-text branch from 450417d to c87d7fd Compare March 17, 2022 20:36
@angela-tran angela-tran marked this pull request as ready for review March 17, 2022 20:38
@angela-tran angela-tran requested a review from thekaveman March 17, 2022 20:39
@thekaveman
Copy link
Member

Before we merge this, we have to update the fixture file(s) in dev

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.

Can you please rename the fields I had added previously, to line up with the new ones you're adding (which I like better):

  • form_sub_pattern (instead of regex)
  • form_name_max_length

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.

One more thing too - this is going to run migrations and create the fields, but the fixtures aren't updated to contain the new data. I updated the ticket with a new task for this.

Populate the fixture with existing msgids from our .po files.
@angela-tran angela-tran force-pushed the feat/verifier-custom-text branch from 6ed42f3 to 984bf73 Compare March 18, 2022 17:48
@angela-tran
Copy link
Member Author

Can you please rename the fields I had added previously, to line up with the new ones you're adding (which I like better):

* `form_sub_pattern` (instead of `regex`)

* `form_name_max_length`

I like that better too. Made this change in 7ed9492.

Note that I also reordered the fields in the model definition and fixtures to group them better. I left the order alone in the migration code to keep my changes minimal.

@angela-tran
Copy link
Member Author

angela-tran commented Mar 18, 2022

One more thing too - this is going to run migrations and create the fields, but the fixtures aren't updated to contain the new data. I updated the ticket with a new task for this.

Made this change in 984bf73. Some notes:

@angela-tran
Copy link
Member Author

Some open questions came up when I was looking at the PO entries. We could resolve them either as a part of this PR or as separate tickets/PRs. I noted the questions in the ticket (#319).

@angela-tran angela-tran requested a review from thekaveman March 18, 2022 18:14
Rename 'instructions_title' and 'form_title' fields to be more
consistent with msgid.
@angela-tran
Copy link
Member Author

@thekaveman I updated the description in #319 and made those changes. This is ready for re-review 🔍

@thekaveman
Copy link
Member

@angela-tran Looking at this again 👀

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.

This is great!

I cleaned the compiled language files and rebuilt my local database; no reported errors, the migrations run, fixtures are imported, and language files compiled:

calitp@93063145f6a8  ~/app (feat/verifier-custom-text)
$ rm django.db 

calitp@93063145f6a8  ~/app (feat/verifier-custom-text)
$ rm benefits/locale/**/*.mo

calitp@93063145f6a8  ~/app (feat/verifier-custom-text)
$ bin/init.sh 
[21/Mar/2022 23:38:46] DEBUG benefits.core.urls:41 Register path converter: TransitAgencyPathConverter
[21/Mar/2022 23:38:46] DEBUG benefits.urls:34 Skip url registrations for admin
Operations to perform:
  Apply all migrations: core, sessions
Running migrations:
  Applying core.0001_initial... OK
  Applying sessions.0001_initial... OK
[21/Mar/2022 23:38:47] DEBUG benefits.core.urls:41 Register path converter: TransitAgencyPathConverter
[21/Mar/2022 23:38:47] DEBUG benefits.urls:34 Skip url registrations for admin
Installed 9 object(s) from 5 fixture(s)
superuser: Django not configured for Admin access
processing file django.po in /home/calitp/app/benefits/locale/en/LC_MESSAGES
processing file django.po in /home/calitp/app/benefits/locale/es/LC_MESSAGES

Launching the local app with F5 I can successfully verify a test user:

image

@angela-tran angela-tran merged commit 93ee0ac into dev Mar 22, 2022
@angela-tran angela-tran deleted the feat/verifier-custom-text branch March 22, 2022 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update EligiblityVerifier model to support custom text content
2 participants