From 7d6d2c128a9a7dbf788bb5d9b541e7e31480f118 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 7 Feb 2019 10:27:17 -0800 Subject: [PATCH] Make "bin/audit delete" synchronize commit audit status, and improve "bin/audit synchronize" documentation Summary: Depends on D20126. See PHI1056. Ref T13244. - `bin/audit delete` destroys audit requests, but does not update the overall audit state for associated commits. For example, if you destroy all audit requests for a commit, it does not move to "No Audit Required". - `bin/audit synchronize` does this synchronize step, but is poorly documented. Make `bin/audit delete` synchronize affected commits. Document `bin/audit synchronize` better. There's some reasonable argument that `bin/audit synchronize` perhaps shouldn't exist, but it does let you recover from an accidentally (or intentionally) mangled database state. For now, let it live. Test Plan: - Ran `bin/audit delete`, saw audits destroyed and affected commits synchornized. - Ran `bin/audit synchronize`, saw behavior unchanged. - Ran `bin/audit help`, got better help. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13244 Differential Revision: https://secure.phabricator.com/D20127 --- ...abricatorAuditManagementDeleteWorkflow.php | 106 ++++++++++++------ .../PhabricatorAuditManagementWorkflow.php | 35 ++++++ ...atorAuditSynchronizeManagementWorkflow.php | 43 ++----- src/docs/user/userguide/audit.diviner | 9 +- 4 files changed, 120 insertions(+), 73 deletions(-) diff --git a/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php b/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php index 23583422fb..7d14df7239 100644 --- a/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php +++ b/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php @@ -105,10 +105,10 @@ final class PhabricatorAuditManagementDeleteWorkflow $query->withPHIDs(mpull($commits, 'getPHID')); } - $commits = $query->execute(); - $commits = mpull($commits, null, 'getPHID'); + $commit_iterator = new PhabricatorQueryIterator($query); + $audits = array(); - foreach ($commits as $commit) { + foreach ($commit_iterator as $commit) { $commit_audits = $commit->getAudits(); foreach ($commit_audits as $key => $audit) { if ($id_map && empty($id_map[$audit->getID()])) { @@ -131,51 +131,87 @@ final class PhabricatorAuditManagementDeleteWorkflow continue; } } - $audits[] = $commit_audits; - } - $audits = array_mergev($audits); - $console = PhutilConsole::getConsole(); + if (!$commit_audits) { + continue; + } - if (!$audits) { - $console->writeErr("%s\n", pht('No audits match the query.')); - return 0; - } + $handles = id(new PhabricatorHandleQuery()) + ->setViewer($viewer) + ->withPHIDs(mpull($commit_audits, 'getAuditorPHID')) + ->execute(); - $handles = id(new PhabricatorHandleQuery()) - ->setViewer($this->getViewer()) - ->withPHIDs(mpull($audits, 'getAuditorPHID')) - ->execute(); + foreach ($commit_audits as $audit) { + $audit_id = $audit->getID(); - - foreach ($audits as $audit) { - $commit = $commits[$audit->getCommitPHID()]; - - $console->writeOut( - "%s\n", - sprintf( + $description = sprintf( '%10d %-16s %-16s %s: %s', - $audit->getID(), + $audit_id, $handles[$audit->getAuditorPHID()]->getName(), PhabricatorAuditStatusConstants::getStatusName( $audit->getAuditStatus()), $commit->getRepository()->formatCommitName( $commit->getCommitIdentifier()), - trim($commit->getSummary()))); + trim($commit->getSummary())); + + $audits[] = array( + 'auditID' => $audit_id, + 'commitPHID' => $commit->getPHID(), + 'description' => $description, + ); + } } - if (!$is_dry_run) { - $message = pht( - 'Really delete these %d audit(s)? They will be permanently deleted '. - 'and can not be recovered.', - count($audits)); - if ($console->confirm($message)) { - foreach ($audits as $audit) { - $id = $audit->getID(); - $console->writeOut("%s\n", pht('Deleting audit %d...', $id)); - $audit->delete(); - } + if (!$audits) { + echo tsprintf( + "%s\n", + pht('No audits match the query.')); + return 0; + } + + foreach ($audits as $audit_spec) { + echo tsprintf( + "%s\n", + $audit_spec['description']); + } + + if ($is_dry_run) { + echo tsprintf( + "%s\n", + pht('This is a dry run, so no changes will be made.')); + return 0; + } + + $message = pht( + 'Really delete these %s audit(s)? They will be permanently deleted '. + 'and can not be recovered.', + phutil_count($audits)); + if (!phutil_console_confirm($message)) { + echo tsprintf( + "%s\n", + pht('User aborted the workflow.')); + return 1; + } + + $audits_by_commit = igroup($audits, 'commitPHID'); + foreach ($audits_by_commit as $commit_phid => $audit_specs) { + $audit_ids = ipull($audit_specs, 'auditID'); + + $audits = id(new PhabricatorRepositoryAuditRequest())->loadAllWhere( + 'id IN (%Ld)', + $audit_ids); + + foreach ($audits as $audit) { + $id = $audit->getID(); + + echo tsprintf( + "%s\n", + pht('Deleting audit %d...', $id)); + + $audit->delete(); } + + $this->synchronizeCommitAuditState($commit_phid); } return 0; diff --git a/src/applications/audit/management/PhabricatorAuditManagementWorkflow.php b/src/applications/audit/management/PhabricatorAuditManagementWorkflow.php index 6112a38e1d..b9d90bddc8 100644 --- a/src/applications/audit/management/PhabricatorAuditManagementWorkflow.php +++ b/src/applications/audit/management/PhabricatorAuditManagementWorkflow.php @@ -87,4 +87,39 @@ abstract class PhabricatorAuditManagementWorkflow return $commits; } + protected function synchronizeCommitAuditState($commit_phid) { + $viewer = $this->getViewer(); + + $commit = id(new DiffusionCommitQuery()) + ->setViewer($viewer) + ->withPHIDs(array($commit_phid)) + ->needAuditRequests(true) + ->executeOne(); + if (!$commit) { + return; + } + + $old_status = $commit->getAuditStatusObject(); + $commit->updateAuditStatus($commit->getAudits()); + $new_status = $commit->getAuditStatusObject(); + + if ($old_status->getKey() == $new_status->getKey()) { + echo tsprintf( + "%s\n", + pht( + 'No synchronization changes for "%s".', + $commit->getDisplayName())); + } else { + echo tsprintf( + "%s\n", + pht( + 'Synchronizing "%s": "%s" -> "%s".', + $commit->getDisplayName(), + $old_status->getName(), + $new_status->getName())); + + $commit->save(); + } + } + } diff --git a/src/applications/audit/management/PhabricatorAuditSynchronizeManagementWorkflow.php b/src/applications/audit/management/PhabricatorAuditSynchronizeManagementWorkflow.php index 96d06e65c2..abd0a3c637 100644 --- a/src/applications/audit/management/PhabricatorAuditSynchronizeManagementWorkflow.php +++ b/src/applications/audit/management/PhabricatorAuditSynchronizeManagementWorkflow.php @@ -6,8 +6,16 @@ final class PhabricatorAuditSynchronizeManagementWorkflow protected function didConstruct() { $this ->setName('synchronize') - ->setExamples('**synchronize** ...') - ->setSynopsis(pht('Update audit status for commits.')) + ->setExamples( + "**synchronize** __repository__ ...\n". + "**synchronize** __commit__ ...\n". + "**synchronize** --all") + ->setSynopsis( + pht( + 'Update commits to make their summary audit state reflect the '. + 'state of their actual audit requests. This can fix inconsistencies '. + 'in database state if audit requests have been mangled '. + 'accidentally (or on purpose).')) ->setArguments( array_merge( $this->getCommitConstraintArguments(), @@ -21,36 +29,7 @@ final class PhabricatorAuditSynchronizeManagementWorkflow foreach ($objects as $object) { $commits = $this->loadCommitsForConstraintObject($object); foreach ($commits as $commit) { - $commit = id(new DiffusionCommitQuery()) - ->setViewer($viewer) - ->withPHIDs(array($commit->getPHID())) - ->needAuditRequests(true) - ->executeOne(); - if (!$commit) { - continue; - } - - $old_status = $commit->getAuditStatusObject(); - $commit->updateAuditStatus($commit->getAudits()); - $new_status = $commit->getAuditStatusObject(); - - if ($old_status->getKey() == $new_status->getKey()) { - echo tsprintf( - "%s\n", - pht( - 'No changes for "%s".', - $commit->getDisplayName())); - } else { - echo tsprintf( - "%s\n", - pht( - 'Updating "%s": "%s" -> "%s".', - $commit->getDisplayName(), - $old_status->getName(), - $new_status->getName())); - - $commit->save(); - } + $this->synchronizeCommitAuditState($commit->getPHID()); } } } diff --git a/src/docs/user/userguide/audit.diviner b/src/docs/user/userguide/audit.diviner index 0d10867906..5223f6c969 100644 --- a/src/docs/user/userguide/audit.diviner +++ b/src/docs/user/userguide/audit.diviner @@ -175,16 +175,13 @@ You can use this command to forcibly delete requests which may have triggered incorrectly (for example, because a package or Herald rule was configured in an overbroad way). -After deleting audits, you may want to run `bin/audit synchronize` to -synchronize audit state. - **Synchronize Audit State**: Synchronize the audit state of commits to the current open audit requests with `bin/audit synchronize`. Normally, overall audit state is automatically kept up to date as changes are -made to an audit. However, if you delete audits or manually update the database -to make changes to audit request state, the state of corresponding commits may -no longer be correct. +made to an audit. However, if you manually update the database to make changes +to audit request state, the state of corresponding commits may no longer be +consistent. This command will update commits so their overall audit state reflects the cumulative state of their actual audit requests.