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..fa0727e07 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..a81b176cf 100644 --- a/tests/Unit/Service/SubmissionServiceTest.php +++ b/tests/Unit/Service/SubmissionServiceTest.php @@ -619,7 +619,7 @@ public function dataValidateSubmission() { // Answers [], // Expected Result - false + 'a', ], 'required-not-answered-string' => [ // Questions @@ -631,7 +631,7 @@ public function dataValidateSubmission() { '1' => [''] ], // Expected Result - false + 'b', ], 'required-empty-other-answer' => [ // Questions @@ -645,7 +645,7 @@ public function dataValidateSubmission() { '1' => [Constants::QUESTION_EXTRASETTINGS_OTHER_PREFIX] ], // Expected Result - false + 'c', ], 'more-than-allowed' => [ // Questions @@ -660,7 +660,7 @@ public function dataValidateSubmission() { '1' => [3,5] ], // Expected Result - false + 'd', ], 'option-not-known' => [ // Questions @@ -675,7 +675,7 @@ public function dataValidateSubmission() { '1' => [3,10] ], // Expected Result - false + 'e', ], 'other-answer-not-allowed' => [ // Questions @@ -690,7 +690,7 @@ public function dataValidateSubmission() { '1' => [3, Constants::QUESTION_EXTRASETTINGS_OTHER_PREFIX . 'other answer'] ], // Expected Result - false + 'f', ], 'question-not-known' => [ // Questions @@ -702,7 +702,7 @@ public function dataValidateSubmission() { '2' => ['answer'] ], // Expected Result - false + 'g', ], 'invalid-multiple-too-many-answers' => [ // Questions @@ -719,7 +719,7 @@ public function dataValidateSubmission() { '1' => [3, 5, 7] ], // Expected Result - false + 'h', ], 'invalid-multiple-too-few-answers' => [ // Questions @@ -736,7 +736,7 @@ public function dataValidateSubmission() { '1' => [3] ], // Expected Result - false + 'j', ], 'valid-multiple-with-limits' => [ // Questions @@ -753,7 +753,7 @@ public function dataValidateSubmission() { '1' => [3,9] ], // Expected Result - true + null, ], 'invalid-short-phone' => [ // Questions @@ -765,7 +765,7 @@ public function dataValidateSubmission() { '1' => ['0800 NEXTCLOUD'] ], // Expected Result - false + 'q', ], 'invalid-short-regex-not-matching' => [ // Questions @@ -777,7 +777,7 @@ public function dataValidateSubmission() { '1' => ['abc'] ], // Expected Result - false + 'w', ], 'invalid-short-number' => [ // Questions @@ -789,7 +789,7 @@ public function dataValidateSubmission() { '1' => ['11i'] ], // Expected Result - false + 'e', ], 'invalid-date-question' => [ // Questions @@ -801,7 +801,7 @@ public function dataValidateSubmission() { '1' => ['31.12.2022'] ], // Expected Result - false + 'r', ], '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'; });