From ceac82fdeef143906dcf1fe9590ddc42c9b1b6ac Mon Sep 17 00:00:00 2001 From: Andrii Ilkiv Date: Sat, 19 Aug 2023 19:28:25 +0300 Subject: [PATCH] feat: Add 'Other' option for radio/checkbox questions. Signed-off-by: Andrii Ilkiv --- docs/API.md | 13 +- docs/DataStructure.md | 24 ++-- lib/Constants.php | 5 + lib/Controller/ApiController.php | 21 +-- lib/Service/SubmissionService.php | 10 +- src/components/Questions/QuestionMultiple.vue | 122 ++++++++++++++++-- src/components/Results/ResultsSummary.vue | 23 +++- src/models/AnswerTypes.js | 4 + tests/Unit/Controller/ApiControllerTest.php | 116 +++++++++++++++++ tests/Unit/Service/SubmissionServiceTest.php | 37 +++++- 10 files changed, 327 insertions(+), 48 deletions(-) diff --git a/docs/API.md b/docs/API.md index 3aa01d15d..2a9b60c8b 100644 --- a/docs/API.md +++ b/docs/API.md @@ -152,7 +152,8 @@ Returns the full-depth object of the requested form (without submissions). "questionId": 1, "text": "Option 2" } - ] + ], + "extraSettings": {} }, { "id": 2, @@ -162,7 +163,8 @@ Returns the full-depth object of the requested form (without submissions). "isRequired": true, "text": "Question 2", "name": "something_other", - "options": [] + "options": [], + "extraSettings": {} } ], "shares": [ @@ -247,6 +249,7 @@ Contains only manipulative question-endpoints. To retrieve questions, request th "name": "", "text": "", "options": [] + "extraSettings": {} } ``` @@ -478,7 +481,8 @@ Get all Submissions to a Form "questionId": 1, "text": "Option 3" } - ] + ], + "extraSettings": {} }, { "id": 2, @@ -487,7 +491,8 @@ Get all Submissions to a Form "type": "short", "isRequired": true, "text": "Question 2", - "options": [] + "options": [], + "extraSettings": {} } ] } diff --git a/docs/DataStructure.md b/docs/DataStructure.md index 6e7ace76e..6cb8719d3 100644 --- a/docs/DataStructure.md +++ b/docs/DataStructure.md @@ -50,16 +50,17 @@ This document describes the Object-Structure, that is used within the Forms App ``` ### Question -| Property | Type | Restrictions | Description | -|-------------|-----------------|--------------|-------------| -| id | Integer | unique | An instance-wide unique id of the question | -| formId | Integer | | The id of the form, the question belongs to | -| order | Integer | unique within form; *not* `0` | The order of the question within that form. Value `0` indicates deleted questions within database (typ. not visible outside) | -| type | [Question-Type](#question-types) | | Type of the question | -| isRequired | Boolean | | If the question is required to fill the form | -| text | String | max. 2048 ch. | The question-text | -| name | String | | Technical identifier of the question, e.g. used as HTML name attribute | -| options | Array of [Options](#option) | | Array of options belonging to the question. Only relevant for question-type with predefined options. | +| Property | Type | Restrictions | Description | +|----------------|-----------------|--------------|-------------| +| id | Integer | unique | An instance-wide unique id of the question | +| formId | Integer | | The id of the form, the question belongs to | +| order | Integer | unique within form; *not* `0` | The order of the question within that form. Value `0` indicates deleted questions within database (typ. not visible outside) | +| type | [Question-Type](#question-types) | | Type of the question | +| isRequired | Boolean | | If the question is required to fill the form | +| text | String | max. 2048 ch. | The question-text | +| name | String | | Technical identifier of the question, e.g. used as HTML name attribute | +| options | Array of [Options](#option) | | Array of options belonging to the question. Only relevant for question-type with predefined options. | +| extraSettings | StdClass | | Additional settings for the question. For dropdown/radio/checkbox, there can be a 'shuffleOptions': true - the list of options should be shuffled. For radio/checkbox, 'allowOtherAnswer': true - allows the user to specify their own option. | ``` { "id": 1, @@ -69,7 +70,8 @@ This document describes the Object-Structure, that is used within the Forms App "isRequired": false, "text": "Question 1", "name": "firstname", - "options": [] + "options": [], + "extraSettings": {} } ``` diff --git a/lib/Constants.php b/lib/Constants.php index 60af263ff..62cb0b064 100644 --- a/lib/Constants.php +++ b/lib/Constants.php @@ -131,4 +131,9 @@ class Constants { */ public const EMPTY_NOTFOUND = 'notfound'; public const EMPTY_EXPIRED = 'expired'; + + /** + * Constants related to extra settings for questions + */ + public const QUESTION_EXTRASETTINGS_OTHER_PREFIX = 'system-other-answer:'; } diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index 673b2a0a5..03bc9bf0c 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']->allowOtherAnswer) && strpos($answer, Constants::QUESTION_EXTRASETTINGS_OTHER_PREFIX) === 0) { + $answerText = str_replace(Constants::QUESTION_EXTRASETTINGS_OTHER_PREFIX, "", $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..04bc0875e 100644 --- a/lib/Service/SubmissionService.php +++ b/lib/Service/SubmissionService.php @@ -309,7 +309,11 @@ 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') || + (!empty($question['extraSettings']->allowOtherAnswer) && !array_filter($answers[$questionId], fn ($value) => $value !== Constants::QUESTION_EXTRASETTINGS_OTHER_PREFIX))) + ) { return false; } @@ -328,9 +332,9 @@ public function validateSubmission(array $questions, array $answers): bool { !$this->validateDateTime($answers[$questionId][0], Constants::ANSWER_PHPDATETIME_FORMAT[$question['type']])) { return false; } - + // 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']->allowOtherAnswer)) { 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..392fdfd72 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', 'Add "other"') }} + @@ -76,7 +96,15 @@ @delete="deleteOption" @update:answer="updateAnswer" /> - +
  • +
    + +
  • item.startsWith(this.QUESTION_EXTRASETTINGS_OTHER_PREFIX)) + return checkedOtherAnswer[0] !== undefined + }, }, watch: { @@ -178,23 +230,27 @@ export default { this.updateOptions(options) } }, - }, - - methods: { - onChange() { - // Checkbox: convert to array of Numbers - if (!this.isUnique) { - const arrOfNum = [] - this.questionValues.forEach(str => { - 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.QUESTION_EXTRASETTINGS_OTHER_PREFIX)) + + if (this.inputOtherAnswer !== '') { + this.questionValues.push(this.valueOtherAnswer) + } + + this.onChange() + }, + }, + + methods: { + onChange() { + this.$emit('update:values', this.isUnique ? [this.questionValues] : this.questionValues) }, /** @@ -348,6 +404,20 @@ export default { input.focus() } }, + + /** + * Update status extra setting allowOtherAnswer and save on DB + * + * @param {boolean} allowOtherAnswer show/hide field for other answer + */ + onAllowOtherAnswerChange(allowOtherAnswer) { + return this.onExtraSettingsChange('allowOtherAnswer', allowOtherAnswer) + }, + + valueToInputOtherAnswer() { + const otherAnswer = this.values.filter(item => item.startsWith(this.QUESTION_EXTRASETTINGS_OTHER_PREFIX)) + return otherAnswer[0] !== undefined ? otherAnswer[0].substring(this.QUESTION_EXTRASETTINGS_OTHER_PREFIX.length) : '' + }, }, } @@ -402,4 +472,28 @@ export default { } } } + +.question__other-answer { + display: flex; + gap: 4px 24px; + flex-wrap: wrap; + + .question__label { + flex-basis: content; + } + + .question__input { + flex: 1; + min-width: 260px; + } + + .input-field__input { + min-height: 44px; + } +} + +.question__other-answer:deep() .input-field__input { + min-height: 44px; +} + diff --git a/src/components/Results/ResultsSummary.vue b/src/components/Results/ResultsSummary.vue index fc1279c04..7889fe3ca 100644 --- a/src/components/Results/ResultsSummary.vue +++ b/src/components/Results/ResultsSummary.vue @@ -92,6 +92,15 @@ export default { percentage: 0, })) + // Also record 'Other' + if (this.question.extraSettings?.allowOtherAnswer) { + 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 +121,15 @@ 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, - }) + if (this.question.extraSettings?.allowOtherAnswer) { + questionOptionsStats[1].count++ + } else { + questionOptionsStats.push({ + text: answer.text, + count: 1, + percentage: 0, + }) + } } else { questionOptionsStats[optionsStatIndex].count++ } diff --git a/src/models/AnswerTypes.js b/src/models/AnswerTypes.js index aaab71ff5..f1f8d4643 100644 --- a/src/models/AnswerTypes.js +++ b/src/models/AnswerTypes.js @@ -70,6 +70,8 @@ export default { validate: question => question.options.length > 0, titlePlaceholder: t('forms', 'Checkbox question title'), + createPlaceholder: t('forms', 'People can submit a different answer'), + submitPlaceholder: t('forms', 'Enter your answer'), warningInvalid: t('forms', 'This question needs a title and at least one answer!'), }, @@ -81,6 +83,8 @@ export default { validate: question => question.options.length > 0, titlePlaceholder: t('forms', 'Radio buttons question title'), + createPlaceholder: t('forms', 'People can submit a different answer'), + submitPlaceholder: t('forms', 'Enter your answer'), warningInvalid: t('forms', 'This question needs a title and at least one answer!'), // Using the same vue-component as multiple, this specifies that the component renders as multiple_unique. diff --git a/tests/Unit/Controller/ApiControllerTest.php b/tests/Unit/Controller/ApiControllerTest.php index 01fd9c00b..8bcaa7376 100644 --- a/tests/Unit/Controller/ApiControllerTest.php +++ b/tests/Unit/Controller/ApiControllerTest.php @@ -46,6 +46,7 @@ function time($expected = null) { namespace OCA\Forms\Tests\Unit\Controller; use OCA\Forms\Activity\ActivityManager; +use OCA\Forms\Constants; use OCA\Forms\Controller\ApiController; use OCA\Forms\Db\AnswerMapper; use OCA\Forms\Db\Form; @@ -490,6 +491,7 @@ public function dataCloneForm() { ] ]; } + /** * @dataProvider dataCloneForm() */ @@ -553,4 +555,118 @@ public function testCloneForm($old, $new) { ->willReturn(new DataResponse('success')); $this->assertEquals(new DataResponse('success'), $apiController->cloneForm(7)); } + + 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)['allowOtherAnswer' => 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::QUESTION_EXTRASETTINGS_OTHER_PREFIX . '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..d946f64de 100644 --- a/tests/Unit/Service/SubmissionServiceTest.php +++ b/tests/Unit/Service/SubmissionServiceTest.php @@ -24,8 +24,8 @@ */ namespace OCA\Forms\Tests\Unit\Service; +use OCA\Forms\Constants; use OCA\Forms\Db\Answer; - use OCA\Forms\Db\AnswerMapper; use OCA\Forms\Db\Form; use OCA\Forms\Db\FormMapper; @@ -561,6 +561,20 @@ public function dataValidateSubmission() { // Expected Result false ], + 'required-empty-other-answer' => [ + // Questions + [ + ['id' => 1, 'type' => 'multiple_unique', 'isRequired' => true, 'extraSettings' => (object)['allowOtherAnswer' => true], 'options' => [ + ['id' => 3] + ]] + ], + // Answers + [ + '1' => [Constants::QUESTION_EXTRASETTINGS_OTHER_PREFIX] + ], + // Expected Result + false + ], 'more-than-allowed' => [ // Questions [ @@ -591,6 +605,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, Constants::QUESTION_EXTRASETTINGS_OTHER_PREFIX . 'other answer'] + ], + // Expected Result + false + ], 'question-not-known' => [ // Questions [ @@ -635,6 +664,9 @@ public function dataValidateSubmission() { ['id' => 6] ]], ['id' => 8, 'type' => 'time', 'isRequired' => false], + ['id' => 9, 'type' => 'multiple_unique', 'isRequired' => true, 'extraSettings' => (object)['allowOtherAnswer' => true], 'options' => [ + ['id' => 3] + ]], ], // Answers [ @@ -645,7 +677,8 @@ public function dataValidateSubmission() { '5' => [1,2], '6' => [4], '7' => [5], - '8' => ['17:45'] + '8' => ['17:45'], + '9' => [Constants::QUESTION_EXTRASETTINGS_OTHER_PREFIX . 'other answer'] ], // Expected Result true