Skip to content

Commit f199862

Browse files
Chartman123Chartman123
authored
Merge pull request #3406 from nextcloud/backport/3403/stable5.3
[stable5.3] fix(QuestionGrid): fix validation for all subtypes and add tests
2 parents 764c193 + 1f20dec commit f199862

2 files changed

Lines changed: 132 additions & 13 deletions

File tree

‎lib/Service/SubmissionService.php‎

Lines changed: 92 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -551,7 +551,7 @@ public function validateSubmission(array $questions, array $answers, string $for
551551
// Check if all answers are within the possible options
552552
if (in_array($question['type'], Constants::ANSWER_TYPES_PREDEFINED) && empty($question['extraSettings']['allowOtherAnswer'])) {
553553
// Normalize option IDs once for consistent comparison (DB may return ints, request may send strings)
554-
$optionIds = array_map('intval', array_column($question['options'] ?? [], 'id'));
554+
$optionIds = $this->normalizeOptionIds($question['options'] ?? []);
555555

556556
foreach ($answers[$questionId] as $answer) {
557557
// Handle linear scale questions
@@ -562,21 +562,22 @@ public function validateSubmission(array $questions, array $answers, string $for
562562
throw new \InvalidArgumentException(sprintf('The answer for question "%s" must be an integer between %d and %d.', $question['text'], $optionsLowest, $optionsHighest));
563563
}
564564
}
565+
// Check if all grid rows, columns and values match the configured grid subtype
566+
elseif ($question['type'] === Constants::ANSWER_TYPE_GRID) {
567+
$this->validateGridQuestion($question, $answers[$questionId]);
568+
}
565569
// Search corresponding option, return false if non-existent
566570
else {
567-
$subAnswers = is_array($answer) ? $answer : [$answer];
568-
foreach ($subAnswers as $subAnswer) {
569-
// Accept numeric strings like "46" from JSON payloads reliably (e.g. with hardening extensions enabled)
570-
$answerId = is_int($subAnswer) ? $subAnswer : (is_string($subAnswer) ? intval(trim($subAnswer)) : null);
571-
572-
// Reject non-numeric / malformed values early
573-
if ($answerId === null || (string)$answerId !== (string)intval($answerId)) {
574-
throw new \InvalidArgumentException(sprintf('Answer "%s" for question "%s" is not a valid option.', is_scalar($subAnswer) ? (string)$subAnswer : gettype($subAnswer), $question['text']));
575-
}
571+
// Accept numeric strings like "46" from JSON payloads reliably (e.g. with hardening extensions enabled)
572+
$answerId = is_int($answer) ? $answer : (is_string($answer) ? intval(trim($answer)) : null);
576573

577-
if (!in_array($answerId, $optionIds, true)) {
578-
throw new \InvalidArgumentException(sprintf('Answer "%s" for question "%s" is not a valid option.', $subAnswer, $question['text']));
579-
}
574+
// Reject non-numeric / malformed values early
575+
if ($answerId === null || (string)$answerId !== (string)intval($answerId)) {
576+
throw new \InvalidArgumentException(sprintf('Answer "%s" for question "%s" is not a valid option.', is_scalar($answer) ? (string)$answer : gettype($answer), $question['text']));
577+
}
578+
579+
if (!in_array($answerId, $optionIds, true)) {
580+
throw new \InvalidArgumentException(sprintf('Answer "%s" for question "%s" is not a valid option.', $answer, $question['text']));
580581
}
581582
}
582583
}
@@ -707,6 +708,84 @@ private function validateShortQuestion(array $question, string $data): bool {
707708
}
708709
}
709710

711+
private function validateGridQuestion(array $question, array $answers): void {
712+
$rowIds = $this->normalizeOptionIds($question['options'] ?? [], Option::OPTION_TYPE_ROW);
713+
$columnIds = $this->normalizeOptionIds($question['options'] ?? [], Option::OPTION_TYPE_COLUMN);
714+
$gridType = $question['extraSettings']['questionType'] ?? null;
715+
716+
foreach ($answers as $rowId => $rowAnswer) {
717+
$normalizedRowId = $this->normalizeNumericOptionId($rowId);
718+
if ($normalizedRowId === null || !in_array($normalizedRowId, $rowIds, true)) {
719+
throw new \InvalidArgumentException(sprintf('Row "%s" for question "%s" is not a valid option.', $rowId, $question['text']));
720+
}
721+
722+
if ($gridType === Constants::ANSWER_GRID_TYPE_RADIO) {
723+
$selectedColumnIds = [$rowAnswer];
724+
} elseif ($gridType === Constants::ANSWER_GRID_TYPE_CHECKBOX) {
725+
$selectedColumnIds = is_array($rowAnswer) ? $rowAnswer : [];
726+
if (!is_array($rowAnswer)) {
727+
throw new \InvalidArgumentException(sprintf('Invalid answer for question "%s".', $question['text']));
728+
}
729+
} elseif ($gridType === Constants::ANSWER_GRID_TYPE_NUMBER) {
730+
if (!is_array($rowAnswer)) {
731+
throw new \InvalidArgumentException(sprintf('Invalid answer for question "%s".', $question['text']));
732+
}
733+
734+
foreach ($rowAnswer as $columnId => $value) {
735+
$normalizedColumnId = $this->normalizeNumericOptionId($columnId);
736+
if ($normalizedColumnId === null || !in_array($normalizedColumnId, $columnIds, true)) {
737+
throw new \InvalidArgumentException(sprintf('Column "%s" for question "%s" is not a valid option.', $columnId, $question['text']));
738+
}
739+
if ($value !== '' && !is_numeric($value)) {
740+
throw new \InvalidArgumentException(sprintf('Answer "%s" for question "%s" must be a number.', is_scalar($value) ? (string)$value : gettype($value), $question['text']));
741+
}
742+
}
743+
continue;
744+
} else {
745+
throw new \InvalidArgumentException(sprintf('Invalid grid type for question "%s".', $question['text']));
746+
}
747+
748+
foreach ($selectedColumnIds as $columnId) {
749+
if ((!is_int($columnId) && (!is_string($columnId) || !ctype_digit($columnId))) || !in_array((int)$columnId, $columnIds, true)) {
750+
throw new \InvalidArgumentException(sprintf('Column "%s" for question "%s" is not a valid option.', is_scalar($columnId) ? (string)$columnId : gettype($columnId), $question['text']));
751+
}
752+
}
753+
}
754+
}
755+
756+
private function normalizeNumericOptionId(mixed $value): ?int {
757+
if (is_int($value)) {
758+
return $value;
759+
}
760+
761+
if (is_string($value) && ctype_digit($value)) {
762+
return (int)$value;
763+
}
764+
765+
return null;
766+
}
767+
768+
/**
769+
* @param array<int, array{id?: mixed, optionType?: string}> $options
770+
* @return list<int>
771+
*/
772+
private function normalizeOptionIds(array $options, ?string $optionType = null): array {
773+
$normalizedIds = [];
774+
775+
foreach ($options as $option) {
776+
if ($optionType !== null && (($option['optionType'] ?? null) !== $optionType)) {
777+
continue;
778+
}
779+
780+
$normalizedId = $this->normalizeNumericOptionId($option['id'] ?? null);
781+
if ($normalizedId !== null) {
782+
$normalizedIds[] = $normalizedId;
783+
}
784+
}
785+
786+
return $normalizedIds;
787+
}
788+
710789
private function setCellValue(Worksheet $activeWorksheet, int $column, int $row, mixed $value, string $fileFormat): void {
711790
if (!is_string($value)) {
712791
$activeWorksheet->setCellValue([$column, $row], $value);

‎tests/Unit/Service/SubmissionServiceTest.php‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1237,6 +1237,46 @@ public static function dataValidateSubmission() {
12371237
// Expected Result
12381238
null,
12391239
],
1240+
'valid-number-grid' => [
1241+
[
1242+
['id' => 458, 'type' => 'grid', 'text' => 'Number grid', 'isRequired' => false, 'extraSettings' => ['questionType' => Constants::ANSWER_GRID_TYPE_NUMBER], 'options' => [
1243+
['id' => 2433, 'optionType' => 'column'],
1244+
['id' => 2434, 'optionType' => 'column'],
1245+
['id' => 2435, 'optionType' => 'column'],
1246+
['id' => 2436, 'optionType' => 'row'],
1247+
['id' => 2437, 'optionType' => 'row'],
1248+
['id' => 2438, 'optionType' => 'row'],
1249+
]],
1250+
],
1251+
[
1252+
'458' => [
1253+
'2436' => ['2433' => '1', '2434' => '', '2435' => ''],
1254+
'2437' => ['2433' => '', '2434' => '1', '2435' => ''],
1255+
'2438' => ['2433' => '', '2434' => '', '2435' => '1'],
1256+
],
1257+
],
1258+
null,
1259+
],
1260+
'invalid-number-grid-value' => [
1261+
[
1262+
['id' => 1, 'type' => 'grid', 'text' => 'Number grid', 'isRequired' => false, 'extraSettings' => ['questionType' => Constants::ANSWER_GRID_TYPE_NUMBER], 'options' => [
1263+
['id' => 10, 'optionType' => 'row'],
1264+
['id' => 20, 'optionType' => 'column'],
1265+
]],
1266+
],
1267+
['1' => ['10' => ['20' => 'not a number']]],
1268+
'Answer "not a number" for question "Number grid" must be a number.',
1269+
],
1270+
'invalid-number-grid-column' => [
1271+
[
1272+
['id' => 1, 'type' => 'grid', 'text' => 'Number grid', 'isRequired' => false, 'extraSettings' => ['questionType' => Constants::ANSWER_GRID_TYPE_NUMBER], 'options' => [
1273+
['id' => 10, 'optionType' => 'row'],
1274+
['id' => 20, 'optionType' => 'column'],
1275+
]],
1276+
],
1277+
['1' => ['10' => ['21' => '1']]],
1278+
'Column "21" for question "Number grid" is not a valid option.',
1279+
],
12401280
'invalid-ranking-missing-option' => [
12411281
// Questions
12421282
[

0 commit comments

Comments
 (0)