-
Notifications
You must be signed in to change notification settings - Fork 2
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
Fetch group geolocations from hitobito's existing JSON:API #22
Conversation
Thanks for the request, we are on it. |
Thank you, @carlobeltrame, for this proactive commitment and your very clear communication. I do completely agree with the goal to have as few specialized API(-endpoints) as possible. The challenge with HealthCheck and the original MiData-API is the need of HealthCheck to request a lot of entries and not single ones. The original API is not built for that. Using the original API would lead to more than 100'000 (e.g. for every person) requests every night. To avoid this overhead, the group health endpoints were built. Do you have any ideas how we can overcome this challenge in the future? About the current topic: I propose to set aside this step for the moment, when we can get rid of the whole group_health-endpoints. Regarding the use case of having the data in HealthCheck: |
@taruxtin Thanks for the feedback and statement. I do agree the proposed patch does introduce some complexity, and I am glad we agree on the long-term goals. I do have a proposal for how to replace the There are some adjustments necessary to MiData to make this work, most notably the past roles of all people need to be exposed in the hitobito API to Abteilungsleitungen (as well as the hitobito web application, because otherwise we're simply lying to the users when we say the permission system of health check is the same or more restrictive as hitobito's). I have started working on these adjustments in hitobito/hitobito#1378, hitobito/hitobito_pbs#204, #28 and digio-ch/pbs-healthcheck-web#11. I will close this PR and focus my efforts on working towards this architecture. However, the changes needed will be a little more fundamental than in this PR. |
In #1 (comment) and hitobito/hitobito_pbs#201, it was proposed to add the geolocation meeting points of the Abteilungen to the dedicated health check API. To avoid feature creep in that separately maintained and highly custom API, please consider instead fetching the geolocations from the normal JSON API hitobito offers for third-party applications. The geolocations are already exposed there, since the Pfadi-Finder also needs them. This PR is therefore meant as a replacement for hitobito/hitobito_pbs#201.
I have implemented the geolocation fetching into a JSON file in one commit and the importing into a new Entity in the database in another. The reason being that I don't know what exactly you intend to do with the geolocations, and the latter commit could easily be reverted if you have different plans for the exact table layout etc.
I have tried to follow the existing code style and have run the CS checker as well as the tests and the run-import.sh script, all successfully (apart from a few code style warnings that were already there before my changes). Let me know if you'd like me to write new unit tests for the functionality I added.
For now, I only implemented importing the geolocations specifically, but in the long term, I'd like to help reduce the amount of data that is fetched via the specialized API to a minimum. Given the growing IT tool landscape of PBS and given the IT strategy document of PBS, I think it is only sensible that all tools should rather use a common MiData API instead of building and maintaining a new specialized API for every new tool.
Keep in mind: With this change, the API token that the healthcheck uses needs an additional
groups
permission in addition to thegroup_health
permission, so that we are allowed to fetch the group details. I tested with the token called "Test Safari" on https://pbs.puzzle.ch.CC @simfeld