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: enrollments can expire #1989

Merged
merged 18 commits into from
Apr 2, 2024
Merged

Feat: enrollments can expire #1989

merged 18 commits into from
Apr 2, 2024

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented Mar 27, 2024

Closes #1877

This PR refactors the enrollment phase so that we no longer catch a 409 error to detect existing enrollments. Instead we see if the enrollment already exists for the group. This also allows us to inspect the expiration date of any existing enrollment.

So this PR also introduces support for enrollments that expire. See the table below for expected behavior.

Combinations of states and their expected outcomes

copied directly from our dev meeting notes

Expand to view table
Eligibility state Enrollment state Expiration state Expected enrollment outcome Expected end page for user
Supports expiration Not enrolled No expiration User can enroll, expiration date set Enrollment success
Supports expiration Not enrolled Future expiration This state should not be possible N/A
Supports expiration Not enrolled Re-enrollment window expiration This state should not be possible N/A
Supports expiration Not enrolled Past expiration This state should not be possible N/A
Supports expiration Enrolled No expiration Update expiration of existing enrollment Enrollment success
Supports expiration Enrolled Future expiration Re-enrollment error Enrollment error with expiration
Supports expiration Enrolled Re-enrollment window expiration Update expiration of existing enrollment Enrollment success
Supports expiration Enrolled Past expiration Update expiration of existing enrollment Enrollment success
Does not support expiration Not enrolled No expiration User can enroll, no expiration date set Enrollment success
Does not support expiration Not enrolled Future expiration This state should not be possible N/A
Does not support expiration Not enrolled Re-enrollment window expiration This state should not be possible N/A
Does not support expiration Not enrolled Past expiration This state should not be possible N/A
Does not support expiration Enrolled No expiration No action Enrollment success
Does not support expiration Enrolled Future expiration Remove expiration date Enrollment success
Does not support expiration Enrolled Re-enrollment window expiration Remove expiration date Enrollment success
Does not support expiration Enrolled Past expiration Remove expiration date Enrollment success

Some functions to be familiar with

In Python 3.9+, the standard library includes zoneinfo for working with time zones.

Django has a django.utils.timezone module that exposes convenience methods for working with the built-in Python datetime module and incorporates its own USES_TZ and TIMEZONE settings. Because of that, I did not need to use zoneinfo (or pytz which was a popular option before zoneinfo was a thing).

I thought it might be helpful to write out brief summaries of how the functions below are useful in this PR.

Used in application code

django.utils.timezone.now()

This function creates a standard Python datetime.datetime object representing the current point in time.

More importantly, if USE_TZ is True, timezone.now() will return an aware datetime with UTC as its timezone.

Benefits has USE_TZ set to True, so we can rely on this as an easy way to make an aware "now" datetime , for example in _is_expired and _is_within_reenrollment_date.

django.utils.timezone.localtime(value=None, timezone=None)

This function lets you give it an aware datetime and converts it to the timezone that you pass in.

If you don't give it a value, it will use timezone.now() for value.

This was useful for calculating the expiration date since we want to make sure the expiration time is midnight Pacific Time.

django.utils.timezone.get_default_timezone()

This function returns what you set the TIME_ZONE setting to, which Django refers to as the "default time zone".

In this PR, we set Benefits's default timezone to America/Los_Angeles. This means we can call timezone.get_default_timezone and give it to timezone.localtime to create an aware datetime that is in Pacific Time.

Aside: "current time zone" is initially the same as the default time zone. If you call timezone.activate with some other time zone, that's when they'd be different. The way I think about this is that it's for cases like, for example, if we gave the user a drop-down of timezones and set their selection as the "current time zone" for their requests to the server. With that understanding, we can see that we don't want to use "current time zone" to calculate the expiration date.

Used only in test code

django.utils.timezone.make_aware(value, timezone=None)

This function lets you give it a naive datetime and a timezone to make an aware datetime. This is useful when you need a specific datetime, which I needed for testing. If there is a more succinct way to do this, I'm open to it.

@angela-tran angela-tran self-assigned this Mar 27, 2024
@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 tests Related to automated testing (unit, UI, integration, etc.) labels Mar 27, 2024
Copy link

github-actions bot commented Mar 27, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  benefits
  settings.py
  benefits/enrollment
  views.py
Project Total  

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

@angela-tran angela-tran force-pushed the feat/expiry-date branch 10 times, most recently from 5b1c1ba to cd5450d Compare March 28, 2024 20:28
@angela-tran angela-tran marked this pull request as ready for review March 28, 2024 21:54
@angela-tran angela-tran requested a review from a team as a code owner March 28, 2024 21:54
benefits/settings.py Show resolved Hide resolved
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.

Good progress on this!

I have a few suggestions for cleaning up the code flow, and noticed a few bugs in the enrollment logic.

tests/pytest/enrollment/test_views.py Outdated Show resolved Hide resolved
benefits/enrollment/views.py Outdated Show resolved Hide resolved
benefits/enrollment/views.py Outdated Show resolved Hide resolved
benefits/enrollment/views.py Outdated Show resolved Hide resolved
benefits/enrollment/views.py Outdated Show resolved Hide resolved
@angela-tran angela-tran marked this pull request as draft March 29, 2024 17:54
@angela-tran angela-tran force-pushed the feat/expiry-date branch 3 times, most recently from c86fb0b to cb11dc2 Compare March 29, 2024 19:25
@angela-tran angela-tran marked this pull request as ready for review March 29, 2024 19:42
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.

Just a minor change but this is looking great.

I can't add a line comment, but I think we can drop a few log statements that aren't very useful (trying to reduce noise and also reduce code in this file!):

Line 76

logger.debug(f"Session contains an {models.EligibilityType.__name__}")

And Line 78

logger.debug("Read tokenized card")

benefits/enrollment/views.py Outdated Show resolved Hide resolved
@angela-tran
Copy link
Member Author

I can't add a line comment, but I think we can drop a few log statements that aren't very useful (trying to reduce noise and also reduce code in this file!):

Removed the log statements in e65d076

angela-tran and others added 18 commits April 1, 2024 22:59
this is instead of catching the 409 response code
see https://docs.djangoproject.com/en/5.0/ref/settings/#std-setting-TIME_ZONE

> Note that this isn’t necessarily the time zone of the server.
> When USE_TZ is True, this is the default time zone that Django will use to display datetimes in templates
> and to interpret datetimes entered in forms.
use `django.utils.timezone` for access to the TIME_ZONE setting.

the business logic is to set the expiration date as midnight in Pacific
Time of the (N+1)th day, where N is the value of `expiration_days`.
we want to remove the expiration date for the scenario of expiration is
not supported and yet there is an expiration date.

this functionality has not been implemented in cal-itp/littlepay, nor
have we confirmed that it is possible
leave view function and template for #1921 to implement
we know that we should be using the value from session.enrollment_expiry
use the specific condition that is leading to the branch of code, which
namely is if the concession expiry is None or not.
this removes the need for any helper function
@angela-tran
Copy link
Member Author

Rebased this to resolve merge conflicts 😄

@angela-tran angela-tran merged commit 479b249 into dev Apr 2, 2024
13 checks passed
@angela-tran angela-tran deleted the feat/expiry-date branch April 2, 2024 17:30
angela-tran added a commit that referenced this pull request Apr 24, 2024
This reverts commit 479b249, reversing
changes made to 96ea690.
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 tests Related to automated testing (unit, UI, integration, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include expiry_date with all CalFresh / Low income enrollments
2 participants