Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #31564 - add email notification #638

Merged
merged 1 commit into from Sep 8, 2021

Conversation

ares
Copy link
Member

@ares ares commented Jul 27, 2021

User can subscribe to either all job notifications, successful jobs or
failed jobs or no notifications. The user who runs the job would then be
notified based on the subscription. It supports both plain text and html
emails.

Since now we have a way to run the notification even on failure, the
notification drawer notification has been also added in case the job
fails.

@ares
Copy link
Member Author

ares commented Jul 27, 2021

When testing, do not forget to seed your DB, then in your user preferences, set the subscription, try running a job that fails and succeeds. Even without the subscription, you should now see a job failure in the notification drawer.

Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general +1.

Could we maybe instead of setting hooks on success and failure set a single hook on :stopped and then inside the hook decide which kind of email to send out to make it a bit DRYer?

Also rubocop

job_invocation.build_notification.deliver!

if ['Subscribe to my succeeded jobs', 'Subscribe to all my jobs'].include?(mail_notification_preference&.interval)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we get constants for all of these?

Comment on lines 60 to 95
def notify_on_success(plan)
job_invocation.build_notification.deliver!

if ['Subscribe to my succeeded jobs', 'Subscribe to all my jobs'].include?(mail_notification_preference&.interval)
RexJobMailer.job_finished(job_invocation, subject: _("REX job has succeeded - %s") % job_invocation.to_s).deliver_now
end
end

def notify_on_failure(plan)
job_invocation.build_notification.deliver!

if ['Subscribe to my failed jobs', 'Subscribe to all my jobs'].include?(mail_notification_preference&.interval)
RexJobMailer.job_finished(job_invocation, subject: _("REX job has failed - %s") % job_invocation.to_s).deliver_now
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had in mind something like this, but don't feel too strongly about it

Suggested change
def notify_on_success(plan)
job_invocation.build_notification.deliver!
if ['Subscribe to my succeeded jobs', 'Subscribe to all my jobs'].include?(mail_notification_preference&.interval)
RexJobMailer.job_finished(job_invocation, subject: _("REX job has succeeded - %s") % job_invocation.to_s).deliver_now
end
end
def notify_on_failure(plan)
job_invocation.build_notification.deliver!
if ['Subscribe to my failed jobs', 'Subscribe to all my jobs'].include?(mail_notification_preference&.interval)
RexJobMailer.job_finished(job_invocation, subject: _("REX job has failed - %s") % job_invocation.to_s).deliver_now
end
end
def notify_on_finish(plan)
job_invocation.build_notification.deliver!
event = plan.result == 'success' ? 'succeeded' : 'failed'
if ["Subscribe to my #{event} jobs", 'Subscribe to all my jobs'].include?(mail_notification_preference&.interval)
RexJobMailer.job_finished(job_invocation, subject: _("REX job has %{event} - %{job}") % {:job => job_invocation.to_s, :event => event}).deliver_now
end
end

@ares
Copy link
Member Author

ares commented Aug 5, 2021

I extracted the strings to constants and fixed the rubocop

@ezr-ondrej
Copy link
Member

The failure is related.

@ares
Copy link
Member Author

ares commented Aug 9, 2021

The problematic test:

describe 'notifications' do
  it 'creates notification on sucess run' do
    FactoryBot.create(:notification_blueprint, :name => 'rex_job_succeeded')
    assert_difference 'NotificationRecipient.where(:user_id => targeting.user.id).count' do
      finalize_action planned
    end
  end
end

I think that's because I now use a different trigger then a code in finalize. @adamruzicka any thoughts on how to test this? Or should I simply drop the test?

@adamruzicka
Copy link
Contributor

IIRC we don't really have a real facility for testing hooks. On the other hand, it is just a regular method. You can just call it in the test to test it behaves how we want it to and then trust dynflow that it will run the hooks. If it helps you sleep any better, hooks are covered with tests in dynflow so there should be no issue there.

@adamruzicka
Copy link
Contributor

ping @ares

@ares
Copy link
Member Author

ares commented Sep 7, 2021

Repushed with the tests, sorry for the delay. Can this get merged before the next demo (please ping me on IRC if something else is required). Note the rubocop complains about snake case, but we call all files like that in seeds.d.

@adamruzicka
Copy link
Contributor

rubocop complains about snake case, but we call all files like that in seeds.d.

Could you add it to .rubocop_todo.yml where all the other migration files are ignored?

@ares ares force-pushed the fix/31564 branch 2 times, most recently from b9d42c2 to a0c5cfd Compare September 7, 2021 10:51
User can subscribe to either all job notifications, successful jobs or
failed jobs or no notifications. The user who runs the job would then be
notified based on the subscription. It supports both plain text and html
emails.

Since now we have a way to run the notification even on failure, the
notification drawer notification has been also added in case the job
fails.
@ares
Copy link
Member Author

ares commented Sep 7, 2021

And rebased since meanwhile it got a conflict

@ares
Copy link
Member Author

ares commented Sep 7, 2021

The test failure is IMHO unrelated, perhaps caused by theforeman/foreman#8711?

Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work well, ACK

@adamruzicka adamruzicka merged commit a6a1667 into theforeman:master Sep 8, 2021
@adamruzicka
Copy link
Contributor

Thank you @ares !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants