mirror of
https://we.phorge.it/source/phorge.git
synced 2025-01-10 23:01:04 +01:00
Raise a warning when mentioning a user in a comment on a draft revision
Summary: See PHI433. Ref T13102. Users in the wild have mixed expecations about exactly what "draft" means. Recent changes have tried to make behavior more clear. As part of clarifying messaging, make it explicit that `@mention` does not work on drafts by showing users a warning when they try to `@mention` a user. Test Plan: - Mentioned users on drafts, got a warning. - Posted normal comments on drafts, no warning. - Posted normal/mention comments on non-drafts, no warning. Maniphest Tasks: T13102 Differential Revision: https://secure.phabricator.com/D19210
This commit is contained in:
parent
3c4f31e4b9
commit
df8d4dff67
5 changed files with 127 additions and 0 deletions
|
@ -2055,6 +2055,8 @@ phutil_register_library_map(array(
|
|||
'PhabricatorApplicationTransactionValidationResponse' => 'applications/transactions/response/PhabricatorApplicationTransactionValidationResponse.php',
|
||||
'PhabricatorApplicationTransactionValueController' => 'applications/transactions/controller/PhabricatorApplicationTransactionValueController.php',
|
||||
'PhabricatorApplicationTransactionView' => 'applications/transactions/view/PhabricatorApplicationTransactionView.php',
|
||||
'PhabricatorApplicationTransactionWarningException' => 'applications/transactions/exception/PhabricatorApplicationTransactionWarningException.php',
|
||||
'PhabricatorApplicationTransactionWarningResponse' => 'applications/transactions/response/PhabricatorApplicationTransactionWarningResponse.php',
|
||||
'PhabricatorApplicationUninstallController' => 'applications/meta/controller/PhabricatorApplicationUninstallController.php',
|
||||
'PhabricatorApplicationsApplication' => 'applications/meta/application/PhabricatorApplicationsApplication.php',
|
||||
'PhabricatorApplicationsController' => 'applications/meta/controller/PhabricatorApplicationsController.php',
|
||||
|
@ -7484,6 +7486,8 @@ phutil_register_library_map(array(
|
|||
'PhabricatorApplicationTransactionValidationResponse' => 'AphrontProxyResponse',
|
||||
'PhabricatorApplicationTransactionValueController' => 'PhabricatorApplicationTransactionController',
|
||||
'PhabricatorApplicationTransactionView' => 'AphrontView',
|
||||
'PhabricatorApplicationTransactionWarningException' => 'Exception',
|
||||
'PhabricatorApplicationTransactionWarningResponse' => 'AphrontProxyResponse',
|
||||
'PhabricatorApplicationUninstallController' => 'PhabricatorApplicationsController',
|
||||
'PhabricatorApplicationsApplication' => 'PhabricatorApplication',
|
||||
'PhabricatorApplicationsController' => 'PhabricatorController',
|
||||
|
|
|
@ -1968,6 +1968,7 @@ abstract class PhabricatorEditEngine
|
|||
->setContinueOnNoEffect($request->isContinueRequest())
|
||||
->setContinueOnMissingFields(true)
|
||||
->setContentSourceFromRequest($request)
|
||||
->setRaiseWarnings(!$request->getBool('editEngine.warnings'))
|
||||
->setIsPreview($is_preview);
|
||||
|
||||
try {
|
||||
|
@ -1980,6 +1981,10 @@ abstract class PhabricatorEditEngine
|
|||
return id(new PhabricatorApplicationTransactionNoEffectResponse())
|
||||
->setCancelURI($view_uri)
|
||||
->setException($ex);
|
||||
} catch (PhabricatorApplicationTransactionWarningException $ex) {
|
||||
return id(new PhabricatorApplicationTransactionWarningResponse())
|
||||
->setCancelURI($view_uri)
|
||||
->setException($ex);
|
||||
}
|
||||
|
||||
if (!$is_preview) {
|
||||
|
|
|
@ -48,6 +48,7 @@ abstract class PhabricatorApplicationTransactionEditor
|
|||
private $mentionedPHIDs;
|
||||
private $continueOnNoEffect;
|
||||
private $continueOnMissingFields;
|
||||
private $raiseWarnings;
|
||||
private $parentMessageID;
|
||||
private $heraldAdapter;
|
||||
private $heraldTranscript;
|
||||
|
@ -273,6 +274,15 @@ abstract class PhabricatorApplicationTransactionEditor
|
|||
return $this->applicationEmail;
|
||||
}
|
||||
|
||||
public function setRaiseWarnings($raise_warnings) {
|
||||
$this->raiseWarnings = $raise_warnings;
|
||||
return $this;
|
||||
}
|
||||
|
||||
public function getRaiseWarnings() {
|
||||
return $this->raiseWarnings;
|
||||
}
|
||||
|
||||
public function getTransactionTypesForObject($object) {
|
||||
$old = $this->object;
|
||||
try {
|
||||
|
@ -919,6 +929,19 @@ abstract class PhabricatorApplicationTransactionEditor
|
|||
throw new PhabricatorApplicationTransactionValidationException($errors);
|
||||
}
|
||||
|
||||
if ($this->raiseWarnings) {
|
||||
$warnings = array();
|
||||
foreach ($xactions as $xaction) {
|
||||
if ($this->hasWarnings($object, $xaction)) {
|
||||
$warnings[] = $xaction;
|
||||
}
|
||||
}
|
||||
if ($warnings) {
|
||||
throw new PhabricatorApplicationTransactionWarningException(
|
||||
$warnings);
|
||||
}
|
||||
}
|
||||
|
||||
$this->willApplyTransactions($object, $xactions);
|
||||
|
||||
if ($object->getID()) {
|
||||
|
@ -4277,4 +4300,28 @@ abstract class PhabricatorApplicationTransactionEditor
|
|||
}
|
||||
}
|
||||
|
||||
private function hasWarnings($object, $xaction) {
|
||||
// TODO: For the moment, this is a very un-modular hack to support
|
||||
// exactly one type of warning (mentioning users on a draft revision)
|
||||
// that we want to show. See PHI433.
|
||||
|
||||
if (!($object instanceof DifferentialRevision)) {
|
||||
return false;
|
||||
}
|
||||
|
||||
if (!$object->isDraft()) {
|
||||
return false;
|
||||
}
|
||||
|
||||
$type = $xaction->getTransactionType();
|
||||
if ($type != PhabricatorTransactions::TYPE_SUBSCRIBERS) {
|
||||
return false;
|
||||
}
|
||||
|
||||
// NOTE: This will currently warn even if you're only removing
|
||||
// subscribers.
|
||||
|
||||
return true;
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
@ -0,0 +1,13 @@
|
|||
<?php
|
||||
|
||||
final class PhabricatorApplicationTransactionWarningException
|
||||
extends Exception {
|
||||
|
||||
private $xactions;
|
||||
|
||||
public function __construct(array $xactions) {
|
||||
$this->xactions = $xactions;
|
||||
parent::__construct();
|
||||
}
|
||||
|
||||
}
|
|
@ -0,0 +1,58 @@
|
|||
<?php
|
||||
|
||||
final class PhabricatorApplicationTransactionWarningResponse
|
||||
extends AphrontProxyResponse {
|
||||
|
||||
private $viewer;
|
||||
private $exception;
|
||||
private $cancelURI;
|
||||
|
||||
public function setCancelURI($cancel_uri) {
|
||||
$this->cancelURI = $cancel_uri;
|
||||
return $this;
|
||||
}
|
||||
|
||||
public function setException(
|
||||
PhabricatorApplicationTransactionWarningException $exception) {
|
||||
$this->exception = $exception;
|
||||
return $this;
|
||||
}
|
||||
|
||||
protected function buildProxy() {
|
||||
return new AphrontDialogResponse();
|
||||
}
|
||||
|
||||
public function reduceProxyResponse() {
|
||||
$request = $this->getRequest();
|
||||
|
||||
$title = pht('Warning: Unexpected Effects');
|
||||
|
||||
$head = pht(
|
||||
'This is a draft revision that will not publish any notifications '.
|
||||
'until the author requests review.');
|
||||
$tail = pht(
|
||||
'Mentioned or subscribed users will not be notified.');
|
||||
|
||||
$continue = pht('Tell No One');
|
||||
|
||||
$dialog = id(new AphrontDialogView())
|
||||
->setViewer($request->getViewer())
|
||||
->setTitle($title);
|
||||
|
||||
$dialog->appendParagraph($head);
|
||||
$dialog->appendParagraph($tail);
|
||||
|
||||
$passthrough = $request->getPassthroughRequestParameters();
|
||||
foreach ($passthrough as $key => $value) {
|
||||
$dialog->addHiddenInput($key, $value);
|
||||
}
|
||||
|
||||
$dialog
|
||||
->addHiddenInput('editEngine.warnings', 1)
|
||||
->addSubmitButton($continue)
|
||||
->addCancelButton($this->cancelURI);
|
||||
|
||||
return $this->getProxy()->setDialog($dialog);
|
||||
}
|
||||
|
||||
}
|
Loading…
Reference in a new issue