Fix issues with Conditional question serialization (offered by @briri

from DMPTool).

Based on DMPtool commit CDLUC3#667.

Changes proposed in this PR (text from cited PR):

    Run SQL script to convert existing records in the conditions table so that they are JSON Arrays (see query below)
    Updated the form partials in app/views/org_admin/conditions/_form.erb.rb to properly send condition data to the controller.
    Removed all JSON.parse calls in the app/helpers/conditions.rb helper

Made the following changes to simplify this patch and to make it a little more user friendly:

    The "Add Conditions" button will now say "Edit Conditions" if there are any.
    Updated the column heading over the "thing that happens when the condition is met" from "Remove" to "Target" since the content of the column can either be questions being removed or an email notification being sent.
    Conditions can be added or removed (not updated anymore)
    Hovering over the email of an existing condition displays a tooltip that shows the email message, subject, etc.

Needed to update underlying data within the table so that they are JSON arrays using MySQL or Postgres scripts in db/migrate-sql-scripts folder:
20241219_UpdateConditions-MySql.sql.txt
20241219_UpdateConditions-Postgres.sql.txt
(Note. The scripts have .txt endings to be able to get round .sql in
.gitignore.)

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,11 @@ def num_section_questions(plan, section, phase = nil)
   def sections_info(plan)
     return [] if plan.nil?
 
-    info = plan.sections.map do |section|
-      section_info(plan, section)
+    info = []
+    plan.sections.each do |section|
+      info.push(section_info(plan, section))
     end
-    info || []
+    info
   end
 
   def section_info(plan, section)
@@ -190,7 +191,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 +210,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 +232,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
@@ -249,7 +250,7 @@ def conditions_to_param_form(conditions)
   def webhook_hash(conditions)
     web_hash = {}
     param_conditions = conditions_to_param_form(conditions)
-    param_conditions.each_value do |params|
+    param_conditions.each do |_title, params|
       web_hash.merge!(params[:number] => params[:webhook_data])
     end
     web_hash

app/models/condition.rb

@@ -27,10 +27,10 @@
 # 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
-  serialize :webhook_data, coder: JSON
+  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, type: Hash, coder: JSON
 
   # Sort order: Number ASC
   default_scope { 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,9 +206,9 @@ 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|
+    param_conditions.each do |_key, value|
       save_condition(value, old_to_new_opts, question_id_map)
     end
   end
@@ -221,28 +221,31 @@ 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.each 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.each do |qid|
+          new_question_ids << question_id_map[qid]
         end
-        c.remove_data = new_question_ids || []
+        c.remove_data = new_question_ids
       end
     else
       c.webhook_data = {
         name: value['webhook-name'],
         email: value['webhook-email'],
         subject: value['webhook-subject'],
         message: value['webhook-message']
-      }.to_json
+      }
     end
     c.save
   end

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,7 +1,10 @@
+<% 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
@@ -10,26 +13,79 @@
       grouped_options_for_select(remove_question_collection)
     multiple = (question.question_format.multiselectbox? || question.question_format.checkbox?)
   %>
-  <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'}) %>
+
+  <%# 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="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>
-  <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'}) %>
+  <div class="row mb-3">
+    <div class="col-md-10 pe-2">
+      <div class="form-label bold"><%= _('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="webhook-replacement display-off my-auto text-center">
-      <%= link_to _('Edit email'), '#' %>
+    <div class="col-md-2 d-flex align-items-center text-end">
+      <a href="#anotherurl" class="delete-condition"><%= _('Remove') %></a>
     </div>
+    <%= render partial: 'org_admin/conditions/webhook_form', locals: {name_start: name_start, condition_no: condition_no} %>
   </div>
-  <%= hidden_field_tag(name_start + "[number]", condition_no) %>
+  <% else %>
+    <%
+    qopt = condition[:question_option_id].any? ? QuestionOption.find_by(id: condition[:question_option_id].first): nil
+    # qopts = condition[:question_option_id].any? ? QuestionOption.find_by(id: condition[:question_option_id]): []
+    rquesArray = condition[:remove_question_id].any? ? Question.where(id: condition[:remove_question_id]) : nil
+    # rques = condition[:remove_question_id].any? ? Question.find_by(id: condition[:remove_question_id].first) : nil
+    %>
+    <div class="row condition-partial mb-3">
+    <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>
+        <%= 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 %>
+
+    </div>
+    <%= hidden_field_tag(name_start + "[number]", condition_no) %>
 
-  <div class="col-md-3">
-    <a href="#anotherurl" class="delete-condition"><%= _('Remove') %></a>
+    <div class="col-md-3">
+      <a href="#anotherurl" class="delete-condition"><%= _('Remove') %></a>
+    </div>
   </div>
-
-  <%= render partial: 'org_admin/conditions/webhook_form', locals: {name_start: name_start, condition_no: condition_no} %>
+  <% end %>
 </div>

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

@@ -45,8 +45,11 @@
         <%= render "/org_admin/question_options/option_fields", f: f, q: question %>
         <!--display for selecting comment box when multiple choice is being used-->
       </div>
+
       <% if question.id != nil && question.question_options[0].text != nil %>
-        <%= link_to _('Add Conditions'), org_admin_question_open_conditions_path(question_id: question.id, conditions: conditions), class: "add-logic btn btn-secondary", 'data-loaded': (conditions.size > 0).to_s, remote: true %>
+        <% cond_lbl = (!conditions.nil? && conditions.any?) ? 'Edit Conditions' : 'Add Conditions' %>
+        <%= cond_lbl %>
+        <%= link_to cond_lbl, org_admin_question_open_conditions_path(question_id: question.id, conditions: conditions), class: "add-logic btn btn-default", 'data-loaded': (conditions.size > 0).to_s, remote: true %>
         <div id="content" class="col-md-offset-2">
           <p>
             <%= render partial: 'org_admin/conditions/container', locals: { f: f, question: question, conditions: conditions } %>

db/migrate-sql-scripts/20241219_UpdateConditions-MySql.sql.txt

@@ -0,0 +1,34 @@
+UPDATE conditions
+SET
+    option_list = CONCAT (
+        CONCAT (
+            '[',
+            REPLACE (
+                REPLACE (
+                    REPLACE (option_list, '---\n- \'', '"'),
+                    '\'\n- \'',
+                    '","'
+                ),
+                '\'\n',
+                '"'
+            )
+        ),
+        ']'
+    ),
+    remove_data = CONCAT (
+        CONCAT (
+            '[',
+            REPLACE (
+                REPLACE (
+                    REPLACE (remove_data, '---\n- \'', '"'),
+                    '\'\n- \'',
+                    '","'
+                ),
+                '\'\n',
+                '"'
+            )
+        ),
+        ']'
+    )
+where
+    option_list like '---%';