revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-30 02:32:42 +01:00
Code Issues Releases Wiki Activity

Allow Herald to trigger audits for users or projects

Browse source 
Summary:
Allows you to write a commit rule that triggers an audit by a user (personal
rules) or a project (global rules).

Mostly this is trying to make auditing more lightweight and accessible in
environments where setting up Owners packages doesn't make sense.

For instance, Disqus wants a rule like "trigger an audit for everything that
didn't have a Differential revision". While not necessarily scalable, this is a
perfectly reasonable rule for a small company, but a lot of work to implement
with Owners (and you'll get a lot of collateral damage if you don't make every
committer a project owner).

Instead, they can create a project called 'Unreviewed Commits' and write a rule
like:

	- When: Differential revision does not exist
 	- Action: Trigger an Audit for project: "Unreviewed Commits"

Then whoever cares can join that project and they'll see those audits in their
queue, and when they approve/raise on commits their actions will affect the
project audit.

Similarly, if I want to look at all commits that match some other rule (say,
XSS) but only want to do it like once a month, I can just set up an audit rule
and go through the queue when I feel like it.

NOTE: This abuses the 'packagePHID' field to also store user and project PHIDs.
Through the magic of handles, this (apparently) works fine for now; I'll do a
big schema patch soon but have several other edits I want to make at the same
time.

Also:

	- Adds an "active" fiew for /audit/, eventually this will be like the
Differential "active" view (stuff that is relevant to you right now).
	- On commits, highlight triggered audits you are responsible for.

Test Plan: Added personal and global audit triggers to Herald, reparsed some
commits with --herald, got audits. Browsed all audit interfaces to make sure
nothing exploded. Viewed a commit where I was responsible for only some audits.
Performed audits and made sure the triggers I am supposed to be responsible for
updated properly.

Reviewers: btrahan, jungejason

Reviewed By: jungejason

CC: aran, epriestley

Maniphest Tasks: T904

Differential Revision: https://secure.phabricator.com/D1690
This commit is contained in:
epriestley 2012-02-27 09:36:30 -08:00
parent 08dd3bc1d9
commit 1094527072
17 changed files with 163 additions and 21 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php
View file

@ -348,7 +348,6 @@ class AphrontDefaultApplicationConfiguration
'/audit/' => array(
'$' => 'PhabricatorAuditListController',
'view/(?P<filter>[^/]+)/$' => 'PhabricatorAuditListController',
'edit/$' => 'PhabricatorAuditEditController',
'addcomment/$' => 'PhabricatorAuditAddCommentController',
),

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

