Merge branch 'maxiperezc/gitlab-ce-issues_17198' into 'master'

Fix "Unsubscribe" link in notification emails that is triggered by anti-virus

## What does this MR do?

* The unsubscribe link in an email body only unsubscribes automatically when logged in, otherwise the user is asked for a confirmation.
* The unsubscribe link in an email header unsubscribes automatically whether logged in or not.

## Are there points in the code the reviewer needs to double check?

This addresses all the comments from https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5241, I think!

## Why was this MR needed?

People were getting unsubscribed automatically by AV software.

## Screenshot

![Screen_Shot_2016-09-20_at_09.51.30](/uploads/083ee2865f1ad6c08e2ed97f1c4e7d0d/Screen_Shot_2016-09-20_at_09.51.30.png)

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- Tests
  - [x] Added for this feature/bug
  - [ ] All builds are passing
- [x] Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?

Fixes #17198.

See merge request !6223

4 files changed, 184 insertions, 18 deletions

diff --git a/spec/controllers/sent_notifications_controller_spec.rb b/spec/controllers/sent_notifications_controller_spec.rb
index 9ced397bd4a..191e290a118 100644
--- a/spec/controllers/sent_notifications_controller_spec.rb
+++ b/spec/controllers/sent_notifications_controller_spec.rb
@@ -1,25 +1,108 @@
 require 'rails_helper'
 
 describe SentNotificationsController, type: :controller do
-  let(:user)              { create(:user) }
-  let(:issue)             { create(:issue, author: user) }
-  let(:sent_notification) { create(:sent_notification, noteable: issue) }
+  let(:user) { create(:user) }
+  let(:project) { create(:empty_project) }
+  let(:sent_notification) { create(:sent_notification, noteable: issue, recipient: user) }
 
-  describe 'GET #unsubscribe' do
-    it 'returns a 404 when calling without existing id' do
-      get(:unsubscribe, id: '0' * 32)
+  let(:issue) do
+    create(:issue, project: project, author: user) do |issue|
+      issue.subscriptions.create(user: user, subscribed: true)
+    end
+  end
+
+  describe 'GET unsubscribe' do
+    context 'when the user is not logged in' do
+      context 'when the force param is passed' do
+        before { get(:unsubscribe, id: sent_notification.reply_key, force: true) }
+
+        it 'unsubscribes the user' do
+          expect(issue.subscribed?(user)).to be_falsey
+        end
+
+        it 'sets the flash message' do
+          expect(controller).to set_flash[:notice].to(/unsubscribed/).now
+        end
+
+        it 'redirects to the login page' do
+          expect(response).to redirect_to(new_user_session_path)
+        end
+      end
+
+      context 'when the force param is not passed' do
+        before { get(:unsubscribe, id: sent_notification.reply_key) }
+
+        it 'does not unsubscribe the user' do
+          expect(issue.subscribed?(user)).to be_truthy
+        end
 
-      expect(response.status).to be 404
+        it 'does not set the flash message' do
+          expect(controller).not_to set_flash[:notice]
+        end
+
+        it 'redirects to the login page' do
+          expect(response).to render_template :unsubscribe
+        end
+      end
     end
 
