diff options
author | Maximiliano Perez Coto <maxi@assembla.com> | 2016-07-14 02:56:54 +0300 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2016-09-20 10:52:57 +0300 |
commit | b335730817e096bb4c68e5e4a4a2a3ec29b25243 (patch) | |
tree | 58e66bb4c97f139d68def7183fb7089166c5670f /spec | |
parent | 95b9421ad3b2678da6e0af0131eafd52cdd0b2a5 (diff) |
Fix "Unsubscribe" link in notification emails that is triggered by anti-virus
* Created a force=true param that will continue with the previous
behaviour of the unsubscribe method
* Created a filter for not-logged users so they see a unsubsribe
confirmation page
* Added the List-Unsubscribe header on emails so the email client can
display it on top
Diffstat (limited to 'spec')
-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 |
4 files changed, 157 insertions, 18 deletions
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 |