Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance report overview #8239

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
46 changes: 33 additions & 13 deletions app/controllers/report_controller.rb
Expand Up @@ -9,14 +9,27 @@ class ReportController < ApplicationController
before_action :redirect_unless_moderator, except: [:create]

def index
@reports = Report.where(reviewed: false)
@unreviewed_reports = Report.join_originator.where(reviewed: false).order(created_at: :desc)
@reviewed_reports = Report.join_originator.where(reviewed: true).order(created_at: :desc)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this is a good idea. Big pods can have a lot of reports, and loading all of them might be a performance problem, especially since this is always loaded, even if you only want to view new unreviewed reports.

So this either needs some sort of pagination, or at least only load the reviewed reports if you actually want to load that tab.

@statistics_by_reporter = statistics_by_reporter
@statistics_by_author = statistics_by_author
end

def create
report = current_user.reports.new(report_params)
report.reported_author_id = report.reported_author.id
if report.save
render json: true, status: :ok
else
head :conflict
end
end

def update
if report = Report.where(id: params[:id]).first
report.mark_as_reviewed
end
redirect_to :action => :index
redirect_to action: :index
end

def destroy
Expand All @@ -28,17 +41,24 @@ def destroy
redirect_to action: :index
end

def create
report = current_user.reports.new(report_params)
if report.save
render json: true, status: 200
else
head :conflict
end
private

def report_params
params.require(:report).permit(:item_id, :item_type, :text)
end

private
def report_params
params.require(:report).permit(:item_id, :item_type, :text)
end
def statistics_by_reporter
sql = "select count(*), diaspora_handle, guid from reports
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't that query be written with ActiveRecord?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, But I don't have an idea how. The trick here is: the group by counts all reports, and adds the diaspora_handle from people's table. If this is possible, I think the sql is more straight-forward.

join people on reports.user_id = people.owner_id
group by diaspora_handle, guid order by 1 desc"
ActiveRecord::Base.connection.exec_query sql
end

def statistics_by_author
sql = "select count(*), diaspora_handle, guid from reports
left join people on reported_author_id = people.id
where reported_author_id is not null
group by diaspora_handle, guid order by 1 desc"
ActiveRecord::Base.connection.exec_query sql
end
end
14 changes: 8 additions & 6 deletions app/helpers/notifier_helper.rb
Expand Up @@ -4,21 +4,23 @@ module NotifierHelper
include PostsHelper

# @param post [Post] The post object.
# @param opts [Hash] Optional hash. Accepts :html parameter.
# @param opts [Hash] Optional hash. Accepts :length parameters.
# @return [String] The formatted post.
def post_message(post, opts={})
rendered = opts[:html] ? post.message&.markdownified_for_mail : post.message&.plain_text_without_markdown
rendered.presence || post_page_title(post)
if post.respond_to? :message
post.message.try(:plain_text_without_markdown).presence || post_page_title(post)
else
I18n.t "notifier.a_post_you_shared"
end
end

# @param comment [Comment] The comment to process.
# @param opts [Hash] Optional hash. Accepts :html parameter.
# @return [String] The formatted comment.
def comment_message(comment, opts={})
if comment.post.public?
opts[:html] ? comment.message.markdownified_for_mail : comment.message.plain_text_without_markdown
comment.message.plain_text_without_markdown
else
I18n.translate "notifier.a_limited_post_comment"
I18n.t "notifier.a_limited_post_comment"
end
end
end
27 changes: 24 additions & 3 deletions app/helpers/report_helper.rb
Expand Up @@ -5,20 +5,41 @@
# the COPYRIGHT file.

