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

Allow reporting of abusive users to moderators or admins #1268

Closed
wants to merge 35 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@grischard
Contributor

grischard commented Aug 14, 2016

Working base for merging #841

sbagroy986 added some commits May 16, 2015

Fixed error
(cherry picked from commit 99f8a76)

Conflicts:
	app/controllers/issues_controller.rb
@@ -3,12 +3,20 @@
<h2>
<a class="geolink" href="<%= root_path %>"><span class="icon close"></span></a>
<%= t('browse.changeset.title', :id => @changeset.id) %>
<% if @user and @user.id != @changeset.user.id %>
<%= link_to new_issue_url(reportable_id: @changeset.id, reportable_type: @changeset.class.name, reported_user_id: @changeset.user.id,referer: request.fullpath), :title => t('browse.changeset.report') do %>

This comment has been minimized.

@Zverik

Zverik Aug 15, 2016

Contributor

Missing space. Also, could you add newlines, so the line is not that long?

@@ -3,6 +3,11 @@
<h2>
<a class="geolink" href="<%= root_path %>"><span class="icon close"></span></a>
<%= t "browse.note.#{@note.status}_title", :note_name => @note.id %>
<% if @user and @user.id!=@note.author.id %>

This comment has been minimized.

@Zverik

Zverik Aug 15, 2016

Contributor

Missing spaces here and below.

<p class="deemphasize comment-heading" id="comment<%= diary_comment.id %>"><%= raw(t('diary_entry.diary_comment.comment_from', :link_user => (link_to h(diary_comment.user.display_name), :controller => 'user', :action => 'view', :display_name => diary_comment.user.display_name), :comment_created_at => link_to(l(diary_comment.created_at, :format => :friendly), :anchor => "comment#{diary_comment.id}"))) %></p>
<p class="deemphasize comment-heading" id="comment<%= diary_comment.id %>"><%= raw(t('diary_entry.diary_comment.comment_from', :link_user => (link_to h(diary_comment.user.display_name), :controller => 'user', :action => 'view', :display_name => diary_comment.user.display_name), :comment_created_at => link_to(l(diary_comment.created_at, :format => :friendly), :anchor => "comment#{diary_comment.id}"))) %>
<% if @user and diary_comment.user.id != @user.id %>
<%= link_to new_issue_url(reportable_id: diary_comment.id, reportable_type: diary_comment.class.name, reported_user_id: diary_comment.user.id, referer: request.fullpath), :title => t('diary_entry.diary_comment.report') do %>

This comment has been minimized.

@Zverik

Zverik Aug 15, 2016

Contributor

Tabs messed up indentation.

@@ -6,6 +6,12 @@
<h2><%= link_to h(diary_entry.title), :action => 'view', :display_name => diary_entry.user.display_name, :id => diary_entry.id %></h2>
<% if @user and diary_entry.user.id != @user.id %>
<%= link_to new_issue_url(reportable_id: diary_entry.id, reportable_type: diary_entry.class.name, reported_user_id: diary_entry.user.id,referer: request.fullpath), :title => t('diary_entry.diary_entry.report') do %>

This comment has been minimized.

@Zverik

Zverik Aug 15, 2016

Contributor

Tabs and a space, again.

@@ -31,6 +37,7 @@
<%= link_to t('diary_entry.diary_entry.edit_link'), :action => 'edit', :display_name => diary_entry.user.display_name, :id => diary_entry.id %>
<% end %>

This comment has been minimized.

@Zverik

Zverik Aug 15, 2016

Contributor

Why?

@@ -10,7 +10,11 @@
<a id="comments"></a>
<div class='comments'>
<%= render :partial => 'diary_comment', :collection => @entry.visible_comments %>
<% if @reported_comment %>
<%= render :partial => 'diary_comment', :collection => @reported_comment %>

This comment has been minimized.

@Zverik

Zverik Aug 15, 2016

Contributor

Is there a separate state for displaying only reported comments? Can it be that some users will see only one reported comment instead of a full comment list?

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

This comment has been minimized.

