Skip to content

Conversation

vmcj
Copy link
Member

@vmcj vmcj commented Sep 24, 2025

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 problem key 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.

Copy link

sentry-io bot commented Sep 24, 2025

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: webapp/src/Controller/Jury/ClarificationController.php

Function Unhandled Issue
App\Controller\Jury\ClarificationController::viewAction ErrorException: Warning: Undefined array key "3-general" /src/Controller/Jury/ClarificationController.php in App\Controller\Jury\ClarificationController::vi...
Event Count: 1

Did you find this useful? React with a 👍 or 👎

@vmcj vmcj force-pushed the clarification_unconnected_problem branch from 31420a1 to 193bb34 Compare September 24, 2025 20:26
@meisterT
Copy link
Member

I think I would prefer to delete clarifications that are associated with problems when the corresponding problem gets deleted.

@Kevinjil
Copy link
Contributor

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.

@Kevinjil Kevinjil added this to the 9.0 milestone Sep 25, 2025
@meisterT
Copy link
Member

You mean write about two different problems or select the incorrect one?

@Kevinjil
Copy link
Contributor

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.

@meisterT
Copy link
Member

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.

@vmcj
Copy link
Member Author

vmcj commented Sep 25, 2025

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.

@Kevinjil
Copy link
Contributor

I think unlinked data is confusing to teams IMO.

@meisterT I can live with that reasoning ;). The warning suggested by @vmcj would be nice-to-have in that case.

@vmcj
Copy link
Member Author

vmcj commented Oct 3, 2025

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.

Copy link
Member

@meisterT meisterT left a 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

@meisterT meisterT force-pushed the clarification_unconnected_problem branch from 193bb34 to 8f44c3c Compare October 5, 2025 15:23
@meisterT meisterT force-pushed the clarification_unconnected_problem branch from 8f44c3c to b394952 Compare October 5, 2025 15:28
@meisterT meisterT enabled auto-merge October 5, 2025 15:28
@meisterT meisterT added this pull request to the merge queue Oct 5, 2025
Merged via the queue into DOMjudge:main with commit cd6e892 Oct 5, 2025
36 checks passed
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.

Clarification page breaks for removed problems
3 participants