i1236 point scoring point display fix#1237
Conversation
johnbrvc
left a comment
There was a problem hiding this comment.
I have tested this fix and it works fine (and, to remind everyone, we did use a variation of this fix at the NAC2026 challenge and it worked).
I am prepared to approve this PR AFTER both the Issue #1236 AND this PR description are fixed. The issue was written on-site during a busy time and is insufficient. We have time now, so I would like to see the issue cleaned up and explained better (recall this had to do with validators that return 'yes' instead of 'accepted', and I don't see that mentioned in the issue). In addition, the template stuff is still in the issue and should be removed. We also need a clear description of what was observed, that is, for a "yes" no score was displayed since the custom validator returned "yes" instead of "accepted".
For the PR, that too has to be completely rewritten to be more descriptive of the fix, how it was tested and the steps to test. You can use the saved profile I have from the NAC2026 challenge in my google profiles folder: profiles/pointScoringContests called NAC2026-Challenge.zip Also, include screenshots such as this to show it didn't work before the PR fix (this was for team3):
and this to show it works after the PR fix:
Complete details on how to test should be included. (load the profile, log in as team3 (for example), observe the score is there for "yes" judgments.) The issue should have the first picture above explaining what was seen and what was expected. Basically, fill out the template properly.
| hasAcceptedJudgement(run: Run): boolean { | ||
| return !!run.judgement && run.judgement.toLowerCase() === 'accepted'; | ||
| } | ||
| return (run.score ?? 0) > 0 || ['accepted', 'yes'].includes(run.judgement?.toLowerCase() ?? ''); |
There was a problem hiding this comment.
I am not a fan of complex expressions like this, especially in an environment where not all developers have equal familiarity with the language (TypeScript in this case). In general I would much rather see code like the following:
const score = run.score ?? 0;
const judgement = run.judgement != null ? run.judgement.toLowerCase() : '';
const isAccepted = judgement === 'accepted' || judgement === 'yes';
return score > 0 || isAccepted;While this may -- or may not -- require an essentially meaningless additional amount of time for the browser to execute, I think it's a LOT easier for humans to understand (and I consider human developer's time to be important).
That said, I agree that the one-line expression is equivalent to the above four lines, so it "works correctly" and I'm not asking for a change. Although I should also point out that there is still nothing in the Issue description, or the PR, that explains that the problem which is being solved is that a validator was returning "Yes" when the TypeScript code was only willing to consider "Accepted" as meaning "is solved". THAT should be explicitly covered somewhere in the explanation of the bug/fix.
Moving on to runtime testing...
|
I updated the PR description to contain an explanantion about how the issue was found (added and "Additional Information" section). |
clevengr
left a comment
There was a problem hiding this comment.
I ran the PR code and verified that it works as intended. I also updated both the PR description and the original Issue by removing template cruft and adding context information.
I approve this PR.
Description of what the PR does
Previously Score would show "--" for correct judgements. With this PR score shows the numeric score number.
Implementation basically checks the run judgement object to show if it should show the points and if it needs to it shows it by reading the correct field.
Previously:

After:

Issue which the PR addresses
fixes_ #1236
Environment in which the PR was developed (OS,IDE, Java version, etc.)
Windows 11, java version "1.8.0_381", Chrome Version 148.0.7778.96 (Official Build) (64-bit)
Precise steps for testing the PR (i.e., how to demonstrate that it works correctly)
Additional Information
This issue was discovered at the NAC2026 contest. It turned out to be because there was a validator which was returning the response string "Yes" for accepted runs, but the TypeScript code in the WTI-UI
runs-page.componentwas only checking for the string "Accepted". An on-the-fly patch to check also for "Yes" was added during the contest, which fixed the problem.