From 85d5b92740ef84a71045247b99725de31d3834fa Mon Sep 17 00:00:00 2001 From: Philip Arndt Date: Sat, 27 May 2023 17:10:21 +1200 Subject: [PATCH 1/2] Send task updated notifications when tasks are updated or created for other users --- app/jobs/task_notification_job.rb | 6 +-- app/models/measure.rb | 17 +++++-- app/models/user_measure.rb | 25 ++++++++++ spec/controllers/measures_controller_spec.rb | 4 +- spec/models/measure_spec.rb | 2 +- spec/models/user_measure_spec.rb | 48 ++++++++++++++++++++ 6 files changed, 91 insertions(+), 11 deletions(-) diff --git a/app/jobs/task_notification_job.rb b/app/jobs/task_notification_job.rb index 85f0592..1c9fdc2 100644 --- a/app/jobs/task_notification_job.rb +++ b/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 diff --git a/app/models/measure.rb b/app/models/measure.rb index 6612cff..b2aec2f 100644 --- a/app/models/measure.rb +++ b/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,20 @@ 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) end end + def queue_task_updated_notification!(user_id:, measure_id:) + return unless notify? + + 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:) @@ -106,7 +116,4 @@ def relationship_updated? saved_change_to_attribute?(:relationship_updated_at) end - def task? - measuretype&.notifications? - end end diff --git a/app/models/user_measure.rb b/app/models/user_measure.rb index 00b759d..56690c1 100644 --- a/app/models/user_measure.rb +++ b/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) diff --git a/spec/controllers/measures_controller_spec.rb b/spec/controllers/measures_controller_spec.rb index ed68b79..fbabf88 100644 --- a/spec/controllers/measures_controller_spec.rb +++ b/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 diff --git a/spec/models/measure_spec.rb b/spec/models/measure_spec.rb index 005b946..35c67be 100644 --- a/spec/models/measure_spec.rb +++ b/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 diff --git a/spec/models/user_measure_spec.rb b/spec/models/user_measure_spec.rb index 9aac4d6..fc75547 100644 --- a/spec/models/user_measure_spec.rb +++ b/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) From 3abc3872e91ab74243c946386f750fa53ba57772 Mon Sep 17 00:00:00 2001 From: Philip Arndt Date: Tue, 19 Dec 2023 20:51:54 +1300 Subject: [PATCH 2/2] delete_existing_task_notifications in queue_task_updated_notification if required --- app/models/measure.rb | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/app/models/measure.rb b/app/models/measure.rb index b2aec2f..e9bab21 100644 --- a/app/models/measure.rb +++ b/app/models/measure.rb @@ -61,13 +61,19 @@ 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| - queue_task_updated_notification!(user_id: user_measure.user_id, measure_id: user_measure.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:) + 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 @@ -115,5 +121,4 @@ def parent_id_allowed_by_measuretype def relationship_updated? saved_change_to_attribute?(:relationship_updated_at) end - end