diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index 4072dedc1..9746beb32 100644 --- a/lib/Controller/ApiController.php +++ b/lib/Controller/ApiController.php @@ -1251,9 +1251,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'; });