-    context 'calling with id' do
-      it 'shows a flash message to the user' do
-        get(:unsubscribe, id: sent_notification.reply_key)
+    context 'when the user is logged in' do
+      before { sign_in(user) }
+
+      context 'when the ID passed does not exist' do
+        before { get(:unsubscribe, id: sent_notification.reply_key.reverse) }
+
+        it 'does not unsubscribe the user' do
+          expect(issue.subscribed?(user)).to be_truthy
+        end
+
+        it 'does not set the flash message' do
+          expect(controller).not_to set_flash[:notice]
+        end
+
+        it 'returns a 404' do
+          expect(response).to have_http_status(:not_found)
+        end
+      end
+
+      context 'when the force param is passed' do
+        before { get(:unsubscribe, id: sent_notification.reply_key, force: true) }
+
+        it 'unsubscribes the user' do
+          expect(issue.subscribed?(user)).to be_falsey
+        end
+
+        it 'sets the flash message' do
+          expect(controller).to set_flash[:notice].to(/unsubscribed/).now
+        end
+
+        it 'redirects to the issue page' do
+          expect(response).
+            to redirect_to(namespace_project_issue_path(project.namespace, project, issue))
+        end
+      end
+
+      context 'when the force param is not passed' do
+        let(:merge_request) do
+          create(:merge_request, source_project: project, author: user) do |merge_request|
+            merge_request.subscriptions.create(user: user, subscribed: true)
+          end
+        end
+        let(:sent_notification) { create(:sent_notification, noteable: merge_request, recipient: user) }
+        before { get(:unsubscribe, id: sent_notification.reply_key) }
+
+        it 'unsubscribes the user' do
+          expect(merge_request.subscribed?(user)).to be_falsey
+        end
 
-        expect(response.status).to be 302
+        it 'sets the flash message' do
+          expect(controller).to set_flash[:notice].to(/unsubscribed/).now
+        end
 
-        expect(response).to redirect_to new_user_session_path
-        expect(controller).to set_flash[:notice].to(/unsubscribed/).now
+        it 'redirects to the merge request page' do
+          expect(response).
+            to redirect_to(namespace_project_merge_request_path(project.namespace, project, merge_request))
+        end
       end
     end
   end
diff --git a/spec/features/unsubscribe_links_spec.rb b/spec/features/unsubscribe_links_spec.rb
new file mode 100644
index 00000000000..cc40671787c
--- /dev/null
+++ b/spec/features/unsubscribe_links_spec.rb
@@ -0,0 +1,75 @@
+require 'spec_helper'
+
+describe 'Unsubscribe links', feature: true do
+  include Warden::Test::Helpers
+
+  let(:recipient) { create(:user) }
+  let(:author) { create(:user) }
+  let(:project) { create(:empty_project, :public) }
+  let(:params) { { title: 'A bug!', description: 'Fix it!', assignee: recipient } }
+  let(:issue) { Issues::CreateService.new(project, author, params).execute }
+
+  let(:mail) { ActionMailer::Base.deliveries.last }
+  let(:body) { Capybara::Node::Simple.new(mail.default_part_body.to_s) }
+  let(:header_link) { mail.header['List-Unsubscribe'] }
+  let(:body_link) { body.find_link('unsubscribe')['href'] }
+
+  before do
+    perform_enqueued_jobs { issue }
+  end
+
+  context 'when logged out' do
+    context 'when visiting the link from the body' do
+      it 'shows the unsubscribe confirmation page and redirects to root path when confirming' do
+        visit body_link
+
+        expect(current_path).to eq unsubscribe_sent_notification_path(SentNotification.last)
+        expect(page).to have_text(%(Unsubscribe from issue #{issue.title} (#{issue.to_reference})))
+        expect(page).to have_text(%(Are you sure you want to unsubscribe from issue #{issue.title} (#{issue.to_reference})?))
+        expect(issue.subscribed?(recipient)).to be_truthy
+
+        click_link 'Unsubscribe'
+
+        expect(issue.subscribed?(recipient)).to be_falsey
+        expect(current_path).to eq new_user_session_path
+      end
+
+      it 'shows the unsubscribe confirmation page and redirects to root path when canceling' do
+        visit body_link
+
+        expect(current_path).to eq unsubscribe_sent_notification_path(SentNotification.last)
+        expect(issue.subscribed?(recipient)).to be_truthy
+
+        click_link 'Cancel'
+
+        expect(issue.subscribed?(recipient)).to be_truthy
+        expect(current_path).to eq new_user_session_path
+      end
+    end
+
+    it 'unsubscribes from the issue when visiting the link from the header' do
+      visit header_link
+
+      expect(page).to have_text('unsubscribed')
+      expect(issue.subscribed?(recipient)).to be_falsey
+    end
+  end
+
+  context 'when logged in' do
+    before { login_as(recipient) }
+
+    it 'unsubscribes from the issue when visiting the link from the email body' do
+      visit body_link
+
+      expect(page).to have_text('unsubscribed')
+      expect(issue.subscribed?(recipient)).to be_falsey
+    end
+
+    it 'unsubscribes from the issue when visiting the link from the header' do
+      visit header_link
+
+      expect(page).to have_text('unsubscribed')
+      expect(issue.subscribed?(recipient)).to be_falsey
+    end
+  end
+end
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index eae9c060c38..0363bc74939 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -861,7 +861,7 @@ describe Notify do
     subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :create) }
 
     it_behaves_like 'it should not have Gmail Actions links'
-    it_behaves_like "a user cannot unsubscribe through footer link"
+    it_behaves_like 'a user cannot unsubscribe through footer link'
     it_behaves_like 'an email with X-GitLab headers containing project details'
     it_behaves_like 'an email that contains a header with author username'
 
@@ -914,7 +914,7 @@ describe Notify do
     subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :delete) }
 
     it_behaves_like 'it should not have Gmail Actions links'
