Skip to content
5 changes: 3 additions & 2 deletions client/src/components/admin/AdminRound/AdminRoundContent.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,11 @@ function AdminRoundContent({ round, competitionId, officialWorldRecords }) {
onError: apolloErrorHandler,
});

function handleResultAttemptsSubmit(attempts) {
function handleResultAttemptsSubmit(attempts, person) {
if (isBatchMode) {
setBatchResults([
...batchResults.filter((result) => result.id !== editedResult.id),
{ id: editedResult.id, attempts, enteredAt: nowISOString() },
{ id: editedResult.id, attempts, person, enteredAt: nowISOString() },

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we should be storing the whole person object in local storage.

What we could do is, right before calling attemptResultsWarning, we can build a copy of results with changes applied. We can map over results and look for a batch result entry with a matching id, if the is one, we take its attempts.

One issue with checking against batch results is that once we show the warning, it may be confusing, because there will be no visible results for the conflicting person (they are in the batch). But I guess it's better than not showing the warning.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! I decided to create a new combinedResults array in the AdminRoundContent component so that it can be recalculated only when it needs to change. Let me know if that looks good to you; I'm not a React expert by any means!

I also realized that this warning would pop up if you pulled up a result and tried to enter it without changing anything. I saw a couple people using this functionality to refresh results data without needing to refresh the entire page. In the interest of not breaking current users' workflows, I opted to filter out the current result from the duplicate check.

]);
setEditedResult(null);
} else {
Expand Down Expand Up @@ -174,6 +174,7 @@ function AdminRoundContent({ round, competitionId, officialWorldRecords }) {
<ResultAttemptsForm
result={editedResult}
results={round.results}
batchResults={batchResults}
onResultChange={setEditedResult}
eventId={round.competitionEvent.event.id}
format={round.format}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
function ResultAttemptsForm({
result,
results,
batchResults,
onResultChange,
eventId,
format,
Expand Down Expand Up @@ -91,7 +92,7 @@ function ResultAttemptsForm({
const attempts = trimTrailingSkipped(attemptResults).map((result) => ({
result,
}));
onSubmit(attempts);
onSubmit(attempts, result.person);
});
}

Expand All @@ -100,6 +101,7 @@ function ResultAttemptsForm({
attemptResults,
eventId,
officialWorldRecords,
[...results, ...batchResults],
);

if (submissionWarning) {
Expand Down
48 changes: 48 additions & 0 deletions client/src/lib/attempt-result.js
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,7 @@ export function attemptResultsWarning(
attemptResults,
eventId,
officialWorldRecords = [],
results = [],
) {
const skippedGapIndex =
trimTrailingSkipped(attemptResults).indexOf(SKIPPED_VALUE);
Expand Down Expand Up @@ -482,6 +483,23 @@ export function attemptResultsWarning(
};
}
}

// Check whether this result is a duplicate of existing results in the same round.
// Excludes FMC and all-DNF results since ties are common.
// TODO: Does NOT check the in-progress batch, if any.
Comment thread
sharikak54 marked this conversation as resolved.
Outdated
if (["333fm"].indexOf(eventId) === -1) {
Comment thread
sharikak54 marked this conversation as resolved.
Outdated
const matches = findAllMatchingResults(attemptResults, results);
if (matches.length > 0) {
const matchesString = matches
.map((match) => `${match.person.name} (${match.person.id})`)
.join(", ");
return {
description: `The result you're trying to submit matches all results for
the following competitor${matches.length > 1 ? "s" : ""}: ${matchesString}.
Please check that the results are accurate.`,
};
}
}
}
return null;
}
Expand Down Expand Up @@ -545,3 +563,33 @@ function checkForDnsFollowedByValidResult(attemptResults) {
index > dnsIndex && attempt !== SKIPPED_VALUE && attempt !== DNS_VALUE,
);
}

/**
* Check whether an attempt matches an existing attempt exactly.
*/
function findAllMatchingResults(attemptResults, results) {
const filteredAttemptResults = trimTrailingSkipped(attemptResults);

const matches = results.filter((result) => {
if (result.attempts.length !== filteredAttemptResults.length) {
return false;
}

let numDnfResults = 0;
for (let i = 0; i < result.attempts.length; i++) {
if (result.attempts[i].result !== filteredAttemptResults[i]) {
return false;
}
if (result.attempts[i].result === DNF_VALUE) {
numDnfResults++;
}
}
// Exclude all-DNF results since ties are common
if (numDnfResults === result.attempts.length) {
return false;
}

return true;
});
return matches;
}
55 changes: 55 additions & 0 deletions client/src/lib/tests/attempt-result.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,61 @@ describe("attemptResultsWarning", () => {
attemptResultsWarning(attemptResults, "333fm", worldRecords),
).toEqual(null);
});

it("complains about exact duplicate results", () => {
const attemptResults = [500, 600, 700, 800, 900];
const existingResults = [
{
attempts: [
{ result: 500 },
{ result: 600 },
{ result: 700 },
{ result: 800 },
{ result: 900 },
],
person: { id: 2, name: "Person 2" },
},
];
expect(
attemptResultsWarning(attemptResults, "333", [], existingResults),
).toMatchObject({
description: `The result you're trying to submit matches all results for
the following competitor: Person 2 (2).
Please check that the results are accurate.`,
});
});

it("does not check for duplicates in FMC", () => {
const attemptResults = [25, 26];
const existingResults = [
{
attempts: [{ result: 25 }, { result: 26 }],
person: { id: 2, name: "Person 2" },
},
];
expect(
attemptResultsWarning(attemptResults, "333fm", [], existingResults),
).toEqual(null);
});

it("does not warn about all-DNF duplicates", () => {
const attemptResults = [-1, -1, -1, -1, -1];
const existingResults = [
{
attempts: [
{ result: -1 },
{ result: -1 },
{ result: -1 },
{ result: -1 },
{ result: -1 },
],
person: { id: 2, name: "Person 2" },
},
];
expect(
attemptResultsWarning(attemptResults, "333fm", [], existingResults),
).toEqual(null);
});
});

describe("applyTimeLimit", () => {
Expand Down