@ -26,13 +26,16 @@ final class PhabricatorAuditListController extends PhabricatorAuditController {
public function processRequest() {
$request = $this->getRequest();
$user = $request->getUser();
$nav = new AphrontSideNavFilterView();
$nav->setBaseURI(new PhutilURI('/audit/view/'));
$nav->addLabel('Active');
$nav->addFilter('active', 'Need Attention');
$nav->addLabel('Audits');
$nav->addFilter('all', 'All');
$nav->addFilter('all', 'All');
$this->filter = $nav->selectFilter($this->filter, 'all');
$this->filter = $nav->selectFilter($this->filter, 'active');
$pager = new AphrontPagerView();
$pager->setURI($request->getRequestURI(), 'offset');
@ -41,6 +44,16 @@ final class PhabricatorAuditListController extends PhabricatorAuditController {
$query->setOffset($pager->getOffset());
$query->setLimit($pager->getPageSize() + 1);
switch ($this->filter) {
case 'all':
break;
case 'active':
$phids = PhabricatorAuditCommentEditor::loadAuditPHIDsForUser($user);
$query->withAuditorPHIDs($phids);
$query->withStatus(PhabricatorAuditQuery::STATUS_OPEN);
break;
}
$audits = $query->execute();
$audits = $pager->sliceResults($audits);

1
src/applications/audit/controller/list/__init__.php
View file

@ -7,6 +7,7 @@
phutil_require_module('phabricator', 'applications/audit/controller/base');
phutil_require_module('phabricator', 'applications/audit/editor/comment');
phutil_require_module('phabricator', 'applications/audit/query/audit');
phutil_require_module('phabricator', 'applications/audit/view/list');
phutil_require_module('phabricator', 'applications/phid/handle/data');

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

@ -103,7 +103,7 @@ final class PhabricatorAuditCommentEditor {
// The user can audit on behalf of all projects they are a member of.
$query = new PhabricatorProjectQuery();
$query->setMembers(array($user->getPHID));
$query->setMembers(array($user->getPHID()));
$projects = $query->execute();
foreach ($projects as $project) {
$phids[$project->getPHID()] = true;

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

@ -21,13 +21,28 @@ final class PhabricatorAuditQuery {
private $offset;
private $limit;
private $auditorPHIDs;
private $commitPHIDs;
private $status = 'status-any';
const STATUS_ANY = 'status-any';
const STATUS_OPEN = 'status-open';
public function withCommitPHIDs(array $commit_phids) {
$this->commitPHIDs = $commit_phids;
return $this;
}
public function withAuditorPHIDs(array $auditor_phids) {
$this->auditorPHIDs = $auditor_phids;
return $this;
}
public function withStatus($status) {
$this->status = $status;
return $this;
}
public function setOffset($offset) {
$this->offset = $offset;
return $this;
@ -67,6 +82,30 @@ final class PhabricatorAuditQuery {
$this->commitPHIDs);
}
if ($this->auditorPHIDs) {
$where[] = qsprintf(
$conn_r,
'packagePHID IN (%Ls)',
$this->auditorPHIDs);
}
$status = $this->status;
switch ($status) {
case self::STATUS_OPEN:
$where[] = qsprintf(
$conn_r,
'auditStatus in (%Ls)',
array(
PhabricatorAuditStatusConstants::AUDIT_REQUIRED,
PhabricatorAuditStatusConstants::CONCERNED,
));
break;
case self::STATUS_ANY:
break;
default:
throw new Exception("Unknown status '{$status}'!");
}
if ($where) {
$where = 'WHERE ('.implode(') AND (', $where).')';
} else {

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

@ -6,6 +6,7 @@
phutil_require_module('phabricator', 'applications/audit/constants/status');
phutil_require_module('phabricator', 'applications/owners/storage/packagecommitrelationship');
phutil_require_module('phabricator', 'storage/qsprintf');
phutil_require_module('phabricator', 'storage/queryfx');

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

@ -20,6 +20,7 @@ final class PhabricatorAuditListView extends AphrontView {
private $audits;
private $handles;
private $authorityPHIDs = array();
public function setAudits(array $audits) {
$this->audits = $audits;
@ -31,6 +32,11 @@ final class PhabricatorAuditListView extends AphrontView {
return $this;
}
public function setAuthorityPHIDs(array $phids) {
$this->authorityPHIDs = $phids;
return $this;
}
public function getRequiredHandlePHIDs() {
$phids = array();
foreach ($this->audits as $audit) {
@ -50,6 +56,10 @@ final class PhabricatorAuditListView extends AphrontView {
public function render() {
$authority = array_fill_keys($this->authorityPHIDs, true);
$rowc = array();
$last = null;
$rows = array();
foreach ($this->audits as $audit) {
@ -77,6 +87,12 @@ final class PhabricatorAuditListView extends AphrontView {
phutil_escape_html($status),
$reasons,
);
if (empty($authority[$audit->getPackagePHID()])) {
$rowc[] = null;
} else {
$rowc[] = 'highlighted';
}
}
$table = new AphrontTableView($rows);
@ -94,7 +110,7 @@ final class PhabricatorAuditListView extends AphrontView {
'',
'wide',
));
$table->setRowClasses($rowc);
return $table->render();
}

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

@ -296,6 +296,8 @@ class DiffusionCommitController extends DiffusionController {
}
private function buildAuditTable($commit) {
$user = $this->getRequest()->getUser();
$query = new PhabricatorAuditQuery();
$query->withCommitPHIDs(array($commit->getPHID()));
$audits = $query->execute();
@ -306,6 +308,8 @@ class DiffusionCommitController extends DiffusionController {
$phids = $view->getRequiredHandlePHIDs();
$handles = id(new PhabricatorObjectHandleData($phids))->loadHandles();
$view->setHandles($handles);
$view->setAuthorityPHIDs(
PhabricatorAuditCommentEditor::loadAuditPHIDsForUser($user));
$panel = new AphrontPanelView();
$panel->setHeader('Audits');

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

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

18
src/applications/herald/adapter/commit/HeraldCommitAdapter.php
View file

@ -26,6 +26,7 @@ class HeraldCommitAdapter extends HeraldObjectAdapter {
protected $commitData;
protected $emailPHIDs = array();
protected $auditMap = array();
protected $affectedPaths;
protected $affectedRevision;
@ -50,6 +51,10 @@ class HeraldCommitAdapter extends HeraldObjectAdapter {
return array_keys($this->emailPHIDs);
}
public function getAuditMap() {
return $this->auditMap;
}
public function getHeraldName() {
return
'r'.
@ -185,6 +190,7 @@ class HeraldCommitAdapter extends HeraldObjectAdapter {
}
public function applyHeraldEffects(array $effects) {
$result = array();
foreach ($effects as $effect) {
$action = $effect->getAction();
@ -196,14 +202,22 @@ class HeraldCommitAdapter extends HeraldObjectAdapter {
'Great success at doing nothing.');
break;
case HeraldActionConfig::ACTION_EMAIL:
foreach ($effect->getTarget() as $fbid) {
$this->emailPHIDs[$fbid] = true;
foreach ($effect->getTarget() as $phid) {
$this->emailPHIDs[$phid] = true;
}
$result[] = new HeraldApplyTranscript(
$effect,
true,
'Added address to email targets.');
break;
case HeraldActionConfig::ACTION_AUDIT:
foreach ($effect->getTarget() as $phid) {
if (empty($this->auditMap[$phid])) {
$this->auditMap[$phid] = array();
}
$this->auditMap[$phid][] = $effect->getRuleID();
}
break;
default:
throw new Exception("No rules to handle action '{$action}'.");
}

5
src/applications/herald/config/action/HeraldActionConfig.php
View file

@ -22,6 +22,7 @@ class HeraldActionConfig {
const ACTION_REMOVE_CC = 'remcc';
const ACTION_EMAIL = 'email';
const ACTION_NOTHING = 'nothing';
const ACTION_AUDIT = 'audit';
public static function getActionMessageMapForRuleType($rule_type) {
$generic_mappings =
@ -36,6 +37,7 @@ class HeraldActionConfig {
self::ACTION_ADD_CC => 'Add emails to CC',
self::ACTION_REMOVE_CC => 'Remove emails from CC',
self::ACTION_EMAIL => 'Send an email to',
self::ACTION_AUDIT => 'Trigger an Audit for project',
);
break;
case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
@ -44,6 +46,7 @@ class HeraldActionConfig {
self::ACTION_ADD_CC => 'CC me',
self::ACTION_REMOVE_CC => 'Remove me from CC',
self::ACTION_EMAIL => 'Email me',
self::ACTION_AUDIT => 'Trigger an Audit by me',
);
break;
default:
@ -70,6 +73,7 @@ class HeraldActionConfig {
$map,
array(
self::ACTION_EMAIL,
self::ACTION_AUDIT,
self::ACTION_NOTHING,
));
case HeraldContentTypeConfig::CONTENT_TYPE_MERGE:
@ -112,6 +116,7 @@ class HeraldActionConfig {
case HeraldActionConfig::ACTION_EMAIL:
case HeraldActionConfig::ACTION_ADD_CC:
case HeraldActionConfig::ACTION_REMOVE_CC:
case HeraldActionConfig::ACTION_AUDIT:
$data[1] = array($author_phid => $author_phid);
break;
case HeraldActionConfig::ACTION_NOTHING:

3
src/applications/herald/config/valuetype/HeraldValueTypeConfig.php
View file

@ -26,6 +26,7 @@ class HeraldValueTypeConfig {
const VALUE_RULE = 'rule';
const VALUE_REPOSITORY = 'repository';
const VALUE_OWNERS_PACKAGE = 'package';
const VALUE_PROJECT = 'project';
public static function getValueTypeForFieldAndCondition($field, $condition) {
switch ($condition) {
@ -89,6 +90,8 @@ class HeraldValueTypeConfig {
return self::VALUE_EMAIL;
case HeraldActionConfig::ACTION_NOTHING:
return self::VALUE_NONE;
case HeraldActionConfig::ACTION_AUDIT:
return self::VALUE_PROJECT;
default:
throw new Exception("Unknown action '{$action}'.");
}

5
src/applications/herald/controller/rule/HeraldRuleController.php
View file

@ -582,10 +582,7 @@ class HeraldRuleController extends HeraldController {
'user' => '/typeahead/common/users/',
'repository' => '/typeahead/common/repositories/',
'package' => '/typeahead/common/packages/',
/*
'tag' => '/datasource/tag/',
*/
'project' => '/typeahead/common/projects/',
),
'markup' => $template,
);

2
src/applications/herald/storage/rule/HeraldRule.php
View file

@ -28,7 +28,7 @@ class HeraldRule extends HeraldDAO {
protected $repetitionPolicy;
protected $ruleType;
protected $configVersion = 8;
protected $configVersion = 9;
private $ruleApplied = array(); // phids for which this rule has been applied
private $invalidOwner = false;

48
src/applications/repository/worker/herald/PhabricatorRepositoryCommitHeraldWorker.php
View file

@ -23,10 +23,6 @@ class PhabricatorRepositoryCommitHeraldWorker
PhabricatorRepository $repository,
PhabricatorRepositoryCommit $commit) {
if ($repository->getDetail('herald-disabled')) {
return;
}
$data = id(new PhabricatorRepositoryCommitData())->loadOneWhere(
'commitID = %d',
$commit->getID());
@ -44,11 +40,21 @@ class PhabricatorRepositoryCommitHeraldWorker
$effects = $engine->applyRules($rules, $adapter);
$engine->applyEffects($effects, $adapter, $rules);
$audit_phids = $adapter->getAuditMap();
if ($audit_phids) {
$this->createAudits($commit, $audit_phids, $rules);
}
$email_phids = $adapter->getEmailPHIDs();
if (!$email_phids) {
return;
}
if ($repository->getDetail('herald-disabled')) {
// This just means "disable email"; audits are (mostly) idempotent.
return;
}
$xscript = $engine->getTranscript();
$commit_name = $adapter->getHeraldName();
@ -137,4 +143,38 @@ EOBODY;
$mailer->saveAndSend();
}
private function createAudits($commit, $map, $rules) {
$table = new PhabricatorOwnersPackageCommitRelationship();
$rships = $table->loadAllWhere(
'commitPHID = %s AND packagePHID IN (%Ls)',
$commit->getPHID(),
array_keys($map));
$rships = mpull($rships, null, 'getPackagePHID');
$rules = mpull($rules, null, 'getID');
foreach ($map as $phid => $rule_ids) {
$rship = idx($rships, $phid);
if ($rship) {
continue;
}
$reasons = array();
foreach ($rule_ids as $id) {
$rule_name = '?';
if ($rules[$id]) {
$rule_name = $rules[$id]->getName();
}
$reasons[] = 'Herald Rule #'.$id.' "'.$rule_name.'" Triggered Audit';
}
$rship = new PhabricatorOwnersPackageCommitRelationship();
$rship->setCommitPHID($commit->getPHID());
$rship->setPackagePHID($phid);
$rship->setAuditStatus(PhabricatorAuditStatusConstants::AUDIT_REQUIRED);
$rship->setAuditReasons($reasons);
$rship->save();
}
}
}

2
src/applications/repository/worker/herald/__init__.php
View file

@ -6,11 +6,13 @@
phutil_require_module('phabricator', 'applications/audit/constants/status');
phutil_require_module('phabricator', 'applications/herald/adapter/commit');
phutil_require_module('phabricator', 'applications/herald/config/contenttype');
phutil_require_module('phabricator', 'applications/herald/engine/engine');
phutil_require_module('phabricator', 'applications/herald/storage/rule');
phutil_require_module('phabricator', 'applications/metamta/storage/mail');
phutil_require_module('phabricator', 'applications/owners/storage/packagecommitrelationship');
phutil_require_module('phabricator', 'applications/phid/handle/data');
phutil_require_module('phabricator', 'applications/repository/storage/commitdata');
phutil_require_module('phabricator', 'applications/repository/worker/base');

17
webroot/rsrc/js/application/herald/HeraldRuleEditor.js
View file

@ -222,6 +222,7 @@ JX.install('HeraldRuleEditor', {
case 'repository':
case 'tag':
case 'package':
case 'project':
var tokenizer = this._newTokenizer(type);
input = tokenizer[0];
get_fn = tokenizer[1];
@ -243,13 +244,19 @@ JX.install('HeraldRuleEditor', {
},
_renderAuthorInput : function() {
var tokenizer = this._newTokenizer('email', 1);
input = tokenizer[0];
set_fn = tokenizer[2];
set_fn(this._config.author);
this._config.authorGetter = tokenizer[1];
var tokenizer = this._newTokenizer('email', 1);
input = tokenizer[0];
set_fn = tokenizer[2];
set_fn(this._config.author);
this._config.authorGetter = tokenizer[1];
try {
var author_cell = JX.$('author-input');
JX.DOM.setContent(author_cell, input);
} catch (ex) {
// TODO: Get rid of the ownership changing stuff or something?
// On global rules, there's no "author" input. Not fixing this
// properly because I think I'm going to nuke it soon.
}
},
_renderValueInputForRow : function(row_id) {