From 7a19e7c01b4351ccd0cf1cd852d49a10e61e0bf0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Tamargo?= Date: Thu, 24 Oct 2024 19:55:30 +0200 Subject: [PATCH] MBS-13770: Allow admins to auto-approve and auto-reject any edit 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). I kept them separate though mostly for testing purposes (otherwise it's very hard to test for the right buttons since the test setup has the DB_STAGING_TESTING_FEATURES on by default) but also to avoid setting the new votes (see below) every time we use the feature in testing, since that might not be desirable. 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. --- lib/MusicBrainz/Server/Constants.pm | 10 +- lib/MusicBrainz/Server/Controller/Admin.pm | 65 +++++- .../Server/Controller/User/Edits.pm | 2 + lib/MusicBrainz/Server/Data/Edit.pm | 9 +- lib/MusicBrainz/Server/Data/Statistics.pm | 22 +- lib/MusicBrainz/Server/Data/Vote.pm | 21 +- lib/MusicBrainz/Server/Entity/Vote.pm | 2 + lib/MusicBrainz/Server/Types.pm | 2 +- root/constants.js | 2 + root/edit/EditIndex.js | 62 ++++-- root/edit/components/EditHeader.js | 5 +- root/edit/components/EditNote.js | 7 +- root/edit/components/EditSummary.js | 26 ++- root/edit/search_macros.tt | 14 +- .../scripts/edit/utility/getVoteName.js | 6 + root/static/styles/edit.less | 8 +- root/statistics/Index.js | 16 ++ root/statistics/stats.js | 15 ++ root/types/vote.js | 4 +- root/user/PrivilegedUsers.js | 3 +- .../Controller/Admin/AcceptRejectEdits.pm | 188 ++++++++++++++++++ .../Server/Controller/Edit/Show.pm | 107 +++++++++- .../Controller/UnconfirmedEmailAddresses.pm | 2 + t/sql/vote.sql | 4 +- 24 files changed, 549 insertions(+), 53 deletions(-) create mode 100644 t/lib/t/MusicBrainz/Server/Controller/Admin/AcceptRejectEdits.pm diff --git a/lib/MusicBrainz/Server/Constants.pm b/lib/MusicBrainz/Server/Constants.pm index 1c71142e40d..82c3d7a9fb9 100644 --- a/lib/MusicBrainz/Server/Constants.pm +++ b/lib/MusicBrainz/Server/Constants.pm @@ -341,10 +341,12 @@ Readonly our $SCRIPT_FREQUENCY_UNCOMMON => 2; Readonly our $SCRIPT_FREQUENCY_OTHER => 3; Readonly our $SCRIPT_FREQUENCY_FREQUENT => 4; -Readonly our $VOTE_ABSTAIN => -1; -Readonly our $VOTE_NO => 0; -Readonly our $VOTE_YES => 1; -Readonly our $VOTE_APPROVE => 2; +Readonly our $VOTE_ABSTAIN => -1; +Readonly our $VOTE_NO => 0; +Readonly our $VOTE_YES => 1; +Readonly our $VOTE_APPROVE => 2; +Readonly our $VOTE_ADMIN_APPROVE => 3; +Readonly our $VOTE_ADMIN_REJECT => 4; Readonly our $STATUS_OPEN => 1; Readonly our $STATUS_APPLIED => 2; diff --git a/lib/MusicBrainz/Server/Controller/Admin.pm b/lib/MusicBrainz/Server/Controller/Admin.pm index 087e78e0247..0bcbd4ea141 100644 --- a/lib/MusicBrainz/Server/Controller/Admin.pm +++ b/lib/MusicBrainz/Server/Controller/Admin.pm @@ -7,7 +7,13 @@ use Try::Tiny; extends 'MusicBrainz::Server::Controller'; -use MusicBrainz::Server::Constants qw( :privileges ); +use DBDefs; + +use MusicBrainz::Server::Constants qw( + :privileges + $VOTE_ADMIN_APPROVE + $VOTE_ADMIN_REJECT +); use MusicBrainz::Server::ControllerUtils::JSON qw( serialize_pager ); use MusicBrainz::Server::Data::Utils qw( boolean_to_json ); @@ -340,6 +346,63 @@ sub unlock_username : Path('/admin/locked-usernames/unlock') Args(1) RequireAuth ); } +sub accept_edit : Path('/admin/accept-edit') Args(1) RequireAuth(account_admin) +{ + my ($self, $c, $edit_id) = @_; + + my $edit = $c->model('Edit')->get_by_id($edit_id) + or $c->detach('/error_404'); + + _accept_edit($c, $edit) if $edit->is_open; + $c->response->redirect($c->uri_for_action('/edit/show', [ $edit->id ])); +} + +sub reject_edit : Path('/admin/reject-edit') Args(1) RequireAuth(account_admin) +{ + my ($self, $c, $edit_id) = @_; + + my $edit = $c->model('Edit')->get_by_id($edit_id) + or $c->detach('/error_404'); + + _reject_edit($c, $edit) if $edit->is_open; + $c->response->redirect($c->uri_for_action('/edit/show', [ $edit->id ])); +} + +sub _accept_edit +{ + my ($c, $edit) = @_; + + my $sql = $c->model('MB')->context->sql; + + Sql::run_in_transaction( sub { + $c->model('Vote')->enter_votes( + $c->user, + [{ + vote => $VOTE_ADMIN_APPROVE, + edit_id => $edit->id, + }], + ); + $c->model('Edit')->accept($edit); + }, $sql ); +} + +sub _reject_edit +{ + my ($c, $edit) = @_; + + my $sql = $c->model('MB')->context->sql; + Sql::run_in_transaction( sub { + $c->model('Vote')->enter_votes( + $c->user, + [{ + vote => $VOTE_ADMIN_REJECT, + edit_id => $edit->id, + }], + ); + $c->model('Edit')->reject($edit); + }, $sql ); +} + 1; =head1 COPYRIGHT AND LICENSE diff --git a/lib/MusicBrainz/Server/Controller/User/Edits.pm b/lib/MusicBrainz/Server/Controller/User/Edits.pm index 9293199d9bf..6acbe75a0e8 100644 --- a/lib/MusicBrainz/Server/Controller/User/Edits.pm +++ b/lib/MusicBrainz/Server/Controller/User/Edits.pm @@ -350,6 +350,8 @@ sub votes : Chained('/user/load') PathPart('votes') RequireAuth HiddenOnMirrors 'conditions.0.args.1' => $VOTE_NO, 'conditions.0.args.2' => $VOTE_YES, 'conditions.0.args.3' => $VOTE_APPROVE, + 'conditions.0.args.4' => $VOTE_ADMIN_APPROVE, + 'conditions.0.args.5' => $VOTE_ADMIN_REJECT, }; $c->stash( diff --git a/lib/MusicBrainz/Server/Data/Edit.pm b/lib/MusicBrainz/Server/Data/Edit.pm index 74e1042b1b1..be195955351 100644 --- a/lib/MusicBrainz/Server/Data/Edit.pm +++ b/lib/MusicBrainz/Server/Data/Edit.pm @@ -932,8 +932,13 @@ sub insert_votes_and_notes { my @votes = @{ $data{votes} || [] }; my @notes = @{ $data{notes} || [] }; - # Filter out approvals, they can only be entered via the approve method - @votes = grep { $_->{vote} != $VOTE_APPROVE } @votes; + # Filter out approvals (they can only be entered via the approve method) + # and admin votes (they too have their own separate mechanisms) + @votes = grep { + $_->{vote} != $VOTE_APPROVE && + $_->{vote} != $VOTE_ADMIN_APPROVE && + $_->{vote} != $VOTE_ADMIN_REJECT + } @votes; Sql::run_in_transaction(sub { $self->c->model('Vote')->enter_votes($editor, \@votes); diff --git a/lib/MusicBrainz/Server/Data/Statistics.pm b/lib/MusicBrainz/Server/Data/Statistics.pm index 3fc9384fa4e..38afb9d93f1 100644 --- a/lib/MusicBrainz/Server/Data/Statistics.pm +++ b/lib/MusicBrainz/Server/Data/Statistics.pm @@ -1553,10 +1553,12 @@ my %stats = ( my %dist = map { @$_ } @$data; +{ - 'count.vote.yes' => $dist{$VOTE_YES} || 0, - 'count.vote.no' => $dist{$VOTE_NO} || 0, - 'count.vote.abstain' => $dist{$VOTE_ABSTAIN} || 0, - 'count.vote.approve' => $dist{$VOTE_APPROVE} || 0, + 'count.vote.yes' => $dist{$VOTE_YES} || 0, + 'count.vote.no' => $dist{$VOTE_NO} || 0, + 'count.vote.abstain' => $dist{$VOTE_ABSTAIN} || 0, + 'count.vote.approve' => $dist{$VOTE_APPROVE} || 0, + 'count.vote.admin_approve' => $dist{$VOTE_ADMIN_APPROVE} || 0, + 'count.vote.admin_reject' => $dist{$VOTE_ADMIN_REJECT} || 0, }; }, NONREPLICATED => 1, @@ -1579,6 +1581,18 @@ my %stats = ( PREREQ_ONLY => 1, NONREPLICATED => 1, }, + 'count.vote.admin_approve' => { + DESC => 'Count of admin approvals', + PREREQ => [qw[ count.vote.yes ]], + PREREQ_ONLY => 1, + NONREPLICATED => 1, + }, + 'count.vote.admin_reject' => { + DESC => 'Count of admin rejections', + PREREQ => [qw[ count.vote.yes ]], + PREREQ_ONLY => 1, + NONREPLICATED => 1, + }, 'count.vote.perday' => { DESC => 'Count of votes per day', SQL => q{SELECT count(id) FROM vote diff --git a/lib/MusicBrainz/Server/Data/Vote.pm b/lib/MusicBrainz/Server/Data/Vote.pm index 6114af29647..0018f73bc28 100644 --- a/lib/MusicBrainz/Server/Data/Vote.pm +++ b/lib/MusicBrainz/Server/Data/Vote.pm @@ -73,9 +73,20 @@ sub enter_votes # not sufficient to filter the vote because the actual approval is happening elsewhere confess 'Unauthorized editor ' . $editor->id . ' tried to approve edit #' . $_->{edit_id}; } + if (any { $_->{vote} == $VOTE_ADMIN_APPROVE && !$editor->is_account_admin } @votes) { + # not sufficient to filter the vote because the actual approval is happening elsewhere + confess 'Unauthorized editor ' . $editor->id . ' tried to admin-approve edit #' . $_->{edit_id}; + } + if (any { $_->{vote} == $VOTE_ADMIN_REJECT && !$editor->is_account_admin } @votes) { + # not sufficient to filter the vote because the actual rejection is happening elsewhere + confess 'Unauthorized editor ' . $editor->id . ' tried to admin-reject edit #' . $_->{edit_id}; + } unless ($opts{override_privs}) { @votes = grep { - $_->{vote} == $VOTE_APPROVE || $edits->{ $_->{edit_id} }->editor_may_vote_on_edit($editor) + $_->{vote} == $VOTE_APPROVE || + $_->{vote} == $VOTE_ADMIN_APPROVE || + $_->{vote} == $VOTE_ADMIN_REJECT || + $edits->{ $_->{edit_id} }->editor_may_vote_on_edit($editor) } @votes; } @@ -169,6 +180,12 @@ sub editor_statistics ? $self->summarize_votes($VOTE_APPROVE, $all_votes, $recent_votes) : (), + # Show admin votes only if editor is an admin + $editor->is_account_admin ? ( + $self->summarize_votes($VOTE_ADMIN_APPROVE, $all_votes, $recent_votes), + $self->summarize_votes($VOTE_ADMIN_REJECT, $all_votes, $recent_votes), + ) : (), + # Add totals { name => l('Total'), @@ -190,6 +207,8 @@ sub summarize_votes $VOTE_NO => lp('No', 'vote'), $VOTE_YES => lp('Yes', 'vote'), $VOTE_APPROVE => lp('Approve', 'vote'), + $VOTE_ADMIN_APPROVE => lp('Admin approval', 'vote'), + $VOTE_ADMIN_REJECT => lp('Admin rejection', 'vote'), ); return ( diff --git a/lib/MusicBrainz/Server/Entity/Vote.pm b/lib/MusicBrainz/Server/Entity/Vote.pm index 333f2a5e559..b93bbd64fd5 100644 --- a/lib/MusicBrainz/Server/Entity/Vote.pm +++ b/lib/MusicBrainz/Server/Entity/Vote.pm @@ -51,6 +51,8 @@ sub vote_name $VOTE_NO => 'No', $VOTE_YES => 'Yes', $VOTE_APPROVE => 'Approve', + $VOTE_ADMIN_APPROVE => 'Admin approval', + $VOTE_ADMIN_REJECT => 'Admin rejection', ); return $names{$self->vote}; } diff --git a/lib/MusicBrainz/Server/Types.pm b/lib/MusicBrainz/Server/Types.pm index 776ffd81ffe..0fbf4eaecfb 100644 --- a/lib/MusicBrainz/Server/Types.pm +++ b/lib/MusicBrainz/Server/Types.pm @@ -48,7 +48,7 @@ subtype AutoEditorElectionStatus, subtype VoteOption, as Int, - where { $_ >= $VOTE_ABSTAIN && $_ <= $VOTE_APPROVE }; + where { $_ >= $VOTE_ABSTAIN && $_ <= $VOTE_ADMIN_REJECT }; subtype EditStatus, as Int, diff --git a/root/constants.js b/root/constants.js index 038762b1704..93b60353f1a 100644 --- a/root/constants.js +++ b/root/constants.js @@ -49,6 +49,8 @@ export const EDIT_VOTE_ABSTAIN = -1; export const EDIT_VOTE_NO = 0; export const EDIT_VOTE_YES = 1; export const EDIT_VOTE_APPROVE = 2; +export const EDIT_VOTE_ADMIN_APPROVE = 3; +export const EDIT_VOTE_ADMIN_REJECT = 4; export const QUALITY_UNKNOWN = -1; export const QUALITY_UNKNOWN_MAPPED = 1; diff --git a/root/edit/EditIndex.js b/root/edit/EditIndex.js index d4d4e89fc49..9c61a1506ed 100644 --- a/root/edit/EditIndex.js +++ b/root/edit/EditIndex.js @@ -16,6 +16,8 @@ import EditLink from '../static/scripts/common/components/EditLink.js'; import EditorLink from '../static/scripts/common/components/EditorLink.js'; import DBDefs from '../static/scripts/common/DBDefs.mjs'; import linkedEntities from '../static/scripts/common/linkedEntities.mjs'; +import {isAccountAdmin} + from '../static/scripts/common/utility/privileges.js'; import FormSubmit from '../static/scripts/edit/components/FormSubmit.js'; import getVoteName from '../static/scripts/edit/utility/getVoteName.js'; import {editorMayAddNote, editorMayVoteOnEdit} @@ -35,10 +37,12 @@ component EditIndex( fullWidth: boolean = false, ) { const $c = React.useContext(CatalystContext); + const isAdmin = isAccountAdmin($c.user); const canAddNote = Boolean($c.user && editorMayAddNote(edit, $c.user)); const isOwnEdit = Boolean($c.user && $c.user.id === edit.editor_id); const canVoteHere = Boolean($c.user && editorMayVoteOnEdit(edit, $c.user)); const detailsElement = getEditDetailsElement(edit); + const showAcceptReject = DBDefs.DB_STAGING_TESTING_FEATURES || isAdmin; return ( @@ -122,26 +126,44 @@ component EditIndex( ) : null} {$c.user ? ( - edit.is_open && DBDefs.DB_STAGING_TESTING_FEATURES ? ( - <> -

{l('Testing features')}

-

- {l(`To aid in testing, the following features - have been made available on testing servers:`)} -

- - + edit.is_open && showAcceptReject ? ( + isAdmin ? ( + <> +

{l_admin('Admin features')}

+ + + ) : ( + <> +

{l('Testing features')}

+

+ {l(`To aid in testing, the following features + have been made available on testing servers:`)} +

+ + + ) ) : null ) : (

diff --git a/root/edit/components/EditHeader.js b/root/edit/components/EditHeader.js index 44b091348e4..31b1a1563a2 100644 --- a/root/edit/components/EditHeader.js +++ b/root/edit/components/EditHeader.js @@ -12,7 +12,7 @@ import * as React from 'react'; import RequestLogin from '../../components/RequestLogin.js'; import SubHeader from '../../components/SubHeader.js'; import VotingPeriod from '../../components/VotingPeriod.js'; -import {EDIT_VOTE_APPROVE} from '../../constants.js'; +import {EDIT_VOTE_ADMIN_APPROVE, EDIT_VOTE_APPROVE} from '../../constants.js'; import {CatalystContext} from '../../context.mjs'; import EditLink from '../../static/scripts/common/components/EditLink.js'; import EditorLink from '../../static/scripts/common/components/EditorLink.js'; @@ -62,7 +62,8 @@ component EditHeader( ? getVoteName(latestVoteForVoter.vote) : null; const editWasApproved = !edit.is_open && edit.votes.some( - (vote) => vote.vote === EDIT_VOTE_APPROVE, + (vote) => (vote.vote === EDIT_VOTE_APPROVE || + vote.vote === EDIT_VOTE_ADMIN_APPROVE), ); const showVoteTally = latestVoteForEditor || isEditEditor || !edit.is_open; diff --git a/root/edit/components/EditNote.js b/root/edit/components/EditNote.js index 0ba2d9b9d3a..f32cc8f3439 100644 --- a/root/edit/components/EditNote.js +++ b/root/edit/components/EditNote.js @@ -10,6 +10,8 @@ import * as React from 'react'; import { + EDIT_VOTE_ADMIN_APPROVE, + EDIT_VOTE_ADMIN_REJECT, EDIT_VOTE_APPROVE, EDIT_VOTE_NO, EDIT_VOTE_YES, @@ -20,6 +22,7 @@ import EditorLink from '../../static/scripts/common/components/EditorLink.js'; import bracketed from '../../static/scripts/common/utility/bracketed.js'; import {isAccountAdmin, isAddingNotesDisabled} from '../../static/scripts/common/utility/privileges.js'; +import {kebabCase} from '../../static/scripts/common/utility/strings.js'; import getVoteName from '../../static/scripts/edit/utility/getVoteName.js'; import formatUserDate from '../../utility/formatUserDate.js'; import parseIsoDate from '../../utility/parseIsoDate.js'; @@ -34,7 +37,7 @@ function returnVoteClass(vote: ?VoteT, isOwner: boolean) { let className = ''; if (vote) { - className = getVoteName(vote.vote); + className = kebabCase(getVoteName(vote.vote)); } if (isOwner) { @@ -68,7 +71,9 @@ component EditNote( )); const showVotingIcon = lastRelevantVote && ( lastRelevantVote.vote === EDIT_VOTE_APPROVE || + lastRelevantVote.vote === EDIT_VOTE_ADMIN_APPROVE || lastRelevantVote.vote === EDIT_VOTE_NO || + lastRelevantVote.vote === EDIT_VOTE_ADMIN_REJECT || lastRelevantVote.vote === EDIT_VOTE_YES ); diff --git a/root/edit/components/EditSummary.js b/root/edit/components/EditSummary.js index ec1cb48f094..437aebf9413 100644 --- a/root/edit/components/EditSummary.js +++ b/root/edit/components/EditSummary.js @@ -15,6 +15,8 @@ import { } from '../../constants.js'; import {CatalystContext} from '../../context.mjs'; import DBDefs from '../../static/scripts/common/DBDefs.mjs'; +import {isAccountAdmin} + from '../../static/scripts/common/utility/privileges.js'; import { editorMayAddNote, editorMayApprove, @@ -31,9 +33,11 @@ component EditSummary( ) { const $c = React.useContext(CatalystContext); const user = $c.user; + const isAdmin = isAccountAdmin(user); const mayAddNote = editorMayAddNote(edit, user); const mayApprove = editorMayApprove(edit, user); const mayCancel = editorMayCancel(edit, user); + const showAcceptReject = DBDefs.DB_STAGING_TESTING_FEATURES || isAdmin; return ( <> @@ -74,8 +78,23 @@ component EditSummary( ) : null} - {edit.status === EDIT_STATUS_OPEN && - DBDefs.DB_STAGING_TESTING_FEATURES ? ( + {edit.status === EDIT_STATUS_OPEN && showAcceptReject ? ( + isAdmin ? ( + <> + + {l('Accept edit')} + + + {l('Reject edit')} + + + ) : ( <> - ) : null} + ) + ) : null} ) : null} ); diff --git a/root/edit/search_macros.tt b/root/edit/search_macros.tt index 32464d1d37b..6e7b8e81cc9 100644 --- a/root/edit/search_macros.tt +++ b/root/edit/search_macros.tt @@ -291,11 +291,13 @@