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() {
|
2013-10-04 18:58:35 -07:00
|
|
|
$actor = $this->requireActor();
|
|
|
|
|
|
|
|
// Reload the revision to pick up reviewer status, until we can lift this
|
|
|
|
// out of here.
|
|
|
|
$this->revision = id(new DifferentialRevisionQuery())
|
|
|
|
->setViewer($actor)
|
|
|
|
->withIDs(array($this->revision->getID()))
|
|
|
|
->needRelationships(true)
|
|
|
|
->needReviewerStatus(true)
|
2013-10-06 17:08:30 -07:00
|
|
|
->needReviewerAuthority(true)
|
2013-10-04 18:58:35 -07:00
|
|
|
->executeOne();
|
|
|
|
|
2012-10-10 10:18:23 -07:00
|
|
|
$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();
|
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-06 17:09:56 -07:00
|
|
|
$update_accepted_status = false;
|
2011-01-30 12:08:40 -08:00
|
|
|
|
|
|
|
$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) {
|
2013-06-21 12:54:56 -07:00
|
|
|
$inline_comments = id(new DifferentialInlineCommentQuery())
|
|
|
|
->withDraftComments($actor_phid, $revision->getID())
|
|
|
|
->execute();
|
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
|
|
|
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.");
|
|
|
|
}
|
2013-10-04 18:58:35 -07:00
|
|
|
|
|
|
|
// If the actor is a reviewer, and their status is "added" (that is,
|
|
|
|
// they haven't accepted or requested changes to the revision),
|
|
|
|
// upgrade their status to "commented". If they have a stronger status
|
|
|
|
// already, don't overwrite it.
|
|
|
|
if (isset($reviewer_phids[$actor_phid])) {
|
|
|
|
$status_added = DifferentialReviewerStatus::STATUS_ADDED;
|
|
|
|
$reviewer_status = $revision->getReviewerStatus();
|
|
|
|
foreach ($reviewer_status as $reviewer) {
|
|
|
|
if ($reviewer->getReviewerPHID() == $actor_phid) {
|
|
|
|
if ($reviewer->getStatus() == $status_added) {
|
|
|
|
DifferentialRevisionEditor::updateReviewerStatus(
|
|
|
|
$revision,
|
2013-10-06 17:08:30 -07:00
|
|
|
$actor,
|
2013-10-04 18:58:35 -07:00
|
|
|
$actor_phid,
|
|
|
|
DifferentialReviewerStatus::STATUS_COMMENTED);
|
|
|
|
}
|
|
|
|
}
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
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
|
|
|
}
|
2013-07-10 11:05:53 -07:00
|
|
|
|
|
|
|
list($added_reviewers, $ignored) = $this->alterReviewers();
|
|
|
|
if ($added_reviewers) {
|
|
|
|
$key = DifferentialComment::METADATA_ADDED_REVIEWERS;
|
|
|
|
$metadata[$key] = $added_reviewers;
|
|
|
|
}
|
|
|
|
|
2013-07-10 13:45:14 -07:00
|
|
|
DifferentialRevisionEditor::updateReviewers(
|
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,
|
2013-07-10 13:45:14 -07:00
|
|
|
$actor,
|
|
|
|
array(),
|
|
|
|
array($actor_phid));
|
|
|
|
|
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-06 17:09:56 -07:00
|
|
|
// If you are a blocking reviewer, your presence as a reviewer may be
|
|
|
|
// the only thing keeping a revision from transitioning to "accepted".
|
|
|
|
// Recalculate state after removing the resigning reviewer.
|
|
|
|
switch ($revision_status) {
|
|
|
|
case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
|
|
|
|
case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
|
|
|
|
$update_accepted_status = true;
|
|
|
|
break;
|
|
|
|
}
|
|
|
|
|
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.');
|
|
|
|
}
|
2013-10-06 17:08:30 -07:00
|
|
|
|
2013-10-06 17:09:02 -07:00
|
|
|
switch ($revision_status) {
|
|
|
|
case ArcanistDifferentialRevisionStatus::ABANDONED:
|
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not accept this revision because it has been ".
|
|
|
|
"abandoned.");
|
|
|
|
case ArcanistDifferentialRevisionStatus::CLOSED:
|
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not accept this revision because it has already ".
|
|
|
|
"been closed.");
|
|
|
|
case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
|
|
|
|
case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
|
|
|
|
case ArcanistDifferentialRevisionStatus::ACCEPTED:
|
|
|
|
// We expect "Accept" from these states.
|
|
|
|
break;
|
|
|
|
default:
|
|
|
|
throw new Exception(
|
|
|
|
"Unexpected revision state '{$revision_status}'!");
|
2011-01-30 12:08:40 -08:00
|
|
|
}
|
|
|
|
|
2013-10-06 17:08:30 -07:00
|
|
|
$was_reviewer_already = false;
|
|
|
|
foreach ($revision->getReviewerStatus() as $reviewer) {
|
|
|
|
if ($reviewer->hasAuthority($actor)) {
|
|
|
|
DifferentialRevisionEditor::updateReviewerStatus(
|
|
|
|
$revision,
|
|
|
|
$actor,
|
|
|
|
$reviewer->getReviewerPHID(),
|
|
|
|
DifferentialReviewerStatus::STATUS_ACCEPTED);
|
|
|
|
if ($reviewer->getReviewerPHID() == $actor_phid) {
|
|
|
|
$was_reviewer_already = true;
|
|
|
|
}
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
if (!$was_reviewer_already) {
|
|
|
|
DifferentialRevisionEditor::updateReviewerStatus(
|
|
|
|
$revision,
|
|
|
|
$actor,
|
|
|
|
$actor_phid,
|
|
|
|
DifferentialReviewerStatus::STATUS_ACCEPTED);
|
|
|
|
}
|
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-06 17:09:56 -07:00
|
|
|
|
|
|
|
$update_accepted_status = true;
|
2011-01-30 12:08:40 -08:00
|
|
|
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:
|
2013-10-06 17:09:02 -07:00
|
|
|
// We expect rejects from these states.
|
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
|
|
|
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
|
|
|
}
|
|
|
|
|
2013-07-10 13:45:14 -07:00
|
|
|
DifferentialRevisionEditor::updateReviewerStatus(
|
|
|
|
$revision,
|
2013-10-06 17:08:30 -07:00
|
|
|
$actor,
|
2013-07-10 13:45:14 -07:00
|
|
|
$actor_phid,
|
|
|
|
DifferentialReviewerStatus::STATUS_REJECTED);
|
2011-01-30 12:08:40 -08:00
|
|
|
|
|
|
|
$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:
|
2013-10-06 17:09:02 -07:00
|
|
|
// We expect accepts from these states.
|
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
|
|
|
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);
|
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-06 17:09:56 -07:00
|
|
|
|
|
|
|
$update_accepted_status = true;
|
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();
|
|
|
|
|
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-06 17:09:56 -07:00
|
|
|
if ($update_accepted_status) {
|
|
|
|
$revision = DifferentialRevisionEditor::updateAcceptedStatus(
|
|
|
|
$actor,
|
|
|
|
$revision);
|
|
|
|
}
|
|
|
|
|
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
|
|
|
}
|
|
|
|
|
2013-05-24 06:38:54 -07:00
|
|
|
$is_new = !$revision->getID();
|
|
|
|
|
|
|
|
$event = new PhabricatorEvent(
|
|
|
|
PhabricatorEventType::TYPE_DIFFERENTIAL_WILLEDITREVISION,
|
|
|
|
array(
|
|
|
|
'revision' => $revision,
|
|
|
|
'new' => $is_new,
|
|
|
|
));
|
|
|
|
|
|
|
|
$event->setUser($actor);
|
|
|
|
PhutilEventEngine::dispatchEvent($event);
|
|
|
|
|
2011-01-30 12:08:40 -08:00
|
|
|
$comment = id(new DifferentialComment())
|
2012-10-10 10:18:23 -07:00
|
|
|
->setAuthorPHID($actor_phid)
|
2013-10-21 17:01:27 -07:00
|
|
|
->setRevision($revision)
|
2011-01-30 12:08:40 -08:00
|
|
|
->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();
|
2013-05-24 06:38:54 -07:00
|
|
|
|
|
|
|
$event = new PhabricatorEvent(
|
|
|
|
PhabricatorEventType::TYPE_DIFFERENTIAL_DIDEDITREVISION,
|
|
|
|
array(
|
|
|
|
'revision' => $revision,
|
|
|
|
'new' => $is_new,
|
|
|
|
));
|
|
|
|
$event->setUser($actor);
|
|
|
|
PhutilEventEngine::dispatchEvent($event);
|
2011-06-24 11:50:19 -07:00
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2012-05-11 16:36:08 -07:00
|
|
|
$revision->saveTransaction();
|
|
|
|
|
2012-10-10 10:18:23 -07:00
|
|
|
$phids = array($actor_phid);
|
2013-09-11 12:27:28 -07:00
|
|
|
$handles = id(new PhabricatorHandleQuery())
|
2013-10-06 17:08:30 -07:00
|
|
|
->setViewer($actor)
|
2013-09-11 12:27:28 -07:00
|
|
|
->withPHIDs($phids)
|
|
|
|
->execute();
|
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))
|
2013-10-06 17:08:30 -07:00
|
|
|
->setActor($actor)
|
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,
|
2013-08-13 10:16:56 -07:00
|
|
|
|
|
|
|
// NOTE: Don't use this, it will be removed after ApplicationTransactions.
|
|
|
|
// For now, it powers inline comment rendering over the Asana brdige.
|
|
|
|
'temporaryCommentID' => $comment->getID(),
|
2011-01-30 12:08:40 -08:00
|
|
|
);
|
2012-10-23 12:03:11 -07: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) {
|
2013-07-10 13:45:14 -07:00
|
|
|
DifferentialRevisionEditor::updateReviewers(
|
2012-01-23 18:34:46 -08:00
|
|
|
$revision,
|
2013-07-10 13:45:14 -07:00
|
|
|
$this->getActor(),
|
2012-01-23 18:34:46 -08:00
|
|
|
$added_reviewers,
|
2013-07-10 13:45:14 -07:00
|
|
|
$removed_reviewers);
|
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
|
|
|
}
|