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

Fetch group geolocations from hitobito's existing JSON:API #22

Closed

Conversation

carlobeltrame
Copy link

@carlobeltrame carlobeltrame commented Aug 5, 2021

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 the group_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

@tobijuon
Copy link

Thanks for the request, we are on it.

@taruxtin
Copy link
Collaborator

taruxtin commented Aug 20, 2021

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:
hitobito/hitobito_pbs#201 does add the geolocations to the already existing group_health/groups-Endpoint. While I do support the overall strategy to get rid of the special group_health-endpoints, I think we should not use the original MiData-API to get the geolocations group by group, when we still use the group_health/groups endpoint to get all the other group-Data. Instead of making the case simpler, we would add another layer of complexity by having to access the original API group-by-group.

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:
They are meant to be visualized on a map (see #1 Akzeptanzkriterien 7 und 9). Due to some challenges with the geocoding, we had to postpone this functionality.

@carlobeltrame
Copy link
Author

carlobeltrame commented Aug 23, 2021

@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 must say I do not agree with the short-term strategy of adding even more health-check-related complexity to MiData, and I kindly ask you to devise and prioritize a plan for how to stop feature creep and deprecate the dedicated health check API. In my eyes, hitobito should be a relatively dumb data store, and application-specific logic does not belong there.

I do have a proposal for how to replace the group_health endpoints. Since last January, hitobito supports fetching data from the normal API using an OAuth access token. Instead of checking an opt-in checkbox in MiData and waiting for one day, an Abteilungsleiter could be offered a sync button in the HealthCheck application, which would trigger the synchronization and aggregation of just the relevant data of that single Abteilung. OAuth is already in place in healthcheck, and the workload for this wouldn't be as big as it is when syncing absolutely everything every night.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants