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: parameterize messages with agency-specific content #1196

Merged
merged 5 commits into from Jan 12, 2023

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented Jan 11, 2023

Closes #1187

Run locally

Make sure to run with updated data migrations (sample data is updated) and run ./bin/init.sh to recompile the language files.

Before merging

  • Update the data migration for dev in Azure.

@angela-tran angela-tran requested a review from a team as a code owner January 11, 2023 19:15
@github-actions github-actions bot added migrations [auto] Review for potential model changes/needed data migrations updates i18n Copy: Language files or Django i18n framework deployment-dev [auto] Changes that will trigger a deploy if merged to dev back-end Django views, sessions, middleware, models, migrations etc. labels Jan 11, 2023
@angela-tran
Copy link
Member Author

Some notes about the change from 4ae331b to 7df9e31:

At first, in 4ae331b, I simply used agency.short_name for the dynamic content. However, the English copy as written says "an MST bus", and the indefinite article "an" or "a" depends on if the agency short name starts with a vowel sound or not.

The solution I came up with in 7df9e31 was to extract the "an MST" part as a property on TransitAgency.

image

image

@thekaveman
Copy link
Member

My initial take on 7df9e31... I think this is a case where storing the entire first sentence is fine, rather than trying to deal with indefinite articles and placeholders where the overall/outside structure of the sentence has to change. It's OK to duplicate some content to make the code / language files more straightforward.

I'm also not a huge fan of that format syntax needed for html_format, it doesn't support fstrings?

@angela-tran
Copy link
Member Author

My initial take on 7df9e31... I think this is a case where storing the entire first sentence is fine, rather than trying to deal with indefinite articles and placeholders where the overall/outside structure of the sentence has to change. It's OK to duplicate some content to make the code / language files more straightforward.

Yeah, I also felt like the approach in 7df9e31 is a little complicated. So just to be sure, are you saying we'd have "You can tap your debit or credit card when you board an MST " as an entry? And then when SacRT is added, we'd have another one saying "You can tap your debit or credit card when you board a SacRT "?

@angela-tran
Copy link
Member Author

I'm also not a huge fan of that format syntax needed for html_format, it doesn't support fstrings?

Yeah, this is actually not because of format_html but rather because of a limitation with Django internationalization in Python code (because of gettext). From Django docs:

Since string extraction is done by the xgettext command, only syntaxes supported by gettext are supported by Django. In particular, Python f-strings are not yet supported by xgettext, and JavaScript template strings need gettext 0.21+.

@thekaveman
Copy link
Member

thekaveman commented Jan 11, 2023

So just to be sure, are you saying we'd have "You can tap your debit or credit card when you board an MST " as an entry?

I'm suggesting you go even further, with these (eventual) entries - entire sentences:

  • "You can tap your credit or debit card when you board an MST bus, and your discounted fare will automatically apply every time you ride."
  • "You can tap your credit or debit card when you board a SacRT bus, and your discounted fare will automatically apply every time you ride."

I guess this is kind of the opposite of parameterizing the msgstr... having multiple versions and configuring different agencies with different versions.

Seeing parameterization within sentences... it looks a little weird, different from doing this for e.g. links/HTML. E.g. eligibility_index_paragraph_part is kind of strange as a model prop? It seems clearer to represent the entire sentence in the PO file.

What do you think?

@angela-tran
Copy link
Member Author

angela-tran commented Jan 11, 2023

Seeing parameterization within sentences... it looks a little weird, different from doing this for e.g. links/HTML. E.g. eligibility_index_paragraph_part is kind of strange as a model prop? It seems clearer to represent the entire sentence in the PO file.

I see - the idea is to not have all these fragments floating around, even if that means some duplication. So it sounds like what we're leaning towards is, if we can easily reuse the agency's short-name/long-name/slug (like in enrollment.pages.index.mst_login.eligibility_confirmed_item.details%(transit_agency_short_name),

"Your eligibility has been verified by Login.gov. This benefit will give you "
"a half-price fare on public transportation with MST."

we should parameterize that. Otherwise, we should just have the full sentence represented on the model.

My only question then is what to call the property on TransitAgency for the full sentence...

@thekaveman
Copy link
Member

the idea is to not have all these fragments floating around, even if that means some duplication. So it sounds like what we're leaning towards...

Yeah I think this is what I'm trying to get at, and matches what I was thinking in terms of a heuristic.

My only question then is what to call the property on TransitAgency for the full sentence...

Tough question! 😅 Maybe eligibility_index_intro?

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.

Yep this looks good 👍 I went through the PO file and don't see any other strings that should be parameterized or split into multiple versions.

@angela-tran angela-tran merged commit 6197e64 into dev Jan 12, 2023
@angela-tran angela-tran deleted the refactor/parameterize-messages branch January 12, 2023 18:47
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 i18n Copy: Language files or Django i18n framework migrations [auto] Review for potential model changes/needed data migrations updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor msgid, parameterize msgstr with agency-specific content
2 participants