Olen
feat: extract payments on club endpoint
This feature is needed for our club. However, this is a new major addition to the module as a whole (new API endpoint club).
Related to #70 as far as I can tell.
Therefore, this is just a draft to see how @Olen and @elliot-100 feel about this new feature and start a conversation around it!
I can add some example script/usage if interested
In principle, I am all for extending the functionalty. I have not tested the "club" functions, but if it is possible to unite the functions from the two endpoints without to many conflicting names I would prefer to keep it as one common class. If not, using something like SpondClub or something is probably a good solution.
Very interesting! I keep meaning to look at Club functions for my own club.
For now - does this imply that other API endpoints (and the rest of the library) still work for you, but the club API endpoint provides additional functions?
I think that is what I was wondering in #70 ...
For now - does this imply that other API endpoints (and the rest of the library) still work for you, but the club API endpoint provides additional functions?
Correct. The current library still works perfectly (i guess same functionality as provided in the front end spond.com/client). This new club endpoint is for the Spond Club functionality
I have not tested the "club" functions, but if it is possible to unite the functions from the two endpoints without to many conflicting names I would prefer to keep it as one common class. If not, using something like SpondClub or something is probably a good solution.
Yeah of course i would prefer that too... but I think since the two front ends are different (spond.com/client vs. club.spond.com) i thought maybe having two classes might be a clearer separation of concern since the functionality covered in the two are also different but of course I'm open to suggestions! I'll continue to have a think on it... The different name is also not a bad idea either if we are ok with two separate classes.
I think since the two front ends are different (spond.com/client vs. club.spond.com) i thought maybe having two classes might be a clearer separation of concern
Agree, I think best to have them separate to start with, especially as it's not necessarily clear how they might functionally overlap or behave similarly-but-different (e.g. differing data structures). Seems safer to develop separately; common elements can always be refactored out to e.g. a base class.
I took a stab... Having both of them in the same class is difficult since we'll need two separate tokens to authenticate! Therefore, I opted for 2 separate classes and refactor out a base class.
manual test script is going through! My downstream scripts seem to also work!
[Edited] Hi @ptmminh - many thanks for this. Could I ask you to run isort . after each commit and refresh with a force-push? This should reduce unnecessary diffs on merge later. It doesn't look like black . is needed. I will look into why isort GitHub action didn't seem to flag anything.
Edit 2: annoyingly GitHub actions don't appear to be completing right now, so I'll come back to that
Thanks for the effort. When the actions work again, I'll try to look at it. The first impression is that it looks good, and I like the way it was implemented. But I don't have any clubs to work with, so I can't verify that it is working...
If anyone can chime in and confirm that it works, and the tests complete successfully, it is fine with me.
Just took a quick look at #114 too, do I also need to update README.md with additional features implemented in this PR (i.e. get_export, get_transactions and change_response)?
Yes, please update the README. Also, since the PR contains quite some refactoring of thr base class, I think we should look into merging a few of the other pending PRs first, so we only need to make any changes required for the refactoring once.
Yes, please update the README.
Sure, done! :heavy_check_mark:
Also, since the PR contains quite some refactoring of thr base class, I think we should look into merging a few of the other pending PRs first, so we only need to make any changes required for the refactoring once.
Ok, feel free to merge other PRs first and then ping me and I'll rebase/refactor and we can merge it then.
Ok, feel free to merge other PRs first and then ping me and I'll rebase/refactor and we can merge it then.
Other PRs merged now @ptmminh . I think I would have a couple of review comments but have run out of time today.
I don't have a Spond Club yet, but manual_test_functions works fine once I updated config.py with a dummy club_id - the attendance xlsx feature looks useful, didn't actually know that existed.
yep, all set, whoever has write access please rebase or merge onto main! :rocket:
Thanks for the review @elliot-100 ! :100: