diff options
| -rw-r--r-- | CHANGELOG | 1 | ||||
| -rw-r--r-- | app/controllers/sent_notifications_controller.rb | 4 | ||||
| -rw-r--r-- | app/mailers/notify.rb | 6 | ||||
| -rw-r--r-- | app/views/layouts/notify.html.haml | 4 | ||||
| -rw-r--r-- | app/views/sent_notifications/unsubscribe.html.haml | 4 | ||||
| -rw-r--r-- | spec/controllers/sent_notifications_controller_spec.rb | 103 | ||||
| -rw-r--r-- | spec/features/unsubscribe_links_spec.rb | 54 | ||||
| -rw-r--r-- | spec/mailers/notify_spec.rb | 10 | ||||
| -rw-r--r-- | spec/mailers/shared/notify.rb | 8 |
9 files changed, 174 insertions, 20 deletions
diff --git a/CHANGELOG b/CHANGELOG index 7a19af6b765..1502f03b951 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -69,6 +69,7 @@ v 8.12.0 (unreleased) - Test migration paths from 8.5 until current release !4874 - Replace animateEmoji timeout with eventListener (ClemMakesApps) - Optimistic locking for Issues and Merge Requests (title and description overriding prevention) + - Require confirmation when not logged in for unsubscribe links !6223 (Maximiliano Perez Coto) - Add `wiki_page_events` to project hook APIs (Ben Boeckel) - Remove Gitorious import - Fix inconsistent background color for filter input field (ClemMakesApps) diff --git a/app/controllers/sent_notifications_controller.rb b/app/controllers/sent_notifications_controller.rb index 7271c933b9b..c4abc597cf1 100644 --- a/app/controllers/sent_notifications_controller.rb +++ b/app/controllers/sent_notifications_controller.rb @@ -2,13 +2,17 @@ class SentNotificationsController < ApplicationController skip_before_action :authenticate_user! def unsubscribe + return redirect_to new_user_session_path unless current_user || params[:force] + @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 diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index 0cc709f68e4..9799f1dc886 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -108,6 +108,12 @@ class Notify < BaseMailer headers["X-GitLab-#{model.class.name}-ID"] = model.id headers['X-GitLab-Reply-Key'] = reply_key + if !@labels_url && @sent_notification && @sent_notification.unsubscribable? + headers['List-Unsubscribe'] = unsubscribe_sent_notification_url(@sent_notification, force: true) + + @sent_notification_url = unsubscribe_sent_notification_url(@sent_notification) + end + 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/views/layouts/notify.html.haml b/app/views/layouts/notify.html.haml index dde2e2889dc..1ec4c3f0c67 100644 --- a/app/views/layouts/notify.html.haml +++ b/app/views/layouts/notify.html.haml @@ -25,8 +25,8 @@ - if @labels_url adjust your #{link_to 'label subscriptions', @labels_url}. - else - - if @sent_notification && @sent_notification.unsubscribable? - = link_to "unsubscribe", unsubscribe_sent_notification_url(@sent_notification) + - if @sent_notification_url + = link_to "unsubscribe", @sent_notification_url from this thread or adjust your notification settings. diff --git a/app/views/sent_notifications/unsubscribe.html.haml b/app/views/sent_notifications/unsubscribe.html.haml new file mode 100644 index 00000000000..568d2ac3af1 --- /dev/null +++ b/app/views/sent_notifications/unsubscribe.html.haml @@ -0,0 +1,4 @@ +%h3.page-title + Are you sure you want to unsubscribe from this thread? + + = link_to "Unsubscribe", unsubscribe_sent_notification_path(@sent_notification, force: true), class: 'btn btn-primary wide' diff --git a/spec/controllers/sent_notifications_controller_spec.rb b/spec/controllers/sent_notifications_controller_spec.rb index 9ced397bd4a..4e75372ffb2 100644 --- a/spec/controllers/sent_notifications_controller_spec.rb +++ b/spec/controllers/sent_notifications_controller_spec.rb @@ -1,25 +1,102 @@ 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 redirect_to(new_user_session_path) + 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 + before { get(:unsubscribe, id: sent_notification.reply_key) } + + it 'unsubscribes the user' do + expect(issue.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 issue page' do + expect(response). + to redirect_to(namespace_project_issue_path(project.namespace, project, issue)) + 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..eafad1a726d --- /dev/null +++ b/spec/features/unsubscribe_links_spec.rb @@ -0,0 +1,54 @@ +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 + it 'redirects to the login page when visiting the link from the body' do + visit body_link + + expect(current_path).to eq new_user_session_path + expect(issue.subscribed?(recipient)).to be_truthy + 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 |