From 0c2e2d2dc0ddc39c4951413caf834e5c62db6137 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 7 Apr 2025 13:04:05 -0600 Subject: [PATCH 01/10] Document callers of conditions/form partial Adding a comment to help with maintainability of `app/views/org_admin/conditions/_form.html.erb` --- app/views/org_admin/conditions/_form.html.erb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/views/org_admin/conditions/_form.html.erb b/app/views/org_admin/conditions/_form.html.erb index 3a7877f098..e42c2c271e 100644 --- a/app/views/org_admin/conditions/_form.html.erb +++ b/app/views/org_admin/conditions/_form.html.erb @@ -1,3 +1,8 @@ +<%# This partial is called from the following files: + - app/controllers/org_admin/conditions_controller.rb + - app/views/org_admin/conditions/_container.html.erb + %> + <% condition ||= nil %>
From 97677d4d8ed9833389233d70b663889c700e934d Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 7 Apr 2025 11:30:01 -0600 Subject: [PATCH 02/10] Remove commented-out code --- app/views/org_admin/conditions/_form.html.erb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/views/org_admin/conditions/_form.html.erb b/app/views/org_admin/conditions/_form.html.erb index e42c2c271e..4bd0d28e89 100644 --- a/app/views/org_admin/conditions/_form.html.erb +++ b/app/views/org_admin/conditions/_form.html.erb @@ -8,7 +8,6 @@
<% 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 From c7abc11e3ade9ccb56ea40f9cc629d3ed9a00834 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 7 Apr 2025 11:28:27 -0600 Subject: [PATCH 03/10] Remove unused variable from conditions/form It appears that the following previous commit removed the need for this variable: https://github.com/DMPRoadmap/roadmap/commit/da4033b2be1a6ac4d4e59761104bbd63b84526db#diff-9b4ef24d6aebd8b59257b1df4b7bd77adc5873a46a803a69a7ff146266d9658fR3-L15 --- app/views/org_admin/conditions/_form.html.erb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/views/org_admin/conditions/_form.html.erb b/app/views/org_admin/conditions/_form.html.erb index 4bd0d28e89..0ed43b039e 100644 --- a/app/views/org_admin/conditions/_form.html.erb +++ b/app/views/org_admin/conditions/_form.html.erb @@ -15,7 +15,6 @@ 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.") %> From b2eaac4f01924a91d30d07d0bb03d5a26c8d7618 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 7 Apr 2025 14:59:35 -0600 Subject: [PATCH 04/10] Replace `condition_exists` w/ `condition.present?` - https://github.com/DMPRoadmap/roadmap/pull/3497 introduces `<% condition ||= nil %>` on line 6. - This refactor replaces the usage of `condition_exists` with `condition.present?` - `app/views/org_admin/conditions/_container.html.erb` appears to be the only `app/views/org_admin/conditions/_form.html.erb` that passes the `locals : { condition` variable. The `<% condition ||= nil %>` code on line 6 should be sufficient for performing this check. - Additionally, if `condition == nil` and `condition_exists == true` ever occurred, then we would encounter `NoMethodError` exceptions on lines 13 and 27. --- app/views/org_admin/conditions/_form.html.erb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/app/views/org_admin/conditions/_form.html.erb b/app/views/org_admin/conditions/_form.html.erb index 0ed43b039e..7c76a2bf06 100644 --- a/app/views/org_admin/conditions/_form.html.erb +++ b/app/views/org_admin/conditions/_form.html.erb @@ -10,9 +10,8 @@ action_type_arr = [["removes", :remove], ["adds notification", :add_webhook]] 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 ? + type_default = condition.present? ? (condition[:action_type] == "remove" ? :remove : :add_webhook) : :remove + remove_question_group = condition.present? ? grouped_options_for_select(remove_question_collection, condition[:remove_question_id]) : grouped_options_for_select(remove_question_collection) 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.") @@ -25,7 +24,7 @@
<%= _('Option') %>
<%= 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][]"}) %> + condition.present? ? 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][]"}) %>
<%= _('Action') %>
From e651186c517fde909d24cdac1dfc52b55127c739 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 7 Apr 2025 15:08:07 -0600 Subject: [PATCH 05/10] Remove redundant `type_default` variable - `<%= select_tag(:action_type, options_for_select(action_type_arr, type_default)` was only being executed when `if condition.nil?` was true (now line 20). Additionally, that was the only line that was using the `type_default` variable. - Thus, no conditional is required for the assigning of `type_default` and we can just use `:remove` explicitly on line 30 now. --- app/views/org_admin/conditions/_form.html.erb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/views/org_admin/conditions/_form.html.erb b/app/views/org_admin/conditions/_form.html.erb index 7c76a2bf06..69effd0053 100644 --- a/app/views/org_admin/conditions/_form.html.erb +++ b/app/views/org_admin/conditions/_form.html.erb @@ -10,7 +10,6 @@ action_type_arr = [["removes", :remove], ["adds notification", :add_webhook]] name_start = "conditions[#{condition_no.to_s}]" remove_question_collection = later_question_list(question) - type_default = condition.present? ? (condition[:action_type] == "remove" ? :remove : :add_webhook) : :remove remove_question_group = condition.present? ? grouped_options_for_select(remove_question_collection, condition[:remove_question_id]) : grouped_options_for_select(remove_question_collection) @@ -28,7 +27,7 @@
<%= _('Action') %>
- <%= 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'}) %> + <%= select_tag(:action_type, options_for_select(action_type_arr, :remove), {name: name_start + "[action_type]", class: 'action-type form-select narrow', 'data-bs-style': 'dropdown-toggle bg-white px-4 py-3'}) %>
From dc04625e834fb07153c668f387714a62f244eb51 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 7 Apr 2025 15:09:53 -0600 Subject: [PATCH 06/10] Remove redundant conditional check The modified code was and is only executed when `condition.nil?` is true (line 20). Thus, the ternary operator always evaluated to the else. --- app/views/org_admin/conditions/_form.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/org_admin/conditions/_form.html.erb b/app/views/org_admin/conditions/_form.html.erb index 69effd0053..459e191c48 100644 --- a/app/views/org_admin/conditions/_form.html.erb +++ b/app/views/org_admin/conditions/_form.html.erb @@ -23,7 +23,7 @@
<%= _('Option') %>
<%= select_tag(:question_option, options_from_collection_for_select(question.question_options.sort_by(&:number), "id", "text", - condition.present? ? 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][]"}) %> + 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][]"}) %>
<%= _('Action') %>
From 5d0ec1e4d781b90dd3972aeea7c4373bb58a21c1 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Tue, 8 Apr 2025 09:06:25 -0600 Subject: [PATCH 07/10] Remove redundant conditional check - Line 35 is the only that uses the `remove_question_group` variable. However, line 35 is only executed when `condition.nil? == true` (line 18). --- app/views/org_admin/conditions/_form.html.erb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/views/org_admin/conditions/_form.html.erb b/app/views/org_admin/conditions/_form.html.erb index 459e191c48..32c3068127 100644 --- a/app/views/org_admin/conditions/_form.html.erb +++ b/app/views/org_admin/conditions/_form.html.erb @@ -10,9 +10,7 @@ action_type_arr = [["removes", :remove], ["adds notification", :add_webhook]] name_start = "conditions[#{condition_no.to_s}]" remove_question_collection = later_question_list(question) - remove_question_group = condition.present? ? - grouped_options_for_select(remove_question_collection, condition[:remove_question_id]) : - grouped_options_for_select(remove_question_collection) + remove_question_group = grouped_options_for_select(remove_question_collection) 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.") %> From 3a1106770b13f10502486f5cdf15bc79fb2b12db Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Tue, 8 Apr 2025 10:35:10 -0600 Subject: [PATCH 08/10] Refactor: Split conditions/_form into two This change refactors `app/views/org_admin/conditions/_form.html.erb`. The change moves the logic for new conditions to `app/views/org_admin/conditions/_new_condition_form.erb` and existing conditions to `app/views/org_admin/conditions/_existing_condition_display.erb`. --- .../_existing_condition_display.erb | 38 +++++++++ app/views/org_admin/conditions/_form.html.erb | 83 +++++-------------- .../conditions/_new_condition_form.erb | 28 +++++++ 3 files changed, 85 insertions(+), 64 deletions(-) create mode 100644 app/views/org_admin/conditions/_existing_condition_display.erb create mode 100644 app/views/org_admin/conditions/_new_condition_form.erb diff --git a/app/views/org_admin/conditions/_existing_condition_display.erb b/app/views/org_admin/conditions/_existing_condition_display.erb new file mode 100644 index 0000000000..21705f29aa --- /dev/null +++ b/app/views/org_admin/conditions/_existing_condition_display.erb @@ -0,0 +1,38 @@ + <% + 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 + %> +
+ <%= qopt[:text]&.slice(0, 25) %> + <%= hidden_field_tag(name_start + "[question_option][]", condition[:question_option_id]) %> +
+
+ <%= condition[:action_type] == 'remove' ? 'Remove' : 'Email' %> + <%= hidden_field_tag(name_start + "[action_type]", condition[:action_type]) %> +
+
+ <% if !rquesArray.nil? %> + <% rquesArray.each do |rques| %> + Question <%= rques[:number] %>: <%= rques.text.gsub(%r{}, '').slice(0, 50) %> + <%= '...' if rques.text.gsub(%r{}, '').length > 50 %> +
+ <% 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']}" + %> + <%= condition[:webhook_data]['email'] %> +
(<%= 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) %> +
+
+ <%= _('Remove') %> +
diff --git a/app/views/org_admin/conditions/_form.html.erb b/app/views/org_admin/conditions/_form.html.erb index 32c3068127..2b6e032de5 100644 --- a/app/views/org_admin/conditions/_form.html.erb +++ b/app/views/org_admin/conditions/_form.html.erb @@ -16,72 +16,27 @@ <%# If this is a new condition then display the interactive controls. otherwise just display the logic %> <% if condition.nil? %> -
Add condition
-
-
-
<%= _('Option') %>
- <%= select_tag(:question_option, options_from_collection_for_select(question.question_options.sort_by(&:number), "id", "text", - 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][]"}) %> -
-
-
<%= _('Action') %>
- <%= select_tag(:action_type, options_for_select(action_type_arr, :remove), {name: name_start + "[action_type]", class: 'action-type form-select narrow', 'data-bs-style': 'dropdown-toggle bg-white px-4 py-3'}) %> -
-
-
-
-
<%= _('Target') %>
-
- <%= 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'}) %> -
-
- <%= link_to _('Edit email'), '#' %> -
- <%= hidden_field_tag(name_start + "[number]", condition_no) %> -
-
- <%= _('Remove') %> -
- <%= render partial: 'org_admin/conditions/webhook_form', locals: {name_start: name_start, condition_no: condition_no} %> -
+ <%= render partial: 'org_admin/conditions/new_condition_form', + locals: { action_type_arr: action_type_arr, + condition_no: condition_no, + question: question, + name_start: name_start, + remove_question_group: remove_question_group, + view_email_content_info: view_email_content_info + } + %> + <% 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 + <%= render partial: 'org_admin/conditions/existing_condition_display', + locals: { action_type_arr: action_type_arr, + condition: condition, + condition_no: condition_no, + name_start: name_start, + question: question, + remove_question_group: remove_question_group, + view_email_content_info: view_email_content_info + } %> -
- <%= qopt[:text]&.slice(0, 25) %> - <%= hidden_field_tag(name_start + "[question_option][]", condition[:question_option_id]) %> -
-
- <%= condition[:action_type] == 'remove' ? 'Remove' : 'Email' %> - <%= hidden_field_tag(name_start + "[action_type]", condition[:action_type]) %> -
-
- <% if !rquesArray.nil? %> - <% rquesArray.each do |rques| %> - Question <%= rques[:number] %>: <%= rques.text.gsub(%r{}, '').slice(0, 50) %> - <%= '...' if rques.text.gsub(%r{}, '').length > 50 %> -
- <% 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']}" - %> - <%= condition[:webhook_data]['email'] %> -
(<%= 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) %> -
-
- <%= _('Remove') %> -
<% end %>
diff --git a/app/views/org_admin/conditions/_new_condition_form.erb b/app/views/org_admin/conditions/_new_condition_form.erb new file mode 100644 index 0000000000..cf38cb3e37 --- /dev/null +++ b/app/views/org_admin/conditions/_new_condition_form.erb @@ -0,0 +1,28 @@ +
Add condition
+
+
+
<%= _('Option') %>
+ <%= select_tag(:question_option, options_from_collection_for_select(question.question_options.sort_by(&:number), "id", "text", + 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][]"}) %> +
+
+
<%= _('Action') %>
+ <%= select_tag(:action_type, options_for_select(action_type_arr, :remove), {name: name_start + "[action_type]", class: 'action-type form-select narrow', 'data-bs-style': 'dropdown-toggle bg-white px-4 py-3'}) %> +
+
+
+
+
<%= _('Target') %>
+
+ <%= 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'}) %> +
+
+ <%= link_to _('Edit email'), '#' %> +
+ <%= hidden_field_tag(name_start + "[number]", condition_no) %> +
+
+ <%= _('Remove') %> +
+ <%= render partial: 'org_admin/conditions/webhook_form', locals: {name_start: name_start, condition_no: condition_no} %> +
From 4724bc7e499878208a99cc3dafa53329b457fe6b Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Tue, 8 Apr 2025 11:01:46 -0600 Subject: [PATCH 09/10] Cleanup after `condtions/_form` refactor - ` action_type_arr` and `remove_question_group` are only needed in `conditions/_new_condition_form.erb`. The assignments have been moved directly to that partial. - - ` view_email_content_info` is only needed in `conditions/_new_condition_form.erb`. The assignment has been moved directly to that partial. --- .../_existing_condition_display.erb | 1 + app/views/org_admin/conditions/_form.html.erb | 31 ++++++------------- .../conditions/_new_condition_form.erb | 6 ++++ 3 files changed, 17 insertions(+), 21 deletions(-) diff --git a/app/views/org_admin/conditions/_existing_condition_display.erb b/app/views/org_admin/conditions/_existing_condition_display.erb index 21705f29aa..b2a76765f4 100644 --- a/app/views/org_admin/conditions/_existing_condition_display.erb +++ b/app/views/org_admin/conditions/_existing_condition_display.erb @@ -1,6 +1,7 @@ <% 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.") %>
<%= qopt[:text]&.slice(0, 25) %> diff --git a/app/views/org_admin/conditions/_form.html.erb b/app/views/org_admin/conditions/_form.html.erb index 2b6e032de5..0fe405972b 100644 --- a/app/views/org_admin/conditions/_form.html.erb +++ b/app/views/org_admin/conditions/_form.html.erb @@ -3,40 +3,29 @@ - app/views/org_admin/conditions/_container.html.erb %> -<% condition ||= nil %> -
<% - action_type_arr = [["removes", :remove], ["adds notification", :add_webhook]] + condition ||= nil name_start = "conditions[#{condition_no.to_s}]" - remove_question_collection = later_question_list(question) - remove_question_group = grouped_options_for_select(remove_question_collection) - 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.") %> <%# If this is a new condition then display the interactive controls. otherwise just display the logic %> <% if condition.nil? %> <%= render partial: 'org_admin/conditions/new_condition_form', - locals: { action_type_arr: action_type_arr, - condition_no: condition_no, - question: question, + locals: { condition_no: condition_no, name_start: name_start, - remove_question_group: remove_question_group, - view_email_content_info: view_email_content_info + question: question } %> <% else %> - <%= render partial: 'org_admin/conditions/existing_condition_display', - locals: { action_type_arr: action_type_arr, - condition: condition, - condition_no: condition_no, - name_start: name_start, - question: question, - remove_question_group: remove_question_group, - view_email_content_info: view_email_content_info - } - %> + <%= render partial: 'org_admin/conditions/existing_condition_display', + locals: { condition: condition, + condition_no: condition_no, + name_start: name_start, + question: question + } + %> <% end %>
diff --git a/app/views/org_admin/conditions/_new_condition_form.erb b/app/views/org_admin/conditions/_new_condition_form.erb index cf38cb3e37..49222c9de6 100644 --- a/app/views/org_admin/conditions/_new_condition_form.erb +++ b/app/views/org_admin/conditions/_new_condition_form.erb @@ -1,3 +1,9 @@ + <% + action_type_arr = [["removes", :remove], ["adds notification", :add_webhook]] + remove_question_collection = later_question_list(question) + remove_question_group = grouped_options_for_select(remove_question_collection) + %> +
Add condition
From 8f952be0d6747bead18056ca9896b3fd4107ce46 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Wed, 9 Apr 2025 08:49:19 -0600 Subject: [PATCH 10/10] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0af92cb483..d744dfd3b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ - 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) ## v4.2.0