Fix issues with Conditional question serialization (offered by @briri

    from DMPTool).nBased on DMPtool commit CDLUC3#667.

    Changes proposed in this PR (text from cited PR):
       - 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.
       - 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 conditions for a given question.
       - 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.
       - We allow only one question option to be selected when adding a Condition unlike inthe DMPTool version because experience with multiple options chosen has been problematic and buggy when used by users in DMPOnline.

CHANGELOG.md

@@ -2,7 +2,7 @@
 
 - Fixed a bug in the deep copy of plans where the old identifier was being copied into the new plan. We now copy the generated id of the new plan to the identifier field.
 - Fixed bar chart click function in the Usage dashboard (GitHub issue #3443)
-
+- 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.
 
 **Note this upgrade is mainly a migration from Bootstrap 3 to Bootstrap 5.** 
 

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/javascript/src/orgAdmin/conditions/updateConditions.js

@@ -97,7 +97,7 @@ 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);
     }

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,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 'removes', 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,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,77 @@
       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 d-flex 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 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>
-  <%= 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
+    rquesArray = condition[:remove_question_id].any? ? Question.where(id: condition[:remove_question_id]) : 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/conditions/_webhook_form.html.erb

@@ -9,25 +9,25 @@
       <div class="modal-body mx-3">
         <div class="md-form mb-5 prefix grey-text">
           <i class="fa-solid fa-user"></i>
-          <%= label_tag name_start + "[webhook-name]", _('Name'), {'data-error': 'wrong', 'data-success': 'right', class: 'form-label'} %>
+          <%= label_tag name_start + "[webhook-name]", _('Name'), {'data-error': 'wrong', 'data-success': 'right', class: 'form-label', "aria-required": true, required: true} %>
           <%= text_field_tag name_start + "[webhook-name]", nil, class: 'form-control validate' %>
         </div>
 
         <div class="md-form mb-5 prefix grey-text">
         <i class="fa-solid fa-envelope"></i> <!-- mdbootstrap should have treatment for bad input for this -->
-          <%= label_tag name_start + "[webhook-email]", _('Email'), {'data-error': 'wrong', 'data-success': 'right', class: 'form-label'} %>
+          <%= label_tag name_start + "[webhook-email]", _('Email'), {'data-error': 'wrong', 'data-success': 'right', class: 'form-label', "aria-required": true, required: true} %>
           <%= email_field_tag name_start + "[webhook-email]", nil, { placeholder: _('recipient_email@example.com'), class: 'form-control validate'} %>
         </div>
 
         <div class="md-form mb-5 prefix grey-text">
           <i class="fa-solid fa-tag"></i>
-          <%= label_tag name_start + "[webhook-subject]", _('Subject'), {'data-error': 'wrong', 'data-success': 'right', class: 'form-label'} %>
+          <%= label_tag name_start + "[webhook-subject]", _('Subject'), {'data-error': 'wrong', 'data-success': 'right', class: 'form-label', "aria-required": true, required: true} %>
           <%= text_field_tag name_start + "[webhook-subject]", nil, class: 'form-control validate' %>
         </div>
 
         <div class="md-form prefix grey-text">
           <i class="fa-solid fa-pencil"></i>
-          <%= label_tag name_start + "[webhook-message]", _('Message'), {'data-error': 'wrong', 'data-success': 'right', class: 'form-label'} %>
+          <%= label_tag name_start + "[webhook-message]", _('Message'), {'data-error': 'wrong', 'data-success': 'right', class: 'form-label', "aria-required": true, required: true}%>
           <%= text_area_tag name_start + "[webhook-message]", nil, { placeholder: _('Email content'), class: 'form-control md-textarea', rows: 4 } %>
         </div>
       </div>