From 688f245a95bef83dbb4fdf45d0062f21881b8280 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 2 Aug 2014 14:44:35 -0700 Subject: [PATCH] Use transactions to apply "add auditors" action in Audit Summary: Ref T4896. Move the write for "Add Auditors" inside the new Editor. There are no longer any readers or writers for metadata, so remove the calls for it. Test Plan: Added auditors from the web UI. Reviewers: joshuaspence, btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T4896 Differential Revision: https://secure.phabricator.com/D10123 --- resources/celerity/map.php | 16 ++++---- .../PhabricatorAuditAddCommentController.php | 6 +-- .../editor/PhabricatorAuditCommentEditor.php | 37 +----------------- .../audit/editor/PhabricatorAuditEditor.php | 37 +++++++++++++++++- .../mail/PhabricatorAuditMailReceiver.php | 1 + .../audit/storage/PhabricatorAuditComment.php | 38 ------------------- ...DiffusionCreateCommentConduitAPIMethod.php | 1 + 7 files changed, 49 insertions(+), 87 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 957f8a1026..d99b6c02b1 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -8,7 +8,7 @@ return array( 'names' => array( 'core.pkg.css' => '66ada2ec', - 'core.pkg.js' => 'c3965034', + 'core.pkg.js' => '8cd3cd8c', 'darkconsole.pkg.js' => 'df001cab', 'differential.pkg.css' => '4a93db37', 'differential.pkg.js' => '79503aa4', @@ -463,7 +463,7 @@ return array( 'rsrc/js/core/behavior-error-log.js' => 'a5d7cf86', 'rsrc/js/core/behavior-fancy-datepicker.js' => 'a5573bcd', 'rsrc/js/core/behavior-file-tree.js' => '88236f00', - 'rsrc/js/core/behavior-form.js' => '3b1557b3', + 'rsrc/js/core/behavior-form.js' => '5c54cbf3', 'rsrc/js/core/behavior-gesture.js' => '3ab51e2c', 'rsrc/js/core/behavior-global-drag-and-drop.js' => '3672899b', 'rsrc/js/core/behavior-high-security-warning.js' => '8fc1c918', @@ -551,7 +551,7 @@ return array( 'javelin-behavior-aphront-basic-tokenizer' => 'b3a4b884', 'javelin-behavior-aphront-crop' => 'fa0f4fc2', 'javelin-behavior-aphront-drag-and-drop-textarea' => '92eb531d', - 'javelin-behavior-aphront-form-disable-on-submit' => '3b1557b3', + 'javelin-behavior-aphront-form-disable-on-submit' => '5c54cbf3', 'javelin-behavior-aphront-more' => 'a80d0378', 'javelin-behavior-audio-source' => '59b251eb', 'javelin-behavior-audit-preview' => 'd835b03a', @@ -1069,11 +1069,6 @@ return array( 'javelin-dom', 'javelin-magical-init', ), - '3b1557b3' => array( - 'javelin-behavior', - 'javelin-stratcom', - 'javelin-dom', - ), '3b3e1664' => array( 'javelin-behavior', 'javelin-dom', @@ -1214,6 +1209,11 @@ return array( 'javelin-behavior', 'javelin-stratcom', ), + '5c54cbf3' => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-dom', + ), '5fefb143' => array( 'javelin-behavior', 'javelin-dom', diff --git a/src/applications/audit/controller/PhabricatorAuditAddCommentController.php b/src/applications/audit/controller/PhabricatorAuditAddCommentController.php index fb0136c814..9cf07058cb 100644 --- a/src/applications/audit/controller/PhabricatorAuditAddCommentController.php +++ b/src/applications/audit/controller/PhabricatorAuditAddCommentController.php @@ -15,6 +15,7 @@ final class PhabricatorAuditAddCommentController $commit = id(new DiffusionCommitQuery()) ->setViewer($user) ->withPHIDs(array($commit_phid)) + ->needAuditRequests(true) ->executeOne(); if (!$commit) { return new Aphront404Response(); @@ -31,10 +32,7 @@ final class PhabricatorAuditAddCommentController $auditors = $request->getArr('auditors'); $comments[] = id(new PhabricatorAuditComment()) ->setAction(PhabricatorAuditActionConstants::ADD_AUDITORS) - ->setMetadata( - array( - PhabricatorAuditComment::METADATA_ADDED_AUDITORS => $auditors, - )); + ->setNewValue(array_fuse($auditors)); break; case PhabricatorAuditActionConstants::ADD_CCS: $comments[] = id(new PhabricatorAuditComment()) diff --git a/src/applications/audit/editor/PhabricatorAuditCommentEditor.php b/src/applications/audit/editor/PhabricatorAuditCommentEditor.php index 357ad2dd5d..c0372f1d73 100644 --- a/src/applications/audit/editor/PhabricatorAuditCommentEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditCommentEditor.php @@ -47,10 +47,7 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor { $audit_phids = self::loadAuditPHIDsForUser($actor); $audit_phids = array_fill_keys($audit_phids, true); - $requests = id(new PhabricatorRepositoryAuditRequest()) - ->loadAllWhere( - 'commitPHID = %s', - $commit->getPHID()); + $requests = $commit->getAudits(); // TODO: We should validate the action, currently we allow anyone to, e.g., // close an audit if they muck with form parameters. I'll followup with this @@ -182,38 +179,6 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor { } } - $auditors = array(); - - foreach ($comments as $comment) { - $meta = $comment->getMetadata(); - - $auditor_phids = idx( - $meta, - PhabricatorAuditComment::METADATA_ADDED_AUDITORS, - array()); - foreach ($auditor_phids as $phid) { - $auditors[] = $phid; - } - } - - $requests_by_auditor = mpull($requests, null, 'getAuditorPHID'); - $requests_phids = array_keys($requests_by_auditor); - - $auditors = array_diff($auditors, $requests_phids); - - if ($auditors) { - foreach ($auditors as $auditor_phid) { - $audit_requested = PhabricatorAuditStatusConstants::AUDIT_REQUESTED; - $requests[] = id (new PhabricatorRepositoryAuditRequest()) - ->setCommitPHID($commit->getPHID()) - ->setAuditorPHID($auditor_phid) - ->setAuditStatus($audit_requested) - ->setAuditReasons( - array('Added by '.$actor->getUsername())) - ->save(); - } - } - $commit->updateAuditStatus($requests); $commit->save(); diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php index d016799988..60c9621f2e 100644 --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -86,7 +86,42 @@ final class PhabricatorAuditEditor case PhabricatorAuditActionConstants::INLINE: return; case PhabricatorAuditActionConstants::ADD_AUDITORS: - // TODO: For now, these are applied externally. + $new = $xaction->getNewValue(); + if (!is_array($new)) { + $new = array(); + } + + $old = $xaction->getOldValue(); + if (!is_array($old)) { + $old = array(); + } + + $add = array_diff_key($new, $old); + + $actor = $this->requireActor(); + + $requests = $object->getAudits(); + $requests = mpull($requests, null, 'getAuditorPHID'); + foreach ($add as $phid) { + if (isset($requests[$phid])) { + continue; + } + + $audit_requested = PhabricatorAuditStatusConstants::AUDIT_REQUESTED; + $requests[] = id (new PhabricatorRepositoryAuditRequest()) + ->setCommitPHID($object->getPHID()) + ->setAuditorPHID($phid) + ->setAuditStatus($audit_requested) + ->setAuditReasons( + array( + 'Added by '.$actor->getUsername(), + )) + ->save(); + } + + $object->updateAuditStatus($requests); + $object->attachAudits($requests); + $object->save(); return; } diff --git a/src/applications/audit/mail/PhabricatorAuditMailReceiver.php b/src/applications/audit/mail/PhabricatorAuditMailReceiver.php index 4b5b342842..4942e84829 100644 --- a/src/applications/audit/mail/PhabricatorAuditMailReceiver.php +++ b/src/applications/audit/mail/PhabricatorAuditMailReceiver.php @@ -17,6 +17,7 @@ final class PhabricatorAuditMailReceiver extends PhabricatorObjectMailReceiver { return id(new DiffusionCommitQuery()) ->setViewer($viewer) ->withIDs(array($id)) + ->needAuditRequests(true) ->executeOne(); } diff --git a/src/applications/audit/storage/PhabricatorAuditComment.php b/src/applications/audit/storage/PhabricatorAuditComment.php index 0c8dd73629..ac19bf9a09 100644 --- a/src/applications/audit/storage/PhabricatorAuditComment.php +++ b/src/applications/audit/storage/PhabricatorAuditComment.php @@ -3,8 +3,6 @@ final class PhabricatorAuditComment implements PhabricatorMarkupInterface { - const METADATA_ADDED_AUDITORS = 'added-auditors'; - const MARKUP_FIELD_BODY = 'markup:body'; private $proxyComment; @@ -145,42 +143,6 @@ final class PhabricatorAuditComment } } - public function setMetadata(array $metadata) { - if (!$this->proxy->getTransactionType()) { - throw new Exception(pht('Call setAction() before getMetadata()!')); - } - - $type = $this->proxy->getTransactionType(); - switch ($type) { - case PhabricatorAuditActionConstants::ADD_AUDITORS: - $raw_phids = idx($metadata, self::METADATA_ADDED_AUDITORS, array()); - break; - default: - throw new Exception(pht('No metadata expected!')); - } - - $this->proxy->setNewValue(array_fuse($raw_phids)); - - return $this; - } - - public function getMetadata() { - if (!$this->proxy->getTransactionType()) { - throw new Exception(pht('Call setAction() before getMetadata()!')); - } - - $type = $this->proxy->getTransactionType(); - $new_value = $this->proxy->getNewValue(); - switch ($type) { - case PhabricatorAuditActionConstants::ADD_AUDITORS: - return array( - self::METADATA_ADDED_AUDITORS => array_keys($new_value), - ); - } - - return array(); - } - public function save() { throw new Exception( pht('This object can no longer be written to directly!')); diff --git a/src/applications/diffusion/conduit/DiffusionCreateCommentConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionCreateCommentConduitAPIMethod.php index 248c64e37b..9ed0ebe235 100644 --- a/src/applications/diffusion/conduit/DiffusionCreateCommentConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionCreateCommentConduitAPIMethod.php @@ -43,6 +43,7 @@ final class DiffusionCreateCommentConduitAPIMethod $commit = id(new DiffusionCommitQuery()) ->setViewer($request->getUser()) ->withPHIDs(array($commit_phid)) + ->needAuditRequests(true) ->executeOne(); if (!$commit) { throw new ConduitException('ERR_BAD_COMMIT');