Skip to content

Conversation

zapotocnylubos
Copy link
Contributor

The stored ID in the audit log is the problem ID, not a testcase ID

So searching for a testcase by a problem ID results in an incorrect link to a completely different problem.

Storing the problematic audit log happens here

$this->dj->auditlog('testcase', $probId, 'updated',

I also saw a generic audit log mechanism in BaseController

$this->dj->auditlog($auditLogType, $id, $isNewEntity ? 'added' : 'updated');

I am wondering if this is a correct-intended behavior. On one side, I see performance/logical reasons why to store only the problem ID, but keeping consistency in mind, it would also seem logical to store the testcase ID and look up the problem ID as it was originally coded.

Should we change the

$this->dj->auditlog('testcase', $probId

in all 3 locations to pass a testcase ID instead? Or use the change in the link generation I provided in this MR?

@vmcj
Copy link
Member

vmcj commented Sep 23, 2025

I am wondering if this is a correct-intended behavior. On one side, I see performance/logical reasons why to store only the problem ID, but keeping consistency in mind, it would also seem logical to store the testcase ID and look up the problem ID as it was originally coded.

Are you willing to do some testing for this? I think the call shouldn't be that heavy so sounds reasonable to log all relevant information.

@zapotocnylubos
Copy link
Contributor Author

Sure, I can try to do some load testing and measure it.
The main problem is that it's now inconsistent.
The controller stores the problem ID, and the auditlog table uses that problem ID as it is a testcase ID (which it is not).
Only one way should be chosen.

  1. Store the testcase ID and the lookup problem ID when rendering the link
  2. Store problem ID, don't do a lookup (in this MR)

@meisterT
Copy link
Member

Since we link to the problem (or all of its testcases) anyway (and the text mentions the test case ID, so all info is there), I think the PR is the right thing to do.

@vmcj vmcj added this pull request to the merge queue Sep 24, 2025
Merged via the queue into DOMjudge:main with commit c1c71c3 Sep 24, 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.

3 participants