-
-
Notifications
You must be signed in to change notification settings - Fork 68
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
Moderator Tool Improvements #767
base: develop
Are you sure you want to change the base?
Conversation
c561fbc
to
e61ba6f
Compare
app/views/users/mod_delete.html.erb
Outdated
<div class="widget--body"> | ||
<h4>Destroy Acccount</h4> | ||
<p>Destroy Accounts of blatant spammers and trolls.</p> | ||
|
||
<%= link_to 'Destroy Account', destroy_user_path(@user.id), remote: true, | ||
method: :delete, class: 'js-destroy-user button is-danger is-filled' %> | ||
</div> |
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.
discuss: We should review whether we actually want this here. Destroying accounts completely is a good way to break the database, and now that we have soft-deletes it's probably not needed.
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 propose to remove "destroy" for the reasons Art said.
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.
@trichoplax if the destroy option is still there, could you remove it from the UI at least? Thanks.
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.
It appears that the destroy button is not new, just moved from a different place in the existing user interface. There are 4 similarly named buttons that appear together in the existing user interface, but have been separated in the new user interface.
Existing delete and destroy buttons
New Delete or Destroy Account section
New Hungry Codidactyl (Delete, Feed to STAT) section
Some questions:
- Which of the 4 buttons are to be removed and which are to remain?
- I'm assuming we remove "Destroy Account" (currently called "Destroy user") and keep "Delete community profile", "Delete user network-wide", and "Feed to STAT (180 days)".
- Should the remaining buttons be relocated?
- I'm assuming "Delete community profile" will stay in a section of its own, since the other 2 buttons are only available to global moderators (the section could be renamed from "Delete or Destroy Account" to just "Delete Account")
- The section name "Hungry Codidactyl (Delete, Feed to STAT)" seems incompatible with non-Codidact instances. It could instead be "Delete Network Account", and just contain 1 button, "Delete user network-wide". The button "Feed to STAT (180 days)" seems more similar to a suspension than a deletion, so perhaps it could be moved to the section "Network-wide Suspension".
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.
- Yes, remove the "Destroy Account" button and keep the other three.
- I defer to @luap42 but I think this is right.
- I defer to luap42 again but yeah, that sounds better for a public platform rather than our own instance. I believe "feed to STAT" is delete+ spam-block.
Would it be practical to split this pull request into smaller changes to allow some to be deployed and get real world feedback? I'm wondering because of the issues that mention this pull request. Do they depend on the whole change or just smaller parts of it that might not need to wait for the rest? |
@ArtOfCode- re the previous comment: do you think it would be easier to subdivide this PR or finish it? (Asking because you reviewed it in the past, so you have better insights than most other people.) |
Probably easier to finish it - would take unnecessary work to split it up. Shouldn't need much more work, though - addressing comments and requests and fixing the conflicts. @trichoplax if you find yourself looking for any more intro to Ruby, this might be doable for you. |
@ArtOfCode- Thanks for the suggestion. I'm going to start by trying to rebase |
New dependencies require ruby 2.7 and rails 6.0 as minimum versions.
It seems that the update to rubocop/rails means we get some new rules. The new rules seem to have changed their minds on how some things should look or be done. This applies the new rules.
Also swap arguments around such that the error message upon failure lists the right value as expected.
Also add a note in the INSTALLATION about the ruby versions that are supported/not supported.
Taeir's explanation: NOTE: I use select here to allow rails to merge the two operations into one query and avoid loading the full ThreadFollower objects
Some improvements to the moderator tools.
(Documentation comes soon)