Fix notifications for private group mentions

- Send notifications when a private group is
mentioned in the title or description of and issue
or merge request
- Do not create a new todo for group members when
editing a note where a private group was mentioned

app/services/issues/update_service.rb

@@ -56,7 +56,7 @@ module Issues
 
       handle_milestone_change(issue)
 
-      added_mentions = issue.mentioned_users - old_mentioned_users
+      added_mentions = issue.mentioned_users(current_user) - old_mentioned_users
 
       if added_mentions.present?
         notification_service.async.new_mentions_in_issue(issue, added_mentions, current_user)

app/services/merge_requests/update_service.rb

@@ -69,7 +69,8 @@ module MergeRequests
         )
       end
 
-      added_mentions = merge_request.mentioned_users - old_mentioned_users
+      added_mentions = merge_request.mentioned_users(current_user) - old_mentioned_users
+
       if added_mentions.present?
         notification_service.async.new_mentions_in_merge_request(
           merge_request,

app/services/notes/update_service.rb

@@ -5,7 +5,7 @@ module Notes
     def execute(note)
       return note unless note.editable?
 
-      old_mentioned_users = note.mentioned_users.to_a
+      old_mentioned_users = note.mentioned_users(current_user).to_a
 
       note.update(params.merge(updated_by: current_user))
 

changelogs/unreleased/30112-private-internal-sub-group-email-notifications-not-sent-when-mentio.yml

+---
+title: Fix notifications for private group mentions in Notes, Issues, and Merge Requests
+merge_request: 18183
+author:
+type: fixed

spec/services/issues/update_service_spec.rb

@@ -6,7 +6,8 @@ describe Issues::UpdateService, :mailer do
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
   let(:user3) { create(:user) }
-  let(:project) { create(:project) }
+  let(:group) { create(:group, :public) }
+  let(:project) { create(:project, :repository, group: group) }
   let(:label) { create(:label, project: project) }
   let(:label2) { create(:label) }
 
@@ -667,6 +668,7 @@ describe Issues::UpdateService, :mailer do
 
     context 'updating mentions' do
       let(:mentionable) { issue }
+
       include_examples 'updating mentions', described_class
     end
 

spec/services/merge_requests/update_service_spec.rb

@@ -5,7 +5,8 @@ require 'spec_helper'
 describe MergeRequests::UpdateService, :mailer do
   include ProjectForksHelper
 
-  let(:project) { create(:project, :repository) }
+  let(:group) { create(:group, :public) }
+  let(:project) { create(:project, :repository, group: group) }
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
   let(:user3) { create(:user) }
@@ -472,6 +473,7 @@ describe MergeRequests::UpdateService, :mailer do
 
     context 'updating mentions' do
       let(:mentionable) { merge_request }
+
       include_examples 'updating mentions', described_class
     end
 

spec/services/notes/update_service_spec.rb

@@ -3,17 +3,25 @@
 require 'spec_helper'
 
 describe Notes::UpdateService do
-  let(:project) { create(:project) }
+  let(:group) { create(:group, :public) }
+  let(:project) { create(:project, :public, group: group) }
+  let(:private_group) { create(:group, :private) }
+  let(:private_project) { create(:project, :private, group: private_group) }
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
   let(:user3) { create(:user) }
   let(:issue) { create(:issue, project: project) }
+  let(:issue2) { create(:issue, project: private_project) }
   let(:note) { create(:note, project: project, noteable: issue, author: user, note: "Old note #{user2.to_reference}") }
 
   before do
     project.add_maintainer(user)
     project.add_developer(user2)
     project.add_developer(user3)
+    group.add_developer(user3)
+    private_group.add_developer(user)
+    private_group.add_developer(user2)
+    private_project.add_developer(user3)
   end
 
   describe '#execute' do
@@ -46,13 +54,17 @@ describe Notes::UpdateService do
     end
 
     context 'todos' do
-      let!(:todo) { create(:todo, :assigned, user: user, project: project, target: issue, author: user2) }
+      shared_examples 'does not update todos' do
+        it 'keep todos' do
+          expect(todo.reload).to be_pending
+        end
 
-      context 'when the note change' do
-        before do
-          update_note({ note: "New note #{user2.to_reference} #{user3.to_reference}" })
+        it 'does not create any new todos' do
+          expect(Todo.count).to eq(1)
+        end
       end
 
+      shared_examples 'creates one todo' do
         it 'marks todos as done' do
           expect(todo.reload).to be_done
         end
@@ -62,17 +74,75 @@ describe Notes::UpdateService do
         end
       end
 
-      context 'when the note does not change' do
+      context 'when note includes a user mention' do
+        let!(:todo) { create(:todo, :assigned, user: user, project: project, target: issue, author: user2) }
+
+        context 'when the note does not change mentions' do
           before do
             update_note({ note: "Old note #{user2.to_reference}" })
           end
 
-        it 'keep todos' do
-          expect(todo.reload).to be_pending
+          it_behaves_like 'does not update todos'
         end
 
-        it 'does not create any new todos' do
-          expect(Todo.count).to eq(1)
+        context 'when the note changes to include one more user mention' do
+          before do
+            update_note({ note: "New note #{user2.to_reference} #{user3.to_reference}" })
+          end
+
+          it_behaves_like 'creates one todo'
+        end
+
+        context 'when the note changes to include a group mentions' do
+          before do
+            update_note({ note: "New note #{private_group.to_reference}" })
+          end
+
+          it_behaves_like 'creates one todo'
+        end
+      end
+
+      context 'when note includes a group mention' do
+        context 'when the group is public' do
+          let(:note) { create(:note, project: project, noteable: issue, author: user, note: "Old note #{group.to_reference}") }
+          let!(:todo) { create(:todo, :assigned, user: user, project: project, target: issue, author: user2) }
+
+          context 'when the note does not change mentions' do
+            before do
+              update_note({ note: "Old note #{group.to_reference}" })
+            end
+
+            it_behaves_like 'does not update todos'
+          end
+
+          context 'when the note changes mentions' do
+            before do
+              update_note({ note: "New note #{user2.to_reference} #{user3.to_reference}" })
+            end
+
+            it_behaves_like 'creates one todo'
+          end
+        end
+
+        context 'when the group is private' do
+          let(:note) { create(:note, project: project, noteable: issue, author: user, note: "Old note #{private_group.to_reference}") }
+          let!(:todo) { create(:todo, :assigned, user: user, project: project, target: issue, author: user2) }
+
+          context 'when the note does not change mentions' do
+            before do
+              update_note({ note: "Old note #{private_group.to_reference}" })
+            end
+
+            it_behaves_like 'does not update todos'
+          end
+
+          context 'when the note changes mentions' do
+            before do
+              update_note({ note: "New note #{user2.to_reference} #{user3.to_reference}" })
+            end
+
+            it_behaves_like 'creates one todo'
+          end
         end
       end
     end

spec/support/shared_examples/updating_mentions_shared_examples.rb

 # frozen_string_literal: true
 
 RSpec.shared_examples 'updating mentions' do |service_class|
-  let(:mentioned_user) { create(:user) }
   let(:service_class)   { service_class }
+  let(:mentioned_user)  { create(:user) }
+  let(:group_member1)   { create(:user) }
+  let(:group_member2)   { create(:user) }
+  let(:external_group)  { create(:group, :private) }
 
   before do
     project.add_developer(mentioned_user)
+    group.add_developer(group_member1)
+    group.add_developer(group_member2)
   end
 
   def update_mentionable(opts)
@@ -16,9 +21,10 @@ RSpec.shared_examples 'updating mentions' do |service_class|
     mentionable.reload
   end
 
+  context 'when mentioning a different user' do
     context 'in title' do
       before do
-      update_mentionable(title: mentioned_user.to_reference)
+        update_mentionable(title: "For #{mentioned_user.to_reference}")
       end
 
       it 'emails only the newly-mentioned user' do
@@ -28,11 +34,61 @@ RSpec.shared_examples 'updating mentions' do |service_class|
 
     context 'in description' do
       before do
-      update_mentionable(description: mentioned_user.to_reference)
+        update_mentionable(description: "For #{mentioned_user.to_reference}")
       end
 
       it 'emails only the newly-mentioned user' do
         should_only_email(mentioned_user)
       end
     end
+  end
+
+  context 'when mentioning a user and a group with access to' do
+    shared_examples 'updating attribute with allowed mentions' do |attribute|
+      before do
+        update_mentionable(
+          { attribute => "For #{group.to_reference}, cc: #{mentioned_user.to_reference}" }
+        )
+      end
+
+      it 'emails group members' do
+        should_email(mentioned_user)
+        should_email(group_member1)
+        should_email(group_member2)
+      end
+    end
+
+    context 'when group is public' do
+      it_behaves_like 'updating attribute with allowed mentions', :title
+      it_behaves_like 'updating attribute with allowed mentions', :description
+    end
+
+    context 'when the group is private' do
+      before do
+        group.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+      end
+
+      it_behaves_like 'updating attribute with allowed mentions', :title
+      it_behaves_like 'updating attribute with allowed mentions', :description
+    end
+  end
+
+  context 'when mentioning a user and a group without access to' do
+    shared_examples 'updating attribute with not allowed mentions' do |attribute|
+      before do
+        update_mentionable(
+          { attribute => "For #{external_group.to_reference}, cc: #{mentioned_user.to_reference}" }
+        )
+      end
+
+      it 'emails mentioned user' do
+        should_only_email(mentioned_user)
+      end
+    end
+
+    context 'when the group is private' do
+      it_behaves_like 'updating attribute with not allowed mentions', :title
+      it_behaves_like 'updating attribute with not allowed mentions', :description
+    end
+  end
 end