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(enrollment): link funding source to group #15

Merged
merged 5 commits into from
Feb 7, 2024

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented Jan 18, 2024

Part of #10

This PR adds a method to Client to link a funding source to a group using their IDs.

@angela-tran angela-tran self-assigned this Jan 18, 2024
Copy link

github-actions bot commented Jan 19, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  littlepay/api
  client.py
  groups.py
Project Total  

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

@thekaveman thekaveman added the api New API feature implementation or refactor label Feb 5, 2024
@angela-tran angela-tran force-pushed the feat/link-funding-source-to-group branch from 53f79ff to 5cc5bac Compare February 6, 2024 16:42
@angela-tran angela-tran force-pushed the feat/link-funding-source-to-group branch from 5cc5bac to c71ccf0 Compare February 6, 2024 16:44
@angela-tran angela-tran marked this pull request as ready for review February 6, 2024 16:48
@angela-tran angela-tran requested a review from a team as a code owner February 6, 2024 16:48
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.

Hmm, this is sort of tricky since the behavior crosses different "domains" (funding sources, groups).

But I think ultimately we should be organizing our mixins to align with the structure of the API and where each endpoint is defined there. So I would recommend defining all of this within the GroupsMixin vs. having FundingSourcesMixin mixin the GroupsMixin.

GroupsMixin can still reference the static FundingSourcesMixin.FUNDING_SOURCES to build the required endpoint.

@angela-tran
Copy link
Member Author

@thekaveman Yeah, it is tricky. The API docs have one section for Funding Sources which is all about various methods for getting the ID for a specific funding source, but then under the Products section, that's where we see the methods related to linking between products, groups, and funding sources.

The endpoint for linking funding sources to groups is very similar to the one for linking products to groups.

Whether we have them in GroupsMixin or in their own mixins, do you think they should follow the same pattern?

https://github.com/cal-itp/littlepay/blob/feat/link-funding-source-to-group/littlepay/api/products.py#L43

https://github.com/cal-itp/littlepay/blob/feat/link-funding-source-to-group/littlepay/api/funding_sources.py#L47

@thekaveman
Copy link
Member

@angela-tran yeah it is unfortunate that the API docs blend Products and Groups, but it is clear that Funding Sources in the API docs are strictly about GETting funding source by various parameters. I can see us implementing some of those other GET calls in the future in the FundingSourcesMixin for e.g. the manual enrollment feature we've discussed.

I would like to organize our mixins by the top-level API endpoint to the extent possible. Since this operation (linking a funding source to a group) falls under the /concession_groups top-level API, and requires a path parameter to the group_id, that's why I suggest it lives under the GroupsMixin. We will also very soon have to expand to include an optional expiry_date parameter (for e.g. CalFresh) which is a property of the funding source being linked in the group, not a property of the funding source itself.

I think a basic copy/paste of what you have here into the the GroupsMixin would be great.

@angela-tran
Copy link
Member Author

@thekaveman Ok, that all sounds fine. Thanks

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.

This is great! Nice work on this full implementation!

@angela-tran angela-tran merged commit eed379f into main Feb 7, 2024
3 checks passed
@angela-tran angela-tran deleted the feat/link-funding-source-to-group branch February 7, 2024 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api New API feature implementation or refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants