Merge pull request #74 from dumparkltd/fix-duplicate-notifications

Send task updated notifications when tasks are updated or created for other users

app/jobs/task_notification_job.rb

@@ -1,9 +1,9 @@
 class TaskNotificationJob
   include Sidekiq::Worker
 
-  def perform(user_measure_id)
-    user_measure = UserMeasure.find(user_measure_id)
-    return unless user_measure&.notify?
+  def perform(user_id, measure_id)
+    user_measure = UserMeasure.find_by(user_id: user_id, measure_id: measure_id)
+    return if !user_measure || !user_measure.notify?
 
     UserMeasureMailer.task_updated(user_measure).deliver_now
   end

app/models/measure.rb

@@ -48,7 +48,7 @@ def self.notifiable_attribute_names
   end
 
   def notifiable_user_measures(user_id:)
-    user_measures.reject { |um| um.user.id == user_id }
+    user_measures.where.not(user_id: user_id)
   end
 
   after_commit :queue_task_updated_notifications!,
@@ -61,10 +61,26 @@ def queue_task_updated_notifications!(user_id: ::PaperTrail.request.whodunnit)
     delete_existing_task_notifications!(user_id:)
 
     notifiable_user_measures(user_id:).each do |user_measure|
-      TaskNotificationJob.perform_in(ENV.fetch("TASK_NOTIFICATION_DELAY", 20).to_i.seconds, user_measure.id)
+      queue_task_updated_notification!(
+        user_id: user_measure.user_id,
+        measure_id: user_measure.measure_id,
+        delete_existing: false # this is already handled above by delete_existing_task_notifications!
+      )
     end
   end
 
+  def queue_task_updated_notification!(user_id:, measure_id:, delete_existing: true)
+    return unless notify?
+
+    delete_existing_task_notifications!(user_id:) if delete_existing
+
+    TaskNotificationJob.perform_in(ENV.fetch("TASK_NOTIFICATION_DELAY", 20).to_i.seconds, user_id, measure_id)
+  end
+
+  def task?
+    measuretype&.notifications?
+  end
+
   private
 
   def delete_existing_task_notifications!(user_id:)
@@ -105,8 +121,4 @@ def parent_id_allowed_by_measuretype
   def relationship_updated?
     saved_change_to_attribute?(:relationship_updated_at)
   end
-
-  def task?
-    measuretype&.notifications?
-  end
 end

app/models/user_measure.rb

@@ -10,10 +10,35 @@ def notify?
     measure.notifications?
   end
 
+  after_commit :set_relationship_created, on: [:create]
   after_commit :set_relationship_updated, on: [:update, :destroy]
 
   private
 
+  def set_relationship_created
+    if user && !user.destroyed?
+      user.update_column(:relationship_updated_at, Time.zone.now)
+      user.update_column(:relationship_updated_by_id, ::PaperTrail.request.whodunnit)
+    end
+
+    if measure && !measure.destroyed?
+      measure.update_column(:relationship_updated_at, Time.zone.now)
+      measure.update_column(:relationship_updated_by_id, ::PaperTrail.request.whodunnit)
+
+      if measure.task?
+        self.class
+          .where(measure_id: measure_id)
+          .where.not(user_id: [user_id, ::PaperTrail.request.whodunnit])
+          .find_each do |other_user_measure|
+            measure.queue_task_updated_notification!(
+              user_id: other_user_measure.user_id,
+              measure_id: other_user_measure.measure_id
+            )
+          end
+      end
+    end
+  end
+
   def set_relationship_updated
     if measure && !measure.destroyed?
       measure.update_attribute(:relationship_updated_at, Time.zone.now)

spec/controllers/measures_controller_spec.rb

@@ -391,7 +391,7 @@
             let(:measure) { FactoryBot.create(:measure, :published, notifications: true) }
 
             it "notifies the user of an update to #{attr}" do
