[mickenordin]

This is an implementation of Conditional Logic. It is not well tested, and I would appreciate review and testing by others to see if it works as intended. I have created a few forms and clicked around to the best of my abilities, but I have implemented the fix for the benefit of others that need it and have no real use case myself to test with.

Fixes: #358

🖼️ Screenshoots

[codecov]

Codecov Report

❌ Patch coverage is 47.68212% with 237 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
lib/Controller/ApiController.php	50.25%	98 Missing ⚠️
lib/Service/SubmissionService.php	42.75%	83 Missing ⚠️
lib/Db/QuestionMapper.php	0.00%	30 Missing ⚠️
lib/Migration/Version050300Date20260118000000.php	0.00%	19 Missing ⚠️
lib/Service/FormsService.php	87.93%	7 Missing ⚠️

📢 Thoughts on this report? Let us know!

[mickenordin]

Codecov Report

❌ Patch coverage is 56.88073% with 141 lines in your changes missing coverage. Please review.
Files with missing lines Patch % Lines
lib/Service/SubmissionService.php 48.87% 68 Missing ⚠️
lib/Db/QuestionMapper.php 0.00% 39 Missing ⚠️
lib/Migration/Version050300Date20260118000000.php 0.00% 19 Missing ⚠️
lib/Controller/ApiController.php 86.04% 12 Missing ⚠️
lib/Service/FormsService.php 93.47% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

Updated with more tests so threshold is well exceeded

[github-actions]

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

[timedin-de]

Thank you! Will test further. It seems a bit sluggish sometimes it throws an
{"ocs":{"meta":{"status":"failure","statuscode":400,"message":"Invalid extraSettings, will not update."},"data":[]}} but seems to work otherwise.

This is the missing feature i need to convince my non-profit to stop using google forms 🙏

[timedin-de]

Hi @mickenordin , as this was stale for 3 Months i started fixing some bugs by myself. I cant push on this branch, should i just post my .patch diffs here?

Problems noticed/fixed:

• Error Messages while editing
• design fixes (hiding header and the broken trigger question header)
• cloning subquestions
• hiding unused "Clone" on triggerquestion
• fixing options (three dot menu) on questions
• fix options and extraSettings on subquestions not safed
• result view (currently only Detail View)
• result view Overview
• export of subquestions
• reordering/moving/dragging of subquestions
• subquestion fileupload not shown in results
• clear not working
• technical name(?)
• complex conditions (like number between....) not working
• show all matching branches (not only first)
• aria labeled by wrong

Further ideas for this (should be new PR):

• nested conditionals
• and/or logic (multiple trigger questions)
• default case (when nothing matches)

[mickenordin]

Yes

Hi @mickenordin , as this was stale for 3 Months i started fixing some bugs by myself. I cant push on this branch, should i just post my .patch diffs here?

Problems noticed/fixed:

* [x]  Error Messages while editing

* [x]   design fixes (hiding header and the broken trigger question header)

* [x]  cloning subquestions

* [x]  hiding unused "Clone" on triggerquestion

* [x]  fixing options (three dot menu) on questions

* [x]  fix options and extraSettings on subquestions not safed

* [x]  result view (currently only Detail View)

* [ ]  result view Overview

* [ ]  export of subquestions

* [ ]  reordering/moving/dragging of subquestions

* [ ]  subquestion fileupload not shown in results

* [ ]  clear not working

* [ ]  complex conditions (like number between....) not working

Further ideas for this (should be new PR):

* [ ]  nested conditionals

Yes, please do. I don't know of a better way since I don't have access to any access settings in the Nextcloud org, even though I was given the ability to branch in their repos myself.

I'll make sure to credit you on the diffs of course.

[timedin-de]

Hey @mickenordin
as I rebased the branch (including changing your commit, to support the new question types etc.) i guess it makes more sense to reset to my branch, if you want.

git remote add timedin git@github.com:timedin-de/forms.git
git fetch timedin
git reset timedin/kano-conditional --hard
git push --force-with-lease --force-if-includes

[mickenordin]

Hey @mickenordin as I rebased the branch (including changing your commit, to support the new question types etc.) i guess it makes more sense to reset to my branch, if you want.

