From a237f967e1cb6484b5083c4605fc4c3f57f47f09 Mon Sep 17 00:00:00 2001 From: Hampton Lintorn-Catlin Date: Fri, 3 Apr 2026 14:53:31 -0500 Subject: [PATCH] Fix notification targeting: don't notify unrelated reviewers New comments now only notify plan authors (created_by_user + author-role collaborators), not all reviewers. Reviewer A leaving feedback no longer pings Reviewers B, C, D who have nothing to do with that thread. Reviewers are still notified for replies to threads they participated in. --- .../services/coplan/notifications/create.rb | 2 +- spec/services/notifications/create_spec.rb | 24 ++++++++++++++++--- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/engine/app/services/coplan/notifications/create.rb b/engine/app/services/coplan/notifications/create.rb index 8f81d54..ed9502d 100644 --- a/engine/app/services/coplan/notifications/create.rb +++ b/engine/app/services/coplan/notifications/create.rb @@ -53,7 +53,7 @@ def plan_author_ids ids = Set[plan.created_by_user_id] ids.merge( plan.plan_collaborators - .where(role: %w[author reviewer]) + .where(role: "author") .pluck(:user_id) ) ids diff --git a/spec/services/notifications/create_spec.rb b/spec/services/notifications/create_spec.rb index 9febac7..dbeca23 100644 --- a/spec/services/notifications/create_spec.rb +++ b/spec/services/notifications/create_spec.rb @@ -28,15 +28,24 @@ expect(result).to be_nil end - it "notifies plan collaborators with author/reviewer role" do - create(:plan_collaborator, plan: plan, user: reviewer, role: "reviewer") + it "notifies plan collaborators with author role" do + co_author = create(:coplan_user) + create(:plan_collaborator, plan: plan, user: co_author, role: "author") expect { described_class.call(comment_thread: thread, actor_id: commenter.id, reason: "new_comment") }.to change(CoPlan::Notification, :count).by(2) notified_user_ids = CoPlan::Notification.pluck(:user_id) - expect(notified_user_ids).to contain_exactly(plan_author.id, reviewer.id) + expect(notified_user_ids).to contain_exactly(plan_author.id, co_author.id) + end + + it "does not notify reviewer collaborators" do + create(:plan_collaborator, plan: plan, user: reviewer, role: "reviewer") + + described_class.call(comment_thread: thread, actor_id: commenter.id, reason: "new_comment") + notified_user_ids = CoPlan::Notification.pluck(:user_id) + expect(notified_user_ids).not_to include(reviewer.id) end it "does not notify viewer collaborators" do @@ -59,6 +68,15 @@ expect(notified_user_ids).to contain_exactly(plan_author.id, commenter.id) end + it "does not notify reviewer collaborators who are not thread participants" do + other_reviewer = create(:coplan_user) + create(:plan_collaborator, plan: plan, user: other_reviewer, role: "reviewer") + + described_class.call(comment_thread: thread, actor_id: create(:coplan_user).id, reason: "reply") + notified_user_ids = CoPlan::Notification.pluck(:user_id) + expect(notified_user_ids).not_to include(other_reviewer.id) + end + it "notifies prior human commenters in the thread" do prior_commenter = create(:coplan_user) create(:comment, comment_thread: thread, author_type: "human", author_id: prior_commenter.id)