Refactor `Question.save_condition` by aaronskiba · Pull Request #3501 · DMPRoadmap/roadmap

aaronskiba · 2025-04-04T19:14:40Z

Changes proposed in this PR:

This PR refactors Question.save_condition via the following changes:
- 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 need for temporary arrays.
- Refactor webhook_data validation and construction
  - This change adds the 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)
- 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.
- 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.

This change uses `.map` to refactor/simplify the mapping of `option_list` and `remove_data`. It also removes the needed for temporary arrays.

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.

- 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.

- 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.

Copilot

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

app/models/question.rb:275

Consider adding unit tests for the new helper methods (handle_option_list, handle_remove_data, and handle_webhook_data) to ensure they handle all expected scenarios, including when the input arrays or required webhook fields are missing.

def handle_option_list(value, opt_map)

github-actions · 2025-04-04T19:15:34Z

	1 Warning
⚠️	There are code changes, but no corresponding tests. Please include tests if this PR introduces any modifications in behavior. \n Ignore this warning if the PR ONLY contains translation.io synced updates.

Generated by 🚫 Danger

johnpinto1

739aad0 Approved
82984e2 Approved
49b9f7d Approved
115ea70 Approved

@aaronskiba I might be testing this f6a232b wrongly, but attempts at evaluating
always return true.

irb(main):088* web_hook = {
irb(main):089* 'webhook-name': 'fg',
irb(main):090* 'webhook-email':'x@example.com',
irb(main):091* 'webhook-subject':'SUBJECT',
irb(main):092* 'webhook-message': 'fgh'
irb(main):093> };
irb(main):094> puts web_hook
{:"webhook-name"=>"fg", :"webhook-email"=>"x@example.com", :"webhook-subject"=>"SUBJECT", :"webhook-message"=>"fgh"}
=> nil
irb(main):095> %w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| web_hook[key].blank? }
=> true
irb(main):096* web_hook = {
irb(main):097* 'webhook-name': 'fg',
irb(main):098* 'webhook-email':'x@example.com',
irb(main):099* 'webhook-subject':'',
irb(main):100* 'webhook-message': 'fgh'
irb(main):101> };
irb(main):102> puts web_hook;
{:"webhook-name"=>"fg", :"webhook-email"=>"x@example.com", :"webhook-subject"=>"", :"webhook-message"=>"fgh"}
irb(main):103> %w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| web_hook[key].blank? };
irb(main):104> %w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| web_hook[key].blank? }
=> true

According to CodyAI one needs to convert the string key to a symbol key.to_sym. That seems to work.

%w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| web_hook[key.to_sym].blank? }

%w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| web_hook[key.to_sym].blank? }
irb(main):105> puts %w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| web_hook[key].blank? }
true
=> nil
irb(main):106> puts %w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| web_hook[key.to_sym].blank? }
true
=> nil
irb(main):107* web_hook = {
irb(main):108* 'webhook-name': 'fg',
irb(main):109* 'webhook-email':'x@example.com',
irb(main):110* 'webhook-subject':'SUBJECT',
irb(main):111* 'webhook-message': 'fgh'
irb(main):112> };
irb(main):113> puts %w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| web_hook[key.to_sym].blank? }
false
=> nil
irb(main):114* web_hook = {
irb(main):115* 'webhook-name': 'fg',
irb(main):116* 'webhook-email':'x@example.com',
irb(main):117* 'webhook-subject':'',
irb(main):118* 'webhook-message': 'fgh'
irb(main):119> };
irb(main):120> puts %w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| web_hook[key.to_sym].blank? }
true

aaronskiba · 2025-04-07T14:53:57Z

739aad0 Approved 82984e2 Approved 49b9f7d Approved 115ea70 Approved

@aaronskiba I might be testing this f6a232b wrongly, but attempts at evaluating always return true.

irb(main):088* web_hook = { irb(main):089* 'webhook-name': 'fg', irb(main):090* 'webhook-email':'x@example.com', irb(main):091* 'webhook-subject':'SUBJECT', irb(main):092* 'webhook-message': 'fgh' irb(main):093> }; irb(main):094> puts web_hook {:"webhook-name"=>"fg", :"webhook-email"=>"x@example.com", :"webhook-subject"=>"SUBJECT", :"webhook-message"=>"fgh"} => nil irb(main):095> %w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| web_hook[key].blank? } => true irb(main):096* web_hook = { irb(main):097* 'webhook-name': 'fg', irb(main):098* 'webhook-email':'x@example.com', irb(main):099* 'webhook-subject':'', irb(main):100* 'webhook-message': 'fgh' irb(main):101> }; irb(main):102> puts web_hook; {:"webhook-name"=>"fg", :"webhook-email"=>"x@example.com", :"webhook-subject"=>"", :"webhook-message"=>"fgh"} irb(main):103> %w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| web_hook[key].blank? }; irb(main):104> %w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| web_hook[key].blank? } => true

According to CodyAI one needs to convert the string key to a symbol key.to_sym. That seems to work.

%w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| web_hook[key.to_sym].blank? }

