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

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(

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);
         }
       }

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);

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;
   }

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);
       }

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;
+  }
+
 }

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',

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'));
       }
     }