Skip to content

Commit

Permalink
Fix issues with Conditional question serialization (offered by @briri
Browse files Browse the repository at this point in the history
        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
           - The user canno longer edit a condition. They need to remove it
             and create a new condition. This applies to the email for 'add
    notifications' too.

        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 of action type 'remove' 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.
  • Loading branch information
John Pinto committed Jan 29, 2025
1 parent 994bb60 commit f105553
Show file tree
Hide file tree
Showing 12 changed files with 235 additions and 70 deletions.
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.**

Expand Down
8 changes: 3 additions & 5 deletions app/controllers/org_admin/questions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
23 changes: 12 additions & 11 deletions app/helpers/conditions_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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(',')
Expand All @@ -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?

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand All @@ -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
Expand All @@ -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
Expand Down
9 changes: 1 addition & 8 deletions app/javascript/src/orgAdmin/conditions/updateConditions.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -97,11 +92,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);
});

Expand All @@ -114,7 +108,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);
}
Expand Down
8 changes: 4 additions & 4 deletions app/models/condition.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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) }
Expand Down
45 changes: 32 additions & 13 deletions app/models/question.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand All @@ -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)
Expand All @@ -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
Expand Down
7 changes: 7 additions & 0 deletions app/views/org_admin/conditions/_add.html.erb
Original file line number Diff line number Diff line change
@@ -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>
7 changes: 4 additions & 3 deletions app/views/org_admin/conditions/_container.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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 %>
Expand Down
91 changes: 72 additions & 19 deletions app/views/org_admin/conditions/_form.html.erb
Original file line number Diff line number Diff line change
@@ -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.")
%>
<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"><%= _('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>
<%= 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><span class="webhook-replacement display-off my-auto text-center" style="display: block;">
<%= link_to _('Edit 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>
Loading

0 comments on commit f105553

Please sign in to comment.