Merge branch 'unsubscribe-from-thread-from-email-footer' into 'master'

Unsubscribe from thread through link in email footer

Closes #3437 

![Screenshot_from_2015-12-11_15-35-06](/uploads/cfb3d8737d4757f527995411f103d0ce/Screenshot_from_2015-12-11_15-35-06.png)

![Screenshot_from_2015-12-11_15-35-56](/uploads/9b7121be7ce4b05e5995ca6d38c5bea4/Screenshot_from_2015-12-11_15-35-56.png)

See merge request !2068

CHANGELOG

@@ -87,6 +87,7 @@ v 8.3.0
   - Add open_issues_count to project API (Stan Hu)
   - Expand character set of usernames created by Omniauth (Corey Hinshaw)
   - Add button to automatically merge a merge request when the build succeeds (Zeger-Jan van de Weg)
+  - Add unsubscribe link in the email footer (Zeger-Jan van de Weg)
   - Provide better diagnostic message upon project creation errors (Stan Hu)
   - Bump devise to 3.5.3 to fix reset token expiring after account creation (Stan Hu)
   - Remove api credentials from link to build_page

app/controllers/sent_notifications_controller.rb

+class SentNotificationsController < ApplicationController
+  skip_before_action :authenticate_user!
+
+  def unsubscribe
+    @sent_notification = SentNotification.for(params[:id])
+    return render_404 unless @sent_notification && @sent_notification.unsubscribable?
+
+    noteable = @sent_notification.noteable
+    noteable.unsubscribe(@sent_notification.recipient)
+
+    flash[:notice] = "You have been unsubscribed from this thread."
+    if current_user
+      case noteable
+      when Issue
+        redirect_to issue_path(noteable)
+      when MergeRequest
+        redirect_to merge_request_path(noteable)
+      else
+        redirect_to root_path
+      end
+    else
+      redirect_to new_user_session_path
+    end
+  end
+end

app/mailers/emails/issues.rb

 module Emails
   module Issues
     def new_issue_email(recipient_id, issue_id)
-      issue_mail_with_notification(issue_id, recipient_id) do
+      setup_issue_mail(issue_id, recipient_id)
+
       mail_new_thread(@issue, issue_thread_options(@issue.author_id, recipient_id))
     end
-    end
 
     def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id, updated_by_user_id)
-      issue_mail_with_notification(issue_id, recipient_id) do
+      setup_issue_mail(issue_id, recipient_id)
+
       @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id
       mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id))
     end
-    end
 
     def closed_issue_email(recipient_id, issue_id, updated_by_user_id)
-      issue_mail_with_notification(issue_id, recipient_id) do
+      setup_issue_mail(issue_id, recipient_id)
+
       @updated_by = User.find updated_by_user_id
       mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id))
     end
-    end
 
     def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_id)
-      issue_mail_with_notification(issue_id, recipient_id) do
+      setup_issue_mail(issue_id, recipient_id)
+
       @issue_status = status
       @updated_by = User.find updated_by_user_id
       mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id))
     end
-    end
 
     private
 
@@ -38,14 +38,12 @@ module Emails
       }
     end
 
-    def issue_mail_with_notification(issue_id, recipient_id)
+    def setup_issue_mail(issue_id, recipient_id)
       @issue = Issue.find(issue_id)
       @project = @issue.project
       @target_url = namespace_project_issue_url(@project.namespace, @project, @issue)
 
-      yield
-
-      SentNotification.record(@issue, recipient_id, reply_key)
+      @sent_notification = SentNotification.record(@issue, recipient_id, reply_key)
     end
   end
 end

app/mailers/emails/merge_requests.rb

 module Emails
   module MergeRequests
     def new_merge_request_email(recipient_id, merge_request_id)
-      @merge_request = MergeRequest.find(merge_request_id)
-      @project = @merge_request.project
-      @target_url = namespace_project_merge_request_url(@project.namespace,
-                                                        @project,
-                                                        @merge_request)
+      setup_merge_request_mail(merge_request_id, recipient_id)
+
       mail_new_thread(@merge_request,
                       from: sender(@merge_request.author_id),
                       to: recipient(recipient_id),
                       subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
-
-      SentNotification.record(@merge_request, recipient_id, reply_key)
     end
 
     def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id, updated_by_user_id)
