More survey improvements (#3133)

* Remove try's from survey template * Move enabling logic out of component Since it might be different depending on the type of field. For example, for a text field dependent on a checkbox, we'll need to check the "checked" property of the checkbox, not its value. * Simplify and refactor some logic * Extract answer rendering to a partial * Remove now unneeded early returns * Slightly improve a test * Test that answer information is properly saved * Split different answer types to separate columns Keeping them on a single column is weird and very unflexible. * Prefer symbols for adding errors to an attribute * Ensure numbering is kept after errors * Fix question type select incorrectly disabled * Better block variable name Answer is a different model not involved here, so this version is less confusing, I think. * Refactor survey answer form specs * Consolidate survey answer validations * Use proper forms around answers This is a similar refactoring to what was done in the backend. Instead of using isolated form helpers, use a proper form builder. This came with the fix/feature of adding proper support for client side validations, improving user experience and reducing server load. * Extract repeated method call to a variable * Normalize comment spacing * Fix incorrect disabling of max choices selector * Fix incorrect disabling of translated fields * Fix bad migration * Proper column type conversion PostgreSQL casting was saving the new text bodies as "\"value\"". * Normalize `input[type=*]` selectors
decidim · Apr 6, 2018 · b70aef9 · b70aef9
1 parent 0a5b498
commit b70aef9
Show file tree

Hide file tree

Showing 22 changed files with 285 additions and 120 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -40,6 +40,7 @@ controller or added a new module you need to rename `feature` to `component`.
 - **decidim-surveys**: Add rich text description to questions [\#3066](https://github.com/decidim/decidim/pull/3066).
 - **decidim-proposals**: Add discard draft button in wizard [\#3064](https://github.com/decidim/decidim/pull/3064)
 - **decidim-surveys**: Allow multiple choice questions to specify a maximum number of options to be checked [\#3091](https://github.com/decidim/decidim/pull/3091)
+- **decidim-surveys**: Client side survey errors are now displayed [\#3133](https://github.com/decidim/decidim/pull/3133)
 
 **Changed**:
 
@@ -67,5 +68,8 @@ controller or added a new module you need to rename `feature` to `component`.
 - **decidim-surveys**: Multiple choice questions without answer options can no longer be created [\#3087](https://github.com/decidim/decidim/pull/3087)
 - **decidim-surveys**: Multiple choice questions with empty answer options can no longer be created [\#3087](https://github.com/decidim/decidim/pull/3087)
 - **decidim-surveys**: Preserve deleted status of questions accross submission failures [\#3089](https://github.com/decidim/decidim/pull/3089)
+- **decidim-surveys**: Question type selector not disabled when survey has already been answered [\#3133](https://github.com/decidim/decidim/pull/3133)
+- **decidim-surveys**: Max choices selector not disabled when survey has already been answered [\#3133](https://github.com/decidim/decidim/pull/3133)
+- **decidim-surveys**: Translated fields not disabled when survey has already been answered [\#3133](https://github.com/decidim/decidim/pull/3133)
 
 Please check [0.10-stable](https://github.com/decidim/decidim/blob/0.10-stable/CHANGELOG.md) for previous changes.
diff --git a/decidim-core/app/assets/javascripts/decidim/editor.js.es6 b/decidim-core/app/assets/javascripts/decidim/editor.js.es6
@@ -6,6 +6,7 @@
 
   const createQuillEditor = (container) => {
     const toolbar = $(container).data("toolbar");
+    const disabled = $(container).data("disabled");
 
     let quillToolbar = [
       ["bold", "italic", "underline"],
@@ -35,6 +36,10 @@
       theme: "snow"
     });
 
+    if (disabled) {
+      quill.disable();
+    }
+
     quill.on("text-change", () => {
       const text = quill.getText();
 

diff --git a/decidim-core/lib/decidim/form_builder.rb b/decidim-core/lib/decidim/form_builder.rb
@@ -83,22 +83,25 @@ def translated(type, name, options = {})
     #
     # name - The name of the field
     # options - The set of options to send to the field
-    #           :label   - The Boolean value to create or not the input label (optional) (default: true)
+    #           :label - The Boolean value to create or not the input label (optional) (default: true)
     #           :toolbar - The String value to configure WYSIWYG toolbar. It should be 'basic' or
     #                      or 'full' (optional) (default: 'basic')
-    #           :lines   - The Integer to indicate how many lines should editor have (optional) (default: 10)
+    #           :lines - The Integer to indicate how many lines should editor have (optional) (default: 10)
+    #           :disabled - Whether the editor should be disabled
     #
     # Renders a container with both hidden field and editor container
     def editor(name, options = {})
       options[:toolbar] ||= "basic"
       options[:lines] ||= 10
+      options[:disabled] ||= false
 
       content_tag(:div, class: "editor") do
         template = ""
         template += label(name, options[:label].to_s || name) if options[:label] != false
         template += hidden_field(name, options)
         template += content_tag(:div, nil, class: "editor-container", data: {
-                                  toolbar: options[:toolbar]
+                                  toolbar: options[:toolbar],
+                                  disabled: options[:disabled]
                                 }, style: "height: #{options[:lines]}rem")
         template += error_for(name, options) if error?(name)
         template.html_safe

diff --git a/...veys/app/assets/javascripts/decidim/surveys/admin/field_dependent_inputs.component.js.es6 b/...veys/app/assets/javascripts/decidim/surveys/admin/field_dependent_inputs.component.js.es6
@@ -5,7 +5,7 @@
       this.wrapperSelector = options.wrapperSelector;
       this.dependentFieldsSelector = options.dependentFieldsSelector;
       this.dependentInputSelector = options.dependentInputSelector;
-      this.enablingValues = options.enablingValues;
+      this.enablingCondition = options.enablingCondition;
       this._bindEvent();
       this._run();
     }
@@ -14,9 +14,8 @@
       const $controllerField = this.controllerField;
       const $dependentFields = $controllerField.parents(this.wrapperSelector).find(this.dependentFieldsSelector);
       const $dependentInputs = $dependentFields.find(this.dependentInputSelector);
-      const value = $controllerField.val();
 
-      if (this.enablingValues.indexOf(value) > -1) {
+      if (this.enablingCondition($controllerField)) {
         $dependentInputs.prop("disabled", false);
         $dependentFields.show();
       } else {

diff --git a/decidim-surveys/app/assets/javascripts/decidim/surveys/admin/surveys.js.es6 b/decidim-surveys/app/assets/javascripts/decidim/surveys/admin/surveys.js.es6
@@ -81,6 +81,12 @@
 
   const dynamicFieldsForAnswerOptions = {};
 
+  const isMultipleChoiceOption = ($selectField) => {
+    const value = $selectField.val();
+
+    return value === "single_option" || value === "multiple_option"
+  }
+
   const setupInitialQuestionAttributes = ($target) => {
     const fieldId = $target.attr("id");
     const $fieldQuestionTypeSelect = $target.find(questionTypeSelector);
@@ -90,25 +96,27 @@
       wrapperSelector: fieldSelector,
       dependentFieldsSelector: answerOptionsWrapperSelector,
       dependentInputSelector: `${answerOptionFieldSelector} input`,
-      enablingValues: ["single_option", "multiple_option"]
+      enablingCondition: ($field) => {
+        return isMultipleChoiceOption($field);
+      }
     });
 
     createFieldDependentInputs({
       controllerField: $fieldQuestionTypeSelect,
       wrapperSelector: fieldSelector,
       dependentFieldsSelector: maxChoicesWrapperSelector,
       dependentInputSelector: "select",
-      enablingValues: ["multiple_option"]
+      enablingCondition: ($field) => {
+        return $field.val() === "multiple_option"
+      }
     });
 
     dynamicFieldsForAnswerOptions[fieldId] = createDynamicFieldsForAnswerOptions(fieldId);
 
     const dynamicFields = dynamicFieldsForAnswerOptions[fieldId];
 
     const onQuestionTypeChange = () => {
-      const value = $fieldQuestionTypeSelect.val();
-
-      if (value === "single_option" || value === "multiple_option") {
+      if (isMultipleChoiceOption($fieldQuestionTypeSelect)) {
         const nOptions = $fieldQuestionTypeSelect.parents(fieldSelector).find(answerOptionFieldSelector).length;
 
         if (nOptions === 0) {

diff --git a/decidim-surveys/app/commands/decidim/surveys/admin/update_survey.rb b/decidim-surveys/app/commands/decidim/surveys/admin/update_survey.rb
@@ -39,7 +39,7 @@ def update_survey_questions
               position: form_question.position,
               mandatory: form_question.mandatory,
               question_type: form_question.question_type,
-              answer_options: form_question.answer_options_to_persist.map { |answer| { "body" => answer.body } },
+              answer_options: form_question.answer_options_to_persist.map { |option| { "body" => option.body } },
               max_choices: form_question.max_choices
             }
 

diff --git a/decidim-surveys/app/commands/decidim/surveys/answer_survey.rb b/decidim-surveys/app/commands/decidim/surveys/answer_survey.rb
@@ -33,7 +33,8 @@ def answer_survey
               user: @current_user,
               survey: @survey,
               question: form_answer.question,
-              body: form_answer.body
+              body: form_answer.body,
+              choices: form_answer.choices
             )
           end
         end

diff --git a/decidim-surveys/app/forms/decidim/surveys/survey_answer_form.rb b/decidim-surveys/app/forms/decidim/surveys/survey_answer_form.rb
@@ -8,18 +8,21 @@ class SurveyAnswerForm < Decidim::Form
 
       attribute :question_id, String
       attribute :body, String
+      attribute :choices, Array[String]
 
-      validates :body, presence: true, if: -> { question.mandatory? }
+      validates :body, presence: true, if: :mandatory_body?
+      validates :choices, presence: true, if: :mandatory_choices?
 
-      validate :body_not_blank, if: -> { question.mandatory? }
       validate :max_answers, if: -> { question.max_choices }
 
+      delegate :mandatory_body?, :mandatory_choices?, to: :question
+
       def question
         @question ||= survey.questions.find(question_id)
       end
 
       def label
-        base = "#{question.position + 1}. #{translated_attribute(question.body)}"
+        base = "#{id}. #{translated_attribute(question.body)}"
         base += " #{mandatory_label}" if question.mandatory?
         base += " (#{max_choices_label})" if question.max_choices
         base
@@ -38,15 +41,8 @@ def survey
         @survey ||= Survey.find_by(component: current_component)
       end
 
-      def body_not_blank
-        return if body.nil?
-        errors.add("body", :blank) if body.all?(&:blank?)
-      end
-
       def max_answers
-        return if body.nil?
-
-        errors.add("body", :too_many_choices) if body.size > question.max_choices
+        errors.add(:choices, :too_many) if choices.size > question.max_choices
       end
 
       def mandatory_label

diff --git a/decidim-surveys/app/forms/decidim/surveys/survey_form.rb b/decidim-surveys/app/forms/decidim/surveys/survey_form.rb
@@ -13,8 +13,8 @@ class SurveyForm < Decidim::Form
       #
       # Returns nothing.
       def map_model(model)
-        self.answers = model.questions.map do |question|
-          SurveyAnswerForm.from_params(question_id: question.id)
+        self.answers = model.questions.each_with_index.map do |question, id|
+          SurveyAnswerForm.new(id: id + 1, question_id: question.id)
         end
       end
     end

diff --git a/decidim-surveys/app/models/decidim/surveys/survey_answer.rb b/decidim-surveys/app/models/decidim/surveys/survey_answer.rb
@@ -8,7 +8,9 @@ class SurveyAnswer < Surveys::ApplicationRecord
       belongs_to :survey, class_name: "Survey", foreign_key: "decidim_survey_id"
       belongs_to :question, class_name: "SurveyQuestion", foreign_key: "decidim_survey_question_id"
 
-      validates :body, presence: true, if: -> { question.mandatory? }
+      validates :body, presence: true, if: -> { question.mandatory_body? }
+      validates :options, presence: true, if: -> { question.mandatory_choices? }
+
       validate :user_survey_same_organization
       validate :question_belongs_to_survey
 

diff --git a/decidim-surveys/app/models/decidim/surveys/survey_question.rb b/decidim-surveys/app/models/decidim/surveys/survey_question.rb
@@ -9,6 +9,18 @@ class SurveyQuestion < Surveys::ApplicationRecord
       belongs_to :survey, class_name: "Survey", foreign_key: "decidim_survey_id"
 
       validates :question_type, inclusion: { in: TYPES }
+
+      def multiple_choice?
+        %w(single_option multiple_option).include?(question_type)
+      end
+
+      def mandatory_body?
+        mandatory? && !multiple_choice?
+      end
+
+      def mandatory_choices?
+        mandatory? && multiple_choice?
+      end
     end
   end
 end
diff --git a/decidim-surveys/app/views/decidim/surveys/admin/surveys/_answer_option.html.erb b/decidim-surveys/app/views/decidim/surveys/admin/surveys/_answer_option.html.erb
@@ -4,7 +4,7 @@
   <div class="card-divider">
     <h2 class="card-title">
     <span><%= t(".answer_option") %></span>
-    <% if survey.questions_editable? %>
+    <% if editable %>
       <button class="button small alert hollow remove-answer-option button--title">
         <%= t(".remove") %>
       </button>
@@ -20,15 +20,15 @@
           :body,
           tabs_id: tabs_id_for_question_answer_option(question, answer_option),
           label: t(".statement"),
-          disabled: !survey.questions_editable?
+          disabled: !editable
         )
       %>
     </div>
   </div>
 
   <% if answer_option.persisted? %>
-    <%= form.hidden_field :id, disabled: !survey.questions_editable? %>
+    <%= form.hidden_field :id, disabled: !editable %>
   <% end %>
 
-  <%= form.hidden_field :deleted, value: false, disabled: !survey.questions_editable? %>
+  <%= form.hidden_field :deleted, value: false, disabled: !editable %>
 </div>
diff --git a/decidim-surveys/app/views/decidim/surveys/admin/surveys/_form.html.erb b/decidim-surveys/app/views/decidim/surveys/admin/surveys/_form.html.erb
@@ -24,7 +24,7 @@
   <% if survey.questions_editable? %>
     <template>
       <%= fields_for "survey[questions][#{blank_question.to_param}]", blank_question do |question_form| %>
-        <%= render "question", form: question_form, id: tabs_id_for_question(blank_question) %>
+        <%= render "question", form: question_form, id: tabs_id_for_question(blank_question), editable: survey.questions_editable? %>
       <% end %>
     </template>
   <% else %>
@@ -36,7 +36,7 @@
   <div class="survey-questions-list">
     <% @form.questions.each do |question| %>
       <%= fields_for "survey[questions][]", question do |question_form| %>
-        <%= render "question", form: question_form, id: tabs_id_for_question(question) %>
+        <%= render "question", form: question_form, id: tabs_id_for_question(question), editable: survey.questions_editable? %>
       <% end %>
     <% end %>
   </div>
@@ -46,4 +46,6 @@
   <% end %>
 </div>
 
-<%= javascript_include_tag "decidim/surveys/admin/surveys" %>
+<% if survey.questions_editable? %>
+  <%= javascript_include_tag "decidim/surveys/admin/surveys" %>
+<% end %>
diff --git a/decidim-surveys/app/views/decidim/surveys/admin/surveys/_question.html.erb b/decidim-surveys/app/views/decidim/surveys/admin/surveys/_question.html.erb
@@ -4,14 +4,14 @@
   <div class="card-divider question-divider">
     <h2 class="card-title">
     <span>
-      <% if survey.questions_editable? %>
+      <% if editable %>
         <%== "#{icon("move")} #{t(".question")}" %>
       <% else %>
         <%= t(".question") %>
       <% end %>
     </span>
 
-    <% if survey.questions_editable? %>
+    <% if editable %>
       <button class="button small alert hollow move-up-question button--title">
         <%== "#{icon("arrow-top")} #{t(".up")}" %>
       </button>
@@ -35,7 +35,7 @@
           :body,
           tabs_id: id,
           label: t(".statement"),
-          disabled: !survey.questions_editable?
+          disabled: !editable
         )
       %>
     </div>
@@ -48,7 +48,7 @@
           toolbar: :full,
           tabs_id: id,
           label: t(".description"),
-          disabled: !survey.questions_editable?
+          disabled: !editable
         )
       %>
     </div>
@@ -57,7 +57,7 @@
       <%=
         form.check_box(
           :mandatory,
-          disabled: !survey.questions_editable?,
+          disabled: !editable,
           label: t("activemodel.attributes.survey_question.mandatory")
         )
       %>
@@ -68,34 +68,35 @@
         form.select(
           :question_type,
           options_from_collection_for_select(question_types, :first, :last, question.question_type),
-          disabled: !survey.questions_editable?
+          {},
+          disabled: !editable
         )
       %>
     </div>
 
     <% if question.persisted? %>
-      <%= form.hidden_field :id, disabled: !survey.questions_editable? %>
+      <%= form.hidden_field :id, disabled: !editable %>
     <% end %>
 
-    <%= form.hidden_field :position, value: question.position || 0, disabled: !survey.questions_editable? %>
-    <%= form.hidden_field :deleted, disabled: !survey.questions_editable? %>
+    <%= form.hidden_field :position, value: question.position || 0, disabled: !editable %>
+    <%= form.hidden_field :deleted, disabled: !editable %>
 
     <div class="survey-question-answer-options">
       <template>
         <%= fields_for "survey[questions][#{question.to_param}][answer_options][]", blank_answer_option do |answer_option_form| %>
-          <%= render "answer_option", form: answer_option_form, question: question %>
+          <%= render "answer_option", form: answer_option_form, question: question, editable: editable %>
         <% end %>
       </template>
 
       <div class="survey-question-answer-options-list">
         <% question.answer_options.each do |answer_option| %>
           <%= fields_for "survey[questions][#{question.to_param}][answer_options][]", answer_option do |answer_option_form| %>
-            <%= render "answer_option", form: answer_option_form, question: question %>
+            <%= render "answer_option", form: answer_option_form, question: question, editable: editable %>
           <% end %>
         <% end %>
       </div>
 
-      <% if survey.questions_editable? %>
+      <% if editable %>
         <button class="button add-answer-option"><%= t(".add_answer_option") %></button>
       <% end %>
     </div>
@@ -105,8 +106,8 @@
         form.select(
           :max_choices,
           (2..question.number_of_options),
-          prompt: t(".any"),
-          disabled: !survey.questions_editable?
+          { prompt: t(".any") },
+          disabled: !editable
         )
       %>
     </div>