2011-01-26 02:17:19 +01:00
|
|
|
<?php
|
|
|
|
|
|
|
|
/**
|
|
|
|
* Handle major edit operations to DifferentialRevision -- adding and removing
|
|
|
|
* reviewers, diffs, and CCs. Unlike simple edits, these changes trigger
|
|
|
|
* complicated email workflows.
|
|
|
|
*/
|
2012-10-10 19:18:23 +02:00
|
|
|
final class DifferentialRevisionEditor extends PhabricatorEditor {
|
2011-01-26 02:17:19 +01:00
|
|
|
|
|
|
|
protected $revision;
|
|
|
|
|
|
|
|
protected $cc = null;
|
|
|
|
protected $reviewers = null;
|
|
|
|
protected $diff;
|
|
|
|
protected $comments;
|
|
|
|
protected $silentUpdate;
|
|
|
|
|
2011-08-10 22:46:01 +02:00
|
|
|
private $auxiliaryFields = array();
|
Track content sources (email, web, conduit, mobile) for replies
Summary:
When an object is updated, record the content source for the update. This mostly
isn't terribly useful but one concrete thing I want to do with it is let admins
audit via-email replies more easily since there are a bunch of options which let
you do hyjinx if you intentionally configure them insecurely. I think having a
little more auditability around this feature is generally good. At some point
I'm going to turn this into a link admins can click to see details.
It also allows us to see how frequently different mechanisms are used, and lets
you see if someone is at their desk or on a mobile or whatever, at least
indirectly.
The "tablet" and "mobile" sources are currently unused but I figured I'd throw
them in anyway. SMS support should definitely happen at some point.
Not 100% sure about the design for this, I might change it to plain text at some
point.
Test Plan: Updated objects and saw update sources rendered.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 844
2011-08-22 19:25:45 +02:00
|
|
|
private $contentSource;
|
2013-09-16 17:04:14 +02:00
|
|
|
private $isCreate;
|
|
|
|
private $aphrontRequestForEventDispatch;
|
|
|
|
|
|
|
|
|
|
|
|
public function setAphrontRequestForEventDispatch(AphrontRequest $request) {
|
|
|
|
$this->aphrontRequestForEventDispatch = $request;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
public function getAphrontRequestForEventDispatch() {
|
|
|
|
return $this->aphrontRequestForEventDispatch;
|
|
|
|
}
|
2011-08-10 22:46:01 +02:00
|
|
|
|
2012-10-10 19:18:23 +02:00
|
|
|
public function __construct(DifferentialRevision $revision) {
|
2011-01-26 02:17:19 +01:00
|
|
|
$this->revision = $revision;
|
2013-09-16 17:04:14 +02:00
|
|
|
$this->isCreate = !($revision->getID());
|
2011-01-26 02:17:19 +01:00
|
|
|
}
|
|
|
|
|
2011-02-06 22:49:23 +01:00
|
|
|
public static function newRevisionFromConduitWithDiff(
|
|
|
|
array $fields,
|
|
|
|
DifferentialDiff $diff,
|
2012-10-10 19:18:23 +02:00
|
|
|
PhabricatorUser $actor) {
|
2011-02-06 22:49:23 +01:00
|
|
|
|
2013-10-09 22:58:00 +02:00
|
|
|
$revision = DifferentialRevision::initializeNewRevision($actor);
|
2011-02-06 22:49:23 +01:00
|
|
|
$revision->setPHID($revision->generatePHID());
|
|
|
|
|
2012-10-10 19:18:23 +02:00
|
|
|
$editor = new DifferentialRevisionEditor($revision);
|
|
|
|
$editor->setActor($actor);
|
2013-05-31 02:33:17 +02:00
|
|
|
$editor->addDiff($diff, null);
|
2011-02-06 22:49:23 +01:00
|
|
|
$editor->copyFieldsFromConduit($fields);
|
|
|
|
|
|
|
|
$editor->save();
|
|
|
|
|
|
|
|
return $revision;
|
|
|
|
}
|
|
|
|
|
|
|
|
public function copyFieldsFromConduit(array $fields) {
|
|
|
|
|
2012-10-10 19:18:23 +02:00
|
|
|
$actor = $this->getActor();
|
2011-02-06 22:49:23 +01:00
|
|
|
$revision = $this->revision;
|
Drive commit message rendering from field specifications
Summary:
When rendering commit messages, drive all the logic through field specification
classes instead of the hard-coded DifferentialCommitMessageData class. This
removes DifferentialCommitMessageData and support classes.
Note that this effectively reverts D546, and will cause a minor break for
Facebook (Task IDs will no longer render in commit messages generated by "arc
amend", and will not be editable via "arc diff --edit"). This can be resolved by
implementing the feature as a custom field. While I've been able to preserve the
task ID functionality elsewhere, I felt this implementation was too complex to
reasonably leave hooks for, and the break is pretty minor.
Test Plan:
- Made numerous calls to differential.getcommitmessage across many diffs in
various states, with and without 'edit' and with and without various field
overrides.
- General behavior seems correct (messages look accurate, and have the
expected information). Special fields like "Reviewed By" and "git-svn-id" seem
to work correctly.
- Edit behavior seems correct (edit mode shows all editable fields, hides
fields like "Reviewed By").
- Field overwrite behavior seems correct (overwritable fields show the correct
values when overwritten, ignore provided values otherwise).
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 814
2011-08-16 06:06:58 +02:00
|
|
|
$revision->loadRelationships();
|
2011-02-06 22:49:23 +01:00
|
|
|
|
2013-05-31 02:33:17 +02:00
|
|
|
$all_fields = DifferentialFieldSelector::newSelector()
|
2011-08-15 03:52:09 +02:00
|
|
|
->getFieldSpecifications();
|
|
|
|
|
2013-05-31 02:33:17 +02:00
|
|
|
$aux_fields = array();
|
|
|
|
foreach ($all_fields as $aux_field) {
|
2011-08-15 03:52:09 +02:00
|
|
|
$aux_field->setRevision($revision);
|
2013-05-31 02:33:17 +02:00
|
|
|
$aux_field->setDiff($this->diff);
|
2012-10-10 19:18:23 +02:00
|
|
|
$aux_field->setUser($actor);
|
2013-05-31 02:33:17 +02:00
|
|
|
if ($aux_field->shouldAppearOnCommitMessage()) {
|
|
|
|
$aux_fields[$aux_field->getCommitMessageKey()] = $aux_field;
|
2011-08-15 03:52:09 +02:00
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
foreach ($fields as $field => $value) {
|
|
|
|
if (empty($aux_fields[$field])) {
|
|
|
|
throw new Exception(
|
|
|
|
"Parsed commit message contains unrecognized field '{$field}'.");
|
|
|
|
}
|
|
|
|
$aux_fields[$field]->setValueFromParsedCommitMessage($value);
|
|
|
|
}
|
|
|
|
|
2011-12-02 00:43:25 +01:00
|
|
|
foreach ($aux_fields as $aux_field) {
|
|
|
|
$aux_field->validateField();
|
|
|
|
}
|
|
|
|
|
2013-05-31 02:33:17 +02:00
|
|
|
$this->setAuxiliaryFields($all_fields);
|
2011-02-06 22:49:23 +01:00
|
|
|
}
|
|
|
|
|
2011-08-10 22:46:01 +02:00
|
|
|
public function setAuxiliaryFields(array $auxiliary_fields) {
|
2012-04-04 01:35:35 +02:00
|
|
|
assert_instances_of($auxiliary_fields, 'DifferentialFieldSpecification');
|
2011-08-10 22:46:01 +02:00
|
|
|
$this->auxiliaryFields = $auxiliary_fields;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
2011-01-26 02:17:19 +01:00
|
|
|
public function getRevision() {
|
|
|
|
return $this->revision;
|
|
|
|
}
|
|
|
|
|
|
|
|
public function setReviewers(array $reviewers) {
|
|
|
|
$this->reviewers = $reviewers;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
public function setCCPHIDs(array $cc) {
|
|
|
|
$this->cc = $cc;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
Track content sources (email, web, conduit, mobile) for replies
Summary:
When an object is updated, record the content source for the update. This mostly
isn't terribly useful but one concrete thing I want to do with it is let admins
audit via-email replies more easily since there are a bunch of options which let
you do hyjinx if you intentionally configure them insecurely. I think having a
little more auditability around this feature is generally good. At some point
I'm going to turn this into a link admins can click to see details.
It also allows us to see how frequently different mechanisms are used, and lets
you see if someone is at their desk or on a mobile or whatever, at least
indirectly.
The "tablet" and "mobile" sources are currently unused but I figured I'd throw
them in anyway. SMS support should definitely happen at some point.
Not 100% sure about the design for this, I might change it to plain text at some
point.
Test Plan: Updated objects and saw update sources rendered.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 844
2011-08-22 19:25:45 +02:00
|
|
|
public function setContentSource(PhabricatorContentSource $content_source) {
|
|
|
|
$this->contentSource = $content_source;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
2011-01-26 02:17:19 +01:00
|
|
|
public function addDiff(DifferentialDiff $diff, $comments) {
|
|
|
|
if ($diff->getRevisionID() &&
|
|
|
|
$diff->getRevisionID() != $this->getRevision()->getID()) {
|
|
|
|
$diff_id = (int)$diff->getID();
|
|
|
|
$targ_id = (int)$this->getRevision()->getID();
|
|
|
|
$real_id = (int)$diff->getRevisionID();
|
|
|
|
throw new Exception(
|
|
|
|
"Can not attach diff #{$diff_id} to Revision D{$targ_id}, it is ".
|
|
|
|
"already attached to D{$real_id}.");
|
|
|
|
}
|
|
|
|
$this->diff = $diff;
|
|
|
|
$this->comments = $comments;
|
2013-09-27 00:28:42 +02:00
|
|
|
|
|
|
|
$repository = id(new DifferentialRepositoryLookup())
|
|
|
|
->setViewer($this->getActor())
|
|
|
|
->setDiff($diff)
|
|
|
|
->lookupRepository();
|
|
|
|
|
|
|
|
if ($repository) {
|
|
|
|
$this->getRevision()->setRepositoryPHID($repository->getPHID());
|
|
|
|
}
|
|
|
|
|
2011-01-26 02:17:19 +01:00
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
protected function getDiff() {
|
|
|
|
return $this->diff;
|
|
|
|
}
|
|
|
|
|
|
|
|
protected function getComments() {
|
|
|
|
return $this->comments;
|
|
|
|
}
|
|
|
|
|
|
|
|
protected function getActorPHID() {
|
2012-10-11 23:34:16 +02:00
|
|
|
return $this->getActor()->getPHID();
|
2011-01-26 02:17:19 +01:00
|
|
|
}
|
|
|
|
|
|
|
|
public function isNewRevision() {
|
|
|
|
return !$this->getRevision()->getID();
|
|
|
|
}
|
|
|
|
|
|
|
|
/**
|
|
|
|
* A silent update does not trigger Herald rules or send emails. This is used
|
|
|
|
* for auto-amends at commit time.
|
|
|
|
*/
|
|
|
|
public function setSilentUpdate($silent) {
|
|
|
|
$this->silentUpdate = $silent;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
public function save() {
|
|
|
|
$revision = $this->getRevision();
|
|
|
|
|
|
|
|
$is_new = $this->isNewRevision();
|
|
|
|
|
|
|
|
$revision->loadRelationships();
|
|
|
|
|
2011-08-14 22:55:30 +02:00
|
|
|
$this->willWriteRevision();
|
|
|
|
|
2011-01-26 02:17:19 +01:00
|
|
|
if ($this->reviewers === null) {
|
|
|
|
$this->reviewers = $revision->getReviewers();
|
|
|
|
}
|
|
|
|
|
|
|
|
if ($this->cc === null) {
|
|
|
|
$this->cc = $revision->getCCPHIDs();
|
|
|
|
}
|
|
|
|
|
2012-11-29 08:01:07 +01:00
|
|
|
if ($is_new) {
|
|
|
|
$content_blocks = array();
|
|
|
|
foreach ($this->auxiliaryFields as $field) {
|
|
|
|
if ($field->shouldExtractMentions()) {
|
|
|
|
$content_blocks[] = $field->renderValueForCommitMessage(false);
|
|
|
|
}
|
|
|
|
}
|
|
|
|
$phids = PhabricatorMarkupEngine::extractPHIDsFromMentions(
|
|
|
|
$content_blocks);
|
|
|
|
$this->cc = array_unique(array_merge($this->cc, $phids));
|
|
|
|
}
|
|
|
|
|
2012-02-03 01:03:44 +01:00
|
|
|
$diff = $this->getDiff();
|
|
|
|
if ($diff) {
|
|
|
|
$revision->setLineCount($diff->getLineCount());
|
|
|
|
}
|
|
|
|
|
|
|
|
// Save the revision, to generate its ID and PHID if it is new. We need
|
|
|
|
// the ID/PHID in order to record them in Herald transcripts, but don't
|
|
|
|
// want to hold a transaction open while running Herald because it is
|
|
|
|
// potentially somewhat slow. The downside is that we may end up with a
|
|
|
|
// saved revision/diff pair without appropriate CCs. We could be better
|
|
|
|
// about this -- for example:
|
|
|
|
//
|
|
|
|
// - Herald can't affect reviewers, so we could compute them before
|
|
|
|
// opening the transaction and then save them in the transaction.
|
|
|
|
// - Herald doesn't *really* need PHIDs to compute its effects, we could
|
|
|
|
// run it before saving these objects and then hand over the PHIDs later.
|
|
|
|
//
|
|
|
|
// But this should address the problem of orphaned revisions, which is
|
|
|
|
// currently the only problem we experience in practice.
|
|
|
|
|
|
|
|
$revision->openTransaction();
|
|
|
|
|
2012-04-10 21:51:34 +02:00
|
|
|
if ($diff) {
|
|
|
|
$revision->setBranchName($diff->getBranch());
|
|
|
|
$revision->setArcanistProjectPHID($diff->getArcanistProjectPHID());
|
|
|
|
}
|
|
|
|
|
2012-02-03 01:03:44 +01:00
|
|
|
$revision->save();
|
2012-04-10 21:51:34 +02:00
|
|
|
|
2012-02-03 01:03:44 +01:00
|
|
|
if ($diff) {
|
|
|
|
$diff->setRevisionID($revision->getID());
|
|
|
|
$diff->save();
|
|
|
|
}
|
|
|
|
|
|
|
|
$revision->saveTransaction();
|
|
|
|
|
|
|
|
|
2011-01-26 02:17:19 +01:00
|
|
|
// We're going to build up three dictionaries: $add, $rem, and $stable. The
|
|
|
|
// $add dictionary has added reviewers/CCs. The $rem dictionary has
|
|
|
|
// reviewers/CCs who have been removed, and the $stable array is
|
|
|
|
// reviewers/CCs who haven't changed. We're going to send new reviewers/CCs
|
|
|
|
// a different ("welcome") email than we send stable reviewers/CCs.
|
|
|
|
|
|
|
|
$old = array(
|
|
|
|
'rev' => array_fill_keys($revision->getReviewers(), true),
|
|
|
|
'ccs' => array_fill_keys($revision->getCCPHIDs(), true),
|
|
|
|
);
|
|
|
|
|
|
|
|
$xscript_header = null;
|
|
|
|
$xscript_uri = null;
|
|
|
|
|
|
|
|
$new = array(
|
|
|
|
'rev' => array_fill_keys($this->reviewers, true),
|
|
|
|
'ccs' => array_fill_keys($this->cc, true),
|
|
|
|
);
|
|
|
|
|
|
|
|
$rem_ccs = array();
|
2012-03-26 18:55:38 +02:00
|
|
|
$xscript_phid = null;
|
2011-01-26 02:17:19 +01:00
|
|
|
if ($diff) {
|
2014-02-12 17:53:40 +01:00
|
|
|
$unsubscribed_phids = PhabricatorEdgeQuery::loadDestinationPHIDs(
|
|
|
|
$revision->getPHID(),
|
|
|
|
PhabricatorEdgeConfig::TYPE_OBJECT_HAS_UNSUBSCRIBER);
|
|
|
|
|
2013-08-02 17:55:13 +02:00
|
|
|
$adapter = HeraldDifferentialRevisionAdapter::newLegacyAdapter(
|
2011-04-08 03:55:43 +02:00
|
|
|
$revision,
|
|
|
|
$diff);
|
|
|
|
$adapter->setExplicitCCs($new['ccs']);
|
|
|
|
$adapter->setExplicitReviewers($new['rev']);
|
2014-02-12 17:53:40 +01:00
|
|
|
$adapter->setForbiddenCCs($unsubscribed_phids);
|
2014-02-04 21:43:31 +01:00
|
|
|
$adapter->setIsNewObject($is_new);
|
2011-04-08 03:55:43 +02:00
|
|
|
|
|
|
|
$xscript = HeraldEngine::loadAndApplyRules($adapter);
|
2012-07-17 04:01:43 +02:00
|
|
|
$xscript_uri = '/herald/transcript/'.$xscript->getID().'/';
|
2011-01-26 02:17:19 +01:00
|
|
|
$xscript_phid = $xscript->getPHID();
|
|
|
|
$xscript_header = $xscript->getXHeraldRulesHeader();
|
|
|
|
|
2011-08-16 22:05:12 +02:00
|
|
|
$xscript_header = HeraldTranscript::saveXHeraldRulesHeader(
|
2011-05-03 06:04:06 +02:00
|
|
|
$revision->getPHID(),
|
|
|
|
$xscript_header);
|
|
|
|
|
2011-01-26 02:17:19 +01:00
|
|
|
$sub = array(
|
2013-10-05 19:36:26 +02:00
|
|
|
'rev' => $adapter->getReviewersAddedByHerald(),
|
2011-04-08 03:55:43 +02:00
|
|
|
'ccs' => $adapter->getCCsAddedByHerald(),
|
2011-01-26 02:17:19 +01:00
|
|
|
);
|
2011-04-08 03:55:43 +02:00
|
|
|
$rem_ccs = $adapter->getCCsRemovedByHerald();
|
2013-10-07 02:09:24 +02:00
|
|
|
$blocking_reviewers = array_keys(
|
|
|
|
$adapter->getBlockingReviewersAddedByHerald());
|
2013-11-09 01:48:17 +01:00
|
|
|
|
|
|
|
HarbormasterBuildable::applyBuildPlans(
|
|
|
|
$diff->getPHID(),
|
|
|
|
$revision->getPHID(),
|
|
|
|
$adapter->getBuildPlans());
|
2011-01-26 02:17:19 +01:00
|
|
|
} else {
|
|
|
|
$sub = array(
|
|
|
|
'rev' => array(),
|
|
|
|
'ccs' => array(),
|
|
|
|
);
|
2013-10-07 02:09:24 +02:00
|
|
|
$blocking_reviewers = array();
|
2011-01-26 02:17:19 +01:00
|
|
|
}
|
|
|
|
|
|
|
|
// Remove any CCs which are prevented by Herald rules.
|
|
|
|
$sub['ccs'] = array_diff_key($sub['ccs'], $rem_ccs);
|
|
|
|
$new['ccs'] = array_diff_key($new['ccs'], $rem_ccs);
|
|
|
|
|
|
|
|
$add = array();
|
|
|
|
$rem = array();
|
|
|
|
$stable = array();
|
|
|
|
foreach (array('rev', 'ccs') as $key) {
|
|
|
|
$add[$key] = array();
|
|
|
|
if ($new[$key] !== null) {
|
|
|
|
$add[$key] += array_diff_key($new[$key], $old[$key]);
|
|
|
|
}
|
|
|
|
$add[$key] += array_diff_key($sub[$key], $old[$key]);
|
|
|
|
|
|
|
|
$combined = $sub[$key];
|
|
|
|
if ($new[$key] !== null) {
|
|
|
|
$combined += $new[$key];
|
|
|
|
}
|
|
|
|
$rem[$key] = array_diff_key($old[$key], $combined);
|
|
|
|
|
|
|
|
$stable[$key] = array_diff_key($old[$key], $add[$key] + $rem[$key]);
|
|
|
|
}
|
|
|
|
|
2013-10-05 19:36:26 +02:00
|
|
|
// Prevent Herald rules from adding a revision's owner as a reviewer.
|
|
|
|
unset($add['rev'][$revision->getAuthorPHID()]);
|
|
|
|
|
2013-07-10 22:45:14 +02:00
|
|
|
self::updateReviewers(
|
2011-01-26 02:17:19 +01:00
|
|
|
$revision,
|
2013-07-10 22:45:14 +02:00
|
|
|
$this->getActor(),
|
2011-01-26 02:17:19 +01:00
|
|
|
array_keys($add['rev']),
|
2013-07-10 22:45:14 +02:00
|
|
|
array_keys($rem['rev']),
|
2013-10-07 02:09:24 +02:00
|
|
|
$blocking_reviewers);
|
2011-01-26 02:17:19 +01:00
|
|
|
|
2012-03-26 18:55:38 +02:00
|
|
|
// We want to attribute new CCs to a "reasonPHID", representing the reason
|
|
|
|
// they were added. This is either a user (if some user explicitly CCs
|
|
|
|
// them, or uses "Add CCs...") or a Herald transcript PHID, indicating that
|
|
|
|
// they were added by a Herald rule.
|
2011-04-08 03:55:43 +02:00
|
|
|
|
2011-02-05 03:16:02 +01:00
|
|
|
if ($add['ccs'] || $rem['ccs']) {
|
2012-03-26 18:55:38 +02:00
|
|
|
$reasons = array();
|
|
|
|
foreach ($add['ccs'] as $phid => $ignored) {
|
|
|
|
if (empty($new['ccs'][$phid])) {
|
|
|
|
$reasons[$phid] = $xscript_phid;
|
2011-02-05 03:16:02 +01:00
|
|
|
} else {
|
2012-10-11 23:34:16 +02:00
|
|
|
$reasons[$phid] = $this->getActorPHID();
|
2011-02-05 03:16:02 +01:00
|
|
|
}
|
|
|
|
}
|
2012-03-26 18:55:38 +02:00
|
|
|
foreach ($rem['ccs'] as $phid => $ignored) {
|
|
|
|
if (empty($new['ccs'][$phid])) {
|
2012-10-11 23:34:16 +02:00
|
|
|
$reasons[$phid] = $this->getActorPHID();
|
2011-02-05 03:16:02 +01:00
|
|
|
} else {
|
2012-03-26 18:55:38 +02:00
|
|
|
$reasons[$phid] = $xscript_phid;
|
2011-02-05 03:16:02 +01:00
|
|
|
}
|
|
|
|
}
|
2012-03-26 18:55:38 +02:00
|
|
|
} else {
|
2012-10-11 23:34:16 +02:00
|
|
|
$reasons = $this->getActorPHID();
|
2011-02-05 03:16:02 +01:00
|
|
|
}
|
2012-03-26 18:55:38 +02:00
|
|
|
|
2011-02-05 03:16:02 +01:00
|
|
|
self::alterCCs(
|
|
|
|
$revision,
|
|
|
|
$this->cc,
|
|
|
|
array_keys($rem['ccs']),
|
|
|
|
array_keys($add['ccs']),
|
2012-03-26 18:55:38 +02:00
|
|
|
$reasons);
|
2011-02-05 03:16:02 +01:00
|
|
|
|
2011-08-10 22:46:01 +02:00
|
|
|
$this->updateAuxiliaryFields();
|
|
|
|
|
2011-05-28 00:52:26 +02:00
|
|
|
// Add the author and users included from Herald rules to the relevant set
|
|
|
|
// of users so they get a copy of the email.
|
2011-01-26 02:17:19 +01:00
|
|
|
if (!$this->silentUpdate) {
|
|
|
|
if ($is_new) {
|
|
|
|
$add['rev'][$this->getActorPHID()] = true;
|
2011-05-28 00:52:26 +02:00
|
|
|
if ($diff) {
|
|
|
|
$add['rev'] += $adapter->getEmailPHIDsAddedByHerald();
|
|
|
|
}
|
2011-01-26 02:17:19 +01:00
|
|
|
} else {
|
|
|
|
$stable['rev'][$this->getActorPHID()] = true;
|
2011-05-28 00:52:26 +02:00
|
|
|
if ($diff) {
|
|
|
|
$stable['rev'] += $adapter->getEmailPHIDsAddedByHerald();
|
|
|
|
}
|
2011-01-26 02:17:19 +01:00
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
$mail = array();
|
|
|
|
|
2011-02-10 07:46:18 +01:00
|
|
|
$phids = array($this->getActorPHID());
|
|
|
|
|
2013-09-11 21:27:28 +02:00
|
|
|
$handles = id(new PhabricatorHandleQuery())
|
2013-03-01 02:15:09 +01:00
|
|
|
->setViewer($this->getActor())
|
2013-09-11 21:27:28 +02:00
|
|
|
->withPHIDs($phids)
|
|
|
|
->execute();
|
2011-02-10 07:46:18 +01:00
|
|
|
$actor_handle = $handles[$this->getActorPHID()];
|
|
|
|
|
2011-01-26 02:17:19 +01:00
|
|
|
$changesets = null;
|
Make "reject" and "blocking reviewer" block acceptance in Differential
Summary:
Ref T1279. This is a logical change.
- "Reject" (nee "Request Changes") is now sticky. The review won't transition to "Accepted" until the reviewer clears their objection. In practice, I think it always worked like this anyway (without technical enforcement, users just followed this rule naturally, since disobeying this rule is kind of a dick move) so I don't expect this to change much. I think this rule is easier to understand than the old rule now, given the multi-reviewer status and blocking reviewers.
- "Blocking Reviewer" and "Reject" now prevent a revision from transitioning to "Accepted". When reviewers accept, resign, or are removed, we do a check to see if the reivsion has: at least one user reviewer who has accepted; zero rejects; and zero blocks. If all conditions are satisfied, we transition it to "accepted".
Practically, the primary net effect of this is just to make blocking reviews actually block.
This is pretty messy, but there's not much we can do about it until after T2222, since we have two completely separate editor pathways which are both responsible for adjusting status. Eventually, these can merge into a single sane editor which implements reasonable rules in reaonable ways. But that day is not today.
Test Plan: With three users and a project, made a bunch of accepts, rejects, resigns and reviewer removals. I think I probably covered most of the pathways? There are a lot of interactions here.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, wisutsak.jaisue.7
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7245
2013-10-07 02:09:56 +02:00
|
|
|
$old_status = $revision->getStatus();
|
|
|
|
|
2011-01-26 02:17:19 +01:00
|
|
|
if ($diff) {
|
|
|
|
$changesets = $diff->loadChangesets();
|
2011-02-05 02:53:14 +01:00
|
|
|
// TODO: This should probably be in DifferentialFeedbackEditor?
|
2011-01-26 02:17:19 +01:00
|
|
|
if (!$is_new) {
|
Allow Differential Mail to accept multiple comments for one mail
Summary:
Ref T2222. Currently, one `DifferentialComment` can do a lot of things (add ccs, reviewers, comments, inline comments, and perform state changes). In the future, each `ApplicationTransaction` does only one thing. This is the primary piece of complexity which makes the upcoming migration risky, because each comment needs to migrate into multiple transactions.
I want to mitigate this complexity as much as possible before the migration itself happens. One approach I'm going to use to do that is to start writing one comment per effect now, so the mapping is more direct when the migration itself happens and the write code can be straightforward (one row per save()) after the migration.
This tackles a small piece of that, which is the mail Differential sends. Currently, Differential mail acts on a single comment. Instead, allow it to act on a list of comments, but always give it one comment for now. In the future, we can hand it several comments instead and still get the expected behavior.
This change should have no impact on any application behaviors.
Test Plan:
- Commented;
- commented with inline;
- added reviewers;
- added CCs;
- added CCs via mentions;
- updated revision;
- looked at all the mail, all of which seemed sane.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8199
2014-02-12 00:21:07 +01:00
|
|
|
$this->createComment();
|
2011-01-26 02:17:19 +01:00
|
|
|
$mail[] = id(new DifferentialNewDiffMail(
|
|
|
|
$revision,
|
2011-02-10 07:46:18 +01:00
|
|
|
$actor_handle,
|
2011-01-26 02:17:19 +01:00
|
|
|
$changesets))
|
2013-03-01 02:15:09 +01:00
|
|
|
->setActor($this->getActor())
|
Allow Differential Mail to accept multiple comments for one mail
Summary:
Ref T2222. Currently, one `DifferentialComment` can do a lot of things (add ccs, reviewers, comments, inline comments, and perform state changes). In the future, each `ApplicationTransaction` does only one thing. This is the primary piece of complexity which makes the upcoming migration risky, because each comment needs to migrate into multiple transactions.
I want to mitigate this complexity as much as possible before the migration itself happens. One approach I'm going to use to do that is to start writing one comment per effect now, so the mapping is more direct when the migration itself happens and the write code can be straightforward (one row per save()) after the migration.
This tackles a small piece of that, which is the mail Differential sends. Currently, Differential mail acts on a single comment. Instead, allow it to act on a list of comments, but always give it one comment for now. In the future, we can hand it several comments instead and still get the expected behavior.
This change should have no impact on any application behaviors.
Test Plan:
- Commented;
- commented with inline;
- added reviewers;
- added CCs;
- added CCs via mentions;
- updated revision;
- looked at all the mail, all of which seemed sane.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8199
2014-02-12 00:21:07 +01:00
|
|
|
->setIsFirstMailAboutRevision(false)
|
|
|
|
->setIsFirstMailToRecipients(false)
|
|
|
|
->setCommentText($this->getComments())
|
2011-01-26 02:17:19 +01:00
|
|
|
->setToPHIDs(array_keys($stable['rev']))
|
|
|
|
->setCCPHIDs(array_keys($stable['ccs']));
|
|
|
|
}
|
|
|
|
|
|
|
|
// Save the changes we made above.
|
|
|
|
|
Use only the first line of update comment in revision history description
Summary:
More complicated updates deserves title and explanation.
This diff uses only the first line from comment in diff's description.
It also removes the truncating at 80 chars which looks as an error.
Test Plan:
var_dump(preg_replace('/\n.*/s', '', "abc"));
var_dump(preg_replace('/\n.*/s', '', "abc\ndef"));
var_dump(preg_replace('/\n.*/s', '', "abc\ndef"));
var_dump(preg_replace('/\n.*/s', '', "abc\ndef\nghi"));
var_dump(preg_replace('/\n.*/s', '', "\nabc")); // empty string
Display revision with 255 chars description in update history - it looks OK.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1812
2012-03-07 20:52:56 +01:00
|
|
|
$diff->setDescription(preg_replace('/\n.*/s', '', $this->getComments()));
|
2011-01-26 02:17:19 +01:00
|
|
|
$diff->save();
|
|
|
|
|
2011-09-14 19:59:52 +02:00
|
|
|
$this->updateAffectedPathTable($revision, $diff, $changesets);
|
2011-09-26 23:20:31 +02:00
|
|
|
$this->updateRevisionHashTable($revision, $diff);
|
2011-09-14 19:59:52 +02:00
|
|
|
|
2012-04-24 02:40:57 +02:00
|
|
|
// An updated diff should require review, as long as it's not closed
|
2011-01-26 02:17:19 +01:00
|
|
|
// or accepted. The "accepted" status is "sticky" to encourage courtesy
|
|
|
|
// re-diffs after someone accepts with minor changes/suggestions.
|
|
|
|
|
|
|
|
$status = $revision->getStatus();
|
2012-04-24 02:40:57 +02:00
|
|
|
if ($status != ArcanistDifferentialRevisionStatus::CLOSED &&
|
2012-01-10 20:39:11 +01:00
|
|
|
$status != ArcanistDifferentialRevisionStatus::ACCEPTED) {
|
|
|
|
$revision->setStatus(ArcanistDifferentialRevisionStatus::NEEDS_REVIEW);
|
2011-01-26 02:17:19 +01:00
|
|
|
}
|
|
|
|
|
|
|
|
} else {
|
2011-01-30 22:20:56 +01:00
|
|
|
$diff = $revision->loadActiveDiff();
|
2011-01-26 02:17:19 +01:00
|
|
|
if ($diff) {
|
2011-01-30 22:20:56 +01:00
|
|
|
$changesets = $diff->loadChangesets();
|
2011-01-26 02:17:19 +01:00
|
|
|
} else {
|
|
|
|
$changesets = array();
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
$revision->save();
|
|
|
|
|
Make "reject" and "blocking reviewer" block acceptance in Differential
Summary:
Ref T1279. This is a logical change.
- "Reject" (nee "Request Changes") is now sticky. The review won't transition to "Accepted" until the reviewer clears their objection. In practice, I think it always worked like this anyway (without technical enforcement, users just followed this rule naturally, since disobeying this rule is kind of a dick move) so I don't expect this to change much. I think this rule is easier to understand than the old rule now, given the multi-reviewer status and blocking reviewers.
- "Blocking Reviewer" and "Reject" now prevent a revision from transitioning to "Accepted". When reviewers accept, resign, or are removed, we do a check to see if the reivsion has: at least one user reviewer who has accepted; zero rejects; and zero blocks. If all conditions are satisfied, we transition it to "accepted".
Practically, the primary net effect of this is just to make blocking reviews actually block.
This is pretty messy, but there's not much we can do about it until after T2222, since we have two completely separate editor pathways which are both responsible for adjusting status. Eventually, these can merge into a single sane editor which implements reasonable rules in reaonable ways. But that day is not today.
Test Plan: With three users and a project, made a bunch of accepts, rejects, resigns and reviewer removals. I think I probably covered most of the pathways? There are a lot of interactions here.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, wisutsak.jaisue.7
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7245
2013-10-07 02:09:56 +02:00
|
|
|
// If the actor just deleted all the blocking/rejected reviewers, we may
|
|
|
|
// be able to put the revision into "accepted".
|
|
|
|
switch ($revision->getStatus()) {
|
|
|
|
case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
|
|
|
|
case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
|
|
|
|
$revision = self::updateAcceptedStatus(
|
|
|
|
$this->getActor(),
|
|
|
|
$revision);
|
|
|
|
break;
|
|
|
|
}
|
|
|
|
|
2011-08-14 22:55:30 +02:00
|
|
|
$this->didWriteRevision();
|
|
|
|
|
2011-06-19 09:27:39 +02:00
|
|
|
$event_data = array(
|
|
|
|
'revision_id' => $revision->getID(),
|
|
|
|
'revision_phid' => $revision->getPHID(),
|
|
|
|
'revision_name' => $revision->getTitle(),
|
|
|
|
'revision_author_phid' => $revision->getAuthorPHID(),
|
|
|
|
'action' => $is_new
|
|
|
|
? DifferentialAction::ACTION_CREATE
|
|
|
|
: DifferentialAction::ACTION_UPDATE,
|
|
|
|
'feedback_content' => $is_new
|
2011-07-12 16:23:04 +02:00
|
|
|
? phutil_utf8_shorten($revision->getSummary(), 140)
|
2011-06-19 09:27:39 +02:00
|
|
|
: $this->getComments(),
|
|
|
|
'actor_phid' => $revision->getAuthorPHID(),
|
|
|
|
);
|
|
|
|
|
2012-10-23 21:03:11 +02:00
|
|
|
$mailed_phids = array();
|
|
|
|
if (!$this->silentUpdate) {
|
|
|
|
$revision->loadRelationships();
|
|
|
|
|
|
|
|
if ($add['rev']) {
|
|
|
|
$message = id(new DifferentialNewDiffMail(
|
|
|
|
$revision,
|
|
|
|
$actor_handle,
|
|
|
|
$changesets))
|
2013-03-01 02:23:23 +01:00
|
|
|
->setActor($this->getActor())
|
2012-10-23 21:03:11 +02:00
|
|
|
->setIsFirstMailAboutRevision($is_new)
|
|
|
|
->setIsFirstMailToRecipients(true)
|
|
|
|
->setToPHIDs(array_keys($add['rev']));
|
|
|
|
|
|
|
|
if ($is_new) {
|
|
|
|
// The first time we send an email about a revision, put the CCs in
|
|
|
|
// the "CC:" field of the same "Review Requested" email that reviewers
|
|
|
|
// get, so you don't get two initial emails if you're on a list that
|
|
|
|
// is CC'd.
|
|
|
|
$message->setCCPHIDs(array_keys($add['ccs']));
|
|
|
|
}
|
|
|
|
|
|
|
|
$mail[] = $message;
|
|
|
|
}
|
|
|
|
|
|
|
|
// If we added CCs, we want to send them an email, but only if they were
|
|
|
|
// not already a reviewer and were not added as one (in these cases, they
|
|
|
|
// got a "NewDiff" mail, either in the past or just a moment ago). You can
|
|
|
|
// still get two emails, but only if a revision is updated and you are
|
|
|
|
// added as a reviewer at the same time a list you are on is added as a
|
|
|
|
// CC, which is rare and reasonable.
|
|
|
|
|
|
|
|
$implied_ccs = self::getImpliedCCs($revision);
|
|
|
|
$implied_ccs = array_fill_keys($implied_ccs, true);
|
|
|
|
$add['ccs'] = array_diff_key($add['ccs'], $implied_ccs);
|
|
|
|
|
|
|
|
if (!$is_new && $add['ccs']) {
|
|
|
|
$mail[] = id(new DifferentialCCWelcomeMail(
|
|
|
|
$revision,
|
|
|
|
$actor_handle,
|
|
|
|
$changesets))
|
2013-03-01 02:15:09 +01:00
|
|
|
->setActor($this->getActor())
|
2012-10-23 21:03:11 +02:00
|
|
|
->setIsFirstMailToRecipients(true)
|
|
|
|
->setToPHIDs(array_keys($add['ccs']));
|
|
|
|
}
|
|
|
|
|
|
|
|
foreach ($mail as $message) {
|
|
|
|
$message->setHeraldTranscriptURI($xscript_uri);
|
|
|
|
$message->setXHeraldRulesHeader($xscript_header);
|
|
|
|
$message->send();
|
|
|
|
|
|
|
|
$mailed_phids[] = $message->getRawMail()->buildRecipientList();
|
|
|
|
}
|
|
|
|
$mailed_phids = array_mergev($mailed_phids);
|
|
|
|
}
|
|
|
|
|
2011-07-10 00:44:49 +02:00
|
|
|
id(new PhabricatorFeedStoryPublisher())
|
2012-10-23 21:03:11 +02:00
|
|
|
->setStoryType('PhabricatorFeedStoryDifferential')
|
2011-07-10 00:44:49 +02:00
|
|
|
->setStoryData($event_data)
|
|
|
|
->setStoryTime(time())
|
|
|
|
->setStoryAuthorPHID($revision->getAuthorPHID())
|
|
|
|
->setRelatedPHIDs(
|
|
|
|
array(
|
|
|
|
$revision->getPHID(),
|
|
|
|
$revision->getAuthorPHID(),
|
|
|
|
))
|
2012-06-09 03:45:26 +02:00
|
|
|
->setPrimaryObjectPHID($revision->getPHID())
|
|
|
|
->setSubscribedPHIDs(
|
|
|
|
array_merge(
|
|
|
|
array($revision->getAuthorPHID()),
|
|
|
|
$revision->getReviewers(),
|
|
|
|
$revision->getCCPHIDs()))
|
2012-10-23 21:03:11 +02:00
|
|
|
->setMailRecipientPHIDs($mailed_phids)
|
2011-07-10 00:44:49 +02:00
|
|
|
->publish();
|
|
|
|
|
Improve Search architecture
Summary:
The search indexing API has several problems right now:
- Always runs in-process.
- It would be nice to push this into the task queue for performance. However, the API currently passses an object all the way through (and some indexers depend on preloaded object attributes), so it can't be dumped into the task queue at any stage since we can't serialize it.
- Being able to use the task queue will also make rebuilding indexes faster.
- Instead, make the API phid-oriented.
- No uniform indexing API.
- Each "Editor" currently calls SomeCustomIndexer::indexThing(). This won't work with AbstractTransactions. The API is also just weird.
- Instead, provide a uniform API.
- No uniform CLI.
- We have `scripts/search/reindex_everything.php`, but it doesn't actually index everything. Each new document type needs to be separately added to it, leading to stuff like D3839. Third-party applications can't provide indexers.
- Instead, let indexers expose documents for indexing.
- Not application-oriented.
- All the indexers live in search/ right now, which isn't the right organization in an application-orietned view of the world.
- Instead, move indexers to applications and load them with SymbolLoader.
Test Plan:
- `bin/search index`
- Indexed one revision, one task.
- Indexed `--type TASK`, `--type DREV`, etc., for all types.
- Indexed `--all`.
- Added the word "saboteur" to a revision, task, wiki page, and question and then searched for it.
- Creating users is a pain; searched for a user after indexing.
- Creating commits is a pain; searched for a commit after indexing.
- Mocks aren't currently loadable in the result view, so their indexing is moot.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: 20after4, aran
Maniphest Tasks: T1991, T2104
Differential Revision: https://secure.phabricator.com/D4261
2012-12-21 23:21:31 +01:00
|
|
|
id(new PhabricatorSearchIndexer())
|
2014-01-14 22:22:56 +01:00
|
|
|
->queueDocumentForIndexing($revision->getPHID());
|
2011-01-26 02:17:19 +01:00
|
|
|
}
|
|
|
|
|
2011-05-31 23:53:03 +02:00
|
|
|
public static function addCCAndUpdateRevision(
|
|
|
|
$revision,
|
|
|
|
$phid,
|
2013-02-08 03:13:38 +01:00
|
|
|
PhabricatorUser $actor) {
|
2011-05-31 23:53:03 +02:00
|
|
|
|
2013-02-08 03:13:38 +01:00
|
|
|
self::addCC($revision, $phid, $actor->getPHID());
|
2011-05-31 23:53:03 +02:00
|
|
|
|
2013-02-02 01:48:10 +01:00
|
|
|
$type = PhabricatorEdgeConfig::TYPE_OBJECT_HAS_UNSUBSCRIBER;
|
|
|
|
id(new PhabricatorEdgeEditor())
|
2013-02-08 03:13:38 +01:00
|
|
|
->setActor($actor)
|
2013-02-02 01:48:10 +01:00
|
|
|
->removeEdge($revision->getPHID(), $type, $phid)
|
|
|
|
->save();
|
2011-05-31 23:53:03 +02:00
|
|
|
}
|
|
|
|
|
|
|
|
public static function removeCCAndUpdateRevision(
|
|
|
|
$revision,
|
|
|
|
$phid,
|
2013-02-08 03:13:38 +01:00
|
|
|
PhabricatorUser $actor) {
|
2011-05-31 23:53:03 +02:00
|
|
|
|
2013-02-08 03:13:38 +01:00
|
|
|
self::removeCC($revision, $phid, $actor->getPHID());
|
2011-05-31 23:53:03 +02:00
|
|
|
|
2013-02-02 01:48:10 +01:00
|
|
|
$type = PhabricatorEdgeConfig::TYPE_OBJECT_HAS_UNSUBSCRIBER;
|
|
|
|
id(new PhabricatorEdgeEditor())
|
2013-02-08 03:13:38 +01:00
|
|
|
->setActor($actor)
|
2013-02-02 01:48:10 +01:00
|
|
|
->addEdge($revision->getPHID(), $type, $phid)
|
|
|
|
->save();
|
2011-05-31 23:53:03 +02:00
|
|
|
}
|
|
|
|
|
2011-02-19 23:36:13 +01:00
|
|
|
public static function addCC(
|
|
|
|
DifferentialRevision $revision,
|
|
|
|
$phid,
|
|
|
|
$reason) {
|
|
|
|
return self::alterCCs(
|
|
|
|
$revision,
|
|
|
|
$revision->getCCPHIDs(),
|
|
|
|
$rem = array(),
|
|
|
|
$add = array($phid),
|
|
|
|
$reason);
|
|
|
|
}
|
|
|
|
|
|
|
|
public static function removeCC(
|
|
|
|
DifferentialRevision $revision,
|
|
|
|
$phid,
|
|
|
|
$reason) {
|
|
|
|
return self::alterCCs(
|
|
|
|
$revision,
|
|
|
|
$revision->getCCPHIDs(),
|
|
|
|
$rem = array($phid),
|
|
|
|
$add = array(),
|
|
|
|
$reason);
|
|
|
|
}
|
|
|
|
|
2011-02-05 03:16:02 +01:00
|
|
|
protected static function alterCCs(
|
2011-01-26 02:17:19 +01:00
|
|
|
DifferentialRevision $revision,
|
2011-02-05 03:16:02 +01:00
|
|
|
array $stable_phids,
|
|
|
|
array $rem_phids,
|
|
|
|
array $add_phids,
|
2011-01-26 02:17:19 +01:00
|
|
|
$reason_phid) {
|
|
|
|
|
2012-04-02 19:41:03 +02:00
|
|
|
$dont_add = self::getImpliedCCs($revision);
|
2012-03-30 15:25:49 +02:00
|
|
|
$add_phids = array_diff($add_phids, $dont_add);
|
|
|
|
|
2014-02-12 17:53:40 +01:00
|
|
|
id(new PhabricatorSubscriptionsEditor())
|
|
|
|
->setActor(PhabricatorUser::getOmnipotentUser())
|
|
|
|
->setObject($revision)
|
|
|
|
->subscribeExplicit($add_phids)
|
|
|
|
->unsubscribe($rem_phids)
|
|
|
|
->save();
|
2011-01-26 02:17:19 +01:00
|
|
|
}
|
|
|
|
|
2012-04-02 19:41:03 +02:00
|
|
|
private static function getImpliedCCs(DifferentialRevision $revision) {
|
|
|
|
return array_merge(
|
|
|
|
$revision->getReviewers(),
|
|
|
|
array($revision->getAuthorPHID()));
|
|
|
|
}
|
2011-02-05 03:16:02 +01:00
|
|
|
|
2013-07-10 22:45:14 +02:00
|
|
|
public static function updateReviewers(
|
|
|
|
DifferentialRevision $revision,
|
|
|
|
PhabricatorUser $actor,
|
|
|
|
array $add_phids,
|
2013-10-07 02:09:24 +02:00
|
|
|
array $remove_phids,
|
|
|
|
array $blocking_phids = array()) {
|
2013-07-10 22:45:14 +02:00
|
|
|
|
|
|
|
$reviewers = $revision->getReviewers();
|
|
|
|
|
|
|
|
$editor = id(new PhabricatorEdgeEditor())
|
|
|
|
->setActor($actor);
|
|
|
|
|
|
|
|
$reviewer_phids_map = array_fill_keys($reviewers, true);
|
|
|
|
|
2013-10-07 02:09:24 +02:00
|
|
|
$blocking_phids = array_fuse($blocking_phids);
|
2013-07-10 22:45:14 +02:00
|
|
|
foreach ($add_phids as $phid) {
|
|
|
|
|
|
|
|
// Adding an already existing edge again would have cause memory loss
|
|
|
|
// That is, the previous state for that reviewer would be lost
|
|
|
|
if (isset($reviewer_phids_map[$phid])) {
|
2013-10-07 02:09:24 +02:00
|
|
|
// TODO: If we're writing a blocking edge, we should overwrite an
|
|
|
|
// existing weaker edge (like "added" or "commented"), just not a
|
|
|
|
// stronger existing edge.
|
2013-07-10 22:45:14 +02:00
|
|
|
continue;
|
|
|
|
}
|
|
|
|
|
2013-10-07 02:09:24 +02:00
|
|
|
if (isset($blocking_phids[$phid])) {
|
|
|
|
$status = DifferentialReviewerStatus::STATUS_BLOCKING;
|
|
|
|
} else {
|
|
|
|
$status = DifferentialReviewerStatus::STATUS_ADDED;
|
|
|
|
}
|
|
|
|
|
|
|
|
$options = array(
|
|
|
|
'data' => array(
|
|
|
|
'status' => $status,
|
|
|
|
)
|
|
|
|
);
|
|
|
|
|
2013-07-10 22:45:14 +02:00
|
|
|
$editor->addEdge(
|
|
|
|
$revision->getPHID(),
|
|
|
|
PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER,
|
|
|
|
$phid,
|
|
|
|
$options);
|
|
|
|
}
|
|
|
|
|
|
|
|
foreach ($remove_phids as $phid) {
|
|
|
|
$editor->removeEdge(
|
|
|
|
$revision->getPHID(),
|
|
|
|
PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER,
|
|
|
|
$phid);
|
|
|
|
}
|
|
|
|
|
|
|
|
$editor->save();
|
|
|
|
}
|
|
|
|
|
|
|
|
public static function updateReviewerStatus(
|
|
|
|
DifferentialRevision $revision,
|
|
|
|
PhabricatorUser $actor,
|
|
|
|
$reviewer_phid,
|
|
|
|
$status) {
|
|
|
|
|
|
|
|
$options = array(
|
|
|
|
'data' => array(
|
|
|
|
'status' => $status
|
|
|
|
)
|
|
|
|
);
|
|
|
|
|
|
|
|
$active_diff = $revision->loadActiveDiff();
|
|
|
|
if ($active_diff) {
|
|
|
|
$options['data']['diff'] = $active_diff->getID();
|
|
|
|
}
|
|
|
|
|
|
|
|
id(new PhabricatorEdgeEditor())
|
|
|
|
->setActor($actor)
|
|
|
|
->addEdge(
|
|
|
|
$revision->getPHID(),
|
|
|
|
PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER,
|
|
|
|
$reviewer_phid,
|
|
|
|
$options)
|
|
|
|
->save();
|
|
|
|
}
|
|
|
|
|
2011-02-05 02:53:14 +01:00
|
|
|
private function createComment() {
|
2014-02-12 00:21:14 +01:00
|
|
|
$template = id(new DifferentialComment())
|
2011-02-05 02:53:14 +01:00
|
|
|
->setAuthorPHID($this->getActorPHID())
|
2014-02-12 00:21:14 +01:00
|
|
|
->setRevision($this->revision);
|
2014-02-14 01:03:23 +01:00
|
|
|
|
2014-02-12 00:21:14 +01:00
|
|
|
if ($this->contentSource) {
|
2014-02-14 01:03:23 +01:00
|
|
|
$content_source = $this->contentSource;
|
|
|
|
} else {
|
|
|
|
$content_source = PhabricatorContentSource::newForSource(
|
|
|
|
PhabricatorContentSource::SOURCE_LEGACY,
|
|
|
|
array());
|
2014-02-12 00:21:14 +01:00
|
|
|
}
|
|
|
|
|
2014-02-14 01:03:23 +01:00
|
|
|
$template->setContentSource($content_source);
|
|
|
|
|
|
|
|
|
2014-02-12 00:21:14 +01:00
|
|
|
// Write the "update active diff" transaction.
|
|
|
|
id(clone $template)
|
2012-02-22 17:03:48 +01:00
|
|
|
->setAction(DifferentialAction::ACTION_UPDATE)
|
|
|
|
->setMetadata(
|
|
|
|
array(
|
|
|
|
DifferentialComment::METADATA_DIFF_ID => $this->getDiff()->getID(),
|
2014-02-12 00:21:14 +01:00
|
|
|
))
|
|
|
|
->save();
|
Track content sources (email, web, conduit, mobile) for replies
Summary:
When an object is updated, record the content source for the update. This mostly
isn't terribly useful but one concrete thing I want to do with it is let admins
audit via-email replies more easily since there are a bunch of options which let
you do hyjinx if you intentionally configure them insecurely. I think having a
little more auditability around this feature is generally good. At some point
I'm going to turn this into a link admins can click to see details.
It also allows us to see how frequently different mechanisms are used, and lets
you see if someone is at their desk or on a mobile or whatever, at least
indirectly.
The "tablet" and "mobile" sources are currently unused but I figured I'd throw
them in anyway. SMS support should definitely happen at some point.
Not 100% sure about the design for this, I might change it to plain text at some
point.
Test Plan: Updated objects and saw update sources rendered.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 844
2011-08-22 19:25:45 +02:00
|
|
|
|
2014-02-12 00:21:14 +01:00
|
|
|
// If we have a comment, write the "add a comment" transaction.
|
|
|
|
if (strlen($this->getComments())) {
|
|
|
|
id(clone $template)
|
|
|
|
->setAction(DifferentialAction::ACTION_COMMENT)
|
|
|
|
->setContent($this->getComments())
|
|
|
|
->save();
|
Track content sources (email, web, conduit, mobile) for replies
Summary:
When an object is updated, record the content source for the update. This mostly
isn't terribly useful but one concrete thing I want to do with it is let admins
audit via-email replies more easily since there are a bunch of options which let
you do hyjinx if you intentionally configure them insecurely. I think having a
little more auditability around this feature is generally good. At some point
I'm going to turn this into a link admins can click to see details.
It also allows us to see how frequently different mechanisms are used, and lets
you see if someone is at their desk or on a mobile or whatever, at least
indirectly.
The "tablet" and "mobile" sources are currently unused but I figured I'd throw
them in anyway. SMS support should definitely happen at some point.
Not 100% sure about the design for this, I might change it to plain text at some
point.
Test Plan: Updated objects and saw update sources rendered.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 844
2011-08-22 19:25:45 +02:00
|
|
|
}
|
2011-01-26 02:17:19 +01:00
|
|
|
}
|
|
|
|
|
2011-08-10 22:46:01 +02:00
|
|
|
private function updateAuxiliaryFields() {
|
|
|
|
$aux_map = array();
|
|
|
|
foreach ($this->auxiliaryFields as $aux_field) {
|
|
|
|
$key = $aux_field->getStorageKey();
|
2011-08-14 23:28:08 +02:00
|
|
|
if ($key !== null) {
|
|
|
|
$val = $aux_field->getValueForStorage();
|
|
|
|
$aux_map[$key] = $val;
|
|
|
|
}
|
2011-08-10 22:46:01 +02:00
|
|
|
}
|
|
|
|
|
|
|
|
if (!$aux_map) {
|
|
|
|
return;
|
|
|
|
}
|
|
|
|
|
|
|
|
$revision = $this->revision;
|
|
|
|
|
|
|
|
$fields = id(new DifferentialAuxiliaryField())->loadAllWhere(
|
|
|
|
'revisionPHID = %s AND name IN (%Ls)',
|
|
|
|
$revision->getPHID(),
|
|
|
|
array_keys($aux_map));
|
|
|
|
$fields = mpull($fields, null, 'getName');
|
|
|
|
|
|
|
|
foreach ($aux_map as $key => $val) {
|
|
|
|
$obj = idx($fields, $key);
|
Save empty fields as no row, not an empty row
Summary: When a user stores the empty string in an auxiliary field, simply don't
store it, and delete it if it already exists.
Test Plan: Edited a revision with an empty "Quack" field, got an empty row in
the DB. Applied patch, edited empty again, row went away. Edited empty again,
still no row. Edited and put something in the field, got a row.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 865
2011-08-26 20:40:42 +02:00
|
|
|
if (!strlen($val)) {
|
|
|
|
// If the new value is empty, just delete the old row if one exists and
|
|
|
|
// don't add a new row if it doesn't.
|
|
|
|
if ($obj) {
|
|
|
|
$obj->delete();
|
|
|
|
}
|
|
|
|
} else {
|
|
|
|
if (!$obj) {
|
|
|
|
$obj = new DifferentialAuxiliaryField();
|
|
|
|
$obj->setRevisionPHID($revision->getPHID());
|
|
|
|
$obj->setName($key);
|
|
|
|
}
|
2011-08-10 22:46:01 +02:00
|
|
|
|
Save empty fields as no row, not an empty row
Summary: When a user stores the empty string in an auxiliary field, simply don't
store it, and delete it if it already exists.
Test Plan: Edited a revision with an empty "Quack" field, got an empty row in
the DB. Applied patch, edited empty again, row went away. Edited empty again,
still no row. Edited and put something in the field, got a row.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 865
2011-08-26 20:40:42 +02:00
|
|
|
if ($obj->getValue() !== $val) {
|
|
|
|
$obj->setValue($val);
|
|
|
|
$obj->save();
|
|
|
|
}
|
2011-08-10 22:46:01 +02:00
|
|
|
}
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2011-08-14 22:55:30 +02:00
|
|
|
private function willWriteRevision() {
|
|
|
|
foreach ($this->auxiliaryFields as $aux_field) {
|
|
|
|
$aux_field->willWriteRevision($this);
|
|
|
|
}
|
2013-09-16 17:04:14 +02:00
|
|
|
|
|
|
|
$this->dispatchEvent(
|
|
|
|
PhabricatorEventType::TYPE_DIFFERENTIAL_WILLEDITREVISION);
|
2011-08-14 22:55:30 +02:00
|
|
|
}
|
|
|
|
|
|
|
|
private function didWriteRevision() {
|
|
|
|
foreach ($this->auxiliaryFields as $aux_field) {
|
|
|
|
$aux_field->didWriteRevision($this);
|
|
|
|
}
|
2013-09-16 17:04:14 +02:00
|
|
|
|
|
|
|
$this->dispatchEvent(
|
|
|
|
PhabricatorEventType::TYPE_DIFFERENTIAL_DIDEDITREVISION);
|
|
|
|
}
|
|
|
|
|
|
|
|
private function dispatchEvent($type) {
|
|
|
|
$event = new PhabricatorEvent(
|
|
|
|
$type,
|
|
|
|
array(
|
|
|
|
'revision' => $this->revision,
|
|
|
|
'new' => $this->isCreate,
|
|
|
|
));
|
|
|
|
|
|
|
|
$event->setUser($this->getActor());
|
|
|
|
|
|
|
|
$request = $this->getAphrontRequestForEventDispatch();
|
|
|
|
if ($request) {
|
|
|
|
$event->setAphrontRequest($request);
|
|
|
|
}
|
|
|
|
|
|
|
|
PhutilEventEngine::dispatchEvent($event);
|
2011-08-14 22:55:30 +02:00
|
|
|
}
|
2011-08-10 22:46:01 +02:00
|
|
|
|
2011-09-14 19:59:52 +02:00
|
|
|
/**
|
2011-09-26 23:20:31 +02:00
|
|
|
* Update the table which links Differential revisions to paths they affect,
|
2011-09-14 19:59:52 +02:00
|
|
|
* so Diffusion can efficiently find pending revisions for a given file.
|
|
|
|
*/
|
|
|
|
private function updateAffectedPathTable(
|
|
|
|
DifferentialRevision $revision,
|
|
|
|
DifferentialDiff $diff,
|
|
|
|
array $changesets) {
|
2012-04-04 22:13:08 +02:00
|
|
|
assert_instances_of($changesets, 'DifferentialChangeset');
|
2011-09-14 19:59:52 +02:00
|
|
|
|
|
|
|
$project = $diff->loadArcanistProject();
|
|
|
|
if (!$project) {
|
|
|
|
// Probably an old revision from before projects.
|
|
|
|
return;
|
|
|
|
}
|
|
|
|
|
|
|
|
$repository = $project->loadRepository();
|
|
|
|
if (!$repository) {
|
|
|
|
// Probably no project <-> repository link, or the repository where the
|
|
|
|
// project lives is untracked.
|
|
|
|
return;
|
|
|
|
}
|
|
|
|
|
|
|
|
$path_prefix = null;
|
|
|
|
|
|
|
|
$local_root = $diff->getSourceControlPath();
|
|
|
|
if ($local_root) {
|
|
|
|
// We're in a working copy which supports subdirectory checkouts (e.g.,
|
|
|
|
// SVN) so we need to figure out what prefix we should add to each path
|
|
|
|
// (e.g., trunk/projects/example/) to get the absolute path from the
|
|
|
|
// root of the repository. DVCS systems like Git and Mercurial are not
|
|
|
|
// affected.
|
|
|
|
|
|
|
|
// Normalize both paths and check if the repository root is a prefix of
|
|
|
|
// the local root. If so, throw it away. Note that this correctly handles
|
|
|
|
// the case where the remote path is "/".
|
|
|
|
$local_root = id(new PhutilURI($local_root))->getPath();
|
|
|
|
$local_root = rtrim($local_root, '/');
|
|
|
|
|
|
|
|
$repo_root = id(new PhutilURI($repository->getRemoteURI()))->getPath();
|
|
|
|
$repo_root = rtrim($repo_root, '/');
|
|
|
|
|
|
|
|
if (!strncmp($repo_root, $local_root, strlen($repo_root))) {
|
|
|
|
$path_prefix = substr($local_root, strlen($repo_root));
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
$paths = array();
|
|
|
|
foreach ($changesets as $changeset) {
|
2012-01-17 08:05:44 +01:00
|
|
|
$paths[] = $path_prefix.'/'.$changeset->getFilename();
|
2011-09-14 19:59:52 +02:00
|
|
|
}
|
|
|
|
|
2011-10-02 21:52:54 +02:00
|
|
|
// Mark this as also touching all parent paths, so you can see all pending
|
|
|
|
// changes to any file within a directory.
|
|
|
|
$all_paths = array();
|
|
|
|
foreach ($paths as $local) {
|
|
|
|
foreach (DiffusionPathIDQuery::expandPathToRoot($local) as $path) {
|
|
|
|
$all_paths[$path] = true;
|
|
|
|
}
|
|
|
|
}
|
|
|
|
$all_paths = array_keys($all_paths);
|
|
|
|
|
2012-11-21 20:08:07 +01:00
|
|
|
$path_ids =
|
|
|
|
PhabricatorRepositoryCommitChangeParserWorker::lookupOrCreatePaths(
|
|
|
|
$all_paths);
|
2011-09-14 19:59:52 +02:00
|
|
|
|
|
|
|
$table = new DifferentialAffectedPath();
|
|
|
|
$conn_w = $table->establishConnection('w');
|
|
|
|
|
|
|
|
$sql = array();
|
2012-11-21 20:08:07 +01:00
|
|
|
foreach ($path_ids as $path_id) {
|
2011-09-14 19:59:52 +02:00
|
|
|
$sql[] = qsprintf(
|
|
|
|
$conn_w,
|
|
|
|
'(%d, %d, %d, %d)',
|
|
|
|
$repository->getID(),
|
|
|
|
$path_id,
|
|
|
|
time(),
|
|
|
|
$revision->getID());
|
|
|
|
}
|
|
|
|
|
|
|
|
queryfx(
|
|
|
|
$conn_w,
|
|
|
|
'DELETE FROM %T WHERE revisionID = %d',
|
|
|
|
$table->getTableName(),
|
|
|
|
$revision->getID());
|
|
|
|
foreach (array_chunk($sql, 256) as $chunk) {
|
|
|
|
queryfx(
|
|
|
|
$conn_w,
|
|
|
|
'INSERT INTO %T (repositoryID, pathID, epoch, revisionID) VALUES %Q',
|
|
|
|
$table->getTableName(),
|
|
|
|
implode(', ', $chunk));
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2011-09-26 23:20:31 +02:00
|
|
|
|
|
|
|
/**
|
|
|
|
* Update the table connecting revisions to DVCS local hashes, so we can
|
|
|
|
* identify revisions by commit/tree hashes.
|
|
|
|
*/
|
|
|
|
private function updateRevisionHashTable(
|
|
|
|
DifferentialRevision $revision,
|
|
|
|
DifferentialDiff $diff) {
|
|
|
|
|
|
|
|
$vcs = $diff->getSourceControlSystem();
|
|
|
|
if ($vcs == DifferentialRevisionControlSystem::SVN) {
|
|
|
|
// Subversion has no local commit or tree hash information, so we don't
|
|
|
|
// have to do anything.
|
|
|
|
return;
|
|
|
|
}
|
|
|
|
|
|
|
|
$property = id(new DifferentialDiffProperty())->loadOneWhere(
|
|
|
|
'diffID = %d AND name = %s',
|
|
|
|
$diff->getID(),
|
|
|
|
'local:commits');
|
|
|
|
if (!$property) {
|
|
|
|
return;
|
|
|
|
}
|
|
|
|
|
|
|
|
$hashes = array();
|
|
|
|
|
|
|
|
$data = $property->getData();
|
|
|
|
switch ($vcs) {
|
|
|
|
case DifferentialRevisionControlSystem::GIT:
|
|
|
|
foreach ($data as $commit) {
|
|
|
|
$hashes[] = array(
|
2012-01-10 20:39:11 +01:00
|
|
|
ArcanistDifferentialRevisionHash::HASH_GIT_COMMIT,
|
2011-09-26 23:20:31 +02:00
|
|
|
$commit['commit'],
|
|
|
|
);
|
|
|
|
$hashes[] = array(
|
2012-01-10 20:39:11 +01:00
|
|
|
ArcanistDifferentialRevisionHash::HASH_GIT_TREE,
|
2011-09-26 23:20:31 +02:00
|
|
|
$commit['tree'],
|
|
|
|
);
|
|
|
|
}
|
|
|
|
break;
|
|
|
|
case DifferentialRevisionControlSystem::MERCURIAL:
|
|
|
|
foreach ($data as $commit) {
|
|
|
|
$hashes[] = array(
|
2012-01-10 20:39:11 +01:00
|
|
|
ArcanistDifferentialRevisionHash::HASH_MERCURIAL_COMMIT,
|
2011-09-26 23:20:31 +02:00
|
|
|
$commit['rev'],
|
|
|
|
);
|
|
|
|
}
|
|
|
|
break;
|
|
|
|
}
|
|
|
|
|
|
|
|
$conn_w = $revision->establishConnection('w');
|
|
|
|
|
|
|
|
$sql = array();
|
|
|
|
foreach ($hashes as $info) {
|
|
|
|
list($type, $hash) = $info;
|
|
|
|
$sql[] = qsprintf(
|
|
|
|
$conn_w,
|
|
|
|
'(%d, %s, %s)',
|
|
|
|
$revision->getID(),
|
|
|
|
$type,
|
|
|
|
$hash);
|
|
|
|
}
|
|
|
|
|
|
|
|
queryfx(
|
|
|
|
$conn_w,
|
|
|
|
'DELETE FROM %T WHERE revisionID = %d',
|
2012-01-10 20:39:11 +01:00
|
|
|
ArcanistDifferentialRevisionHash::TABLE_NAME,
|
2011-09-26 23:20:31 +02:00
|
|
|
$revision->getID());
|
|
|
|
|
|
|
|
if ($sql) {
|
|
|
|
queryfx(
|
|
|
|
$conn_w,
|
|
|
|
'INSERT INTO %T (revisionID, type, hash) VALUES %Q',
|
2012-01-10 20:39:11 +01:00
|
|
|
ArcanistDifferentialRevisionHash::TABLE_NAME,
|
2011-09-26 23:20:31 +02:00
|
|
|
implode(', ', $sql));
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
Make "reject" and "blocking reviewer" block acceptance in Differential
Summary:
Ref T1279. This is a logical change.
- "Reject" (nee "Request Changes") is now sticky. The review won't transition to "Accepted" until the reviewer clears their objection. In practice, I think it always worked like this anyway (without technical enforcement, users just followed this rule naturally, since disobeying this rule is kind of a dick move) so I don't expect this to change much. I think this rule is easier to understand than the old rule now, given the multi-reviewer status and blocking reviewers.
- "Blocking Reviewer" and "Reject" now prevent a revision from transitioning to "Accepted". When reviewers accept, resign, or are removed, we do a check to see if the reivsion has: at least one user reviewer who has accepted; zero rejects; and zero blocks. If all conditions are satisfied, we transition it to "accepted".
Practically, the primary net effect of this is just to make blocking reviews actually block.
This is pretty messy, but there's not much we can do about it until after T2222, since we have two completely separate editor pathways which are both responsible for adjusting status. Eventually, these can merge into a single sane editor which implements reasonable rules in reaonable ways. But that day is not today.
Test Plan: With three users and a project, made a bunch of accepts, rejects, resigns and reviewer removals. I think I probably covered most of the pathways? There are a lot of interactions here.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, wisutsak.jaisue.7
Maniphest Tasks: T1279
Differential Revision: https://secure.phabricator.com/D7245
2013-10-07 02:09:56 +02:00
|
|
|
/**
|
|
|
|
* Try to move a revision to "accepted". We look for:
|
|
|
|
*
|
|
|
|
* - at least one accepting reviewer who is a user; and
|
|
|
|
* - no rejects; and
|
|
|
|
* - no blocking reviewers.
|
|
|
|
*/
|
|
|
|
public static function updateAcceptedStatus(
|
|
|
|
PhabricatorUser $viewer,
|
|
|
|
DifferentialRevision $revision) {
|
|
|
|
|
|
|
|
$revision = id(new DifferentialRevisionQuery())
|
|
|
|
->setViewer($viewer)
|
|
|
|
->withIDs(array($revision->getID()))
|
|
|
|
->needRelationships(true)
|
|
|
|
->needReviewerStatus(true)
|
|
|
|
->needReviewerAuthority(true)
|
|
|
|
->executeOne();
|
|
|
|
|
|
|
|
$has_user_accept = false;
|
|
|
|
foreach ($revision->getReviewerStatus() as $reviewer) {
|
|
|
|
$status = $reviewer->getStatus();
|
|
|
|
if ($status == DifferentialReviewerStatus::STATUS_BLOCKING) {
|
|
|
|
// We have a blocking reviewer, so just leave the revision in its
|
|
|
|
// existing state.
|
|
|
|
return $revision;
|
|
|
|
}
|
|
|
|
|
|
|
|
if ($status == DifferentialReviewerStatus::STATUS_REJECTED) {
|
|
|
|
// We have a rejecting reviewer, so leave the revisoin as is.
|
|
|
|
return $revision;
|
|
|
|
}
|
|
|
|
|
|
|
|
if ($reviewer->isUser()) {
|
|
|
|
if ($status == DifferentialReviewerStatus::STATUS_ACCEPTED) {
|
|
|
|
$has_user_accept = true;
|
|
|
|
}
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
if ($has_user_accept) {
|
|
|
|
$revision
|
|
|
|
->setStatus(ArcanistDifferentialRevisionStatus::ACCEPTED)
|
|
|
|
->save();
|
|
|
|
}
|
|
|
|
|
|
|
|
return $revision;
|
|
|
|
}
|
|
|
|
|
2011-01-26 02:17:19 +01:00
|
|
|
}
|
|
|
|
|