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,9 @@
 - 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.
+- Refactor `org_admin/conditions/_form.html.erb` [#3502](https://github.com/DMPRoadmap/roadmap/pull/3502)
+- Refactor `Question.save_condition` [#3501](https://github.com/DMPRoadmap/roadmap/pull/3501)
 
 ## v4.2.0
 

app/controllers/org_admin/questions_controller.rb

@@ -211,30 +211,32 @@ def destroy
 
     private
 
-    # param_conditions looks like:
-    #   [
-    #     {
-    #       "conditions_N" => {
-    #         name: ...
-    #         subject ...
-    #         ...
-    #       }
-    #       ...
-    #     }
-    #   ]
+    # param_conditions is a hash of a hash like this (example where
+    # action_types is "remove" and "add_webhook" respectively):
+    # Parameters:
+    # {"0"=>{"question_option"=>["212159"],
+    #        "action_type"=>"remove",
+    #        "remove_question_id"=>["191471 191472"],
+    #        "number"=>"0"},
+    # "1"=>{"question_option"=>["212160"],
+    #       "action_type"=>"add_webhook",
+    #        "number"=>"1",
+    #        "webhook-name"=>"DMP Admin",
+    #        "webhook-email"=>"dmp-admin@example.com",
+    #        "webhook-subject"=>"Woodcote cillum quis elit consectetur epsom",
+    #        "webhook-message"=>"Labore ut epsom downs exercitation ...."}
+    #  }
     def sanitize_hash(param_conditions)
       return {} if param_conditions.nil?
       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,48 +206,45 @@ 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)
     end
   end
 
-  # rubocop:disable Metrics/MethodLength, Metrics/AbcSize
-  # rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
+  # rubocop:disable Metrics/AbcSize, Metrics/MethodLength
   def save_condition(value, opt_map, question_id_map)
     c = conditions.build
     c.action_type = value['action_type']
     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]
-      end
-      c.option_list = new_question_options || []
+    c.option_list = handle_option_list(value, opt_map)
+    # Do not save the condition if the option_list is empty
+    if c.option_list.empty?
+      c.destroy
+      return
     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]
-        end
-        c.remove_data = new_question_ids || []
+      c.remove_data = handle_remove_data(value, question_id_map)
+      # Do not save the condition if remove_data is empty
+      if 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
+      c.webhook_data = handle_webhook_data(value)
+      # Do not save the condition if webhook_data is nil
+      if c.webhook_data.nil?
+        c.destroy
+        return
+      end
     end
     c.save
   end
-  # rubocop:enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
-  # rubocop:enable Metrics/MethodLength, Metrics/AbcSize
+  # rubocop:enable Metrics/AbcSize, Metrics/MethodLength
 
   private
 
@@ -274,4 +271,33 @@ def check_remove_conditions
     end
   end
   # rubocop:enable Metrics/AbcSize
+
+  def handle_option_list(value, opt_map)
+    if opt_map.present?
+      value['question_option'].map { |qopt| opt_map[qopt] }
+    else
+      value['question_option']
+    end
+  end
+
+  def handle_remove_data(value, question_id_map)
+    if question_id_map.present?
+      value['remove_question_id'].map { |qid| question_id_map[qid] }
+    else
+      value['remove_question_id']
+    end
+  end
+
+  def handle_webhook_data(value)
+    # return nil if any of the webhook fields are blank
+    return if %w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| value[key].blank? }
+
+    # else return the constructed webhook_data hash
+    {
+      name: value['webhook-name'],
+      email: value['webhook-email'],
+      subject: value['webhook-subject'],
+      message: value['webhook-message']
+    }
+  end
 end

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

@@ -1,5 +1,13 @@
 <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/_existing_condition_display.erb

@@ -0,0 +1,41 @@
+    <%
+    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
+    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">
+      <%= 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']}\n"
+          hook_tip += "#{_('Email')}: #{condition[:webhook_data]['email']}\n"
+          hook_tip += "#{_('Subject')}: #{condition[:webhook_data]['subject']}\n"
+          hook_tip += "#{_('Message')}: #{condition[:webhook_data]['message']}"
+        %>
+        <span title="<%= hook_tip %>"><%= condition[:webhook_data]['email'] %></span>
+        <br>(<%= view_email_content_info %>)
+
+        <%= 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>