-    it_behaves_like "a user cannot unsubscribe through footer link"
+    it_behaves_like 'a user cannot unsubscribe through footer link'
     it_behaves_like 'an email with X-GitLab headers containing project details'
     it_behaves_like 'an email that contains a header with author username'
 
@@ -936,7 +936,7 @@ describe Notify do
     subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/tags/v1.0', action: :delete) }
 
     it_behaves_like 'it should not have Gmail Actions links'
-    it_behaves_like "a user cannot unsubscribe through footer link"
+    it_behaves_like 'a user cannot unsubscribe through footer link'
     it_behaves_like 'an email with X-GitLab headers containing project details'
     it_behaves_like 'an email that contains a header with author username'
 
@@ -964,7 +964,7 @@ describe Notify do
     subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare, reverse_compare: false, diff_refs: diff_refs, send_from_committer_email: send_from_committer_email) }
 
     it_behaves_like 'it should not have Gmail Actions links'
-    it_behaves_like "a user cannot unsubscribe through footer link"
+    it_behaves_like 'a user cannot unsubscribe through footer link'
     it_behaves_like 'an email with X-GitLab headers containing project details'
     it_behaves_like 'an email that contains a header with author username'
 
@@ -1066,7 +1066,7 @@ describe Notify do
     subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare, diff_refs: diff_refs) }
 
     it_behaves_like 'it should show Gmail Actions View Commit link'
-    it_behaves_like "a user cannot unsubscribe through footer link"
+    it_behaves_like 'a user cannot unsubscribe through footer link'
     it_behaves_like 'an email with X-GitLab headers containing project details'
     it_behaves_like 'an email that contains a header with author username'
 
diff --git a/spec/mailers/shared/notify.rb b/spec/mailers/shared/notify.rb
index 93de5850ba2..56872da9a8f 100644
--- a/spec/mailers/shared/notify.rb
+++ b/spec/mailers/shared/notify.rb
@@ -169,10 +169,18 @@ shared_examples 'it should show Gmail Actions View Commit link' do
 end
 
 shared_examples 'an unsubscribeable thread' do
+  it 'has a List-Unsubscribe header' do
+    is_expected.to have_header 'List-Unsubscribe', /unsubscribe/
+  end
+
   it { is_expected.to have_body_text /unsubscribe/ }
 end
 
 shared_examples 'a user cannot unsubscribe through footer link' do
+  it 'does not have a List-Unsubscribe header' do
+    is_expected.not_to have_header 'List-Unsubscribe', /unsubscribe/
+  end
+
   it { is_expected.not_to have_body_text /unsubscribe/ }
 end