-
Notifications
You must be signed in to change notification settings - Fork 278
Fix clarifications for unlinked problems #3117
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
Fix clarifications for unlinked problems #3117
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: webapp/src/Controller/Jury/ClarificationController.php
Did you find this useful? React with a 👍 or 👎 |
31420a1 to
193bb34
Compare
I think I would prefer to delete clarifications that are associated with problems when the corresponding problem gets deleted.
I think I would prefer to delete clarifications that are associated with problems when the corresponding problem gets deleted.
I was thinking back about the discussion we had in Baku about teams possibly sharing other info in the clars (which could be exposed by quoting) of a problem and for that reason I think it makes sense to keep them.
You mean write about two different problems or select the incorrect one?
I mostly meant writing about stuff that relates to different / more problems and/or is slightly derailed of the thread about the problem and contains some other info.
I think unlinked data is confusing to teams IMO.
In case a team uses the wrong problem, the jury can change it before deleting the problem.
In case a team replies to a thread of problem A with a question about B, the jury can change it before deleting a problem.
In case a team is asking about multiple problems in one question, and the jury decides to reply meaningfully to both questions, they should reply twice, one for each problem and set the subject accordingly for each reply respectively.
I think I would prefer to delete clarifications that are associated with problems when the corresponding problem gets deleted.
I agree, and couldn't come up with a good reasoning except there are currently no real warnings when you unlink. So pressing it by accident would remove the clarifications. I think the last one who touched that code is @tuupke he might know the reasoning.
I've opened an issue as reminder to discuss this (well... it's more a reminder to myself to implement what we need).
So this can be merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved.
I think after the next release, we should revert the change and replace it with changing the cascade to DELETE here after thinking through all the consequences: https://github.com/DOMjudge/domjudge/blob/main/webapp/src/Entity/Clarification.php#L90
193bb34 to
8f44c3c
Compare
8f44c3c to
b394952
Compare
Closes: #3032
I'm not 100% sure if we even do the right thing. If someone removes a problem from a contest would they expect the clarifications to stay.
This does remove some problems from the API which we keep in the UI, if we want to fix that we would need to remove the
problemkey from the clarification and do this check in code. I think that's worse as it's very hard to forget and that would create referential integrity issues.