From 32c5a098895c43c621b6c0c589b950f7a9d651ad Mon Sep 17 00:00:00 2001 From: Kostiantyn Miakshyn Date: Wed, 29 Jan 2025 15:49:07 +0100 Subject: [PATCH 1/2] Improve error messages for invalid submissions Signed-off-by: Kostiantyn Miakshyn --- lib/Controller/ApiController.php | 3 - lib/Service/SubmissionService.php | 27 ++++----- tests/Unit/Controller/ApiControllerTest.php | 6 +- tests/Unit/Service/SubmissionServiceTest.php | 62 ++++++++++---------- 4 files changed, 47 insertions(+), 51 deletions(-) diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index d19afd7da..3a59f1596 100644 --- a/lib/Controller/ApiController.php +++ b/lib/Controller/ApiController.php @@ -1255,9 +1255,6 @@ public function newSubmission(int $formId, array $answers, string $shareHash = ' if (is_string($isSubmissionValid)) { throw new OCSBadRequestException($isSubmissionValid); } - if ($isSubmissionValid === false) { - throw new OCSBadRequestException('At least one submitted answer is not valid'); - } // Create Submission $submission = new Submission(); diff --git a/lib/Service/SubmissionService.php b/lib/Service/SubmissionService.php index ce6b8218a..05ebc993f 100644 --- a/lib/Service/SubmissionService.php +++ b/lib/Service/SubmissionService.php @@ -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 boolean|string True for valid submission, false or error message for invalid + * @return null|string Error message if validation failed, null otherwise */ - public function validateSubmission(array $questions, array $answers, string $formOwnerId): bool|string { + public function validateSubmission(array $questions, array $answers, string $formOwnerId): ?string { // Check by questions foreach ($questions as $question) { $questionId = $question['id']; @@ -342,7 +342,7 @@ public function validateSubmission(array $questions, array $answers, string $for // Check if all required questions have an answer if ($question['isRequired'] && (!$questionAnswered || - !array_filter($answers[$questionId], function (string|array $value): bool { + !array_filter($answers[$questionId], static function (string|array $value): bool { // file type if (is_array($value)) { return !empty($value['uploadedFileId']); @@ -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 false; + return sprintf('Question "%s" is required.', $question['text']); } // Perform further checks only for answered questions @@ -368,11 +368,11 @@ public function validateSubmission(array $questions, array $answers, string $for // If number of answers is limited check the limits if (($minOptions > 0 && $answersCount < $minOptions) || ($maxOptions > 0 && $answersCount > $maxOptions)) { - return false; + return sprintf('Question "%s" requires between %d and %d answers.', $question['text'], $minOptions, $maxOptions); } } elseif ($answersCount > 1 && $question['type'] !== Constants::ANSWER_TYPE_FILE) { // Check if non-multiple questions have not more than one answer - return false; + return sprintf('Question "%s" can only have one answer.', $question['text']); } /* @@ -381,22 +381,22 @@ 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 false; + return 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 (array_search($answer, array_column($question['options'], 'id')) === false) { - return false; + if (!in_array($answer, array_column($question['options'], 'id'))) { + return 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 false; + return sprintf('Invalid input for question "%s".', $question['text']); } if ($question['type'] === Constants::ANSWER_TYPE_FILE) { @@ -422,13 +422,12 @@ public function validateSubmission(array $questions, array $answers, string $for // Check for excess answers foreach ($answers as $id => $answerArray) { // Search corresponding question, return false if not found - $questionIndex = array_search($id, array_column($questions, 'id')); - if ($questionIndex === false) { - return false; + if (!in_array($id, array_column($questions, 'id'))) { + return sprintf('Answer for non-existent question with ID %d.', $id); } } - return true; + return null; } /** diff --git a/tests/Unit/Controller/ApiControllerTest.php b/tests/Unit/Controller/ApiControllerTest.php index 20bf8e2f4..7cf623b25 100644 --- a/tests/Unit/Controller/ApiControllerTest.php +++ b/tests/Unit/Controller/ApiControllerTest.php @@ -692,7 +692,7 @@ public function testNewSubmission_answers() { $this->submissionService ->method('validateSubmission') - ->willReturn(true); + ->willReturn(null); $this->submissionMapper->expects($this->once()) ->method('insert') @@ -797,7 +797,7 @@ public function testNewSubmission_forbiddenException($hasUserAccess, $hasFormExp $this->submissionService ->method('validateSubmission') - ->willReturn(true); + ->willReturn(null); $this->formAccess($hasUserAccess, $hasFormExpired, $canSubmit); @@ -825,7 +825,7 @@ public function testNewSubmission_validateSubmission() { $this->submissionService ->method('validateSubmission') - ->willReturn(false); + ->willReturn('error message'); $this->expectException(OCSBadRequestException::class); diff --git a/tests/Unit/Service/SubmissionServiceTest.php b/tests/Unit/Service/SubmissionServiceTest.php index 0878c990d..4b1690d88 100644 --- a/tests/Unit/Service/SubmissionServiceTest.php +++ b/tests/Unit/Service/SubmissionServiceTest.php @@ -614,29 +614,29 @@ public function dataValidateSubmission() { 'required-not-answered' => [ // Questions [ - ['id' => 1, 'type' => 'short', 'isRequired' => true] + ['id' => 1, 'type' => 'short', 'text' => 'q1', 'isRequired' => true] ], // Answers [], // Expected Result - false + 'Question "q1" is required.', ], 'required-not-answered-string' => [ // Questions [ - ['id' => 1, 'type' => 'short', 'isRequired' => true] + ['id' => 1, 'type' => 'short', 'text' => 'q1', 'isRequired' => true] ], // Answers [ '1' => [''] ], // Expected Result - false + 'Question "q1" is required.', ], 'required-empty-other-answer' => [ // Questions [ - ['id' => 1, 'type' => 'multiple_unique', 'isRequired' => true, 'extraSettings' => ['allowOtherAnswer' => true], 'options' => [ + ['id' => 1, 'type' => 'multiple_unique', 'text' => 'q1', 'isRequired' => true, 'extraSettings' => ['allowOtherAnswer' => true], 'options' => [ ['id' => 3] ]] ], @@ -645,12 +645,12 @@ public function dataValidateSubmission() { '1' => [Constants::QUESTION_EXTRASETTINGS_OTHER_PREFIX] ], // Expected Result - false + 'Question "q1" is required.', ], 'more-than-allowed' => [ // Questions [ - ['id' => 1, 'type' => 'multiple_unique', 'isRequired' => false, 'options' => [ + ['id' => 1, 'type' => 'multiple_unique', 'text' => 'q1', 'isRequired' => false, 'options' => [ ['id' => 3], ['id' => 5] ]] @@ -660,12 +660,12 @@ public function dataValidateSubmission() { '1' => [3,5] ], // Expected Result - false + 'Question "q1" can only have one answer.' ], 'option-not-known' => [ // Questions [ - ['id' => 1, 'type' => 'multiple', 'isRequired' => false, 'options' => [ + ['id' => 1, 'type' => 'multiple', 'text' => 'q1', 'isRequired' => false, 'options' => [ ['id' => 3], ['id' => 5] ]], @@ -675,12 +675,12 @@ public function dataValidateSubmission() { '1' => [3,10] ], // Expected Result - false + 'Answer "10" for question "q1" is not a valid option.', ], 'other-answer-not-allowed' => [ // Questions [ - ['id' => 1, 'type' => 'multiple', 'isRequired' => false, 'options' => [ + ['id' => 1, 'type' => 'multiple', 'text' => 'q1', 'isRequired' => false, 'options' => [ ['id' => 3], ['id' => 5] ]], @@ -690,24 +690,24 @@ public function dataValidateSubmission() { '1' => [3, Constants::QUESTION_EXTRASETTINGS_OTHER_PREFIX . 'other answer'] ], // Expected Result - false + 'Answer "system-other-answer:other answer" for question "q1" is not a valid option.', ], 'question-not-known' => [ // Questions [ - ['id' => 1, 'type' => 'short', 'isRequired' => false] + ['id' => 1, 'type' => 'short', 'text' => 'q1', 'isRequired' => false] ], // Answers [ '2' => ['answer'] ], // Expected Result - false + 'Answer for non-existent question with ID 2.', ], 'invalid-multiple-too-many-answers' => [ // Questions [ - ['id' => 1, 'type' => 'multiple', 'isRequired' => false, 'extraSettings' => ['optionsLimitMax' => 2], 'options' => [ + ['id' => 1, 'type' => 'multiple', 'text' => 'q1', 'isRequired' => false, 'extraSettings' => ['optionsLimitMax' => 2], 'options' => [ ['id' => 3], ['id' => 5], ['id' => 7], @@ -719,12 +719,12 @@ public function dataValidateSubmission() { '1' => [3, 5, 7] ], // Expected Result - false + 'Question "q1" requires between -1 and 2 answers.', ], 'invalid-multiple-too-few-answers' => [ // Questions [ - ['id' => 1, 'type' => 'multiple', 'isRequired' => false, 'extraSettings' => ['optionsLimitMin' => 2], 'options' => [ + ['id' => 1, 'type' => 'multiple', 'text' => 'q1', 'isRequired' => false, 'extraSettings' => ['optionsLimitMin' => 2], 'options' => [ ['id' => 3], ['id' => 5], ['id' => 7], @@ -736,12 +736,12 @@ public function dataValidateSubmission() { '1' => [3] ], // Expected Result - false + 'Question "q1" requires between 2 and -1 answers.', ], 'valid-multiple-with-limits' => [ // Questions [ - ['id' => 1, 'type' => 'multiple', 'isRequired' => false, 'extraSettings' => ['optionsLimitMin' => 2, 'optionsLimitMax' => 3], 'options' => [ + ['id' => 1, 'type' => 'multiple', 'text' => 'q1', 'isRequired' => false, 'extraSettings' => ['optionsLimitMin' => 2, 'optionsLimitMax' => 3], 'options' => [ ['id' => 3], ['id' => 5], ['id' => 7], @@ -753,55 +753,55 @@ public function dataValidateSubmission() { '1' => [3,9] ], // Expected Result - true + null, ], 'invalid-short-phone' => [ // Questions [ - ['id' => 1, 'type' => 'short', 'isRequired' => false, 'extraSettings' => ['validationType' => 'phone']] + ['id' => 1, 'type' => 'short', 'text' => 'q1', 'isRequired' => false, 'extraSettings' => ['validationType' => 'phone']] ], // Answers [ '1' => ['0800 NEXTCLOUD'] ], // Expected Result - false + 'Invalid input for question "q1".', ], 'invalid-short-regex-not-matching' => [ // Questions [ - ['id' => 1, 'type' => 'short', 'isRequired' => false, 'extraSettings' => ['validationType' => 'regex', 'validationRegex' => '/[a-z]{4}/']] + ['id' => 1, 'type' => 'short', 'text' => 'q1', 'isRequired' => false, 'extraSettings' => ['validationType' => 'regex', 'validationRegex' => '/[a-z]{4}/']] ], // Answers [ '1' => ['abc'] ], // Expected Result - false + 'Invalid input for question "q1".', ], 'invalid-short-number' => [ // Questions [ - ['id' => 1, 'type' => 'short', 'isRequired' => false, 'extraSettings' => ['validationType' => 'number']] + ['id' => 1, 'type' => 'short', 'text' => 'q1', 'isRequired' => false, 'extraSettings' => ['validationType' => 'number']] ], // Answers [ '1' => ['11i'] ], // Expected Result - false + 'Invalid input for question "q1".', ], 'invalid-date-question' => [ // Questions [ - ['id' => 1, 'type' => 'date', 'isRequired' => false] + ['id' => 1, 'type' => 'date', 'text' => 'q1', 'isRequired' => false] ], // Answers [ '1' => ['31.12.2022'] ], // Expected Result - false + 'Invalid date/time format for question "q1".', ], 'full-good-submission' => [ // Questions @@ -850,7 +850,7 @@ public function dataValidateSubmission() { '13' => ['abc123'], ], // Expected Result - true + null, ] ]; } @@ -860,9 +860,9 @@ public function dataValidateSubmission() { * * @param array $questions * @param array $answers - * @param bool $expected + * @param null|string $expected */ - public function testValidateSubmission(array $questions, array $answers, bool $expected) { + public function testValidateSubmission(array $questions, array $answers, ?string $expected) { $this->mailer->method('validateMailAddress')->willReturnCallback(function ($mail) { return $mail === 'some.name+context@example.com'; }); From 04db1c4a1cfcb08a926a2fc88f36fd81a560994c Mon Sep 17 00:00:00 2001 From: Kostiantyn Miakshyn Date: Mon, 10 Feb 2025 06:48:20 +0100 Subject: [PATCH 2/2] Improve error messages for invalid submissions (fix code review comments) Signed-off-by: Kostiantyn Miakshyn --- lib/Controller/ApiController.php | 9 +++--- lib/Service/SubmissionService.php | 32 +++++++++++--------- tests/Unit/Controller/ApiControllerTest.php | 11 ++----- tests/Unit/Service/SubmissionServiceTest.php | 12 ++++++-- 4 files changed, 33 insertions(+), 31 deletions(-) diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index 3a59f1596..ea30608d6 100644 --- a/lib/Controller/ApiController.php +++ b/lib/Controller/ApiController.php @@ -1250,10 +1250,11 @@ 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 diff --git a/lib/Service/SubmissionService.php b/lib/Service/SubmissionService.php index 05ebc993f..9023d5816 100644 --- a/lib/Service/SubmissionService.php +++ b/lib/Service/SubmissionService.php @@ -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']; @@ -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 @@ -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'])); } /* @@ -381,7 +385,7 @@ 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 @@ -389,31 +393,31 @@ public function validateSubmission(array $questions, array $answers, string $for 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'])); } } } @@ -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; } /** diff --git a/tests/Unit/Controller/ApiControllerTest.php b/tests/Unit/Controller/ApiControllerTest.php index 7cf623b25..96640d101 100644 --- a/tests/Unit/Controller/ApiControllerTest.php +++ b/tests/Unit/Controller/ApiControllerTest.php @@ -690,10 +690,6 @@ public function testNewSubmission_answers() { $this->formAccess(); - $this->submissionService - ->method('validateSubmission') - ->willReturn(null); - $this->submissionMapper->expects($this->once()) ->method('insert') ->with($this->callback(function ($submission) { @@ -795,10 +791,6 @@ public function testNewSubmission_forbiddenException($hasUserAccess, $hasFormExp ->with(1) ->willReturn($form); - $this->submissionService - ->method('validateSubmission') - ->willReturn(null); - $this->formAccess($hasUserAccess, $hasFormExpired, $canSubmit); $this->expectException($exception); @@ -825,9 +817,10 @@ public function testNewSubmission_validateSubmission() { $this->submissionService ->method('validateSubmission') - ->willReturn('error message'); + ->willThrowException(new \InvalidArgumentException('error message')); $this->expectException(OCSBadRequestException::class); + $this->expectExceptionMessage('error message'); $this->apiController->newSubmission(1, [], ''); } diff --git a/tests/Unit/Service/SubmissionServiceTest.php b/tests/Unit/Service/SubmissionServiceTest.php index 4b1690d88..e0f46c59e 100644 --- a/tests/Unit/Service/SubmissionServiceTest.php +++ b/tests/Unit/Service/SubmissionServiceTest.php @@ -719,7 +719,7 @@ public function dataValidateSubmission() { '1' => [3, 5, 7] ], // Expected Result - 'Question "q1" requires between -1 and 2 answers.', + 'Question "q1" requires at most 2 answers.', ], 'invalid-multiple-too-few-answers' => [ // Questions @@ -736,7 +736,7 @@ public function dataValidateSubmission() { '1' => [3] ], // Expected Result - 'Question "q1" requires between 2 and -1 answers.', + 'Question "q1" requires at least 2 answers.', ], 'valid-multiple-with-limits' => [ // Questions @@ -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); } };