-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
base: develop
Are you sure you want to change the base?
Enhance report overview #8239
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
@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 | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Couldn't that query be written with ActiveRecord? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
Comment on lines
+7
to
25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These helpers also get used for the notification mails (hence the name "NotifierHelper"), you can't just completely change it's functionality and drop html support. But it looks like you aren't even using these helpers anymore, so maybe just revert these changes? |
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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`!" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rails/I18nLocaleTexts: Move locale texts to the locale files in the |
||
} | ||
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? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -50,12 +61,16 @@ def destroy_reported_item | |
item.destroy | ||
end | ||
end | ||
mark_as_reviewed | ||
mark_as_reviewed(STATUS_DELETED) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no |
||
end | ||
# rubocop:enable Rails/SkipsModelValidations | ||
|
||
def send_report_notification | ||
Workers::Mail::ReportWorker.perform_async(id) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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| | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
- @unreviewed_reports.each do |report| | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lint/UselessAssignment: Useless assignment to variable - |
||
.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')}:" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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')}:" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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" |
There was a problem hiding this comment.
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.