Repository navigation
Conditional questions: go to a question based on the answer - #3833
AndyScherzinger wants to merge 4 commits into
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
I'll work on fixing CI checks and test coverage next |
b833f24 to
9d9b414
Compare
Chartman123
left a comment
There was a problem hiding this comment.
Added a few general comments/questions.
One thing that came to my mind: would it be easier to not have a new "question type" conditional but to make this part of the props of a "normal" question?
21f29c0 to
c2cf51a
Compare
I personally also would prefer this Version as this could allow trigger questions to be seperated from the conditional question. (E.g Last question is only shown when first question is selected). Grouping functionality would get implemented independently by other running PRs. |
b444385 to
025620c
Compare
Agreed, and done: there is no |
|
@timedin-de thanks, that is the direction the stack has taken now: there is no I looked at how MS Forms does it. There, each option of a choice question jumps forward to a later question, a section or the end of the form, and "nested" comes from chaining those jumps. Two things from your comment are not covered by this stack yet:
Both would come as follow-up PRs after this stack, so the stack stays reviewable. Section navigation ("after this section go to …", "go to section" per option, "end of the form") belongs to the sections work in #3664. Does that cover what you had in mind, or do you need jumps to single questions? |
ce2e434 to
06bd420
Compare
|
@Chartman123 @timedin-de @mickenordin before I push the next round, a heads-up on a scope change for this stack. It is larger than a fixup, so I would rather hear objections now. 1. Conditions move onto each answer Today the rules sit in a panel below the question ("Follow-up questions of …", one card per branch with its own condition editor). Google Forms and Microsoft Forms both define the rule in the row of the answer, and I will follow that:
Several answers can point to the same group. The stored format ( 2. Fewer trigger types Follow-up questions stay on radio, dropdown, checkboxes, linear scale, date, date and time, and time. I will drop them on short text, long text, color and file:
Reason: these types have no fixed set of answers, so they do not fit "on each answer". Google Forms does not branch on them, and as far as I can tell Microsoft Forms does not either. It also takes the condition types from twelve to four and removes regex evaluation on submit. What this does and does not remove:
The removed code stays in the branch history of #3836–#3838. 3. Stricter validation on save
4. What comes after this stack
All PRs of the stack are drafts until the rework is pushed. I will also rebase onto If you disagree with dropping the text, color and file triggers, or with the per-answer editor, please say so here before I push. |
8c15e5a to
b2161c8
Compare
22a1ccf to
3554e9b
Compare
0cd263d to
3a6a749
Compare
The check for answers to unknown questions compares the question ids strictly, as the validation of each question already does, and names the id as it was sent. Option ids are compared as canonical integers with the ids of the options of the question. Answers to questions that allow the answer "Other" are an option id of the question or an answer "Other", and storing an answer uses the same check for the answer "Other" as the validation. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
As in Microsoft and Google Forms, an answer can lead to a later question or to the end of the form, while the form stays a flat list of questions. Radio and dropdown questions store a target per option and for the answer "Other" in extraSettings.goTo. The other question types with jumps (short and long text, file, color, ranking, grid) store one target after the question in extraSettings.next. A question type has either goTo or next, never both: sending the other one is rejected as invalid extraSettings, and an answer without a jump leads to the following question. A target is the id of a question of the same form or "submit", and an option id belongs to the question; ids of questions of other forms and of options that are not options of the question are rejected, while ids of deleted questions of the form are accepted. The path of a respondent starts at the first question and follows these targets. Only the first value of an answer decides, which keeps the walk linear in the size of a submission, and answers to questions the submit view does not show, such as questions without a title, do not count, so they cannot skip required questions. A target that is not a later question of the form, for example after deleting it, counts as the following question. Answers to the questions off the path are ignored, also when a form changed while it was answered, and required only applies on the path. An answer "Other" needs a text, as it decides the path. Jumps never go back. Updating, reordering and copying a question remove the jumps to the question itself or to an earlier question; the request still succeeds with its usual status and a payload of the same shape, and the response header X-Forms-Adjusted lists the questions concerned as removed-jumps. A reorder returns the new extraSettings of these questions in its own response type QuestionOrder. Copying or importing a form maps the targets and option ids and drops such jumps as well, without the header, together with jumps whose target or option was not copied, jumps per answer with an invalid structure, and goTo or next on a question type without it. Exports keep the option ids for that. tests/fixtures/jump-paths.json holds the path cases, shared with the frontend. Based on the conditional questions of #3123 by Micke Nordin and TimedIn. Co-Authored-By: Micke Nordin <kano@sunet.se> Co-Authored-By: TimedIn <git@timedin.net> Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
"Go to based on answer" in the question menu shows a select at the end of each option row of radio and dropdown questions, and of the row "Other": the next question, a later question or the end of the form. The other question types with jumps get the select "After question N" below them instead. The accessible names of the selects of the answers include the question number as well. Below a width of 480px the select of an option moves onto its own line. A target that was deleted or moved before the question is shown as going to the next question. Unchecking "Go to" closes the menu and asks before it removes the jumps of the question. The warnings let jumps of questions the submit view does not show lead nowhere, and count a jump of the answer "Other" as broken while "Other" is off, as a reorder removes it as well. Changes are kept in the question and saved as complete lists. Extra settings the server adjusted replace the local ones unless they changed in the meantime, and deleting an option drops its jumps once the server confirms the deletion. playwright/e2e/jump-paths.spec.ts runs the path cases shared with the server against the frontend walk. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
The submit view shows the questions on the path of the current answers and announces when questions are added or skipped. Answers to skipped questions are kept while the form is open, but not submitted. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
3a6a749 to
e71a212
Compare
Chartman123
left a comment
There was a problem hiding this comment.
First review, mainly for the backend. Added a few comments to discuss :)
|
|
||
| // Delete current answers | ||
| $this->answerMapper->deleteBySubmission($submissionId); | ||
| $this->answerMapper->deleteBySubmission($submissionId, array_column(SubmissionService::getQuestionsOnPath($questions, $answers), 'id')); |
There was a problem hiding this comment.
Why do we delete only answers to questions on the path and not all answers like we do now?
| * @param list<int>|null $questionIds only the answers to these questions, all answers if null | ||
| */ | ||
| public function deleteBySubmission(int $submissionId): void { | ||
| public function deleteBySubmission(int $submissionId, ?array $questionIds = null): void { |
There was a problem hiding this comment.
See comment on ApiController
| if (in_array($questionType, Constants::QUESTION_TYPES_WITH_NEXT, true)) { | ||
| $allowed['next'] = ['integer', 'string']; | ||
| } | ||
| if (in_array($questionType, Constants::QUESTION_TYPES_WITH_GOTO, true)) { | ||
| $allowed['goTo'] = ['array']; | ||
| } |
There was a problem hiding this comment.
why introduce a new way of testing here and not integrate it in the match above? would of course need a reorganizing in Constants.php as well, but would make it clearer there.
| 'shuffleOptions' => ['boolean'], | ||
| ]; | ||
|
|
||
| public const JUMP_TARGET_SUBMIT = 'submit'; |
There was a problem hiding this comment.
reorder constants to better fitting sections in this file :)
|
|
||
| public const JUMP_TARGET_SUBMIT = 'submit'; | ||
|
|
||
| public const QUESTION_TYPES_WITH_GOTO = [ |
There was a problem hiding this comment.
same here, also add allowed extraSettings directly to the existing constants for file types (see other comment)
| self::ANSWER_TYPE_MULTIPLEUNIQUE, | ||
| ]; | ||
|
|
||
| public const QUESTION_TYPES_WITH_NEXT = [ |
There was a problem hiding this comment.
same here, also add allowed extraSettings directly to the existing constants for file types (see other comment)
There was a problem hiding this comment.
should this stay in the Questions folder? Would then of course also be valid for AnswerInput, which also is not directly a question component...
I know, both are closely related, so also fine if they stay here.
Conditional questions as in Microsoft and Google Forms, based on #3123 by @mickenordin with the fixes by @timedin-de, split into a stack so it can be reviewed in parts (as suggested by @Chartman123 in #3123 (comment)).
Fixes #358
Stack
Each PR is based on the one before it. Review and merge from the top.
This PR
An answer can lead to a later question or to the end of the form. The form stays a flat list of questions; the jumps live in the question's
extraSettings, as suggested by @Chartman123 in #3833 (review).Editor
Data
goToholds one target per answer,nextone target after the question; a type has one of them, never both."submit"; ids from other forms or questions are rejected with a 400.X-Forms-Adjusted(RFC 8941, e.g.removed-jumps=(3 7)); status and payload shape stay the same.X-Forms-Adjusted.Answers
tests/fixtures/jump-paths.jsonholds the path cases, run by PHPUnit and Playwright, so both sides walk the same way.Follow-ups
Sections with "go to section" (#357, #624) follow as a separate feature, see #3664; a section will be a valid jump target without a format change.
Tests
IUserFoldererrorsmainalso has on the test instance.🤖 AI (if applicable)