Skip to content

Removes unneeded transactions. #2909

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tuupke
Copy link
Contributor

@tuupke tuupke commented Feb 28, 2025

No description provided.

@tuupke tuupke force-pushed the Feature/remove-transactions branch 2 times, most recently from eda63ec to 73b1804 Compare February 28, 2025 16:20
@tuupke tuupke force-pushed the Feature/remove-transactions branch from 73b1804 to f427290 Compare July 4, 2025 15:05
@DOMjudge DOMjudge deleted a comment from github-actions bot Jul 4, 2025
@tuupke tuupke force-pushed the Feature/remove-transactions branch 9 times, most recently from 14f4141 to 9284d1c Compare July 5, 2025 14:31
@tuupke tuupke force-pushed the Feature/remove-transactions branch from 9284d1c to 00f98e2 Compare July 5, 2025 15:01
@tuupke tuupke marked this pull request as ready for review July 5, 2025 15:02
@tuupke
Copy link
Contributor Author

tuupke commented Jul 5, 2025

Just the first three transactions. But can be merged. Probably easiest to check commit-by-commit.

@@ -910,37 +910,22 @@ public function internalErrorAction(Request $request): ?int
*/
protected function giveBackJudging(int $judgingId, ?Judgehost $judgehost): void
{
// Reset the judgings without using Doctrine, it has no support for update queries containing a join.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doctrine 🤯

->getArrayResult();

if ($rows == 0) {
// TODO, handle this case. Nothing was updated.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything to be handled? If there is no outstanding that we would disable here, we don't have an issue, do we?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, I'll remove.

Reading this back, this cannot be correct! I've had to create a manual query for updates with joins. 🤯

Apparently this case is never hit in the testsuite either.

Tried removing this transaction for the removal of all unneccesary
transactions. It turns out that this is actually a valid usecase that
should be kept.
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.

None yet

2 participants