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 #1576

Merged
merged 158 commits into from Jun 16, 2018

Conversation

Projects
None yet
@gravitystorm
Collaborator

gravitystorm commented Jul 5, 2017

Supersedes PR #1268 , see also issue #841 and this diary entry.

This PR builds on the work by @sbagroy986, @zerebubuth, @woodpeck and @grischard as we try to shepherd this GSoC project through review and into production.

I have cleaned up some aspects of this PR, purely to minimise the distractions when comparing diffs and trying to understand how it works. Please avoid nitpicking the code for now - we need to consider bigger questions first. For the avoidance of doubt, this PR not in a fit state for merging!

sbagroy986 and others added some commits May 16, 2015

Fixed error
(cherry picked from commit 99f8a76)

Conflicts:
	app/controllers/issues_controller.rb
Fixed related issues positioning
(cherry picked from commit 156e678)
Added helper + minor UI changes
(cherry picked from commit 3a6550f)
Update en.yml
Replace placeholder language with something closer to finalized language
(cherry picked from commit 1ef7ea1)
Update en-GB.yml
(cherry picked from commit aa06110)
@gravitystorm

This comment has been minimized.

Show comment
Hide comment
@gravitystorm

gravitystorm Mar 28, 2018

Collaborator

Latest batch of changes are now in - just want to highlight that ee1a863 involved another database change.

I don't have anything left on my todo list, so further reviews welcome.

Collaborator

gravitystorm commented Mar 28, 2018

Latest batch of changes are now in - just want to highlight that ee1a863 involved another database change.

I don't have anything left on my todo list, so further reviews welcome.

@gravitystorm gravitystorm changed the title from [WIP] Allow reporting of abusive users to moderators or admins to Allow reporting of abusive users to moderators or admins Apr 3, 2018

@mmd-osm

This comment has been minimized.

Show comment
Hide comment
@mmd-osm

mmd-osm Apr 5, 2018

Contributor

The only thing I found slightly inconvenient during testing was the disconnect between hiding an entry (say a blog post), and setting all related issues to "resolved". Currently, these are two separate steps. Maybe a future enhancement could be to explicitly ask the moderator, if related issues should be automatically set to resolved.

Quite oddly even moderators can't see hidden blog posts anymore. So when checking my list of (open/rejected/resolved) issues, some list entries might point to already hidden posts == dead links (!). This might be inconvenient, when someone wants to check why a particular entry was hidden in the first place, but can't really access it anymore now.

All in all this looks very nice, great work.... let's 🚀

Contributor

mmd-osm commented Apr 5, 2018

The only thing I found slightly inconvenient during testing was the disconnect between hiding an entry (say a blog post), and setting all related issues to "resolved". Currently, these are two separate steps. Maybe a future enhancement could be to explicitly ask the moderator, if related issues should be automatically set to resolved.

Quite oddly even moderators can't see hidden blog posts anymore. So when checking my list of (open/rejected/resolved) issues, some list entries might point to already hidden posts == dead links (!). This might be inconvenient, when someone wants to check why a particular entry was hidden in the first place, but can't really access it anymore now.

All in all this looks very nice, great work.... let's 🚀

@gravitystorm

This comment has been minimized.

Show comment
Hide comment
@gravitystorm

gravitystorm Apr 6, 2018

Collaborator

@mmd-osm thanks for the review, I really appreciate the time you've taken to try this out.

Resolving issues automatically when the item is "dealt with" (e.g. if a moderator hides a diary entry while viewing it, automatically close the issue, if it exists) is something that would be very good for a follow-on pull request.

You make a good point about moderators / admins being able to view hidden posts, I hadn't thought about that. Again, would be a good follow-on enhancement.

Collaborator

gravitystorm commented Apr 6, 2018

@mmd-osm thanks for the review, I really appreciate the time you've taken to try this out.

Resolving issues automatically when the item is "dealt with" (e.g. if a moderator hides a diary entry while viewing it, automatically close the issue, if it exists) is something that would be very good for a follow-on pull request.

You make a good point about moderators / admins being able to view hidden posts, I hadn't thought about that. Again, would be a good follow-on enhancement.

@pnorman

This comment has been minimized.

Show comment
Hide comment
@pnorman

pnorman Apr 6, 2018

Contributor

I sent the following email for discussion among the DWG

The "moderation" feature of the website is nearly complete. As the people with the moderator role and those in charge of data-related disputes, we'll be handling some of the reports. It's important to make sure we test the report handling, as we don't want the feature to roll out and reports not get processed.

I'm not sure what the best way to do this testing is, and am open to suggestions. The GitHub issue tracking the new feature is #1576. We should be focusing on the side the public doesn't see, since it's what we'll be using. In particular, we should look at the interface between getting a report and action by the DWG.

Any suggestions on how to review this?

Contributor

pnorman commented Apr 6, 2018

I sent the following email for discussion among the DWG

The "moderation" feature of the website is nearly complete. As the people with the moderator role and those in charge of data-related disputes, we'll be handling some of the reports. It's important to make sure we test the report handling, as we don't want the feature to roll out and reports not get processed.

I'm not sure what the best way to do this testing is, and am open to suggestions. The GitHub issue tracking the new feature is #1576. We should be focusing on the side the public doesn't see, since it's what we'll be using. In particular, we should look at the interface between getting a report and action by the DWG.

