From 2044e512062556f682bed039d180eb81139e121f Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 26 Mar 2012 09:44:06 -0700 Subject: [PATCH] Add "Resign from Audit" and "Close Audit" actions to Diffusion Summary: See some discussion in D2002. Add two new actions: - Resign: (auditor only) closes your open request (user request ONLY) by putting it in a "resigned" state. - Close: (author only) closes all open requests by putting them in a "closed" state. @davidreuss, this is probably conflict-city with D2002 -- I'll wait for you to land first and then handle the merge on my end. Test Plan: Resigned from and closed audits. Reviewers: 20after4, davidreuss, btrahan Reviewed By: btrahan CC: aran, epriestley Maniphest Tasks: T904 Differential Revision: https://secure.phabricator.com/D2013 --- .../PhabricatorAuditActionConstants.php | 26 ++-- .../audit/constants/action/__init__.php | 2 - .../PhabricatorAuditStatusConstants.php | 16 +++ .../list/PhabricatorAuditListController.php | 7 + .../comment/PhabricatorAuditCommentEditor.php | 120 ++++++++++++++---- .../query/audit/PhabricatorAuditQuery.php | 72 +++++++++-- .../audit/query/audit/__init__.php | 1 + .../view/list/PhabricatorAuditListView.php | 42 ++++-- .../commit/DiffusionCommitController.php | 106 ++++++++++++++-- .../diffusion/controller/commit/__init__.php | 1 + .../view/comment/DiffusionCommentView.php | 30 ++--- .../diffusion/view/comment/__init__.php | 1 + .../PhabricatorDirectoryMainController.php | 1 + 13 files changed, 339 insertions(+), 86 deletions(-) diff --git a/src/applications/audit/constants/action/PhabricatorAuditActionConstants.php b/src/applications/audit/constants/action/PhabricatorAuditActionConstants.php index 2bbb116d2b..7fc4db86f1 100644 --- a/src/applications/audit/constants/action/PhabricatorAuditActionConstants.php +++ b/src/applications/audit/constants/action/PhabricatorAuditActionConstants.php @@ -18,36 +18,38 @@ final class PhabricatorAuditActionConstants { - const CONCERN = 'concern'; - const ACCEPT = 'accept'; - const COMMENT = 'comment'; + const CONCERN = 'concern'; + const ACCEPT = 'accept'; + const COMMENT = 'comment'; + const RESIGN = 'resign'; + const CLOSE = 'close'; public static function getActionNameMap() { static $map = array( self::COMMENT => 'Comment', self::CONCERN => 'Raise Concern', self::ACCEPT => 'Accept Commit', + self::RESIGN => 'Resign from Audit', + self::CLOSE => 'Close Audit', ); return $map; } + public static function getActionName($constant) { + $map = self::getActionNameMap(); + return idx($map, $constant, 'Unknown'); + } + public static function getActionPastTenseVerb($action) { static $map = array( self::COMMENT => 'commented on', self::CONCERN => 'raised a concern with', self::ACCEPT => 'accepted', + self::RESIGN => 'resigned from', + self::CLOSE => 'closed', ); return idx($map, $action, 'updated'); } - public static function getStatusNameMap() { - static $map = array( - self::CONCERN => PhabricatorAuditStatusConstants::CONCERNED, - self::ACCEPT => PhabricatorAuditStatusConstants::ACCEPTED, - ); - - return $map; - } - } diff --git a/src/applications/audit/constants/action/__init__.php b/src/applications/audit/constants/action/__init__.php index a1ffd25b96..2c6f3903f9 100644 --- a/src/applications/audit/constants/action/__init__.php +++ b/src/applications/audit/constants/action/__init__.php @@ -6,8 +6,6 @@ -phutil_require_module('phabricator', 'applications/audit/constants/status'); - phutil_require_module('phutil', 'utils'); diff --git a/src/applications/audit/constants/status/PhabricatorAuditStatusConstants.php b/src/applications/audit/constants/status/PhabricatorAuditStatusConstants.php index 517a7c8199..02e334f195 100644 --- a/src/applications/audit/constants/status/PhabricatorAuditStatusConstants.php +++ b/src/applications/audit/constants/status/PhabricatorAuditStatusConstants.php @@ -24,6 +24,8 @@ final class PhabricatorAuditStatusConstants { const CONCERNED = 'concerned'; const ACCEPTED = 'accepted'; const AUDIT_REQUESTED = 'requested'; + const RESIGNED = 'resigned'; + const CLOSED = 'closed'; public static function getStatusNameMap() { static $map = array( @@ -33,6 +35,8 @@ final class PhabricatorAuditStatusConstants { self::CONCERNED => 'Concern Raised', self::ACCEPTED => 'Accepted', self::AUDIT_REQUESTED => 'Audit Requested', + self::RESIGNED => 'Resigned', + self::CLOSED => 'Closed', ); return $map; @@ -42,4 +46,16 @@ final class PhabricatorAuditStatusConstants { return idx(self::getStatusNameMap(), $code, 'Unknown'); } + public static function getOpenStatusConstants() { + return array( + self::AUDIT_REQUIRED, + self::AUDIT_REQUESTED, + self::CONCERNED, + ); + } + + public static function isOpenStatus($status) { + return in_array($status, self::getOpenStatusConstants()); + } + } diff --git a/src/applications/audit/controller/list/PhabricatorAuditListController.php b/src/applications/audit/controller/list/PhabricatorAuditListController.php index fa75d73c0d..2203d9ca70 100644 --- a/src/applications/audit/controller/list/PhabricatorAuditListController.php +++ b/src/applications/audit/controller/list/PhabricatorAuditListController.php @@ -301,6 +301,8 @@ final class PhabricatorAuditListController extends PhabricatorAuditController { $query->setLimit($pager->getPageSize() + 1); } + $awaiting = null; + $phids = null; switch ($this->filter) { case 'user': @@ -312,6 +314,7 @@ final class PhabricatorAuditListController extends PhabricatorAuditController { throw new Exception("Invalid user!"); } $phids = PhabricatorAuditCommentEditor::loadAuditPHIDsForUser($obj); + $awaiting = $obj; break; case 'project': case 'package': @@ -327,6 +330,10 @@ final class PhabricatorAuditListController extends PhabricatorAuditController { $query->withAuditorPHIDs($phids); } + if ($awaiting) { + $query->withAwaitingUser($awaiting); + } + switch ($this->filter) { case 'audits': case 'user': diff --git a/src/applications/audit/editor/comment/PhabricatorAuditCommentEditor.php b/src/applications/audit/editor/comment/PhabricatorAuditCommentEditor.php index dd127dcd4a..2b2d6e926d 100644 --- a/src/applications/audit/editor/comment/PhabricatorAuditCommentEditor.php +++ b/src/applications/audit/editor/comment/PhabricatorAuditCommentEditor.php @@ -83,36 +83,102 @@ final class PhabricatorAuditCommentEditor { $commit->getPHID()); $action = $comment->getAction(); - $status_map = PhabricatorAuditActionConstants::getStatusNameMap(); - $status = idx($status_map, $action, null); - // Status may be empty for updates which don't affect status, like - // "comment". - $have_any_requests = false; - foreach ($requests as $request) { - if (empty($audit_phids[$request->getAuditorPHID()])) { - continue; - } - $have_any_requests = true; - if ($status) { - $request->setAuditStatus($status); - $request->save(); - } - } - if (!$have_any_requests) { + // 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 + // and handle the no-effect cases (e.g., closing and already-closed audit). + + + $user_is_author = ($user->getPHID() == $commit->getAuthorPHID()); + + if ($action == PhabricatorAuditActionConstants::CLOSE) { + // "Close" means wipe out all the concerns. + $concerned_status = PhabricatorAuditStatusConstants::CONCERNED; + foreach ($requests as $request) { + if ($request->getAuditStatus() == $concerned_status) { + $request->setAuditStatus(PhabricatorAuditStatusConstants::CLOSED); + $request->save(); + } + } + } else { + $have_any_requests = false; + foreach ($requests as $request) { + if (empty($audit_phids[$request->getAuditorPHID()])) { + continue; + } + + $request_is_for_user = ($request->getAuditorPHID() == $user->getPHID()); + + $have_any_requests = true; + $new_status = null; + switch ($action) { + case PhabricatorAuditActionConstants::COMMENT: + // Comments don't change audit statuses. + break; + case PhabricatorAuditActionConstants::ACCEPT: + if (!$user_is_author || $request_is_for_user) { + // When modifying your own commits, you act only on behalf of + // yourself, not your packages/projects -- the idea being that + // you can't accept your own commits. + $new_status = PhabricatorAuditStatusConstants::ACCEPTED; + } + break; + case PhabricatorAuditActionConstants::CONCERN: + if (!$user_is_author || $request_is_for_user) { + // See above. + $new_status = PhabricatorAuditStatusConstants::CONCERNED; + } + break; + case PhabricatorAuditActionConstants::RESIGN: + // NOTE: Resigning resigns ONLY your user request, not the requests + // of any projects or packages you are a member of. + if ($request_is_for_user) { + $new_status = PhabricatorAuditStatusConstants::RESIGNED; + } + break; + default: + throw new Exception("Unknown action '{$action}'!"); + } + if ($new_status !== null) { + $request->setAuditStatus($new_status); + $request->save(); + } + } + // If the user has no current authority over any audit trigger, make a // new one to represent their audit state. - $request = id(new PhabricatorRepositoryAuditRequest()) - ->setCommitPHID($commit->getPHID()) - ->setAuditorPHID($user->getPHID()) - ->setAuditStatus( - $status - ? $status - : PhabricatorAuditStatusConstants::AUDIT_NOT_REQUIRED) - ->setAuditReasons(array("Voluntary Participant")) - ->save(); - $requests[] = $request; + if (!$have_any_requests) { + $new_status = null; + switch ($action) { + case PhabricatorAuditActionConstants::COMMENT: + $new_status = PhabricatorAuditStatusConstants::AUDIT_NOT_REQUIRED; + break; + case PhabricatorAuditActionConstants::ACCEPT: + $new_status = PhabricatorAuditStatusConstants::ACCEPTED; + break; + case PhabricatorAuditActionConstants::CONCERN: + $new_status = PhabricatorAuditStatusConstants::CONCERNED; + break; + case PhabricatorAuditActionConstants::RESIGN: + // If you're on an audit because of a package, we write an explicit + // resign row to remove it from your queue. + $new_status = PhabricatorAuditStatusConstants::RESIGNED; + break; + case PhabricatorAuditActionConstants::CLOSE: + // Impossible to reach this block with 'close'. + default: + throw new Exception("Unknown or invalid action '{$action}'!"); + } + + $request = id(new PhabricatorRepositoryAuditRequest()) + ->setCommitPHID($commit->getPHID()) + ->setAuditorPHID($user->getPHID()) + ->setAuditStatus($new_status) + ->setAuditReasons(array("Voluntary Participant")) + ->save(); + $requests[] = $request; + } } $commit->updateAuditStatus($requests); @@ -206,6 +272,8 @@ final class PhabricatorAuditCommentEditor { $map = array( PhabricatorAuditActionConstants::CONCERN => 'Raised Concern', PhabricatorAuditActionConstants::ACCEPT => 'Accepted', + PhabricatorAuditActionConstants::RESIGN => 'Resigned', + PhabricatorAuditActionConstants::CLOSE => 'Closed', ); $verb = idx($map, $comment->getAction(), 'Commented On'); diff --git a/src/applications/audit/query/audit/PhabricatorAuditQuery.php b/src/applications/audit/query/audit/PhabricatorAuditQuery.php index 3c912441cf..b54bae8648 100644 --- a/src/applications/audit/query/audit/PhabricatorAuditQuery.php +++ b/src/applications/audit/query/audit/PhabricatorAuditQuery.php @@ -27,6 +27,8 @@ final class PhabricatorAuditQuery { private $needCommits; private $needCommitData; + private $awaitingUser; + private $status = 'status-any'; const STATUS_ANY = 'status-any'; const STATUS_OPEN = 'status-open'; @@ -43,6 +45,11 @@ final class PhabricatorAuditQuery { return $this; } + public function withAwaitingUser(PhabricatorUser $user) { + $this->awaitingUser = $user; + return $this; + } + public function withStatus($status) { $this->status = $status; return $this; @@ -72,14 +79,16 @@ final class PhabricatorAuditQuery { $table = new PhabricatorRepositoryAuditRequest(); $conn_r = $table->establishConnection('r'); + $joins = $this->buildJoinClause($conn_r); $where = $this->buildWhereClause($conn_r); $order = $this->buildOrderClause($conn_r); $limit = $this->buildLimitClause($conn_r); $data = queryfx_all( $conn_r, - 'SELECT * FROM %T %Q %Q %Q', + 'SELECT req.* FROM %T req %Q %Q %Q %Q', $table->getTableName(), + $joins, $where, $order, $limit); @@ -112,34 +121,77 @@ final class PhabricatorAuditQuery { return $this->commits; } + private function buildJoinClause($conn_r) { + + $joins = array(); + + if ($this->awaitingUser) { + // Join the request table on the awaiting user's requests, so we can + // filter out package and project requests which the user has resigned + // from. + $joins[] = qsprintf( + $conn_r, + 'LEFT JOIN %T awaiting ON req.commitPHID = awaiting.commitPHID AND + awaiting.auditorPHID = %s', + id(new PhabricatorRepositoryAuditRequest())->getTableName(), + $this->awaitingUser->getPHID()); + + // Join the commit table so we can get the commit author into the result + // row and filter by it later. + $joins[] = qsprintf( + $conn_r, + 'JOIN %T commit ON req.commitPHID = commit.phid', + id(new PhabricatorRepositoryCommit())->getTableName()); + } + + if ($joins) { + return implode(' ', $joins); + } else { + return ''; + } + } + private function buildWhereClause($conn_r) { $where = array(); if ($this->commitPHIDs) { $where[] = qsprintf( $conn_r, - 'commitPHID IN (%Ls)', + 'req.commitPHID IN (%Ls)', $this->commitPHIDs); } if ($this->auditorPHIDs) { $where[] = qsprintf( $conn_r, - 'auditorPHID IN (%Ls)', + 'req.auditorPHID IN (%Ls)', $this->auditorPHIDs); } + if ($this->awaitingUser) { + // Exclude package and project audits associated with commits where + // the user is the author. + $where[] = qsprintf( + $conn_r, + '(commit.authorPHID IS NULL OR commit.authorPHID != %s) + OR (req.auditorPHID = %s)', + $this->awaitingUser->getPHID(), + $this->awaitingUser->getPHID()); + } + $status = $this->status; switch ($status) { case self::STATUS_OPEN: $where[] = qsprintf( $conn_r, - 'auditStatus in (%Ls)', - array( - PhabricatorAuditStatusConstants::AUDIT_REQUIRED, - PhabricatorAuditStatusConstants::CONCERNED, - PhabricatorAuditStatusConstants::AUDIT_REQUESTED, - )); + 'req.auditStatus in (%Ls)', + PhabricatorAuditStatusConstants::getOpenStatusConstants()); + if ($this->awaitingUser) { + $where[] = qsprintf( + $conn_r, + 'awaiting.auditStatus IS NULL OR awaiting.auditStatus != %s', + PhabricatorAuditStatusConstants::RESIGNED); + } break; case self::STATUS_ANY: break; @@ -169,7 +221,7 @@ final class PhabricatorAuditQuery { } private function buildOrderClause($conn_r) { - return 'ORDER BY id DESC'; + return 'ORDER BY req.id DESC'; } } diff --git a/src/applications/audit/query/audit/__init__.php b/src/applications/audit/query/audit/__init__.php index 332df08a56..5f3ed4ad7a 100644 --- a/src/applications/audit/query/audit/__init__.php +++ b/src/applications/audit/query/audit/__init__.php @@ -9,6 +9,7 @@ phutil_require_module('phabricator', 'applications/audit/constants/status'); phutil_require_module('phabricator', 'applications/audit/query/commit'); phutil_require_module('phabricator', 'applications/repository/storage/auditrequest'); +phutil_require_module('phabricator', 'applications/repository/storage/commit'); phutil_require_module('phabricator', 'storage/qsprintf'); phutil_require_module('phabricator', 'storage/queryfx'); diff --git a/src/applications/audit/view/list/PhabricatorAuditListView.php b/src/applications/audit/view/list/PhabricatorAuditListView.php index 75bb642316..b98a016bac 100644 --- a/src/applications/audit/view/list/PhabricatorAuditListView.php +++ b/src/applications/audit/view/list/PhabricatorAuditListView.php @@ -23,6 +23,8 @@ final class PhabricatorAuditListView extends AphrontView { private $authorityPHIDs = array(); private $noDataString; private $commits; + private $user; + private $showDescriptions = true; public function setAudits(array $audits) { $this->audits = $audits; @@ -53,6 +55,16 @@ final class PhabricatorAuditListView extends AphrontView { return $this; } + public function setUser(PhabricatorUser $user) { + $this->user = $user; + return $this; + } + + public function setShowDescriptions($show_descriptions) { + $this->showDescriptions = $show_descriptions; + return $this; + } + public function getRequiredHandlePHIDs() { $phids = array(); foreach ($this->audits as $audit) { @@ -84,6 +96,7 @@ final class PhabricatorAuditListView extends AphrontView { } public function render() { + $user = $this->user; $authority = array_fill_keys($this->authorityPHIDs, true); @@ -120,11 +133,24 @@ final class PhabricatorAuditListView extends AphrontView { $reasons, ); - if (empty($authority[$audit->getAuditorPHID()])) { - $rowc[] = null; - } else { - $rowc[] = 'highlighted'; + $row_class = null; + + $has_authority = !empty($authority[$audit->getAuditorPHID()]); + if ($has_authority) { + $commit_author = $this->commits[$commit_phid]->getAuthorPHID(); + + // You don't have authority over package and project audits on your own + // commits. + + $auditor_is_user = ($audit->getAuditorPHID() == $user->getPHID()); + $user_is_author = ($commit_author == $user->getPHID()); + + if ($auditor_is_user || !$user_is_author) { + $row_class = 'highlighted'; + } } + + $rowc[] = $row_class; } $table = new AphrontTableView($rows); @@ -139,16 +165,16 @@ final class PhabricatorAuditListView extends AphrontView { $table->setColumnClasses( array( 'pri', - (($this->commits === null) ? '' : 'wide'), + ($this->showDescriptions ? 'wide' : ''), '', '', - (($this->commits === null) ? 'wide' : ''), + ($this->showDescriptions ? '' : 'wide'), )); $table->setRowClasses($rowc); $table->setColumnVisibility( array( - true, - ($this->commits !== null), + $this->showDescriptions, + $this->showDescriptions, true, true, true, diff --git a/src/applications/diffusion/controller/commit/DiffusionCommitController.php b/src/applications/diffusion/controller/commit/DiffusionCommitController.php index 27153582c4..2e78adb9ff 100644 --- a/src/applications/diffusion/controller/commit/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/commit/DiffusionCommitController.php @@ -20,6 +20,8 @@ final class DiffusionCommitController extends DiffusionController { const CHANGES_LIMIT = 100; + private $auditAuthorityPHIDs; + public function willProcessRequest(array $data) { // This controller doesn't use blob/path stuff, just pass the dictionary // in directly instead of using the AphrontRequest parsing mechanism. @@ -50,6 +52,7 @@ final class DiffusionCommitController extends DiffusionController { } $commit_data = $drequest->loadCommitData(); + $commit->attachCommitData($commit_data); $is_foreign = $commit_data->getCommitDetail('foreign-svn-stub'); if ($is_foreign) { @@ -93,7 +96,14 @@ final class DiffusionCommitController extends DiffusionController { $content[] = $detail_panel; } - $content[] = $this->buildAuditTable($commit); + $query = new PhabricatorAuditQuery(); + $query->withCommitPHIDs(array($commit->getPHID())); + $audit_requests = $query->execute(); + + $this->auditAuthorityPHIDs = + PhabricatorAuditCommentEditor::loadAuditPHIDsForUser($user); + + $content[] = $this->buildAuditTable($commit, $audit_requests); $content[] = $this->buildComments($commit); $change_query = DiffusionPathChangeQuery::newFromDiffusionRequest( @@ -247,7 +257,7 @@ final class DiffusionCommitController extends DiffusionController { $content[] = $change_list; } - $content[] = $this->buildAddCommentView($commit); + $content[] = $this->buildAddCommentView($commit, $audit_requests); return $this->buildStandardPageResponse( $content, @@ -331,21 +341,19 @@ final class DiffusionCommitController extends DiffusionController { ''; } - private function buildAuditTable($commit) { + private function buildAuditTable($commit, $audits) { $user = $this->getRequest()->getUser(); - $query = new PhabricatorAuditQuery(); - $query->withCommitPHIDs(array($commit->getPHID())); - $audits = $query->execute(); - $view = new PhabricatorAuditListView(); $view->setAudits($audits); + $view->setCommits(array($commit)); + $view->setUser($user); + $view->setShowDescriptions(false); $phids = $view->getRequiredHandlePHIDs(); $handles = id(new PhabricatorObjectHandleData($phids))->loadHandles(); $view->setHandles($handles); - $view->setAuthorityPHIDs( - PhabricatorAuditCommentEditor::loadAuditPHIDsForUser($user)); + $view->setAuthorityPHIDs($this->auditAuthorityPHIDs); $panel = new AphrontPanelView(); $panel->setHeader('Audits'); @@ -387,7 +395,7 @@ final class DiffusionCommitController extends DiffusionController { return $view; } - private function buildAddCommentView($commit) { + private function buildAddCommentView($commit, array $audit_requests) { $user = $this->getRequest()->getUser(); $is_serious = PhabricatorEnv::getEnvConfig('phabricator.serious-business'); @@ -409,6 +417,8 @@ final class DiffusionCommitController extends DiffusionController { $draft = null; } + $actions = $this->getAuditActions($commit, $audit_requests); + $form = id(new AphrontFormView()) ->setUser($user) ->setAction('/audit/addcomment/') @@ -418,7 +428,7 @@ final class DiffusionCommitController extends DiffusionController { ->setLabel('Action') ->setName('action') ->setID('audit-action') - ->setOptions(PhabricatorAuditActionConstants::getActionNameMap())) + ->setOptions($actions)) ->appendChild( id(new AphrontFormTextAreaControl()) ->setLabel('Comments') @@ -466,6 +476,80 @@ final class DiffusionCommitController extends DiffusionController { return $view; } + + /** + * Return a map of available audit actions for rendering into a