Commit 9b127028 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'rs-abuse-reports-refactor' into 'master'

Abuse Report refactors

- Redirect back to user profile after report
- "Tell, Don't Ask" for sending report notifications

See merge request !2293
parents 2479af4d 46a220ae
...@@ -9,12 +9,10 @@ class AbuseReportsController < ApplicationController ...@@ -9,12 +9,10 @@ class AbuseReportsController < ApplicationController
@abuse_report.reporter = current_user @abuse_report.reporter = current_user
if @abuse_report.save if @abuse_report.save
if current_application_settings.admin_notification_email.present? @abuse_report.notify
AbuseReportMailer.notify(@abuse_report.id).deliver_later
end
message = "Thank you for your report. A GitLab administrator will look into it shortly." 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 else
render :new render :new
end end
...@@ -23,6 +21,9 @@ class AbuseReportsController < ApplicationController ...@@ -23,6 +21,9 @@ class AbuseReportsController < ApplicationController
private private
def report_params def report_params
params.require(:abuse_report).permit(:user_id, :message) params.require(:abuse_report).permit(%i(
message
user_id
))
end end
end end
...@@ -2,6 +2,8 @@ class AbuseReportMailer < BaseMailer ...@@ -2,6 +2,8 @@ class AbuseReportMailer < BaseMailer
include Gitlab::CurrentSettings include Gitlab::CurrentSettings
def notify(abuse_report_id) def notify(abuse_report_id)
return unless deliverable?
@abuse_report = AbuseReport.find(abuse_report_id) @abuse_report = AbuseReport.find(abuse_report_id)
mail( mail(
...@@ -9,4 +11,10 @@ class AbuseReportMailer < BaseMailer ...@@ -9,4 +11,10 @@ class AbuseReportMailer < BaseMailer
subject: "#{@abuse_report.user.name} (#{@abuse_report.user.username}) was reported for abuse" subject: "#{@abuse_report.user.name} (#{@abuse_report.user.username}) was reported for abuse"
) )
end end
private
def deliverable?
current_application_settings.admin_notification_email.present?
end
end end
...@@ -18,4 +18,10 @@ class AbuseReport < ActiveRecord::Base ...@@ -18,4 +18,10 @@ class AbuseReport < ActiveRecord::Base
validates :user, presence: true validates :user, presence: true
validates :message, presence: true validates :message, presence: true
validates :user_id, uniqueness: true validates :user_id, uniqueness: true
def notify
return unless self.persisted?
AbuseReportMailer.notify(self.id).deliver_later
end
end end
...@@ -3,74 +3,44 @@ require 'spec_helper' ...@@ -3,74 +3,44 @@ require 'spec_helper'
describe AbuseReportsController do describe AbuseReportsController do
let(:reporter) { create(:user) } let(:reporter) { create(:user) }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:message) { "This user is a spammer" } let(:attrs) do
attributes_for(:abuse_report) do |hash|
hash[:user_id] = user.id
end
end
before do before do
sign_in(reporter) sign_in(reporter)
end end
describe "POST create" do describe 'POST create' do
context "with admin notification email set" do context 'with valid attributes' do
let(:admin_email) { "admin@example.com"} it 'saves the abuse report' do
expect do
before(:each) do post :create, abuse_report: attrs
stub_application_setting(admin_notification_email: admin_email) end.to change { AbuseReport.count }.by(1)
end end
it "sends a notification email" do it 'calls notify' do
perform_enqueued_jobs do expect_any_instance_of(AbuseReport).to receive(:notify)
post :create,
abuse_report: {
user_id: user.id,
message: message
}
email = ActionMailer::Base.deliveries.last post :create, abuse_report: attrs
expect(email.to).to eq([admin_email])
expect(email.subject).to include(user.username)
expect(email.text_part.body).to include(message)
end
end end
it "saves the abuse report" do it 'redirects back to the reported user' do
perform_enqueued_jobs do post :create, abuse_report: attrs
expect do
post :create,
abuse_report: {
user_id: user.id,
message: message
}
end.to change { AbuseReport.count }.by(1)
end
end
end
context "without admin notification email set" do expect(response).to redirect_to user
before(:each) do
stub_application_setting(admin_notification_email: nil)
end end
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 end
it "saves the abuse report" do context 'with invalid attributes' do
expect do it 'renders new' do
post :create, attrs.delete(:user_id)
abuse_report: { post :create, abuse_report: attrs
user_id: user.id,
message: message expect(response).to render_template(:new)
}
end.to change { AbuseReport.count }.by(1)
end end
end end
end end
end end
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
...@@ -28,4 +28,21 @@ RSpec.describe AbuseReport, type: :model do ...@@ -28,4 +28,21 @@ RSpec.describe AbuseReport, type: :model do
it { is_expected.to validate_presence_of(:message) } it { is_expected.to validate_presence_of(:message) }
it { is_expected.to validate_uniqueness_of(:user_id) } it { is_expected.to validate_uniqueness_of(:user_id) }
end 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 end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment