Merge pull request #17394 from carbonin/dedup_embedded_ansible_notifi…

…cations

Deduplicate embedded ansible notifications

app/models/embedded_ansible_worker/runner.rb

@@ -97,7 +97,12 @@ def raise_role_notification(notification_type)
       :role_name   => ServerRole.find_by(:name => worker.class.required_roles.first).description,
       :server_name => MiqServer.my_server.name
     }
-    Notification.create(:type => notification_type, :options => notification_options)
+
+    # Create the notification only if there are no existing ones for embedded ansible which are unseen
+    Notification.create(:type => notification_type, :options => notification_options) if Notification.of_type(notification_type).none? do |n|
+      correct_notification = n.options == notification_options
+      correct_notification && !n.seen_by_all_recipients?
+    end
   end
 
   def embedded_ansible

app/models/notification.rb

@@ -16,6 +16,8 @@ class Notification < ApplicationRecord
   serialize :options, Hash
   default_value_for(:options) { Hash.new }
 
+  scope :of_type, ->(notification_type) { joins(:notification_type).where(:notification_types => {:name => notification_type}) }
+
   def type=(typ)
     self.notification_type = NotificationType.find_by!(:name => typ)
   end
@@ -35,6 +37,10 @@ def to_h
     }
   end
 
+  def seen_by_all_recipients?
+    notification_recipients.unseen.empty?
+  end
+
   private
 
   def emit_message

spec/models/embedded_ansible_worker/runner_spec.rb

@@ -21,37 +21,8 @@
     }
 
     context "#do_before_work_loop" do
-      let(:start_notification_id) { NotificationType.find_by(:name => "role_activate_start").id }
-      let(:success_notification_id) { NotificationType.find_by(:name => "role_activate_success").id }
-
-      before do
-        ServerRole.seed
-        NotificationType.seed
-      end
-
-      it "creates a notification to inform the user that the service has started" do
-        expect(runner).to receive(:setup_ansible)
-        expect(runner).to receive(:update_embedded_ansible_provider)
-
-        runner.do_before_work_loop
-
-        note = Notification.find_by(:notification_type_id => success_notification_id)
-        expect(note.options[:role_name]).to eq("Embedded Ansible")
-        expect(note.options.keys).to include(:server_name)
-      end
-
-      it "creates a notification to inform the user that the role has been assigned" do
-        expect(runner).to receive(:setup_ansible)
-        expect(runner).to receive(:update_embedded_ansible_provider)
-
-        runner.do_before_work_loop
-
-        note = Notification.find_by(:notification_type_id => start_notification_id)
-        expect(note.options[:role_name]).to eq("Embedded Ansible")
-        expect(note.options.keys).to include(:server_name)
-      end
-
       it "exits on exceptions" do
+        allow(runner).to receive(:raise_role_notification)
         expect(runner).to receive(:setup_ansible)
         expect(runner).to receive(:update_embedded_ansible_provider).and_raise(StandardError)
         expect(runner).to receive(:do_exit)
@@ -170,5 +141,61 @@
         end
       end
     end
+
+    context "#raise_role_notification (private)" do
+      let(:start_notification_id) { NotificationType.find_by(:name => "role_activate_start").id }
+      let(:success_notification_id) { NotificationType.find_by(:name => "role_activate_success").id }
+
+      before do
+        ServerRole.seed
+        NotificationType.seed
+        FactoryGirl.create(:user_admin)
+      end
+
+      it "creates a notification to inform the user that the service has started" do
+        runner.send(:raise_role_notification, :role_activate_start)
+
+        note = Notification.find_by(:notification_type_id => start_notification_id)
+        expect(note.options[:role_name]).to eq("Embedded Ansible")
+        expect(note.options.keys).to include(:server_name)
+      end
+
+      it "creates a notification to inform the user that the role has been assigned" do
+        runner.send(:raise_role_notification, :role_activate_success)
+
+        note = Notification.find_by(:notification_type_id => success_notification_id)
+        expect(note.options[:role_name]).to eq("Embedded Ansible")
+        expect(note.options.keys).to include(:server_name)
+      end
+
+      it "doesn't create additional notifications if an unread one exists" do
+        runner.send(:raise_role_notification, :role_activate_start)
+        expect(Notification.count).to eq(1)
+
+        runner.send(:raise_role_notification, :role_activate_start)
+        expect(Notification.count).to eq(1)
+      end
+
+      it "creates a new notification if the existing one was read" do
+        runner.send(:raise_role_notification, :role_activate_start)
+        expect(Notification.count).to eq(1)
+
+        Notification.first.notification_recipients.each { |r| r.update_attributes(:seen => true) }
+        runner.send(:raise_role_notification, :role_activate_start)
+        expect(Notification.count).to eq(2)
+      end
+
+      it "creates a new notification if there is one for a different role" do
+        Notification.create(:type => :role_activate_start, :options => {:role_name => "someotherrole", :server_name => miq_server.name})
+        runner.send(:raise_role_notification, :role_activate_start)
+        expect(Notification.count).to eq(2)
+      end
+
+      it "creates a new notification if there is one for a different server" do
+        Notification.create(:type => :role_activate_start, :options => {:role_name => "Embedded Ansible", :server_name => "#{miq_server.name}somenonsense"})
+        runner.send(:raise_role_notification, :role_activate_start)
+        expect(Notification.count).to eq(2)
+      end
+    end
   end
 end

spec/models/notification_spec.rb

@@ -4,6 +4,23 @@
   let(:tenant) { FactoryGirl.create(:tenant) }
   let!(:user) { FactoryGirl.create(:user_with_group, :tenant => tenant) }
 
+  describe '.of_type' do
+    it "only returns notification of the given type" do
+      types = NotificationType.all
+      type_name_one   = types[0].name
+      type_name_two   = types[1].name
+      type_name_three = types[2].name
+
+      Notification.create(:type => type_name_one, :initiator => user)
+      Notification.create(:type => type_name_one, :initiator => user)
+      Notification.create(:type => type_name_two, :initiator => user)
+
+      expect(Notification.of_type(type_name_one).count).to eq(2)
+      expect(Notification.of_type(type_name_two).count).to eq(1)
+      expect(Notification.of_type(type_name_three).count).to eq(0)
+    end
+  end
+
   describe '.emit' do
     context 'successful' do
       let(:vm) { FactoryGirl.create(:vm, :tenant => tenant) }
@@ -96,4 +113,22 @@
       )
     end
   end
+
+  describe "#seen_by_all_recipients?" do
+    let(:notification) { FactoryGirl.create(:notification, :initiator => user) }
+
+    it "is false if a user has not seen the notification" do
+      expect(notification.seen_by_all_recipients?).to be_falsey
+    end
+
+    it "is true when all recipients have seen the notification" do
+      notification.notification_recipients.each { |r| r.update_attributes(:seen => true) }
+      expect(notification.seen_by_all_recipients?).to be_truthy
+    end
+
+    it "is true when there are no recipients" do
+      notification.notification_recipients.destroy_all
+      expect(notification.seen_by_all_recipients?).to be_truthy
+    end
+  end
 end