Merge branch '23888-fix-unsubscription-link-for-snippet-notification' into 'master'

Don't show an "Unsubscribe" link in snippet comment notifications

Closes #23888

See merge request gitlab-org/gitlab-ce!14764

app/models/sent_notification.rb

@@ -53,13 +53,17 @@ class SentNotification < ActiveRecord::Base
   end
 
   def unsubscribable?
-    !for_commit?
+    !(for_commit? || for_snippet?)
   end
 
   def for_commit?
     noteable_type == "Commit"
   end
 
+  def for_snippet?
+    noteable_type.end_with?('Snippet')
+  end
+
   def noteable
     if for_commit?
       project.commit(commit_id) rescue nil

changelogs/unreleased/23888-fix-unsubscription-link-for-snippet-notification.yml

+---
+title: Don't show an "Unsubscribe" link in snippet comment notifications
+merge_request: 14764
+author:
+type: fixed

spec/mailers/notify_spec.rb

@@ -28,18 +28,15 @@ describe Notify do
   end
 
   def have_referable_subject(referable, reply: false)
-    prefix = referable.project.name if referable.project
-    prefix = "Re: #{prefix}" if reply
+    prefix = referable.project ? "#{referable.project.name} | " : ''
+    prefix.prepend('Re: ') if reply
 
     suffix = "#{referable.title} (#{referable.to_reference})"
 
-    have_subject [prefix, suffix].compact.join(' | ')
+    have_subject [prefix, suffix].compact.join
   end
 
   context 'for a project' do
-    describe 'items that are assignable, the email' do
-      let(:previous_assignee) { create(:user, name: 'Previous Assignee') }
-
     shared_examples 'an assignee email' do
       it 'is sent to the assignee as the author' do
         sender = subject.header[:from].addrs.first
@@ -86,7 +83,8 @@ describe Notify do
         end
       end
 
-        describe 'that have been reassigned' do
+      describe 'that are reassigned' do
+        let(:previous_assignee) { create(:user, name: 'Previous Assignee') }
         subject { described_class.reassigned_issue_email(recipient.id, issue.id, [previous_assignee.id], current_user.id) }
 
         it_behaves_like 'a multiple recipients email'
@@ -241,6 +239,7 @@ describe Notify do
       end
 
       describe 'that are reassigned' do
+        let(:previous_assignee) { create(:user, name: 'Previous Assignee') }
         subject { described_class.reassigned_merge_request_email(recipient.id, merge_request.id, previous_assignee.id, current_user.id) }
 
         it_behaves_like 'a multiple recipients email'
@@ -342,6 +341,22 @@ describe Notify do
         end
       end
     end
+
+    context 'for snippet notes' do
+      let(:project_snippet) { create(:project_snippet, project: project) }
+      let(:project_snippet_note) { create(:note_on_project_snippet, project: project, noteable: project_snippet) }
+
+      subject { described_class.note_snippet_email(project_snippet_note.author_id, project_snippet_note.id) }
+
+      it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
+        let(:model) { project_snippet }
+      end
+      it_behaves_like 'a user cannot unsubscribe through footer link'
+
+      it 'has the correct subject and body' do
+        is_expected.to have_referable_subject(project_snippet, reply: true)
+        is_expected.to have_html_escaped_body_text project_snippet_note.note
+      end
     end
 
     describe 'project was moved' do
@@ -1239,4 +1254,18 @@ describe Notify do
       end
     end
   end
+
+  context 'for personal snippet notes' do
+    let(:personal_snippet) { create(:personal_snippet) }
+    let(:personal_snippet_note) { create(:note_on_personal_snippet, noteable: personal_snippet) }
+
+    subject { described_class.note_personal_snippet_email(personal_snippet_note.author_id, personal_snippet_note.id) }
+
+    it_behaves_like 'a user cannot unsubscribe through footer link'
+
+    it 'has the correct subject and body' do
+      is_expected.to have_referable_subject(personal_snippet, reply: true)
+      is_expected.to have_html_escaped_body_text personal_snippet_note.note
+    end
+  end
 end

spec/models/sent_notification_spec.rb

 require 'spec_helper'
 
 describe SentNotification do
+  set(:user) { create(:user) }
+  set(:project) { create(:project) }
+
   describe 'validation' do
     describe 'note validity' do
       context "when the project doesn't match the noteable's project" do
@@ -34,7 +37,6 @@ describe SentNotification do
   end
 
   describe '.record' do
