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: simplify Eligibility Index templates #2705

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented Feb 27, 2025

This PR refactors the eligibility.index view so that it uses a single template. It uses a context dictionary from the agency to provide the template with copy.

The EligibilityIndex class and eligibility/index.html template support one or more paragraphs.

Testing locally

  • Checkout this branch - git checkout refactor/eligibility-index-context
  • Run migrations - ./bin/init.sh
  • Check the Eligibility Index for each agency slug - cst, mst, nevco, sacrt, sbmtd (see notes below)

CST

If you're using the sample fixtures, you'll have one TransitAgency with slug cst and see this Eligibility Index:

MST

To see MST's Eligibility Index, you need a TransitAgency with slug mst. An easy way to do this is to change the slug of your existing TransitAgency in the admin interface.

image

The Eligibility Index shows MST copy ✅

image

NevCo

We can repeat the same process for the rest of the slugs, but notice that for some slugs, like nevco, an error is raised when we go to the Eligibility Index:

django.template.exceptions.TemplateDoesNotExist: eligibility/includes/selection-label--nevco-agency-card.html

It's trying to load a Nevada County agency card template, but we don't have that template because that agency-flow combination isn't actually supported, so we need to go remove that flow from the agency.

  • Go to page for editing Enrollment flows
    image
  • Click on the "Agency Cardholder" flow
  • Set its Transit agency field to be blank
    image
  • Click "Save"
Now we see Nevada County's Eligibility Index ✅

image

SacRT

SacRT's Eligibility Index has its two paragraphs of copy ✅

SBMTD

@angela-tran angela-tran self-assigned this Feb 27, 2025
@github-actions github-actions bot added i18n Copy: Language files or Django i18n framework back-end Django views, sessions, middleware, models, migrations etc. front-end HTML/CSS/JavaScript and Django templates tests Related to automated testing (unit, UI, integration, etc.) deployment-dev [auto] Changes that will trigger a deploy if merged to dev migrations [auto] Review for potential model changes/needed data migrations updates and removed back-end Django views, sessions, middleware, models, migrations etc. labels Feb 27, 2025
@angela-tran angela-tran force-pushed the refactor/eligibility-index-context branch 2 times, most recently from 87bfb66 to 0aab438 Compare February 27, 2025 03:24
@angela-tran angela-tran added the back-end Django views, sessions, middleware, models, migrations etc. label Feb 27, 2025
@angela-tran angela-tran force-pushed the refactor/eligibility-index-context branch 3 times, most recently from 9d35d85 to 4546b45 Compare February 27, 2025 03:41
Copy link

github-actions bot commented Feb 27, 2025

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  benefits/core/models
  transit.py
  benefits/eligibility
  views.py
  benefits/eligibility/context
  __init__.py
  agency.py
Project Total  

This report was generated by python-coverage-comment-action

@angela-tran
Copy link
Member Author

Annoying issue with % characters in copy

Some reminders:

  • makemessages scans our code and templates for translatable strings and generates PO files
  • compilemessages reads the PO files and compiles them into binary MO files which are used when the app runs

makemessages is called when we run the helper ./bin/makemessages.sh script.
compilemessages is called whenever the ./bin/init.sh script is run, which would be either when you run ./bin/reset_db.sh or when the app container starts.

Background

Before this PR, our copy containing % characters works fine.

In our templates we have the translatable string, e.g.:

# eligibility/index--cst.html
...

{% translate "Cal-ITP doesn’t save any of your information. All CST transit benefits reduce fares by 50% for bus service on fixed routes." %}

...

When the makemessages command generates a PO entry for this string, it marks the entry as #, python-format (because % in Python is the old-style string-formatting operator). It also realizes this % is not actually being used for interpolation, so it escapes the % character, which is done by putting another % in front. This results in the following PO entry (notice the %%):

#, python-format
msgid ""
"Cal-ITP doesn’t save any of your information. All CST transit benefits "
"reduce fares by 50%% for bus service on fixed routes."
msgstr ""

Then, in the Spanish translation, we also use %%.

#, python-format
msgid ""
"Cal-ITP doesn’t save any of your information. All CST transit benefits "
"reduce fares by 50%% for bus service on fixed routes."
msgstr ""
"Cal-ITP no almacena su información. Todos los beneficios de tránsito de CST "
"reducen las tarifas de autobuses en un 50%%."

compilemessages is able to compile the PO files, and the copy is correct when the template is rendered:

image

Moving the translatable string to Python code

With our context approach, we need to move the string from the template into Python code, e.g.:

...

eligibility_index = {
AgencySlug.CST.value: EligibilityIndex(form_text=_("Cal-ITP doesn’t save any of your information. All CST transit benefits reduce fares by 50%% for bus service on fixed routes."))
...
}

...

Notice we use an escaped percent sign (%%).

makemessages sees this and generates the same PO entries as before. compilemessages is able to compile the PO files.

But the copy is not correct when the template is rendered. It shows %%.

image

