diff --git a/src/applications/maniphest/conduit/ConduitAPI_maniphest_Method.php b/src/applications/maniphest/conduit/ConduitAPI_maniphest_Method.php index aa6a652bfc..c2074ac7e8 100644 --- a/src/applications/maniphest/conduit/ConduitAPI_maniphest_Method.php +++ b/src/applications/maniphest/conduit/ConduitAPI_maniphest_Method.php @@ -204,6 +204,10 @@ abstract class ConduitAPI_maniphest_Method extends ConduitAPIMethod { ->setContentSource($content_source) ->setContinueOnNoEffect(true); + if (!$is_new) { + $editor->setContinueOnMissingFields(true); + } + $editor->applyTransactions($task, $transactions); $event = new PhabricatorEvent( diff --git a/src/applications/maniphest/controller/ManiphestBatchEditController.php b/src/applications/maniphest/controller/ManiphestBatchEditController.php index 5b1c71ad9c..ffc917989d 100644 --- a/src/applications/maniphest/controller/ManiphestBatchEditController.php +++ b/src/applications/maniphest/controller/ManiphestBatchEditController.php @@ -35,6 +35,7 @@ final class ManiphestBatchEditController extends ManiphestController { ->setActor($user) ->setContentSourceFromRequest($request) ->setContinueOnNoEffect(true) + ->setContinueOnMissingFields(true) ->applyTransactions($task, $xactions); } } diff --git a/src/applications/maniphest/controller/ManiphestSubpriorityController.php b/src/applications/maniphest/controller/ManiphestSubpriorityController.php index d1e707924f..199e98433b 100644 --- a/src/applications/maniphest/controller/ManiphestSubpriorityController.php +++ b/src/applications/maniphest/controller/ManiphestSubpriorityController.php @@ -44,7 +44,8 @@ final class ManiphestSubpriorityController extends ManiphestController { $editor = id(new ManiphestTransactionEditorPro()) ->setActor($user) - ->setContinueOnNoEffect($request->isContinueRequest()) + ->setContinueOnMissingFields(true) + ->setContinueOnNoEffect(true) ->setContentSourceFromRequest($request); $editor->applyTransactions($task, $xactions); diff --git a/src/applications/search/controller/PhabricatorSearchAttachController.php b/src/applications/search/controller/PhabricatorSearchAttachController.php index 5c9483dad8..769ad90045 100644 --- a/src/applications/search/controller/PhabricatorSearchAttachController.php +++ b/src/applications/search/controller/PhabricatorSearchAttachController.php @@ -157,8 +157,11 @@ final class PhabricatorSearchAttachController return $response; } - $editor = new ManiphestTransactionEditor(); - $editor->setActor($user); + $editor = id(new ManiphestTransactionEditorPro()) + ->setActor($user) + ->setContentSourceFromRequest($this->getRequest()) + ->setContinueOnNoEffect(true) + ->setContinueOnMissingFields(true); $task_names = array(); @@ -172,13 +175,22 @@ final class PhabricatorSearchAttachController $target->getAuthorPHID(), $target->getOwnerPHID()); - $close_task = id(new ManiphestTransaction()) - ->setAuthorPHID($user->getPHID()) + $close_task = id(new ManiphestTransactionPro()) ->setTransactionType(ManiphestTransactionType::TYPE_STATUS) - ->setNewValue(ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE) - ->setComments("\xE2\x9C\x98 Merged into {$merge_into_name}."); + ->setNewValue(ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE); - $editor->applyTransactions($target, array($close_task)); + $merge_comment = id(new ManiphestTransactionPro()) + ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT) + ->attachComment( + id(new ManiphestTransactionComment()) + ->setContent("\xE2\x9C\x98 Merged into {$merge_into_name}.")); + + $editor->applyTransactions( + $target, + array( + $close_task, + $merge_comment, + )); $task_names[] = 'T'.$target->getID(); } @@ -188,12 +200,17 @@ final class PhabricatorSearchAttachController $task_names = implode(', ', $task_names); - $add_ccs = id(new ManiphestTransaction()) - ->setAuthorPHID($user->getPHID()) + $add_ccs = id(new ManiphestTransactionPro()) ->setTransactionType(ManiphestTransactionType::TYPE_CCS) - ->setNewValue($all_ccs) - ->setComments("\xE2\x97\x80 Merged tasks: {$task_names}."); - $editor->applyTransactions($task, array($add_ccs)); + ->setNewValue($all_ccs); + + $merged_comment = id(new ManiphestTransactionPro()) + ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT) + ->attachComment( + id(new ManiphestTransactionComment()) + ->setContent("\xE2\x97\x80 Merged tasks: {$task_names}.")); + + $editor->applyTransactions($task, array($add_ccs, $merged_comment)); return $response; } diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 38fb82a17a..254969cb2b 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -15,6 +15,7 @@ abstract class PhabricatorApplicationTransactionEditor private $isNewObject; private $mentionedPHIDs; private $continueOnNoEffect; + private $continueOnMissingFields; private $parentMessageID; private $heraldAdapter; private $heraldTranscript; @@ -44,6 +45,36 @@ abstract class PhabricatorApplicationTransactionEditor return $this->continueOnNoEffect; } + + /** + * When the editor tries to apply transactions which don't populate all of + * an object's required fields, should it raise an exception (default) or + * drop them and continue? + * + * For example, if a user adds a new required custom field (like "Severity") + * to a task, all existing tasks won't have it populated. When users + * manually edit existing tasks, it's usually desirable to have them provide + * a severity. However, other operations (like batch editing just the + * owner of a task) will fail by default. + * + * By setting this flag for edit operations which apply to specific fields + * (like the priority, batch, and merge editors in Maniphest), these + * operations can continue to function even if an object is outdated. + * + * @param bool True to continue when transactions don't completely satisfy + * all required fields. + * @return this + */ + public function setContinueOnMissingFields($continue_on_missing_fields) { + $this->continueOnMissingFields = $continue_on_missing_fields; + return $this; + } + + public function getContinueOnMissingFields() { + return $this->continueOnMissingFields; + } + + /** * Not strictly necessary, but reply handlers ideally set this value to * make email threading work better. @@ -363,6 +394,14 @@ abstract class PhabricatorApplicationTransactionEditor } $errors = array_mergev($errors); + + $continue_on_missing = $this->getContinueOnMissingFields(); + foreach ($errors as $key => $error) { + if ($continue_on_missing && $error->getIsMissingFieldError()) { + unset($errors[$key]); + } + } + if ($errors) { throw new PhabricatorApplicationTransactionValidationException($errors); } diff --git a/src/applications/transactions/error/PhabricatorApplicationTransactionValidationError.php b/src/applications/transactions/error/PhabricatorApplicationTransactionValidationError.php index 65661abc06..abad50b7bd 100644 --- a/src/applications/transactions/error/PhabricatorApplicationTransactionValidationError.php +++ b/src/applications/transactions/error/PhabricatorApplicationTransactionValidationError.php @@ -7,6 +7,7 @@ final class PhabricatorApplicationTransactionValidationError private $transaction; private $shortMessage; private $message; + private $isMissingFieldError; public function __construct( $type, @@ -36,4 +37,13 @@ final class PhabricatorApplicationTransactionValidationError return $this->message; } + public function setIsMissingFieldError($is_missing_field_error) { + $this->isMissingFieldError = $is_missing_field_error; + return $this; + } + + public function getIsMissingFieldError() { + return $this->isMissingFieldError; + } + } diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php index 8579d6a774..e64b5b86ff 100644 --- a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php +++ b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php @@ -281,7 +281,7 @@ class PhabricatorApplicationTransactionView extends AphrontView { $engine = $this->getOrBuildEngine(); $comment = $xaction->getComment(); - if ($comment) { + if ($xaction->hasComment()) { if ($comment->getIsDeleted()) { return phutil_tag( 'em', diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php index ec42904f2b..085805e466 100644 --- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php @@ -282,11 +282,13 @@ abstract class PhabricatorStandardCustomField } } if ($this->isValueEmpty($value)) { - $errors[] = new PhabricatorApplicationTransactionValidationError( + $error = new PhabricatorApplicationTransactionValidationError( $type, pht('Required'), pht('%s is required.', $this->getFieldName()), $transaction); + $error->setIsMissingFieldError(true); + $errors[] = $error; $this->setFieldError(pht('Required')); } }