-    let(:user) { create(:user) }
     let(:issue) { create(:issue) }
 
     it 'creates a new SentNotification' do
@@ -43,7 +45,6 @@ describe SentNotification do
   end
 
   describe '.record_note' do
-    let(:user) { create(:user) }
     let(:note) { create(:diff_note_on_merge_request) }
 
     it 'creates a new SentNotification' do
@@ -51,6 +52,123 @@ describe SentNotification do
     end
   end
 
+  describe '#unsubscribable?' do
+    shared_examples 'an unsubscribable notification' do |noteable_type|
+      subject { described_class.record(noteable, user.id) }
+
+      context "for #{noteable_type}" do
+        it { expect(subject).to be_unsubscribable }
+      end
+    end
+
+    shared_examples 'a non-unsubscribable notification' do |noteable_type|
+      subject { described_class.record(noteable, user.id) }
+
+      context "for a #{noteable_type}" do
+        it { expect(subject).not_to be_unsubscribable }
+      end
+    end
+
+    it_behaves_like 'an unsubscribable notification', 'issue' do
+      let(:noteable) { create(:issue, project: project) }
+    end
+
+    it_behaves_like 'an unsubscribable notification', 'merge request' do
+      let(:noteable) { create(:merge_request, source_project: project) }
+    end
+
+    it_behaves_like 'a non-unsubscribable notification', 'commit' do
+      let(:project) { create(:project, :repository) }
+      let(:noteable) { project.commit }
+    end
+
+    it_behaves_like 'a non-unsubscribable notification', 'personal snippet' do
+      let(:noteable) { create(:personal_snippet, project: project) }
+    end
+
+    it_behaves_like 'a non-unsubscribable notification', 'project snippet' do
+      let(:noteable) { create(:project_snippet, project: project) }
+    end
+  end
+
+  describe '#for_commit?' do
+    shared_examples 'a commit notification' do |noteable_type|
+      subject { described_class.record(noteable, user.id) }
+
+      context "for #{noteable_type}" do
+        it { expect(subject).to be_for_commit }
+      end
+    end
+
+    shared_examples 'a non-commit notification' do |noteable_type|
+      subject { described_class.record(noteable, user.id) }
+
+      context "for a #{noteable_type}" do
+        it { expect(subject).not_to be_for_commit }
+      end
+    end
+
+    it_behaves_like 'a non-commit notification', 'issue' do
+      let(:noteable) { create(:issue, project: project) }
+    end
+
+    it_behaves_like 'a non-commit notification', 'merge request' do
+      let(:noteable) { create(:merge_request, source_project: project) }
+    end
+
+    it_behaves_like 'a commit notification', 'commit' do
+      let(:project) { create(:project, :repository) }
+      let(:noteable) { project.commit }
+    end
+
+    it_behaves_like 'a non-commit notification', 'personal snippet' do
+      let(:noteable) { create(:personal_snippet, project: project) }
+    end
+
+    it_behaves_like 'a non-commit notification', 'project snippet' do
+      let(:noteable) { create(:project_snippet, project: project) }
+    end
+  end
+
+  describe '#for_snippet?' do
+    shared_examples 'a snippet notification' do |noteable_type|
+      subject { described_class.record(noteable, user.id) }
+
+      context "for #{noteable_type}" do
+        it { expect(subject).to be_for_snippet }
+      end
+    end
+
+    shared_examples 'a non-snippet notification' do |noteable_type|
+      subject { described_class.record(noteable, user.id) }
+
+      context "for a #{noteable_type}" do
+        it { expect(subject).not_to be_for_snippet }
+      end
+    end
+
+    it_behaves_like 'a non-snippet notification', 'issue' do
+      let(:noteable) { create(:issue, project: project) }
+    end
+
+    it_behaves_like 'a non-snippet notification', 'merge request' do
+      let(:noteable) { create(:merge_request, source_project: project) }
+    end
+
+    it_behaves_like 'a non-snippet notification', 'commit' do
+      let(:project) { create(:project, :repository) }
+      let(:noteable) { project.commit }
+    end
+
+    it_behaves_like 'a snippet notification', 'personal snippet' do
+      let(:noteable) { create(:personal_snippet, project: project) }
+    end
+
+    it_behaves_like 'a snippet notification', 'project snippet' do
+      let(:noteable) { create(:project_snippet, project: project) }
+    end
+  end
+
   describe '#create_reply' do
     context 'for issue' do
       let(:issue) { create(:issue) }