revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-23 14:00:56 +01:00
Code Issues Releases Wiki Activity

Add "Resign from Audit" and "Close Audit" actions to Diffusion

Browse source 
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
This commit is contained in:
epriestley 2012-03-26 09:44:06 -07:00
parent 638627cea5
commit 2044e51206
13 changed files with 339 additions and 86 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

20
src/applications/audit/constants/action/PhabricatorAuditActionConstants.php
View file

@ -21,33 +21,35 @@ final class PhabricatorAuditActionConstants {
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;
}
}

2
src/applications/audit/constants/action/__init__.php
View file

@ -6,8 +6,6 @@
phutil_require_module('phabricator', 'applications/audit/constants/status');
phutil_require_module('phutil', 'utils');

16
src/applications/audit/constants/status/PhabricatorAuditStatusConstants.php
View file

@ -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());
}
}

7
src/applications/audit/controller/list/PhabricatorAuditListController.php
View file

@ -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':

90
src/applications/audit/editor/comment/PhabricatorAuditCommentEditor.php
View file

@ -83,37 +83,103 @@ 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".
// 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;
if ($status) {
$request->setAuditStatus($status);
$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 (!$have_any_requests) {
// If the user has no current authority over any audit trigger, make a
// new one to represent their audit state.
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(
$status
? $status
: PhabricatorAuditStatusConstants::AUDIT_NOT_REQUIRED)
->setAuditStatus($new_status)
->setAuditReasons(array("Voluntary Participant"))
->save();
$requests[] = $request;
}
}
$commit->updateAuditStatus($requests);
$commit->save();
@ -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');

72
src/applications/audit/query/audit/PhabricatorAuditQuery.php
View file

@ -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';
}
}

1
src/applications/audit/query/audit/__init__.php
View file

@ -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');

42
src/applications/audit/view/list/PhabricatorAuditListView.php
View file

@ -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,13 +133,26 @@ 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);
$table->setHeaders(
array(
@ -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,

106
src/applications/diffusion/controller/commit/DiffusionCommitController.php
View file

@ -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 {
'</table>';
}
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 <select />.
* This shows the user valid actions, and does not show nonsense/invalid
* actions (like closing an already-closed commit, or resigning from a commit
* you have no association with).
*/
private function getAuditActions(
PhabricatorRepositoryCommit $commit,
array $audit_requests) {
$user = $this->getRequest()->getUser();
$user_is_author = ($commit->getAuthorPHID() == $user->getPHID());
$user_request = null;
foreach ($audit_requests as $audit_request) {
if ($audit_request->getAuditorPHID() == $user->getPHID()) {
$user_request = $audit_request;
break;
}
}
$actions = array();
$actions[PhabricatorAuditActionConstants::COMMENT] = true;
// We allow you to accept your own commits. A use case here is that you
// notice an issue with your own commit and "Raise Concern" as an indicator
// to other auditors that you're on top of the issue, then later resolve it
// and "Accept". You can not accept on behalf of projects or packages,
// however.
$actions[PhabricatorAuditActionConstants::ACCEPT] = true;
$actions[PhabricatorAuditActionConstants::CONCERN] = true;
// To resign, a user must have authority on some request and not be the
// commit's author.
if (!$user_is_author) {
$may_resign = false;
foreach ($audit_requests as $request) {
if (empty($this->auditAuthorityPHIDs[$request->getAuditorPHID()])) {
continue;
}
$may_resign = true;
break;
}
// If the user has already resigned, don't show "Resign...".
$status_resigned = PhabricatorAuditStatusConstants::RESIGNED;
if ($user_request) {
if ($user_request->getAuditStatus() == $status_resigned) {
$may_resign = false;
}
}
if ($may_resign) {
$actions[PhabricatorAuditActionConstants::RESIGN] = true;
}
}
$status_concern = PhabricatorAuditCommitStatusConstants::CONCERN_RAISED;
$concern_raised = ($commit->getAuditStatus() == $status_concern);
if ($user_is_author && $concern_raised) {
$actions[PhabricatorAuditActionConstants::CLOSE] = true;
}
foreach ($actions as $constant => $ignored) {
$actions[$constant] =
PhabricatorAuditActionConstants::getActionName($constant);
}
return $actions;
}
private function buildMergesTable(PhabricatorRepositoryCommit $commit) {
$drequest = $this->getDiffusionRequest();

1
src/applications/diffusion/controller/commit/__init__.php
View file

@ -8,6 +8,7 @@
phutil_require_module('phabricator', 'applications/audit/constants/action');
phutil_require_module('phabricator', 'applications/audit/constants/commitstatus');
phutil_require_module('phabricator', 'applications/audit/constants/status');
phutil_require_module('phabricator', 'applications/audit/editor/comment');
phutil_require_module('phabricator', 'applications/audit/query/audit');
phutil_require_module('phabricator', 'applications/audit/storage/auditcomment');

20
src/applications/diffusion/view/comment/DiffusionCommentView.php
View file

@ -110,19 +110,11 @@ final class DiffusionCommentView extends AphrontView {
$author = $this->getHandle($comment->getActorPHID());
$author_link = $author->renderLink();
$action = $comment->getAction();
$verb = PhabricatorAuditActionConstants::getActionPastTenseVerb($action);
$actions = array();
switch ($comment->getAction()) {
case PhabricatorAuditActionConstants::ACCEPT:
$actions[] = "{$author_link} accepted this commit.";
break;
case PhabricatorAuditActionConstants::CONCERN:
$actions[] = "{$author_link} raised concerns with this commit.";
break;
case PhabricatorAuditActionConstants::COMMENT:
default:
$actions[] = "{$author_link} commented on this commit.";
break;
}
$actions[] = "{$author_link} ".phutil_escape_html($verb)." this commit.";
foreach ($actions as $key => $action) {
$actions[$key] = '<div>'.$action.'</div>';
@ -135,12 +127,16 @@ final class DiffusionCommentView extends AphrontView {
$comment = $this->comment;
$engine = $this->getEngine();
if (!strlen($comment->getContent())) {
return null;
} else {
return
'<div class="phabricator-remarkup">'.
$engine->markupText($comment->getContent()).
$this->renderSingleView($this->renderInlines()).
'</div>';
}
}
private function renderInlines() {
if (!$this->inlineComments) {

1
src/applications/diffusion/view/comment/__init__.php
View file

@ -12,6 +12,7 @@ phutil_require_module('phabricator', 'infrastructure/diff/view/inline');
phutil_require_module('phabricator', 'view/base');
phutil_require_module('phabricator', 'view/layout/transaction');
phutil_require_module('phutil', 'markup');
phutil_require_module('phutil', 'utils');

1
src/applications/directory/controller/main/PhabricatorDirectoryMainController.php
View file

@ -576,6 +576,7 @@ final class PhabricatorDirectoryMainController
$query = new PhabricatorAuditQuery();
$query->withAuditorPHIDs($phids);
$query->withStatus(PhabricatorAuditQuery::STATUS_OPEN);
$query->withAwaitingUser($user);
$query->needCommitData(true);
$query->setLimit(10);