diff --git a/CHANGELOG b/CHANGELOG index 921dac01264e9f48bdb58a1b2ebd96b10759bae0..38c0dbf0934cf30cdba94e7b40c301026a98728e 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -59,6 +59,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 diff --git a/app/controllers/sent_notifications_controller.rb b/app/controllers/sent_notifications_controller.rb new file mode 100644 index 0000000000000000000000000000000000000000..93dcc16094f76f9bd6045815a39b4560b49e6b8d --- /dev/null +++ b/app/controllers/sent_notifications_controller.rb @@ -0,0 +1,25 @@ +class SentNotificationsController < ApplicationController + skip_before_action :authenticate_user! + + def unsubscribe + @sent_notification = SentNotification.for(params[:id]) + return render_404 unless @sent_notification && !@sent_notification.for_commit? + + noteable = @sent_notification.noteable + noteable.unsubscribe(@sent_notification.recipient) + + flash[:notice] = "You have been unsubscribed from this thread." + if current_user + case @sent_notification.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 diff --git a/app/mailers/emails/issues.rb b/app/mailers/emails/issues.rb index abdeefed5ef8b05a3d1b529a1e31771adf704fb1..4a88cb61132d26e85dfcb2ef8a3fd3df4c470d79 100644 --- a/app/mailers/emails/issues.rb +++ b/app/mailers/emails/issues.rb @@ -1,31 +1,31 @@ module Emails module Issues def new_issue_email(recipient_id, issue_id) - issue_mail_with_notification(issue_id, recipient_id) do - mail_new_thread(@issue, issue_thread_options(@issue.author_id, recipient_id)) - end + setup_issue_mail(issue_id, recipient_id) + + mail_new_thread(@issue, issue_thread_options(@issue.author_id, recipient_id)) 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 - @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 + 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 def closed_issue_email(recipient_id, issue_id, updated_by_user_id) - issue_mail_with_notification(issue_id, recipient_id) do - @updated_by = User.find updated_by_user_id - mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id)) - end + 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 def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_id) - issue_mail_with_notification(issue_id, recipient_id) do - @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 + 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 private @@ -38,14 +38,12 @@ def issue_thread_options(sender_id, recipient_id) } 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 diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb index 7923fb770d0427885b69524fe75cdab46f281703..325996e2e163fa81903c64b557fa9e345dd95514 100644 --- a/app/mailers/emails/merge_requests.rb +++ b/app/mailers/emails/merge_requests.rb @@ -1,77 +1,64 @@ 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 diff --git a/app/mailers/emails/notes.rb b/app/mailers/emails/notes.rb index e1382d2da12e158a886ee81b38590f2202e8566a..ccd33b880f7fb11189aaa2f94352fa44f72366a9 100644 --- a/app/mailers/emails/notes.rb +++ b/app/mailers/emails/notes.rb @@ -1,31 +1,31 @@ module Emails module Notes def note_commit_email(recipient_id, note_id) - note_mail_with_notification(note_id, recipient_id) do - @commit = @note.noteable - @target_url = namespace_project_commit_url(*note_target_url_options) - - mail_answer_thread(@commit, - from: sender(@note.author_id), - to: recipient(recipient_id), - subject: subject("#{@commit.title} (#{@commit.short_id})")) - end + note_mail_with_notification(note_id, recipient_id) + + @commit = @note.noteable + @target_url = namespace_project_commit_url(*note_target_url_options) + + mail_answer_thread(@commit, + from: sender(@note.author_id), + to: recipient(recipient_id), + subject: subject("#{@commit.title} (#{@commit.short_id})")) end def note_issue_email(recipient_id, note_id) - note_mail_with_notification(note_id, recipient_id) do - @issue = @note.noteable - @target_url = namespace_project_issue_url(*note_target_url_options) - mail_answer_thread(@issue, note_thread_options(recipient_id)) - end + note_mail_with_notification(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 def note_merge_request_email(recipient_id, note_id) - note_mail_with_notification(note_id, recipient_id) do - @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 + note_mail_with_notification(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 private @@ -46,9 +46,7 @@ def note_mail_with_notification(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 diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index 3bbdd9cee76c2761718bc65259cbebfd5f8c65bf..e1cd075a9787d051187740045fdcd3c9d7c4a876 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -107,10 +107,9 @@ def mail_thread(model, headers = {}) end headers["X-GitLab-#{model.class.name}-ID"] = model.id + headers['X-GitLab-Reply-Key'] = reply_key - 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 diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 18a00f95b48291abe09e17d7f9cbe2869988cb19..04650a9e67a14d0374098b0bf7653c8e953f4ac2 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -119,6 +119,12 @@ def toggle_subscription(user) 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, diff --git a/app/models/sent_notification.rb b/app/models/sent_notification.rb index f36eda1531b803cabd4026f54d003d2d3f2710f7..fd12615717b6f06f1b8bfe93c790de8413a34001 100644 --- a/app/models/sent_notification.rb +++ b/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 @@ -59,7 +57,7 @@ def record(noteable, recipient_id, reply_key, params = {}) def record_note(note, recipient_id, reply_key, params = {}) params[:line_code] = note.line_code - + record(note.noteable, recipient_id, reply_key, params) end end @@ -75,4 +73,8 @@ def noteable super end end + + def to_param + self.reply_key + end end diff --git a/app/views/layouts/notify.html.haml b/app/views/layouts/notify.html.haml index 3ca4c340406511d7488e76997834f7dd75e0aa64..da7de41abadadd92d85c0a0759b5ad29cc4c766d 100644 --- a/app/views/layouts/notify.html.haml +++ b/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.for_commit? + = link_to "unsubscribe", unsubscribe_sent_notification_url(@sent_notification) + from this thread or + adjust your notification settings. - = email_action @target_url \ No newline at end of file + = email_action @target_url diff --git a/config/routes.rb b/config/routes.rb index 4fcea1185c022494cbb1b14da2b51dee0fc42f10..2739e4961f7c5ffa682b4505a140925efdd1e90f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -88,6 +88,12 @@ end end + resources :sent_notifications, only: [], constraints: { id: /[0-9a-f]{32}/ } do + member do + get :unsubscribe + end + end + # Spam reports resources :abuse_reports, only: [:new, :create] diff --git a/spec/controllers/sent_notification_controller_spec.rb b/spec/controllers/sent_notification_controller_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..9ced397bd4a05864b5dbef191e862341feae7ddc --- /dev/null +++ b/spec/controllers/sent_notification_controller_spec.rb @@ -0,0 +1,26 @@ +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 diff --git a/spec/factories.rb b/spec/factories.rb index d6b4efa9a03d78b858ccf43b2e2d0cc54e57c9c0..2a81684dfcf0a6f0f7c6ba6369e9e747b8602f4f 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -212,4 +212,11 @@ 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 diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 154901a2fbcd44b91332fee867a2ff68fa05e0a8..08ade644643a8e2f819546ebf498392763235f3b 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -104,6 +104,14 @@ 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 can not 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 @@ 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 can not 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 @@ 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 @@ 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 can not 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 @@ it_behaves_like 'an email sent from GitLab' it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like 'a user can not unsubscribe through footer link' it 'is sent to the new user' do is_expected.to deliver_to key.user.email @@ -181,6 +191,7 @@ subject { Notify.new_email_email(email.id) } it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like 'a user can not unsubscribe through footer link' it 'is sent to the new user' do is_expected.to deliver_to email.user.email @@ -227,6 +238,7 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ it_behaves_like 'an email sent from GitLab' it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like "a user can not unsubscribe through footer link" it 'has the correct subject' do is_expected.to have_subject /Project was moved/ @@ -468,6 +488,7 @@ it_behaves_like 'an email sent from GitLab' it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like "a user can not unsubscribe through footer link" it 'has the correct subject' do is_expected.to have_subject /Access to project was granted/ @@ -518,6 +539,7 @@ 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 can not unsubscribe through footer link" it 'has the correct subject' do is_expected.to have_subject /#{commit.title} \(#{commit.short_id}\)/ @@ -538,6 +560,7 @@ 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 @@ 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 @@ it_behaves_like 'an email sent from GitLab' it_behaves_like 'it should not have Gmail Actions links' + it_behaves_like "a user can not unsubscribe through footer link" it 'has the correct subject' do is_expected.to have_subject /Access to group was granted/ @@ -607,6 +632,7 @@ subject { ActionMailer::Base.deliveries.last } it_behaves_like 'an email sent from GitLab' + it_behaves_like "a user can not 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 @@ 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 can not unsubscribe through footer link" it 'is sent as the author' do sender = subject.header[:from].addrs[0] @@ -657,6 +684,7 @@ 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 can not unsubscribe through footer link" it 'is sent as the author' do sender = subject.header[:from].addrs[0] @@ -684,6 +712,7 @@ 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 can not unsubscribe through footer link" it 'is sent as the author' do sender = subject.header[:from].addrs[0] @@ -707,6 +736,7 @@ 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 can not unsubscribe through footer link" it 'is sent as the author' do sender = subject.header[:from].addrs[0] @@ -734,6 +764,7 @@ 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 can not unsubscribe through footer link" it 'is sent as the author' do sender = subject.header[:from].addrs[0] @@ -839,6 +870,7 @@ 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 can not unsubscribe through footer link" it 'is sent as the author' do sender = subject.header[:from].addrs[0]