Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix error handling / messaging on import validation errors. #2707

Merged
merged 1 commit into from
Sep 29, 2024
Merged

Fix error handling / messaging on import validation errors. #2707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 46 additions & 22 deletions webapp/src/Service/ImportExportService.php
View file
Open in desktop
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
Open in desktop
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
Loading