-              expect(TaskNotificationJob).to receive(:perform_in).with(ENV.fetch("TASK_NOTIFICATION_DELAY", 20).to_i.seconds, user_measure.id)
+              expect(TaskNotificationJob).to receive(:perform_in).with(ENV.fetch("TASK_NOTIFICATION_DELAY", 20).to_i.seconds, user_measure.user_id, user_measure.measure_id)
 
               put :update, format: :json, params: {id: measure, measure: {attr => "test"}}
             end
@@ -418,7 +418,7 @@
 
             context "and is updated to not archived" do
               it "does notify the user of an update to #{attr}" do
-                expect(TaskNotificationJob).to receive(:perform_in).with(ENV.fetch("TASK_NOTIFICATION_DELAY", 20).to_i.seconds, user_measure.id)
+                expect(TaskNotificationJob).to receive(:perform_in).with(ENV.fetch("TASK_NOTIFICATION_DELAY", 20).to_i.seconds, user_measure.user_id, user_measure.measure_id)
 
                 put :update, format: :json, params: {id: measure, measure: {attr => "test", :is_archive => false}}
               end

spec/models/measure_spec.rb

@@ -130,7 +130,7 @@
         let(:user_id) { FactoryBot.create(:user).id }
 
         it "will queue notifications when relationship_updated_at changes" do
-          expect(TaskNotificationJob).to receive(:perform_in).with(ENV.fetch("TASK_NOTIFICATION_DELAY", 20).to_i.seconds, user_measure.id)
+          expect(TaskNotificationJob).to receive(:perform_in).with(ENV.fetch("TASK_NOTIFICATION_DELAY", 20).to_i.seconds, user_measure.user_id, user_measure.measure_id)
 
           subject.touch(:relationship_updated_at)
         end

spec/models/user_measure_spec.rb

@@ -17,6 +17,46 @@
 
     subject { described_class.create(user: user, measure: measure) }
 
+    it "create sets the relationship_updated_at on the user" do
+      expect { subject }.to change { user.reload.relationship_updated_at }
+    end
+
+    it "create sets the relationship_updated_at on the measure" do
+      expect { subject }.to change { measure.reload.relationship_updated_at }
+    end
+
+    context "when the measure is a task" do
+      let(:measure) { FactoryBot.create(:measure, measuretype: FactoryBot.create(:measuretype, notifications: true)) }
+
+      it "create does not send a task updated email to the user it was just assigned to" do
+        expect(TaskNotificationJob).not_to receive(:perform_in).with(anything, user.id, measure.id)
+        expect(measure).not_to receive(:queue_task_updated_notifications!)
+
+        subject
+      end
+
+      context "when there are other users assigned to the task" do
+        let(:other_user) { FactoryBot.create(:user) }
+
+        before do
+          FactoryBot.create(:user_measure, user: other_user, measure: measure)
+        end
+
+        it "create sends a task updated email to the other users assigned to it" do
+          expect(measure).not_to receive(:queue_task_updated_notifications!)
+          expect(measure).to receive(:queue_task_updated_notification!)
+            .with(user_id: other_user.id, measure_id: measure.id)
+            .and_call_original
+          expect(TaskNotificationJob).to receive(:perform_in)
+            .with(anything, other_user.id, measure.id)
+          expect(TaskNotificationJob).not_to receive(:perform_in)
+            .with(anything, user.id, measure.id)
+
+          subject
+        end
+      end
+    end
+
     it "update sets the relationship_updated_at on the user" do
       subject
       expect { subject.touch }.to change { user.reload.relationship_updated_at }
@@ -35,6 +75,14 @@
       expect { subject.destroy }.to change { measure.reload.relationship_updated_at }
     end
 
+    it "create sets the relationship_updated_by_id on the user" do
+      expect { subject }.to change { user.reload.relationship_updated_by_id }.to(whodunnit)
+    end
+
+    it "create sets the relationship_updated_by_id on the measure" do
+      expect { subject }.to change { measure.reload.relationship_updated_by_id }.to(whodunnit)
+    end
+
     it "update sets the relationship_updated_by_id on the user" do
       subject
       user.update_column(:relationship_updated_by_id, nil)