-
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: include expiration date during enrollment #1973
Conversation
benefits/enrollment/views.py
Outdated
expiry_date = expiration.calculate_expiry_date(matching_group_funding_source, eligibility.expiration_days) | ||
client.link_concession_group_funding_source( | ||
funding_source_id=funding_source.id, group_id=group_id, expiry_date=expiry_date | ||
) |
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.
@thekaveman I put all this business logic right here in the view function 1) so I could get it written down for starters, and 2) so that we could see what might make sense to extract out as helper functions.
I actually think this looks ok. It's kind of long, but in terms of level of abstraction, it seems appropriate to have the API calls here and delegate out the date calculation to a separate module.
Thoughts? Also are the comments helpful or just noisy?
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.
By "all this business logic" I mean lines 90-120
New version of |
stub out a module for handling the calculations related to expiration / re-enrollment
d4f1998
to
617b050
Compare
@thekaveman and I discussed #1973 (comment) yesterday and came up with the full list of possible combinations to handle. We met with @indexing today to define the behavior for cases that are unlikely but still possible. Closing this PR to start a new one that reflects what we decided. |
If you think it makes sense, I think it would be OK to have the helper functions previously in the |
Closes #1877
This is very much a WIP, but pushing up for visibility / feedback.