Skip to content

Commit b06c9b7

Browse files
committed
fix: harden conditional submission handling
- validate active-branch subquestions with full per-question rules - persist subquestion answers only for the activated branch - use -PHP_FLOAT_MAX as the value_range lower bound - drop unused QuestionMapper::deleteByParentQuestion Signed-off-by: Micke Nordin <kano@sunet.se>
1 parent 77070f3 commit b06c9b7

3 files changed

Lines changed: 23 additions & 40 deletions

File tree

lib/Controller/ApiController.php

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2076,7 +2076,6 @@ private function storeFileAnswer(Form $form, int $submissionId, array $question,
20762076
private function storeConditionalAnswer(Form $form, int $submissionId, array $question, array $answerData): void {
20772077
$extraSettings = $question['extraSettings'] ?? [];
20782078
$triggerType = $extraSettings['triggerType'] ?? null;
2079-
$branches = $extraSettings['branches'] ?? [];
20802079

20812080
// Handle the structure: could be {trigger: [...], subQuestions: {...}} or just trigger array
20822081
$triggerAnswers = $answerData['trigger'] ?? $answerData;
@@ -2119,8 +2118,9 @@ private function storeConditionalAnswer(Form $form, int $submissionId, array $qu
21192118
}
21202119
}
21212120

2122-
// Store subquestion answers
2123-
// Find the active branch to get subquestion definitions
2121+
// Store subquestion answers only for the branch the trigger activated
2122+
$activeBranch = $this->submissionService->getActiveBranch($question, $triggerAnswers);
2123+
$branches = $activeBranch === null ? [] : [$activeBranch];
21242124
foreach ($branches as $branch) {
21252125
$branchSubQuestions = $branch['subQuestions'] ?? [];
21262126
foreach ($branchSubQuestions as $subQuestion) {

lib/Db/QuestionMapper.php

Lines changed: 0 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -111,27 +111,6 @@ public function findByBranch(int $parentQuestionId, string $branchId): array {
111111
return $this->findEntities($qb);
112112
}
113113

114-
/**
115-
* Delete all subquestions of a parent conditional question
116-
*
117-
* @param int $parentQuestionId The ID of the parent conditional question
118-
*/
119-
public function deleteByParentQuestion(int $parentQuestionId): void {
120-
// First delete options for all subquestions
121-
$subQuestions = $this->findByParentQuestion($parentQuestionId, true);
122-
foreach ($subQuestions as $subQuestion) {
123-
$this->optionMapper->deleteByQuestion($subQuestion->getId());
124-
}
125-
126-
$qb = $this->db->getQueryBuilder();
127-
$qb->delete($this->getTableName())
128-
->where(
129-
$qb->expr()->eq('parent_question_id', $qb->createNamedParameter($parentQuestionId, IQueryBuilder::PARAM_INT))
130-
);
131-
132-
$qb->executeStatement();
133-
}
134-
135114
/**
136115
* @param int $formId
137116
*/

lib/Service/SubmissionService.php

Lines changed: 20 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -782,25 +782,29 @@ private function validateConditionalQuestion(array $question, array $answerData,
782782
return;
783783
}
784784

785-
// If we have an active branch, validate its subquestions
785+
// Validate the active branch's subquestions with the full per-question rules
786786
if ($activeBranch !== null && isset($activeBranch['subQuestions'])) {
787-
$subQuestions = $activeBranch['subQuestions'];
788-
789-
// Build a questions array from subquestions for validation
790-
foreach ($subQuestions as $subQuestion) {
791-
$subQuestionId = $subQuestion['id'];
792-
$subQuestionAnswered = isset($subQuestionAnswers[$subQuestionId]);
793-
794-
// Check if required subquestions have an answer
795-
if ($subQuestion['isRequired'] ?? false) {
796-
if (!$subQuestionAnswered || !array_filter($subQuestionAnswers[$subQuestionId], fn ($v) => $v !== '' && $v !== null)) {
797-
throw new \InvalidArgumentException(sprintf('Subquestion "%s" in conditional question "%s" is required.', $subQuestion['text'] ?? 'Unknown', $question['text']));
798-
}
799-
}
800-
}
787+
$this->validateSubmission($activeBranch['subQuestions'], $subQuestionAnswers, $formOwnerId);
801788
}
802789
}
803790

791+
/**
792+
* Resolve the branch a conditional question's trigger answer activates
793+
*
794+
* @param array $question The conditional question
795+
* @param array $triggerAnswer The trigger answer values
796+
* @return array|null The active branch or null if none matches
797+
*/
798+
public function getActiveBranch(array $question, array $triggerAnswer): ?array {
799+
$extraSettings = $question['extraSettings'] ?? [];
800+
return $this->findActiveBranch(
801+
$extraSettings['triggerType'] ?? '',
802+
$triggerAnswer,
803+
$extraSettings['branches'] ?? [],
804+
$question['options'] ?? [],
805+
);
806+
}
807+
804808
/**
805809
* Find the active branch based on trigger answer
806810
*
@@ -899,7 +903,7 @@ private function evaluateBranchConditions(string $triggerType, array $triggerAns
899903
return true;
900904
}
901905
} elseif ($type === 'value_range') {
902-
$min = $condition['min'] ?? PHP_FLOAT_MIN;
906+
$min = $condition['min'] ?? -PHP_FLOAT_MAX;
903907
$max = $condition['max'] ?? PHP_FLOAT_MAX;
904908
if ($numValue >= $min && $numValue <= $max) {
905909
return true;

0 commit comments

Comments
 (0)