From 7f28f36ae9b869463d455062fcf629b6e764abc5 Mon Sep 17 00:00:00 2001 From: Andrii Ilkiv Date: Sat, 19 Aug 2023 19:28:25 +0300 Subject: [PATCH] Added a new feature: 'Other' option for radio and checkbox questions. Signed-off-by: Andrii Ilkiv --- lib/Constants.php | 2 + lib/Controller/ApiController.php | 21 +- lib/Service/SubmissionService.php | 5 +- src/components/Questions/QuestionMultiple.vue | 81 ++++++-- src/components/Results/ResultsSummary.vue | 13 +- tests/Unit/Controller/ApiControllerTest.php | 179 ++++++++++++++++++ tests/Unit/Service/SubmissionServiceTest.php | 33 +++- 7 files changed, 303 insertions(+), 31 deletions(-) diff --git a/lib/Constants.php b/lib/Constants.php index 60af263ff..2fb127086 100644 --- a/lib/Constants.php +++ b/lib/Constants.php @@ -131,4 +131,6 @@ class Constants { */ public const EMPTY_NOTFOUND = 'notfound'; public const EMPTY_EXPIRED = 'expired'; + + public const PREFIX_FOR_OTHER_ANSWER = 'system-other-answer:'; } diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index 673b2a0a5..3d1a567a1 100644 --- a/lib/Controller/ApiController.php +++ b/lib/Controller/ApiController.php @@ -1002,27 +1002,30 @@ public function insertSubmission(int $formId, array $answers, string $shareHash $questionIndex = array_search($questionId, array_column($questions, 'id')); if ($questionIndex === false) { continue; - } else { - $question = $questions[$questionIndex]; } + + $question = $questions[$questionIndex]; foreach ($answerArray as $answer) { + $answerText = ''; + // Are we using answer ids as values if (in_array($question['type'], Constants::ANSWER_TYPES_PREDEFINED)) { // Search corresponding option, skip processing if not found $optionIndex = array_search($answer, array_column($question['options'], 'id')); - if ($optionIndex === false) { - continue; - } else { - $option = $question['options'][$optionIndex]; + if ($optionIndex !== false) { + $answerText = $question['options'][$optionIndex]['text']; + } elseif (!empty($question['extraSettings']->otherAnswer) && strpos($answer, Constants::PREFIX_FOR_OTHER_ANSWER) === 0) { + $answerText = str_replace(Constants::PREFIX_FOR_OTHER_ANSWER, "", $answer); } - - // Load option-text - $answerText = $option['text']; } else { $answerText = $answer; // Not a multiple-question, answerText is given answer } + if ($answerText === "") { + continue; + } + $answerEntity = new Answer(); $answerEntity->setSubmissionId($submissionId); $answerEntity->setQuestionId($question['id']); diff --git a/lib/Service/SubmissionService.php b/lib/Service/SubmissionService.php index 5a4980b55..6de92e33f 100644 --- a/lib/Service/SubmissionService.php +++ b/lib/Service/SubmissionService.php @@ -309,7 +309,8 @@ public function validateSubmission(array $questions, array $answers): bool { $questionAnswered = array_key_exists($questionId, $answers); // Check if all required questions have an answer - if ($question['isRequired'] && (!$questionAnswered || !array_filter($answers[$questionId], 'strlen'))) { + if ($question['isRequired'] && + (!$questionAnswered || !array_filter($answers[$questionId], 'strlen') || !array_filter($answers[$questionId], fn ($value) => $value !== Constants::PREFIX_FOR_OTHER_ANSWER))) { return false; } @@ -330,7 +331,7 @@ public function validateSubmission(array $questions, array $answers): bool { } // Check if all answers are within the possible options - if (in_array($question['type'], Constants::ANSWER_TYPES_PREDEFINED)) { + if (in_array($question['type'], Constants::ANSWER_TYPES_PREDEFINED) && empty($question['extraSettings']->otherAnswer)) { foreach ($answers[$questionId] as $answer) { // Search corresponding option, return false if non-existent if (array_search($answer, array_column($question['options'], 'id')) === false) { diff --git a/src/components/Questions/QuestionMultiple.vue b/src/components/Questions/QuestionMultiple.vue index be7903533..ac52e6369 100644 --- a/src/components/Questions/QuestionMultiple.vue +++ b/src/components/Questions/QuestionMultiple.vue @@ -43,6 +43,10 @@ @update:checked="onShuffleOptionsChange"> {{ t('forms', 'Shuffle options') }} + + {{ t('forms', 'Input for "other"') }} + @@ -76,7 +90,16 @@ @delete="deleteOption" @update:answer="updateAnswer" /> - +
  • +
    + +
  • { - arrOfNum.push(Number(str)) - }) - this.$emit('update:values', arrOfNum) + inputOtherAnswer() { + if (this.isUnique) { + this.questionValues = this.valueOtherAnswer + this.onChange() return } - // Radio: create array - this.$emit('update:values', [this.questionValues]) + this.questionValues = this.questionValues.filter(item => !item.startsWith(this.PREFIX_FOR_OTHER_ANSWER)) + + if (this.inputOtherAnswer !== '') { + this.questionValues.push(this.valueOtherAnswer) + } + + this.onChange() + }, + }, + + methods: { + onChange() { + this.$emit('update:values', this.isUnique ? [this.questionValues] : this.questionValues) }, /** @@ -348,6 +387,20 @@ export default { input.focus() } }, + + /** + * Update status extra setting otherAnswer and save on DB + * + * @param {boolean} showInputOtherAnswer show/hide field for other answer + */ + onOtherAnswerChange(showInputOtherAnswer) { + return this.onExtraSettingsChange('otherAnswer', showInputOtherAnswer) + }, + + valueToInputOtherAnswer() { + const otherAnswer = this.values.filter(item => item.startsWith(this.PREFIX_FOR_OTHER_ANSWER)) + return otherAnswer[0] !== undefined ? otherAnswer[0].substring(this.PREFIX_FOR_OTHER_ANSWER.length) : '' + }, }, } diff --git a/src/components/Results/ResultsSummary.vue b/src/components/Results/ResultsSummary.vue index fc1279c04..dbe1af846 100644 --- a/src/components/Results/ResultsSummary.vue +++ b/src/components/Results/ResultsSummary.vue @@ -92,6 +92,13 @@ export default { percentage: 0, })) + // Also record 'Other' + questionOptionsStats.unshift({ + text: t('forms', 'Other'), + count: 0, + percentage: 0, + }) + // Also record 'No response' questionOptionsStats.unshift({ // TRANSLATORS Counts on Results-Summary, how many users did not respond to this question. @@ -112,11 +119,7 @@ export default { answers.forEach(answer => { const optionsStatIndex = questionOptionsStats.findIndex(option => option.text === answer.text) if (optionsStatIndex < 0) { - questionOptionsStats.push({ - text: answer.text, - count: 1, - percentage: 0, - }) + questionOptionsStats[1].count++ } else { questionOptionsStats[optionsStatIndex].count++ } diff --git a/tests/Unit/Controller/ApiControllerTest.php b/tests/Unit/Controller/ApiControllerTest.php index 01fd9c00b..e0eb4a821 100644 --- a/tests/Unit/Controller/ApiControllerTest.php +++ b/tests/Unit/Controller/ApiControllerTest.php @@ -67,6 +67,7 @@ function time($expected = null) { use OCP\IUser; use OCP\IUserManager; use OCP\IUserSession; +use OCA\Forms\Constants; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; @@ -490,6 +491,7 @@ public function dataCloneForm() { ] ]; } + /** * @dataProvider dataCloneForm() */ @@ -553,4 +555,181 @@ public function testCloneForm($old, $new) { ->willReturn(new DataResponse('success')); $this->assertEquals(new DataResponse('success'), $apiController->cloneForm(7)); } + + public function testInsertSubmission_formNotFound() { + $exception = $this->createMock(MapperException::class); + $this->formMapper->expects($this->once()) + ->method('findById') + ->with(1) + ->willThrowException($exception); + $this->expectException(OCSBadRequestException::class); + $this->apiController->insertSubmission(1, [], ''); + } + + public function dataForCheckForbiddenException() + { + return [ + [false, true, true], + [true, true, true], + [true, false, false], + ]; + } + + /** + * @dataProvider dataForCheckForbiddenException() + */ + public function testInsertSubmission_forbiddenException($hasUserAccess, $hasFormExpired, $canSubmit) { + $form = new Form(); + $form->setId(1); + + $this->formMapper->expects($this->once()) + ->method('findById') + ->with(1) + ->willReturn($form); + + $this->formAccess($hasUserAccess, $hasFormExpired, $canSubmit); + + $this->expectException(OCSForbiddenException::class); + + $this->apiController->insertSubmission(1, [], ''); + } + + public function testInsertSubmission_validateSubmission() { + $form = new Form(); + $form->setId(1); + + $this->formMapper->expects($this->once()) + ->method('findById') + ->with(1) + ->willReturn($form); + + $this->formsService->expects($this->once()) + ->method('getQuestions') + ->with(1) + ->willReturn([]); + + $this->formAccess(); + + $this->submissionService + ->method('validateSubmission') + ->willReturn(false); + + $this->expectException(OCSBadRequestException::class); + + $this->apiController->insertSubmission(1, [], ''); + } + + private function formAccess(bool $hasUserAccess = true, bool $hasFormExpired = false, bool $canSubmit = true) { + $this->formsService + ->method('hasUserAccess') + ->with(1) + ->willReturn($hasUserAccess); + + $this->formsService + ->method('hasFormExpired') + ->with(1) + ->willReturn($hasFormExpired); + + $this->formsService + ->method('canSubmit') + ->with(1) + ->willReturn($canSubmit); + } + + public function testInsertSubmission_answers() { + $form = new Form(); + $form->setId(1); + + $questions = [ + [ + 'id' => 2, + 'type' => Constants::ANSWER_TYPE_MULTIPLE, + 'extraSettings' => (object)['otherAnswer' => true], + 'options' => [ + ['id' => 1, 'text' => 'test id 1'], + ['id' => 2, 'text' => 'test id 2'], + ], + ], + [ + 'id' => 3, + 'type' => Constants::ANSWER_TYPE_SHORT, + ], + [ + 'id' => 1, + 'type' => Constants::ANSWER_TYPE_MULTIPLE, + 'options' => [ + ['id' => 3, 'text' => 'test id 3'], + ['id' => 4, 'text' => 'test id 4'], + ], + ], + ]; + + $answers = [ + 1 => ['3'], + 2 => ['2', '5', Constants::PREFIX_FOR_OTHER_ANSWER . 'other answer'], + 3 => ['short anwer'], + 4 => ['ignore unknown question'], + ]; + + $this->formMapper->expects($this->once()) + ->method('findById') + ->with(1) + ->willReturn($form); + + $this->formsService->expects($this->once()) + ->method('getQuestions') + ->with(1) + ->willReturn($questions); + + $this->formAccess(); + + $this->submissionService + ->method('validateSubmission') + ->willReturn(true); + + $this->submissionMapper->expects($this->once()) + ->method('insert') + ->with($this->callback(function ($submission) { + $submission->setId(12); + return true; + })); + + $this->answerMapper->expects($this->exactly(4)) + ->method('insert') + ->with($this->callback(function ($answer) { + if ($answer->getSubmissionId() !== 12) { + return false; + } + + switch ($answer->getQuestionId()) { + case 1: + if ($answer->getText() !== 'test id 3') { + return false; + } + break; + case 2: + if (!in_array($answer->getText(), ['other answer', 'test id 2'])) { + return false; + } + break; + case 3: + if ($answer->getText() !== 'short anwer') { + return false; + } + break; + } + + return true; + })); + + $this->formsService->expects($this->once()) + ->method('setLastUpdatedTimestamp') + ->with(1); + + $this->activityManager->expects($this->once()) + ->method('publishNewSubmission') + ->with($form, 'currentUser'); + + $this->apiController->insertSubmission(1, $answers, ''); + } } diff --git a/tests/Unit/Service/SubmissionServiceTest.php b/tests/Unit/Service/SubmissionServiceTest.php index ccfe348a2..65ce69ac3 100644 --- a/tests/Unit/Service/SubmissionServiceTest.php +++ b/tests/Unit/Service/SubmissionServiceTest.php @@ -561,6 +561,18 @@ public function dataValidateSubmission() { // Expected Result false ], + 'required-emty-other-answer' => [ + // Questions + [ + ['id' => 1, 'type' => 'multiple_unique', 'isRequired' => true] + ], + // Answers + [ + '1' => ['system-other-answer:'] + ], + // Expected Result + false + ], 'more-than-allowed' => [ // Questions [ @@ -591,6 +603,21 @@ public function dataValidateSubmission() { // Expected Result false ], + 'other-answer-not-allowed' => [ + // Questions + [ + ['id' => 1, 'type' => 'multiple', 'isRequired' => false, 'options' => [ + ['id' => 3], + ['id' => 5] + ]], + ], + // Answers + [ + '1' => [3,'system-other-answer:other answer'] + ], + // Expected Result + false + ], 'question-not-known' => [ // Questions [ @@ -635,6 +662,9 @@ public function dataValidateSubmission() { ['id' => 6] ]], ['id' => 8, 'type' => 'time', 'isRequired' => false], + ['id' => 9, 'type' => 'multiple_unique', 'isRequired' => true, 'extraSettings' => (object)['otherAnswer' => true], 'options' => [ + ['id' => 3] + ]], ], // Answers [ @@ -645,7 +675,8 @@ public function dataValidateSubmission() { '5' => [1,2], '6' => [4], '7' => [5], - '8' => ['17:45'] + '8' => ['17:45'], + '9' => ['system-other-answer:other answer'] ], // Expected Result true