-      @merge_request = MergeRequest.find(merge_request_id)
+      setup_merge_request_mail(merge_request_id, recipient_id)
+
       @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id
-      @project = @merge_request.project
-      @target_url = namespace_project_merge_request_url(@project.namespace,
-                                                        @project,
-                                                        @merge_request)
       mail_answer_thread(@merge_request,
                          from: sender(updated_by_user_id),
                          to: recipient(recipient_id),
                          subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
-
-      SentNotification.record(@merge_request, recipient_id, reply_key)
     end
 
     def closed_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
-      @merge_request = MergeRequest.find(merge_request_id)
+      setup_merge_request_mail(merge_request_id, recipient_id)
+
       @updated_by = User.find updated_by_user_id
-      @project = @merge_request.project
-      @target_url = namespace_project_merge_request_url(@project.namespace,
-                                                        @project,
-                                                        @merge_request)
       mail_answer_thread(@merge_request,
                          from: sender(updated_by_user_id),
                          to: recipient(recipient_id),
                          subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
-
-      SentNotification.record(@merge_request, recipient_id, reply_key)
     end
 
     def merged_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
-      @merge_request = MergeRequest.find(merge_request_id)
-      @project = @merge_request.project
-      @target_url = namespace_project_merge_request_url(@project.namespace,
-                                                        @project,
-                                                        @merge_request)
+      setup_merge_request_mail(merge_request_id, recipient_id)
+
       mail_answer_thread(@merge_request,
                          from: sender(updated_by_user_id),
                          to: recipient(recipient_id),
                          subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
-
-      SentNotification.record(@merge_request, recipient_id, reply_key)
     end
 
     def merge_request_status_email(recipient_id, merge_request_id, status, updated_by_user_id)
-      @merge_request = MergeRequest.find(merge_request_id)
+      setup_merge_request_mail(merge_request_id, recipient_id)
+
       @mr_status = status
-      @project = @merge_request.project
       @updated_by = User.find updated_by_user_id
-      @target_url = namespace_project_merge_request_url(@project.namespace,
-                                                        @project,
-                                                        @merge_request)
       mail_answer_thread(@merge_request,
                          from: sender(updated_by_user_id),
                          to: recipient(recipient_id),
                          subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
+    end
+
+    private
+
+    def setup_merge_request_mail(merge_request_id, recipient_id)
+      @merge_request = MergeRequest.find(merge_request_id)
+      @project = @merge_request.project
+      @target_url = namespace_project_merge_request_url(@project.namespace,
+                                                        @project,
+                                                        @merge_request)
 
-      SentNotification.record(@merge_request, recipient_id, reply_key)
+      @sent_notification = SentNotification.record(@merge_request, recipient_id, reply_key)
     end
   end
 end

app/mailers/emails/notes.rb

 module Emails
   module Notes
     def note_commit_email(recipient_id, note_id)
-      note_mail_with_notification(note_id, recipient_id) do
+      setup_note_mail(note_id, recipient_id)
+
       @commit = @note.noteable
       @target_url = namespace_project_commit_url(*note_target_url_options)
 
@@ -10,23 +11,22 @@ module Emails
                          to: recipient(recipient_id),
                          subject: subject("#{@commit.title} (#{@commit.short_id})"))
     end
-    end
 
     def note_issue_email(recipient_id, note_id)
-      note_mail_with_notification(note_id, recipient_id) do
+      setup_note_mail(note_id, recipient_id)
+
       @issue = @note.noteable
       @target_url = namespace_project_issue_url(*note_target_url_options)
       mail_answer_thread(@issue, note_thread_options(recipient_id))
     end
-    end
 
     def note_merge_request_email(recipient_id, note_id)
-      note_mail_with_notification(note_id, recipient_id) do
+      setup_note_mail(note_id, recipient_id)
+
       @merge_request = @note.noteable
       @target_url = namespace_project_merge_request_url(*note_target_url_options)
       mail_answer_thread(@merge_request, note_thread_options(recipient_id))
     end
-    end
 
     private
 
@@ -42,13 +42,11 @@ module Emails
       }
     end
 
-    def note_mail_with_notification(note_id, recipient_id)
+    def setup_note_mail(note_id, recipient_id)
       @note = Note.find(note_id)
       @project = @note.project
 
-      yield
-
-      SentNotification.record_note(@note, recipient_id, reply_key)
+      @sent_notification = SentNotification.record_note(@note, recipient_id, reply_key)
     end
   end
 end

app/mailers/notify.rb

@@ -107,10 +107,9 @@ class Notify < BaseMailer
     end
 
     headers["X-GitLab-#{model.class.name}-ID"] = model.id
-
-    if reply_key
     headers['X-GitLab-Reply-Key'] = reply_key
 
+    if Gitlab::IncomingEmail.enabled?
       address = Mail::Address.new(Gitlab::IncomingEmail.reply_address(reply_key))
       address.display_name = @project.name_with_namespace
 

app/models/concerns/issuable.rb

@@ -119,6 +119,12 @@ module Issuable
       update(subscribed: !subscribed?(user))
   end
 
+  def unsubscribe(user)
+    subscriptions.
+      find_or_initialize_by(user_id: user.id).
+      update(subscribed: false)
+  end
+
   def to_hook_data(user)
     {
       object_kind: self.class.name.underscore,

app/models/sent_notification.rb

@@ -25,8 +25,6 @@ class SentNotification < ActiveRecord::Base
 
   class << self
     def reply_key
-      return nil unless Gitlab::IncomingEmail.enabled?
-
       SecureRandom.hex(16)
     end
 
@@ -64,6 +62,10 @@ class SentNotification < ActiveRecord::Base
     end
   end
 
+  def unsubscribable?
+    !for_commit?
+  end
+
   def for_commit?
     noteable_type == "Commit"
   end
@@ -75,4 +77,8 @@ class SentNotification < ActiveRecord::Base
       super
     end
   end
+
+  def to_param
+    self.reply_key
+  end
 end

app/views/layouts/notify.html.haml

@@ -44,6 +44,10 @@
           %br
           -# Don't link the host is the line below, one link in the email is easier to quickly click than two.
           You're receiving this email because of your account on #{Gitlab.config.gitlab.host}.
-          If you'd like to receive fewer emails, you can adjust your notification settings.
+          If you'd like to receive fewer emails, you can
+          - if @sent_notification && @sent_notification.unsubscribable?
+            = link_to "unsubscribe", unsubscribe_sent_notification_url(@sent_notification)
+            from this thread or
+          adjust your notification settings.
 
           = email_action @target_url

config/routes.rb

@@ -88,6 +88,12 @@ Rails.application.routes.draw do
     end
   end
 
+  resources :sent_notifications, only: [], constraints: { id: /\h{32}/ } do
+    member do
+      get :unsubscribe
+    end
+  end
+
   # Spam reports
   resources :abuse_reports, only: [:new, :create]
 

spec/controllers/sent_notification_controller_spec.rb

+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) }
+
+  describe 'GET #unsubscribe' do
+    it 'returns a 404 when calling without existing id' do
+      get(:unsubscribe, id: '0' * 32)
+
+      expect(response.status).to be 404
+    end
+
+    context 'calling with id' do
+      it 'shows a flash message to the user' do
+        get(:unsubscribe, id: sent_notification.reply_key)
+
+        expect(response.status).to be 302
+
+        expect(response).to redirect_to new_user_session_path
+        expect(controller).to set_flash[:notice].to(/unsubscribed/).now
+      end
+    end
+  end
+end

spec/factories.rb

@@ -212,4 +212,11 @@ FactoryGirl.define do
     provider 'ldapmain'
     extern_uid 'my-ldap-id'
   end
+
+  factory :sent_notification do
+    project
+    recipient factory: :user
+    noteable factory: :issue
+    reply_key "0123456789abcdef" * 2
+  end
 end

spec/mailers/notify_spec.rb

@@ -104,6 +104,14 @@ describe Notify do
     it { is_expected.to have_body_text /View Commit/ }
   end
 
+  shared_examples 'an unsubscribeable thread' do
+    it { is_expected.to have_body_text /unsubscribe/ }
+  end
+
+  shared_examples "a user cannot unsubscribe through footer link" do
+    it { is_expected.not_to have_body_text /unsubscribe/ }
+  end
+
   describe 'for new users, the email' do
     let(:example_site_path) { root_path }
     let(:new_user) { create(:user, email: new_user_address, created_by_id: 1) }
@@ -115,6 +123,7 @@ describe Notify do
     it_behaves_like 'an email sent from GitLab'
     it_behaves_like 'a new user email', new_user_address
     it_behaves_like 'it should not have Gmail Actions links'
+    it_behaves_like 'a user cannot unsubscribe through footer link'
 
     it 'contains the password text' do
       is_expected.to have_body_text /Click here to set your password/
@@ -134,7 +143,6 @@ describe Notify do
     end
   end
 
-
   describe 'for users that signed up, the email' do
     let(:example_site_path) { root_path }
     let(:new_user) { create(:user, email: new_user_address, password: "securePassword") }
@@ -144,6 +152,7 @@ describe Notify do
     it_behaves_like 'an email sent from GitLab'
     it_behaves_like 'a new user email', new_user_address
     it_behaves_like 'it should not have Gmail Actions links'
+    it_behaves_like 'a user cannot unsubscribe through footer link'
 
     it 'should not contain the new user\'s password' do
       is_expected.not_to have_body_text /password/
@@ -157,6 +166,7 @@ describe Notify do
 
     it_behaves_like 'an email sent from GitLab'
     it_behaves_like 'it should not have Gmail Actions links'
+    it_behaves_like 'a user cannot unsubscribe through footer link'
 
     it 'is sent to the new user' do
       is_expected.to deliver_to key.user.email
@@ -181,6 +191,7 @@ describe Notify do
     subject { Notify.new_email_email(email.id) }
 
     it_behaves_like 'it should not have Gmail Actions links'
+    it_behaves_like 'a user cannot unsubscribe through footer link'
 
     it 'is sent to the new user' do
       is_expected.to deliver_to email.user.email
@@ -227,6 +238,7 @@ describe Notify do
           it_behaves_like 'an assignee email'
           it_behaves_like 'an email starting a new thread', 'issue'
           it_behaves_like 'it should show Gmail Actions View Issue link'
+          it_behaves_like 'an unsubscribeable thread'
 
           it 'has the correct subject' do
             is_expected.to have_subject /#{project.name} \| #{issue.title} \(##{issue.iid}\)/
@@ -253,6 +265,7 @@ describe Notify do
           it_behaves_like 'a multiple recipients email'
           it_behaves_like 'an answer to an existing thread', 'issue'
           it_behaves_like 'it should show Gmail Actions View Issue link'
+          it_behaves_like "an unsubscribeable thread"
 
           it 'is sent as the author' do
             sender = subject.header[:from].addrs[0]
@@ -283,6 +296,7 @@ describe Notify do
 
           it_behaves_like 'an answer to an existing thread', 'issue'
           it_behaves_like 'it should show Gmail Actions View Issue link'
+          it_behaves_like 'an unsubscribeable thread'
 
           it 'is sent as the author' do
             sender = subject.header[:from].addrs[0]
@@ -319,6 +333,7 @@ describe Notify do
           it_behaves_like 'an assignee email'
           it_behaves_like 'an email starting a new thread', 'merge_request'
           it_behaves_like 'it should show Gmail Actions View Merge request link'
+          it_behaves_like "an unsubscribeable thread"
 
           it 'has the correct subject' do
             is_expected.to have_subject /#{merge_request.title} \(##{merge_request.iid}\)/
@@ -345,6 +360,7 @@ describe Notify do
           subject { Notify.new_merge_request_email(merge_request_with_description.assignee_id, merge_request_with_description.id) }
 
           it_behaves_like 'it should show Gmail Actions View Merge request link'
+          it_behaves_like "an unsubscribeable thread"
 
           it 'contains the description' do
             is_expected.to have_body_text /#{merge_request_with_description.description}/
@@ -357,6 +373,7 @@ describe Notify do
           it_behaves_like 'a multiple recipients email'
           it_behaves_like 'an answer to an existing thread', 'merge_request'
           it_behaves_like 'it should show Gmail Actions View Merge request link'
+          it_behaves_like "an unsubscribeable thread"
 
           it 'is sent as the author' do
             sender = subject.header[:from].addrs[0]
@@ -387,6 +404,7 @@ describe Notify do
 
           it_behaves_like 'an answer to an existing thread', 'merge_request'
           it_behaves_like 'it should show Gmail Actions View Merge request link'
+          it_behaves_like "an unsubscribeable thread"
 
           it 'is sent as the author' do
             sender = subject.header[:from].addrs[0]
@@ -417,6 +435,7 @@ describe Notify do
           it_behaves_like 'a multiple recipients email'
           it_behaves_like 'an answer to an existing thread', 'merge_request'
           it_behaves_like 'it should show Gmail Actions View Merge request link'
+          it_behaves_like "an unsubscribeable thread"
 
           it 'is sent as the merge author' do
             sender = subject.header[:from].addrs[0]
@@ -446,6 +465,7 @@ describe Notify do
 
       it_behaves_like 'an email sent from GitLab'
       it_behaves_like 'it should not have Gmail Actions links'
+      it_behaves_like "a user cannot unsubscribe through footer link"
 
       it 'has the correct subject' do
         is_expected.to have_subject /Project was moved/
@@ -468,6 +488,7 @@ describe Notify do
 
       it_behaves_like 'an email sent from GitLab'
       it_behaves_like 'it should not have Gmail Actions links'
+      it_behaves_like "a user cannot unsubscribe through footer link"
 
       it 'has the correct subject' do
         is_expected.to have_subject /Access to project was granted/
@@ -518,6 +539,7 @@ describe Notify do
         it_behaves_like 'a note email'
         it_behaves_like 'an answer to an existing thread', 'commit'
         it_behaves_like 'it should show Gmail Actions View Commit link'
+        it_behaves_like "a user cannot unsubscribe through footer link"
 
         it 'has the correct subject' do
           is_expected.to have_subject /#{commit.title} \(#{commit.short_id}\)/
@@ -538,6 +560,7 @@ describe Notify do
         it_behaves_like 'a note email'
         it_behaves_like 'an answer to an existing thread', 'merge_request'
         it_behaves_like 'it should show Gmail Actions View Merge request link'
+        it_behaves_like 'an unsubscribeable thread'
 
         it 'has the correct subject' do
           is_expected.to have_subject /#{merge_request.title} \(##{merge_request.iid}\)/
@@ -558,6 +581,7 @@ describe Notify do
         it_behaves_like 'a note email'
         it_behaves_like 'an answer to an existing thread', 'issue'
         it_behaves_like 'it should show Gmail Actions View Issue link'
+        it_behaves_like 'an unsubscribeable thread'
 
         it 'has the correct subject' do
           is_expected.to have_subject /#{issue.title} \(##{issue.iid}\)/
@@ -579,6 +603,7 @@ describe Notify do
 
     it_behaves_like 'an email sent from GitLab'
     it_behaves_like 'it should not have Gmail Actions links'
+    it_behaves_like "a user cannot unsubscribe through footer link"
 
     it 'has the correct subject' do
       is_expected.to have_subject /Access to group was granted/
@@ -607,6 +632,7 @@ describe Notify do
     subject { ActionMailer::Base.deliveries.last }
 
     it_behaves_like 'an email sent from GitLab'
+    it_behaves_like "a user cannot unsubscribe through footer link"
 
     it 'is sent to the new user' do
       is_expected.to deliver_to 'new-email@mail.com'
@@ -629,6 +655,7 @@ describe Notify do
     subject { Notify.repository_push_email(project.id, 'devs@company.name', 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 'is sent as the author' do
       sender = subject.header[:from].addrs[0]
@@ -657,6 +684,7 @@ describe Notify do
     subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/tags/v1.0', action: :create) }
 
     it_behaves_like 'it should not have Gmail Actions links'
+    it_behaves_like "a user cannot unsubscribe through footer link"
 
     it 'is sent as the author' do
       sender = subject.header[:from].addrs[0]
@@ -684,6 +712,7 @@ describe Notify do
     subject { Notify.repository_push_email(project.id, 'devs@company.name', 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 'is sent as the author' do
       sender = subject.header[:from].addrs[0]
@@ -707,6 +736,7 @@ describe Notify do
     subject { Notify.repository_push_email(project.id, 'devs@company.name', 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 'is sent as the author' do
       sender = subject.header[:from].addrs[0]
@@ -734,6 +764,7 @@ describe Notify do
     subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare, reverse_compare: false, 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 'is sent as the author' do
       sender = subject.header[:from].addrs[0]
@@ -839,6 +870,7 @@ describe Notify do
     subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare) }
 
     it_behaves_like 'it should show Gmail Actions View Commit link'
+    it_behaves_like "a user cannot unsubscribe through footer link"
 
     it 'is sent as the author' do
       sender = subject.header[:from].addrs[0]