Add question types to surveys by beagleknight · Pull Request #1398 · decidim/decidim

beagleknight · 2017-05-24T09:56:15Z

🎩 What? Why?

This adds question types to survey questions:

Short answer: Users fill in the answer in a simple text input
Long answer: Users fill in the answer in a textarea
Single option: Users must choose a single answer of the given ones.
Multiple option: Users can check as many answers as they want of the given ones.

In this PR I also included a lot of refactors for the JS code. I built a mini library to add/remove dynamic fields from a form. I have not included documentation yet because the code is changing in every PR. I save the final docs before merging the feature branch.

📌 Related Issues

Related to Add Surveys engine #1342

📋 Subtasks

Refactor templates
Admin can select question types
Add short and long answer question types
Admin can fill in option answers for a question
Add single and multiple option question types

📷 Screenshots (optional)

👻 GIF

houndci-bot · 2017-05-24T09:56:32Z

decidim-surveys/app/helpers/decidim/surveys/admin/application_helper.rb

+
+        def label_for_question(survey, question)
+          return survey.published? ? t('.question') : "#{icon("move")} #{t('.question')}".html_safe if question.persisted?
+          "#{icon("move")} #{t('.question')} #${questionLabelPosition}".html_safe


Prefer double-quoted strings inside interpolations.

houndci-bot · 2017-05-24T09:56:32Z

decidim-surveys/app/helpers/decidim/surveys/admin/application_helper.rb

+        end
+
+        def label_for_question(survey, question)
+          return survey.published? ? t('.question') : "#{icon("move")} #{t('.question')}".html_safe if question.persisted?


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
Prefer double-quoted strings inside interpolations.

codecov · 2017-05-24T10:08:42Z

Codecov Report

Merging #1398 into feature/surveys will increase coverage by 0.02%.
The diff coverage is 100%.

@@                 Coverage Diff                 @@
##           feature/surveys    #1398      +/-   ##
===================================================
+ Coverage            94.86%   94.89%   +0.02%     
===================================================
  Files                  444      445       +1     
  Lines                 7554     7593      +39     
===================================================
+ Hits                  7166     7205      +39     
  Misses                 388      388

houndci-bot · 2017-05-24T10:08:54Z

decidim-surveys/spec/forms/decidim/surveys/survey_form_spec.rb

-        described_class.from_model(survey)
+        described_class.from_model(survey).with_context({
+          current_feature: survey.feature
+        })


Indent the right brace the same as the first position after the preceding left parenthesis.

houndci-bot · 2017-05-24T10:08:54Z

decidim-surveys/spec/forms/decidim/surveys/survey_form_spec.rb

      subject do
