Fix issues with Conditional question serialization (offered by @briri from DMPTool) in PR https://github.com/CDLUC3/dmptool/pull/667

CHANGELOG.md

@@ -12,6 +12,7 @@
 - Add pdf handling in `render_respond_to_format_with_error_message` [#3482](https://github.com/DMPRoadmap/roadmap/pull/3482)
 - Lower PostgreSQL GitHub Action Chrome Version to Address Breaking Changes Between Latest Chrome Version (134) and `/features` Tests [#3491](https://github.com/DMPRoadmap/roadmap/pull/3491)
 - Bumped dependencies via `bundle update && yarn upgrade` [#3483](https://github.com/DMPRoadmap/roadmap/pull/3483)
+- Fixed issues with Conditional Question serialization offered by @briri from PR https://github.com/CDLUC3/dmptool/pull/667 for DMPTool. There is a migration file with code for MySQL and Postgres to update the Conditions table to convert JSON Arrays in string format records in the conditions table so that they are JSON Arrays.
 
 ## v4.2.0
 

app/controllers/org_admin/questions_controller.rb

@@ -227,14 +227,12 @@ def sanitize_hash(param_conditions)
       return {} if param_conditions.empty?
 
       res = {}
-      hash_of_hashes = param_conditions[0]
-      hash_of_hashes.each do |cond_name, cond_hash|
+      param_conditions.each do |cond_id, cond_hash|
         sanitized_hash = {}
         cond_hash.each do |k, v|
-          v = ActionController::Base.helpers.sanitize(v) if k.start_with?('webhook')
-          sanitized_hash[k] = v
+          sanitized_hash[k] = k.start_with?('webhook') ? ActionController::Base.helpers.sanitize(v) : v
         end
-        res[cond_name] = sanitized_hash
+        res[cond_id] = sanitized_hash
       end
       res
     end

app/helpers/conditions_helper.rb

@@ -10,7 +10,7 @@ def remove_list(object)
     id_list = []
     plan_answers = object.answers if object.is_a?(Plan)
     plan_answers = object[:answers] if object.is_a?(Hash)
-    return [] unless plan_answers.present?
+    return [] if plan_answers.blank?
 
     plan_answers.each { |answer| id_list += answer_remove_list(answer) }
     id_list
@@ -32,7 +32,7 @@ def answer_remove_list(answer, user = nil)
           rems = cond.remove_data.map(&:to_i)
           id_list += rems
         elsif !user.nil?
-          UserMailer.question_answered(JSON.parse(cond.webhook_data), user, answer,
+          UserMailer.question_answered(cond.webhook_data, user, answer,
                                        chosen.join(' and ')).deliver_now
         end
       end
@@ -57,7 +57,7 @@ def email_trigger_list(answer)
       chosen = answer.question_option_ids.sort
       next unless chosen == opts
 
-      email_list << JSON.parse(cond.webhook_data)['email'] if action == 'add_webhook'
+      email_list << cond.webhook_data['email'] if action == 'add_webhook'
     end
     # uniq because could get same remove id from diff conds
     email_list.uniq.join(',')
@@ -70,7 +70,7 @@ def num_section_answers(plan, section)
     plan_remove_list = remove_list(plan)
     plan.answers.each do |answer|
       next unless answer.question.section_id == section.id &&
-                  !plan_remove_list.include?(answer.question_id) &&
+                  plan_remove_list.exclude?(answer.question_id) &&
                   section.question_ids.include?(answer.question_id) &&
                   answer.answered?
 
@@ -107,10 +107,9 @@ def num_section_questions(plan, section, phase = nil)
   def sections_info(plan)
     return [] if plan.nil?
 
-    info = plan.sections.map do |section|
+    plan.sections.map do |section|
       section_info(plan, section)
     end
-    info || []
   end
 
   def section_info(plan, section)
@@ -190,7 +189,7 @@ def condition_to_text(conditions)
       return_string += "<dd>#{_('Answering')} "
       return_string += opts.join(' and ')
       if cond.action_type == 'add_webhook'
-        subject_string = text_formatted(JSON.parse(cond.webhook_data)['subject'])
+        subject_string = text_formatted(cond.webhook_data['subject'])
         return_string += format(_(' will <b>send an email</b> with subject %{subject_name}'),
                                 subject_name: subject_string)
       else
@@ -209,7 +208,7 @@ def condition_to_text(conditions)
   def text_formatted(object)
     text = Question.find(object).text if object.is_a?(Integer)
     text = object if object.is_a?(String)
-    return 'type error' unless text.present?
+    return 'type error' if text.blank?
 
     cleaned_text = text
     text = ActionController::Base.helpers.truncate(cleaned_text, length: DISPLAY_LENGTH,
@@ -231,7 +230,7 @@ def conditions_to_param_form(conditions)
                           webhook_data: condition.webhook_data } }
       if param_conditions.key?(title)
         param_conditions[title].merge!(condition_hash[title]) do |_key, val1, val2|
-          if val1.is_a?(Array) && !val1.include?(val2[0])
+          if val1.is_a?(Array) && val1.exclude?(val2[0])
             val1 + val2
           else
             val1

app/javascript/src/orgAdmin/conditions/updateConditions.js

@@ -15,11 +15,6 @@ export default function updateConditions(id) {
       addLogicButton.get(0).click();
     }
   }
-  // set up form-select select boxes for condition options
-  const setSelectPicker = () => {
-    // $('.form-select.narrow').selectpicker({ width: 120 });
-    // $('.form-select.regular').selectpicker({ width: 150 });
-  };
 
   // test if a webhook is selected and set up if so
   const allowWebhook = (selectObject, webhook = false) => { // webhook false => new condition
@@ -30,8 +25,10 @@ export default function updateConditions(id) {
         // Retreive 'data-bs-target' for modal and create Jquery element
         const associatedModal = $(condition.find('.pseudo-webhook-btn').attr('data-bs-target'));
         associatedModal.modal('show');
+        condition.find('.display-if-action-remove').hide();
       } else { // condition type is remove
         condition.find('.remove-dropdown').show();
+        condition.find('.display-if-action-remove').show();
         condition.find('.webhook-replacement').hide();
       }
     } else { // loading already saved conditions
@@ -97,11 +94,10 @@ export default function updateConditions(id) {
     addLogicButton.attr('data-loaded', 'true');
     addLogicButton.addClass('disabled');
     addLogicButton.blur();
-    addLogicButton.text('Conditions');
+    addLogicButton.text('Edit Conditions');
     if (isObject(content)) {
       content.html(e.detail[0].container);
     }
-    setSelectPicker();
     webhookForm(e.detail[0].webhooks, undefined);
   });
 
@@ -114,7 +110,6 @@ export default function updateConditions(id) {
       conditionList.append(e.detail[0].attachment_partial);
       addDiv.html(e.detail[0].add_link);
       conditionList.attr('data-loaded', 'false');
-      setSelectPicker();
       const selectObject = conditionList.find('.form-select.action-type').last();
       webhookForm(undefined, selectObject);
     }

app/models/condition.rb

@@ -27,9 +27,9 @@
 # Object that represents a condition of a conditional question
 class Condition < ApplicationRecord
   belongs_to :question
-  enum action_type: %i[remove add_webhook]
-  serialize :option_list, type: Array
-  serialize :remove_data, type: Array
+  enum :action_type, { remove: 0, add_webhook: 1 }
+  serialize :option_list, type: Array, coder: JSON
+  serialize :remove_data, type: Array, coder: JSON
   serialize :webhook_data, coder: JSON
 
   # Sort order: Number ASC

app/models/question.rb

@@ -145,7 +145,7 @@ def guidance_for_org(org)
     guidances = {}
     if theme_ids.any?
       GuidanceGroup.includes(guidances: :themes)
-                   .where(org_id: org.id).each do |group|
+                   .where(org_id: org.id).find_each do |group|
         group.guidances.each do |g|
           g.themes.each do |theme|
             guidances["#{group.name} " + _('guidance on') + " #{theme.title}"] = g if theme_ids.include? theme.id
@@ -196,8 +196,8 @@ def annotations_per_org(org_id)
                                          type: Annotation.types[:example_answer])
     guidance = annotations.find_by(org_id: org_id,
                                    type: Annotation.types[:guidance])
-    example_answer = annotations.build(type: :example_answer, text: '', org_id: org_id) unless example_answer.present?
-    guidance = annotations.build(type: :guidance, text: '', org_id: org_id) unless guidance.present?
+    example_answer = annotations.build(type: :example_answer, text: '', org_id: org_id) if example_answer.blank?
+    guidance = annotations.build(type: :guidance, text: '', org_id: org_id) if guidance.blank?
     [example_answer, guidance]
   end
 
@@ -206,7 +206,7 @@ def annotations_per_org(org_id)
   # after versioning
   def update_conditions(param_conditions, old_to_new_opts, question_id_map)
     conditions.destroy_all
-    return unless param_conditions.present?
+    return if param_conditions.blank?
 
     param_conditions.each_value do |value|
       save_condition(value, old_to_new_opts, question_id_map)
@@ -221,28 +221,47 @@ def save_condition(value, opt_map, question_id_map)
     c.number = value['number']
     # question options may have changed so rewrite them
     c.option_list = value['question_option']
-    unless opt_map.blank?
-      new_question_options = c.option_list.map do |qopt|
-        opt_map[qopt]
+
+    if opt_map.present?
+      new_question_options = []
+      c.option_list.map do |qopt|
+        new_question_options << opt_map[qopt]
       end
-      c.option_list = new_question_options || []
+      c.option_list = new_question_options
     end
 
     if value['action_type'] == 'remove'
       c.remove_data = value['remove_question_id']
-      unless question_id_map.blank?
-        new_question_ids = c.remove_data.each do |qid|
-          question_id_map[qid]
+      if question_id_map.present?
+        new_question_ids = []
+        c.remove_data.map do |qid|
+          new_question_ids << question_id_map[qid]
         end
-        c.remove_data = new_question_ids || []
+        c.remove_data = new_question_ids
+      end
+
+      # Do not save the condition if the option_list or remove_data is empty
+      if c.option_list.empty? || c.remove_data.empty?
+        c.destroy
+        return
       end
     else
       c.webhook_data = {
         name: value['webhook-name'],
         email: value['webhook-email'],
         subject: value['webhook-subject'],
         message: value['webhook-message']
-      }.to_json
+      }
+
+      # Do not save the condition if the option_list or if any webhook_data fields is empty
+      if c.option_list.empty? ||
+         c.webhook_data['name'].blank? ||
+         c.webhook_data['email'].blank? ||
+         c.webhook_data['subject'].blank? ||
+         c.webhook_data['message'].blank?
+        c.destroy
+        return
+      end
     end
     c.save
   end

