Skip to content

Commit

Permalink
Fix error handling / messaging on import validation errors.
Browse files Browse the repository at this point in the history 
Fixes #2705.

While we are at it, also improve the usefulness of the error messages.
  • Loading branch information
@meisterT
meisterT committed Sep 29, 2024
1 parent 6d65290 commit 9137d92
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 31 deletions.
68 changes: 46 additions & 22 deletions webapp/src/Service/ImportExportService.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ public function importContestData(mixed $data, ?string &$errorMessage = null, st
$messages = [];
/** @var ConstraintViolationInterface $error */
foreach ($errors as $error) {
$messages[] = sprintf('%s: %s', $error->getPropertyPath(), $error->getMessage());
$messages[] = sprintf(' • `%s`: %s', $error->getPropertyPath(), $error->getMessage());
}

$errorMessage = sprintf("Contest has errors:\n\n%s", implode("\n", $messages));
Expand Down Expand Up @@ -547,7 +547,7 @@ public function getResultsData(
);
continue;
}

if (!$skip && $rank - $skippedTeams <= $contest->getGoldMedals()) {
$awardString = 'Gold Medal';
$lowestMedalPoints = $teamScore->numPoints;
Expand Down Expand Up @@ -791,10 +791,13 @@ protected function importGroupData(
$messages = [];
/** @var ConstraintViolationInterface $error */
foreach ($errors as $error) {
$messages[] = sprintf('%s: %s', $error->getPropertyPath(), $error->getMessage());
$messages[] = sprintf(' • `%s`: %s', $error->getPropertyPath(), $error->getMessage());
}

$message .= sprintf("Group at index %d has errors:\n\n%s\n", $index, implode("\n", $messages));
$message .= sprintf("Group at index %d (%s) has errors:\n%s\n\n",
$index,
json_encode($groupItem),
implode("\n", $messages));
$anyErrors = true;
} else {
$allCategories[] = $teamCategory;
Expand All @@ -810,7 +813,7 @@ protected function importGroupData(
}

if ($anyErrors) {
return 0;
return -1;
}

foreach ($allCategories as $category) {
Expand Down Expand Up @@ -898,10 +901,13 @@ protected function importOrganizationData(
$messages = [];
/** @var ConstraintViolationInterface $error */
foreach ($errors as $error) {
$messages[] = sprintf('%s: %s', $error->getPropertyPath(), $error->getMessage());
$messages[] = sprintf(' • `%s`: %s', $error->getPropertyPath(), $error->getMessage());
}

$message .= sprintf("Organization at index %d has errors:\n\n%s\n", $index, implode("\n", $messages));
$message .= sprintf("Organization at index %d (%s) has errors:\n%s\n\n",
$index,
json_encode($organizationItem),
implode("\n", $messages));
$anyErrors = true;
} else {
$allOrganizations[] = $teamAffiliation;
Expand All @@ -917,7 +923,7 @@ protected function importOrganizationData(
}

if ($anyErrors) {
return 0;
return -1;
}

foreach ($allOrganizations as $organization) {
Expand Down Expand Up @@ -1146,10 +1152,13 @@ protected function importTeamData(array $teamData, ?string &$message, ?array &$s
$messages = [];
/** @var ConstraintViolationInterface $error */
foreach ($errors as $error) {
$messages[] = sprintf('%s: %s', $error->getPropertyPath(), $error->getMessage());
$messages[] = sprintf(' • `%s`: %s', $error->getPropertyPath(), $error->getMessage());
}

$message .= sprintf("Organization for team at index %d has errors:\n\n%s\n", $index, implode("\n", $messages));
$message .= sprintf("Organization for team at index %d (%s) has errors:\n%s\n\n",
$index,
json_encode($teamItem),
implode("\n", $messages));
$anyErrors = true;
} else {
$createdAffiliations[] = $teamAffiliation;
Expand All @@ -1169,10 +1178,13 @@ protected function importTeamData(array $teamData, ?string &$message, ?array &$s
$messages = [];
/** @var ConstraintViolationInterface $error */
foreach ($errors as $error) {
$messages[] = sprintf('%s: %s', $error->getPropertyPath(), $error->getMessage());
$messages[] = sprintf(' • `%s`: %s', $error->getPropertyPath(), $error->getMessage());
}

$message .= sprintf("Organization for team at index %d has errors:\n\n%s\n", $index, implode("\n", $messages));
$message .= sprintf("Organization for team at index %d (%s) has errors:\n%s\n\n",
$index,
json_encode($teamItem),
implode("\n", $messages));
$anyErrors = true;
} else {
$createdAffiliations[] = $teamAffiliation;
Expand All @@ -1195,10 +1207,13 @@ protected function importTeamData(array $teamData, ?string &$message, ?array &$s
$messages = [];
/** @var ConstraintViolationInterface $error */
foreach ($errors as $error) {
$messages[] = sprintf('%s: %s', $error->getPropertyPath(), $error->getMessage());
$messages[] = sprintf(' • `%s`: %s', $error->getPropertyPath(), $error->getMessage());
}

$message .= sprintf("Group for team at index %d has errors:\n\n%s\n", $index, implode("\n", $messages));
$message .= sprintf("Group for team at index %d (%s) has errors:\n%s\n\n",
$index,
json_encode($teamItem),
implode("\n", $messages));
$anyErrors = true;
} else {
$createdCategories[] = $teamCategory;
Expand Down Expand Up @@ -1244,10 +1259,13 @@ protected function importTeamData(array $teamData, ?string &$message, ?array &$s
$messages = [];
/** @var ConstraintViolationInterface $error */
foreach ($errors as $error) {
$messages[] = sprintf('%s: %s', $error->getPropertyPath(), $error->getMessage());
$messages[] = sprintf(' • `%s`: %s', $error->getPropertyPath(), $error->getMessage());
}

$message .= sprintf("Team at index %d has errors:\n\n%s\n", $index, implode("\n", $messages));
$message .= sprintf("Team at index %d (%s) has errors:\n%s\n\n",
$index,
json_encode($teamItem),
implode("\n", $messages));
$anyErrors = true;
} else {
$allTeams[] = $team;
Expand All @@ -1265,7 +1283,7 @@ protected function importTeamData(array $teamData, ?string &$message, ?array &$s
}

if ($anyErrors) {
return 0;
return -1;
}

foreach ($createdAffiliations as $affiliation) {
Expand Down Expand Up @@ -1348,10 +1366,13 @@ protected function importAccountData(
$messages = [];
/** @var ConstraintViolationInterface $error */
foreach ($errors as $error) {
$messages[] = sprintf('%s: %s', $error->getPropertyPath(), $error->getMessage());
$messages[] = sprintf(' • `%s`: %s', $error->getPropertyPath(), $error->getMessage());
}

$message .= sprintf("Team for user at index %d has errors:\n\n%s\n", $index, implode("\n", $messages));
$message .= sprintf("Team for user at index %d (%s) has errors:\n%s\n\n",
$index,
json_encode($accountItem),
implode("\n", $messages));
$anyErrors = true;
} else {
$newTeams[] = [
Expand Down Expand Up @@ -1397,10 +1418,13 @@ protected function importAccountData(
$messages = [];
/** @var ConstraintViolationInterface $error */
foreach ($errors as $error) {
$messages[] = sprintf('%s: %s', $error->getPropertyPath(), $error->getMessage());
$messages[] = sprintf(' • `%s`: %s', $error->getPropertyPath(), $error->getMessage());
}

$message .= sprintf("User at index %d has errors:\n\n%s\n", $index, implode("\n", $messages));
$message .= sprintf("User at index %d (%s) has errors:\n%s\n\n",
$index,
json_encode($accountItem),
implode("\n", $messages));
$anyErrors = true;
} else {
$allUsers[] = $user;
Expand All @@ -1412,7 +1436,7 @@ protected function importAccountData(
}

if ($anyErrors) {
return 0;
return -1;
}

foreach ($allUsers as $user) {
Expand Down
18 changes: 9 additions & 9 deletions webapp/tests/Unit/Service/ImportExportServiceTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public function provideImportContestDataErrors(): Generator
'start-time' => '2020-01-01T12:34:56+02:00',
'scoreboard-freeze-length' => '30:00',
],
"Contest has errors:\n\nname: This value should not be blank.\nshortname: This value should not be blank."
"Contest has errors:\n\n • `name`: This value should not be blank.\n • `shortname`: This value should not be blank."
];
}

Expand Down Expand Up @@ -501,7 +501,7 @@ public function testImportAccountsJsonError(): void
// Remove the file, we don't need it anymore.
unlink($fileName);

self::assertEquals(0, $importCount);
self::assertEquals(-1, $importCount);
self::assertMatchesRegularExpression('/Only alphanumeric characters and _-@. are allowed/', $message);

$postCount = $em->getRepository(User::class)->count([]);
Expand Down Expand Up @@ -870,8 +870,8 @@ public function testImportTeamsJsonError(): void
// Remove the file, we don't need it anymore.
unlink($fileName);

self::assertMatchesRegularExpression('/name: This value should not be blank./', $message);
self::assertEquals(0, $importCount);
self::assertMatchesRegularExpression('/.*`name`: This value should not be blank.*/', $message);
self::assertEquals(-1, $importCount);

$postCount = $em->getRepository(Team::class)->count([]);
self::assertEquals($preCount, $postCount);
Expand Down Expand Up @@ -908,8 +908,8 @@ public function testImportTeamsJsonErrorEmptyString(): void
// Remove the file, we don't need it anymore.
unlink($fileName);

self::assertMatchesRegularExpression('/name: This value should not be blank./', $message);
self::assertEquals(0, $importCount);
self::assertMatchesRegularExpression('/.*`name`: This value should not be blank.*/', $message);
self::assertEquals(-1, $importCount);

$postCount = $em->getRepository(Team::class)->count([]);
self::assertEquals($preCount, $postCount);
Expand Down Expand Up @@ -1050,8 +1050,8 @@ public function testImportGroupsJsonError(): void
// Remove the file, we don't need it anymore.
unlink($fileName);

self::assertMatchesRegularExpression('/name: This value should not be blank/', $message);
self::assertEquals(0, $importCount);
self::assertMatchesRegularExpression('/.*`name`: This value should not be blank.*/', $message);
self::assertEquals(-1, $importCount);

$postCount = $em->getRepository(TeamCategory::class)->count([]);
self::assertEquals($preCount, $postCount);
Expand Down Expand Up @@ -1150,7 +1150,7 @@ public function testImportOrganizationsErrorJson(): void
unlink($fileName);

self::assertMatchesRegularExpression('/ISO3166-1 alpha-3 values are allowed/', $message);
self::assertEquals(0, $importCount);
self::assertEquals(-1, $importCount);

$postCount = $em->getRepository(TeamAffiliation::class)->count([]);
self::assertEquals($preCount, $postCount);
Expand Down

0 comments on commit 9137d92

Please sign in to comment.