From 739aad0e1e51b2031d67984d30a084468ecff191 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Fri, 4 Apr 2025 10:38:10 -0600 Subject: [PATCH 1/6] Refactor mapping of `remove_data` & `option_list` This change uses `.map` to refactor/simplify the mapping of `option_list` and `remove_data`. It also removes the needed for temporary arrays. --- app/models/question.rb | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/app/models/question.rb b/app/models/question.rb index 97d41237db..58bfa7533f 100644 --- a/app/models/question.rb +++ b/app/models/question.rb @@ -221,24 +221,11 @@ 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'] - - 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 - end + c.option_list = c.option_list.map { |qopt| opt_map[qopt] } if opt_map.present? if value['action_type'] == 'remove' c.remove_data = value['remove_question_id'] - 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 - end + c.remove_data = c.remove_data.map { |qid| question_id_map[qid] } if question_id_map.present? # Do not save the condition if the option_list or remove_data is empty if c.option_list.empty? || c.remove_data.empty? From f6a232b47f2c8ff3ee594a3670130377652597ee Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Fri, 4 Apr 2025 11:41:53 -0600 Subject: [PATCH 2/6] Refactor webhook_data validation and construction Refactored `save_condition` method via new `handle_webhook_data` helper method. - Helper method returns nil if any of the required webhook_data fields are absent. Else, it constructs and returns the webhook_data hash - Returning nil when any fields are absent enables us to simplify the if check that immediately follows (now line 240) to `if c.option_list.empty? || c.webhook_data.nil?` - Overall, these changes simplify the `save_condition` method and even remove a previous rubocop offense. --- app/models/question.rb | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/app/models/question.rb b/app/models/question.rb index 58bfa7533f..cdaf3df006 100644 --- a/app/models/question.rb +++ b/app/models/question.rb @@ -213,7 +213,7 @@ def update_conditions(param_conditions, old_to_new_opts, question_id_map) end end - # rubocop:disable Metrics/MethodLength, Metrics/AbcSize + # rubocop:disable Metrics/AbcSize # rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity def save_condition(value, opt_map, question_id_map) c = conditions.build @@ -233,19 +233,10 @@ def save_condition(value, opt_map, question_id_map) return end else - c.webhook_data = { - name: value['webhook-name'], - email: value['webhook-email'], - subject: value['webhook-subject'], - message: value['webhook-message'] - } - - # 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.webhook_data = handle_webhook_data(value) + + # Do not save the condition if the option_list is empty or webhook_data is nil + if c.option_list.empty? || c.webhook_data.nil? c.destroy return end @@ -253,7 +244,7 @@ def save_condition(value, opt_map, question_id_map) c.save end # rubocop:enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity - # rubocop:enable Metrics/MethodLength, Metrics/AbcSize + # rubocop:enable Metrics/AbcSize private @@ -280,4 +271,17 @@ def check_remove_conditions end end # rubocop:enable Metrics/AbcSize + + 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 From 82984e25bdd039246a8dfebb8d0ab78b57fe4f17 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Fri, 4 Apr 2025 11:55:00 -0600 Subject: [PATCH 3/6] Refactor handling of `c.option_list.empty?` - Moved `c.option_list.empty?` immediately after `c.option_list` assignment to streamline logic. - This change reduces unnecessary processing of `c.remove_data` and `c.webhook_data`. - Isolating the `c.option_list.empty?` check also simplifies subsequent checks for `c.remove_data.empty?` and c.webhook_data.nil?` later in the code. --- app/models/question.rb | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/app/models/question.rb b/app/models/question.rb index cdaf3df006..9eb035d359 100644 --- a/app/models/question.rb +++ b/app/models/question.rb @@ -222,21 +222,24 @@ def save_condition(value, opt_map, question_id_map) # question options may have changed so rewrite them c.option_list = value['question_option'] c.option_list = c.option_list.map { |qopt| opt_map[qopt] } if opt_map.present? + # 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'] c.remove_data = c.remove_data.map { |qid| question_id_map[qid] } if question_id_map.present? - - # Do not save the condition if the option_list or remove_data is empty - if c.option_list.empty? || c.remove_data.empty? + # Do not save the condition if remove_data is empty + if c.remove_data.empty? c.destroy return end else c.webhook_data = handle_webhook_data(value) - - # Do not save the condition if the option_list is empty or webhook_data is nil - if c.option_list.empty? || c.webhook_data.nil? + # Do not save the condition if webhook_data is nil + if c.webhook_data.nil? c.destroy return end From 49b9f7d4408c68d77594a59ae57de18c6a5e1d31 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Fri, 4 Apr 2025 12:48:35 -0600 Subject: [PATCH 4/6] Refactor option_list and remove_data handling - Moved logic for handling option_list and remove_data into separate helper methods (handle_option_list and handle_remove_data). - This simplifies our `save_condition` method and resolves some previously disabled rubocop offenses. --- app/models/question.rb | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/app/models/question.rb b/app/models/question.rb index 9eb035d359..2c7031a3c5 100644 --- a/app/models/question.rb +++ b/app/models/question.rb @@ -214,14 +214,13 @@ def update_conditions(param_conditions, old_to_new_opts, question_id_map) end # rubocop:disable Metrics/AbcSize - # rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity 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'] - c.option_list = c.option_list.map { |qopt| opt_map[qopt] } if opt_map.present? + 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 @@ -229,8 +228,7 @@ def save_condition(value, opt_map, question_id_map) end if value['action_type'] == 'remove' - c.remove_data = value['remove_question_id'] - c.remove_data = c.remove_data.map { |qid| question_id_map[qid] } if question_id_map.present? + 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 @@ -246,7 +244,6 @@ def save_condition(value, opt_map, question_id_map) end c.save end - # rubocop:enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity # rubocop:enable Metrics/AbcSize private @@ -275,6 +272,22 @@ def check_remove_conditions 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? } From 115ea702768a1470aad964dfe3d0773e56410076 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Fri, 4 Apr 2025 12:55:48 -0600 Subject: [PATCH 5/6] Put back `# rubocop:disable Metrics/MethodLength` --- app/models/question.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/question.rb b/app/models/question.rb index 2c7031a3c5..6f7d006750 100644 --- a/app/models/question.rb +++ b/app/models/question.rb @@ -213,7 +213,7 @@ def update_conditions(param_conditions, old_to_new_opts, question_id_map) end end - # rubocop:disable Metrics/AbcSize + # rubocop:disable Metrics/AbcSize, Metrics/MethodLength def save_condition(value, opt_map, question_id_map) c = conditions.build c.action_type = value['action_type'] @@ -244,7 +244,7 @@ def save_condition(value, opt_map, question_id_map) end c.save end - # rubocop:enable Metrics/AbcSize + # rubocop:enable Metrics/AbcSize, Metrics/MethodLength private From 0fde2537ddcd3b6e56aa106f4d0103acb6177447 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 7 Apr 2025 09:50:53 -0600 Subject: [PATCH 6/6] Update CHANGELOG.md This is --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0af92cb483..df8056bee6 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 `Question.save_condition` [#3501](https://github.com/DMPRoadmap/roadmap/pull/3501) ## v4.2.0