Skip to content

Commit

Permalink
Improve error messages for invalid submissions (fix code review comme…
Browse files Browse the repository at this point in the history 
…nts)

Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>
  • Loading branch information
@Koc
Koc committed Feb 10, 2025
1 parent 5f006f2 commit 4d26227
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 22 deletions.
11 changes: 6 additions & 5 deletions lib/Controller/ApiController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1246,11 +1246,12 @@ public function newSubmission(int $formId, array $answers, string $shareHash = '
$form = $this->loadFormForSubmission($formId, $shareHash);

$questions = $this->formsService->getQuestions($formId);
// Is the submission valid
$isSubmissionValid = $this->submissionService->validateSubmission($questions, $answers, $form->getOwnerId());
if (is_string($isSubmissionValid)) {
throw new OCSBadRequestException($isSubmissionValid);
}
try {
// Is the submission valid
$this->submissionService->validateSubmission($questions, $answers, $form->getOwnerId());
} catch (\InvalidArgumentException $e) {
throw new OCSBadRequestException($e->getMessage());
}

// Create Submission
$submission = new Submission();
Expand Down
34 changes: 18 additions & 16 deletions lib/Service/SubmissionService.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -331,9 +331,9 @@ private function exportData(array $header, array $data, string $fileFormat, ?Fil
* @param array $questions Array of the questions of the form
* @param array $answers Array of the submitted answers
* @param string $formOwnerId Owner of the form
* @return null|string Error message if validation failed, null otherwise
* @throw \InvalidArgumentException if validation failed
*/
public function validateSubmission(array $questions, array $answers, string $formOwnerId): ?string {
public function validateSubmission(array $questions, array $answers, string $formOwnerId): void {
// Check by questions
foreach ($questions as $question) {
$questionId = $question['id'];
Expand All @@ -352,7 +352,7 @@ public function validateSubmission(array $questions, array $answers, string $for
}) ||
(!empty($question['extraSettings']['allowOtherAnswer']) && !array_filter($answers[$questionId], fn ($value) => $value !== Constants::QUESTION_EXTRASETTINGS_OTHER_PREFIX)))
) {
return sprintf('Question "%s" is required.', $question['text']);
throw new \InvalidArgumentException(sprintf('Question "%s" is required.', $question['text']));
}

// Perform further checks only for answered questions
Expand All @@ -367,12 +367,16 @@ public function validateSubmission(array $questions, array $answers, string $for
$maxOptions = $question['extraSettings']['optionsLimitMax'] ?? -1;
// If number of answers is limited check the limits
if (($minOptions > 0 && $answersCount < $minOptions)
|| ($maxOptions > 0 && $answersCount > $maxOptions)) {
return sprintf('Question "%s" requires between %d and %d answers.', $question['text'], $minOptions, $maxOptions);
}
&& ($maxOptions > 0 && $answersCount > $maxOptions)) {
throw new \InvalidArgumentException(sprintf('Question "%s" requires between %d and %d answers.', $question['text'], $minOptions, $maxOptions));
} elseif ($minOptions > 0 && $answersCount < $minOptions) {
throw new \InvalidArgumentException(sprintf('Question "%s" requires at least %d answers.', $question['text'], $minOptions));
} elseif ($maxOptions > 0 && $answersCount > $maxOptions) {
throw new \InvalidArgumentException(sprintf('Question "%s" requires at most %d answers.', $question['text'], $maxOptions));
}
} elseif ($answersCount > 1 && $question['type'] !== Constants::ANSWER_TYPE_FILE) {
// Check if non-multiple questions have not more than one answer
return sprintf('Question "%s" can only have one answer.', $question['text']);
throw new \InvalidArgumentException(sprintf('Question "%s" can only have one answer.', $question['text']));
}

/*
Expand All @@ -381,39 +385,39 @@ public function validateSubmission(array $questions, array $answers, string $for
*/
if (in_array($question['type'], Constants::ANSWER_TYPES_DATETIME) &&
!$this->validateDateTime($answers[$questionId][0], Constants::ANSWER_PHPDATETIME_FORMAT[$question['type']])) {
return sprintf('Invalid date/time format for question "%s".', $question['text']);
throw new \InvalidArgumentException(sprintf('Invalid date/time format for question "%s".', $question['text']));
}

// Check if all answers are within the possible options
if (in_array($question['type'], Constants::ANSWER_TYPES_PREDEFINED) && empty($question['extraSettings']['allowOtherAnswer'])) {
foreach ($answers[$questionId] as $answer) {
// Search corresponding option, return false if non-existent
if (!in_array($answer, array_column($question['options'], 'id'))) {
return sprintf('Answer "%s" for question "%s" is not a valid option.', $answer, $question['text']);
throw new \InvalidArgumentException(sprintf('Answer "%s" for question "%s" is not a valid option.', $answer, $question['text']));
}
}
}

// Handle custom validation of short answers
if ($question['type'] === Constants::ANSWER_TYPE_SHORT && !$this->validateShortQuestion($question, $answers[$questionId][0])) {
return sprintf('Invalid input for question "%s".', $question['text']);
throw new \InvalidArgumentException(sprintf('Invalid input for question "%s".', $question['text']));
}

if ($question['type'] === Constants::ANSWER_TYPE_FILE) {
$maxAllowedFilesCount = $question['extraSettings']['maxAllowedFilesCount'] ?? 0;
if ($maxAllowedFilesCount > 0 && count($answers[$questionId]) > $maxAllowedFilesCount) {
return sprintf('Too many files uploaded for question "%s". Maximum allowed: %d', $question['text'], $maxAllowedFilesCount);
throw new \InvalidArgumentException(sprintf('Too many files uploaded for question "%s". Maximum allowed: %d', $question['text'], $maxAllowedFilesCount));
}

foreach ($answers[$questionId] as $answer) {
$uploadedFile = $this->uploadedFileMapper->findByUploadedFileId($answer['uploadedFileId']);
if (!$uploadedFile) {
return sprintf('File "%s" for question "%s" not exists anymore. Please delete and re-upload the file.', $answer['fileName'] ?? $answer['uploadedFileId'], $question['text']);
throw new \InvalidArgumentException(sprintf('File "%s" for question "%s" not exists anymore. Please delete and re-upload the file.', $answer['fileName'] ?? $answer['uploadedFileId'], $question['text']));
}

$nodes = $this->rootFolder->getUserFolder($formOwnerId)->getById($uploadedFile->getFileId());
if (empty($nodes)) {
return sprintf('File "%s" for question "%s" not exists anymore. Please delete and re-upload the file.', $answer['fileName'] ?? $answer['uploadedFileId'], $question['text']);
throw new \InvalidArgumentException(sprintf('File "%s" for question "%s" not exists anymore. Please delete and re-upload the file.', $answer['fileName'] ?? $answer['uploadedFileId'], $question['text']));
}
}
}
Expand All @@ -423,11 +427,9 @@ public function validateSubmission(array $questions, array $answers, string $for
foreach ($answers as $id => $answerArray) {
// Search corresponding question, return false if not found
if (!in_array($id, array_column($questions, 'id'))) {
return sprintf('Answer for non-existent question with ID %d.', $id);
throw new \InvalidArgumentException(sprintf('Answer for non-existent question with ID %d.', $id));
}
}

return null;
}

/**
Expand Down
8 changes: 7 additions & 1 deletion tests/Unit/Service/SubmissionServiceTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -867,6 +867,12 @@ public function testValidateSubmission(array $questions, array $answers, ?string
return $mail === 'some.name+context@example.com';
});

$this->assertEquals($expected, $this->submissionService->validateSubmission($questions, $answers, 'admin'));
if ($expected !== null) {
$this->expectException(\InvalidArgumentException::class);
$this->expectExceptionMessage($expected);
}

$this->submissionService->validateSubmission($questions, $answers, 'admin');
$this->assertTrue(true);
}
};

0 comments on commit 4d26227

Please sign in to comment.