Give warning for duplicate results

[sharikak54]

I saw this issue #274 and thought I'd give it a try.

It would be nice if there were a pop-up warning that the results that you entered are an exact match with existing results in the same round. Duplicate results typically happen when the person who entered the results for the wrong person by typoing their ID or selecting the wrong name by accident and overwrites existing results. Duplicate results are often overlooked when the duplicate results overwrite already-double-checked results.

Exceptions would include FMC results and DNF results since ties are common under these circumstances.

Breaking change

In order to check for duplicate results in the current stored batch, I changed the format of the stored batch results in localStorage to include the person attribute. I figured that made more sense than fetching it each time, since theoretically this should affect few people (only data batches which are in-progress before deployment and need to be refreshed after); but maybe this is a rare enough case that it's better not to introduce a breaking change. What do you think?

Removed after discussion, in favor of creating a combinedResults array

[jonatanklosko]

Hey, thanks for looking into this, I dropped some comments :)

[jonatanklosko]

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.

[sharikak54]

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.

[jonatanklosko]

I would compute combinedResults together with the filter here.

Computing them upfront, as in the current version, doesn't have any benefit, because it would be recomputed after every submission anyway.

[jonatanklosko]

Btw. you can also write it like this:

Suggested change

	const combinedResults = [...results];
	batchResults.forEach((batchedResult) => {
	const i = combinedResults.findIndex((res) => res.id === batchedResult.id);
	combinedResults[i] = {
	...combinedResults[i],
	attempts: batchedResult.attempts,
	};
	});
	return combinedResults;
	return results.map((result) => {
	const batchResult = batchResults.find((batchResult) => batchResult.id === result.id);
	if (batchResult) {
	return { ...result, attempts: batchResult.attempts };
	} else {
	return result;
	}
	);

I consider it slightly more direct in terms of readability, though either version is fine :)