Skip to content
Snippets Groups Projects

Send an email (to support) when a user is reported for spam

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • username-removed-257774 Title changed from WIP: Fix 2839 Send an email (to support) when a user is reported for spam to WIP: Fix #2839 (closed) Send an email (to support) when a user is reported for spam

    Title changed from WIP: Fix 2839 Send an email (to support) when a user is reported for spam to WIP: Fix #2839 (closed) Send an email (to support) when a user is reported for spam

  • Added 1 commit:

    • bf6ab496 - Send an email (to support) when a user is reported for spam
  • Added 1 commit:

    • aeaeb45f - Send an email (to support) when a user is reported for spam
  • Douwe Maan Title changed from WIP: Fix #2839 (closed) Send an email (to support) when a user is reported for spam to WIP: Send an email (to support) when a user is reported for spam

    Title changed from WIP: Fix #2839 (closed) Send an email (to support) when a user is reported for spam to WIP: Send an email (to support) when a user is reported for spam

  • Thanks @jrochkind!

  • Added 1 commit:

    • d09e148a - Send an email (to support) when a user is reported for spam
  • Added 1 commit:

    • ae4fbae2 - Send an email (to support) when a user is reported for spam
  • I don't know why I'm getting a merge conflict reported here -- any tips? I did rebase on master.

    This does not yet do "Also add a webhook, if possible" mentioned in #2839 (closed).

    But it does do the email abuse report notification feature itself.

    I am not sure about the UI, perhaps the admin should be able to opt in or out of receiving these emails? In this MR right now, if admin notification email is set in admin screen, you get the emails.

    But it is done and tested with that basic UI. @DouweM

  • username-removed-257774 Title changed from WIP: Send an email (to support) when a user is reported for spam to Send an email (to support) when a user is reported for spam

    Title changed from WIP: Send an email (to support) when a user is reported for spam to Send an email (to support) when a user is reported for spam

  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
11 11 if @abuse_report.save
12 12 message = "Thank you for your report. A GitLab administrator will look into it shortly."
13 13 redirect_to root_path, notice: message
14 if current_application_settings.admin_notification_email.present?
15 AbuseReportMailer.delay.notify(@abuse_report, current_application_settings.admin_notification_email)
  • Since this is put onto the Sidekiq queue, we shouldn't serialize the whole AbuseReport, but rather only its id, and fetch it again in the worker.

  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 11 11 if @abuse_report.save
    12 12 message = "Thank you for your report. A GitLab administrator will look into it shortly."
    13 13 redirect_to root_path, notice: message
    14 if current_application_settings.admin_notification_email.present?
    • It doesn't really make a difference, but I prefer to have the render or redirect step be the final statement in an action. Please move this before message = ..., with a blank line between them.

  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 1 class AbuseReportMailer < BaseMailer
    2
    3 def notify(abuse_report, to_email)
    4 @abuse_report = abuse_report
    5
    6 mail(to: to_email, subject: "[Gitlab] Abuse report filed for `#{@abuse_report.user.username}`")
      • [Gitlab] isn't needed, the sender already makes that clear.
      • Please remove the backticks around the username
      • Please show both full name and username, like in the admin area: #{@abuse_report.user.name} (#{@abuse_report.user.username})
      • NAME was reported for abuse is a little shorter
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 1 An abuse report was filed on `#{@abuse_report.user.username}` by `#{@abuse_report.reporter.username}`.
    • I think this looks better:

      #{@abuse_report.user.name} (#{@abuse_report.user.username}) was reported for abuse by #{@abuse_report.reporter.name} (#{@abuse_report.repotert.username})
    • Please add an HTML email as well.

  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 1 An abuse report was filed on `#{@abuse_report.user.username}` by `#{@abuse_report.reporter.username}`.
    2 \
    3 = @abuse_report.message
    4 \
    5 Abuse report admin screen: #{abuse_reports_url}
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 47 47 = f.label :version_check_enabled do
    48 48 = f.check_box :version_check_enabled
    49 49 Version check enabled
    50 .form-group
    51 = f.label :admin_notification_email, class: 'control-label col-sm-2'
    52 .col-sm-10
    53 = f.text_field :admin_notification_email, class: 'form-control'
    • Please add a %span.help-block with information on what this is used for, and the fact that it isn't when it's empty.

  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 1 require 'spec_helper'
    2
    3 describe AbuseReportsController do
    4 let(:reporter) { create(:user) }
    5 let(:user) { create(:user) }
    6 let(:message) { "This user is a spammer" }
    7
    8 before do
    9 sign_in(reporter)
    10 end
    11
    12 describe "with admin notification_email set" do
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 1 require 'spec_helper'
    2
    3 describe AbuseReportsController do
    4 let(:reporter) { create(:user) }
    5 let(:user) { create(:user) }
    6 let(:message) { "This user is a spammer" }
    7
    8 before do
    9 sign_in(reporter)
    10 end
    11
    12 describe "with admin notification_email set" do
    13 let(:admin_email) { "admin@example.com"}
    14 before(:example) { allow(current_application_settings).to receive(:admin_notification_email).and_return(admin_email) }
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 18 abuse_report: {
    19 user_id: user.id,
    20 message: message
    21 }
    22 )
    23
    24 expect(response).to have_http_status(:redirect)
    25 expect(flash[:notice]).to start_with("Thank you for your report")
    26
    27 email = ActionMailer::Base.deliveries.last
    28
    29 expect(email).to be_present
    30 expect(email.subject).to eq("[Gitlab] Abuse report filed for `#{user.username}`")
    31 expect(email.to).to eq([admin_email])
    32 expect(email.body).to include(message)
    33 end
    • I know this wasn't part of your MR, but I think we should also test if the AbuseReport was added to the database.

  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 9 sign_in(reporter)
    10 end
    11
    12 describe "with admin notification_email set" do
    13 let(:admin_email) { "admin@example.com"}
    14 before(:example) { allow(current_application_settings).to receive(:admin_notification_email).and_return(admin_email) }
    15
    16 it "sends a notification email" do
    17 post(:create,
    18 abuse_report: {
    19 user_id: user.id,
    20 message: message
    21 }
    22 )
    23
    24 expect(response).to have_http_status(:redirect)
    • If we want to test this, I think we should test where it redirects.

      Also, this isn't part of it "sends a notification email" so it should have a separate it block.

  • Douwe Maan mentioned in merge request !1634 (merged)

    mentioned in merge request !1634 (merged)

  • Replaced by !1634 (merged).

  • Douwe Maan Status changed to closed

    Status changed to closed

  • Robert Speicher mentioned in commit ba41b2ba

    mentioned in commit ba41b2ba

  • Robert Speicher mentioned in commit 955f4248

    mentioned in commit 955f4248

  • Please register or sign in to reply
    Loading