2011-01-30 12:08:40 -08:00
|
|
|
<?php
|
|
|
|
|
2012-10-10 10:18:23 -07:00
|
|
|
final class DifferentialCommentEditor extends PhabricatorEditor {
|
2011-01-30 12:08:40 -08:00
|
|
|
|
|
|
|
protected $revision;
|
|
|
|
protected $action;
|
|
|
|
|
|
|
|
protected $attachInlineComments;
|
|
|
|
protected $message;
|
|
|
|
protected $changedByCommit;
|
|
|
|
protected $addedReviewers = array();
|
2012-06-26 18:48:53 -07:00
|
|
|
protected $removedReviewers = array();
|
2011-06-24 12:21:48 -07:00
|
|
|
private $addedCCs = array();
|
2011-01-30 12:08:40 -08:00
|
|
|
|
2011-06-22 12:41:19 -07:00
|
|
|
private $parentMessageID;
|
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 10:25:45 -07:00
|
|
|
private $contentSource;
|
2012-08-20 14:08:52 -07:00
|
|
|
private $noEmail;
|
2011-06-22 12:41:19 -07:00
|
|
|
|
2012-03-07 10:20:17 -08:00
|
|
|
private $isDaemonWorkflow;
|
|
|
|
|
2011-01-30 12:08:40 -08:00
|
|
|
public function __construct(
|
|
|
|
DifferentialRevision $revision,
|
|
|
|
$action) {
|
|
|
|
|
|
|
|
$this->revision = $revision;
|
|
|
|
$this->action = $action;
|
|
|
|
}
|
|
|
|
|
2011-06-22 12:41:19 -07:00
|
|
|
public function setParentMessageID($parent_message_id) {
|
|
|
|
$this->parentMessageID = $parent_message_id;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
2011-01-30 12:08:40 -08:00
|
|
|
public function setMessage($message) {
|
|
|
|
$this->message = $message;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
public function setAttachInlineComments($attach) {
|
|
|
|
$this->attachInlineComments = $attach;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
public function setChangedByCommit($changed_by_commit) {
|
|
|
|
$this->changedByCommit = $changed_by_commit;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
public function getChangedByCommit() {
|
|
|
|
return $this->changedByCommit;
|
|
|
|
}
|
|
|
|
|
2012-04-17 14:59:31 -07:00
|
|
|
public function setAddedReviewers(array $added_reviewers) {
|
2011-01-30 12:08:40 -08:00
|
|
|
$this->addedReviewers = $added_reviewers;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
public function getAddedReviewers() {
|
|
|
|
return $this->addedReviewers;
|
|
|
|
}
|
|
|
|
|
2012-06-26 18:48:53 -07:00
|
|
|
public function setRemovedReviewers(array $removeded_reviewers) {
|
|
|
|
$this->removedReviewers = $removeded_reviewers;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
public function getRemovedReviewers() {
|
|
|
|
return $this->removedReviewers;
|
|
|
|
}
|
|
|
|
|
2011-06-24 12:21:48 -07:00
|
|
|
public function setAddedCCs($added_ccs) {
|
|
|
|
$this->addedCCs = $added_ccs;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
public function getAddedCCs() {
|
|
|
|
return $this->addedCCs;
|
|
|
|
}
|
|
|
|
|
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 10:25:45 -07:00
|
|
|
public function setContentSource(PhabricatorContentSource $content_source) {
|
|
|
|
$this->contentSource = $content_source;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
2012-03-07 10:20:17 -08:00
|
|
|
public function setIsDaemonWorkflow($is_daemon) {
|
|
|
|
$this->isDaemonWorkflow = $is_daemon;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
2012-08-20 14:08:52 -07:00
|
|
|
public function setNoEmail($no_email) {
|
|
|
|
$this->noEmail = $no_email;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
2011-01-30 12:08:40 -08:00
|
|
|
public function save() {
|
2012-10-10 10:18:23 -07:00
|
|
|
$actor = $this->requireActor();
|
|
|
|
$revision = $this->revision;
|
|
|
|
$action = $this->action;
|
|
|
|
$actor_phid = $actor->getPHID();
|
|
|
|
$actor_is_author = ($actor_phid == $revision->getAuthorPHID());
|
|
|
|
$allow_self_accept = PhabricatorEnv::getEnvConfig(
|
2013-01-19 12:11:11 -08:00
|
|
|
'differential.allow-self-accept');
|
2012-08-28 14:17:23 -07:00
|
|
|
$always_allow_close = PhabricatorEnv::getEnvConfig(
|
2013-01-19 12:11:11 -08:00
|
|
|
'differential.always-allow-close');
|
2013-01-19 09:10:15 -08:00
|
|
|
$allow_reopen = PhabricatorEnv::getEnvConfig(
|
2013-01-19 12:11:11 -08:00
|
|
|
'differential.allow-reopen');
|
2012-10-10 10:18:23 -07:00
|
|
|
$revision_status = $revision->getStatus();
|
2011-01-30 12:08:40 -08:00
|
|
|
|
|
|
|
$revision->loadRelationships();
|
|
|
|
$reviewer_phids = $revision->getReviewers();
|
|
|
|
if ($reviewer_phids) {
|
2013-01-25 17:06:55 -08:00
|
|
|
$reviewer_phids = array_fuse($reviewer_phids);
|
2011-01-30 12:08:40 -08:00
|
|
|
}
|
|
|
|
|
2011-06-01 11:19:55 -07:00
|
|
|
$metadata = array();
|
|
|
|
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
$inline_comments = array();
|
|
|
|
if ($this->attachInlineComments) {
|
|
|
|
$inline_comments = id(new DifferentialInlineComment())->loadAllWhere(
|
|
|
|
'authorPHID = %s AND revisionID = %d AND commentID IS NULL',
|
2012-10-10 10:18:23 -07:00
|
|
|
$actor_phid,
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
$revision->getID());
|
|
|
|
}
|
|
|
|
|
2011-01-30 12:08:40 -08:00
|
|
|
switch ($action) {
|
|
|
|
case DifferentialAction::ACTION_COMMENT:
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
if (!$this->message && !$inline_comments) {
|
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You are submitting an empty comment with no action: ".
|
|
|
|
"you must act on the revision or post a comment.");
|
|
|
|
}
|
2011-01-30 12:08:40 -08:00
|
|
|
break;
|
|
|
|
|
|
|
|
case DifferentialAction::ACTION_RESIGN:
|
|
|
|
if ($actor_is_author) {
|
|
|
|
throw new Exception('You can not resign from your own revision!');
|
|
|
|
}
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
if (empty($reviewer_phids[$actor_phid])) {
|
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not resign from this revision because you are not ".
|
|
|
|
"a reviewer.");
|
2011-01-30 12:08:40 -08:00
|
|
|
}
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
DifferentialRevisionEditor::alterReviewers(
|
|
|
|
$revision,
|
|
|
|
$reviewer_phids,
|
|
|
|
$rem = array($actor_phid),
|
|
|
|
$add = array(),
|
|
|
|
$actor_phid);
|
2011-01-30 12:08:40 -08:00
|
|
|
break;
|
|
|
|
|
|
|
|
case DifferentialAction::ACTION_ABANDON:
|
2012-04-17 14:59:31 -07:00
|
|
|
if (!$actor_is_author) {
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
throw new Exception('You can only abandon your own revisions.');
|
2011-01-30 12:08:40 -08:00
|
|
|
}
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
|
2012-04-23 17:40:57 -07:00
|
|
|
if ($revision_status == ArcanistDifferentialRevisionStatus::CLOSED) {
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not abandon this revision because it has already ".
|
2012-04-23 17:40:57 -07:00
|
|
|
"been closed.");
|
2011-01-30 12:08:40 -08:00
|
|
|
}
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
|
|
|
|
if ($revision_status == ArcanistDifferentialRevisionStatus::ABANDONED) {
|
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not abandon this revision because it has already ".
|
|
|
|
"been abandoned.");
|
2011-01-30 12:08:40 -08:00
|
|
|
}
|
|
|
|
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
$revision->setStatus(ArcanistDifferentialRevisionStatus::ABANDONED);
|
2011-01-30 12:08:40 -08:00
|
|
|
break;
|
|
|
|
|
|
|
|
case DifferentialAction::ACTION_ACCEPT:
|
2012-07-03 00:38:49 -07:00
|
|
|
if ($actor_is_author && !$allow_self_accept) {
|
2011-01-30 12:08:40 -08:00
|
|
|
throw new Exception('You can not accept your own revision.');
|
|
|
|
}
|
2012-01-10 11:39:11 -08:00
|
|
|
if (($revision_status !=
|
|
|
|
ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) &&
|
|
|
|
($revision_status !=
|
|
|
|
ArcanistDifferentialRevisionStatus::NEEDS_REVISION)) {
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
|
|
|
|
switch ($revision_status) {
|
|
|
|
case ArcanistDifferentialRevisionStatus::ACCEPTED:
|
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not accept this revision because someone else ".
|
|
|
|
"already accepted it.");
|
|
|
|
case ArcanistDifferentialRevisionStatus::ABANDONED:
|
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not accept this revision because it has been ".
|
|
|
|
"abandoned.");
|
2012-04-23 17:40:57 -07:00
|
|
|
case ArcanistDifferentialRevisionStatus::CLOSED:
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not accept this revision because it has already ".
|
2012-04-23 17:40:57 -07:00
|
|
|
"been closed.");
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
default:
|
|
|
|
throw new Exception(
|
|
|
|
"Unexpected revision state '{$revision_status}'!");
|
|
|
|
}
|
2011-01-30 12:08:40 -08:00
|
|
|
}
|
|
|
|
|
|
|
|
$revision
|
2012-01-12 19:22:09 -08:00
|
|
|
->setStatus(ArcanistDifferentialRevisionStatus::ACCEPTED);
|
2011-01-30 12:08:40 -08:00
|
|
|
|
|
|
|
if (!isset($reviewer_phids[$actor_phid])) {
|
2011-02-04 22:45:42 -08:00
|
|
|
DifferentialRevisionEditor::alterReviewers(
|
2011-01-30 12:08:40 -08:00
|
|
|
$revision,
|
|
|
|
$reviewer_phids,
|
|
|
|
$rem = array(),
|
|
|
|
$add = array($actor_phid),
|
|
|
|
$actor_phid);
|
|
|
|
}
|
|
|
|
break;
|
|
|
|
|
|
|
|
case DifferentialAction::ACTION_REQUEST:
|
|
|
|
if (!$actor_is_author) {
|
|
|
|
throw new Exception('You must own a revision to request review.');
|
|
|
|
}
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
|
|
|
|
switch ($revision_status) {
|
|
|
|
case ArcanistDifferentialRevisionStatus::ACCEPTED:
|
|
|
|
case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
|
|
|
|
$revision->setStatus(
|
|
|
|
ArcanistDifferentialRevisionStatus::NEEDS_REVIEW);
|
|
|
|
break;
|
|
|
|
case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
|
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not request review of this revision because it has ".
|
|
|
|
"been abandoned.");
|
|
|
|
case ArcanistDifferentialRevisionStatus::ABANDONED:
|
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not request review of this revision because it has ".
|
|
|
|
"been abandoned.");
|
2012-04-23 17:40:57 -07:00
|
|
|
case ArcanistDifferentialRevisionStatus::CLOSED:
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not request review of this revision because it has ".
|
2012-04-23 17:40:57 -07:00
|
|
|
"already been closed.");
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
default:
|
|
|
|
throw new Exception(
|
|
|
|
"Unexpected revision state '{$revision_status}'!");
|
2011-01-30 12:08:40 -08:00
|
|
|
}
|
|
|
|
|
2012-06-26 18:48:53 -07:00
|
|
|
list($added_reviewers, $ignored) = $this->alterReviewers();
|
2012-01-23 18:34:46 -08:00
|
|
|
if ($added_reviewers) {
|
|
|
|
$key = DifferentialComment::METADATA_ADDED_REVIEWERS;
|
|
|
|
$metadata[$key] = $added_reviewers;
|
|
|
|
}
|
|
|
|
|
2011-01-30 12:08:40 -08:00
|
|
|
break;
|
|
|
|
|
|
|
|
case DifferentialAction::ACTION_REJECT:
|
|
|
|
if ($actor_is_author) {
|
|
|
|
throw new Exception(
|
|
|
|
'You can not request changes to your own revision.');
|
|
|
|
}
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
|
|
|
|
switch ($revision_status) {
|
|
|
|
case ArcanistDifferentialRevisionStatus::ACCEPTED:
|
|
|
|
case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
|
|
|
|
case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
|
|
|
|
// NOTE: We allow you to reject an already-rejected revision
|
|
|
|
// because it doesn't create any ambiguity and avoids a rather
|
|
|
|
// needless dialog.
|
|
|
|
break;
|
|
|
|
case ArcanistDifferentialRevisionStatus::ABANDONED:
|
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not request changes to this revision because it has ".
|
|
|
|
"been abandoned.");
|
2012-04-23 17:40:57 -07:00
|
|
|
case ArcanistDifferentialRevisionStatus::CLOSED:
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not request changes to this revision because it has ".
|
2012-04-23 17:40:57 -07:00
|
|
|
"already been closed.");
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
default:
|
|
|
|
throw new Exception(
|
|
|
|
"Unexpected revision state '{$revision_status}'!");
|
2011-01-30 12:08:40 -08:00
|
|
|
}
|
|
|
|
|
|
|
|
if (!isset($reviewer_phids[$actor_phid])) {
|
2011-02-04 22:45:42 -08:00
|
|
|
DifferentialRevisionEditor::alterReviewers(
|
2011-01-30 12:08:40 -08:00
|
|
|
$revision,
|
|
|
|
$reviewer_phids,
|
|
|
|
$rem = array(),
|
|
|
|
$add = array($actor_phid),
|
|
|
|
$actor_phid);
|
|
|
|
}
|
|
|
|
|
|
|
|
$revision
|
2012-01-12 19:22:09 -08:00
|
|
|
->setStatus(ArcanistDifferentialRevisionStatus::NEEDS_REVISION);
|
2011-01-30 12:08:40 -08:00
|
|
|
break;
|
|
|
|
|
2011-04-13 16:10:54 -07:00
|
|
|
case DifferentialAction::ACTION_RETHINK:
|
|
|
|
if (!$actor_is_author) {
|
|
|
|
throw new Exception(
|
|
|
|
"You can not plan changes to somebody else's revision");
|
|
|
|
}
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
|
|
|
|
switch ($revision_status) {
|
|
|
|
case ArcanistDifferentialRevisionStatus::ACCEPTED:
|
|
|
|
case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
|
|
|
|
case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
|
|
|
|
break;
|
|
|
|
case ArcanistDifferentialRevisionStatus::ABANDONED:
|
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not plan changes to this revision because it has ".
|
|
|
|
"been abandoned.");
|
2012-04-23 17:40:57 -07:00
|
|
|
case ArcanistDifferentialRevisionStatus::CLOSED:
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not plan changes to this revision because it has ".
|
2012-04-23 17:40:57 -07:00
|
|
|
"already been closed.");
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
default:
|
|
|
|
throw new Exception(
|
|
|
|
"Unexpected revision state '{$revision_status}'!");
|
2011-04-13 16:10:54 -07:00
|
|
|
}
|
|
|
|
|
|
|
|
$revision
|
2012-01-12 19:22:09 -08:00
|
|
|
->setStatus(ArcanistDifferentialRevisionStatus::NEEDS_REVISION);
|
2011-04-13 16:10:54 -07:00
|
|
|
break;
|
|
|
|
|
2011-01-30 12:08:40 -08:00
|
|
|
case DifferentialAction::ACTION_RECLAIM:
|
|
|
|
if (!$actor_is_author) {
|
|
|
|
throw new Exception('You can not reclaim a revision you do not own.');
|
|
|
|
}
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
|
|
|
|
|
|
|
|
if ($revision_status != ArcanistDifferentialRevisionStatus::ABANDONED) {
|
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not reclaim this revision because it is not abandoned.");
|
2011-01-30 12:08:40 -08:00
|
|
|
}
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
|
2011-01-30 12:08:40 -08:00
|
|
|
$revision
|
2012-01-12 19:22:09 -08:00
|
|
|
->setStatus(ArcanistDifferentialRevisionStatus::NEEDS_REVIEW);
|
2011-01-30 12:08:40 -08:00
|
|
|
break;
|
|
|
|
|
2012-04-23 17:40:57 -07:00
|
|
|
case DifferentialAction::ACTION_CLOSE:
|
2012-03-07 09:47:31 -08:00
|
|
|
|
2012-04-23 17:40:57 -07:00
|
|
|
// NOTE: The daemons can mark things closed from any state. We treat
|
2012-03-07 10:20:17 -08:00
|
|
|
// them as completely authoritative.
|
|
|
|
|
|
|
|
if (!$this->isDaemonWorkflow) {
|
2012-08-28 14:17:23 -07:00
|
|
|
if (!$actor_is_author && !$always_allow_close) {
|
2012-03-07 10:20:17 -08:00
|
|
|
throw new Exception(
|
2012-04-23 17:40:57 -07:00
|
|
|
"You can not mark a revision you don't own as closed.");
|
2012-03-07 10:20:17 -08:00
|
|
|
}
|
|
|
|
|
2012-04-23 17:40:57 -07:00
|
|
|
$status_closed = ArcanistDifferentialRevisionStatus::CLOSED;
|
2012-03-07 10:20:17 -08:00
|
|
|
$status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED;
|
|
|
|
|
2012-04-23 17:40:57 -07:00
|
|
|
if ($revision_status == $status_closed) {
|
2012-03-07 10:20:17 -08:00
|
|
|
throw new DifferentialActionHasNoEffectException(
|
2012-04-23 17:40:57 -07:00
|
|
|
"You can not mark this revision as closed because it has ".
|
|
|
|
"already been marked as closed.");
|
2012-03-07 10:20:17 -08:00
|
|
|
}
|
|
|
|
|
|
|
|
if ($revision_status != $status_accepted) {
|
|
|
|
throw new DifferentialActionHasNoEffectException(
|
2012-04-23 17:40:57 -07:00
|
|
|
"You can not mark this revision as closed because it is ".
|
|
|
|
"has not been accepted.");
|
2012-03-07 10:20:17 -08:00
|
|
|
}
|
|
|
|
}
|
2012-03-07 09:47:31 -08:00
|
|
|
|
2012-04-19 00:17:58 -07:00
|
|
|
if (!$revision->getDateCommitted()) {
|
|
|
|
$revision->setDateCommitted(time());
|
|
|
|
}
|
|
|
|
|
2012-04-23 17:40:57 -07:00
|
|
|
$revision->setStatus(ArcanistDifferentialRevisionStatus::CLOSED);
|
2011-01-30 12:08:40 -08:00
|
|
|
break;
|
|
|
|
|
2013-01-19 09:10:15 -08:00
|
|
|
case DifferentialAction::ACTION_REOPEN:
|
|
|
|
if (!$allow_reopen) {
|
|
|
|
throw new Exception(
|
|
|
|
"You cannot reopen a revision when this action is disabled.");
|
|
|
|
}
|
|
|
|
|
|
|
|
if ($revision_status != ArcanistDifferentialRevisionStatus::CLOSED) {
|
|
|
|
throw new Exception(
|
|
|
|
"You cannot reopen a revision that is not currently closed.");
|
|
|
|
}
|
|
|
|
|
|
|
|
$revision->setStatus(ArcanistDifferentialRevisionStatus::NEEDS_REVIEW);
|
|
|
|
|
|
|
|
break;
|
|
|
|
|
2011-01-30 12:08:40 -08:00
|
|
|
case DifferentialAction::ACTION_ADDREVIEWERS:
|
2012-06-26 18:48:53 -07:00
|
|
|
list($added_reviewers, $ignored) = $this->alterReviewers();
|
2011-01-30 12:08:40 -08:00
|
|
|
|
|
|
|
if ($added_reviewers) {
|
2011-06-01 11:19:55 -07:00
|
|
|
$key = DifferentialComment::METADATA_ADDED_REVIEWERS;
|
|
|
|
$metadata[$key] = $added_reviewers;
|
2011-01-30 12:08:40 -08:00
|
|
|
} else {
|
2012-01-23 18:34:46 -08:00
|
|
|
$user_tried_to_add = count($this->getAddedReviewers());
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
if ($user_tried_to_add == 0) {
|
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not add reviewers, because you did not specify any ".
|
|
|
|
"reviewers.");
|
|
|
|
} else if ($user_tried_to_add == 1) {
|
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not add that reviewer, because they are already an ".
|
|
|
|
"author or reviewer.");
|
|
|
|
} else {
|
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not add those reviewers, because they are all already ".
|
|
|
|
"authors or reviewers.");
|
|
|
|
}
|
2011-01-30 12:08:40 -08:00
|
|
|
}
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
|
2011-01-30 12:08:40 -08:00
|
|
|
break;
|
2011-06-24 12:21:48 -07:00
|
|
|
case DifferentialAction::ACTION_ADDCCS:
|
|
|
|
$added_ccs = $this->getAddedCCs();
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
$user_tried_to_add = count($added_ccs);
|
2011-06-24 12:21:48 -07:00
|
|
|
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
$added_ccs = $this->filterAddedCCs($added_ccs);
|
2011-01-30 12:08:40 -08:00
|
|
|
|
2011-06-24 12:21:48 -07:00
|
|
|
if ($added_ccs) {
|
|
|
|
foreach ($added_ccs as $cc) {
|
|
|
|
DifferentialRevisionEditor::addCC(
|
|
|
|
$revision,
|
|
|
|
$cc,
|
2012-10-10 10:18:23 -07:00
|
|
|
$actor_phid);
|
2011-06-24 12:21:48 -07:00
|
|
|
}
|
|
|
|
|
|
|
|
$key = DifferentialComment::METADATA_ADDED_CCS;
|
|
|
|
$metadata[$key] = $added_ccs;
|
|
|
|
|
|
|
|
} else {
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
if ($user_tried_to_add == 0) {
|
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not add CCs, because you did not specify any ".
|
|
|
|
"CCs.");
|
|
|
|
} else if ($user_tried_to_add == 1) {
|
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not add that CC, because they are already an ".
|
|
|
|
"author, reviewer or CC.");
|
|
|
|
} else {
|
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not add those CCs, because they are all already ".
|
|
|
|
"authors, reviewers or CCs.");
|
|
|
|
}
|
2011-06-24 12:21:48 -07:00
|
|
|
}
|
2012-04-17 14:59:31 -07:00
|
|
|
break;
|
|
|
|
case DifferentialAction::ACTION_CLAIM:
|
|
|
|
if ($actor_is_author) {
|
|
|
|
throw new Exception("You can not commandeer your own revision.");
|
|
|
|
}
|
|
|
|
|
|
|
|
switch ($revision_status) {
|
2012-04-23 17:40:57 -07:00
|
|
|
case ArcanistDifferentialRevisionStatus::CLOSED:
|
2012-04-17 14:59:31 -07:00
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not commandeer this revision because it has ".
|
2012-04-23 17:40:57 -07:00
|
|
|
"already been closed.");
|
2012-04-17 14:59:31 -07:00
|
|
|
break;
|
|
|
|
}
|
|
|
|
|
|
|
|
$this->setAddedReviewers(array($revision->getAuthorPHID()));
|
2012-06-26 18:48:53 -07:00
|
|
|
$this->setRemovedReviewers(array($actor_phid));
|
2012-04-17 14:59:31 -07:00
|
|
|
|
|
|
|
// NOTE: Set the new author PHID before calling addReviewers(), since it
|
|
|
|
// doesn't permit the author to become a reviewer.
|
|
|
|
$revision->setAuthorPHID($actor_phid);
|
|
|
|
|
2012-06-26 18:48:53 -07:00
|
|
|
list($added_reviewers, $removed_reviewers) = $this->alterReviewers();
|
2012-04-17 14:59:31 -07:00
|
|
|
if ($added_reviewers) {
|
|
|
|
$key = DifferentialComment::METADATA_ADDED_REVIEWERS;
|
|
|
|
$metadata[$key] = $added_reviewers;
|
|
|
|
}
|
|
|
|
|
2012-06-26 18:48:53 -07:00
|
|
|
if ($removed_reviewers) {
|
|
|
|
$key = DifferentialComment::METADATA_REMOVED_REVIEWERS;
|
|
|
|
$metadata[$key] = $removed_reviewers;
|
|
|
|
}
|
|
|
|
|
2011-06-24 12:21:48 -07:00
|
|
|
break;
|
2011-01-30 12:08:40 -08:00
|
|
|
default:
|
|
|
|
throw new Exception('Unsupported action.');
|
|
|
|
}
|
|
|
|
|
2012-03-05 10:51:47 -08:00
|
|
|
// Update information about reviewer in charge.
|
|
|
|
if ($action == DifferentialAction::ACTION_ACCEPT ||
|
|
|
|
$action == DifferentialAction::ACTION_REJECT) {
|
|
|
|
$revision->setLastReviewerPHID($actor_phid);
|
|
|
|
}
|
|
|
|
|
2012-05-11 16:36:08 -07:00
|
|
|
// TODO: Call beginReadLocking() prior to loading the revision.
|
|
|
|
$revision->openTransaction();
|
|
|
|
|
2012-01-12 19:22:09 -08:00
|
|
|
// Always save the revision (even if we didn't actually change any of its
|
|
|
|
// properties) so that it jumps to the top of the revision list when sorted
|
|
|
|
// by "updated". Notably, this allows "ping" comments to push it to the
|
|
|
|
// top of the action list.
|
|
|
|
$revision->save();
|
|
|
|
|
2012-03-30 06:25:49 -07:00
|
|
|
if ($action != DifferentialAction::ACTION_RESIGN) {
|
2011-02-19 15:06:22 -08:00
|
|
|
DifferentialRevisionEditor::addCC(
|
|
|
|
$revision,
|
2012-10-10 10:18:23 -07:00
|
|
|
$actor_phid,
|
|
|
|
$actor_phid);
|
2011-02-19 15:06:22 -08:00
|
|
|
}
|
|
|
|
|
2011-01-30 12:08:40 -08:00
|
|
|
$comment = id(new DifferentialComment())
|
2012-10-10 10:18:23 -07:00
|
|
|
->setAuthorPHID($actor_phid)
|
2011-01-30 12:08:40 -08:00
|
|
|
->setRevisionID($revision->getID())
|
|
|
|
->setAction($action)
|
|
|
|
->setContent((string)$this->message)
|
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 10:25:45 -07:00
|
|
|
->setMetadata($metadata);
|
|
|
|
|
|
|
|
if ($this->contentSource) {
|
|
|
|
$comment->setContentSource($this->contentSource);
|
|
|
|
}
|
|
|
|
|
|
|
|
$comment->save();
|
2011-01-30 12:08:40 -08:00
|
|
|
|
2011-02-19 15:06:22 -08:00
|
|
|
$changesets = array();
|
2011-01-30 12:08:40 -08:00
|
|
|
if ($inline_comments) {
|
2011-02-19 15:06:22 -08:00
|
|
|
$load_ids = mpull($inline_comments, 'getChangesetID');
|
|
|
|
if ($load_ids) {
|
|
|
|
$load_ids = array_unique($load_ids);
|
|
|
|
$changesets = id(new DifferentialChangeset())->loadAllWhere(
|
|
|
|
'id in (%Ld)',
|
|
|
|
$load_ids);
|
2011-01-30 12:08:40 -08:00
|
|
|
}
|
|
|
|
foreach ($inline_comments as $inline) {
|
2011-02-02 19:38:43 -08:00
|
|
|
$inline->setCommentID($comment->getID());
|
2011-01-30 12:08:40 -08:00
|
|
|
$inline->save();
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2011-06-24 11:50:19 -07:00
|
|
|
// Find any "@mentions" in the comment blocks.
|
|
|
|
$content_blocks = array($comment->getContent());
|
|
|
|
foreach ($inline_comments as $inline) {
|
|
|
|
$content_blocks[] = $inline->getContent();
|
|
|
|
}
|
Generalize the markup engine factory
Summary:
This thing services every app but it lives inside Differential right now. Pull
it out, and separate the factory interfaces per-application.
This will let us accommodate changes we need to make for Phriction to support
wiki linking.
Test Plan: Tested remarkup in differential, diffusion, maniphest, people,
slowvote.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, hsb
Differential Revision: 646
2011-07-11 15:58:32 -07:00
|
|
|
$mention_ccs = PhabricatorMarkupEngine::extractPHIDsFromMentions(
|
2011-06-24 11:50:19 -07:00
|
|
|
$content_blocks);
|
|
|
|
if ($mention_ccs) {
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
$mention_ccs = $this->filterAddedCCs($mention_ccs);
|
2011-06-24 11:50:19 -07:00
|
|
|
if ($mention_ccs) {
|
|
|
|
$metadata = $comment->getMetadata();
|
|
|
|
$metacc = idx(
|
|
|
|
$metadata,
|
|
|
|
DifferentialComment::METADATA_ADDED_CCS,
|
|
|
|
array());
|
|
|
|
foreach ($mention_ccs as $cc_phid) {
|
|
|
|
DifferentialRevisionEditor::addCC(
|
|
|
|
$revision,
|
|
|
|
$cc_phid,
|
2012-10-10 10:18:23 -07:00
|
|
|
$actor_phid);
|
2011-06-24 11:50:19 -07:00
|
|
|
$metacc[] = $cc_phid;
|
|
|
|
}
|
|
|
|
$metadata[DifferentialComment::METADATA_ADDED_CCS] = $metacc;
|
|
|
|
|
|
|
|
$comment->setMetadata($metadata);
|
|
|
|
$comment->save();
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2012-05-11 16:36:08 -07:00
|
|
|
$revision->saveTransaction();
|
|
|
|
|
2012-10-10 10:18:23 -07:00
|
|
|
$phids = array($actor_phid);
|
2011-02-09 09:58:48 -08:00
|
|
|
$handles = id(new PhabricatorObjectHandleData($phids))
|
|
|
|
->loadHandles();
|
2012-10-10 10:18:23 -07:00
|
|
|
$actor_handle = $handles[$actor_phid];
|
2011-02-09 09:58:48 -08:00
|
|
|
|
2011-05-02 21:04:06 -07:00
|
|
|
$xherald_header = HeraldTranscript::loadXHeraldRulesHeader(
|
|
|
|
$revision->getPHID());
|
|
|
|
|
2012-10-23 12:03:11 -07:00
|
|
|
$mailed_phids = array();
|
2012-08-20 14:08:52 -07:00
|
|
|
if (!$this->noEmail) {
|
2012-10-23 12:03:11 -07:00
|
|
|
$mail = id(new DifferentialCommentMail(
|
2012-08-20 14:08:52 -07:00
|
|
|
$revision,
|
|
|
|
$actor_handle,
|
|
|
|
$comment,
|
|
|
|
$changesets,
|
|
|
|
$inline_comments))
|
2012-10-10 10:18:23 -07:00
|
|
|
->setExcludeMailRecipientPHIDs($this->getExcludeMailRecipientPHIDs())
|
2012-08-20 14:08:52 -07:00
|
|
|
->setToPHIDs(
|
|
|
|
array_merge(
|
|
|
|
$revision->getReviewers(),
|
|
|
|
array($revision->getAuthorPHID())))
|
|
|
|
->setCCPHIDs($revision->getCCPHIDs())
|
|
|
|
->setChangedByCommit($this->getChangedByCommit())
|
|
|
|
->setXHeraldRulesHeader($xherald_header)
|
|
|
|
->setParentMessageID($this->parentMessageID)
|
|
|
|
->send();
|
2012-10-23 12:03:11 -07:00
|
|
|
|
|
|
|
$mailed_phids = $mail->getRawMail()->buildRecipientList();
|
2012-08-20 14:08:52 -07:00
|
|
|
}
|
2011-01-30 12:08:40 -08:00
|
|
|
|
2011-04-11 15:20:51 -07:00
|
|
|
$event_data = array(
|
|
|
|
'revision_id' => $revision->getID(),
|
|
|
|
'revision_phid' => $revision->getPHID(),
|
|
|
|
'revision_name' => $revision->getTitle(),
|
|
|
|
'revision_author_phid' => $revision->getAuthorPHID(),
|
|
|
|
'action' => $comment->getAction(),
|
|
|
|
'feedback_content' => $comment->getContent(),
|
2012-10-10 10:18:23 -07:00
|
|
|
'actor_phid' => $actor_phid,
|
2011-01-30 12:08:40 -08:00
|
|
|
);
|
2012-10-23 12:03:11 -07:00
|
|
|
|
|
|
|
// TODO: Get rid of this
|
2011-04-11 15:20:51 -07:00
|
|
|
id(new PhabricatorTimelineEvent('difx', $event_data))
|
|
|
|
->recordEvent();
|
2011-01-30 12:08:40 -08:00
|
|
|
|
2011-07-09 15:44:49 -07:00
|
|
|
id(new PhabricatorFeedStoryPublisher())
|
2012-10-23 12:03:11 -07:00
|
|
|
->setStoryType('PhabricatorFeedStoryDifferential')
|
2011-07-09 15:44:49 -07:00
|
|
|
->setStoryData($event_data)
|
|
|
|
->setStoryTime(time())
|
2012-10-10 10:18:23 -07:00
|
|
|
->setStoryAuthorPHID($actor_phid)
|
2011-07-09 15:44:49 -07:00
|
|
|
->setRelatedPHIDs(
|
|
|
|
array(
|
|
|
|
$revision->getPHID(),
|
2012-10-10 10:18:23 -07:00
|
|
|
$actor_phid,
|
2011-07-09 15:44:49 -07:00
|
|
|
$revision->getAuthorPHID(),
|
|
|
|
))
|
2012-06-08 18:45:26 -07:00
|
|
|
->setPrimaryObjectPHID($revision->getPHID())
|
|
|
|
->setSubscribedPHIDs(
|
|
|
|
array_merge(
|
|
|
|
array($revision->getAuthorPHID()),
|
|
|
|
$revision->getReviewers(),
|
|
|
|
$revision->getCCPHIDs()))
|
2012-10-23 12:03:11 -07:00
|
|
|
->setMailRecipientPHIDs($mailed_phids)
|
2011-07-09 15:44:49 -07: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 14:21:31 -08:00
|
|
|
id(new PhabricatorSearchIndexer())
|
|
|
|
->indexDocumentByPHID($revision->getPHID());
|
2011-06-15 07:43:43 -07:00
|
|
|
|
2011-01-30 12:08:40 -08:00
|
|
|
return $comment;
|
|
|
|
}
|
|
|
|
|
Improve behavior when user submits a no-op action in Differential
Summary:
See T730 and the slightly-less-pretty version of this in D1398.
When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:
Action Has No Effect
You can not accept this revision because it has already been accepted.
Do you want to post the feedback anyway, as a normal comment?
[Cancel] [Post as Comment]
If they have no comment text, the dialog only says "Cancel".
I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).
This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.
Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran, vrana
Maniphest Tasks: T730
Differential Revision: https://secure.phabricator.com/D1403
2012-01-14 11:39:22 -08:00
|
|
|
private function filterAddedCCs(array $ccs) {
|
|
|
|
$revision = $this->revision;
|
|
|
|
|
|
|
|
$current_ccs = $revision->getCCPHIDs();
|
|
|
|
$current_ccs = array_fill_keys($current_ccs, true);
|
|
|
|
|
|
|
|
$reviewer_phids = $revision->getReviewers();
|
|
|
|
$reviewer_phids = array_fill_keys($reviewer_phids, true);
|
|
|
|
|
|
|
|
foreach ($ccs as $key => $cc) {
|
|
|
|
if (isset($current_ccs[$cc])) {
|
|
|
|
unset($ccs[$key]);
|
|
|
|
}
|
|
|
|
if (isset($reviewer_phids[$cc])) {
|
|
|
|
unset($ccs[$key]);
|
|
|
|
}
|
|
|
|
if ($cc == $revision->getAuthorPHID()) {
|
|
|
|
unset($ccs[$key]);
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
return $ccs;
|
|
|
|
}
|
|
|
|
|
2012-06-26 18:48:53 -07:00
|
|
|
private function alterReviewers() {
|
2012-10-10 10:18:23 -07:00
|
|
|
$actor_phid = $this->getActor()->getPHID();
|
|
|
|
$revision = $this->revision;
|
|
|
|
$added_reviewers = $this->getAddedReviewers();
|
2012-06-26 18:48:53 -07:00
|
|
|
$removed_reviewers = $this->getRemovedReviewers();
|
2012-10-10 10:18:23 -07:00
|
|
|
$reviewer_phids = $revision->getReviewers();
|
2012-10-12 07:41:43 -07:00
|
|
|
$allow_self_accept = PhabricatorEnv::getEnvConfig(
|
2013-01-19 12:11:11 -08:00
|
|
|
'differential.allow-self-accept');
|
2012-01-23 18:34:46 -08:00
|
|
|
|
2012-06-26 18:48:53 -07:00
|
|
|
$reviewer_phids_map = array_fill_keys($reviewer_phids, true);
|
2012-01-23 18:34:46 -08:00
|
|
|
foreach ($added_reviewers as $k => $user_phid) {
|
2012-10-12 07:41:43 -07:00
|
|
|
if (!$allow_self_accept && $user_phid == $revision->getAuthorPHID()) {
|
2012-01-23 18:34:46 -08:00
|
|
|
unset($added_reviewers[$k]);
|
|
|
|
}
|
2012-06-26 18:48:53 -07:00
|
|
|
if (isset($reviewer_phids_map[$user_phid])) {
|
2012-01-23 18:34:46 -08:00
|
|
|
unset($added_reviewers[$k]);
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2012-06-26 18:48:53 -07:00
|
|
|
foreach ($removed_reviewers as $k => $user_phid) {
|
|
|
|
if (!isset($reviewer_phids_map[$user_phid])) {
|
|
|
|
unset($removed_reviewers[$k]);
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2012-01-23 18:34:46 -08:00
|
|
|
$added_reviewers = array_unique($added_reviewers);
|
2012-06-26 18:48:53 -07:00
|
|
|
$removed_reviewers = array_unique($removed_reviewers);
|
2012-01-23 18:34:46 -08:00
|
|
|
|
|
|
|
if ($added_reviewers) {
|
|
|
|
DifferentialRevisionEditor::alterReviewers(
|
|
|
|
$revision,
|
|
|
|
$reviewer_phids,
|
2012-06-26 18:48:53 -07:00
|
|
|
$removed_reviewers,
|
2012-01-23 18:34:46 -08:00
|
|
|
$added_reviewers,
|
2012-10-10 10:18:23 -07:00
|
|
|
$actor_phid);
|
2012-01-23 18:34:46 -08:00
|
|
|
}
|
|
|
|
|
2012-06-26 18:48:53 -07:00
|
|
|
return array($added_reviewers, $removed_reviewers);
|
2012-01-23 18:34:46 -08:00
|
|
|
}
|
|
|
|
|
2011-01-30 12:08:40 -08:00
|
|
|
}
|