Refactor Question.save_condition
#3501
Conversation
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.
There was a problem hiding this comment.
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)
Generated by 🚫 Danger |
There was a problem hiding this comment.
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: Even though both of the above used a string when defining 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? }
falseTEST 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? }
trueIt appears to be behaving correctly when operating on the |
This is
There was a problem hiding this comment.
@aaronskiba Thanks for explaining why my test was not correct in this instance.
c879520
into
johnpinto1-updated-port-of-dmptool-conditional-questions-fix-for-rails7
Changes proposed in this PR:
Question.save_conditionvia the following changes:.mapto refactor/simplify the mapping ofoption_listandremove_data. It also removes the need for temporary arrays.handle_webhook_datahelper methodwebhook_datafields are absent. Else, it constructs and returns thewebhook_datahashc.option_list.empty?immediately afterc.option_listassignment to streamline logic.c.remove_dataandc.webhook_data.c.option_list.empty?check also simplifies subsequent checks forc.remove_data.empty?andc.webhook_data.nil?later in the code.option_listandremove_datainto separate helper methods (handle_option_listandhandle_remove_data).save_conditionmethod and resolves some previously disabled rubocop offenses.