Feature: Improve remote report display #7062

ThisIsMissEm · 2018-04-07T08:09:18Z

Resolves #6994

ThisIsMissEm · 2018-04-07T08:21:21Z

Have just fixed the failing test — didn't realise there wasn't an admin account by default for tests.

ClearlyClaire · 2018-04-07T12:09:32Z

Hmm, this is an improvement, but it still doesn't make clear that the person listed is not necessarily the person who has actually filed the report.

ThisIsMissEm · 2018-04-07T22:48:37Z

@ThibG welcome to suggestions, at least now it's guaranteed to be an admin on the remote server, rather than any random first unsuspended user.

Gargron · 2018-04-08T00:15:07Z

app/services/report_service.rb

-  def some_local_account
-    @some_local_account ||= Account.local.where(suspended: false).first
+  def local_admin_account
+    @local_admin_account ||= User.admins.first.account


Idea: Why not use "contact account" when it's set? Also idea: Abstract it out into, for example, Account.representative... 🤔

As in Settings.site_contact_username ?

I didn't know it existed, but we certainly could use that.

@ThibG this change should make the remote user the admin / contact person

@Gargron I started doing this, and the I realised: there's absolutely no validation on the Setting.site_contact_username value — you can set it to pretty much anything.

Interestingly the about page doesn't show the correct user is site_contact_username is a remote user, despite this being accepted in settings.

So, we'd end up with probably this:

def representative return nil unless local? site_contact = Account.find_local(Setting.site_contact_username) if Setting.site_contact_username.present? site_contact || User.admins.first.account end

If we're to follow the lead from InstancePresenter, then:

def representative return nil unless local? site_contact = Account.find_local(Setting.site_contact_username) site_contact || User.admins.first.account end

Which relies on the fact that username can't be an empty string

ClearlyClaire · 2018-04-08T09:35:06Z

@ThisIsMissEm maybe “Report made on remote instance” instead of “Report by remote account”, as the latter suggests that the listed account is the person who made the report?
In a similar fashion, “Reports assigned to this account” rather than “Reports created by this account” in the case of remote toots… but this may also be misleading for other reasons…

ThisIsMissEm · 2018-04-08T10:52:22Z

@ThibG how about "reports created from this instance" / "reports about from this instance" and "Report made on remote instance (admin)"

i.e., the idea is that the user below is the person to contact on the remote instance about the report created on their instance.

ClearlyClaire · 2018-04-08T10:59:58Z

“Reports created from this instance” / “Reports about this instance” and “Report made on a remote instance (admin)” sounds good to me. With the small caveat that with the “representative” user selection not quite addressed yet, remote reports might be split across a few different users…

ThisIsMissEm · 2018-04-08T11:23:13Z

@ThibG

With the small caveat that with the “representative” user selection not quite addressed yet, remote reports might be split across a few different users…

See the comment about the change of the representative user, essentially we'll change this to be the instance contact user or the first administrator found. So perhaps just: "Report made on a remote instance (representative)"

ClearlyClaire · 2018-04-08T12:04:07Z

@ThisIsMissEm yeah, I've seen this change and I'm in favor of it. But not everyone has applied this change yet, and the “representative” may change over time, hence the caveat that the “Reports created from this instance” / “Reports about this instance” might be misleading if that points to reports made by/about the representative account and they change over time.

ThisIsMissEm · 2018-04-08T13:16:44Z

There doesn't seem to be a good way to lay this out..

It'd be perhaps better if the report stats showed next to the account, rather than beneath the account

ThisIsMissEm · 2018-04-08T13:20:58Z

@ThibG what do you think of the changes in aef560b ?

ClearlyClaire · 2018-04-08T13:33:40Z

app/models/report.rb

+
+  def targeting_domain
+    Report.where(target_account: Account.by_domain(account.domain))
+  end


I am worried about the performances here, but I'm no Rails expert, so I'll let someone more knowledgeable comment.

Yeah, it'd probably be better to include domain on the Report object, such that we could just do Report.where(target_domain: domain).count and Report.where(domain: domain).count

But this would require database changes, and be backwards incompatible.

This becomes a SELECT ... WHERE id IN (SELECT ... ) query which is fine, it's not like all accounts are loaded into Rails before being passed into the where clause

ClearlyClaire · 2018-04-08T13:34:38Z

app/views/admin/reports/show.html.haml

+              %td= link_to pluralize(@report.from_domain.count, t('admin.accounts.show.report')), admin_reports_path(account_id: @report.account.id)
+            %tr
+              %td= t('admin.reports.targeting_remote_instance')
+              %td= link_to pluralize(@report.targeting_domain.count, t('admin.accounts.show.report')), admin_reports_path(target_account_id: @report.account.id)


The links still point to reports attributed to / against the particular account and not reports against / reported by users on the instance.

Good catch.. this would require changes to ReportFilter to support domains, so you'd end up with a url like /admin/reports?domain=switter.at or /admin/reports?targeted_domain=switter.at

I'm not sure if that's a good idea.

@Gargron would there be any support for a "domains" model which was just the string of the domain + an ID which we could use for joins? It feels a little odd to use the domain in the filter URL — alternatively, I could do the hash of the domain, as prevent the raw domain being in the URL.

Gargron · 2018-04-16T18:56:31Z

@ThisIsMissEm To be honest I would prefer if this PR contained the least possible amount of changes, aka just the display and the some_local_account change. I don't think there's a pressing need for new filtering functionality

ThisIsMissEm · 2018-04-16T20:01:47Z

@Gargron I would agree — I can just remove the links here, as that'd probably make most sense.

