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.

Sign up for GitHub

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

Jump to bottom

Feat: include expiration date during enrollment #1973

Closed
wants to merge 2 commits into from
Closed

Feat: include expiration date during enrollment #1973

wants to merge 2 commits into from

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented Mar 21, 2024 •
edited

Closes #1877

This is very much a WIP, but pushing up for visibility / feedback.

@angela-tran angela-tran self-assigned this Mar 21, 2024
@github-actions github-actions bot added deployment-dev [auto] Changes that will trigger a deploy if merged to dev back-end Django views, sessions, middleware, models, migrations etc. labels Mar 21, 2024
angela-tran
angela-tran commented Mar 21, 2024
View reviewed changes
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
)
Copy link
Member Author

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?

Copy link
Member Author

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

@thekaveman
Copy link
Member

New version of littlepay published to PyPI with the concession expiry endpoints: https://github.com/cal-itp/littlepay/releases/tag/2024.03.2

@angela-tran
Copy link
Member Author

@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.

@angela-tran angela-tran deleted the feat/expiry-date branch March 26, 2024 22:16
@thekaveman
Copy link
Member

@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 expiration module directly in the views, now that we won't need all of them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@angela-tran angela-tran

Labels
back-end Django views, sessions, middleware, models, migrations etc. deployment-dev [auto] Changes that will trigger a deploy if merged to dev
Projects
None yet
Milestone
Low-income rider enrollment pathway
Development

Successfully merging this pull request may close these issues.

Include expiry_date with all CalFresh / Low income enrollments
2 participants
@angela-tran @thekaveman