<p><b>@tomhughes</b> requested changes on this pull request.</p>
<p>I think I've gone through all models, code and view now - haven't looked at the tests yet.</p><hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172050499">app/controllers/issue_comments_controller.rb</a>:</p>
<pre style='color:#555'>> + 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")
</pre>
<p>It looks like this only needs a moderator, not an administrator?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172050990">app/controllers/issues_controller.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,128 @@
+class IssuesController < ApplicationController
+ layout "site"
+
+ before_action :authorize_web
+ before_action :require_user
+ before_action :set_issues
</pre>
<p>What's the point of this? All it does is define a couple of member variables which are then used once in the <code>index</code> method to assign to a different member variable...</p>
<p>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 <code>index</code> is probably most sensible?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172051481">app/controllers/issues_controller.rb</a>:</p>
<pre style='color:#555'>> + @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
</pre>
<p>This function doesn't seem to be used anywhere?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172051507">app/controllers/issues_controller.rb</a>:</p>
<pre style='color:#555'>> + 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
</pre>
<p>This function doesn't seem to be used anywhere?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172051527">app/controllers/issues_controller.rb</a>:</p>
<pre style='color:#555'>> + 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
</pre>
<p>This function doesn't seem to be used anywhere? I think it got moved to the report controller?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172051540">app/controllers/issues_controller.rb</a>:</p>
<pre style='color:#555'>> + 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
</pre>
<p>This function doesn't seem to be used anywhere? Moved to the issue comment controller?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172051557">app/controllers/issues_controller.rb</a>:</p>
<pre style='color:#555'>> + 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
</pre>
<p>This function doesn't seem to be used anywhere.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172051591">app/controllers/issues_controller.rb</a>:</p>
<pre style='color:#555'>> + 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
</pre>
<p>This function doesn't seem to be used anywhere.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172051692">app/controllers/reports_controller.rb</a>:</p>
<pre style='color:#555'>> @@ -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?
</pre>
<p>When would the parameters not be present, and what should happen then? It looks to me like the view would just bomb because <code>@report</code> would not exist, so presumably it's not supposed to happen?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172051726">app/controllers/reports_controller.rb</a>:</p>
<pre style='color:#555'>> +
+ 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")
</pre>
<p>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?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172051867">app/models/issue.rb</a>:</p>
<pre style='color:#555'>> @@ -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
</pre>
<p>Shouldn't this be <code>not null</code> here? Is it ever possible to have a report that doesn't map to a user?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172051890">app/models/issue.rb</a>:</p>
<pre style='color:#555'>> @@ -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
</pre>
<p>Why is this an integer rather than an enum? Is this a requirement for using the state machine?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172051941">app/models/issue.rb</a>:</p>
<pre style='color:#555'>> +# 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
</pre>
<p>I'm not sure I like having <code>ON DELETE CASCADE</code> on these (and other) foreign keys. None of our existing ones have that and it's quite dangerous.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172051960">app/models/issue.rb</a>:</p>
<pre style='color:#555'>> +# 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
</pre>
<p>Shouldn't there be a foreign key constraint on <code>resolved_by</code> as well?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172051996">app/models/issue.rb</a>:</p>
<pre style='color:#555'>> +# 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
</pre>
<p>Should we have a <code>belongs_to</code> for <code>resolved_by</code> as well?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172052103">app/models/issue.rb</a>:</p>
<pre style='color:#555'>> + 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
</pre>
<p>Would <code>resolved_reports</code> be a better name here? That seems to be what it is actually doing? Same goes for the inverse version of course.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172052135">app/models/issue.rb</a>:</p>
<pre style='color:#555'>> +
+ # 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
</pre>
<p>There's probably no right or wrong answer here, but do we really gain anything from pulling in <code>aasm</code> here and doing a state machine rather than just defining three fairly simple methods?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172052252">app/models/issue.rb</a>:</p>
<pre style='color:#555'>> + 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?
</pre>
<p>This seems to be written in quite an obtuse way. I'd be inclined to go for something like:</p>
<pre><code>if assigned_role.blank?
self.assigned_role = case reportable.class.name
when "Note" then "moderator"
else "administrator"
end
end
</code></pre>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172052306">app/models/issue.rb</a>:</p>
<pre style='color:#555'>> +# 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)
</pre>
<p>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.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172052330">app/models/report.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,40 @@
+# == Schema Information
+#
+# Table name: reports
+#
+# id :integer not null, primary key
+# issue_id :integer
</pre>
<p>I assume this should have a <code>not null</code> constraint?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172052332">app/models/report.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,40 @@
+# == Schema Information
+#
+# Table name: reports
+#
+# id :integer not null, primary key
+# issue_id :integer
+# user_id :integer
</pre>
<p>I assume this should have a <code>not null</code> constraint?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172052354">app/models/report.rb</a>:</p>
<pre style='color:#555'>> @@ -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
</pre>
<p>Should this be an enum? The categories are bounded right?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172052463">app/models/user.rb</a>:</p>
<pre style='color:#555'>> @@ -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
</pre>
<p>Does this make any sense? It's on <code>updated_by</code> so it's linking to issues updated by this user, but is <code>has_one</code> which doesn't seem to be right?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172052583">app/views/browse/note.html.erb</a>:</p>
<pre style='color:#555'>> @@ -17,6 +17,11 @@
<br/>
<%= note_event(@note.status, @note.closed_at, @note_comments.last.author) %>
<% end %>
+ <% if current_user && current_user != @note.author %>
</pre>
<p>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?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172052634">app/views/browse/note.html.erb</a>:</p>
<pre style='color:#555'>> @@ -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 %>
+ ⚐
</pre>
<p>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...</p>
<p>If we can't come up with anything better then maybe we should just use text like <code>Report user</code> etc instead?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172052752">app/views/issues/_comments.html.erb</a>:</p>
<pre style='color:#555'>> @@ -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 %>
</pre>
<p>Shouldn't this be going through the rich text renderer? Either that or we shouldn't be using a <code>richtext_area</code> to collect the comments...</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172052986">app/views/issues/_reports.html.erb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,19 @@
+<% reports.each do |report| %>
+ <div class="reports">
</pre>
<p>As this is inside the loop, and hence only contains one report, this seems like the wrong class name to use?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172053037">app/views/issues/show.html.erb</a>:</p>
<pre style='color:#555'>> + </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">
</pre>
<p>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.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172053088">app/views/issues/index.html.erb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,44 @@
+<% content_for :heading do %>
+ <h1><%= t ".title" %></h1>
+<% end %>
+
+<%= form_tag(issues_path, :method => :get) do %>
</pre>
<p>I think this should be a POST form as currently it leads to very ugly URLs which doesn't seem very nice?</p>
<p>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.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172053165">app/views/issues/index.html.erb</a>:</p>
<pre style='color:#555'>> +<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>
</pre>
<p>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.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172053201">app/views/issues/index.html.erb</a>:</p>
<pre style='color:#555'>> + <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>
</pre>
<p>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?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172053396">app/views/issues/_reports.html.erb</a>:</p>
<pre style='color:#555'>> @@ -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 %>
</pre>
<p>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.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172053531">app/views/reports/new.html.erb</a>:</p>
<pre style='color:#555'>> @@ -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">
</pre>
<p>I think this box could do with a bit more padding, especially left and right?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172053623">app/views/reports/new.html.erb</a>:</p>
<pre style='color:#555'>> +
+<%= 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/>
</pre>
<p>I'm inclined to say we should run <code>underscore</code> on the class name here to make the locale keys more typical?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172053669">config/locales/en.yml</a>:</p>
<pre style='color:#555'>> + 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
</pre>
<p>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.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#discussion_r172053688">config/locales/en.yml</a>:</p>
<pre style='color:#555'>> + 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
</pre>
<p>Not sure how a note can be vandalism? What exactly is it vandalising?</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#pullrequestreview-101007408">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABWnLZjsez44zropg6W88ZBaWS9wFz9Qks5tbA7pgaJpZM4OOfLC">mute the thread</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/ABWnLUmXsLo-mDGEsRTiZxVIOOq-mPXPks5tbA7pgaJpZM4OOfLC.gif" width="1" /></p>
<div itemscope itemtype="http://schema.org/EmailMessage">
<div itemprop="action" itemscope itemtype="http://schema.org/ViewAction">
<link itemprop="url" href="https://github.com/openstreetmap/openstreetmap-website/pull/1576#pullrequestreview-101007408"></link>
<meta itemprop="name" content="View Pull Request"></meta>
</div>
<meta itemprop="description" content="View this Pull Request on GitHub"></meta>
</div>
<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/openstreetmap/openstreetmap-website","title":"openstreetmap/openstreetmap-website","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/openstreetmap/openstreetmap-website"}},"updates":{"snippets":[{"icon":"PERSON","message":"@tomhughes requested changes on #1576"}],"action":{"name":"View Pull Request","url":"https://github.com/openstreetmap/openstreetmap-website/pull/1576#pullrequestreview-101007408"}}}</script>