Skip to content

Commit a1516fe

Browse files
author
epriestley
committed
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
1 parent 0a0afa4 commit a1516fe

File tree

3 files changed

+38
-8
lines changed

3 files changed

+38
-8
lines changed

src/applications/project/controller/PhabricatorProjectBoardImportController.php

+23-8
Original file line numberDiff line numberDiff line change
@@ -21,27 +21,45 @@ public function handleRequest(AphrontRequest $request) {
2121
}
2222
$this->setProject($project);
2323

24+
$project_id = $project->getID();
25+
$board_uri = $this->getApplicationURI("board/{$project_id}/");
26+
27+
// See PHI1025. We only want to prevent the import if the board already has
28+
// real columns. If it has proxy columns (for example, for milestones) you
29+
// can still import columns from another board.
2430
$columns = id(new PhabricatorProjectColumnQuery())
2531
->setViewer($viewer)
2632
->withProjectPHIDs(array($project->getPHID()))
33+
->withIsProxyColumn(false)
2734
->execute();
2835
if ($columns) {
29-
return new Aphront400Response();
36+
return $this->newDialog()
37+
->setTitle(pht('Workboard Already Has Columns'))
38+
->appendParagraph(
39+
pht(
40+
'You can not import columns into this workboard because it '.
41+
'already has columns. You can only import into an empty '.
42+
'workboard.'))
43+
->addCancelButton($board_uri);
3044
}
3145

32-
$project_id = $project->getID();
33-
$board_uri = $this->getApplicationURI("board/{$project_id}/");
34-
3546
if ($request->isFormPost()) {
3647
$import_phid = $request->getArr('importProjectPHID');
3748
$import_phid = reset($import_phid);
3849

3950
$import_columns = id(new PhabricatorProjectColumnQuery())
4051
->setViewer($viewer)
4152
->withProjectPHIDs(array($import_phid))
53+
->withIsProxyColumn(false)
4254
->execute();
4355
if (!$import_columns) {
44-
return new Aphront400Response();
56+
return $this->newDialog()
57+
->setTitle(pht('Source Workboard Has No Columns'))
58+
->appendParagraph(
59+
pht(
60+
'You can not import columns from that workboard because it has '.
61+
'no importable columns.'))
62+
->addCancelButton($board_uri);
4563
}
4664

4765
$table = id(new PhabricatorProjectColumn())
@@ -50,9 +68,6 @@ public function handleRequest(AphrontRequest $request) {
5068
if ($import_column->isHidden()) {
5169
continue;
5270
}
53-
if ($import_column->getProxy()) {
54-
continue;
55-
}
5671

5772
$new_column = PhabricatorProjectColumn::initializeNewColumn($viewer)
5873
->setSequence($import_column->getSequence())

src/applications/project/query/PhabricatorProjectColumnQuery.php

+14
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ final class PhabricatorProjectColumnQuery
88
private $projectPHIDs;
99
private $proxyPHIDs;
1010
private $statuses;
11+
private $isProxyColumn;
1112

1213
public function withIDs(array $ids) {
1314
$this->ids = $ids;
@@ -34,6 +35,11 @@ public function withStatuses(array $status) {
3435
return $this;
3536
}
3637

38+
public function withIsProxyColumn($is_proxy) {
39+
$this->isProxyColumn = $is_proxy;
40+
return $this;
41+
}
42+
3743
public function newResultObject() {
3844
return new PhabricatorProjectColumn();
3945
}
@@ -156,6 +162,14 @@ protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
156162
$this->statuses);
157163
}
158164

165+
if ($this->isProxyColumn !== null) {
166+
if ($this->isProxyColumn) {
167+
$where[] = qsprintf($conn, 'proxyPHID IS NOT NULL');
168+
} else {
169+
$where[] = qsprintf($conn, 'proxyPHID IS NULL');
170+
}
171+
}
172+
159173
return $where;
160174
}
161175

src/applications/project/typeahead/PhabricatorProjectDatasource.php

+1
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ public function loadResults() {
5353
$columns = id(new PhabricatorProjectColumnQuery())
5454
->setViewer($viewer)
5555
->withProjectPHIDs(array_keys($projs))
56+
->withIsProxyColumn(false)
5657
->execute();
5758
$has_cols = mgroup($columns, 'getProjectPHID');
5859
} else {

0 commit comments

Comments
 (0)