Any suggestions on how to review this?

@SomeoneElseOSM

This comment has been minimized.

Show comment
Hide comment
@SomeoneElseOSM

SomeoneElseOSM Apr 8, 2018

@pnorman is there anything that describes the functionality that we're supposed to see here?

I can login in as "user1", create a diary entry https://moderation.apis.dev.openstreetmap.org/user/user1/diary/5 , report it as a different user (dwg3 FWIW). I can't see anything for that entry in https://moderation.apis.dev.openstreetmap.org/issues - I'm guessing that that's because it's being dealt with by admins not moderators (as diary spam is now) - is that correct?

SomeoneElseOSM commented Apr 8, 2018

@pnorman is there anything that describes the functionality that we're supposed to see here?

I can login in as "user1", create a diary entry https://moderation.apis.dev.openstreetmap.org/user/user1/diary/5 , report it as a different user (dwg3 FWIW). I can't see anything for that entry in https://moderation.apis.dev.openstreetmap.org/issues - I'm guessing that that's because it's being dealt with by admins not moderators (as diary spam is now) - is that correct?

@gravitystorm

This comment has been minimized.

Show comment
Hide comment
@gravitystorm

gravitystorm Apr 11, 2018

Collaborator

I'm guessing that that's because it's being dealt with by admins not moderators (as diary spam is now) - is that correct?

@SomeoneElseOSM yes, that's correct. Currently issues with Notes are assigned to moderators, and everything else to admins. This can of course be changed if required.

Collaborator

gravitystorm commented Apr 11, 2018

I'm guessing that that's because it's being dealt with by admins not moderators (as diary spam is now) - is that correct?

@SomeoneElseOSM yes, that's correct. Currently issues with Notes are assigned to moderators, and everything else to admins. This can of course be changed if required.

I believe everything in this review has been addressed

Show outdated Hide outdated test/system/report_diary_comment_test.rb
fill_in "report_details", :with => "This comment is spam"
click_on "Create Report"
assert page.has_content? "Your report has been registered sucessfully"

This comment has been minimized.

@tilmanb

tilmanb Apr 11, 2018

Contributor

Does the string also need to be translated via I18n.t(...) ?

@tilmanb

tilmanb Apr 11, 2018

Contributor

Does the string also need to be translated via I18n.t(...) ?

This comment has been minimized.

@tomhughes

tomhughes Apr 11, 2018

Member

Not in the tests, no - those always run in English. If it's not translated at the point where it is generated then that would be an issue.

@tomhughes

tomhughes Apr 11, 2018

Member

Not in the tests, no - those always run in English. If it's not translated at the point where it is generated then that would be an issue.

This comment has been minimized.

@tilmanb

tilmanb Apr 11, 2018

Contributor

Ok, thanks. Then just out of curiosity - why do line 14 and 24 of the test use the internationalisation?

@tilmanb

tilmanb Apr 11, 2018

Contributor

Ok, thanks. Then just out of curiosity - why do line 14 and 24 of the test use the internationalisation?

This comment has been minimized.

@tomhughes

tomhughes Apr 11, 2018

Member

I suspect they don't need to, but it won't matter.

@tomhughes

tomhughes Apr 11, 2018

Member

I suspect they don't need to, but it won't matter.

@gravitystorm

This comment has been minimized.

Show comment
Hide comment
@gravitystorm

gravitystorm Apr 27, 2018

Collaborator

@tomhughes please let me know if you'd like any changes to this PR, or if there's anything that I can do to make the review easier.

Collaborator

gravitystorm commented Apr 27, 2018

@tomhughes please let me know if you'd like any changes to this PR, or if there's anything that I can do to make the review easier.

@SomeoneElseOSM

This comment has been minimized.

Show comment
Hide comment
@SomeoneElseOSM

SomeoneElseOSM Jun 6, 2018

I'm currently getting an error 500 trying to log in with "user2" at https://moderation.apis.dev.openstreetmap.org . "user1" oddly seems fine. Is that something that needs to be fixed here or elsewhere?

SomeoneElseOSM commented Jun 6, 2018

I'm currently getting an error 500 trying to log in with "user2" at https://moderation.apis.dev.openstreetmap.org . "user1" oddly seems fine. Is that something that needs to be fixed here or elsewhere?

@tomhughes

This comment has been minimized.

Show comment
Hide comment
@tomhughes

tomhughes Jun 10, 2018

Member

I've been through everything again and pushed some changes, which are now live on the test site.

Unless anybody spots new problems I plan to merge this soon...

Member

tomhughes commented Jun 10, 2018

I've been through everything again and pushed some changes, which are now live on the test site.

Unless anybody spots new problems I plan to merge this soon...

@tomhughes tomhughes merged commit 2767935 into openstreetmap:master Jun 16, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.5%) to 78.583%
Details
@don-vip

This comment has been minimized.

Show comment
Hide comment
@don-vip

don-vip Jun 24, 2018

Is there an API if we want to be able to report abusive users from JOSM?

don-vip commented Jun 24, 2018

Is there an API if we want to be able to report abusive users from JOSM?

@tomhughes

This comment has been minimized.

Show comment
Hide comment
@tomhughes

tomhughes Jun 24, 2018

Member

No.

Member

tomhughes commented Jun 24, 2018

No.

@gravitystorm gravitystorm deleted the gravitystorm:moderation branch Jul 23, 2018

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