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.

Sign up for GitHub

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

Jump to bottom

Refactor: eligibility start copy #956

Merged
merged 6 commits into from
Sep 28, 2022
Merged

Refactor: eligibility start copy #956

merged 6 commits into from
Sep 28, 2022

Conversation

angela-tran
Copy link
Member

Closes #942

Verification Flow Before After
Courtesy Cards (not reachable) image image
Login.gov image image

@angela-tran angela-tran added this to the Courtesy Cards milestone Sep 26, 2022
@angela-tran angela-tran self-assigned this Sep 26, 2022
@angela-tran angela-tran requested a review from a team as a code owner September 26, 2022 20:33
@github-actions github-actions bot added back-end Django views, sessions, middleware, models, migrations etc. front-end HTML/CSS/JavaScript and Django templates i18n Copy: Language files or Django i18n framework deployment-dev [auto] Changes that will trigger a deploy if merged to dev labels Sep 26, 2022
@thekaveman thekaveman mentioned this pull request Sep 26, 2022
Merged
thekaveman
thekaveman approved these changes Sep 26, 2022
View reviewed changes
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.

Non-blocking, but it may be a slight confusion to use the title msgid naming here for something other than the the page <title> element, which is how we're mostly using it now right?

thekaveman
thekaveman requested changes Sep 26, 2022
View reviewed changes
Copy link
Member

@thekaveman thekaveman left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, one thing - can you prep a PR for the updates to our data migrations in benefits-secrets

machikoyasuda
machikoyasuda previously approved these changes Sep 26, 2022
View reviewed changes
@angela-tran angela-tran force-pushed the refactor/eligibility-start-copy branch from 24205f2 to c09ade9 Compare September 26, 2022 22:20
machikoyasuda
machikoyasuda requested changes Sep 26, 2022
View reviewed changes
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.

Request to change dmv to mst_login on django.po/migration files

@angela-tran
Copy link
Member Author

Request to change dmv to mst_login on django.po/migration files

I think part of a PR that Kegan's working on will remove dmv. I can certainly rename the oauth msgids to mst_login though... @thekaveman let me know if you see any problems with me doing that in this PR.

@machikoyasuda machikoyasuda self-requested a review September 26, 2022 23:05
machikoyasuda
machikoyasuda previously approved these changes Sep 26, 2022
View reviewed changes
@thekaveman
Copy link
Member

Yeah let's rename the oauth keys to mst_login but leave the DMV ones alone for that other PR.

@angela-tran
Copy link
Member Author

  • Renamed oauth keys to mst_login
  • Prepped data migrations in benefits-secrets

This is ready for re-review

@angela-tran
Copy link
Member Author

Re: cal-itp.benefits Infra check failing: #876 (comment)

machikoyasuda
machikoyasuda reviewed Sep 27, 2022
View reviewed changes
benefits/core/migrations/0002_sample_data.py
start_item_description=_("eligibility.pages.start.mst_login.items[0].text"),
start_blurb=_("eligibility.pages.start.mst_login.p[0]"),
unverified_title=_("eligibility.pages.unverified.mst_login.title"),
unverified_content_title=_("eligibility.pages.unverified.mst_login.content_title"),
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should rename this to mst_login.headline to align with the change on line 167 - and slowly start using headline/sub headline instead of content title.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is a great idea. I'd like to avoid conflicts with #958 so I created separate issue #962 to do this. Does this seem ok to you @machikoyasuda?

machikoyasuda
machikoyasuda requested changes Sep 27, 2022
View reviewed changes
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.

One small naming change, but otherwise all good!

@thekaveman thekaveman mentioned this pull request Sep 27, 2022
Closed
1 task
@angela-tran angela-tran mentioned this pull request Sep 27, 2022
Closed
2 tasks
thekaveman
thekaveman previously approved these changes Sep 27, 2022
View reviewed changes
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 won't block, but I kind of wish mst was mst_cc or similar to align with mst_login

thekaveman
thekaveman previously approved these changes Sep 27, 2022
View reviewed changes
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.

🚀

@thekaveman thekaveman mentioned this pull request Sep 27, 2022
Merged
3 tasks
@angela-tran
Copy link
Member Author

Once @machikoyasuda approves this, is it ok to merge in even though the Azure pipeline is failing?

machikoyasuda
machikoyasuda previously approved these changes Sep 28, 2022
View reviewed changes
@angela-tran angela-tran force-pushed the refactor/eligibility-start-copy branch from e0ffe11 to 9766990 Compare September 28, 2022 00:21
@angela-tran
Copy link
Member Author

Sorry, had to rebase to resolve conflicts with recent merges

@angela-tran angela-tran merged commit 8b9066a into dev Sep 28, 2022
@angela-tran angela-tran deleted the refactor/eligibility-start-copy branch September 28, 2022 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thekaveman thekaveman thekaveman approved these changes

@machikoyasuda machikoyasuda machikoyasuda left review comments

Assignees

@angela-tran angela-tran

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
Milestone
Courtesy Cards
Development

Successfully merging this pull request may close these issues.

Update Eligibility Start page with dynamic verifier copy
3 participants
@angela-tran @thekaveman @machikoyasuda