Improve error messages for invalid submissions

Signed-off-by: Kostiantyn Miakshyn <[email protected]>
nextcloud · Jan 29, 2025 · bd658c4 · bd658c4
1 parent 862f39e
commit bd658c4
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 37 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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 === '[email protected]';
 		});