app/views/org_admin/conditions/_add.html.erb

@@ -1,5 +1,12 @@
 <div class="row">
   <div class="col-md-12 mt-2">
     <%= link_to _('Add condition'), new_org_admin_question_condition_path(question_id: question.id, condition_no: condition_no), remote: true, class: "add-condition" %> 
+    <p>To add a condition you must have selected an Option and Action together with
+      <ul>
+        <li>if Action is 'remove', you need to select one or more choices in Target.</li>
+        <li>if Action is 'add notification', you need to fill in all the fields in the 'Send email' popup.</li>
+      </ul>
+      Otherwise, the condition will not be saved.
+    </p>
   </div>
 </div>

app/views/org_admin/conditions/_container.html.erb

@@ -4,13 +4,14 @@
       <div class="col-md-3 mb-2 bold">
         <%= label(:text, _('Option'), class: "control-label")%>
       </div>
-      <div class="col-md-3 mb-2">
+      <div class="col-md-3 mb-2 bold">
         <%= label(:text, _('Action'), class: "control-label") %>
       </div>
       <div class="col-md-3 mb-2 bold">
-        <%= _('Remove')%>
+        <%= label(:text, _('Target'), class: "control-label") %>
       </div>
-      <div class="col-md-3">
+      <div class="col-md-3 mb-2 bold">
+        <%= label(:text, _('Remove'), class: "control-label") %>
       </div>
     </div>
     <% conditions_params =  conditions_to_param_form(conditions).sort_by { |key| key }.to_h %>

