From 0052b5496773c724f07aebc1147c004a91abb5a5 Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Thu, 30 Aug 2018 11:08:37 +0200 Subject: [PATCH] Update models & tests with new notification template --- app/mailers/notifications_mailer.rb | 5 +- app/models/check.rb | 10 +-- app/models/notification.rb | 20 ++---- app/models/user.rb | 3 +- app/services/notifier/channels/base.rb | 16 ++--- app/services/notifier/channels/email.rb | 8 +-- app/services/notifier/processor.rb | 4 +- app/services/notifier/resolver.rb | 4 +- test/factories/notifications.rb | 18 +---- test/mailers/notifications_mailer_test.rb | 28 ++++---- test/models/check_test.rb | 14 ++-- test/services/notifier/channels/base_test.rb | 38 +++++----- test/services/notifier/processor_test.rb | 12 ++-- test/services/notifier/resolver_test.rb | 76 ++++++++++---------- 14 files changed, 121 insertions(+), 135 deletions(-) diff --git a/app/mailers/notifications_mailer.rb b/app/mailers/notifications_mailer.rb index b8cdc01..b57de6c 100644 --- a/app/mailers/notifications_mailer.rb +++ b/app/mailers/notifications_mailer.rb @@ -5,8 +5,9 @@ class NotificationsMailer < ApplicationMailer helper :application before_action except: :recurrent_failures do - @notification = params.fetch(:notification) - @check = @notification.check + @check_notification = params.fetch(:check_notification) + @check = @check_notification.check + @notification = @check_notification.notification end def domain_expires_soon diff --git a/app/models/check.rb b/app/models/check.rb index 292283f..6334e17 100644 --- a/app/models/check.rb +++ b/app/models/check.rb @@ -34,7 +34,9 @@ class Check < ApplicationRecord belongs_to :user has_many :logs, class_name: "CheckLog", dependent: :destroy - has_many :notifications, validate: true, dependent: :destroy + has_many :check_notifications, dependent: :destroy + has_many :notifications, through: :check_notifications, validate: true + accepts_nested_attributes_for :notifications, allow_destroy: true, reject_if: lambda { |at| at["recipient"].blank? && at["interval"].blank? } @@ -58,7 +60,7 @@ class Check < ApplicationRecord before_save :reset_consecutive_failures before_save :set_mode - after_update :reset_notifications + after_update :reset_check_notifications after_save :enqueue_sync scope :active, -> { where(active: true) } @@ -119,10 +121,10 @@ class Check < ApplicationRecord ResyncJob.perform_later(id) end - def reset_notifications + def reset_check_notifications return unless (saved_changes.keys & %w[domain domain_expires_at]).present? - notifications.each(&:reset!) + check_notifications.each(&:reset!) end def reset_consecutive_failures diff --git a/app/models/notification.rb b/app/models/notification.rb index cec38ce..2c39b0e 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -27,26 +27,14 @@ # class Notification < ApplicationRecord - belongs_to :check + belongs_to :user + has_many :check_notifications + has_many :checks, through: :notifications enum channel: [:email] - enum status: [:pending, :ongoing, :succeed, :failed] + validates :label, presence: true validates :channel, presence: true validates :interval, numericality: { only_integer: true, greater_than_or_equal_to: 1 } validates :recipient, presence: true - - scope :active_check, -> { Check.active } - scope :check_last_run_failed, -> { Check.last_run_failed } - - def pending! - self.sent_at = nil - super - end - alias reset! pending! - - def ongoing! - self.sent_at = Time.now - super - end end diff --git a/app/models/user.rb b/app/models/user.rb index 8ec2a96..ab649ed 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -39,7 +39,8 @@ class User < ApplicationRecord devise :database_authenticatable, :registerable, :recoverable, :rememberable, :validatable, :confirmable - has_many :checks + has_many :checks, dependent: :destroy + has_many :notifications, dependent: :destroy validates :tos_accepted, acceptance: true validates :locale, inclusion: { in: I18n.available_locales.map(&:to_s) } diff --git a/app/services/notifier/channels/base.rb b/app/services/notifier/channels/base.rb index 264a318..40a5b49 100644 --- a/app/services/notifier/channels/base.rb +++ b/app/services/notifier/channels/base.rb @@ -4,26 +4,26 @@ module Notifier module Channels class Base - def notify(notification) # rubocop:disable Metrics/MethodLength - return unless supports?(notification) + def notify(check_notification) # rubocop:disable Metrics/MethodLength + return unless supports?(check_notification) - notification.ongoing! + check_notification.ongoing! - case notification.check.kind.to_sym + case check_notification.check.kind.to_sym when :domain - domain_notify_expires_soon(notification) + domain_notify_expires_soon(check_notification) when :ssl - ssl_notify_expires_soon(notification) + ssl_notify_expires_soon(check_notification) else fail ArgumentError, - "Invalid notification for check kind `#{notification.check.kind}`." + "Invalid notification for check kind `#{check_notification.check.kind}`." end end private # :nocov: - def supports?(_notification) + def supports?(_check_notification) fail NotImplementedError, "#{self.class.name} channel did not implemented method #{__callee__}" end diff --git a/app/services/notifier/channels/email.rb b/app/services/notifier/channels/email.rb index 743aa5a..e745804 100644 --- a/app/services/notifier/channels/email.rb +++ b/app/services/notifier/channels/email.rb @@ -11,17 +11,17 @@ module Notifier protected - def supports?(_notification) + def supports?(_check_notification) true end # Expiration notifications - def domain_notify_expires_soon(notification) - NotificationsMailer.with(notification: notification).domain_expires_soon.deliver_now + def domain_notify_expires_soon(check_notification) + NotificationsMailer.with(check_notification: check_notification).domain_expires_soon.deliver_now end def ssl_notify_expires_soon(notification) - NotificationsMailer.with(notification: notification).ssl_expires_soon.deliver_now + NotificationsMailer.with(check_notification: check_notification).ssl_expires_soon.deliver_now end end end diff --git a/app/services/notifier/processor.rb b/app/services/notifier/processor.rb index 502c867..de60c28 100644 --- a/app/services/notifier/processor.rb +++ b/app/services/notifier/processor.rb @@ -19,8 +19,8 @@ module Notifier end def process_expires_soon - resolver.notifications_expiring_soon.find_each do |notification| - notifier_channel_for(notification).notify(notification) + resolver.notifications_expiring_soon.find_each do |check_notification| + notifier_channel_for(check_notification.notification).notify(check_notification) sleep configuration.interval end diff --git a/app/services/notifier/resolver.rb b/app/services/notifier/resolver.rb index bc1120c..d99577f 100644 --- a/app/services/notifier/resolver.rb +++ b/app/services/notifier/resolver.rb @@ -21,8 +21,8 @@ module Notifier private def scope - Notification - .includes(:check) + CheckNotification + .includes(:check, :notification) .where(status: [:pending, :failed]) .merge(Check.active) .where.not(checks: { user: ignore_users }) diff --git a/test/factories/notifications.rb b/test/factories/notifications.rb index de8052c..434e386 100644 --- a/test/factories/notifications.rb +++ b/test/factories/notifications.rb @@ -28,28 +28,14 @@ FactoryBot.define do factory :notification do - check + user interval 30 channel :email + label { "#{recipient} (#{interval})" } recipient "recipient@domain.fr" - status :pending - sent_at nil trait :email do channel :email end - - trait :ongoing do - status :ongoing - end - - trait :succeed do - status :succeed - sent_at { 1.day.ago } - end - - trait :failed do - status :failed - end end end diff --git a/test/mailers/notifications_mailer_test.rb b/test/mailers/notifications_mailer_test.rb index dc84db2..29543bf 100644 --- a/test/mailers/notifications_mailer_test.rb +++ b/test/mailers/notifications_mailer_test.rb @@ -6,10 +6,11 @@ require "test_helper" class NotificationsMailerTest < ActionMailer::TestCase # rubocop:disable Metrics/ClassLength test "domain_expires_soon" do check = create(:check, domain_expires_at: Time.new(2018, 6, 10, 12, 0, 5, "+02:00")) - notification = build(:notification, interval: 10, check: check, recipient: "colin@example.org") + notification = build(:notification, interval: 10, recipient: "colin@example.org") + check_notification = build(:check_notification, check: check, notification: notification) Date.stub :today, Date.new(2018, 6, 2) do - mail = NotificationsMailer.with(notification: notification).domain_expires_soon + mail = NotificationsMailer.with(check_notification: check_notification).domain_expires_soon assert_emails 1 do mail.deliver_now @@ -37,10 +38,11 @@ class NotificationsMailerTest < ActionMailer::TestCase # rubocop:disable Metrics check = create(:check, domain_expires_at: Time.new(2018, 6, 10, 12, 0, 5, "+02:00"), user: build(:user, :fr)) - notification = build(:notification, interval: 10, check: check, recipient: "colin@example.org") + notification = build(:notification, interval: 10, recipient: "colin@example.org") + check_notification = build(:check_notification, check: check, notification: notification) Date.stub :today, Date.new(2018, 6, 2) do - mail = NotificationsMailer.with(notification: notification).domain_expires_soon + mail = NotificationsMailer.with(check_notification: check_notification).domain_expires_soon assert_emails 1 do mail.deliver_now @@ -69,9 +71,9 @@ class NotificationsMailerTest < ActionMailer::TestCase # rubocop:disable Metrics domain_expires_at: 1.week.from_now, comment: "My comment", vendor: "The vendor") - notification = build(:notification, check: check) + check_notification = build(:check_notification, check: check) - mail = NotificationsMailer.with(notification: notification).domain_expires_soon + mail = NotificationsMailer.with(check_notification: check_notification).domain_expires_soon parts = [mail.text_part.decode_body, mail.html_part.decode_body] @@ -87,9 +89,9 @@ class NotificationsMailerTest < ActionMailer::TestCase # rubocop:disable Metrics comment: "My comment", vendor: "The vendor", user: build(:user, :fr)) - notification = build(:notification, check: check) + check_notification = build(:check_notification, check: check) - mail = NotificationsMailer.with(notification: notification).domain_expires_soon + mail = NotificationsMailer.with(check_notification: check_notification).domain_expires_soon parts = [mail.text_part.decode_body, mail.html_part.decode_body] @@ -158,10 +160,11 @@ class NotificationsMailerTest < ActionMailer::TestCase # rubocop:disable Metrics test "ssl_expires_soon" do check = create(:check, :ssl, domain_expires_at: Time.new(2018, 6, 10, 12, 0, 5, "+02:00")) - notification = build(:notification, interval: 10, check: check, recipient: "colin@example.org") + notification = build(:notification, interval: 10, recipient: "colin@example.org") + check_notification = build(:check_notification, check: check, notification: notification) Date.stub :today, Date.new(2018, 6, 2) do - mail = NotificationsMailer.with(notification: notification).ssl_expires_soon + mail = NotificationsMailer.with(check_notification: check_notification).ssl_expires_soon assert_emails 1 do mail.deliver_now @@ -190,10 +193,11 @@ class NotificationsMailerTest < ActionMailer::TestCase # rubocop:disable Metrics check = create(:check, :ssl, domain_expires_at: Time.new(2018, 6, 10, 12, 0, 5, "+02:00"), user: build(:user, :fr)) - notification = build(:notification, interval: 10, check: check, recipient: "colin@example.org") + notification = build(:notification, interval: 10, recipient: "colin@example.org") + check_notification = build(:check_notification, check: check, notification: notification) Date.stub :today, Date.new(2018, 6, 2) do - mail = NotificationsMailer.with(notification: notification).ssl_expires_soon + mail = NotificationsMailer.with(check_notification: check_notification).ssl_expires_soon assert_emails 1 do mail.deliver_now diff --git a/test/models/check_test.rb b/test/models/check_test.rb index 8064463..96b6946 100644 --- a/test/models/check_test.rb +++ b/test/models/check_test.rb @@ -35,23 +35,23 @@ require "test_helper" class CheckTest < ActiveSupport::TestCase test "notifications are resetted when domain expiration date has changed" do check = create(:check) - notification = create(:notification, :succeed, check: check) + check_notification = create(:check_notification, :succeed, check: check) check.comment = "Will not reset because of this attribute" check.save! - notification.reload + check_notification.reload - assert notification.succeed? - assert_not_nil notification.sent_at + assert check_notification.succeed? + assert_not_nil check_notification.sent_at check.domain_expires_at = 1.year.from_now check.save! - notification.reload + check_notification.reload - assert notification.pending? - assert_nil notification.sent_at + assert check_notification.pending? + assert_nil check_notification.sent_at end test "days_from_last_success without any success" do diff --git a/test/services/notifier/channels/base_test.rb b/test/services/notifier/channels/base_test.rb index d696745..f20779f 100644 --- a/test/services/notifier/channels/base_test.rb +++ b/test/services/notifier/channels/base_test.rb @@ -8,8 +8,8 @@ module Notifier class BaseTest < ActiveSupport::TestCase setup do class FakeChannel < Base - def supports?(notification) - notification.interval < 1_000 + def supports?(check_notification) + check_notification.notification.interval < 1_000 end def domain_notify_expires_soon(*); end @@ -19,45 +19,45 @@ module Notifier end test "#notify change the status of the notification" do - notification = create(:notification) + check_notification = create(:check_notification) - @channel.notify(notification) + @channel.notify(check_notification) - notification.reload + check_notification.reload - assert notification.ongoing? - assert_just_now notification.sent_at + assert check_notification.ongoing? + assert_just_now check_notification.sent_at end test "#notify raises an exception for a non supported check kind" do - notification = Minitest::Mock.new - notification.expect :ongoing!, true - notification.expect :interval, 10 + check_notification = Minitest::Mock.new + check_notification.expect :ongoing!, true + check_notification.expect :notification, OpenStruct.new(interval: 10) check = Minitest::Mock.new check.expect(:kind, :invalid_kind) check.expect(:kind, :invalid_kind) # twice (second call for exception message) - notification.expect :check, check - notification.expect :check, check + check_notification.expect :check, check + check_notification.expect :check, check assert_raises ArgumentError do - @channel.notify(notification) + @channel.notify(check_notification) end check.verify - notification.verify + check_notification.verify end test "#notify does nothing when channel doesn't support a notification whatever the reason" do - notification = create(:notification, interval: 10_000) + check_notification = create(:check_notification, notification: build(:notification, interval: 10_000)) - @channel.notify(notification) + @channel.notify(check_notification) - notification.reload + check_notification.reload - assert notification.pending? - assert_nil notification.sent_at + assert check_notification.pending? + assert_nil check_notification.sent_at end end end diff --git a/test/services/notifier/processor_test.rb b/test/services/notifier/processor_test.rb index 2b2693f..14121ee 100644 --- a/test/services/notifier/processor_test.rb +++ b/test/services/notifier/processor_test.rb @@ -6,9 +6,9 @@ require "test_helper" module Notifier class ProcessorTest < ActiveSupport::TestCase test "#process_expires_soon sends an email for checks expiring soon" do - create_list(:notification, 3, :email, check: build(:check, :expires_next_week)) - create(:notification, :email, check: build(:check, :nil_dates)) - create(:notification, :email, check: build(:check, :inactive)) + create_list(:check_notification, 3, notification: email_notification, check: build(:check, :expires_next_week)) + create(:check_notification, notification: email_notification, check: build(:check, :nil_dates)) + create(:check_notification, notification: email_notification, check: build(:check, :inactive)) processor = Processor.new assert_difference "ActionMailer::Base.deliveries.size", +3 do @@ -20,7 +20,7 @@ module Notifier end test "#process_expires_soon respects the interval configuration between sends" do - create_list(:notification, 3, :email, check: build(:check, :expires_next_week)) + create_list(:check_notification, 3, notification: email_notification, check: build(:check, :expires_next_week)) test_interval_respected(:process_expires_soon, 3) end @@ -33,6 +33,10 @@ module Notifier private + def email_notification + build(:notification, :email) + end + # rubocop:disable Metrics/MethodLength def test_interval_respected(process_method, count_expected) configuration = Minitest::Mock.new diff --git a/test/services/notifier/resolver_test.rb b/test/services/notifier/resolver_test.rb index 2ed9c84..419adfe 100644 --- a/test/services/notifier/resolver_test.rb +++ b/test/services/notifier/resolver_test.rb @@ -10,74 +10,74 @@ module Notifier end test "#notifications_expiring_soon ignores user having notification disabled" do - n1 = create(:notification, check: build(:check, :expires_next_week)) + n1 = create(:check_notification, check: build(:check, :expires_next_week)) n1.check.user.update_attribute(:notifications_enabled, false) - n2 = create(:notification, check: build(:check, :expires_next_week)) + n2 = create(:check_notification, check: build(:check, :expires_next_week)) - notifications = @resolver.notifications_expiring_soon + check_notifications = @resolver.notifications_expiring_soon - assert_not_includes notifications, n1 - assert_includes notifications, n2 + assert_not_includes check_notifications, n1 + assert_includes check_notifications, n2 end test "#notifications_expiring_soon ignores inactive checks" do - n1 = create(:notification, check: build(:check, :expires_next_week, :inactive)) - n2 = create(:notification, check: build(:check, :expires_next_week)) + n1 = create(:check_notification, check: build(:check, :expires_next_week, :inactive)) + n2 = create(:check_notification, check: build(:check, :expires_next_week)) - notifications = @resolver.notifications_expiring_soon + check_notifications = @resolver.notifications_expiring_soon - assert_not_includes notifications, n1 - assert_includes notifications, n2 + assert_not_includes check_notifications, n1 + assert_includes check_notifications, n2 end test "#notifications_expiring_soon gets only checks inside interval" do - n1 = create(:notification, check: build(:check, :expires_next_week), interval: 6) - n2 = create(:notification, check: build(:check, :expires_next_week), interval: 7) + n1 = create(:check_notification, check: build(:check, :expires_next_week), notification: build(:notification, interval: 6)) + n2 = create(:check_notification, check: build(:check, :expires_next_week), notification: build(:notification, interval: 7)) - notifications = @resolver.notifications_expiring_soon + check_notifications = @resolver.notifications_expiring_soon - assert_not_includes notifications, n1 - assert_includes notifications, n2 + assert_not_includes check_notifications, n1 + assert_includes check_notifications, n2 end test "#notifications_expiring_soon can gets several notifications for a same check" do check = create(:check, :expires_next_week) - n1 = create(:notification, check: check, interval: 3) - n2 = create(:notification, check: check, interval: 10) - n3 = create(:notification, check: check, interval: 30) + n1 = create(:check_notification, check: check, notification: build(:notification, interval: 3)) + n2 = create(:check_notification, check: check, notification: build(:notification, interval: 10)) + n3 = create(:check_notification, check: check, notification: build(:notification, interval: 30)) - notifications = @resolver.notifications_expiring_soon + check_notifications = @resolver.notifications_expiring_soon - assert_not_includes notifications, n1 - assert_includes notifications, n2 - assert_includes notifications, n3 + assert_not_includes check_notifications, n1 + assert_includes check_notifications, n2 + assert_includes check_notifications, n3 end test "#notifications_expiring_soon takes care of the status" do check = create(:check, :expires_next_week) - n1 = create(:notification, check: check) - n2 = create(:notification, :failed, check: check) - n3 = create(:notification, :ongoing, check: check) - n4 = create(:notification, :succeed, check: check) + n1 = create(:check_notification, check: check) + n2 = create(:check_notification, :failed, check: check) + n3 = create(:check_notification, :ongoing, check: check) + n4 = create(:check_notification, :succeed, check: check) - notifications = @resolver.notifications_expiring_soon + check_notifications = @resolver.notifications_expiring_soon - assert_includes notifications, n1 - assert_includes notifications, n2 - assert_not_includes notifications, n3 - assert_not_includes notifications, n4 + assert_includes check_notifications, n1 + assert_includes check_notifications, n2 + assert_not_includes check_notifications, n3 + assert_not_includes check_notifications, n4 end test "#notifications_expiring_soon ignores checks expired and without date" do - n1 = create(:notification, check: build(:check, :expires_next_week)) - n2 = create(:notification, check: build(:check, domain_expires_at: 1.week.ago)) - n3 = create(:notification, check: build(:check, :nil_dates)) + n1 = create(:check_notification, check: build(:check, :expires_next_week)) + n2 = create(:check_notification, check: build(:check, domain_expires_at: 1.week.ago)) + n3 = create(:check_notification, check: build(:check, :nil_dates)) - notifications = @resolver.notifications_expiring_soon + check_notifications = @resolver.notifications_expiring_soon - assert_includes notifications, n1 - assert_not_includes notifications, n2 - assert_not_includes notifications, n3 + assert_includes check_notifications, n1 + assert_not_includes check_notifications, n2 + assert_not_includes check_notifications, n3 end test "#checks_recurrent_failures ignores inactive checks" do