Refactor: simplify Eligibility Start templates #2726
Conversation
github-actions
bot
added
back-end
angela-tran
changed the title
angela-tran
force-pushed
the
refactor/eligibility-start-context
branch
from
656e62f to
380e89a
Compare
angela-tran
changed the title
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
angela-tran
force-pushed
the
refactor/eligibility-start-context
branch
3 times, most recently
from
5411cf0 to
6d60b5c
Compare
extra_classes is helpful when we have additional styling to do for a particular type of EligibilityStart.
angela-tran
force-pushed
the
refactor/eligibility-start-context
branch
from
6d60b5c to
25bc8e4
Compare
|
Preview url: https://benefits-2726--cal-itp-previews.netlify.app |
angela-tran
force-pushed
the
refactor/eligibility-start-context
branch
from
c113f0e to
4fc9821
Compare
EnrollmentFlow model was updated to replace eligibility_start_template_override with eligibility_start_context.
they kept failing with an error saying that `eligibility_start_context` was a MagicMock instead of a dict. when running with a debugger, I could see that `session.flow` returns a MagicMock. maybe that was fine before, but now we need to use the actual test model instance.
angela-tran
force-pushed
the
refactor/eligibility-start-context
branch
from
4fc9821 to
57f2faf
Compare
There was a problem hiding this comment.
This looks good to me! I liked the organization/cleanup in 0b3c428 and 6e067f9
Also tested locally and compared it on the browser with https://dev-benefits.calitp.org/ and the Eligibility Start page looked identical 👍
| class EligibilityStart: | ||
| page_title: str | ||
| headline_text: str | ||
| eligibility_item_template: str |
There was a problem hiding this comment.
Maybe we can take a follow-up item to refactor this field away from using these templates, and to encode the item strings directly in context.
There was a problem hiding this comment.
Yep, great idea! Created #2759 for it
There was a problem hiding this comment.
I put it in the sprint, but feel free to change that if you disagree
| {% include "eligibility/includes/eligibility-item--contactless-card--start.html" %} | ||
| </ul> | ||
| {% endblock inner-content %} | ||
|
|
||
| {% block call-to-action-button %} | ||
| {% translate call_to_action_button.text as button_text %} |
There was a problem hiding this comment.
This is something I was wondering about when working on #2753:
Since we already use the gettext() helper (aliased as _()) setting up the context, to mark the string as translatable -- do we also need to translate the string in the template with this {% translate %} (or {% blocktranslate %} tag?
I think the answer, from #2753 anyway, seems to be: no. You can just render the variable directly in the template and it will be translated as necessary. E.g. this template change: https://github.com/cal-itp/benefits/pull/2753/files#diff-70ed6fd5fa2321e83db0d63df8312f680cd40428de4ada50dab22e93f55db7c7R109
Translation still works
There was a problem hiding this comment.
I agree that we can remove translate from the templates 👍
There was a problem hiding this comment.
I'm thinking to merge this PR as-is, and a follow-up PR can remove the unnecessary translates in the templates
There was a problem hiding this comment.
Created #2760 as that follow-up ticket
Closes #2728
This PR refactors the
eligibility.startview so that it uses a single template. It uses a context dictionary from the flow to provide the template with copy.The
EligibilityStartclass models the pieces of copy that are needed for the page. I noticed enough duplication with the Login.gov-based instances and also with the agency-card instances ofEligibilityStart, so I decided to create subclasses -LoginGovEligibilityStartandAgencyCardEligibilityStart- to consolidate the copy that was being duplicated.Testing locally
./bin/init.sh