app/views/org_admin/conditions/_form.html.erb

@@ -1,35 +1,88 @@
+<% condition = condition ||= nil %>
+
 <div class="row condition-partial mb-3">
   <%
-   action_type_arr = [["removes", :remove], ["adds notification", :add_webhook]]
-   name_start = "conditions[]condition_" + condition_no.to_s
+    action_type_arr = [["removes", :remove], ["adds notification", :add_webhook]]
+    # name_start = "conditions[]condition_" + condition_no.to_s
+    name_start = "conditions[#{condition_no.to_s}]"
     remove_question_collection = later_question_list(question)
     condition_exists = local_assigns.has_key? :condition
     type_default = condition_exists ? (condition[:action_type] == "remove" ? :remove : :add_webhook) : :remove
     remove_question_group = condition_exists ?
       grouped_options_for_select(remove_question_collection, condition[:remove_question_id]) :
       grouped_options_for_select(remove_question_collection)
     multiple = (question.question_format.multiselectbox? || question.question_format.checkbox?)
+    view_email_content_info = _("Hover over the email address to view email content. To change email details you need to remove and add the condition again.")
   %>
-  <div class="col-md-3 pe-2">
-    <%= select_tag(:question_option, options_from_collection_for_select(question.question_options.sort_by(&:number), "id", "text",
-        condition_exists ? condition[:question_option_id] : question.question_options.sort_by(&:number)[0]), {class: 'form-select regular', 'data-bs-style': 'dropdown-toggle bg-white px-4 py-3', name: name_start + "[question_option][]"}) %>
-  </div>
-  <div class="col-md-3 pe-2">
-    <%= select_tag(:action_type, options_for_select(action_type_arr, type_default), {name: name_start + "[action_type]", class: 'action-type form-select narrow', 'data-bs-style': 'dropdown-toggle bg-white px-4 py-3'}) %>
-  </div>
-  <div class="col-md-3 pe-2">
-    <div class="remove-dropdown">
-      <%= select_tag(:remove_question_id, remove_question_group, {name: name_start + "[remove_question_id][]", class: 'form-select regular', multiple: true, 'data-bs-style': 'dropdown-toggle bg-white px-4 py-3'}) %>
+
+  <%# If this is a new condition then display the interactive controls. otherwise just display the logic %>
+  <% if condition.nil? %>
+  <div class="form-label bold">Add condition</div>
+  <div class="row  mb-3">
+    <div class="col-md-9 pe-2">
+      <div class="form-label bold"><%= _('Option') %></div>
+      <%= select_tag(:question_option, options_from_collection_for_select(question.question_options.sort_by(&:number), "id", "text",
+          condition_exists ? condition[:question_option_id] : question.question_options.sort_by(&:number)[0]), {class: 'form-select regular', 'data-bs-style': 'dropdown-toggle bg-white px-4 py-3', name: name_start + "[question_option][]"}) %>
     </div>