git remote add timedin git@github.com:timedin-de/forms.git
git fetch timedin
git reset timedin/kano-conditional --hard
git push --force-with-lease --force-if-includes

I did, please make sure to sign-off your commits, and perhaps squash them to fewer logical units. You can take a look at the failing tests and see how to fix them.

[AndyScherzinger]

Accessibility: the per-branch buttons can't be told apart, and a branch isn't a named group. Each branch has a "Delete branch" button here and an "Add subquestion" menu (line 202) with the same accessible name, and the branch's label (.branch__label, e.g. "Yes") is a plain <span>. With two or more branches, a screen-reader user tabbing through, or browsing by buttons, hears "Delete branch, Delete branch, Add subquestion, Add subquestion…", with no way to tell which branch each one belongs to (WCAG 2.4.6, 1.3.1). That's what the accessibility tree shows on a running instance built from 1b4726ed.

Suggestion: make each branch a group named after its label, and include the label in the button names:

<div class="branch" role="group" :aria-labelledby="`branch-label-${branch.id}`">
	<span :id="`branch-label-${branch.id}`" class="branch__label">…</span>
	<NcButton :aria-label="t('forms', 'Delete branch {label}', { label: getBranchLabel(branch, branchIndex) })" …>

and the same for t('forms', 'Add subquestion to branch {label}', …).

[AndyScherzinger]

❌ Withdrawn after a live check, see the reply below. The client shows nothing before the trigger is answered, and an untouched trigger is not submitted at all.

An unanswered linear-scale trigger is treated as the value 0: (float)($triggerAnswer[0] ?? 0) here, and parseFloat(this.triggerValues[0]) || 0 in QuestionConditional.vue:1055. So before the respondent has touched the scale, value_not_equals (any non-zero value), value_max and a value_range that includes 0 already count as matched, on both client and server.

Result: the subquestions of those branches show up straight away, and if they are required, the submission is rejected even though the trigger wasn't answered. An empty trigger answer should match no branch at all, e.g. if ($triggerAnswer === [] || $triggerAnswer[0] === '') { return false; } before the numeric conversion, and the same on the client.

[AndyScherzinger]

Correction, I'm withdrawing this one. I checked it on a running instance and it doesn't hold up for real users:

• activeBranches returns null while the trigger has no value, so the client shows no subquestions before the scale is answered;
• an untouched conditional isn't sent at all (payload {"answers":{"<other question>":["x"]}}), and the submission succeeds with 201;
• the server only takes this path when a client explicitly sends {"trigger": []}. That case falls under the missing trigger validation in the SubmissionService.php:994 thread.

Sorry for the noise.

[AndyScherzinger]

This runs one query per conditional question (findByParentQuestion), plus one getOptions() query per subquestion (line 176). getQuestions above was optimised to load all options in one batch (optionMapper->findByQuestions($questionIds), line 121), so a form with a few conditionals of several subquestions each brings the N+1 pattern back.

This block also repeats the accept handling for file questions, and the isRequired logic for a minimum number of options, from the top-level question path, so the two will drift apart. Suggestion: collect all subquestion IDs of the form first, then load their options in one findByQuestions() call, and run subquestions through the same per-question formatting helper as top-level questions.

[AndyScherzinger]

Accessibility: subquestions appear and disappear silently. Selecting a trigger answer inserts or removes this block, but nothing tells assistive technology. Focus stays on the trigger and there is no status message. Switching from "Yes" to "No" also removes the questions the user may have just filled in, without notice (WCAG 4.1.3 Status Messages, 3.2.2 On Input).

The content is inserted directly after the trigger in DOM order, so it is reachable. A short polite status message when the set of visible subquestions changes would make the change noticeable, e.g. an aria-live="polite" region (visually hidden) that reads "2 follow-up questions shown" / "Follow-up questions removed". Seen on a running instance built from 1b4726ed: after selecting "Yes", the page's only live regions belonged to unrelated components, and focus stayed on the radio.

[AndyScherzinger]

