From 8c08c5403f46c50febfc75bb077cb48ed2744f40 Mon Sep 17 00:00:00 2001 From: Josef Behr Date: Thu, 16 Jul 2026 12:46:03 +0200 Subject: [PATCH 1/5] fix(api): default option type to choice for non-grid questions Creating an option through POST .../questions/{id}/options without an optionType stored the option with a null type. Such options are saved but never rendered, so the question shows up with no selectable answers. The option_type migration (Version050300Date20250914000000) already backfills null types to 'choice', and DataStructure.md documents 'choice' as the default for normal option lists. newOption() just never applied that default to new inserts. Default optionType to 'choice' for any non-grid question when the caller omits it. Grid questions are left untouched, as their options require an explicit row/column type. Adds the missing Option::OPTION_TYPE_CHOICE constant alongside the existing row/column ones. Signed-off-by: Josef Behr Co-Authored-By: Claude Opus 4.8 (1M context) --- lib/Controller/ApiController.php | 7 +++++++ lib/Db/Option.php | 1 + tests/Integration/Api/ApiV3Test.php | 4 +++- 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index 848155601..580ede1d7 100644 --- a/lib/Controller/ApiController.php +++ b/lib/Controller/ApiController.php @@ -938,6 +938,13 @@ public function newOption(int $formId, int $questionId, array $optionTexts, ?str throw new OCSBadRequestException('This question is not part ot the given form'); } + // Options of non-grid questions default to the 'choice' type, mirroring + // the column default applied by the option_type migration. Without this, + // options created through the API keep a null type and are not rendered. + if ($optionType === null && $question->getType() !== Constants::ANSWER_TYPE_GRID) { + $optionType = Option::OPTION_TYPE_CHOICE; + } + // Retrieve all options sorted by 'order'. Takes the order of the last array-element and adds one. $options = $this->optionMapper->findByQuestion($questionId, $optionType); $lastOption = array_pop($options); diff --git a/lib/Db/Option.php b/lib/Db/Option.php index c36b04f76..87747530a 100644 --- a/lib/Db/Option.php +++ b/lib/Db/Option.php @@ -31,6 +31,7 @@ class Option extends Entity { protected ?int $order; protected ?string $optionType; + public const OPTION_TYPE_CHOICE = 'choice'; public const OPTION_TYPE_ROW = 'row'; public const OPTION_TYPE_COLUMN = 'column'; diff --git a/tests/Integration/Api/ApiV3Test.php b/tests/Integration/Api/ApiV3Test.php index a4f4328d2..596cfc6dd 100644 --- a/tests/Integration/Api/ApiV3Test.php +++ b/tests/Integration/Api/ApiV3Test.php @@ -1027,7 +1027,9 @@ public static function dataCreateNewOption() { // 'questionId' => Done dynamically below. 'text' => 'A new Option.', 'order' => 4, - 'optionType' => null, + // Non-grid questions default to the 'choice' option type + // when none is provided. + 'optionType' => 'choice', ] ] ]; From 754d826527b0ab4e7f0b8b67f7383e8c1f8af844 Mon Sep 17 00:00:00 2001 From: Josef Behr Date: Thu, 16 Jul 2026 15:04:54 +0200 Subject: [PATCH 2/5] fix(api): apply option-type default after ordering lookup Addresses review feedback on the placement of the default. Moving the default before findByQuestion() changed the ordering: the lookup filters by option type, so a freshly defaulted 'choice' no longer matched existing options that predate the option_type migration (or were inserted without a type), resetting the order to 1. Apply the default after the ordering lookup instead. findByQuestion() keeps the caller-provided type (null means "all options of the question"), so option order is unchanged, and the inserted option still gets 'choice'. Signed-off-by: Josef Behr Co-Authored-By: Claude Opus 4.8 (1M context) --- lib/Controller/ApiController.php | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index 580ede1d7..d20b65dbc 100644 --- a/lib/Controller/ApiController.php +++ b/lib/Controller/ApiController.php @@ -938,14 +938,9 @@ public function newOption(int $formId, int $questionId, array $optionTexts, ?str throw new OCSBadRequestException('This question is not part ot the given form'); } - // Options of non-grid questions default to the 'choice' type, mirroring - // the column default applied by the option_type migration. Without this, - // options created through the API keep a null type and are not rendered. - if ($optionType === null && $question->getType() !== Constants::ANSWER_TYPE_GRID) { - $optionType = Option::OPTION_TYPE_CHOICE; - } - // Retrieve all options sorted by 'order'. Takes the order of the last array-element and adds one. + // The lookup keeps the caller-provided $optionType (null means "all options of the question"), + // so the ordering is not affected by the default applied below. $options = $this->optionMapper->findByQuestion($questionId, $optionType); $lastOption = array_pop($options); if ($lastOption) { @@ -954,6 +949,13 @@ public function newOption(int $formId, int $questionId, array $optionTexts, ?str $optionOrder = 1; } + // Options of non-grid questions default to the 'choice' type, mirroring the + // value applied by the option_type migration. Without a type the stored + // option is not rendered by the frontend. + if ($optionType === null && $question->getType() !== Constants::ANSWER_TYPE_GRID) { + $optionType = Option::OPTION_TYPE_CHOICE; + } + $addedOptions = []; foreach ($optionTexts as $text) { $option = new Option(); From f66dd9be1901fc13006858231d49e3513b75bb15 Mon Sep 17 00:00:00 2001 From: Josef Behr Date: Thu, 16 Jul 2026 15:18:22 +0200 Subject: [PATCH 3/5] fix(api): default option_type via column default and migration Follow-up to review: move the default out of the controller logic and into the schema, as suggested. - New migration gives the option_type column a 'choice' default and backfills rows still stored without a type. - newOption() no longer writes an explicit null, so the column default applies for normal option lists, and re-reads the inserted option so the response reflects the stored value. - Grid options are unaffected, as callers always pass an explicit row/column type. Removes the earlier controller-side default and the OPTION_TYPE_CHOICE constant. Integration fixtures now receive 'choice' from the column default, so the affected expectations are updated. Signed-off-by: Josef Behr Co-Authored-By: Claude Opus 4.8 (1M context) --- lib/Controller/ApiController.php | 21 +++--- lib/Db/Option.php | 1 - .../Version050300Date20260716000000.php | 65 +++++++++++++++++++ tests/Integration/Api/ApiV3Test.php | 6 +- 4 files changed, 77 insertions(+), 16 deletions(-) create mode 100644 lib/Migration/Version050300Date20260716000000.php diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index d20b65dbc..c2ec6676e 100644 --- a/lib/Controller/ApiController.php +++ b/lib/Controller/ApiController.php @@ -939,8 +939,6 @@ public function newOption(int $formId, int $questionId, array $optionTexts, ?str } // Retrieve all options sorted by 'order'. Takes the order of the last array-element and adds one. - // The lookup keeps the caller-provided $optionType (null means "all options of the question"), - // so the ordering is not affected by the default applied below. $options = $this->optionMapper->findByQuestion($questionId, $optionType); $lastOption = array_pop($options); if ($lastOption) { @@ -949,13 +947,6 @@ public function newOption(int $formId, int $questionId, array $optionTexts, ?str $optionOrder = 1; } - // Options of non-grid questions default to the 'choice' type, mirroring the - // value applied by the option_type migration. Without a type the stored - // option is not rendered by the frontend. - if ($optionType === null && $question->getType() !== Constants::ANSWER_TYPE_GRID) { - $optionType = Option::OPTION_TYPE_CHOICE; - } - $addedOptions = []; foreach ($optionTexts as $text) { $option = new Option(); @@ -963,12 +954,18 @@ public function newOption(int $formId, int $questionId, array $optionTexts, ?str $option->setQuestionId($questionId); $option->setText($text); $option->setOrder($optionOrder++); - $option->setOptionType($optionType); + // Only set the type when the caller provides one, so the 'choice' + // column default applies for normal option lists. Setting a null + // here would override that default and leave the option unrendered. + if ($optionType !== null) { + $option->setOptionType($optionType); + } try { $option = $this->optionMapper->insert($option); - // Add the stored option to the collection of added options - $addedOptions[] = $option->read(); + // Re-read so the response reflects the column default that was + // applied when no type was set explicitly. + $addedOptions[] = $this->optionMapper->findById($option->getId())->read(); } catch (IMapperException $e) { $this->logger->error("Failed to add option: {$e->getMessage()}"); // Optionally handle the error, e.g., by continuing to the next iteration or returning an error response diff --git a/lib/Db/Option.php b/lib/Db/Option.php index 87747530a..c36b04f76 100644 --- a/lib/Db/Option.php +++ b/lib/Db/Option.php @@ -31,7 +31,6 @@ class Option extends Entity { protected ?int $order; protected ?string $optionType; - public const OPTION_TYPE_CHOICE = 'choice'; public const OPTION_TYPE_ROW = 'row'; public const OPTION_TYPE_COLUMN = 'column'; diff --git a/lib/Migration/Version050300Date20260716000000.php b/lib/Migration/Version050300Date20260716000000.php new file mode 100644 index 000000000..a65b6957a --- /dev/null +++ b/lib/Migration/Version050300Date20260716000000.php @@ -0,0 +1,65 @@ +getTable('forms_v2_options'); + + if ($table->hasColumn('option_type')) { + $column = $table->getColumn('option_type'); + if ($column->getDefault() === null) { + $column->setDefault('choice'); + } + } + + return $schema; + } + + /** + * @param IOutput $output + * @param Closure(): ISchemaWrapper $schemaClosure + * @param array $options + */ + public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void { + $qbUpdate = $this->db->getQueryBuilder(); + + $qbUpdate->update('forms_v2_options') + ->set('option_type', $qbUpdate->createNamedParameter('choice')) + ->where($qbUpdate->expr()->isNull('option_type')) + ->executeStatement(); + } +} diff --git a/tests/Integration/Api/ApiV3Test.php b/tests/Integration/Api/ApiV3Test.php index 596cfc6dd..e869016b2 100644 --- a/tests/Integration/Api/ApiV3Test.php +++ b/tests/Integration/Api/ApiV3Test.php @@ -497,17 +497,17 @@ public static function dataGetFullForm() { [ 'text' => 'Option 1', 'order' => 1, - 'optionType' => null, + 'optionType' => 'choice', ], [ 'text' => 'Option 2', 'order' => 2, - 'optionType' => null, + 'optionType' => 'choice', ], [ 'text' => '', 'order' => 3, - 'optionType' => null, + 'optionType' => 'choice', ] ], 'accept' => [], From 99ea1cf5bae78de9491771966207ecc351a5ad13 Mon Sep 17 00:00:00 2001 From: Josef Behr Date: Thu, 16 Jul 2026 22:16:03 +0200 Subject: [PATCH 4/5] fix(api): apply review suggestions for option_type default - Set the newOption() signature default to 'choice' instead of resolving it in the method body, now that the migration guarantees stored options carry a type. - Drop the explicit-null guard and the post-insert re-read; they are no longer needed with the signature default in place. - Return early from the migration schema change when nothing is altered. - Document the 'choice' default on the optionType parameter. Signed-off-by: Josef Behr Co-Authored-By: Claude Opus 4.8 (1M context) --- lib/Controller/ApiController.php | 16 +++++----------- .../Version050300Date20260716000000.php | 4 +++- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index c2ec6676e..096cf5e45 100644 --- a/lib/Controller/ApiController.php +++ b/lib/Controller/ApiController.php @@ -894,7 +894,7 @@ public function reorderQuestions(int $formId, array $newOrder): DataResponse { * @param int $formId id of the form * @param int $questionId id of the question * @param list $optionTexts the new option text - * @param string|null $optionType the new option type (e.g. 'row') + * @param string|null $optionType the new option type (e.g. 'row'), defaults to 'choice' * @return DataResponse, array{}> Returns a DataResponse containing the added options * @throws OCSBadRequestException This question is not part ot the given form * @throws OCSForbiddenException This form is archived and can not be modified @@ -908,7 +908,7 @@ public function reorderQuestions(int $formId, array $newOrder): DataResponse { #[NoAdminRequired()] #[BruteForceProtection(action: 'form')] #[ApiRoute(verb: 'POST', url: '/api/v3/forms/{formId}/questions/{questionId}/options')] - public function newOption(int $formId, int $questionId, array $optionTexts, ?string $optionType = null): DataResponse { + public function newOption(int $formId, int $questionId, array $optionTexts, ?string $optionType = 'choice'): DataResponse { $this->logger->debug('Adding new options: formId: {formId}, questionId: {questionId}, text: {text}, optionType: {optionType}', [ 'formId' => $formId, 'questionId' => $questionId, @@ -954,18 +954,12 @@ public function newOption(int $formId, int $questionId, array $optionTexts, ?str $option->setQuestionId($questionId); $option->setText($text); $option->setOrder($optionOrder++); - // Only set the type when the caller provides one, so the 'choice' - // column default applies for normal option lists. Setting a null - // here would override that default and leave the option unrendered. - if ($optionType !== null) { - $option->setOptionType($optionType); - } + $option->setOptionType($optionType); try { $option = $this->optionMapper->insert($option); - // Re-read so the response reflects the column default that was - // applied when no type was set explicitly. - $addedOptions[] = $this->optionMapper->findById($option->getId())->read(); + // Add the stored option to the collection of added options + $addedOptions[] = $option->read(); } catch (IMapperException $e) { $this->logger->error("Failed to add option: {$e->getMessage()}"); // Optionally handle the error, e.g., by continuing to the next iteration or returning an error response diff --git a/lib/Migration/Version050300Date20260716000000.php b/lib/Migration/Version050300Date20260716000000.php index a65b6957a..c5becf402 100644 --- a/lib/Migration/Version050300Date20260716000000.php +++ b/lib/Migration/Version050300Date20260716000000.php @@ -38,15 +38,17 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt /** @var ISchemaWrapper $schema */ $schema = $schemaClosure(); $table = $schema->getTable('forms_v2_options'); + $changed = false; if ($table->hasColumn('option_type')) { $column = $table->getColumn('option_type'); if ($column->getDefault() === null) { $column->setDefault('choice'); + $changed = true; } } - return $schema; + return $changed ? $schema : null; } /** From bbbaae96fd09f9c01e7b71f57b8c1c60f047d4e8 Mon Sep 17 00:00:00 2001 From: Josef Behr Date: Thu, 16 Jul 2026 22:20:21 +0200 Subject: [PATCH 5/5] chore(openapi): regenerate for option_type default change Reflects the 'choice' default now declared on the newOption optionType parameter. The reorder-options parameter is unchanged. Signed-off-by: Josef Behr Co-Authored-By: Claude Opus 4.8 (1M context) --- openapi.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openapi.json b/openapi.json index 5bedb6c5d..f9eb1da32 100644 --- a/openapi.json +++ b/openapi.json @@ -2702,8 +2702,8 @@ "optionType": { "type": "string", "nullable": true, - "default": null, - "description": "the new option type (e.g. 'row')" + "default": "choice", + "description": "the new option type (e.g. 'row'), defaults to 'choice'" } } }