Why?? If you think about the template's perspective, the difference from before is that the string to be translated now has %% in it. So we need to somehow get it to have just % instead.

@angela-tran
Copy link
Member Author

angela-tran commented Feb 28, 2025

Failed attempts to solve the issue

Use one % in the translatable string (506f241)

We modify the translatable string to only use one % character:

...

eligibility_index = {
AgencySlug.CST.value: EligibilityIndex(form_text=_("Cal-ITP doesn’t save any of your information. All CST transit benefits "
"reduce fares by 50% for bus service on fixed routes."))
...
}

...

makemessages is able to generate a PO entry with just one % character:

#, python-format
msgid ""
"Cal-ITP doesn’t save any of your information. All CST transit benefits "
"reduce fares by 50% for bus service on fixed routes."
msgstr ""

and we update the Spanish entry to also use one % character:

#, python-format
msgid ""
"Cal-ITP doesn’t save any of your information. All CST transit benefits "
"reduce fares by 50% for bus service on fixed routes."
msgstr ""
"Cal-ITP no almacena su información. Todos los beneficios de tránsito de CST "
"reducen las tarifas de autobuses en un 50%."

However, then compilemessages raises an error. Because the entry is marked as #, python-format, compilemessages checks the string and sees that it is not a valid Python string (the % character is not doing interpolation nor is it escaped).

Execution of msgfmt failed: /home/runner/work/benefits/benefits/benefits/locale/es/LC_MESSAGES/django.po:47: 'msgstr' is not a valid Python format string, unlike 'msgid'. Reason: The string ends in the middle of a directive.
/home/runner/work/benefits/benefits/benefits/locale/es/LC_MESSAGES/django.po:55: 'msgstr' is not a valid Python format string, unlike 'msgid'. Reason: The string ends in the middle of a directive.
/home/runner/work/benefits/benefits/benefits/locale/es/LC_MESSAGES/django.po:63: 'msgstr' is not a valid Python format string, unlike 'msgid'. Reason: The string ends in the middle of a directive.
/home/runner/work/benefits/benefits/benefits/locale/es/LC_MESSAGES/django.po:71: 'msgstr' is not a valid Python format string, unlike 'msgid'. Reason: In the directive number 1, the character 'p' is not a valid conversion specifier.
/home/runner/work/benefits/benefits/benefits/locale/es/LC_MESSAGES/django.po:92: number of format specifications in 'msgid' and 'msgstr' does not match
msgfmt: found 5 fatal errors
CommandError: compilemessages generated one or more errors.

(see this failed init in pytest action)

Remove #, python-format from the generated PO files (1a1c26a)

Building off the above failed attempt, we manually remove #, python-format from these Eligibility Index PO entries.

This works:

image

However, when we inevitably run makemessages again, it'll just put back those #, python-format, so this is not a sustainable solution.

(see corresponding failed makemessages check in GitHub Action)

Solution

The Django docs tell us that if your translatable string is coming from a variable or is computed, then makemessages will not see it. It only sees the literal strings that are passed into some gettext function call.

image

We can take advantage of this to solve our problem.

In our Python code, we call .replace("%%", "%") on the literal string:

...

eligibility_index = {
AgencySlug.CST.value: EligibilityIndex(form_text=_("Cal-ITP doesn’t save any of your information. All CST transit benefits "
"reduce fares by 50%% for bus service on fixed routes.".replace("%%", "%")))
...
}

...

makemessages will see the literal string and generate a valid Python string with %%:

#, python-format
msgid ""
"Cal-ITP doesn’t save any of your information. All CST transit benefits "
"reduce fares by 50%% for bus service on fixed routes."
msgstr ""

compilemessages is able to compile the PO files.

The template is given the string that now has just one %, and the copy is correct:

image

@angela-tran
Copy link
Member Author

I'll need to rebase on top of changes from #2714 and add a test to check eligibility_index contains all AgencySlugs.

@angela-tran angela-tran force-pushed the refactor/eligibility-index-context branch from b5ed1ce to 506f241 Compare March 6, 2025 21:02
unfortunately this will just be added back if we run `makemessages`
again. not sure how to solve this problem, will be thinking more on this.
this makes it so the template gets the computed value, and
`makemessages` and `compilemessages` get the literal value.
@angela-tran angela-tran marked this pull request as ready for review March 6, 2025 21:43
@angela-tran angela-tran requested a review from a team as a code owner March 6, 2025 21:43
@angela-tran angela-tran marked this pull request as draft March 6, 2025 22:53
@angela-tran angela-tran force-pushed the refactor/eligibility-index-context branch from a2d4fb3 to e009625 Compare March 6, 2025 23:06
@angela-tran angela-tran marked this pull request as ready for review March 6, 2025 23:07
@thekaveman thekaveman linked an issue Mar 7, 2025 that may be closed by this pull request
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 front-end HTML/CSS/JavaScript and Django templates i18n Copy: Language files or Django i18n framework migrations [auto] Review for potential model changes/needed data migrations updates tests Related to automated testing (unit, UI, integration, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify templates: Eligibility Index
1 participant