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). 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.
metabrainz · Jan 6, 2025 · 721a07e · 721a07e
1 parent f803e5f
commit 721a07e
Show file tree

Hide file tree

Showing 9 changed files with 451 additions and 29 deletions.
diff --git 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/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;
         }
 

diff --git a/root/edit/EditIndex.js b/root/edit/EditIndex.js
@@ -17,6 +17,8 @@ import EditorLink from '../static/scripts/common/components/EditorLink.js';
 import {DB_STAGING_TESTING_FEATURES}
   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}
@@ -36,10 +38,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 = DB_STAGING_TESTING_FEATURES || isAdmin;
 
   return (
     <Layout fullWidth={fullWidth} title={texp.l('Edit #{id}', {id: edit.id})}>
@@ -123,26 +127,44 @@ component EditIndex(
           ) : null}
 
           {$c.user ? (
-            edit.is_open && DB_STAGING_TESTING_FEATURES ? (
-              <>
-                <h2>{l('Testing features')}</h2>
-                <p>
-                  {l(`To aid in testing, the following features
-                      have been made available on testing servers:`)}
-                </p>
-                <ul>
-                  <li>
-                    <a href={'/test/accept-edit/' + edit.id}>
-                      {l('Accept edit')}
-                    </a>
-                  </li>
-                  <li>
-                    <a href={'/test/reject-edit/' + edit.id}>
-                      {l('Reject edit')}
-                    </a>
-                  </li>
-                </ul>
-              </>
+            edit.is_open && showAcceptReject ? (
+              isAdmin ? (
+                <>
+                  <h2>{l_admin('Admin features')}</h2>
+                  <ul>
+                    <li>
+                      <a href={'/admin/accept-edit/' + edit.id}>
+                        {l('Accept edit')}
+                      </a>
+                    </li>
+                    <li>
+                      <a href={'/admin/reject-edit/' + edit.id}>
+                        {l('Reject edit')}
+                      </a>
+                    </li>
+                  </ul>
+                </>
+              ) : (
+                <>
+                  <h2>{l('Testing features')}</h2>
+                  <p>
+                    {l(`To aid in testing, the following features
+                        have been made available on testing servers:`)}
+                  </p>
+                  <ul>
+                    <li>
+                      <a href={'/test/accept-edit/' + edit.id}>
+                        {l('Accept edit')}
+                      </a>
+                    </li>
+                    <li>
+                      <a href={'/test/reject-edit/' + edit.id}>
+                        {l('Reject edit')}
+                      </a>
+                    </li>
+                  </ul>
+                </>
+              )
             ) : null
           ) : (
             <p>

diff --git a/root/edit/components/EditSummary.js b/root/edit/components/EditSummary.js
@@ -18,6 +18,8 @@ import {
   DB_READ_ONLY,
   DB_STAGING_TESTING_FEATURES,
 } from '../../static/scripts/common/DBDefs.mjs';
+import {isAccountAdmin}
+  from '../../static/scripts/common/utility/privileges.js';
 import {
   editorMayAddNote,
   editorMayApprove,
@@ -34,9 +36,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 = DB_STAGING_TESTING_FEATURES || isAdmin;
 
   return (
     <>
@@ -77,8 +81,23 @@ component EditSummary(
             </a>
           ) : null}
 
-          {edit.status === EDIT_STATUS_OPEN &&
-            DB_STAGING_TESTING_FEATURES ? (
+          {edit.status === EDIT_STATUS_OPEN && showAcceptReject ? (
+            isAdmin ? (
+              <>
+                <a
+                  className="positive"
+                  href={`/admin/accept-edit/${edit.id}`}
+                >
+                  {l('Accept edit')}
+                </a>
+                <a
+                  className="negative"
+                  href={`/admin/reject-edit/${edit.id}`}
+                >
+                  {l('Reject edit')}
+                </a>
+              </>
+            ) : (
               <>
                 <a
                   className="positive"
@@ -93,7 +112,8 @@ component EditSummary(
                   {l('Reject edit')}
                 </a>
               </>
-            ) : null}
+            )
+          ) : null}
         </div>) : null}
     </>
   );

diff --git a/root/user/PrivilegedUsers.js b/root/user/PrivilegedUsers.js
@@ -110,7 +110,8 @@ component PrivilegedUsers(
 
         <h2>{lp('Account administrators', 'header')}</h2>
         <p>
-          {l('Account administrators can edit and delete user accounts.')}
+          {l(`Account administrators can edit and delete user accounts,
+              and immediately accept and reject edits.`)}
         </p>
         <p>
           {texp.l(