-        described_class.from_model(survey)
+        described_class.from_model(survey).with_context({
+          current_feature: survey.feature


Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.

houndci-bot · 2017-05-24T10:08:54Z

decidim-surveys/spec/forms/decidim/surveys/survey_form_spec.rb


      subject do
-        described_class.from_model(survey)
+        described_class.from_model(survey).with_context({


Redundant curly braces around a hash parameter.

houndci-bot · 2017-05-24T10:08:54Z

decidim-surveys/spec/forms/decidim/surveys/survey_answer_form_spec.rb

+      subject do
+        described_class.from_model(survey_answer).with_context({
+          current_feature: survey.feature
+        })


Indent the right brace the same as the first position after the preceding left parenthesis.

houndci-bot · 2017-05-24T10:08:54Z

decidim-surveys/spec/forms/decidim/surveys/survey_answer_form_spec.rb

+
+      subject do
+        described_class.from_model(survey_answer).with_context({
+          current_feature: survey.feature


Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.

houndci-bot · 2017-05-24T10:08:54Z

decidim-surveys/spec/forms/decidim/surveys/survey_answer_form_spec.rb

+      let!(:survey_answer) { create(:survey_answer, user: user, survey: survey, question: survey_question) }
+
+      subject do
+        described_class.from_model(survey_answer).with_context({


Redundant curly braces around a hash parameter.

houndci-bot · 2017-05-25T12:43:44Z

decidim-surveys/spec/forms/decidim/surveys/admin/survey_form_spec.rb

              },
-              position: 0
+              position: 0,
+              question_type: "short_answer"              


Trailing whitespace detected.

houndci-bot · 2017-05-25T14:58:07Z

decidim-surveys/spec/commands/decidim/surveys/admin/update_survey_spec.rb

-                "position" => "0"
+                "position" => "0",
+                "question_type" => "short_answer",
+                "answer_options" => []                


Trailing whitespace detected.

houndci-bot · 2017-05-25T14:58:07Z

decidim-surveys/app/helpers/decidim/surveys/admin/application_helper.rb

+        end
+
+        def label_for_question(survey, question)
+          return survey.published? ? t(".question") : "#{icon("move")} #{t(".question")}".html_safe


Redundant return detected.

houndci-bot · 2017-05-25T14:58:08Z

decidim-surveys/app/helpers/decidim/surveys/admin/application_helper.rb

+          "${tabsId}"
+        end
+
+        def label_for_question(survey, question)


Unused method argument - question. If it's necessary, use _ or _question as an argument name to indicate that it won't be used.

houndci-bot · 2017-05-25T14:58:08Z

decidim-surveys/app/forms/decidim/surveys/admin/survey_question_answer_option_form.rb

+      end
+    end
+  end
+end


Final newline missing.

mention-bot · 2017-05-29T10:31:01Z

@beagleknight, thanks for your PR! By analyzing the history of the files in this pull request, we identified @lastpotion and @mrcasals to be potential reviewers.

mrcasals · 2017-05-30T08:22:08Z

decidim-surveys/app/forms/decidim/surveys/admin/survey_question_form.rb

+
+        # Private: answer options should be mapped manually.
+        def map_model(model)
+          self.answer_options = model.answer_options


This shouldn't be needed :/

It is needed! I did some tests with Virtus stuff and the problem is answer_options is not a real model, just a serialized field and he doesn't know how to build the actual form field.

Doesn't it call the method? I mean, you are not changing anything here. We usually use this method because of some form field names not matching the model method, so we need to manually set them. Here you are not doing anything special, you're calling the same method name that you are setting so it should be done automatically :/

It looks like @mrcasals is right, but 🤷‍♂️

mrcasals · 2017-05-30T08:22:31Z

decidim-surveys/app/helpers/decidim/surveys/admin/application_helper.rb

+      # Custom helpers, scoped to the surveys engine.
+      #
+      module ApplicationHelper
+        def tabs_id_for_question(question)


Docs for all methods?

I will do a PR with all the documentation because the API is changing too much.

mrcasals · 2017-05-30T08:23:36Z

decidim-surveys/app/models/decidim/surveys/survey_question.rb

      belongs_to :survey, class_name: Survey, foreign_key: "decidim_survey_id"
+
+      def answer_options
+        self[:answer_options] || []


This smells like you're missing a default: [] value in the migration that adds this column

mrcasals · 2017-05-30T08:25:38Z

decidim-surveys/db/migrate/20170525132233_add_answer_options_to_surveys_questions.rb

@@ -0,0 +1,5 @@
+class AddAnswerOptionsToSurveysQuestions < ActiveRecord::Migration[5.0]
+  def change
+    add_column :decidim_surveys_survey_questions, :answer_options, :jsonb


Add a default non-nil value. See the comment I left in the model

default: [] should be goos enough, if you need an array here

josepjaume

Nice!! Outstanding work!

josepjaume · 2017-05-30T09:45:46Z

decidim-surveys/app/forms/decidim/surveys/admin/survey_question_form.rb

+
+        # Private: answer options should be mapped manually.
+        def map_model(model)
+          self.answer_options = model.answer_options


It looks like @mrcasals is right, but 🤷‍♂️

beagleknight · 2017-05-30T10:11:20Z

@josepjaume @mrcasals 🤷‍♂️

josepjaume · 2017-05-30T10:34:46Z

decidim-surveys/app/models/decidim/surveys/survey_question.rb

+      # Rectify can't handle a hash when using the from_model method so
+      # the answer options must be converted to struct.
+      def answer_options
+        self[:answer_options].map { |option| OpenStruct.new(option) }


This makes me cry so hard

beagleknight added the in-progress label May 24, 2017

beagleknight self-assigned this May 24, 2017

houndci-bot reviewed May 24, 2017

View reviewed changes

beagleknight force-pushed the feature/surveys-question-types branch from 1a25fa9 to cabe8ee Compare May 24, 2017 10:08

houndci-bot reviewed May 24, 2017

View reviewed changes

beagleknight changed the title ~~Add question types~~ Add question types to surveys May 24, 2017

beagleknight mentioned this pull request May 24, 2017

Add Surveys engine #1342

Merged

8 tasks

beagleknight force-pushed the feature/surveys-question-types branch from cabe8ee to b14d909 Compare May 24, 2017 10:13

beagleknight added 3 commits May 24, 2017 15:17

Huge refactor templates

d868850

Fix rubocop issues

0479dfb

Admins can select question type

29bbda5

beagleknight force-pushed the feature/surveys-question-types branch from b14d909 to 29bbda5 Compare May 24, 2017 13:17

beagleknight added 4 commits May 24, 2017 15:29

Short and long answers working

d15de4c

Factor out auto label position code

6fc94c2

Moar refactor

d16b597

Huge JS refactor

5e23f24

houndci-bot reviewed May 25, 2017

View reviewed changes

Add dynamic answer options for a question

f03ebe0

houndci-bot reviewed May 25, 2017

View reviewed changes

beagleknight added 3 commits May 29, 2017 11:45

Admin survey form working with dynamic answer options

74da3c5

Single option question type working

a86f8d9

Single and multiple options working

dd19847

beagleknight added the in-review label May 29, 2017

beagleknight removed the in-progress label May 29, 2017

beagleknight added 4 commits May 29, 2017 12:33

Fixed rubocop issues

aced62b

Fix i18n problems

e4e1a85

Ignore unused i18n

1e00ef3

Add ignore unused

281a5f2

mrcasals reviewed May 30, 2017

View reviewed changes

Add answer option default value

e9d3569

josepjaume previously approved these changes May 30, 2017

View reviewed changes

Fix rectify problem

084d61e

beagleknight dismissed josepjaume’s stale review via 084d61e May 30, 2017 10:34

josepjaume reviewed May 30, 2017

View reviewed changes

josepjaume approved these changes May 30, 2017

View reviewed changes

mrcasals approved these changes May 30, 2017

View reviewed changes

beagleknight merged commit 25d95a1 into feature/surveys May 30, 2017

beagleknight deleted the feature/surveys-question-types branch May 30, 2017 12:15

beagleknight removed the in-review label May 30, 2017

beagleknight mentioned this pull request May 31, 2017

Restore missing PRs in feature/surveys #1421

Merged

+                    end
+                  end
+                end
+              end

                
                    No newline at end of file

Uh oh!

Conversation

beagleknight commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎩 What? Why?

📌 Related Issues

📋 Subtasks

📷 Screenshots (optional)

👻 GIF

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mention-bot commented May 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josepjaume left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beagleknight commented May 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

beagleknight commented May 24, 2017 •

edited

Loading

codecov bot commented May 24, 2017 •

edited

Loading