%w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| web_hook[key.to_sym].blank? } irb(main):105> puts %w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| web_hook[key].blank? } true => nil irb(main):106> puts %w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| web_hook[key.to_sym].blank? } true => nil irb(main):107* web_hook = { irb(main):108* 'webhook-name': 'fg', irb(main):109* 'webhook-email':'x@example.com', irb(main):110* 'webhook-subject':'SUBJECT', irb(main):111* 'webhook-message': 'fgh' irb(main):112> }; irb(main):113> puts %w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| web_hook[key.to_sym].blank? } false => nil irb(main):114* web_hook = { irb(main):115* 'webhook-name': 'fg', irb(main):116* 'webhook-email':'x@example.com', irb(main):117* 'webhook-subject':'', irb(main):118* 'webhook-message': 'fgh' irb(main):119> }; irb(main):120> puts %w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| web_hook[key.to_sym].blank? } true

Hi @johnpinto1, I'm definitely using more and more AI for these refactors myself, so I really appreciate the thorough testing. Here's a bit of a response to your observations:

irb(main):001> test_hash1 = {'key-1':'value'}
=> {:"key-1"=>"value"}
irb(main):002> test_hash2 = {'key-1' => 'value'}
=> {"key-1"=>"value"}

Even though both of the above used a string when defining key-1, it is a hash for test_hash1 and a string for test_hash2.

But what will be the type of the keys here?

  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? }

TEST 1 (All Fields Filled Out)

   291:   def handle_webhook_data(value)
   292:     # return nil if any of the webhook fields are blank
   293:     byebug
=> 294:     return if %w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| value[key].blank? }
   295: 
   296:     # else return the constructed webhook_data hash
   297:     {
   298:       name: value['webhook-name'],
(byebug) value
{"question_option"=>["4854"], "action_type"=>"add_webhook", "number"=>"0", "webhook-name"=>"name", "webhook-email"=>"email@gmail.com", "webhook-subject"=>"subject", "webhook-message"=>"message"}
(byebug) %w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| value[key].blank? }
false

TEST 2 (With a Field Missing)

   291:   def handle_webhook_data(value)
   292:     # return nil if any of the webhook fields are blank
   293:     byebug
=> 294:     return if %w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| value[key].blank? }
   295: 
   296:     # else return the constructed webhook_data hash
   297:     {
   298:       name: value['webhook-name'],
(byebug) value
{"question_option"=>["4854"], "action_type"=>"add_webhook", "number"=>"0", "webhook-name"=>"name", "webhook-email"=>"email@gmail.com", "webhook-subject"=>"subject", "webhook-message"=>""}
(byebug) %w[webhook-name webhook-email webhook-subject webhook-message].any? { |key| value[key].blank? }
true

It appears to be behaving correctly when operating on the value arg passed to def handle_webhook_data

This is

johnpinto1

@aaronskiba Thanks for explaining why my test was not correct in this instance.

aaronskiba added 5 commits April 4, 2025 12:11

Refactor mapping of remove_data & option_list

739aad0

This change uses `.map` to refactor/simplify the mapping of `option_list` and `remove_data`. It also removes the needed for temporary arrays.

Refactor option_list and remove_data handling

49b9f7d

- 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.

Put back # rubocop:disable Metrics/MethodLength

115ea70

aaronskiba requested review from Copilot and johnpinto1 April 4, 2025 19:14

Copilot AI reviewed Apr 4, 2025

View reviewed changes

aaronskiba mentioned this pull request Apr 4, 2025

Fix issues with Conditional question serialization (offered by @briri from DMPTool) in PR https://github.com/CDLUC3/dmptool/pull/667 #3497

Merged

johnpinto1 reviewed Apr 7, 2025

View reviewed changes

Update CHANGELOG.md

0fde253

This is

johnpinto1 approved these changes Apr 7, 2025

View reviewed changes

aaronskiba merged commit c879520 into johnpinto1-updated-port-of-dmptool-conditional-questions-fix-for-rails7 Apr 7, 2025
8 checks passed

aaronskiba deleted the aaron/refactor-question-save-condition branch April 7, 2025 20:18

aaronskiba mentioned this pull request Apr 10, 2025

Merge development Into main For Upcoming Release #3495

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `Question.save_condition`#3501

Refactor `Question.save_condition`#3501
aaronskiba merged 6 commits intojohnpinto1-updated-port-of-dmptool-conditional-questions-fix-for-rails7from
aaron/refactor-question-save-condition

aaronskiba commented Apr 4, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

github-actions bot commented Apr 4, 2025 •

edited

Loading

Uh oh!

johnpinto1 left a comment

Uh oh!

aaronskiba commented Apr 7, 2025

Uh oh!

johnpinto1 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

aaronskiba commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnpinto1 left a comment

Choose a reason for hiding this comment

Uh oh!

aaronskiba commented Apr 7, 2025

TEST 1 (All Fields Filled Out)

TEST 2 (With a Field Missing)

Uh oh!

johnpinto1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aaronskiba commented Apr 4, 2025 •

edited

Loading

github-actions bot commented Apr 4, 2025 •

edited

Loading