ThisIsMissEm · 2018-04-16T20:28:21Z

Having an issue running the tests, something about rPam2 missing, yet bundler says everything is installed correctly?

rspec spec/models/account_spec.rb
                                                                                                                                                                
An error occurred while loading ./spec/models/account_spec.rb.
Failure/Error: require File.expand_path('../../config/environment', __FILE__)

NameError:
  uninitialized constant Rpam2
# ./config/environments/test.rb:65:in `<top (required)>'
# ./config/environment.rb:5:in `<top (required)>'
# ./spec/rails_helper.rb:2:in `<top (required)>'
# ./spec/models/account_spec.rb:1:in `<top (required)>'
# ------------------
# --- Caused by: ---
# NameError:
#   uninitialized constant Rpam2
#   ./config/environments/test.rb:65:in `<top (required)>'
No examples found.                                                                                                                                              
 0/0 |================================================================= 100 ==================================================================>| Time: 00:00:00 
[rspec-sidekiq] WARNING! Sidekiq will *NOT* process jobs in this environment. See https://github.com/philostler/rspec-sidekiq/wiki/FAQ-&-Troubleshooting

Finished in 0.00099 seconds (files took 3.04 seconds to load)
0 examples, 0 failures, 1 error occurred outside of examples
Coverage report generated for RSpec to /mastodon/coverage. 65 / 16042 LOC (0.41%) covered.

Gargron · 2018-04-17T11:54:06Z

I think I want to make the PAM test suite optional because it's really annoying, but anyway to make it work currently you need to bundle install --with pam_authentication

…tance

ThisIsMissEm · 2018-04-18T01:34:58Z

@Gargron okay, I managed to get the tests running, but I still have like, 30 failures locally. I don't think they're related to anything I've done, a lot are about this new rails 6 issue then there are issues with the URLs mismatching.

I use .dotenv locally whenever I switch directories to load up the correct .env configuration, so I'm guessing that's something to do with it.

ThisIsMissEm · 2018-04-18T01:35:52Z

Here's the updated UI / Display:

ThisIsMissEm · 2018-04-23T19:32:31Z

Closing this as it now is obsolete / conflicts heavily due to #7188

Gargron reviewed Apr 8, 2018

View reviewed changes

ThisIsMissEm force-pushed the feature/improve-remote-report-display branch from 2bb514d to aef560b Compare April 8, 2018 13:20

ClearlyClaire reviewed Apr 8, 2018

View reviewed changes

ThisIsMissEm added 2 commits April 18, 2018 03:30

Implement Account#representative for usage by report federation

af94fc6

Improve Reports UI to show clearly when a report is from a remote ins…

ce06236

…tance

ThisIsMissEm force-pushed the feature/improve-remote-report-display branch from aef560b to ce06236 Compare April 18, 2018 01:32

ThisIsMissEm closed this Apr 23, 2018

ThisIsMissEm deleted the feature/improve-remote-report-display branch April 23, 2018 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Improve remote report display #7062

Feature: Improve remote report display #7062

ThisIsMissEm commented Apr 7, 2018 •

edited

Loading

ThisIsMissEm commented Apr 7, 2018

ClearlyClaire commented Apr 7, 2018

ThisIsMissEm commented Apr 7, 2018

Gargron Apr 8, 2018

ThisIsMissEm Apr 8, 2018

ThisIsMissEm Apr 8, 2018

ThisIsMissEm Apr 8, 2018

ThisIsMissEm Apr 8, 2018

ClearlyClaire commented Apr 8, 2018

ThisIsMissEm commented Apr 8, 2018

ClearlyClaire commented Apr 8, 2018

ThisIsMissEm commented Apr 8, 2018

ClearlyClaire commented Apr 8, 2018

ThisIsMissEm commented Apr 8, 2018

ThisIsMissEm commented Apr 8, 2018

ClearlyClaire Apr 8, 2018

ThisIsMissEm Apr 8, 2018

Gargron Apr 9, 2018

ClearlyClaire Apr 8, 2018

ThisIsMissEm Apr 8, 2018

ThisIsMissEm Apr 16, 2018

Gargron commented Apr 16, 2018

ThisIsMissEm commented Apr 16, 2018

ThisIsMissEm commented Apr 16, 2018

Gargron commented Apr 17, 2018

ThisIsMissEm commented Apr 18, 2018 •

edited

Loading

ThisIsMissEm commented Apr 18, 2018

ThisIsMissEm commented Apr 23, 2018

Feature: Improve remote report display #7062

Feature: Improve remote report display #7062

Conversation

ThisIsMissEm commented Apr 7, 2018 • edited Loading

ThisIsMissEm commented Apr 7, 2018

ClearlyClaire commented Apr 7, 2018

ThisIsMissEm commented Apr 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ClearlyClaire commented Apr 8, 2018

ThisIsMissEm commented Apr 8, 2018

ClearlyClaire commented Apr 8, 2018

ThisIsMissEm commented Apr 8, 2018

ClearlyClaire commented Apr 8, 2018

ThisIsMissEm commented Apr 8, 2018

ThisIsMissEm commented Apr 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gargron commented Apr 16, 2018

ThisIsMissEm commented Apr 16, 2018

ThisIsMissEm commented Apr 16, 2018

Gargron commented Apr 17, 2018

ThisIsMissEm commented Apr 18, 2018 • edited Loading

ThisIsMissEm commented Apr 18, 2018

ThisIsMissEm commented Apr 23, 2018

ThisIsMissEm commented Apr 7, 2018 •

edited

Loading

ThisIsMissEm commented Apr 18, 2018 •

edited

Loading