@Zverik

Zverik Aug 15, 2016

Contributor

Too many spaces. Also, this file also has tab-based indentation.

<br/>
<%= submit_tag 'Submit' %>
<% end %>
</div>

This comment has been minimized.

@Zverik

Zverik Aug 15, 2016

Contributor

Missing newline.

@@ -0,0 +1,16 @@
<% reports.each do |report| %>
<% details = report.details.split("--||--") %>

This comment has been minimized.

@Zverik

Zverik Aug 15, 2016

Contributor

An extra space. And tabs. And this delimiter should be made a constant or something.

<td style="width:160px;"><b> <%= sortable("report_count", "Number of Reports") %></b></td>
<td style="width:141px;"><b> <%= sortable("updated_at","Last updated at") %></b></td>
<td style="width:140px;"><b> <%= sortable("updated_by","Last updated by") %></b></td>
<td style="width:203px;"><b> Link to reports </b></td>

This comment has been minimized.

@Zverik

Zverik Aug 15, 2016

Contributor

Non-translated strings here and generally in this file.

<div class="new-report-form">
<% @report_strings_yaml.each do |k,v| %>
<div style="padding-left:5px">

This comment has been minimized.

@Zverik

Zverik Aug 15, 2016

Contributor

These pixels should go into css, replaced here by proper class names.

<div class="report-related-block">
<div class="report-block">
<h3>Reports under this issue:</h3>

This comment has been minimized.

@Zverik

Zverik Aug 15, 2016

Contributor

General formatting, missing spaces and untranslated strings.

@zerebubuth

This comment has been minimized.

Contributor

zerebubuth commented Aug 22, 2016

I've rebased, made rubocop happy and fixed the tests on a branch. I think the code still needs a lot of work before it's ready to PR. Certainly it doesn't seem to have nearly enough tests to cover the amount of new code.

I'm still not entirely sure what's going on in many parts of the code, so will need to dig deeper to understand what else needs to be done, and what extra cases need tests. Any help cleaning it up further, testing it, commenting on it would be very welcome.

@pnorman

This comment has been minimized.

Contributor

pnorman commented Feb 9, 2017

@grischard do you plan to do the rebasing or incorporate the rebasing work Matt did?

@grischard

This comment has been minimized.

Contributor

grischard commented Feb 9, 2017

@pnorman I unfortunately don't have the ruby-fu to do that. Anyone who wants to work on this has my blessing.

@hamlet

This comment has been minimized.

hamlet commented Feb 28, 2017

@pnorman : it seems that the code was mostly written by @sbagroy986 and yourself, is it not ?
I'd really like to see moderation on user diaries, my rails skills are quite rusty, but I could give it a try.

@zerebubuth : the work left on your branch is mostly to write new tests ?
I imagine the workflow would be to install a test instance, run the tests, click everywhere to find unexpected use-cases, write missing tests, documentation, etc...
Please give any relevant advice.

Sorry to jump in like that, but I'd like to help, even if I'm not really in the party.
Regards

@pnorman

This comment has been minimized.

Contributor

pnorman commented Feb 28, 2017

@pnorman : it seems that the code was mostly written by @sbagroy986 and yourself, is it not ?

No - I just rebased it. @zerebubuth's branch is an additional rebase, then more work on top of that.

@zerebubuth : the work left on your branch is mostly to write new tests ?
I imagine the workflow would be to install a test instance, run the tests, click everywhere to find unexpected use-cases, write missing tests, documentation, etc...
Please give any relevant advice.

What it really needs is someone to take ownership of the code, identify what's missing, and finish it off. @zerebubuth identified tests as one thing that's missing, but there's probably other stuff.

Sorry to jump in like that, but I'd like to help, even if I'm not really in the party.

This is open to anyone who wants to take it on.

@hamlet

This comment has been minimized.

hamlet commented Feb 28, 2017

What it really needs is someone to take ownership of the code, identify what's missing, and finish it off. @zerebubuth identified tests as one thing that's missing, but there's probably other stuff.

