diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index ebc411fd1..bcfb791c4 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,12 +589,19 @@ 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); $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/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, 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 によって変換されたページ (->オリジナル) /