From 2ea0955b59409afb3610cd6a533f222409fc45df Mon Sep 17 00:00:00 2001 From: global-prog Date: Wed, 9 Sep 2026 03:33:57 +0300 Subject: [PATCH 1/2] feat: copy questions from another form Rebuilding the same question set by hand for each new form is tedious, and the server already knew how to clone a question together with its options - it simply refused to do so across forms. That restriction is lifted for forms the user is allowed to EDIT. Simply dropping the same-form test would not have been safe: any question could then be read out of any form by guessing ids, so the source form is permission-checked. One bug had to be fixed for this to work at all: Question::read() carries the source question's formId, so a clone taken from another form would have been created back in that form rather than in the target one. The target id is now set explicitly. A dialog lists the forms the user can edit, then the questions of the chosen one, with select-all. Copies are made sequentially because each is appended at the end of the form, and issuing them in parallel would give an unpredictable resulting order. A partial failure reports that some questions were copied rather than implying none were. No schema change, and no change to the API surface: the existing fromId parameter and its documented behaviour are unchanged for same-form cloning. Signed-off-by: global-prog --- lib/Controller/ApiController.php | 12 +- src/components/ImportQuestionsDialog.vue | 332 +++++++++++++++++++++++ src/views/Create.vue | 41 +++ 3 files changed, 383 insertions(+), 2 deletions(-) create mode 100644 src/components/ImportQuestionsDialog.vue diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index ebc411fd1..47e7979df 100644 --- a/lib/Controller/ApiController.php +++ b/lib/Controller/ApiController.php @@ -570,9 +570,14 @@ public function newQuestion(int $formId, ?string $type = null, ?string $subtype try { $sourceQuestion = $this->questionMapper->findById($fromId); - // Only allow cloning questions that belong to the same form + // A question may be cloned from another form, but only from one the user is + // allowed to edit. Without that check any question could be read out of any + // form by guessing ids. if ($sourceQuestion->getFormId() !== $formId) { - throw new OCSBadRequestException('Question doesn\'t belong to given form'); + $this->formsService->getFormIfAllowed( + $sourceQuestion->getFormId(), + Constants::PERMISSION_EDIT, + ); } $sourceOptions = $this->optionMapper->findByQuestion($fromId); } catch (IMapperException) { @@ -584,6 +589,9 @@ public function newQuestion(int $formId, ?string $type = null, ?string $subtype $questionData = $sourceQuestion->read(); unset($questionData['id']); + // read() carries the source question's formId, so a clone taken from another + // form would otherwise be created back in that form rather than this one. + $questionData['formId'] = $formId; if ($position !== null) { $position = $this->shiftQuestionsForInsert($allQuestions, $position); diff --git a/src/components/ImportQuestionsDialog.vue b/src/components/ImportQuestionsDialog.vue new file mode 100644 index 000000000..c6201eb58 --- /dev/null +++ b/src/components/ImportQuestionsDialog.vue @@ -0,0 +1,332 @@ + + + + + + + diff --git a/src/views/Create.vue b/src/views/Create.vue index 3bdd25f39..5c7d32335 100644 --- a/src/views/Create.vue +++ b/src/views/Create.vue @@ -207,7 +207,21 @@ :hasSubtypes="hasSubtypes" primary @addQuestion="addQuestion" /> + + + {{ t('forms', 'Import questions') }} + + @@ -217,6 +231,7 @@ import type { ComponentPublicInstance, PropType } from 'vue' import type { FormsForm, FormsOption, FormsQuestion } from '../types/Entities.d.ts' +import IconImport from '@material-symbols/svg-400/outlined/library_add.svg?raw' import IconLock from '@material-symbols/svg-400/outlined/lock.svg?raw' import axios from '@nextcloud/axios' import { showError } from '@nextcloud/dialogs' @@ -230,11 +245,13 @@ import debounce from 'debounce' import { computed, defineComponent, nextTick, onMounted, ref, watch } from 'vue' import { VueDraggable as Draggable } from 'vue-draggable-plus' import NcAppContent from '@nextcloud/vue/components/NcAppContent' +import NcButton from '@nextcloud/vue/components/NcButton' import NcEmptyContent from '@nextcloud/vue/components/NcEmptyContent' import NcIconSvgWrapper from '@nextcloud/vue/components/NcIconSvgWrapper' import NcLoadingIcon from '@nextcloud/vue/components/NcLoadingIcon' import NcNoteCard from '@nextcloud/vue/components/NcNoteCard' import AddQuestionMenu from '../components/AddQuestionMenu.vue' +import ImportQuestionsDialog from '../components/ImportQuestionsDialog.vue' import Question from '../components/Questions/Question.vue' import QuestionLong from '../components/Questions/QuestionLong.vue' import QuestionMultiple from '../components/Questions/QuestionMultiple.vue' @@ -260,9 +277,11 @@ export default defineComponent({ name: 'Create', components: { Draggable, + ImportQuestionsDialog, NcIconSvgWrapper, AddQuestionMenu, NcAppContent, + NcButton, NcEmptyContent, NcLoadingIcon, NcNoteCard, @@ -631,6 +650,25 @@ export default defineComponent({ * @param subtype the question subtype, see AnswerTypes.subtypes * @param position where the new question should be added */ + const showImportDialog = ref(false) + + /** + * Append questions copied from another form. + * + * The server has already created them, so this only reflects them in the open + * editor rather than refetching the whole form. + * + * @param created the questions the server returned + */ + const onQuestionsImported = (created: FormsQuestion[]): void => { + const questions = [ + ...props.form.questions, + ...created.map((question) => ({ ...question, answers: [] })), + ] + emit('update:form', { ...props.form, questions }) + emitEvent('forms:last-updated:set', props.form.id) + } + const addQuestion = async ( type: string, subtype: string | null = null, @@ -828,6 +866,9 @@ export default defineComponent({ resizeTitle, resizeDescription, addQuestion, + showImportDialog, + onQuestionsImported, + IconImport, deleteQuestion, insertQuestion, cloneQuestion, From 5cc43a7e5684d9db34d9d5e13ff5ac207f6b340c Mon Sep 17 00:00:00 2001 From: global-prog Date: 2026年9月11日 17:51:16 +0300 Subject: [PATCH 2/2] fix: copying a question into an empty form failed Copying appends at the end by reading the order of the target form's last question, and end() of an empty list is false rather than a question, so the call on it was fatal. That could not happen while copying was limited to the same form, which always held at least the source question. Now that the source may be another form, copying into a new form with nothing in it yet -- the ordinary case -- failed with a server error. It now starts at 1, as adding a new question to an empty form already does. Adds tests for copying from another form, into an empty form, and from a form the user cannot edit, which also covers the permission check the first commit added. Signed-off-by: global-prog --- lib/Controller/ApiController.php | 6 +- tests/Unit/Controller/ApiControllerTest.php | 95 +++++++++++++++++++++ 2 files changed, 100 insertions(+), 1 deletion(-) diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index 47e7979df..bcfb791c4 100644 --- a/lib/Controller/ApiController.php +++ b/lib/Controller/ApiController.php @@ -597,7 +597,11 @@ public function newQuestion(int $formId, ?string $type = null, ?string $subtype $position = $this->shiftQuestionsForInsert($allQuestions, $position); $questionData['order'] = $position; } else { - $questionData['order'] = end($allQuestions)->getOrder() + 1; + // Append at the end. The target form may have no questions yet -- the usual + // case when copying into a new form -- and end() of an empty list is false, + // not a question. + $lastQuestion = end($allQuestions); + $questionData['order'] = $lastQuestion ? $lastQuestion->getOrder() + 1 : 1; } $newQuestion = Question::fromParams($questionData); diff --git a/tests/Unit/Controller/ApiControllerTest.php b/tests/Unit/Controller/ApiControllerTest.php index dbd0952cd..ad1b93af5 100644 --- a/tests/Unit/Controller/ApiControllerTest.php +++ b/tests/Unit/Controller/ApiControllerTest.php @@ -1375,6 +1375,101 @@ public function testCloneFormWithConfirmationEmailQuestionId(): void { $this->assertEquals(11, $clonedForm->getConfirmationEmailQuestionId()); } + /** + * @param int $formId the form a copied question comes from + * @return Question the source question, with one text to recognise it by + */ + private function sourceQuestionInForm(int $formId): Question { + return Question::fromParams([ + 'id' => 10, + 'formId' => $formId, + 'order' => 1, + 'type' => 'short', + 'text' => 'Source question', + 'description' => '', + 'isRequired' => false, + ]); + } + + public function testCloneQuestionFromAnotherForm(): void { + $targetForm = Form::fromParams(['id' => 1, 'ownerId' => 'currentUser']); + $sourceForm = Form::fromParams(['id' => 2, 'ownerId' => 'currentUser']); + + // Editing rights are checked on both forms: the one being added to, and the one + // the question is read out of. + $this->formsService->expects($this->exactly(2)) + ->method('getFormIfAllowed') + ->willReturnCallback(fn (int $id, string $permission) => match ([$id, $permission]) { + [1, Constants::PERMISSION_EDIT] => $targetForm, + [2, Constants::PERMISSION_EDIT] => $sourceForm, + }); + + $this->questionMapper->method('findById')->with(10)->willReturn($this->sourceQuestionInForm(2)); + $this->optionMapper->method('findByQuestion')->with(10)->willReturn([]); + $this->questionMapper->method('findByForm')->with(1)->willReturn([ + Question::fromParams(['id' => 20, 'formId' => 1, 'order' => 3, 'type' => 'short']), + ]); + + $inserted = null; + $this->questionMapper->expects($this->once()) + ->method('insert') + ->with($this->callback(function (Question $question) use (&$inserted) { + $inserted = $question; + return true; + })); + + $this->apiController->newQuestion(1, fromId: 10); + + // Created in the form it was copied into, not back in the one it came from. + $this->assertEquals(1, $inserted->getFormId()); + $this->assertEquals(4, $inserted->getOrder()); + $this->assertEquals('Source question', $inserted->getText()); + } + + public function testCloneQuestionIntoEmptyForm(): void { + $targetForm = Form::fromParams(['id' => 1, 'ownerId' => 'currentUser']); + $sourceForm = Form::fromParams(['id' => 2, 'ownerId' => 'currentUser']); + $this->formsService->method('getFormIfAllowed') + ->willReturnCallback(fn (int $id) => $id === 1 ? $targetForm : $sourceForm); + + $this->questionMapper->method('findById')->with(10)->willReturn($this->sourceQuestionInForm(2)); + $this->optionMapper->method('findByQuestion')->with(10)->willReturn([]); + // A new form with nothing in it yet: the usual target when copying questions over. + $this->questionMapper->method('findByForm')->with(1)->willReturn([]); + + $inserted = null; + $this->questionMapper->expects($this->once()) + ->method('insert') + ->with($this->callback(function (Question $question) use (&$inserted) { + $inserted = $question; + return true; + })); + + $this->apiController->newQuestion(1, fromId: 10); + + $this->assertEquals(1, $inserted->getFormId()); + $this->assertEquals(1, $inserted->getOrder()); + } + + public function testCloneQuestionFromFormWithoutEditRights(): void { + $targetForm = Form::fromParams(['id' => 1, 'ownerId' => 'currentUser']); + $this->formsService->method('getFormIfAllowed') + ->willReturnCallback(function (int $id) use ($targetForm) { + if ($id === 1) { + return $targetForm; + } + throw new NoSuchFormException('User has no permissions to get this form'); + }); + + $this->questionMapper->method('findById')->with(10)->willReturn($this->sourceQuestionInForm(2)); + // Nothing may be read out of a form the user cannot edit, let alone copied. + $this->optionMapper->expects($this->never())->method('findByQuestion'); + $this->questionMapper->expects($this->never())->method('insert'); + + $this->expectException(NoSuchFormException::class); + $this->apiController->newQuestion(1, fromId: 10); + } + public function testTransferOwnerNotOwner() { $form = new Form(); $form->setId(1);

AltStyle によって変換されたページ (->オリジナル) /