-    <div class="webhook-replacement display-off my-auto text-center">
-      <%= link_to _('Edit email'), '#' %>
+    <div class="col-md-3 pe-2">
+      <div class="form-label bold"><%= _('Action') %></div>
+      <%= select_tag(:action_type, options_for_select(action_type_arr, type_default), {name: name_start + "[action_type]", class: 'action-type form-select narrow', 'data-bs-style': 'dropdown-toggle bg-white px-4 py-3'}) %>
     </div>
   </div>
-  <%= hidden_field_tag(name_start + "[number]", condition_no) %>
-
-  <div class="col-md-3">
-    <a href="#anotherurl" class="delete-condition"><%= _('Remove') %></a>
+  <div class="row d-flex mb-3">
+    <div class="col-md-10 pe-2">
+      <div class="form-label bold display-if-action-remove"><%= _('Target') %></div>
+      <div class="remove-dropdown">
+        <%= select_tag(:remove_question_id, remove_question_group, {name: name_start + "[remove_question_id][]", class: 'form-select regular', multiple: true, 'data-bs-style': 'dropdown-toggle bg-white px-4 py-3'}) %>
+      </div>
+      <div class="webhook-replacement display-off my-auto text-center">
+        <%= link_to _('Edit email'), '#' %>
+      </div>
+      <%= hidden_field_tag(name_start + "[number]", condition_no) %>
+    </div>
+    <div class="col-md-2 align-self-center">
+      <a href="#anotherurl" class="delete-condition btn btn-primary"><%= _('Remove') %></a>
+    </div>
+    <%= render partial: 'org_admin/conditions/webhook_form', locals: {name_start: name_start, condition_no: condition_no} %>
   </div>
