From 0e60282e36faab8b0f4faee0b71716987df28416 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Mon, 4 Jan 2016 18:46:43 -0500 Subject: [PATCH 1/3] Redirect back to user profile page after abuse report Now the reporter will see the fruits of their labor, namely, the red icon! --- app/controllers/abuse_reports_controller.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app/controllers/abuse_reports_controller.rb b/app/controllers/abuse_reports_controller.rb index 20bc5173f1d..5718fd22de9 100644 --- a/app/controllers/abuse_reports_controller.rb +++ b/app/controllers/abuse_reports_controller.rb @@ -14,7 +14,7 @@ class AbuseReportsController < ApplicationController end message = "Thank you for your report. A GitLab administrator will look into it shortly." - redirect_to root_path, notice: message + redirect_to @abuse_report.user, notice: message else render :new end @@ -23,6 +23,9 @@ class AbuseReportsController < ApplicationController private def report_params - params.require(:abuse_report).permit(:user_id, :message) + params.require(:abuse_report).permit(%i( + message + user_id + )) end end From 01248d205103fe6c408e914e8943873ceb7acb2a Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Mon, 4 Jan 2016 18:56:48 -0500 Subject: [PATCH 2/3] Make AbuseReportMailer responsible for knowing if it should deliver --- app/mailers/abuse_report_mailer.rb | 10 ++++++- spec/mailers/abuse_report_mailer_spec.rb | 38 ++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) create mode 100644 spec/mailers/abuse_report_mailer_spec.rb diff --git a/app/mailers/abuse_report_mailer.rb b/app/mailers/abuse_report_mailer.rb index f0c41f69a5c..d0ce827a595 100644 --- a/app/mailers/abuse_report_mailer.rb +++ b/app/mailers/abuse_report_mailer.rb @@ -2,11 +2,19 @@ class AbuseReportMailer < BaseMailer include Gitlab::CurrentSettings def notify(abuse_report_id) + return unless deliverable? + @abuse_report = AbuseReport.find(abuse_report_id) mail( - to: current_application_settings.admin_notification_email, + to: current_application_settings.admin_notification_email, subject: "#{@abuse_report.user.name} (#{@abuse_report.user.username}) was reported for abuse" ) end + + private + + def deliverable? + current_application_settings.admin_notification_email.present? + end end diff --git a/spec/mailers/abuse_report_mailer_spec.rb b/spec/mailers/abuse_report_mailer_spec.rb new file mode 100644 index 00000000000..eb433c38873 --- /dev/null +++ b/spec/mailers/abuse_report_mailer_spec.rb @@ -0,0 +1,38 @@ +require 'rails_helper' + +describe AbuseReportMailer do + include EmailSpec::Matchers + + describe '.notify' do + context 'with admin_notification_email set' do + before do + stub_application_setting(admin_notification_email: 'admin@example.com') + end + + it 'sends to the admin_notification_email' do + report = create(:abuse_report) + + mail = described_class.notify(report.id) + + expect(mail).to deliver_to 'admin@example.com' + end + + it 'includes the user in the subject' do + report = create(:abuse_report) + + mail = described_class.notify(report.id) + + expect(mail).to have_subject "#{report.user.name} (#{report.user.username}) was reported for abuse" + end + end + + context 'with no admin_notification_email set' do + it 'returns early' do + stub_application_setting(admin_notification_email: nil) + + expect { described_class.notify(spy).deliver_now }. + not_to change { ActionMailer::Base.deliveries.count } + end + end + end +end From 46a220ae3c0e646aac63a3230399fcc8979df6ec Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Mon, 4 Jan 2016 18:59:42 -0500 Subject: [PATCH 3/3] Add `AbuseReport#notify` Tell, Don't Ask. --- app/controllers/abuse_reports_controller.rb | 4 +- app/models/abuse_report.rb | 6 ++ .../abuse_reports_controller_spec.rb | 80 ++++++------------- spec/models/abuse_report_spec.rb | 17 ++++ 4 files changed, 49 insertions(+), 58 deletions(-) diff --git a/app/controllers/abuse_reports_controller.rb b/app/controllers/abuse_reports_controller.rb index 5718fd22de9..38814459f66 100644 --- a/app/controllers/abuse_reports_controller.rb +++ b/app/controllers/abuse_reports_controller.rb @@ -9,9 +9,7 @@ class AbuseReportsController < ApplicationController @abuse_report.reporter = current_user if @abuse_report.save - if current_application_settings.admin_notification_email.present? - AbuseReportMailer.notify(@abuse_report.id).deliver_later - end + @abuse_report.notify message = "Thank you for your report. A GitLab administrator will look into it shortly." redirect_to @abuse_report.user, notice: message diff --git a/app/models/abuse_report.rb b/app/models/abuse_report.rb index 89b3116b9f2..55864236b2f 100644 --- a/app/models/abuse_report.rb +++ b/app/models/abuse_report.rb @@ -18,4 +18,10 @@ class AbuseReport < ActiveRecord::Base validates :user, presence: true validates :message, presence: true validates :user_id, uniqueness: true + + def notify + return unless self.persisted? + + AbuseReportMailer.notify(self.id).deliver_later + end end diff --git a/spec/controllers/abuse_reports_controller_spec.rb b/spec/controllers/abuse_reports_controller_spec.rb index 15824a1c67f..80a418feb3e 100644 --- a/spec/controllers/abuse_reports_controller_spec.rb +++ b/spec/controllers/abuse_reports_controller_spec.rb @@ -1,76 +1,46 @@ require 'spec_helper' describe AbuseReportsController do - let(:reporter) { create(:user) } - let(:user) { create(:user) } - let(:message) { "This user is a spammer" } + let(:reporter) { create(:user) } + let(:user) { create(:user) } + let(:attrs) do + attributes_for(:abuse_report) do |hash| + hash[:user_id] = user.id + end + end before do sign_in(reporter) end - describe "POST create" do - context "with admin notification email set" do - let(:admin_email) { "admin@example.com"} - - before(:each) do - stub_application_setting(admin_notification_email: admin_email) + describe 'POST create' do + context 'with valid attributes' do + it 'saves the abuse report' do + expect do + post :create, abuse_report: attrs + end.to change { AbuseReport.count }.by(1) end - it "sends a notification email" do - perform_enqueued_jobs do - post :create, - abuse_report: { - user_id: user.id, - message: message - } + it 'calls notify' do + expect_any_instance_of(AbuseReport).to receive(:notify) - email = ActionMailer::Base.deliveries.last - - expect(email.to).to eq([admin_email]) - expect(email.subject).to include(user.username) - expect(email.text_part.body).to include(message) - end + post :create, abuse_report: attrs end - it "saves the abuse report" do - perform_enqueued_jobs do - expect do - post :create, - abuse_report: { - user_id: user.id, - message: message - } - end.to change { AbuseReport.count }.by(1) - end + it 'redirects back to the reported user' do + post :create, abuse_report: attrs + + expect(response).to redirect_to user end end - context "without admin notification email set" do - before(:each) do - stub_application_setting(admin_notification_email: nil) - end + context 'with invalid attributes' do + it 'renders new' do + attrs.delete(:user_id) + post :create, abuse_report: attrs - it "does not send a notification email" do - expect do - post :create, - abuse_report: { - user_id: user.id, - message: message - } - end.not_to change { ActionMailer::Base.deliveries.count } - end - - it "saves the abuse report" do - expect do - post :create, - abuse_report: { - user_id: user.id, - message: message - } - end.to change { AbuseReport.count }.by(1) + expect(response).to render_template(:new) end end end - end diff --git a/spec/models/abuse_report_spec.rb b/spec/models/abuse_report_spec.rb index d45319b25d4..46cab1644c7 100644 --- a/spec/models/abuse_report_spec.rb +++ b/spec/models/abuse_report_spec.rb @@ -28,4 +28,21 @@ RSpec.describe AbuseReport, type: :model do it { is_expected.to validate_presence_of(:message) } it { is_expected.to validate_uniqueness_of(:user_id) } end + + describe '#notify' do + it 'delivers' do + expect(AbuseReportMailer).to receive(:notify).with(subject.id). + and_return(spy) + + subject.notify + end + + it 'returns early when not persisted' do + report = build(:abuse_report) + + expect(AbuseReportMailer).not_to receive(:notify) + + report.notify + end + end end