Skip to content

i1236 point scoring point display fix#1237

Merged
johnbrvc merged 3 commits into
pc2ccs:i1006_point_scoringfrom
kkarakas:i1236-point-scoring-point-display-fix
May 27, 2026
Merged

i1236 point scoring point display fix#1237
johnbrvc merged 3 commits into
pc2ccs:i1006_point_scoringfrom
kkarakas:i1236-point-scoring-point-display-fix

Conversation

@kkarakas
Copy link
Copy Markdown
Collaborator

@kkarakas kkarakas commented Mar 21, 2026

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:
588968267-19fa0fc9-4e98-45e6-97fb-e28de8a2e065

After:
588968670-031cc452-ed89-400b-9f7b-963c2cac7650

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)

  • Start a point scoring contest. You can use NAC2026 challenge discussed below.
  • Login as one of the contestants.
  • Submit a correct answer to a problem as a contestant.
  • Notice that in runs page instead of '--' there is your score attached to it.

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.component was 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.

@johnbrvc johnbrvc changed the base branch from develop to i1006_point_scoring March 27, 2026 01:18
Copy link
Copy Markdown
Collaborator

@johnbrvc johnbrvc left a comment

Choose a reason for hiding this comment

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

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):

Image

and this to show it works after the PR fix:

Image

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() ?? '');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@clevengr
Copy link
Copy Markdown
Contributor

clevengr commented May 25, 2026

I updated the PR description to contain an explanantion about how the issue was found (added and "Additional Information" section).

Copy link
Copy Markdown
Contributor

@clevengr clevengr left a comment

Choose a reason for hiding this comment

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

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.

@johnbrvc johnbrvc merged commit 3aa9694 into pc2ccs:i1006_point_scoring May 27, 2026
2 of 3 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