[openstreetmap/openstreetmap-website] [WIP] Allow reporting of abusive users to moderators or admins (#1576)

Tom Hughes notifications at github.com
Sun Mar 4 15:57:29 UTC 2018


tomhughes requested changes on this pull request.

I think I've gone through all models, code and view now - haven't looked at the tests yet.

> +    comment.user = current_user
+    comment.save!
+    notice = t("issues.comment.comment_created")
+    reassign_issue(@issue) if params[:reassign]
+    redirect_to @issue, :notice => notice
+  end
+
+  private
+
+  def issue_comment_params
+    params.require(:issue_comment).permit(:body)
+  end
+
+  def check_permission
+    unless current_user.administrator? || current_user.moderator?
+      flash[:error] = t("application.require_admin.not_an_admin")

It looks like this only needs a moderator, not an administrator?

> @@ -0,0 +1,128 @@
+class IssuesController < ApplicationController
+  layout "site"
+
+  before_action :authorize_web
+  before_action :require_user
+  before_action :set_issues

What's the point of this? All it does is define a couple of member variables which are then used once in the `index` method to assign to a different member variable...

If they were actually used more than once I'd suggest making them class constants but as it is I suspect just assigning the relevant arrays in `index` is probably most sensible?

> +      @issue.updated_by = current_user.id
+      @issue.save!
+      redirect_to @issue, :notice => t("issues.reopened")
+    else
+      render :show
+    end
+  end
+
+  private
+
+  def set_issues
+    @admin_issues = %w[DiaryEntry DiaryComment User]
+    @moderator_issues = %w[Note]
+  end
+
+  def check_if_updated

This function doesn't seem to be used anywhere?

> +      false
+    end
+  end
+
+  def find_issue
+    @issue = Issue.find(params[:id])
+  end
+
+  def check_permission
+    unless current_user.administrator? || current_user.moderator?
+      flash[:error] = t("application.require_admin.not_an_admin")
+      redirect_to root_path
+    end
+  end
+
+  def issue_params

This function doesn't seem to be used anywhere?

> +  def find_issue
+    @issue = Issue.find(params[:id])
+  end
+
+  def check_permission
+    unless current_user.administrator? || current_user.moderator?
+      flash[:error] = t("application.require_admin.not_an_admin")
+      redirect_to root_path
+    end
+  end
+
+  def issue_params
+    params[:issue].permit(:reportable_id, :reportable_type)
+  end
+
+  def report_params

This function doesn't seem to be used anywhere? I think it got moved to the report controller?

> +  def check_permission
+    unless current_user.administrator? || current_user.moderator?
+      flash[:error] = t("application.require_admin.not_an_admin")
+      redirect_to root_path
+    end
+  end
+
+  def issue_params
+    params[:issue].permit(:reportable_id, :reportable_type)
+  end
+
+  def report_params
+    params[:report].permit(:details)
+  end
+
+  def issue_comment_params

This function doesn't seem to be used anywhere? Moved to the issue comment controller?

> +    end
+  end
+
+  def issue_params
+    params[:issue].permit(:reportable_id, :reportable_type)
+  end
+
+  def report_params
+    params[:report].permit(:details)
+  end
+
+  def issue_comment_params
+    params.require(:issue_comment).permit(:body)
+  end
+
+  def sort_column

This function doesn't seem to be used anywhere.

> +    params[:issue].permit(:reportable_id, :reportable_type)
+  end
+
+  def report_params
+    params[:report].permit(:details)
+  end
+
+  def issue_comment_params
+    params.require(:issue_comment).permit(:body)
+  end
+
+  def sort_column
+    Issue.column_names.include?(params[:sort]) ? params[:sort] : "status"
+  end
+
+  def sort_direction

This function doesn't seem to be used anywhere.

> @@ -0,0 +1,36 @@
+class ReportsController < ApplicationController
+  layout "site"
+
+  before_action :authorize_web
+  before_action :require_user
+
+  def new
+    if create_new_report_params.present?

When would the parameters not be present, and what should happen then? It looks to me like the view would just bomb because `@report` would not exist, so presumably it's not supposed to happen?

> +
+  def new
+    if create_new_report_params.present?
+      @report = Report.new
+      @report.issue = Issue.find_or_initialize_by(create_new_report_params)
+    end
+  end
+
+  def create
+    @report = current_user.reports.new(report_params)
+    @report.issue = Issue.find_or_initialize_by(:reportable_id => params[:report][:issue][:reportable_id], :reportable_type => params[:report][:issue][:reportable_type])
+
+    if @report.save
+      @report.issue.save
+      @report.issue.reopen! unless @report.issue.open?
+      redirect_to root_path, :notice => t("issues.create.successful_report")

Should we not be trying to remember where the user was when they made a report and then send them back there rather than dumping them back at the home page?

> @@ -0,0 +1,99 @@
+# == Schema Information
+#
+# Table name: issues
+#
+#  id               :integer          not null, primary key
+#  reportable_type  :string           not null
+#  reportable_id    :integer          not null
+#  reported_user_id :integer

Shouldn't this be `not null` here? Is it ever possible to have a report that doesn't map to a user?

> @@ -0,0 +1,99 @@
+# == Schema Information
+#
+# Table name: issues
+#
+#  id               :integer          not null, primary key
+#  reportable_type  :string           not null
+#  reportable_id    :integer          not null
+#  reported_user_id :integer
+#  status           :integer

Why is this an integer rather than an enum? Is this a requirement for using the state machine?

> +#  resolved_at      :datetime
+#  resolved_by      :integer
+#  updated_by       :integer
+#  reports_count    :integer          default(0)
+#  created_at       :datetime         not null
+#  updated_at       :datetime         not null
+#
+# Indexes
+#
+#  index_issues_on_reportable_id_and_reportable_type  (reportable_id,reportable_type)
+#  index_issues_on_reported_user_id                   (reported_user_id)
+#  index_issues_on_updated_by                         (updated_by)
+#
+# Foreign Keys
+#
+#  issues_reported_user_id_fkey  (reported_user_id => users.id) ON DELETE => cascade

I'm not sure I like having `ON DELETE CASCADE` on these (and other) foreign keys. None of our existing ones have that and it's quite dangerous.

> +#  resolved_by      :integer
+#  updated_by       :integer
+#  reports_count    :integer          default(0)
+#  created_at       :datetime         not null
+#  updated_at       :datetime         not null
+#
+# Indexes
+#
+#  index_issues_on_reportable_id_and_reportable_type  (reportable_id,reportable_type)
+#  index_issues_on_reported_user_id                   (reported_user_id)
+#  index_issues_on_updated_by                         (updated_by)
+#
+# Foreign Keys
+#
+#  issues_reported_user_id_fkey  (reported_user_id => users.id) ON DELETE => cascade
+#  issues_updated_by_fkey        (updated_by => users.id) ON DELETE => cascade

Shouldn't there be a foreign key constraint on `resolved_by` as well?

> +# Indexes
+#
+#  index_issues_on_reportable_id_and_reportable_type  (reportable_id,reportable_type)
+#  index_issues_on_reported_user_id                   (reported_user_id)
+#  index_issues_on_updated_by                         (updated_by)
+#
+# Foreign Keys
+#
+#  issues_reported_user_id_fkey  (reported_user_id => users.id) ON DELETE => cascade
+#  issues_updated_by_fkey        (updated_by => users.id) ON DELETE => cascade
+#
+
+class Issue < ActiveRecord::Base
+  belongs_to :reportable, :polymorphic => true
+  belongs_to :reported_user, :class_name => "User", :foreign_key => :reported_user_id
+  belongs_to :user_updated, :class_name => "User", :foreign_key => :updated_by

Should we have a `belongs_to` for `resolved_by` as well?

> +  has_many :comments, :class_name => "IssueComment", :dependent => :destroy
+
+  validates :reportable_id, :uniqueness => { :scope => [:reportable_type] }
+
+  ASSIGNED_ROLES = %w[administrator moderator].freeze
+  validates :assigned_role, :presence => true, :inclusion => ASSIGNED_ROLES
+
+  before_validation :set_default_assigned_role
+  before_validation :set_reported_user
+
+  # Check if more statuses are needed
+  enum :status => %w[open ignored resolved]
+
+  scope :with_status, ->(issue_status) { where(:status => statuses[issue_status]) }
+
+  def read_reports

Would `resolved_reports` be a better name here? That seems to be what it is actually doing? Same goes for the inverse version of course.

> +
+  # Check if more statuses are needed
+  enum :status => %w[open ignored resolved]
+
+  scope :with_status, ->(issue_status) { where(:status => statuses[issue_status]) }
+
+  def read_reports
+    resolved_at.present? ? reports.where("updated_at < ?", resolved_at) : nil
+  end
+
+  def unread_reports
+    resolved_at.present? ? reports.where("updated_at >= ?", resolved_at) : reports
+  end
+
+  include AASM
+  aasm :column => :status, :no_direct_assignment => true do

There's probably no right or wrong answer here, but do we really gain anything from pulling in `aasm` here and doing a state machine rather than just defining three fairly simple methods?

> +  private
+
+  def set_reported_user
+    self.reported_user = case reportable.class.name
+                         when "User"
+                           reportable
+                         when "Note"
+                           reportable.author
+                         else
+                           reportable.user
+                         end
+  end
+
+  def set_default_assigned_role
+    role = %w[Note].include?(reportable.class.name) ? "moderator" : "administrator"
+    self.assigned_role = role if assigned_role.blank?

This seems to be written in quite an obtuse way. I'd be inclined to go for something like:

```
if assigned_role.blank?
  self.assigned_role = case reportable.class.name
                       when "Note" then "moderator"
                       else "administrator"
                       end
end
```

> +#  id               :integer          not null, primary key
+#  reportable_type  :string           not null
+#  reportable_id    :integer          not null
+#  reported_user_id :integer
+#  status           :integer
+#  assigned_role    :enum             not null
+#  resolved_at      :datetime
+#  resolved_by      :integer
+#  updated_by       :integer
+#  reports_count    :integer          default(0)
+#  created_at       :datetime         not null
+#  updated_at       :datetime         not null
+#
+# Indexes
+#
+#  index_issues_on_reportable_id_and_reportable_type  (reportable_id,reportable_type)

It probably doesn't matter, unless we're going to be looking up issues by type without an id, but I would have put type before id on this index.

> @@ -0,0 +1,40 @@
+# == Schema Information
+#
+# Table name: reports
+#
+#  id         :integer          not null, primary key
+#  issue_id   :integer

I assume this should have a `not null` constraint?

> @@ -0,0 +1,40 @@
+# == Schema Information
+#
+# Table name: reports
+#
+#  id         :integer          not null, primary key
+#  issue_id   :integer
+#  user_id    :integer

I assume this should have a `not null` constraint?

> @@ -0,0 +1,40 @@
+# == Schema Information
+#
+# Table name: reports
+#
+#  id         :integer          not null, primary key
+#  issue_id   :integer
+#  user_id    :integer
+#  details    :text             not null
+#  category   :string           not null

Should this be an enum? The categories are bounded right?

> @@ -73,6 +73,12 @@ class User < ActiveRecord::Base
 
   has_many :roles, :class_name => "UserRole"
 
+  has_many :issues, :class_name => "Issue", :foreign_key => :reported_user_id
+  has_one :issue, :class_name => "Issue", :foreign_key => :updated_by

Does this make any sense? It's on `updated_by` so it's linking to issues updated by this user, but is `has_one` which doesn't seem to be right?

> @@ -17,6 +17,11 @@
       <br/>
       <%= note_event(@note.status, @note.closed_at, @note_comments.last.author) %>
     <% end %>
+    <% if current_user && current_user != @note.author %>

I suspect that this whole block, which is going to be repeated a number of times, should probably be in a helper. I guess it would need to be given the object and the title and could probably figure out everything else from that?

> @@ -17,6 +17,11 @@
       <br/>
       <%= note_event(@note.status, @note.closed_at, @note_comments.last.author) %>
     <% end %>
+    <% if current_user && current_user != @note.author %>
+      <%= link_to new_report_url(reportable_id: @note.id, reportable_type: @note.class.name), :title => t('browse.note.report') do %>
+           ⚐

I really don't like the use of that unicode flag character - is anybody likely to recognise what it means? I don't think it's commonly used elsewhere? There's also the question of how widely it will render correctly...

If we can't come up with anything better then maybe we should just use text like `Report user` etc instead?

> @@ -0,0 +1,25 @@
+<div class="issue-comments">
+  <% comments.each do |comment| %>
+    <div class="comment">
+      <div style="float:left">
+        <%= link_to user_thumbnail(comment.user), :controller => :user, :action =>:view, :display_name => comment.user.display_name %>
+      </div>
+      <b> <%= link_to comment.user.display_name, :controller => :user, :action =>:view, :display_name => comment.user.display_name %> </b> <br/>
+      <%= comment.body %>

Shouldn't this be going through the rich text renderer? Either that or we shouldn't be using a `richtext_area` to collect the comments...

> @@ -0,0 +1,19 @@
+<% reports.each do |report| %>
+  <div class="reports">

As this is inside the loop, and hence only contains one report, this seems like the wrong class name to use?

> +    </div>
+    <% end %>
+
+    <% if @unread_reports.any? %>
+    <div class="unread-reports">
+      <h4><%= t ".new_reports" %></h4>
+      <%= render 'reports', reports: @unread_reports %>
+    </div>
+    <% end %>
+    <br/>
+  </div>
+
+  <% if @issue.reported_user %>
+    <div class="related-block">
+      <h3><%= t ".other_issues_against_this_user" %></h3>
+      <div class="unread-reports">

I think we could do with some sort of styling here to separate entries in the "other issues" list as currently it's just a mass of link text (which sometimes wraps) and it's not clear where one ends and the next begins.

> @@ -0,0 +1,44 @@
+<% content_for :heading do %>
+  <h1><%= t ".title" %></h1>
+<% end %>
+
+<%= form_tag(issues_path, :method => :get) do %>

I think this should be a POST form as currently it leads to very ugly URLs which doesn't seem very nice?

Also the form doesn't seem to preserve state so I can't tell what I'm currently filtering on and I have to start from scratch each time if I want to adjust my filter.

> +<table>
+  <thead>
+    <tr>
+      <td><b><%= t ".status" %></b></td>
+      <td><b><%= t ".reports" %></b></td>
+      <td><b><%= t ".reported_item" %></b></td>
+      <td><b><%= t ".reported_user" %></b></td>
+      <td><b><%= t ".last_updated_by" %></b></td>
+      <td><b><%= t ".last_updated_at" %></b></td>
+    </tr>
+  </thead>
+  <tbody>
+    <% @issues.each do |issue| %>
+      <tr>
+        <td><%= t "issues.states.#{issue.status}" %></td>
+        <td><%= link_to t(".reports_count", :count => issue.reports_count), issue %></td>

Currently this column can, and does, wrap between the number and the word reports. I think it would be better marked as nowrap to stop that.

> +      <td><b><%= t ".reports" %></b></td>
+      <td><b><%= t ".reported_item" %></b></td>
+      <td><b><%= t ".reported_user" %></b></td>
+      <td><b><%= t ".last_updated_by" %></b></td>
+      <td><b><%= t ".last_updated_at" %></b></td>
+    </tr>
+  </thead>
+  <tbody>
+    <% @issues.each do |issue| %>
+      <tr>
+        <td><%= t "issues.states.#{issue.status}" %></td>
+        <td><%= link_to t(".reports_count", :count => issue.reports_count), issue %></td>
+        <td><%= link_to reportable_title(issue.reportable), reportable_url(issue.reportable) %></td>
+        <td><%= link_to issue.reported_user.display_name, :controller => :user, :action => :view, :display_name => issue.reported_user.display_name if issue.reported_user %></td>
+        <td><% if issue.user_updated %> <%= issue.user_updated.display_name %> <% else %> - <% end %></td>
+        <td><%= l(issue.updated_at.to_datetime, :format => :friendly) %></td>

I wonder if we should make this a friendly date, and maybe merge it with the previous column as "Last Updated" with entries like "3 minutes ago by fred" which might save some horizontal space?

> @@ -0,0 +1,19 @@
+<% reports.each do |report| %>
+  <div class="reports">
+    <div style="float:left">
+      <%= link_to user_thumbnail(report.user), :controller => :user, :action =>:view, :display_name => report.user.display_name %>
+    </div>
+    <%= t(".reported_by_html", :user_name => report.user.display_name, :user_url => url_for(:controller => :user, :action => :view, :display_name => report.user.display_name)) %> <br/>
+    <span class="deemphasize">
+      <%= t(".updated_at", :datetime => l(report.updated_at.to_datetime, :format => :friendly)) %>
+    </span>
+    <br/>
+    <span class="deemphasize">
+      <%= t ".category", category: report.category %>

I'm tempted to get rid of this as a separate line and merge into the first line as "Reported as %{category} by %{user}" to save a bit of space and rid of the line with the ugly "Category:" prefix.

> @@ -0,0 +1,42 @@
+<% content_for :heading do %>
+  <h1><%= t ".title_html", :link => link_to(reportable_title(@report.issue.reportable), reportable_url(@report.issue.reportable)) %></h1>
+<% end %>
+
+<div class="report-disclaimer">

I think this box could do with a bit more padding, especially left and right?

> +
+<%= form_for(@report) do |f| %>
+  <%= f.error_messages %>
+  <fieldset>
+    <%= f.fields_for @report.issue do |issue_form| %>
+      <%= issue_form.hidden_field :reportable_id %>
+      <%= issue_form.hidden_field :reportable_type %>
+    <% end %>
+
+    <div class='form-row'>
+      <p><%= t('issues.new.select') %></p>
+      <ul>
+      <% Report.categories_for(@report.issue.reportable).each do |c| %>
+        <li>
+          <%= radio_button :report, :category, c %>
+          <%= label_tag "report_category_#{c}", t("reports.categories.#{@report.issue.reportable.class.name}.#{c}") %> <br/>

I'm inclined to say we should run `underscore` on the class name here to make the locale keys more typical?

> +      category: "Category: %{category}"
+      updated_at: "On %{datetime}"
+      reported_by_html: "Reported by <a href=\"%{user_url}\">%{user_name}</a>"
+    resolved: Issue status has been set to 'Resolved'
+    ignored: Issue status has been set to 'Ignored'
+    reopened: Issue status has been set to 'Open'
+    states:
+      ignored: Ignored
+      open: Open
+      resolved: Resolved
+  reports:
+    new:
+      title_html: "Report %{link}"
+    categories:
+      DiaryEntry:
+        spam: This Diary Entry is/contains spam

I don't think "Diary Entry" should be capitalised here - same goes for diary comments and users. Oddly notes is already lower case, which is inconsistent anyway.

> +        threat: This Diary Entry contains a threat
+        other: Other
+      DiaryComment:
+        spam: This Diary Comment is/contains spam
+        offensive: This Diary Comment is obscene/offensive
+        threat: This Diary Comment contains a threat
+        other: Other
+      User:
+        spam: This User profile is/contains spam
+        offensive: This User profile is obscene/offensive
+        threat: This User profile contains a threat
+        vandal: This User is a vandal
+        other: Other
+      Note:
+        spam: This note is spam
+        vandalism: This note is vandalism

Not sure how a note can be vandalism? What exactly is it vandalising?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/1576#pullrequestreview-101007408
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20180304/3faf21ec/attachment-0001.html>


More information about the rails-dev mailing list