2011-01-30 21:08:40 +01:00
|
|
|
<?php
|
|
|
|
|
|
|
|
/*
|
2012-01-10 20:39:11 +01:00
|
|
|
* Copyright 2012 Facebook, Inc.
|
2011-01-30 21:08:40 +01:00
|
|
|
*
|
|
|
|
* Licensed under the Apache License, Version 2.0 (the "License");
|
|
|
|
* you may not use this file except in compliance with the License.
|
|
|
|
* You may obtain a copy of the License at
|
|
|
|
*
|
|
|
|
* http://www.apache.org/licenses/LICENSE-2.0
|
|
|
|
*
|
|
|
|
* Unless required by applicable law or agreed to in writing, software
|
|
|
|
* distributed under the License is distributed on an "AS IS" BASIS,
|
|
|
|
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
|
|
|
* See the License for the specific language governing permissions and
|
|
|
|
* limitations under the License.
|
|
|
|
*/
|
|
|
|
|
2012-03-13 19:18:11 +01:00
|
|
|
final class DifferentialCommentEditor {
|
2011-01-30 21:08:40 +01:00
|
|
|
|
|
|
|
protected $revision;
|
|
|
|
protected $actorPHID;
|
|
|
|
protected $action;
|
|
|
|
|
|
|
|
protected $attachInlineComments;
|
|
|
|
protected $message;
|
|
|
|
protected $changedByCommit;
|
|
|
|
protected $addedReviewers = array();
|
2012-06-27 03:48:53 +02:00
|
|
|
protected $removedReviewers = array();
|
2011-06-24 21:21:48 +02:00
|
|
|
private $addedCCs = array();
|
2011-01-30 21:08:40 +01:00
|
|
|
|
2011-06-22 21:41:19 +02: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 19:25:45 +02:00
|
|
|
private $contentSource;
|
2011-06-22 21:41:19 +02:00
|
|
|
|
2012-03-07 19:20:17 +01:00
|
|
|
private $isDaemonWorkflow;
|
|
|
|
|
2011-01-30 21:08:40 +01:00
|
|
|
public function __construct(
|
|
|
|
DifferentialRevision $revision,
|
|
|
|
$actor_phid,
|
|
|
|
$action) {
|
|
|
|
|
|
|
|
$this->revision = $revision;
|
|
|
|
$this->actorPHID = $actor_phid;
|
|
|
|
$this->action = $action;
|
|
|
|
}
|
|
|
|
|
2011-06-22 21:41:19 +02:00
|
|
|
public function setParentMessageID($parent_message_id) {
|
|
|
|
$this->parentMessageID = $parent_message_id;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
2011-01-30 21:08:40 +01: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 23:59:31 +02:00
|
|
|
public function setAddedReviewers(array $added_reviewers) {
|
2011-01-30 21:08:40 +01:00
|
|
|
$this->addedReviewers = $added_reviewers;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
public function getAddedReviewers() {
|
|
|
|
return $this->addedReviewers;
|
|
|
|
}
|
|
|
|
|
2012-06-27 03:48:53 +02:00
|
|
|
public function setRemovedReviewers(array $removeded_reviewers) {
|
|
|
|
$this->removedReviewers = $removeded_reviewers;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
public function getRemovedReviewers() {
|
|
|
|
return $this->removedReviewers;
|
|
|
|
}
|
|
|
|
|
2011-06-24 21:21:48 +02: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 19:25:45 +02:00
|
|
|
public function setContentSource(PhabricatorContentSource $content_source) {
|
|
|
|
$this->contentSource = $content_source;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
2012-03-07 19:20:17 +01:00
|
|
|
public function setIsDaemonWorkflow($is_daemon) {
|
|
|
|
$this->isDaemonWorkflow = $is_daemon;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
2011-01-30 21:08:40 +01:00
|
|
|
public function save() {
|
|
|
|
$revision = $this->revision;
|
|
|
|
$action = $this->action;
|
|
|
|
$actor_phid = $this->actorPHID;
|
2011-10-25 23:03:34 +02:00
|
|
|
$actor = id(new PhabricatorUser())->loadOneWhere('PHID = %s', $actor_phid);
|
2011-01-30 21:08:40 +01:00
|
|
|
$actor_is_author = ($actor_phid == $revision->getAuthorPHID());
|
|
|
|
$revision_status = $revision->getStatus();
|
|
|
|
|
|
|
|
$revision->loadRelationships();
|
|
|
|
$reviewer_phids = $revision->getReviewers();
|
|
|
|
if ($reviewer_phids) {
|
|
|
|
$reviewer_phids = array_combine($reviewer_phids, $reviewer_phids);
|
|
|
|
}
|
|
|
|
|
2011-06-01 20:19:55 +02: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 20:39:22 +01:00
|
|
|
$inline_comments = array();
|
|
|
|
if ($this->attachInlineComments) {
|
|
|
|
$inline_comments = id(new DifferentialInlineComment())->loadAllWhere(
|
|
|
|
'authorPHID = %s AND revisionID = %d AND commentID IS NULL',
|
|
|
|
$this->actorPHID,
|
|
|
|
$revision->getID());
|
|
|
|
}
|
|
|
|
|
2011-01-30 21:08:40 +01: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 20:39:22 +01: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 21:08:40 +01: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 20:39:22 +01: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 21:08:40 +01: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 20:39:22 +01:00
|
|
|
DifferentialRevisionEditor::alterReviewers(
|
|
|
|
$revision,
|
|
|
|
$reviewer_phids,
|
|
|
|
$rem = array($actor_phid),
|
|
|
|
$add = array(),
|
|
|
|
$actor_phid);
|
2011-01-30 21:08:40 +01:00
|
|
|
break;
|
|
|
|
|
|
|
|
case DifferentialAction::ACTION_ABANDON:
|
2012-04-17 23:59:31 +02: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 20:39:22 +01:00
|
|
|
throw new Exception('You can only abandon your own revisions.');
|
2011-01-30 21:08:40 +01: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 20:39:22 +01:00
|
|
|
|
2012-04-24 02:40:57 +02: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 20:39:22 +01:00
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not abandon this revision because it has already ".
|
2012-04-24 02:40:57 +02:00
|
|
|
"been closed.");
|
2011-01-30 21:08:40 +01: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 20:39:22 +01:00
|
|
|
|
|
|
|
if ($revision_status == ArcanistDifferentialRevisionStatus::ABANDONED) {
|
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not abandon this revision because it has already ".
|
|
|
|
"been abandoned.");
|
2011-01-30 21:08:40 +01: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 20:39:22 +01:00
|
|
|
$revision->setStatus(ArcanistDifferentialRevisionStatus::ABANDONED);
|
2011-01-30 21:08:40 +01:00
|
|
|
break;
|
|
|
|
|
|
|
|
case DifferentialAction::ACTION_ACCEPT:
|
|
|
|
if ($actor_is_author) {
|
|
|
|
throw new Exception('You can not accept your own revision.');
|
|
|
|
}
|
2012-01-10 20:39:11 +01: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 20:39:22 +01: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-24 02:40:57 +02: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 20:39:22 +01:00
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not accept this revision because it has already ".
|
2012-04-24 02:40:57 +02: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 20:39:22 +01:00
|
|
|
default:
|
|
|
|
throw new Exception(
|
|
|
|
"Unexpected revision state '{$revision_status}'!");
|
|
|
|
}
|
2011-01-30 21:08:40 +01:00
|
|
|
}
|
|
|
|
|
|
|
|
$revision
|
2012-01-13 04:22:09 +01:00
|
|
|
->setStatus(ArcanistDifferentialRevisionStatus::ACCEPTED);
|
2011-01-30 21:08:40 +01:00
|
|
|
|
|
|
|
if (!isset($reviewer_phids[$actor_phid])) {
|
2011-02-05 07:45:42 +01:00
|
|
|
DifferentialRevisionEditor::alterReviewers(
|
2011-01-30 21:08:40 +01: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 20:39:22 +01: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-24 02:40:57 +02: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 20:39:22 +01:00
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not request review of this revision because it has ".
|
2012-04-24 02:40:57 +02: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 20:39:22 +01:00
|
|
|
default:
|
|
|
|
throw new Exception(
|
|
|
|
"Unexpected revision state '{$revision_status}'!");
|
2011-01-30 21:08:40 +01:00
|
|
|
}
|
|
|
|
|
2012-06-27 03:48:53 +02:00
|
|
|
list($added_reviewers, $ignored) = $this->alterReviewers();
|
2012-01-24 03:34:46 +01:00
|
|
|
if ($added_reviewers) {
|
|
|
|
$key = DifferentialComment::METADATA_ADDED_REVIEWERS;
|
|
|
|
$metadata[$key] = $added_reviewers;
|
|
|
|
}
|
|
|
|
|
2011-01-30 21:08:40 +01: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 20:39:22 +01: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-24 02:40:57 +02: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 20:39:22 +01:00
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not request changes to this revision because it has ".
|
2012-04-24 02:40:57 +02: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 20:39:22 +01:00
|
|
|
default:
|
|
|
|
throw new Exception(
|
|
|
|
"Unexpected revision state '{$revision_status}'!");
|
2011-01-30 21:08:40 +01:00
|
|
|
}
|
|
|
|
|
|
|
|
if (!isset($reviewer_phids[$actor_phid])) {
|
2011-02-05 07:45:42 +01:00
|
|
|
DifferentialRevisionEditor::alterReviewers(
|
2011-01-30 21:08:40 +01:00
|
|
|
$revision,
|
|
|
|
$reviewer_phids,
|
|
|
|
$rem = array(),
|
|
|
|
$add = array($actor_phid),
|
|
|
|
$actor_phid);
|
|
|
|
}
|
|
|
|
|
|
|
|
$revision
|
2012-01-13 04:22:09 +01:00
|
|
|
->setStatus(ArcanistDifferentialRevisionStatus::NEEDS_REVISION);
|
2011-01-30 21:08:40 +01:00
|
|
|
break;
|
|
|
|
|
2011-04-14 01:10:54 +02: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 20:39:22 +01: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-24 02:40:57 +02: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 20:39:22 +01:00
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not plan changes to this revision because it has ".
|
2012-04-24 02:40:57 +02: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 20:39:22 +01:00
|
|
|
default:
|
|
|
|
throw new Exception(
|
|
|
|
"Unexpected revision state '{$revision_status}'!");
|
2011-04-14 01:10:54 +02:00
|
|
|
}
|
|
|
|
|
|
|
|
$revision
|
2012-01-13 04:22:09 +01:00
|
|
|
->setStatus(ArcanistDifferentialRevisionStatus::NEEDS_REVISION);
|
2011-04-14 01:10:54 +02:00
|
|
|
break;
|
|
|
|
|
2011-01-30 21:08:40 +01: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 20:39:22 +01:00
|
|
|
|
|
|
|
|
|
|
|
if ($revision_status != ArcanistDifferentialRevisionStatus::ABANDONED) {
|
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not reclaim this revision because it is not abandoned.");
|
2011-01-30 21:08:40 +01: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 20:39:22 +01:00
|
|
|
|
2011-01-30 21:08:40 +01:00
|
|
|
$revision
|
2012-01-13 04:22:09 +01:00
|
|
|
->setStatus(ArcanistDifferentialRevisionStatus::NEEDS_REVIEW);
|
2011-01-30 21:08:40 +01:00
|
|
|
break;
|
|
|
|
|
2012-04-24 02:40:57 +02:00
|
|
|
case DifferentialAction::ACTION_CLOSE:
|
2012-03-07 18:47:31 +01:00
|
|
|
|
2012-04-24 02:40:57 +02:00
|
|
|
// NOTE: The daemons can mark things closed from any state. We treat
|
2012-03-07 19:20:17 +01:00
|
|
|
// them as completely authoritative.
|
|
|
|
|
|
|
|
if (!$this->isDaemonWorkflow) {
|
|
|
|
if (!$actor_is_author) {
|
|
|
|
throw new Exception(
|
2012-04-24 02:40:57 +02:00
|
|
|
"You can not mark a revision you don't own as closed.");
|
2012-03-07 19:20:17 +01:00
|
|
|
}
|
|
|
|
|
2012-04-24 02:40:57 +02:00
|
|
|
$status_closed = ArcanistDifferentialRevisionStatus::CLOSED;
|
2012-03-07 19:20:17 +01:00
|
|
|
$status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED;
|
|
|
|
|
2012-04-24 02:40:57 +02:00
|
|
|
if ($revision_status == $status_closed) {
|
2012-03-07 19:20:17 +01:00
|
|
|
throw new DifferentialActionHasNoEffectException(
|
2012-04-24 02:40:57 +02:00
|
|
|
"You can not mark this revision as closed because it has ".
|
|
|
|
"already been marked as closed.");
|
2012-03-07 19:20:17 +01:00
|
|
|
}
|
|
|
|
|
|
|
|
if ($revision_status != $status_accepted) {
|
|
|
|
throw new DifferentialActionHasNoEffectException(
|
2012-04-24 02:40:57 +02:00
|
|
|
"You can not mark this revision as closed because it is ".
|
|
|
|
"has not been accepted.");
|
2012-03-07 19:20:17 +01:00
|
|
|
}
|
|
|
|
}
|
2012-03-07 18:47:31 +01:00
|
|
|
|
2012-04-19 09:17:58 +02:00
|
|
|
if (!$revision->getDateCommitted()) {
|
|
|
|
$revision->setDateCommitted(time());
|
|
|
|
}
|
|
|
|
|
2012-04-24 02:40:57 +02:00
|
|
|
$revision->setStatus(ArcanistDifferentialRevisionStatus::CLOSED);
|
2011-01-30 21:08:40 +01:00
|
|
|
break;
|
|
|
|
|
|
|
|
case DifferentialAction::ACTION_ADDREVIEWERS:
|
2012-06-27 03:48:53 +02:00
|
|
|
list($added_reviewers, $ignored) = $this->alterReviewers();
|
2011-01-30 21:08:40 +01:00
|
|
|
|
|
|
|
if ($added_reviewers) {
|
2011-06-01 20:19:55 +02:00
|
|
|
$key = DifferentialComment::METADATA_ADDED_REVIEWERS;
|
|
|
|
$metadata[$key] = $added_reviewers;
|
2011-01-30 21:08:40 +01:00
|
|
|
} else {
|
2012-01-24 03:34:46 +01: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 20:39:22 +01: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 21:08:40 +01: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 20:39:22 +01:00
|
|
|
|
2011-01-30 21:08:40 +01:00
|
|
|
break;
|
2011-06-24 21:21:48 +02: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 20:39:22 +01:00
|
|
|
$user_tried_to_add = count($added_ccs);
|
2011-06-24 21:21:48 +02: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 20:39:22 +01:00
|
|
|
$added_ccs = $this->filterAddedCCs($added_ccs);
|
2011-01-30 21:08:40 +01:00
|
|
|
|
2011-06-24 21:21:48 +02:00
|
|
|
if ($added_ccs) {
|
|
|
|
foreach ($added_ccs as $cc) {
|
|
|
|
DifferentialRevisionEditor::addCC(
|
|
|
|
$revision,
|
|
|
|
$cc,
|
|
|
|
$this->actorPHID);
|
|
|
|
}
|
|
|
|
|
|
|
|
$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 20:39:22 +01: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 21:21:48 +02:00
|
|
|
}
|
2012-04-17 23:59:31 +02: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-24 02:40:57 +02:00
|
|
|
case ArcanistDifferentialRevisionStatus::CLOSED:
|
2012-04-17 23:59:31 +02:00
|
|
|
throw new DifferentialActionHasNoEffectException(
|
|
|
|
"You can not commandeer this revision because it has ".
|
2012-04-24 02:40:57 +02:00
|
|
|
"already been closed.");
|
2012-04-17 23:59:31 +02:00
|
|
|
break;
|
|
|
|
}
|
|
|
|
|
|
|
|
$this->setAddedReviewers(array($revision->getAuthorPHID()));
|
2012-06-27 03:48:53 +02:00
|
|
|
$this->setRemovedReviewers(array($actor_phid));
|
2012-04-17 23:59:31 +02: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-27 03:48:53 +02:00
|
|
|
list($added_reviewers, $removed_reviewers) = $this->alterReviewers();
|
2012-04-17 23:59:31 +02:00
|
|
|
if ($added_reviewers) {
|
|
|
|
$key = DifferentialComment::METADATA_ADDED_REVIEWERS;
|
|
|
|
$metadata[$key] = $added_reviewers;
|
|
|
|
}
|
|
|
|
|
2012-06-27 03:48:53 +02:00
|
|
|
if ($removed_reviewers) {
|
|
|
|
$key = DifferentialComment::METADATA_REMOVED_REVIEWERS;
|
|
|
|
$metadata[$key] = $removed_reviewers;
|
|
|
|
}
|
|
|
|
|
2011-06-24 21:21:48 +02:00
|
|
|
break;
|
2011-01-30 21:08:40 +01:00
|
|
|
default:
|
|
|
|
throw new Exception('Unsupported action.');
|
|
|
|
}
|
|
|
|
|
2012-03-05 19:51:47 +01:00
|
|
|
// Update information about reviewer in charge.
|
|
|
|
if ($action == DifferentialAction::ACTION_ACCEPT ||
|
|
|
|
$action == DifferentialAction::ACTION_REJECT) {
|
|
|
|
$revision->setLastReviewerPHID($actor_phid);
|
|
|
|
}
|
|
|
|
|
2012-05-12 01:36:08 +02:00
|
|
|
// TODO: Call beginReadLocking() prior to loading the revision.
|
|
|
|
$revision->openTransaction();
|
|
|
|
|
2012-01-13 04:22:09 +01: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 15:25:49 +02:00
|
|
|
if ($action != DifferentialAction::ACTION_RESIGN) {
|
2011-02-20 00:06:22 +01:00
|
|
|
DifferentialRevisionEditor::addCC(
|
|
|
|
$revision,
|
|
|
|
$this->actorPHID,
|
|
|
|
$this->actorPHID);
|
|
|
|
}
|
|
|
|
|
2011-01-30 21:08:40 +01:00
|
|
|
$comment = id(new DifferentialComment())
|
|
|
|
->setAuthorPHID($this->actorPHID)
|
|
|
|
->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 19:25:45 +02:00
|
|
|
->setMetadata($metadata);
|
|
|
|
|
|
|
|
if ($this->contentSource) {
|
|
|
|
$comment->setContentSource($this->contentSource);
|
|
|
|
}
|
|
|
|
|
|
|
|
$comment->save();
|
2011-01-30 21:08:40 +01:00
|
|
|
|
2011-02-20 00:06:22 +01:00
|
|
|
$changesets = array();
|
2011-01-30 21:08:40 +01:00
|
|
|
if ($inline_comments) {
|
2011-02-20 00:06:22 +01: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 21:08:40 +01:00
|
|
|
}
|
|
|
|
foreach ($inline_comments as $inline) {
|
2011-02-03 04:38:43 +01:00
|
|
|
$inline->setCommentID($comment->getID());
|
2011-01-30 21:08:40 +01:00
|
|
|
$inline->save();
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2011-06-24 20:50:19 +02: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-12 00:58:32 +02:00
|
|
|
$mention_ccs = PhabricatorMarkupEngine::extractPHIDsFromMentions(
|
2011-06-24 20:50:19 +02: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 20:39:22 +01:00
|
|
|
$mention_ccs = $this->filterAddedCCs($mention_ccs);
|
2011-06-24 20:50:19 +02: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,
|
|
|
|
$this->actorPHID);
|
|
|
|
$metacc[] = $cc_phid;
|
|
|
|
}
|
|
|
|
$metadata[DifferentialComment::METADATA_ADDED_CCS] = $metacc;
|
|
|
|
|
|
|
|
$comment->setMetadata($metadata);
|
|
|
|
$comment->save();
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2012-05-12 01:36:08 +02:00
|
|
|
$revision->saveTransaction();
|
|
|
|
|
2011-02-09 18:58:48 +01:00
|
|
|
$phids = array($this->actorPHID);
|
|
|
|
$handles = id(new PhabricatorObjectHandleData($phids))
|
|
|
|
->loadHandles();
|
|
|
|
$actor_handle = $handles[$this->actorPHID];
|
|
|
|
|
2011-05-03 06:04:06 +02:00
|
|
|
$xherald_header = HeraldTranscript::loadXHeraldRulesHeader(
|
|
|
|
$revision->getPHID());
|
|
|
|
|
2011-01-30 21:08:40 +01:00
|
|
|
id(new DifferentialCommentMail(
|
|
|
|
$revision,
|
2011-02-09 18:58:48 +01:00
|
|
|
$actor_handle,
|
2011-01-30 21:08:40 +01:00
|
|
|
$comment,
|
2011-02-20 00:06:22 +01:00
|
|
|
$changesets,
|
|
|
|
$inline_comments))
|
2011-01-30 21:08:40 +01:00
|
|
|
->setToPHIDs(
|
|
|
|
array_merge(
|
|
|
|
$revision->getReviewers(),
|
|
|
|
array($revision->getAuthorPHID())))
|
|
|
|
->setCCPHIDs($revision->getCCPHIDs())
|
|
|
|
->setChangedByCommit($this->getChangedByCommit())
|
2011-05-03 06:04:06 +02:00
|
|
|
->setXHeraldRulesHeader($xherald_header)
|
2011-06-22 21:41:19 +02:00
|
|
|
->setParentMessageID($this->parentMessageID)
|
2011-01-30 21:08:40 +01:00
|
|
|
->send();
|
|
|
|
|
2011-04-12 00:20:51 +02: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(),
|
|
|
|
'actor_phid' => $this->actorPHID,
|
2011-01-30 21:08:40 +01:00
|
|
|
);
|
2011-04-12 00:20:51 +02:00
|
|
|
id(new PhabricatorTimelineEvent('difx', $event_data))
|
|
|
|
->recordEvent();
|
2011-01-30 21:08:40 +01:00
|
|
|
|
2011-07-10 00:44:49 +02:00
|
|
|
// TODO: Move to a daemon?
|
|
|
|
id(new PhabricatorFeedStoryPublisher())
|
|
|
|
->setStoryType(PhabricatorFeedStoryTypeConstants::STORY_DIFFERENTIAL)
|
|
|
|
->setStoryData($event_data)
|
|
|
|
->setStoryTime(time())
|
|
|
|
->setStoryAuthorPHID($this->actorPHID)
|
|
|
|
->setRelatedPHIDs(
|
|
|
|
array(
|
|
|
|
$revision->getPHID(),
|
|
|
|
$this->actorPHID,
|
|
|
|
$revision->getAuthorPHID(),
|
|
|
|
))
|
2012-06-09 03:45:26 +02:00
|
|
|
->setPrimaryObjectPHID($revision->getPHID())
|
|
|
|
->setSubscribedPHIDs(
|
|
|
|
array_merge(
|
|
|
|
array($revision->getAuthorPHID()),
|
|
|
|
$revision->getReviewers(),
|
|
|
|
$revision->getCCPHIDs()))
|
2011-07-10 00:44:49 +02:00
|
|
|
->publish();
|
|
|
|
|
2011-06-15 16:43:43 +02:00
|
|
|
// TODO: Move to a daemon?
|
|
|
|
PhabricatorSearchDifferentialIndexer::indexRevision($revision);
|
|
|
|
|
2011-01-30 21:08:40 +01: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 20:39:22 +01: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-27 03:48:53 +02:00
|
|
|
private function alterReviewers() {
|
2012-01-24 03:34:46 +01:00
|
|
|
$revision = $this->revision;
|
|
|
|
$added_reviewers = $this->getAddedReviewers();
|
2012-06-27 03:48:53 +02:00
|
|
|
$removed_reviewers = $this->getRemovedReviewers();
|
2012-01-24 03:34:46 +01:00
|
|
|
$reviewer_phids = $revision->getReviewers();
|
|
|
|
|
2012-06-27 03:48:53 +02:00
|
|
|
$reviewer_phids_map = array_fill_keys($reviewer_phids, true);
|
2012-01-24 03:34:46 +01:00
|
|
|
foreach ($added_reviewers as $k => $user_phid) {
|
|
|
|
if ($user_phid == $revision->getAuthorPHID()) {
|
|
|
|
unset($added_reviewers[$k]);
|
|
|
|
}
|
2012-06-27 03:48:53 +02:00
|
|
|
if (isset($reviewer_phids_map[$user_phid])) {
|
2012-01-24 03:34:46 +01:00
|
|
|
unset($added_reviewers[$k]);
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2012-06-27 03:48:53 +02:00
|
|
|
foreach ($removed_reviewers as $k => $user_phid) {
|
|
|
|
if (!isset($reviewer_phids_map[$user_phid])) {
|
|
|
|
unset($removed_reviewers[$k]);
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2012-01-24 03:34:46 +01:00
|
|
|
$added_reviewers = array_unique($added_reviewers);
|
2012-06-27 03:48:53 +02:00
|
|
|
$removed_reviewers = array_unique($removed_reviewers);
|
2012-01-24 03:34:46 +01:00
|
|
|
|
|
|
|
if ($added_reviewers) {
|
|
|
|
DifferentialRevisionEditor::alterReviewers(
|
|
|
|
$revision,
|
|
|
|
$reviewer_phids,
|
2012-06-27 03:48:53 +02:00
|
|
|
$removed_reviewers,
|
2012-01-24 03:34:46 +01:00
|
|
|
$added_reviewers,
|
|
|
|
$this->actorPHID);
|
|
|
|
}
|
|
|
|
|
2012-06-27 03:48:53 +02:00
|
|
|
return array($added_reviewers, $removed_reviewers);
|
2012-01-24 03:34:46 +01:00
|
|
|
}
|
|
|
|
|
2011-01-30 21:08:40 +01:00
|
|
|
}
|