Fix question form errors not being displayed (#3046)

* Remove unused attribute * Avoid the need to pass the template id I'll be using proper form builder helpers for the inputs of these forms so that proper error messages can be displayed around each one. In order to do that, each template must have different base input names to be able to distinguish which question each answer option belongs to. So we render a separate template next to each "add field" button, and we can locate it from the input button itself without needing any ids. * Stop caring about the position of the placeholder In later commits nested templates could go through several interpolations and the placeholder won't always be in the middle, so stop caring about its position. This allows us to extract an utility method since the replacement is consistently done in the same way. * Prefer an empty form to an `OpenStruct` For consistency with the `blank_question` method. * Use rectify's default of integer id's * Simplify unique nested field id generation I think this is "more unique" and it also generates an integer, which will be handy later. * Pass placeholder id directly So that the generate unique key is actually an integer. This will be useful later when will use this id as the id for our rectify form. * Remove OpenStruct weirdness from SurveyQuestion Instead, create a different virtual attribute at the form level and do proper mapping in there. * Minor reordering in specs I think it reads slightly better like this. * A few more spec readability improvements * Closer match to `fiels_for` naming * Fix error messages not displayed on question error We now use proper form builder methods that wrap actual objects, so we get full error messages, required attribute detection, and so on.
decidim · Mar 20, 2018 · ed4fa90 · ed4fa90
1 parent d02571c
commit ed4fa90
Show file tree

Hide file tree

Showing 14 changed files with 166 additions and 130 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -39,5 +39,6 @@ controller or added a new module you need to rename `feature` to `component`.
 - **decidim-surveys**: Fix errored questions being re-rendered with disabled inputs [\#3014](https://github.com/decidim/decidim/pull/3014)
 - **decidim-surveys**: Fix errored questions rendering answer options as empty fields [\#3014](https://github.com/decidim/decidim/pull/3014)
 - **decidim-surveys**: Fix translated fields of freshly created questions not working after form errors [\#3026](https://github.com/decidim/decidim/pull/3026)
+- **decidim-surveys**: Fix question form errors not being displayed [\#3046](https://github.com/decidim/decidim/pull/3046)
 
 Please check [0.10-stable](https://github.com/decidim/decidim/blob/0.10-stable/CHANGELOG.md) for previous changes.
diff --git a/decidim-surveys/app/assets/javascripts/decidim/surveys/admin/dynamic_fields.component.js.es6 b/decidim-surveys/app/assets/javascripts/decidim/surveys/admin/dynamic_fields.component.js.es6
@@ -1,7 +1,6 @@
 ((exports) => {
   class DynamicFieldsComponent {
     constructor(options = {}) {
-      this.templateId = options.templateId;
       this.wrapperSelector = options.wrapperSelector;
       this.containerSelector = options.containerSelector;
       this.fieldSelector = options.fieldSelector;
@@ -13,34 +12,35 @@
       this.onRemoveField = options.onRemoveField;
       this.onMoveUpField = options.onMoveUpField;
       this.onMoveDownField = options.onMoveDownField;
-      this.tabsPrefix = options.tabsPrefix;
+      this.placeholderId = options.placeholderId;
+      this.elementCounter = 0;
       this._enableInterpolation();
       this._activateFields();
       this._bindEvents();
     }
 
     _enableInterpolation() {
-      $.fn.template = function(placeholder, value) {
-        $(this).find(`[id^=${placeholder}]`).each((index, element) => {
-          $(element).attr("id", $(element).attr("id").replace(placeholder, value));
-        });
-
-        $(this).find(`[data-tabs-content=${placeholder}]`).each((index, element) => {
-          $(element).attr("data-tabs-content", value);
+      $.fn.replaceAttribute = function(attribute, placeholder, value) {
+        $(this).find(`[${attribute}*=${placeholder}]`).each((index, element) => {
+          $(element).attr(attribute, $(element).attr(attribute).replace(placeholder, value));
         });
 
-        $(this).find(`[for^=${placeholder}]`).each((index, element) => {
-          $(element).attr("for", $(element).attr("for").replace(placeholder, value));
-        });
+        return this;
+      }
 
-        $(this).find(`[tabs_id=${placeholder}]`).each((index, element) => {
-          $(element).attr("tabs_id", value);
-        });
+      $.fn.template = function(placeholder, value) {
+        const $subtemplate = $(this).find("template");
 
+        if ($subtemplate.length > 0) {
+          $subtemplate.html((index, oldHtml) => $(oldHtml).template(placeholder, value)[0].outerHTML);
+        }
 
-        $(this).find(`[href^='#${placeholder}']`).each((index, element) => {
-          $(element).attr("href", $(element).attr("href").replace(placeholder, value));
-        });
+        $(this).replaceAttribute("id", placeholder, value);
+        $(this).replaceAttribute("name", placeholder, value);
+        $(this).replaceAttribute("data-tabs-content", placeholder, value);
+        $(this).replaceAttribute("for", placeholder, value);
+        $(this).replaceAttribute("tabs_id", placeholder, value);
+        $(this).replaceAttribute("href", placeholder, value);
 
         return this;
       }
@@ -82,7 +82,8 @@
 
     _addField() {
       const $container = $(this.wrapperSelector).find(this.containerSelector);
-      const $newField = $($(`#${this.templateId}`).html()).template(this._getPlaceholderTabId(), this._getUniqueTabId());
+      const $template = $(this.wrapperSelector).children("template");
+      const $newField = $($template.html()).template(this.placeholderId, this._getUID());
 
       $newField.find('ul.tabs').attr('data-tabs', true);
 
@@ -141,22 +142,16 @@
 
     _activateFields() {
       $(this.fieldSelector).each((idx, el) => {
-        $(el).template(this._getPlaceholderTabId(), this._getUniqueTabId());
+        $(el).template(this.placeholderId, this._getUID());
 
         $(el).find('ul.tabs').attr('data-tabs', true);
       })
     }
 
-    _getPlaceholderTabId() {
-      return `${this.tabsPrefix}-id`;
-    }
-
-    _getUniqueTabId() {
-      return `${this.tabsPrefix}-${this._getUID()}`;
-    }
-
     _getUID() {
-      return `${new Date().getTime()}-${Math.floor(Math.random() * 1000000)}`;
+      this.elementCounter += 1;
+
+      return (new Date().getTime()) + this.elementCounter;
     }
   }
 

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
@@ -7,14 +7,14 @@
 
   const wrapperSelector = '.survey-questions';
   const fieldSelector = '.survey-question';
-  const questionTypeSelector = '[name="survey[questions][][question_type]"]';
+  const questionTypeSelector = 'select[name$=\\[question_type\\]]';
   const answerOptionsWrapperSelector = '.survey-question-answer-options';
 
   const autoLabelByPosition = new AutoLabelByPositionComponent({
     listSelector: '.survey-question:not(.hidden)',
     labelSelector: '.card-title span:first',
     onPositionComputed: (el, idx) => {
-      $(el).find('input[name="survey[questions][][position]"]').val(idx);
+      $(el).find('input[name$=\\[position\\]]').val(idx);
     }
   });
 
@@ -35,8 +35,7 @@
 
   const createDynamicFieldsForAnswerOptions = (fieldId) => {
     createDynamicFields({
-      templateId: `survey-question-answer-option-tmpl`,
-      tabsPrefix: `survey-question-answer-option`,
+      placeholderId: `survey-question-answer-option-id`,
       wrapperSelector: `#${fieldId} ${answerOptionsWrapperSelector}`,
       containerSelector: `.survey-question-answer-options-list`,
       fieldSelector: `.survey-question-answer-option`,
@@ -57,8 +56,7 @@
   };
 
   createDynamicFields({
-    templateId: 'survey-question-tmpl',
-    tabsPrefix: 'survey-question',
+    placeholderId: 'survey-question-id',
     wrapperSelector: wrapperSelector,
     containerSelector: '.survey-questions-list',
     fieldSelector: fieldSelector,

diff --git a/decidim-surveys/app/commands/decidim/surveys/admin/update_survey.rb b/decidim-surveys/app/commands/decidim/surveys/admin/update_survey.rb
@@ -38,7 +38,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.map { |answer| { "body" => answer.body } }
+              answer_options: form_question.options.map { |answer| { "body" => answer.body } }
             }
 
             if form_question.id.present?

diff --git a/decidim-surveys/app/controllers/decidim/surveys/admin/surveys_controller.rb b/decidim-surveys/app/controllers/decidim/surveys/admin/surveys_controller.rb
@@ -41,7 +41,7 @@ def blank_question
         end
 
         def blank_answer_option
-          @blank_answer_option ||= OpenStruct.new(body: {})
+          @blank_answer_option ||= Admin::SurveyQuestionAnswerOptionForm.new
         end
 
         def question_types

diff --git a/decidim-surveys/app/forms/decidim/surveys/admin/survey_question_answer_option_form.rb b/decidim-surveys/app/forms/decidim/surveys/admin/survey_question_answer_option_form.rb
@@ -7,8 +7,11 @@ module Admin
       class SurveyQuestionAnswerOptionForm < Decidim::Form
         include TranslatableAttributes
 
-        attribute :body, String
         translatable_attribute :body, String
+
+        def to_param
+          id || "survey-question-answer-option-id"
+        end
       end
     end
   end

diff --git a/decidim-surveys/app/forms/decidim/surveys/admin/survey_question_form.rb b/decidim-surveys/app/forms/decidim/surveys/admin/survey_question_form.rb
@@ -7,18 +7,33 @@ module Admin
       class SurveyQuestionForm < Decidim::Form
         include TranslatableAttributes
 
-        attribute :id, String
         attribute :position, Integer
         attribute :mandatory, Boolean, default: false
         attribute :question_type, String
-        attribute :answer_options, Array[SurveyQuestionAnswerOptionForm]
+        attribute :options, Array[SurveyQuestionAnswerOptionForm]
         attribute :deleted, Boolean, default: false
 
         translatable_attribute :body, String
 
         validates :position, numericality: { greater_than_or_equal_to: 0 }
         validates :question_type, inclusion: { in: SurveyQuestion::TYPES }
         validates :body, translatable_presence: true, unless: :deleted
+
+        def map_model(model)
+          self.options = model.answer_options.each_with_index.map do |option, id|
+            [id + 1, option]
+          end
+        end
+
+        def options=(value)
+          @options = value.map do |id, option|
+            SurveyQuestionAnswerOptionForm.new(option.merge(id: id.to_s.to_i))
+          end
+        end
+
+        def to_param
+          id || "survey-question-id"
+        end
       end
     end
   end

diff --git a/decidim-surveys/app/helpers/decidim/surveys/admin/application_helper.rb b/decidim-surveys/app/helpers/decidim/surveys/admin/application_helper.rb
@@ -7,15 +7,11 @@ module Admin
       #
       module ApplicationHelper
         def tabs_id_for_question(question)
-          id = question.persisted? ? question.id : "id"
-
-          "survey-question-#{id}"
+          "survey_question_#{question.to_param}"
         end
 
-        def tabs_id_for_question_answer_option(answer_option, idx)
-          id = answer_option.persisted? ? "#{answer_option.question.id}-#{idx}" : "id"
-
-          "survey-question-answer-option-#{id}"
+        def tabs_id_for_question_answer_option(question, answer_option)
+          "survey_question_#{question.to_param}_answer_option_#{answer_option.to_param}"
         end
       end
     end

diff --git a/decidim-surveys/app/models/decidim/surveys/survey_question.rb b/decidim-surveys/app/models/decidim/surveys/survey_question.rb
@@ -9,12 +9,6 @@ class SurveyQuestion < Surveys::ApplicationRecord
       belongs_to :survey, class_name: "Survey", foreign_key: "decidim_survey_id"
 
       validates :question_type, inclusion: { in: TYPES }
-
-      # 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) }
-      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
@@ -13,12 +13,10 @@
   <div class="card-section">
     <div class="row column">
       <%=
-        translated_field_tag(
-          :text_field_tag,
-          "survey[questions][][answer_options][]",
-          "body",
-          answer_option.body.with_indifferent_access,
-          tabs_id: tabs_id_for_question_answer_option(answer_option, idx),
+        form.translated(
+          :text_field,
+          :body,
+          tabs_id: tabs_id_for_question_answer_option(question, answer_option),
           label: t(".statement"),
           disabled: !survey.questions_editable?,
           enable_tabs: answer_option.persisted?

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
@@ -22,12 +22,10 @@
 
 <div class="survey-questions">
   <% if survey.questions_editable? %>
-    <template id="survey-question-tmpl">
-      <%= render "question", question: blank_question, id: tabs_id_for_question(blank_question) %>
-    </template>
-
-    <template id="survey-question-answer-option-tmpl">
-      <%= render "answer_option", answer_option: blank_answer_option, idx: nil %>
+    <template>
+      <%= fields_for "survey[questions][#{blank_question.to_param}]", blank_question do |question_form| %>
+        <%= render "question", question: blank_question, form: question_form, id: tabs_id_for_question(blank_question) %>
+      <% end %>
     </template>
   <% else %>
     <div class="callout warning">
@@ -37,7 +35,9 @@
 
   <div class="survey-questions-list">
     <% @form.questions.each do |question| %>
-      <%= render "question", question: question, id: tabs_id_for_question(question) %>
+      <%= fields_for "survey[questions][]", question do |question_form| %>
+        <%= render "question", question: question, form: question_form, id: tabs_id_for_question(question) %>
+      <% end %>
     <% end %>
   </div>
 

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
@@ -28,12 +28,9 @@
   <div class="card-section">
     <div class="row column">
       <%=
-        translated_field_tag(
-          :text_field_tag,
-          "survey[questions][]",
-          "body",
-          question.body.with_indifferent_access,
-          object: question,
+        form.translated(
+          :text_field,
+          :body,
           tabs_id: id,
           label: t(".statement"),
           disabled: !survey.questions_editable?,
@@ -44,33 +41,43 @@
 
     <div class="row column">
       <%=
-        check_box_tag(
-          "survey[questions][][mandatory]",
-          "1",
-          question.mandatory,
-          id: "#{id}-mandatory",
-          disabled: !survey.questions_editable?
+        form.check_box(
+          :mandatory,
+          disabled: !survey.questions_editable?,
+          label: t("activemodel.attributes.survey_question.mandatory")
         )
       %>
-      <%= label_tag "", t("activemodel.attributes.survey_question.mandatory"), for: "#{id}-mandatory" %>
     </div>
 
     <div class="row column">
-      <%= label_tag "", t("activemodel.attributes.survey_question.question_type"), for: "#{id}-question_type" %>
-      <%= select_tag "survey[questions][][question_type]", options_from_collection_for_select(question_types, :first, :last, question.question_type), id: "#{id}-question_type", disabled: !survey.questions_editable? %>
+      <%=
+        form.select(
+          :question_type,
+          options_from_collection_for_select(question_types, :first, :last, question.question_type),
+          disabled: !survey.questions_editable?
+        )
+      %>
     </div>
 
     <% if question.persisted? %>
-      <%= hidden_field_tag "survey[questions][][id]", question.id, disabled: !survey.questions_editable? %>
+      <%= form.hidden_field :id, disabled: !survey.questions_editable? %>
     <% end %>
 
-    <%= hidden_field_tag "survey[questions][][position]", question.position || 0, disabled: !survey.questions_editable? %>
-    <%= hidden_field_tag "survey[questions][][deleted]", false, disabled: !survey.questions_editable? %>
+    <%= form.hidden_field :position, value: question.position || 0, disabled: !survey.questions_editable? %>
+    <%= form.hidden_field :deleted, value: false, disabled: !survey.questions_editable? %>
 
     <div class="survey-question-answer-options">
+      <template>
+        <%= fields_for "survey[questions][#{question.to_param}][options][]", blank_answer_option do |answer_option_form| %>
+          <%= render "answer_option", answer_option: blank_answer_option, form: answer_option_form, question: question %>
+        <% end %>
+      </template>
+
       <div class="survey-question-answer-options-list">
-        <% question.answer_options.each_with_index do |answer_option, idx| %>
-          <%= render "answer_option", answer_option: answer_option, idx: idx %>
+        <% question.options.each do |answer_option| %>
+          <%= fields_for "survey[questions][#{question.to_param}][options][]", answer_option do |answer_option_form| %>
+            <%= render "answer_option", answer_option: answer_option, form: answer_option_form, question: question %>
+          <% end %>
         <% end %>
       </div>