From a1516fefb67a255e92d4b669d577e6d8ddc69368 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 16 Jan 2019 06:36:10 -0800
Subject: [PATCH] Fix an issue where "Import Columns" could fail on a board for
 a project with milestones

Summary:
See PHI1025. When you "Import Columns", we test if you're trying to import into a board that already has columns. However, this test is too broad (it incorrectly detects "proxy" columns for milestones as columns) and not user-friendly (it returns 400 instead of a readable error).

Correct these issues, and refine some of the logic around proxy columns.

Test Plan:
  - Created a project, A.
  - Created a milestone under that project.
  - Imported another project's columns to A's workboard.
    - Before change: Unhelpful 400.
    - After change: import worked fine.
  - Also, hit the new error dialogs and read through them.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19978
---
 ...habricatorProjectBoardImportController.php | 31 ++++++++++++++-----
 .../query/PhabricatorProjectColumnQuery.php   | 14 +++++++++
 .../PhabricatorProjectDatasource.php          |  1 +
 3 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/src/applications/project/controller/PhabricatorProjectBoardImportController.php b/src/applications/project/controller/PhabricatorProjectBoardImportController.php
index c344bc0af0..67bddaaa52 100644
--- a/src/applications/project/controller/PhabricatorProjectBoardImportController.php
+++ b/src/applications/project/controller/PhabricatorProjectBoardImportController.php
@@ -21,17 +21,28 @@ final class PhabricatorProjectBoardImportController
     }
     $this->setProject($project);
 
+    $project_id = $project->getID();
+    $board_uri = $this->getApplicationURI("board/{$project_id}/");
+
+    // See PHI1025. We only want to prevent the import if the board already has
+    // real columns. If it has proxy columns (for example, for milestones) you
+    // can still import columns from another board.
     $columns = id(new PhabricatorProjectColumnQuery())
       ->setViewer($viewer)
       ->withProjectPHIDs(array($project->getPHID()))
+      ->withIsProxyColumn(false)
       ->execute();
     if ($columns) {
-      return new Aphront400Response();
+      return $this->newDialog()
+        ->setTitle(pht('Workboard Already Has Columns'))
+        ->appendParagraph(
+          pht(
+            'You can not import columns into this workboard because it '.
+            'already has columns. You can only import into an empty '.
+            'workboard.'))
+        ->addCancelButton($board_uri);
     }
 
-    $project_id = $project->getID();
-    $board_uri = $this->getApplicationURI("board/{$project_id}/");
-
     if ($request->isFormPost()) {
       $import_phid = $request->getArr('importProjectPHID');
       $import_phid = reset($import_phid);
@@ -39,9 +50,16 @@ final class PhabricatorProjectBoardImportController
       $import_columns = id(new PhabricatorProjectColumnQuery())
         ->setViewer($viewer)
         ->withProjectPHIDs(array($import_phid))
+        ->withIsProxyColumn(false)
         ->execute();
       if (!$import_columns) {
-        return new Aphront400Response();
+        return $this->newDialog()
+          ->setTitle(pht('Source Workboard Has No Columns'))
+          ->appendParagraph(
+            pht(
+              'You can not import columns from that workboard because it has '.
+              'no importable columns.'))
+          ->addCancelButton($board_uri);
       }
 
       $table = id(new PhabricatorProjectColumn())
@@ -50,9 +68,6 @@ final class PhabricatorProjectBoardImportController
         if ($import_column->isHidden()) {
           continue;
         }
-        if ($import_column->getProxy()) {
-          continue;
-        }
 
         $new_column = PhabricatorProjectColumn::initializeNewColumn($viewer)
           ->setSequence($import_column->getSequence())
diff --git a/src/applications/project/query/PhabricatorProjectColumnQuery.php b/src/applications/project/query/PhabricatorProjectColumnQuery.php
index 13f2f52a43..441c33e8cb 100644
--- a/src/applications/project/query/PhabricatorProjectColumnQuery.php
+++ b/src/applications/project/query/PhabricatorProjectColumnQuery.php
@@ -8,6 +8,7 @@ final class PhabricatorProjectColumnQuery
   private $projectPHIDs;
   private $proxyPHIDs;
   private $statuses;
+  private $isProxyColumn;
 
   public function withIDs(array $ids) {
     $this->ids = $ids;
@@ -34,6 +35,11 @@ final class PhabricatorProjectColumnQuery
     return $this;
   }
 
+  public function withIsProxyColumn($is_proxy) {
+    $this->isProxyColumn = $is_proxy;
+    return $this;
+  }
+
   public function newResultObject() {
     return new PhabricatorProjectColumn();
   }
@@ -156,6 +162,14 @@ final class PhabricatorProjectColumnQuery
         $this->statuses);
     }
 
+    if ($this->isProxyColumn !== null) {
+      if ($this->isProxyColumn) {
+        $where[] = qsprintf($conn, 'proxyPHID IS NOT NULL');
+      } else {
+        $where[] = qsprintf($conn, 'proxyPHID IS NULL');
+      }
+    }
+
     return $where;
   }
 
diff --git a/src/applications/project/typeahead/PhabricatorProjectDatasource.php b/src/applications/project/typeahead/PhabricatorProjectDatasource.php
index e5b24335cf..5b999a997f 100644
--- a/src/applications/project/typeahead/PhabricatorProjectDatasource.php
+++ b/src/applications/project/typeahead/PhabricatorProjectDatasource.php
@@ -53,6 +53,7 @@ final class PhabricatorProjectDatasource
       $columns = id(new PhabricatorProjectColumnQuery())
         ->setViewer($viewer)
         ->withProjectPHIDs(array_keys($projs))
+        ->withIsProxyColumn(false)
         ->execute();
       $has_cols = mgroup($columns, 'getProjectPHID');
     } else {