+  <% else %>
+    <%
+    qopt = condition[:question_option_id].any? ? QuestionOption.find_by(id: condition[:question_option_id].first): nil
+    rquesArray = condition[:remove_question_id].any? ? Question.where(id: condition[:remove_question_id]) : nil
+    %>
+    <div class="col-md-3 pe-2">
+      <%= qopt[:text]&.slice(0, 25) %>
+      <%= hidden_field_tag(name_start + "[question_option][]", condition[:question_option_id]) %>
+    </div>
+    <div class="col-md-3 pe-2">
+      <%= condition[:action_type] == 'remove' ? 'Remove' : 'Email' %>
+      <%= hidden_field_tag(name_start + "[action_type]", condition[:action_type]) %>
+    </div>
+    <div class="col-md-3 pe-2">
+      <% if !rquesArray.nil? %>
+        <% rquesArray.each do |rques| %>
+           Question <%= rques[:number] %>: <%= rques.text.gsub(%r{</?p>}, '').slice(0, 50) %>
+           <%= '...' if rques.text.gsub(%r{</?p>}, '').length > 50 %>
+           <br>
+        <% end %>
+        <%= hidden_field_tag(name_start + "[remove_question_id][]", condition[:remove_question_id]) %>
+      <% else %>
+        <%
+        hook_tip = "Name: #{condition[:webhook_data]['name']}\nEmail: #{condition[:webhook_data]['email']}\n"
+        hook_tip += "Subject: #{condition[:webhook_data]['subject']}\nMessage: #{condition[:webhook_data]['message']}"
+        %>
+        <span title="<%= hook_tip %>"><%= condition[:webhook_data]['email'] %></span>
+        <br>(<%= view_email_content_info %>)
 
-  <%= render partial: 'org_admin/conditions/webhook_form', locals: {name_start: name_start, condition_no: condition_no} %>
+        <%= hidden_field_tag(name_start + "[webhook-email]", condition[:webhook_data]['email']) %>
+        <%= hidden_field_tag(name_start + "[webhook-name]", condition[:webhook_data]['name']) %>
+        <%= hidden_field_tag(name_start + "[webhook-subject]", condition[:webhook_data]['subject']) %>
+        <%= hidden_field_tag(name_start + "[webhook-message]", condition[:webhook_data]['message']) %>
+      <% end %>
+      <%= hidden_field_tag(name_start + "[number]", condition_no) %>
+    </div>
+    <div class="col-md-3">
+      <a href="#anotherurl" class="delete-condition"><%= _('Remove') %></a>
+    </div>
+  <% end %>
 </div>