Skip to content

Commit 2994695

Browse files
josefbehrclaude
andauthored
fix(api): default option type to choice for non-grid questions (#3504)
* 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 <code@josef-behr.de> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * 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 <code@josef-behr.de> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * 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 <code@josef-behr.de> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * 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 <code@josef-behr.de> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * 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 <code@josef-behr.de> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Signed-off-by: Josef Behr <code@josef-behr.de> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 3a0eacb commit 2994695

4 files changed

Lines changed: 77 additions & 8 deletions

File tree

lib/Controller/ApiController.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -894,7 +894,7 @@ public function reorderQuestions(int $formId, array $newOrder): DataResponse {
894894
* @param int $formId id of the form
895895
* @param int $questionId id of the question
896896
* @param list<string> $optionTexts the new option text
897-
* @param string|null $optionType the new option type (e.g. 'row')
897+
* @param string|null $optionType the new option type (e.g. 'row'), defaults to 'choice'
898898
* @return DataResponse<Http::STATUS_CREATED, list<FormsOption>, array{}> Returns a DataResponse containing the added options
899899
* @throws OCSBadRequestException This question is not part ot the given form
900900
* @throws OCSForbiddenException This form is archived and can not be modified
@@ -908,7 +908,7 @@ public function reorderQuestions(int $formId, array $newOrder): DataResponse {
908908
#[NoAdminRequired()]
909909
#[BruteForceProtection(action: 'form')]
910910
#[ApiRoute(verb: 'POST', url: '/api/v3/forms/{formId}/questions/{questionId}/options')]
911-
public function newOption(int $formId, int $questionId, array $optionTexts, ?string $optionType = null): DataResponse {
911+
public function newOption(int $formId, int $questionId, array $optionTexts, ?string $optionType = 'choice'): DataResponse {
912912
$this->logger->debug('Adding new options: formId: {formId}, questionId: {questionId}, text: {text}, optionType: {optionType}', [
913913
'formId' => $formId,
914914
'questionId' => $questionId,
Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\Forms\Migration;
11+
12+
use Closure;
13+
use OCP\DB\ISchemaWrapper;
14+
use OCP\IDBConnection;
15+
use OCP\Migration\IOutput;
16+
use OCP\Migration\SimpleMigrationStep;
17+
18+
/**
19+
* Gives the option_type column a 'choice' default and backfills existing
20+
* rows that were stored without a type. Options created through the API
21+
* without an explicit optionType previously kept a null type, which the
22+
* frontend does not render.
23+
*/
24+
class Version050300Date20260716000000 extends SimpleMigrationStep {
25+
26+
public function __construct(
27+
protected IDBConnection $db,
28+
) {
29+
}
30+
31+
/**
32+
* @param IOutput $output
33+
* @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper`
34+
* @param array $options
35+
* @return null|ISchemaWrapper
36+
*/
37+
public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
38+
/** @var ISchemaWrapper $schema */
39+
$schema = $schemaClosure();
40+
$table = $schema->getTable('forms_v2_options');
41+
$changed = false;
42+
43+
if ($table->hasColumn('option_type')) {
44+
$column = $table->getColumn('option_type');
45+
if ($column->getDefault() === null) {
46+
$column->setDefault('choice');
47+
$changed = true;
48+
}
49+
}
50+
51+
return $changed ? $schema : null;
52+
}
53+
54+
/**
55+
* @param IOutput $output
56+
* @param Closure(): ISchemaWrapper $schemaClosure
57+
* @param array $options
58+
*/
59+
public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void {
60+
$qbUpdate = $this->db->getQueryBuilder();
61+
62+
$qbUpdate->update('forms_v2_options')
63+
->set('option_type', $qbUpdate->createNamedParameter('choice'))
64+
->where($qbUpdate->expr()->isNull('option_type'))
65+
->executeStatement();
66+
}
67+
}

openapi.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2702,8 +2702,8 @@
27022702
"optionType": {
27032703
"type": "string",
27042704
"nullable": true,
2705-
"default": null,
2706-
"description": "the new option type (e.g. 'row')"
2705+
"default": "choice",
2706+
"description": "the new option type (e.g. 'row'), defaults to 'choice'"
27072707
}
27082708
}
27092709
}

tests/Integration/Api/ApiV3Test.php

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -497,17 +497,17 @@ public static function dataGetFullForm() {
497497
[
498498
'text' => 'Option 1',
499499
'order' => 1,
500-
'optionType' => null,
500+
'optionType' => 'choice',
501501
],
502502
[
503503
'text' => 'Option 2',
504504
'order' => 2,
505-
'optionType' => null,
505+
'optionType' => 'choice',
506506
],
507507
[
508508
'text' => '',
509509
'order' => 3,
510-
'optionType' => null,
510+
'optionType' => 'choice',
511511
]
512512
],
513513
'accept' => [],
@@ -1027,7 +1027,9 @@ public static function dataCreateNewOption() {
10271027
// 'questionId' => Done dynamically below.
10281028
'text' => 'A new Option.',
10291029
'order' => 4,
1030-
'optionType' => null,
1030+
// Non-grid questions default to the 'choice' option type
1031+
// when none is provided.
1032+
'optionType' => 'choice',
10311033
]
10321034
]
10331035
];

0 commit comments

Comments
 (0)