Merge branch 'support-notifications-on-project-snippets' into 'master'

Support e-mail notifications for comments on project snippets

While working with project snippets recently, I noticed that notifications would not be sent out for comments on notes. This MR fixes this.

Note: I'm not completely sure why `ProjectSnippets#participants` returns an empty array if you don't include the concern that is already in `Snippets` but didn't dig into it any more.

Closes #2334

See merge request !3987

CHANGELOG

@@ -4,6 +4,7 @@ v 8.8.0 (unreleased)
   - Project#open_branches has been cleaned up and no longer loads entire records into memory.
   - Make build status canceled if any of the jobs was canceled and none failed
   - Remove future dates from contribution calendar graph.
+  - Support e-mail notifications for comments on project snippets
   - Use ActionDispatch Remote IP for Akismet checking
   - Fix error when visiting commit builds page before build was updated
   - Add 'l' shortcut to open Label dropdown on issuables and 'i' to create new issue on a project

app/mailers/emails/notes.rb

@@ -28,6 +28,14 @@ module Emails
       mail_answer_thread(@merge_request, note_thread_options(recipient_id))
     end
 
+    def note_snippet_email(recipient_id, note_id)
+      setup_note_mail(note_id, recipient_id)
+
+      @snippet = @note.noteable
+      @target_url = namespace_project_snippet_url(*note_target_url_options)
+      mail_answer_thread(@snippet, note_thread_options(recipient_id))
+    end
+
     private
 
     def note_target_url_options

app/models/project_snippet.rb

@@ -22,4 +22,6 @@ class ProjectSnippet < Snippet
 
   # Scopes
   scope :fresh, -> { order("created_at DESC") }
+
+  participant :author, :notes
 end

app/views/notify/note_snippet_email.html.haml

+= render 'note_message'

app/views/notify/note_snippet_email.text.erb

+New comment for Snippet <%= @snippet.id %>
+
+<%= url_for(namespace_project_snippet_url(@snippet.project.namespace, @snippet.project, @snippet, anchor: "note_#{@note.id}")) %>
+
+
+Author: <%= @note.author_name %>
+
+<%= @note.note %>

spec/services/notification_service_spec.rb

@@ -10,7 +10,7 @@ describe NotificationService, services: true do
   end
 
   describe 'Keys' do
-    describe :new_key do
+    describe '#new_key' do
       let!(:key) { create(:personal_key) }
 
       it { expect(notification.new_key(key)).to be_truthy }
@@ -22,7 +22,7 @@ describe NotificationService, services: true do
   end
 
   describe 'Email' do
-    describe :new_email do
+    describe '#new_email' do
       let!(:email) { create(:email) }
 
       it { expect(notification.new_email(email)).to be_truthy }
@@ -147,8 +147,8 @@ describe NotificationService, services: true do
         ActionMailer::Base.deliveries.clear
       end
 
-      describe :new_note do
-        it do
+      describe '#new_note' do
+        it 'notifies the team members' do
           notification.new_note(note)
 
           # Notify all team members
@@ -177,6 +177,39 @@ describe NotificationService, services: true do
       end
     end
 
+    context 'project snippet note' do
+      let(:project) { create(:empty_project, :public) }
+      let(:snippet) { create(:project_snippet, project: project, author: create(:user)) }
+      let(:note) { create(:note_on_project_snippet, noteable: snippet, project_id: snippet.project.id, note: '@all mentioned') }
+
+      before do
+        build_team(note.project)
+        note.project.team << [note.author, :master]
+        ActionMailer::Base.deliveries.clear
+      end
+
+      describe '#new_note' do
+        it 'notifies the team members' do
+          notification.new_note(note)
+
+          # Notify all team members
+          note.project.team.members.each do |member|
+            # User with disabled notification should not be notified
+            next if member.id == @u_disabled.id
+            # Author should not be notified
+            next if member.id == note.author.id
+            should_email(member)
+          end
+
+          should_email(note.noteable.author)
+          should_not_email(note.author)
+          should_email(@u_mentioned)
+          should_not_email(@u_disabled)
+          should_email(@u_not_mentioned)
+        end
+      end
+    end
+
     context 'commit note' do
       let(:project) { create(:project, :public) }
       let(:note) { create(:note_on_commit, project: project) }
@@ -187,7 +220,7 @@ describe NotificationService, services: true do
         allow_any_instance_of(Commit).to receive(:author).and_return(@u_committer)
       end
 
-      describe :new_note, :perform_enqueued_jobs do
+      describe '#new_note, #perform_enqueued_jobs' do
         it do
           notification.new_note(note)
 
@@ -230,7 +263,7 @@ describe NotificationService, services: true do
       ActionMailer::Base.deliveries.clear
     end
 
-    describe :new_issue do
+    describe '#new_issue' do
       it do
         notification.new_issue(issue, @u_disabled)
 
@@ -289,7 +322,7 @@ describe NotificationService, services: true do
       end
     end
 
-    describe :reassigned_issue do
+    describe '#reassigned_issue' do
       it 'emails new assignee' do
         notification.reassigned_issue(issue, @u_disabled)
 
@@ -419,7 +452,7 @@ describe NotificationService, services: true do
       end
     end
 
-    describe :close_issue do
+    describe '#close_issue' do
       it 'should sent email to issue assignee and issue author' do
         notification.close_issue(issue, @u_disabled)
 
@@ -435,7 +468,7 @@ describe NotificationService, services: true do
       end
     end
 
-    describe :reopen_issue do
+    describe '#reopen_issue' do
       it 'should send email to issue assignee and issue author' do
         notification.reopen_issue(issue, @u_disabled)
 
@@ -461,7 +494,7 @@ describe NotificationService, services: true do
       ActionMailer::Base.deliveries.clear
     end
 
-    describe :new_merge_request do
+    describe '#new_merge_request' do
       it do
         notification.new_merge_request(merge_request, @u_disabled)
 
@@ -483,7 +516,7 @@ describe NotificationService, services: true do
       end
     end
 
-    describe :reassigned_merge_request do
+    describe '#reassigned_merge_request' do
       it do
         notification.reassigned_merge_request(merge_request, merge_request.author)
 
@@ -498,7 +531,7 @@ describe NotificationService, services: true do
       end
     end
 
-    describe :relabel_merge_request do
+    describe '#relabel_merge_request' do
       let(:label) { create(:label, merge_requests: [merge_request]) }
       let(:label2) { create(:label) }
       let!(:subscriber_to_label) { create(:user).tap { |u| label.toggle_subscription(u) } }
@@ -527,7 +560,7 @@ describe NotificationService, services: true do
       end
     end
 
-    describe :closed_merge_request do
+    describe '#closed_merge_request' do
       it do
         notification.close_mr(merge_request, @u_disabled)
 
@@ -542,7 +575,7 @@ describe NotificationService, services: true do
       end
     end
 
-    describe :merged_merge_request do
+    describe '#merged_merge_request' do
       it do
         notification.merge_mr(merge_request, @u_disabled)
 
@@ -557,7 +590,7 @@ describe NotificationService, services: true do
       end
     end
 
-    describe :reopen_merge_request do
+    describe '#reopen_merge_request' do
       it do
         notification.reopen_mr(merge_request, @u_disabled)
 
@@ -581,7 +614,7 @@ describe NotificationService, services: true do
       ActionMailer::Base.deliveries.clear
     end
 
-    describe :project_was_moved do
+    describe '#project_was_moved' do
       it do
         notification.project_was_moved(project, "gitlab/gitlab")