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

Add proper state manipulation logging #101

Open
ondryaso opened this issue Jan 15, 2022 · 0 comments
Open

Add proper state manipulation logging #101

ondryaso opened this issue Jan 15, 2022 · 0 comments
Assignees
Labels
backend A backend-related issue comp-states Related to the state management functionality critical A critical issue

Comments

@ondryaso
Copy link
Member

In the original design, state modifications did not change the state's MadeById. However, it is actually quite important that the state carries information about the last person that has changed it – because chillzone take-overs and prolongations are perfomed by modifying the current state. On the other hand, this approach doesn't keep history of people opening chillzones which is something that is higly required for the purpose of analysis and rewarding active SU members.

There are two possible options:

  • either add additional logging facilities over state modifications – add a new 'StateChangeLog' database table that would simply hold information about all state manipulations;
  • or go the legacy (IsKachnaOpen) way, that is, when someone else 'modifies' the current state in the sense that they take it over (possibly changing its end time), don't handle this modification as a modification but instead do it as if they were planning a new state.

I'm really not sure which way is better. Planning new states is a bit less 'natural' – you are not planning a new state, you're just changing the person on the current one. From the user's point of view, it would also bloat the calendar/states list. Also, notification sending would have to be adjusted so that 'modifying' the state this way would not send a notification. This could be partially improved by implementing a form of optional state aggregation on the backend – the frontend could ask for an aggregated version of states list that would group together states of the same type linked by the following state ID attribute. It would have to be optional so that the administrators' state list could still differentiate the states. The thing is, I'm afraid this would bring even more complexity to the already quite hard-to-understand codebase in ClubStateService.

The other option would certainly be much easier to implement. Basically, the serilog logging commands in state management would be replaced/complemented with a simple database insert. Nothing would be changed on the frontend and in the state presentation logic. However, the past states list in the admin section would still provide possibly misleading information about who opened what state – so it should be revamped so that it provides more information about the states – so the states API would have to change anyway (this could be done most non-instrusively by introducing a new endpoint that would return modification history for a single specified state). Also, this would result in certain data redundancy.

I'm probably more leaning towards the second option as it is easier to implement without extensive changes to the states logic. Still, I'll have to think the details out thouroughly.

@ondryaso ondryaso added backend A backend-related issue comp-states Related to the state management functionality critical A critical issue labels Jan 15, 2022
@ondryaso ondryaso self-assigned this Jan 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend A backend-related issue comp-states Related to the state management functionality critical A critical issue
Projects
None yet
Development

No branches or pull requests

1 participant