-
Notifications
You must be signed in to change notification settings - Fork 9
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
Conversation
fb0e08f
to
a2d1dfe
Compare
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
5b1c1ba
to
cd5450d
Compare
There was a problem hiding this 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.
c86fb0b
to
cb11dc2
Compare
There was a problem hiding this 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!):
logger.debug(f"Session contains an {models.EligibilityType.__name__}")
And Line 78
logger.debug("Read tokenized card")
Removed the log statements in |
this is an unlikely but possible scenario
also add view test for this scenario
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.
similar to what was done in 5666973
this removes the need for any helper function
4a11cd4
to
8cb5e33
Compare
Rebased this to resolve merge conflicts 😄 |
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
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 Pythondatetime
module and incorporates its ownUSES_TZ
andTIMEZONE
settings. Because of that, I did not need to usezoneinfo
(orpytz
which was a popular option beforezoneinfo
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
isTrue
,timezone.now()
will return an awaredatetime
with UTC as its timezone.Benefits has
USE_TZ
set toTrue
, 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 thetimezone
that you pass in.If you don't give it a
value
, it will usetimezone.now()
forvalue
.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 calltimezone.get_default_timezone
and give it totimezone.localtime
to create an awaredatetime
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 awaredatetime
. 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.