module ReportHelper
# rubocop:disable Rails/OutputSafety
def report_content(report)
case (item = report.item)
when Post
raw t("report.post_label", content: link_to(post_message(item), post_path(item.id)))
raw t("report.post_label", content: link_to(post_page_title(item), post_path(item.id)))
when Comment
raw t("report.comment_label", data: link_to(
h(comment_message(item)),
raw t("report.comment_label", data: link_to(item.message.title,
post_path(item.post.id, anchor: item.guid)
))
else
t("report.not_found")
end
end

# rubocop:enable Rails/OutputSafety

def link_to_content(report)
case (item = report.item)
when Post
link_to("", post_path(item.id),
{title: t("report.view_reported_element"),
class: "entypo-eye",
target: "_blank",
rel: "noopener"})
when Comment
link_to("", post_path(item.post.id, anchor: item.guid),
{title: t("report.view_reported_element"),
class: "entypo-eye",
target: "_blank",
rel: "noopener"})
else
t("report.not_found")
end
end

def unreviewed_reports_count
@unreviewed_reports_count ||= Report.where(reviewed: false).size
end
Expand Down
35 changes: 25 additions & 10 deletions app/models/report.rb
Expand Up @@ -4,33 +4,44 @@ class Report < ApplicationRecord
validates :user_id, presence: true
validates :item_id, presence: true
validates :item_type, presence: true, inclusion: {
in: %w(Post Comment), message: "Type should match `Post` or `Comment`!"}
in: %w[Post Comment], message: "Type should match `Post` or `Comment`!"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rails/I18nLocaleTexts: Move locale texts to the locale files in the config/locales directory.

}
validates :text, presence: true

validate :entry_does_not_exist, :on => :create
validate :post_or_comment_does_exist, :on => :create
validate :entry_does_not_exist, on: :create
validate :post_or_comment_does_exist, on: :create

belongs_to :user
belongs_to :post, optional: true
belongs_to :comment, optional: true
belongs_to :item, polymorphic: true

after_commit :send_report_notification, :on => :create
STATUS_DELETED = "deleted"
STATUS_NO_ACTION = "no_action"

after_commit :send_report_notification, on: :create

scope :join_originator, -> {
joins("LEFT JOIN people ON reported_author_id = people.id ")
.select("reports.*, people.diaspora_handle as reported_author, people.guid as reported_author_guid")
}

def reported_author
item.author if item
return Person.find(reported_author_id) if reported_author_id.present?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/EmptyLineAfterGuardClause: Add empty line after guard clause.


item&.author
end

def entry_does_not_exist
return unless Report.where(item_id: item_id, item_type: item_type).exists?(user_id: user_id)

errors.add(:base, "You cannot report the same post twice.")
errors[:base] << "You cannot report the same post twice."

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rails/DeprecatedActiveModelErrorsMethods: Avoid manipulating ActiveModel errors as hash directly.

end

def post_or_comment_does_exist
return unless Post.find_by(id: item_id).nil? && Comment.find_by(id: item_id).nil?

errors.add(:base, "Post or comment was already deleted or doesn't exists.")
errors[:base] << "Post or comment was already deleted or doesn't exists."

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rails/DeprecatedActiveModelErrorsMethods: Avoid manipulating ActiveModel errors as hash directly.

end

def destroy_reported_item
Expand All @@ -50,12 +61,16 @@ def destroy_reported_item
item.destroy
end
end
mark_as_reviewed
mark_as_reviewed(STATUS_DELETED)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/AbcSize: Assignment Branch Condition size for destroy_reported_item is too high. [<0, 32, 7> 32.76/20]

end

def mark_as_reviewed
Report.where(item_id: item_id, item_type: item_type).update_all(reviewed: true)
# rubocop:disable Rails/SkipsModelValidations

def mark_as_reviewed(with_action=STATUS_NO_ACTION)
Report.where(item_id: item_id, item_type: item_type)
.update_all(reviewed: true, action: with_action)
end
# rubocop:enable Rails/SkipsModelValidations

def send_report_notification
Workers::Mail::ReportWorker.perform_async(id)
Expand Down
42 changes: 42 additions & 0 deletions app/views/report/_checked.haml
@@ -0,0 +1,42 @@
%table.table
%tr
%th
= t("report.report_created_at")
%th
= t("report.reported_by")
%th
= t("report.reason")
%th
= t("report.type")
%th
= t("report.reported_author")
%th
= t("report.decision")
%th
= t("report.action")
- @reviewed_reports.each do |report|

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using instance variables in partials views

%tr
%td
= I18n.l(report.created_at, format: :short)
%td
= link_to(report.user.username, user_profile_path(report.user.username))
%td
= report.text
%td
= report.item_type
%td
- if report.reported_author.nil?
= t("report.reported_author_unknown")
- else
= link_to(report.reported_author.diaspora_handle, "/people/#{report.reported_author_guid}")
%td
= t("report.#{report.action}") if report.action.present?
= t("report.decision_unknown") if report.action.nil?
%td
- unless report.item.nil?
= link_to_content(report)
= link_to(report_path(report.id, type: report.item_type),
data: {confirm: t("report.confirm_deletion")},
title: t("report.delete_link"),
class: "delete", method: :delete) do
%i.entypo-trash
43 changes: 43 additions & 0 deletions app/views/report/_pending.haml
@@ -0,0 +1,43 @@
- @unreviewed_reports.each do |report|

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using instance variables in partials views

.panel.panel-default
- reported_by = report.user.username

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint/UselessAssignment: Useless assignment to variable - reported_by. Did you mean report?

.panel-heading
.reporter.pull-right
!= t("report.reported_label", person: link_to(reported_by, user_profile_path(reported_by)))
.author
%span.reason-label
!= "#{t('report.reported_author')}:"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

%span
= report.reported_author.name if report.item
!= "-" if report.item.nil?
.created-at
%span.reason-label
!= "#{t('report.report_created_at')}:"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

%span
= I18n.l(report.created_at, format: :short)
.reason
%span.reason-label
= t("report.reason_label")
%span
= report.text
.panel-body
.content
= report_content(report)
.segment-selection
- if report.item.present?
= button_to t("report.reported_user_details"),
user_search_path(admins_controller_user_search: {guid: report.reported_author.guid}),
class: "btn pull-left btn-info btn-small col-md-3 col-xs-12",
method: :post
= button_to t("report.review_link"),
report_path(report.id, type: report.item_type),
class: "btn pull-left btn-info btn-small col-md-3 col-xs-12",
method: :put
= button_to t("report.delete_link"),
report_path(report.id, type: report.item_type),
data: {confirm: t("report.confirm_deletion")},
class: "btn pull-right btn-danger btn-small col-md-3 col-xs-12",
method: :delete
- if @unreviewed_reports.empty?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using instance variables in partials views

%h3
= t("report.no_pending_reports")
72 changes: 33 additions & 39 deletions app/views/report/_reports.haml
@@ -1,44 +1,38 @@

- content_for :head do
= stylesheet_link_tag :admin

.reports
%h1
= t("report.title")
- if @reports.empty?
%p
= t("report.unreviewed_reports", count: 0)
- @reports.each do |report|
- if report.item
.panel.panel-default
- username = report.user.username
.panel-heading
.reporter.pull-right
!= t("report.reported_label", person: link_to(username, user_profile_path(username)))
.reason
%span.reason-label
= t("report.reason_label")
%span
= report.text
.panel-body
.content
= report_content(report)
.segment-selection
= button_to t("report.reported_user_details"),
user_search_path(admins_controller_user_search: {guid: report.reported_author.guid}),
class: "btn pull-left btn-info btn-small col-md-3 col-xs-12", method: :post
= button_to t("report.review_link"), report_path(report.id, type: report.item_type),
class: "btn pull-left btn-info btn-small col-md-3 col-xs-12", method: :put
= button_to t("report.delete_link"), report_path(report.id, type: report.item_type),
data: {confirm: t("report.confirm_deletion")},
class: "btn pull-right btn-danger btn-small col-md-3 col-xs-12", method: :delete
- else
.panel.panel-default
- username = report.user.username
.panel-heading
.reporter.pull-right
!= t("report.reported_label", person: link_to(username, user_profile_path(username)))
.title
= report_content(report)
.panel-body
= button_to t("report.review_link"), report_path(report.id, type: report.item_type),
class: "btn pull-left btn-info btn-small", method: :put
%ul.nav.nav-tabs{role: "tablist"}
%li.nav-item.active{role: "presentation"}
%a{
href: "#reports",
title: t("report.tooltip_incomming"),
aria: {controls: "reports", selected: true},
data: {toggle: "tab"}
}
= t("report.reported_tab")
%li.nav-item{role: "presentation"}
%a{
href: "#checked",
title: t("report.tooltip_reviewed"),
aria: {controls: "checked", selected: false},
data: {toggle: "tab"}
}
= t("report.reviewed_tab")
%li.nav-item{role: "presentation"}
%a{
href: "#statistics",
title: t("report.tooltip_statistics"),
aria: {controls: "statistics", selected: false},
data: {toggle: "tab"}
}
= t("report.statistics_tab")
.tab-content
.tab-pane.active#reports{role: "tabpanel"}
= render partial: "report/pending"
.tab-pane#checked{role: "tabpanel"}
= render partial: "report/checked"
.tab-pane#statistics{role: "tabpanel"}
= render partial: "report/statistics"