Well, I can't promise anything, I'm more admin sys than dev, but I'll take a few days off at the end of this week and try my best on this subject.

@zerebubuth : did you use @Zverik remarks, or is this work still to do ?

I will work on @zerebubuth branch, then rebase again if necessary, clean and test as much as I can and propose a new PR.
It should fill my weekend, then I'll wait a bit for feedback, update the PR if necessary.

Sounds right ?

@zerebubuth

This comment has been minimized.

Contributor

zerebubuth commented Feb 28, 2017

@zerebubuth identified tests as one thing that's missing, but there's probably other stuff.

@zerebubuth : did you use @Zverik remarks, or is this work still to do ?

I started off with getting rubocop to pass, which would have addressed most of @Zverik's formatting remarks. I wanted to cover all the existing functionality, but didn't get anywhere near that. IIRC, I only fixed the few tests that there already were.

Writing tests is a good way to start; partly because there weren't enough tests before, and partly because writing tests can be a good way to understand how the code works.

There is a lot of new code, spread over a lot of changes to existing files and I found it hard to navigate all of it. I think there is a good argument for re-examining the goals of this code and perhaps starting from scratch, or at least laying down some broad design ideas and cleaning up the code so that it's more consistent, readable and isolated from existing code.

When adding this sort of functionality, I think the "Rails-y" way of doing it would be to add an acts_as plugin, called something like acts_as_flaggable or acts_as_reportable, and use that to keep the code for reporting problems and moderating them separate from the existing code. This would be more readable and allow the testing of that functionality separated from the context it's used in.

@hamlet

This comment has been minimized.

hamlet commented Feb 28, 2017

Well, @tomhughes said on osm-talk that:

Look most of the code is already written, somebody just needs to finish it off!

That's why I thought I could be useful.
But if you say that it should be rewritten from scratch, I'm afraid it'll be way over my skills and availability... And it's certainly not the project I would start with.

@zerebubuth : Please close this PR if you think it's useless.

@pnorman, @Zverik : do you confirm that this PR is not good enough to work on ?

Regards

@zerebubuth

This comment has been minimized.

Contributor

zerebubuth commented Feb 28, 2017

Well, @tomhughes said on osm-talk that:

Look most of the code is already written, somebody just needs to finish it off!

He's the maintainer, not me! So I'd take his word over mine. 😄

I don't think this PR is useless. I was saying that I, personally, felt that the code needed a lot of work - either to finish off what's there or to start again. YMMV, and @tomhughes' opinion (which matters more than mine) may be very different from mine!

@hamlet

This comment has been minimized.

hamlet commented Feb 28, 2017

All right, I'll wait for @tomhughes advice on the matter.

I'm trying to set up the development environment, then this weekend I hope I'll be able to do some real work on this, we'll see if this is useful.

@tomhughes

This comment has been minimized.

Member

tomhughes commented Feb 28, 2017

@hamlet I haven't done any detailed code review - we normally try and make sure we're happy with functionality and UI etc before doing that.

I believe I did look at it at a high level and offer advice during the original GSOC project.

I would think the first thing to do is to get it rebased/merged with current master and resolve and conflicts and we can update the test instance at https://moderation.apis.dev.openstreetmap.org/ and make sure we're happy with the UI and can then think about a code review.

@pnorman

This comment has been minimized.

Contributor

pnorman commented Mar 1, 2017

Given that someone new will need to take ownership of a new PR, there's a more up to date branch from @zerebubuth, and this pr points at "unknown repository" (wtf?) I recommend closing this PR.

Someone who is willing to take this on can then open up a new PR against a working branch.

@grischard

This comment has been minimized.

Contributor

grischard commented Mar 1, 2017

Yeah, no idea what "unknown repository" means!

I'll definitely close this as soon as anyone opens a new, better PR, but it's a good central place for discussion for now.

@gravitystorm

This comment has been minimized.

Collaborator

gravitystorm commented Jul 5, 2017

Superseded by #1576

@bhousel bhousel referenced this pull request May 18, 2018

Closed

Moderation queue #8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment