Fix VM Migrate complete email when To field is nil. #177

billfitzgerald0120 · 2017-08-29T20:37:49Z

Made change to not send email when the To field is nil.
Added log message that email will not be sent.

https://bugzilla.redhat.com/show_bug.cgi?id=1486461

billfitzgerald0120 · 2017-08-29T20:40:18Z

@miq-bot add_label bug, fine/yes

billfitzgerald0120 · 2017-08-29T20:40:44Z

@miq-bot assign @gmcculloug

billfitzgerald0120 · 2017-08-29T20:49:50Z

@tinaafitz Please Review

gmcculloug · 2017-08-31T20:46:13Z

@tinaafitz Changes are easier to review by ignoring whitespace: https://github.com/ManageIQ/manageiq-content/pull/177/files?w=1

gmcculloug · 2017-08-31T20:47:39Z

...utomate/ManageIQ/Infrastructure/VM/Migrate/Email.class/__methods__/vmmigratetask_complete.rb

@@ -27,25 +27,30 @@
 $evm.log("info", "VM Owner: #{owner.inspect}")

 # to_email_address from owner.email then from model if nil
-to = owner.email || $evm.object['to_email_address']
+to = owner.email unless owner.nil?


@billfitzgerald0120 Keep the original format and use try to handle the nil owner object.

to = owner.try(:email) || $evm.object['to_email_address']

billfitzgerald0120 · 2017-08-31T21:55:43Z

@gmcculloug made changes as requested

gmcculloug · 2017-09-01T11:49:07Z

spec/automation/unit/method_validation/vm_migrate_task_complete_spec.rb

@@ -11,11 +11,21 @@
    expect(GenericMailer).to receive(:deliver).with(:automation_notification,
                                                    hash_including(:to   => user.email,
                                                                   :from => "evmadmin@example.com"
-                                                                  )
+                                                                )


@billfitzgerald0120 The change in whitespace here is revealing the rubocop warnings around alignment. Please update the closing parentheses, like you did in the new test below, to remove the warnings.

gmcculloug · 2017-09-01T11:51:09Z

spec/automation/unit/method_validation/vm_migrate_task_complete_spec.rb

                                                   )
    attrs = ["MiqServer::miq_server=#{miq_server.id}"]
    attrs << "MiqRequestTask::vm_migrate_task=#{miq_request_task.id}"
    attrs << "vm_migrate_task_id=#{miq_request_task.id}"
    MiqAeEngine.instantiate("/Infrastructure/VM/Migrate/Email/VmMigrateTask_Complete?event=vm_migrated&#{attrs.join('&')}", user)
  end
+
+  it 'sends email to nil' do


Test name is misleading as you are expecting the logic to not send an email in this case. Please reword.

billfitzgerald0120 · 2017-09-01T14:12:04Z

@gmcculloug Made changes as requested, please review

gmcculloug · 2017-09-01T19:52:53Z

@billfitzgerald0120 @tinaafitz Looks like we have this exposure in a number of email methods. Do we handle this anywhere else? Would it make more sense to send the email to someone like an administrator in this case instead of just skipping the email when someone migrates a VM?

billfitzgerald0120 · 2017-09-01T20:18:04Z

@gmcculloug @tinaafitz We handle this in Infrastructure and Cloud MiqProvision_Complete method.
Sending it to an administrator sounds better that skipping.

syncrou · 2017-09-05T18:54:52Z

...utomate/ManageIQ/Infrastructure/VM/Migrate/Email.class/__methods__/vmmigratetask_complete.rb

@@ -27,25 +27,29 @@
 $evm.log("info", "VM Owner: #{owner.inspect}")

 # to_email_address from owner.email then from model if nil
-to = owner.email || $evm.object['to_email_address']
+to = owner.try(:email) || $evm.object['to_email_address']


@billfitzgerald0120 - I like the try here - so it's probably fine - but your fix works without it. The addition of your check for if to.nil? seems to be the real fix. I'm questioning the need for the try apart from readability.

billfitzgerald0120 · 2017-09-11T21:15:29Z

@tinaafitz I added the requester_email as the second choice for the 'to' email value.

billfitzgerald0120 · 2017-09-15T13:46:20Z

@tinaafitz Please Review

gmcculloug · 2017-09-15T15:55:10Z

@tinaafitz Does the method need to change for this or can this be fixed in the schema? I recall we briefly discussed this.

@billfitzgerald0120 Can you describe the condition in which this happens since the schema includes a default email address? When would the to email address be empty?

billfitzgerald0120 · 2017-09-15T17:40:46Z

@gmcculloug @tinaafitz The problem was the owner.email was failing. I added the .try which fixed the problem. I added the requester.email as the second option. We talked about making sure that we send an email to someone and not just abort the email. The schema should always have a value but the requester is probably a better option. It's possible the schema has the default values for to_email_address instead of the actual requester.

-to = owner.email || $evm.object['to_email_address']
+to = owner.try(:email) || requester.email || $evm.object['to_email_address']

billfitzgerald0120 · 2017-09-19T19:37:41Z

@tinaafitz Please review

Made change to not send email when the To field is nil. Added log message that email will not be sent. https://bugzilla.redhat.com/show_bug.cgi?id=1486461

miq-bot · 2017-09-20T14:28:44Z

Checked commit billfitzgerald0120@655f095 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

Fix VM Migrate complete email when To field is nil. (cherry picked from commit 07fe288) https://bugzilla.redhat.com/show_bug.cgi?id=1496937

simaishi · 2017-11-10T17:39:32Z

Fine backport details:

$ git log -1
commit a78b743cd0f2f857bace5192141d2f8b5e8864bd
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Wed Sep 20 10:39:16 2017 -0400

    Merge pull request #177 from billfitzgerald0120/vmmigrate_email_fix
    
    Fix VM Migrate complete email when To field is nil.
    (cherry picked from commit 07fe288e98886d1c9bb98cf392a876c59b287413)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1496937

miq-bot added bug fine/yes labels Aug 29, 2017

miq-bot assigned gmcculloug Aug 29, 2017

gmcculloug requested a review from tinaafitz August 31, 2017 20:43

gmcculloug reviewed Aug 31, 2017

View reviewed changes

billfitzgerald0120 force-pushed the vmmigrate_email_fix branch from dc9e767 to cde51a9 Compare August 31, 2017 21:40

gmcculloug reviewed Sep 1, 2017

View reviewed changes

billfitzgerald0120 force-pushed the vmmigrate_email_fix branch from cde51a9 to 91a6f71 Compare September 1, 2017 13:36

syncrou reviewed Sep 5, 2017

View reviewed changes

billfitzgerald0120 force-pushed the vmmigrate_email_fix branch from 91a6f71 to 936b0c5 Compare September 11, 2017 21:13

Fix VM Migrate complete email when To field is nil.

655f095

Made change to not send email when the To field is nil. Added log message that email will not be sent. https://bugzilla.redhat.com/show_bug.cgi?id=1486461

billfitzgerald0120 force-pushed the vmmigrate_email_fix branch from 936b0c5 to 655f095 Compare September 20, 2017 14:24

gmcculloug merged commit 07fe288 into ManageIQ:master Sep 20, 2017

gmcculloug added this to the Sprint 70 Ending Oct 2, 2017 milestone Sep 20, 2017

simaishi pushed a commit that referenced this pull request Nov 10, 2017

Merge pull request #177 from billfitzgerald0120/vmmigrate_email_fix

a78b743

Fix VM Migrate complete email when To field is nil. (cherry picked from commit 07fe288) https://bugzilla.redhat.com/show_bug.cgi?id=1496937

simaishi added fine/backported and removed fine/yes labels Nov 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix VM Migrate complete email when To field is nil. #177

Fix VM Migrate complete email when To field is nil. #177

billfitzgerald0120 commented Aug 29, 2017

billfitzgerald0120 commented Aug 29, 2017

billfitzgerald0120 commented Aug 29, 2017

billfitzgerald0120 commented Aug 29, 2017

gmcculloug commented Aug 31, 2017

gmcculloug Aug 31, 2017

billfitzgerald0120 commented Aug 31, 2017

gmcculloug Sep 1, 2017

gmcculloug Sep 1, 2017

billfitzgerald0120 commented Sep 1, 2017

gmcculloug commented Sep 1, 2017

billfitzgerald0120 commented Sep 1, 2017

syncrou Sep 5, 2017

billfitzgerald0120 commented Sep 11, 2017

billfitzgerald0120 commented Sep 15, 2017

gmcculloug commented Sep 15, 2017

billfitzgerald0120 commented Sep 15, 2017

billfitzgerald0120 commented Sep 19, 2017

miq-bot commented Sep 20, 2017

simaishi commented Nov 10, 2017

Fix VM Migrate complete email when To field is nil. #177

Fix VM Migrate complete email when To field is nil. #177

Conversation

billfitzgerald0120 commented Aug 29, 2017

billfitzgerald0120 commented Aug 29, 2017

billfitzgerald0120 commented Aug 29, 2017

billfitzgerald0120 commented Aug 29, 2017

gmcculloug commented Aug 31, 2017

gmcculloug Aug 31, 2017

Choose a reason for hiding this comment

billfitzgerald0120 commented Aug 31, 2017

gmcculloug Sep 1, 2017

Choose a reason for hiding this comment

gmcculloug Sep 1, 2017

Choose a reason for hiding this comment

billfitzgerald0120 commented Sep 1, 2017

gmcculloug commented Sep 1, 2017

billfitzgerald0120 commented Sep 1, 2017

syncrou Sep 5, 2017

Choose a reason for hiding this comment

billfitzgerald0120 commented Sep 11, 2017

billfitzgerald0120 commented Sep 15, 2017

gmcculloug commented Sep 15, 2017

billfitzgerald0120 commented Sep 15, 2017

billfitzgerald0120 commented Sep 19, 2017

miq-bot commented Sep 20, 2017

simaishi commented Nov 10, 2017