Accessibility (minor): the heading level skips. In the editor the outline is h1 Edit form → h2 Form title → h4 Conditional Branches. Question titles there are text boxes, not headings, so there is no h3 in between, and each conditional adds another "Conditional Branches" h4 with no question context. Screen-reader users who move through headings get a skipped level and several identical headings (WCAG 1.3.1, best practice). Consider an h3, or include the question title, e.g. t('forms', 'Branches for "{question}"', { question: text }).

[AndyScherzinger]

Review summary (code, security and accessibility at 1b4726ed)

I reviewed the full diff and deployed this branch on a fresh Nextcloud dev instance, with a clean Forms install so all migrations run from scratch. I tested the findings there with real requests and browser runs: two unprivileged test users plus anonymous public-link visitors, forms built through the API, the edit and submit views driven with Playwright, and axe-core (WCAG 2.2 A/AA) plus the accessibility tree for the UI. Rows marked code-level below were not tested live. Each inline comment now starts with its verification status.

🔒 Security

• Cross-user injection of questions into other users' forms (details in the ApiController.php:549 thread). updateQuestion accepts any parentQuestionId without checking ownership. Verified end to end:

  • an anonymous visitor of a public link gets the conditional's ID and branch ID from the page's form initial state;
  • with only those, another user's PATCH attached their own question (200) and could keep editing its text afterwards (200);
  • a separate anonymous respondent on the public link was shown the injected question, and their answer was stored in the form owner's results.

  This should block merging. All parts involved (the parent_question_id column and the parent handling) are new in this PR; no released version is affected.

• Checked and not an issue:

  • a subquestion ID sent as a top-level answer is rejected (400);
  • newQuestion and reorderSubQuestions both check that the parent and the questions belong to the form;
  • the server-side regex condition showed no slowdown with /^(a+)+$/ and 10,000-character input;
  • no new v-html sinks (code-level).

Verification status of the inline findings

Finding	Live result
Copying a form drops all subquestions	✅ the copy has 0; an old-style snapshot in extraSettings is copied and stored twice
Dropdown triggers never activate on the server	✅ the browser sends "trigger":[7], the branch is shown, and the typed subquestion answer is not stored; the unit tests only use string IDs
Trigger answer not validated	✅ a bogus option is stored as text; a required conditional with an empty trigger is accepted; a file trigger with a valid upload stores nothing (Results rendering not checked)
storeConditionalAnswer differs from top-level storage	✅ ranking is stored as 3 rows instead of JSON; the "other" prefix is kept for subquestions and triggers; grid only via the API
Deleting a subquestion renumbers top-level questions	✅ top-level orders 2–5 become 1–4, and the branch keeps a gap; a cloned subquestion gets order 6
"Change trigger type" can't be saved	✅ 400, the change is lost; switching multiple → short with shuffleOptions is also 400
Type changes bypass the nested-conditional guard	✅ retyping to conditional gets 200 (creating one gets 400); a retyped conditional keeps its export columns
Deleting a branch orphans its subquestions	✅ still an export column; subquestion columns interleave with top-level ones
Copying a single conditional	✅ no subquestions, and the condition references the original's option ID
multiple trigger: any vs. all conditions	⚠️ narrowed: only for multi-condition branches, which the editor can't create
Unanswered linear scale counts as 0	❌ withdrawn: an untouched trigger isn't submitted and the client shows nothing
N+1 queries in loadConditionalSubQuestions	code-level (performance)
Accessibility: ID/number collision, unlabelled condition controls, nested <li>, ambiguous branch buttons, heading skip, no status message	✅ observed on the instance
Accessibility: colour condition shown only as a swatch	code-level

Not tied to a line

• No end-to-end or accessibility test coverage for the conditional UI. The PR adds no tests under playwright/, although a11y-question-inputs.spec.ts and submit-form.spec.ts exist. A single e2e test that creates a dropdown conditional, picks an option, fills the revealed subquestion, submits and checks the results would have caught the dropdown data loss, the ID collision and the nested <li>. An axe pass in the style of the existing a11y spec would cover the rest.

Already raised by @blizzz and not repeated: branch switching keeping subquestion answers, regex delimiters between frontend and backend, the user-provided subQuestionType, and the unused constant. The parentQuestionId question there turned out to be the security issue above.