From 9afbb9b83b5f8803f3055caebcd6bdfc1af34209 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 23 Sep 2013 14:32:32 -0700 Subject: [PATCH] Route task merges through new editor Summary: Ref T2217. Ship "Merge in Duplicates" through the new editor. The only notable thing here is `setContinueOnMissingFields()`. The problem this solves is that if you add a custom field and mark it as required, all existing tasks are "invalid" since they don't have a value, and trying to edit them will raise an error like "Some Custom Field is required!". This is fine for normal edits via the UI, since the user can just select/provide a value, but surgical edits to specific fields should just ignore these errors. Add the ability to ignore these errors and use it on all the field-speific editors. Test Plan: Merged duplicates, including "invalid" duplicates with missing fields. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T2217 Differential Revision: https://secure.phabricator.com/D7084 --- .../conduit/ConduitAPI_maniphest_Method.php | 4 ++ .../ManiphestBatchEditController.php | 1 + .../ManiphestSubpriorityController.php | 3 +- .../PhabricatorSearchAttachController.php | 41 +++++++++++++------ ...habricatorApplicationTransactionEditor.php | 39 ++++++++++++++++++ ...rApplicationTransactionValidationError.php | 10 +++++ .../PhabricatorApplicationTransactionView.php | 2 +- .../PhabricatorStandardCustomField.php | 4 +- 8 files changed, 89 insertions(+), 15 deletions(-) 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')); } }