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

Conversation

tclaus
Copy link
Member

@tclaus tclaus commented Apr 28, 2021

This PR enhances the admin/moderators report overview by

  • adding a view that shows recent reports and the action done (Item Deleted or not)
  • adding a statistics view (Who reports and who is the originator of a reported item)
  • a once 'reviewed report can be deleted retrospectively
  • let podsmins show the originator even /after/ a reported item was deleted
  • links to the originator profile directly (and not to the Search view)

Heads up!
It needs a migration!
Two fields are added: a 'action' textfield that is used to describe what happened to the report. Its free text, but filled based on the action.
The second fields holds the diaspora_handel for the originator. This is needed because after deleting a report this information is gone.

What is the outcome?
A Podmin now has more insights in old reports and in most reporter and most originators. A valuable base for more decisions what to do.

Future
List of reports grows without limits - a paginated view would be nice, but that was currently above my skill level.

Screenshots

(Added date time)
Bildschirmfoto 2021-04-28 um 08 11 07

( deleted or reviewed reports)
Bildschirmfoto 2021-04-28 um 08 10 56

(statistics with links to the authors / originators)
Bildschirmfoto 2021-04-28 um 08 11 17

Comments, Ideas, critics... welcome!

@tclaus
Copy link
Member Author

tclaus commented Apr 28, 2021

This Feature was requested in this discussion:
https://discourse.diasporafoundation.org/t/better-abilities-for-podmins-for-spam-analytics-and-controls/3896/6
and on this Issue: #8222

It is also IMHO a pre-work and touches issue #7847 "handling fake accounts / forward to pod".

@@ -8,17 +8,36 @@ module ReportHelper
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(truncated_post_message(item), post_path(item.id)))

Choose a reason for hiding this comment

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

Rails/OutputSafety: Tagging a string as html safe may be a security risk.

Copy link
Member Author

@tclaus tclaus Aug 26, 2021

Choose a reason for hiding this comment

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

@Flaburgan Here is the old ugly diaspora link-inside-translated text part. I did not care to change anything here

tclaus added a commit to tclaus/diaspora that referenced this pull request Aug 22, 2021
tclaus added a commit to tclaus/diaspora that referenced this pull request Aug 22, 2021
tclaus added a commit to tclaus/diaspora that referenced this pull request Aug 22, 2021
tclaus added a commit to tclaus/diaspora that referenced this pull request Aug 22, 2021
tclaus added a commit to tclaus/diaspora that referenced this pull request Aug 23, 2021
tclaus added a commit to tclaus/diaspora that referenced this pull request Aug 23, 2021
@Flaburgan
Copy link
Member

Hey! This is very needed, thank you for working on this. I'm going to try it and do a review, but two quick remarks:

  • I guess the reports only appear in the "Reviewed" tab if they have been reviewed, meaning they have been either "marked as reviewed" or "deleted". That means a boolean "deleted" or if you prefer an enum ["ignored", "deleted"] should be enough to be stored in the database, instead of a free text field. Strings displayed in the interface shouldn't be stored in the DB, it makes them untranslatable, and is bad practice anyway
  • Originator isn't clear to me. Why not using "Author" as everywhere else?

@tclaus
Copy link
Member Author

tclaus commented Aug 25, 2021

The Images in the first comment are not matching the text.

  • Originator isn't clear to me. Why not using "Author" as everywhere else?

"Reporter" and "Author" then - I missed this.

Do you mean to use the Database' ENUM type? Like the Idea.

@Flaburgan
Copy link
Member

Flaburgan commented Aug 25, 2021

Do you mean to use the Database' ENUM type? Like the Idea.

I mean your mockup gave the impression that "No action" was the text stored in the database. This must be avoided, the state (deleted or ignored) has to be stored in the DB, then the UI will display a label depending of the state.

@tclaus
Copy link
Member Author

tclaus commented Aug 26, 2021

@Flaburgan Can you make a re-Review? I have abstracted the action - text from database to UI.

Wouldn't it be more convenient to open the github "Reviewers" - section?

@Flaburgan
Copy link
Member

Flaburgan commented Aug 26, 2021

