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: CalFresh enrollment success with expiration #1988

Merged
merged 5 commits into from
Apr 29, 2024

Conversation

thekaveman
Copy link
Member

@thekaveman thekaveman commented Mar 26, 2024

Closes #1919

Todo

Questions sent to @srhhnry in Figma

  • Confirm spacing on Desktop: between top heading and image
  • Confirm spacing on Mobile: between image and expiry heading, between expiry heading and first paragraph
  • Rebase on feat/expiry-date to de-dupe the TIME_ZONE setting change

How to test

  1. In the Admin, update a flow that you can pass eligibility for (e.g. MST Courtesy Cards): check supports expiration and add a value for the expiration days, re-enrollment days, and a QA group ID
  2. In the Admin, update the corresponding agency's PaymentProcessor to connect to the QA environment
  3. Pass eligibility
  4. Enroll a test credit card
  5. See the new success page

Screenshot - English

image

Screenshot - Spanish

image

@github-actions github-actions bot added 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 labels Mar 26, 2024
Copy link

github-actions bot commented Mar 26, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  benefits
  settings.py
  benefits/locale/en
  formats.py
  benefits/locale/es
  formats.py
Project Total  

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

@thekaveman thekaveman self-assigned this Mar 26, 2024
@thekaveman thekaveman marked this pull request as ready for review March 28, 2024 22:35
@thekaveman thekaveman requested a review from a team as a code owner March 28, 2024 22:35
@thekaveman thekaveman force-pushed the feat/calfresh-enrollment-success branch from b265454 to 531a793 Compare March 28, 2024 22:39
@thekaveman thekaveman marked this pull request as draft March 28, 2024 23:30
@thekaveman thekaveman force-pushed the feat/calfresh-enrollment-success branch from 531a793 to 4726c9a Compare April 2, 2024 17:22
@thekaveman thekaveman changed the base branch from dev to feat/expiry-date April 2, 2024 17:22
Base automatically changed from feat/expiry-date to dev April 2, 2024 17:30
@thekaveman thekaveman marked this pull request as ready for review April 2, 2024 17:34
@thekaveman thekaveman marked this pull request as draft April 2, 2024 20:11
@thekaveman thekaveman marked this pull request as ready for review April 25, 2024 20:12
@thekaveman
Copy link
Member Author

I think we can actually merge this in, as it won't be activated for flows until we get back the expiration logic for enrollment (after #2052). And it doesn't have any conflicts with those changes, it just changed the enrollment/success.html template.

Copy link
Member

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

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

To test this locally, I:

  • (temporarily) merged dev into my local feat/calfresh-enrollment-success branch so that I have the revert from Revert "Feat: enrollments can expire (#1989)" #2052
  • edited my Courtesy Card eligibility type to support expiration
  • ran through the full Courtesy Card flow

I then see the success screen:
image

The expiration date doesn't show because currently there isn't any code that sets the enrollment_expiry session variable, and that's ok because this screen won't be accessible until after #2052 is reverted.

Hard-coding in an enrollment_expiry to test date formatting

Just to test the date-formatting, I hard-coded a value in for expiry in the session.enrollment_expiry function:

    # expiry = request.session.get(_ENROLLMENT_EXP)
    expiry = datetime(2024, 12, 1).timestamp()

English:

image

Spanish:
image

@thekaveman Do you know how the date formatting gets set? I'm not sure why I'm getting an abbreviated month in English

@angela-tran
Copy link
Member

With a March date, I see the full month name:

image

@thekaveman
Copy link
Member Author

thekaveman commented Apr 26, 2024

@angela-tran:

@thekaveman Do you know how the date formatting gets set? I'm not sure why I'm getting an abbreviated month in English

We're using the date filter, which uses the DATE_FORMAT setting by default, which is:

DATE_FORMAT = "N j, Y"

Broken down as (from the table here):

  • N: Month abbreviation in Associated Press style. Proprietary extension.
  • j: Day of the month without leading zeros.
  • Y: Year, 4 digits with leading zeros.

It looks like for N, some months are fully spelled out and others are abbreviated. I guess we should change the default and instead use one of:

  • E: Month, locale specific alternative representation usually used for long date representation.
  • F: Month, textual, long.

I think each language has its own default format string, so changing this one should only affect English.

with the TIME_ZONE='America/Los_Angeles' setting, Django will display
the expiration date in that time zone

Django automatically formats the date according to the user's locale
(e.g. 'en' or 'es')

turned off djlint around this section because the different <p> tags in
branches of the {% if %} block was throwing errors and causing weird formatting"
align with the 8-point grid and updated comps in Figma
@thekaveman thekaveman force-pushed the feat/calfresh-enrollment-success branch from 4726c9a to cb42683 Compare April 26, 2024 16:52
@thekaveman
Copy link
Member Author

@angela-tran I rebased on dev so this branch now has the revert from #2052.

I introduced custom date formats per locale and now we get the expected full-month formatting for both English and Spanish:

image

image

angela-tran
angela-tran previously approved these changes Apr 26, 2024
@angela-tran
Copy link
Member

Nice!

Copy link
Member

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

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

The coverage report is flagging that the formats.py files aren't tested... should we do something about that?

adding a few init files here to get test discovery to work with the
new locale directories
@thekaveman
Copy link
Member Author

The coverage report is flagging that the formats.py files aren't tested... should we do something about that?

Good call on this! I added tests for each in 9cdacdc

Copy link
Member

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

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

🚀

@angela-tran
Copy link
Member

Just noticed the infra pipeline failure... looking into it

@angela-tran
Copy link
Member

terraform plan failed with Error: Unable to list provider registration status... seems unrelated to this PR's changes

@angela-tran
Copy link
Member

angela-tran commented Apr 29, 2024

Re-ran it and it worked 😂

image

@thekaveman thekaveman merged commit fecc1cc into dev Apr 29, 2024
13 checks passed
@thekaveman thekaveman deleted the feat/calfresh-enrollment-success branch April 29, 2024 20:24
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enrollment success: Add expiration date text
2 participants