-
-
Notifications
You must be signed in to change notification settings - Fork 285
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
MBS-13770: Allow admins to auto-approve and auto-reject any edit #3392
base: master
Are you sure you want to change the base?
Conversation
0f0fc65
to
7a19e7c
Compare
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.
The code looks great and I tested it locally as both an admin and non-admin with testing features enabled (making sure the non-admin couldn't access the new admin endpoints, and that neither could access the test endpoints if DB_STAGING_TESTING_FEATURES
was disabled).
One issue is that if we put this on beta, the new vote types will cause the edits that were voted on to ISE in production. Even though it will only affect spam edits, I think it would be a good idea to factor out the changes necessary to support and display the new vote types into a separate commit which we could apply to production, then keep all the changes needed for actually entering the votes in the next commit.
Seems sensible! Made the change, and made sure that my existing test edits with the votes that ISEd on master work with the first commit only. |
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.
The first commit should also include the validation, otherwise you can just edit the HTML to submit those votes on production. (They won't actually do anything, but.)
<tr> | ||
<th /> | ||
<th colSpan="3"> | ||
{addColonText(lp_statistics('Admin approval', 'vote'))} |
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.
Can't these use the strings from mb_server instead?
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.
tbh I'm not sure? I assume if it could we would do it already for the other votes...
This adds two special vote types ("Admin approval" and "Admin rejection") that will be used in a subsequent commit for a feature that allows admins to immediately accept or reject any edit. These are added first in a separate commit so that it can be released on production while the main part of the new feature is released on beta, avoiding any temporary production ISEs when encountering the new vote types. "Admin approval" is treated as an approval for icons and headers, while "Admin rejection" is treated as a No vote. The new vote types are searchable in edit searches, their stats can be seen on admin profiles, etc.
There's plenty of cases where a removal of a spam release for example sits open for days, and closing it earlier requires asking for more eyes on it which does seem like the opposite of what you want to do with spam. This allows account admins to use the same accept/reject mechanism we already used in test servers to immediately accept or reject any open edit (including their own). When doing so, a vote is added to the edit (of the newly added types "Admin approval" or "Admin reject", added in a previous commit so they can be visible in production), so it is as clear as possible why the edit applied or failed. I kept the mechanisms separate, still keeping a /test/ endpoint for the existing testing mechanism while adding a new /admin/ one for the new feature. This is mostly for testing purposes (it's very hard to test for the feature otherwise since the test setup has the buttons on by default because of its DB_STAGING_TESTING_FEATURES setting) but also to avoid setting the new votes every time we use the feature in testing, since that might not be desirable.
We seemingly were never hitting this, because as soon as we did during my testing, it died horribly. Unsurprisingly, since it was trying to access $_ outside the any call. Unsure how useful this is given we never hit it, but for now, this at least actually should do what the old code tried to.
Implement MBS-13770
Problem
Admins should have a way to deal with edits, mostly for spam issues. There's plenty of cases where a removal of a spam release for example sits open for days, and closing it earlier requires asking for more eyes on it which does seem like the opposite of what you want to do with spam.
Solution
This allows account admins to use the same accept/reject mechanism we already used in test servers to immediately accept or reject any open edit (including their own).
To make what has happened clear, a special vote ("Admin approval" or "Admin rejection") is added to the edit when closing it. "Admin approval" is treated as an approval for icons and headers, while "Admin rejection" is treated as a No vote.
The new vote types are searchable in edit searches, their stats can be seen on admin profiles, etc.
Testing
Manually, turning off
DB_STAGING_TESTING_FEATURES
and making sure the buttons still show for admins, work properly, leave the intended votes, etc. Also tested edit searches, and that the right numbers are shown on editor profiles.Added some tests as well to make sure that the endpoint cannot be hit by non-admins, but can be hit by admins.