@Flaburgan Can you make a re-Review? I have abstracted the action - text from database to UI.

I was actually doing it right now 😄

Copy link
Member

@Flaburgan Flaburgan left a comment

Choose a reason for hiding this comment

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

This is GREAT, I love it!

I did some remarks but that's mostly in good shape already. But you would need to also add tests. I started to add them in #8035 but I'm actually not able to navigate to /report in my PR. If you help me fix that PR you should then be able to rebase on me and complete the tests there.

end

def update
if report = Report.where(id: params[:id]).first
report.mark_as_reviewed
report.update(action: "No Action")
Copy link
Member

Choose a reason for hiding this comment

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

You should use the constant there, shouldn't you?

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.

end

def statistics_by_author
sql = "select count(*), originator_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.

Same as above, is there any reason why you use SQL?

Copy link
Member Author

Choose a reason for hiding this comment

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

See above, it seems SQL is easier to read (and I don't have a clue how to build this in ActiveRecored) If this can be done, make a proposal without changing the returning data. (haml files rely on that)

end
end

def truncated_post_message(post)
Copy link
Member

Choose a reason for hiding this comment

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

This could have probably be done with an ellipsis in CSS, but I'm also fine with that code, especially because it checks the title

Copy link
Member

Choose a reason for hiding this comment

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

Actually, more than the title is displayed in the admin screen so not sure what this is about.

Copy link
Member Author

Choose a reason for hiding this comment

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

The old diaspora code has the link.tag as a translated text, which adds the un-markdoned directly inside the link-tag. So here can't be simply css cuts the text, it must be done in the plan-text part.


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"
Copy link
Member

Choose a reason for hiding this comment

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

I would say I prefer "ignored" than "no action", but this is a matter of taste probably. @goobertron opinion?

Choose a reason for hiding this comment

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

I'd need to know the context into which this text fits.

Copy link
Member

Choose a reason for hiding this comment

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

You have it in the column "Action" of the first screenshot in #8239 (comment)

But to me it's also about the internal naming. But that's not that important.

Choose a reason for hiding this comment

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

'no action' seems fine to me in that context. It makes clear that a decision has been made to take no action on that content. The label 'ignored' is potentially confusing: it could mean that the person who looked at the report decided to ignore the report (didn't make a decision), or that the content was 'ignored' i.e. hidden, or something else.

I'm not sure why some of the reports that have 'no action' next to them also have eye and bin icons to the right of them and others don't – is that clear to anyone using this feature?

Copy link
Member Author

Choose a reason for hiding this comment

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

As Fla found out, with introduction of the new "action" field, no existing Reports have a default initial value. I've added a migration and set all existing Reports to 'NO_ACTION', so every reviewed report should now have the action buttons.


class AddActionToReport < ActiveRecord::Migration[5.2]
def change
add_column :reports, :action, :string
Copy link
Member

Choose a reason for hiding this comment

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

You also need to add a migration for the previously reports to be set to "No action"

Copy link
Member

Choose a reason for hiding this comment

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

This isn't true, as not all reports were done with "no action", some might also have been deleted. So you could either decide if a post was deleted by checking if the post still exists (but that would also be guessing, as it could also be the author of the post who self deleted the post), or just keep it as NULL and then show it as "Unknown" in the frontend if there is no action, as we don't know what happened.

Edit: Ok, I just saw that you then mark them as deleted if they reported item doesn't exist anymore in the second migration. As said, I would combine those migrations to one (so not everything needs to be touched twice), and I still want to keep my original comment here, as marking them as deleted if it doesn't exist anymore still is kind of guessing, it doesn't mean it was deleted as part of the report, so I think just keeping them NULL, as that reflects the truth, and we don't know what happened.

What you could do as middle way is, that you mark them as "no action" if it still exists (because that one we know), and as NULL (and show "Unknown" in the UI) if it doesn't exist anymore, because that one we don't know.

Copy link
Member Author

@tclaus tclaus Oct 30, 2022

Choose a reason for hiding this comment

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

I have merged the two migrations together and adjusted the behavior according to your suggestion.

= report.text
.panel-body
.content
= report_content(report)
Copy link
Member

Choose a reason for hiding this comment

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

As you are modifying this code, could you please also add the author of the reported post?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in line 12 report.author.name is a delegate to report.item.author, aka the author of the reported post.

app/assets/javascripts/app/views.js Outdated Show resolved Hide resolved
app/assets/javascripts/app/views.js Outdated Show resolved Hide resolved
db/migrate/20210421064041_add_action_to_report.rb Outdated Show resolved Hide resolved
find("tr", text: "Deleted")
end
end

Choose a reason for hiding this comment

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

Layout/TrailingEmptyLines: 1 trailing blank lines detected.

@tclaus
Copy link
Member Author

tclaus commented Aug 28, 2021

@Flaburgan I rebased this on Enhance Report Form

tclaus added a commit to tclaus/diaspora that referenced this pull request Sep 19, 2021
With undecided / reviewed and statistics page.
Already reviewed reports stay visible.
Enhances Spam/user control

fixes diaspora#8239
tclaus added a commit to tclaus/diaspora that referenced this pull request Oct 5, 2021
tclaus added a commit to tclaus/diaspora that referenced this pull request Oct 5, 2021
tclaus added a commit to tclaus/diaspora that referenced this pull request Oct 5, 2021
tclaus added a commit to tclaus/diaspora that referenced this pull request Nov 9, 2021
tclaus added a commit to tclaus/diaspora that referenced this pull request Nov 9, 2021
tclaus added a commit to tclaus/diaspora that referenced this pull request Nov 9, 2021
@tclaus
Copy link
Member Author

tclaus commented Jun 10, 2022

@Flaburgan , @SuperTux88
Besides the (old and already existing) issues in Linter and Rubocop, is there anything which blocks a review?

@tclaus tclaus requested a review from SuperTux88 June 10, 2022 09:34
@Flaburgan
Copy link
Member

#8035 is adding tests on that part of the code so we said we should merge it first, but it's blocked by some weird Jasmine flickering tests. If you want to help there you're welcome!

Copy link
Member

@SuperTux88 SuperTux88 left a comment

Choose a reason for hiding this comment

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

As this is depending on #8035, this isn't a full review yet (I maybe have missed some things between all the changes of the other PR), and you might also want to wait until #8035 is done before working on some of my comments (as there will be conflicts). But there is also already some feedback you can work on without having conflicts, so I already wanted to submit that before the other PR is done.

Also, I couldn't add a comment to that file, as it doesn't have a single line, but you accidentally deleted tmp/pids/.gitkeep.

@@ -0,0 +1,19 @@
# frozen_string_literal: true

class AddUserFieldsToReports < ActiveRecord::Migration[5.2]
Copy link
Member

Choose a reason for hiding this comment

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

You can combine the two migrations into one adding both columns at the same time.


class AddUserFieldsToReports < ActiveRecord::Migration[5.2]
def change
add_column :reports, :originator_diaspora_handle, :string, index: true
Copy link
Member

Choose a reason for hiding this comment

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

First, please don't use the term "diaspora handle" anymore, it's called "diaspora* ID" now. There are still some really old columns that are called diaspora_handle but that's just because nobody renamed them yet.

Second, instead of storing the diaspora ID, you should just store the person ID.

I'm also not sure about "originator", I think reported_person_id or reported_author_id is clearer, that it's about the reported content not about the reporter (the originator of the report).

Copy link
Member Author

Choose a reason for hiding this comment

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

With ID you mean the numeric id column or the unique guid?

Copy link
Member

Choose a reason for hiding this comment

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

with "person ID" I mean the numeric id column of the people table.


after_commit :send_report_notification, :on => :create
STATUS_DELETED = "deleted"
STATUS_NO_ACTION = "no action"
Copy link
Member

Choose a reason for hiding this comment

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

I would define this as "no_action" (with underscore), so it can be directly used in translations keys without the need of asking for action_no_action?.

Comment on lines 79 to 85
def action_deleted?
action&.downcase == STATUS_DELETED.downcase
end

def action_no_action?
action&.downcase == STATUS_NO_ACTION.downcase
end
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 think these are needed, if you define the status variables with underscores, they can be directly used in translation keys and you don't need this extra step of asking this method. And for the tests where you used this method, you can just check if action equals the key it should.

Also I don't think this would need a downcase. This can basically only be something else than the status defined in the code if somebody manipulated the database manually, which isn't supported (if people do that, it's on their own risk, things will break, and if they not only change the casing but also add a typo, it wouldn't work anymore anyway).

Comment on lines 68 to 72
def mark_as_reviewed_and_deleted
Report.where(item_id: item_id, item_type: item_type)
.update_all(reviewed: true, action: STATUS_DELETED)
end

def mark_as_reviewed
Report.where(item_id: item_id, item_type: item_type).update_all(reviewed: true)
Report.where(item_id: item_id, item_type: item_type)
.update_all(reviewed: true, action: STATUS_NO_ACTION)
end
Copy link
Member

Choose a reason for hiding this comment

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

I would just create one method mark_as_reviewed (as before) with the action as parameter. Having them both, but one with _and_deleted is a bit confusing (and also basically code duplication as the action is the only difference).

Comment on lines 14 to 16
def truncated_post_message(post)
if post.respond_to? :message
plain_text = post.message.try(:plain_text_without_markdown).presence || post_page_title(post)
truncate(plain_text, length: 300)
else
I18n.t "notifier.a_post_you_shared"
end
end

Copy link
Member

Choose a reason for hiding this comment

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

This has nothing to do with the notifier, so it shouldn't be added here.

Also, as commented in #8035 (comment), I would just use post_page_title(post) in all cases (which already is truncated), so this isn't even needed at all (also I think this should already be fixed in #8035, so you wouldn't need to do anything).

Copy link
Member Author

@tclaus tclaus Oct 30, 2022

Choose a reason for hiding this comment

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

Removed as suggested

Comment on lines 34 to 25
def truncated_comment_message(comment)
if comment.post.public?
plain_text = comment.message.plain_text_without_markdown
truncate(plain_text, length: 180)
else
I18n.t "notifier.a_limited_post_comment"
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, this isn't needed, and #8035 should already use comment.message.title directly, so you don't need to do anything here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed as suggested


belongs_to :user
belongs_to :post, optional: true
belongs_to :comment, optional: true
belongs_to :item, polymorphic: true
delegate :author, to: :item
Copy link
Member

Choose a reason for hiding this comment

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

Adding the author of the item as author here is very confusing, as report.author then looks like this is the author if the report, please add at least a prefix: true here.

Also, as the item might get deleted, it would probably be smarter to instead use the new column which stores the author of the reported item even if the item itself gets deleted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, if the item is deleted the author is also not every interesting. I'll take the prefix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm 2: There exists already a 'reported_author' in the report model. This is already a better name for what it means. I drop the delegate and use the existing code.

Comment on lines 6 to 22
.reporter.pull-right
!= t("report.reported_label", person: link_to(username, user_profile_path(username)))
.author
%span.reason-label
!= "#{t('report.author')}:"
%span
= report.author.name
.created-at
%span.reason-label
!= "#{t('report.created_at')}:"
%span
= I18n.l(report.created_at, format: :short)
.reason
%span.reason-label
= t("report.reason_label")
%span
= report.text
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be very confusing, what is about what. We have "Created at" which is about the report, and then we have "Author" which is the author of the reported item. I don't really know what's the best solution for this, maybe changing the text to "Reported Author/Person" makes it clearer, or maybe having two columns, one for the stuff that is about the report, and one that is about the reported item? But really, I'm mostly throwing ideas here and don't know what is best.

Also, as this information here is also interesting even if the item was deleted (for example by the author of the reported item), I would restructure the code there to always show the same things. The only difference is that if the item was deleted you can't show the text/link of the reported item anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

report.author is now prefixed with item_author. I also changed the localized text. But you are right, we have the report author, message and creation date as well as the items (post, comments) attributes of the same meaning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I now see, there is already a reported_author in the model. I use this.

Comment on lines 28 to 29
- unless report.originator_diaspora_handle.nil?
= link_to(report.originator_diaspora_handle, "/people/#{report.originator_guid}")
Copy link
Member

Choose a reason for hiding this comment

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

If we don't know the author of the reported item anymore (because it was deleted before the migration), then I would show "Unknown" here instead of just blank.

@tclaus tclaus changed the title Enhance Reports Enhance report overview Oct 30, 2022
tclaus added a commit to tclaus/diaspora that referenced this pull request Oct 30, 2022
With undecided / reviewed and statistics page
Already reviewed reports stay visible
Enhances Spam/user control

fixes diaspora#8239
$("#reportModal").modal("hide");
let textarea = document.getElementById("report-reason-field");
let report = {
item_id: form.dataset.reportId,

Choose a reason for hiding this comment

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

Identifier 'item_id' is not in camel case.

let textarea = document.getElementById("report-reason-field");
let report = {
item_id: form.dataset.reportId,
item_type: form.dataset.reportType,

Choose a reason for hiding this comment

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

Identifier 'item_type' is not in camel case.


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.

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.")
if Report.where(item_id: item_id, item_type: item_type).exists?(user_id: user_id)

Choose a reason for hiding this comment

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

Style/GuardClause: Use a guard clause (return unless Report.where(item_id: item_id, item_type: item_type).exists?(user_id: user_id)) instead of wrapping the code inside a conditional expression.

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.")
if Post.find_by(id: item_id).nil? && Comment.find_by(id: item_id).nil?

Choose a reason for hiding this comment

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

Style/GuardClause: Use a guard clause (return unless Post.find_by(id: item_id).nil? && Comment.find_by(id: item_id).nil?) instead of wrapping the code inside a conditional expression.

@@ -0,0 +1,20 @@
class AddActionFieldsToReports < ActiveRecord::Migration[6.1]

Choose a reason for hiding this comment

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

Style/FrozenStringLiteralComment: Missing frozen string literal comment.

@@ -0,0 +1,20 @@
class AddActionFieldsToReports < ActiveRecord::Migration[6.1]

Choose a reason for hiding this comment

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

Layout/EmptyLinesAroundClassBody: Extra empty line detected at class body beginning.


Report.find_each do |report|
# get reported author id from item before item gets deleted
if report.reported_author.present?

Choose a reason for hiding this comment

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

Style/IfUnlessModifier: Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.

if report.reported_author.present?
report.reported_author_id = report.reported_author.id
end
if report.item.present?

Choose a reason for hiding this comment

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

Style/IfUnlessModifier: Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.

@@ -0,0 +1,16 @@
# frozen_string_literal: true
class AddActionFieldsToReports < ActiveRecord::Migration[6.1]

Choose a reason for hiding this comment

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

Layout/EmptyLineAfterMagicComment: Add an empty line after magic comments.

@tclaus
Copy link
Member Author

tclaus commented Oct 30, 2022

@Flaburgan I rebased this to your #8035 and fixed most of my issues here.

Flaburgan pushed a commit to tclaus/diaspora that referenced this pull request Nov 13, 2023
With undecided / reviewed and statistics page
Already reviewed reports stay visible
Enhances Spam/user control

fixes diaspora#8239
= 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

@@ -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

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

@@ -0,0 +1,43 @@
- @unreviewed_reports.each do |report|
.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?

!= 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.

= t("report.count")
%th
= t("report.reported_author")
- @statistics_by_author.rows.each do |count, author_diaspora_handle, guid|

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

@@ -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.

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.

@@ -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]

@Flaburgan
Copy link
Member

So because the report modal got merged, I rebased this MR, squashed the commits and fixed the front-end pronto problems. There are still a lot of them but I'd like help to resolve those.
I did not even run the code yet, I will obviously try this and polish the UI as we said in the other PR.

With undecided / reviewed and statistics page
Already reviewed reports stay visible
Enhances Spam/user control

fixes diaspora#8239
@Flaburgan
Copy link
Member

@SuperTux88 about the remark you made about the design now that multi lines reports are possible.

It looks fine (the design isn't broken) but we are losing the line break:

image
image

Is there anything else you want me to check on the front-end side?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants