-
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: store/retrieve enrollment expiry in session #1985
Conversation
these cause a lot of noise in our logs and aren't very useful
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
returned expiry datetime is always an aware datetime instance in UTC
fda313f
to
09cbaed
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.
This looks great! I thought through the implications of having the session calculate the reenrollment date every time (as opposed to having the view set it on the session), and that seems fine to me.
I think it does mean that there's no need for any |
Hmm good point. I think for the first thought, it seems OK for the reenrollment date to be recalculated each time (regardless of where) since it is a fixed offset from the known/stored expiration date. For the second thought (where to do this calculation) -- I'm open. My thinking was it made sense to put it here since the session will be the "source of truth" for the expiration date for the rest of the app, so it could also be the "source of truth" for the reenrollment date. It does sort of abuse the session and put business logic there, rather than just storing data... so I'm open to moving this out. |
Yeah, I think ultimately we need the reenrollment date in the context processor and therefore need it on the session. As for if the the calculation is done directly inside the Maybe in my PR I can move the calculation into |
Hmmm but the problem is we can't/don't want |
Oh I see. I'm leaning now towards just keeping it in |
Closes #1981
New
session
helpers:enrollment_expiry
gets the current user's expiration as a datetime, or Noneenrollment_reenrollment
gets the current user's reenrollment as a datetime, or Noneupdate
acceptsenrollment_expiry
as a datetimeNew
context_processor
:expiration
,reenrollment
, andsupports_expiration
under theenrollment
keyAlso removed most of the noisy logging from
session
.