Christopher Johnson (WMDE) has uploaded a new change for review.
https://gerrit.wikimedia.org/r/172465
Change subject: Add sorting to Tasks Table
......................................................................
Add sorting to Tasks Table
Change-Id: I7148ba1dd395696ea9f60bf861fa49d022473ba7
---
M .arcconfig
A scripts/__init_script__.php
M src/__phutil_library_map__.php
A src/__tests__/SprintQueryTestCase.php
A src/__tests__/SprintTestCase.php
M src/controller/BurndownDataViewController.php
M src/query/SprintQuery.php
M src/view/BurndownDataView.php
8 files changed, 384 insertions(+), 33 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/phabricator/extensions/Sprint
refs/changes/65/172465/1
diff --git a/.arcconfig b/.arcconfig
index c54867b..2af3035 100644
--- a/.arcconfig
+++ b/.arcconfig
@@ -1,5 +1,6 @@
{
"project.name" : "sprint",
+ "unit.engine" : "PhutilUnitTestEngine",
"load" : [
"src/"
]
diff --git a/scripts/__init_script__.php b/scripts/__init_script__.php
new file mode 100644
index 0000000..933b7d8
--- /dev/null
+++ b/scripts/__init_script__.php
@@ -0,0 +1,25 @@
+<?php
+
+function init_phabricator_script() {
+ error_reporting(E_ALL | E_STRICT);
+ ini_set('display_errors', 1);
+
+ $include_path = ini_get('include_path');
+ ini_set(
+ 'include_path',
+ $include_path.PATH_SEPARATOR.dirname(__FILE__).'/../../');
+ @include_once 'libphutil/scripts/__init_script__.php';
+ if (!@constant('__LIBPHUTIL__')) {
+ echo "ERROR: Unable to load libphutil. Update your PHP 'include_path' to ".
+ "include the parent directory of libphutil/.\n";
+ exit(1);
+ }
+
+ phutil_load_library('/srv/phab/libext/Sprint/src');
+ phutil_load_library('arcanist/src');
+ phutil_load_library('phabricator/src/');
+
+ PhabricatorEnv::initializeScriptEnvironment();
+}
+
+init_phabricator_script();
\ No newline at end of file
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index b04706d..b79e59d 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -27,10 +27,12 @@
'SprintEndDateField' => 'customfield/SprintEndDateField.php',
'SprintProjectCustomField' => 'customfield/SprintProjectCustomField.php',
'SprintQuery' => 'query/SprintQuery.php',
+ 'SprintQueryTestCase' => '__tests__/SprintQueryTestCase.php',
'SprintReportBurndownView' => 'view/SprintReportBurndownView.php',
'SprintReportController' => 'controller/SprintReportController.php',
'SprintReportOpenTasksView' => 'view/SprintReportOpenTasksView.php',
'SprintTaskStoryPointsField' =>
'customfield/SprintTaskStoryPointsField.php',
+ 'SprintTestCase' => '__tests__/SprintTestCase.php',
'SprintTransaction' => 'storage/SprintTransaction.php',
'SprintView' => 'view/SprintView.php',
'UserOpenTasksView' => 'view/UserOpenTasksView.php',
@@ -55,6 +57,7 @@
'PhabricatorProjectCustomField',
'PhabricatorStandardCustomFieldInterface',
),
+ 'SprintQueryTestCase' => 'SprintTestCase',
'SprintReportBurndownView' => 'SprintView',
'SprintReportController' => 'BurndownController',
'SprintReportOpenTasksView' => 'SprintView',
@@ -62,6 +65,7 @@
'ManiphestCustomField',
'PhabricatorStandardCustomFieldInterface',
),
+ 'SprintTestCase' => 'ArcanistPhutilTestCase',
'SprintView' => 'AphrontView',
'UserOpenTasksView' => 'OpenTasksView',
),
diff --git a/src/__tests__/SprintQueryTestCase.php
b/src/__tests__/SprintQueryTestCase.php
new file mode 100644
index 0000000..5286443
--- /dev/null
+++ b/src/__tests__/SprintQueryTestCase.php
@@ -0,0 +1,40 @@
+<?php
+final class SprintQueryTestCase extends SprintTestCase {
+
+ public function getRequestObject()
+ {
+ $r = new AphrontRequest('example.com', '/');
+ return $r;
+ }
+
+ public function testRequestSetUser()
+ {
+ $r = new AphrontRequest('example.com', '/');
+ $user = $this->generateNewTestUser();
+ $r->setUser($user);
+ $this->assertEqual($user, $r->getUser());
+ return $r;
+ }
+
+
+ public function PHIDProvider()
+ {
+ return array(
+ 'PHID-PROJ-mp777tqnkvivubj26ufu',
+ );
+ }
+
+ /**
+ * @depends testRequestSetUser
+ */
+ public function testGetViewerHandles()
+ {
+ $r = new AphrontRequest('example.com', '/');
+ $r->setUser($this->generateNewTestUser());
+ $q = new SprintQuery();
+ $phids = $this->PHIDProvider();
+ $handle = $q->getViewerHandles($r, $phids);
+ $this->assertInstanceof('PhabricatorObjectHandle', $handle);
+ }
+
+}
\ No newline at end of file
diff --git a/src/__tests__/SprintTestCase.php b/src/__tests__/SprintTestCase.php
new file mode 100644
index 0000000..eba4084
--- /dev/null
+++ b/src/__tests__/SprintTestCase.php
@@ -0,0 +1,222 @@
+<?php
+
+abstract class SprintTestCase extends ArcanistPhutilTestCase {
+
+ const NAMESPACE_PREFIX = 'phabricator_unittest_';
+
+ /**
+ * If true, put Lisk in process-isolated mode for the duration of the tests
so
+ * that it will establish only isolated, side-effect-free database
+ * connections. Defaults to true.
+ *
+ * NOTE: You should disable this only in rare circumstances. Unit tests
should
+ * not rely on external resources like databases, and should not produce
+ * side effects.
+ */
+ const PHABRICATOR_TESTCONFIG_ISOLATE_LISK = 'isolate-lisk';
+
+ /**
+ * If true, build storage fixtures before running tests, and connect to them
+ * during test execution. This will impose a performance penalty on test
+ * execution (currently, it takes roughly one second to build the fixture)
+ * but allows you to perform tests which require data to be read from storage
+ * after writes. The fixture is shared across all test cases in this process.
+ * Defaults to false.
+ *
+ * NOTE: All connections to fixture storage open transactions when
established
+ * and roll them back when tests complete. Each test must independently
+ * write data it relies on; data will not persist across tests.
+ *
+ * NOTE: Enabling this implies disabling process isolation.
+ */
+ const PHABRICATOR_TESTCONFIG_BUILD_STORAGE_FIXTURES = 'storage-fixtures';
+
+ private $configuration;
+ private $env;
+
+ private static $storageFixtureReferences = 0;
+ private static $storageFixture;
+ private static $storageFixtureObjectSeed = 0;
+ private static $testsAreRunning = 0;
+
+ protected function getPhabricatorTestCaseConfiguration() {
+ return array();
+ }
+
+ private function getComputedConfiguration() {
+ $config = $this->getPhabricatorTestCaseConfiguration() + array(
+ self::PHABRICATOR_TESTCONFIG_ISOLATE_LISK => true,
+ self::PHABRICATOR_TESTCONFIG_BUILD_STORAGE_FIXTURES => false,
+ );
+
+ if ($config[self::PHABRICATOR_TESTCONFIG_BUILD_STORAGE_FIXTURES]) {
+ // Fixtures don't make sense with process isolation.
+ $config[self::PHABRICATOR_TESTCONFIG_ISOLATE_LISK] = false;
+ }
+
+ return $config;
+ }
+
+ public function willRunTestCases(array $test_cases) {
+ $root = dirname(phutil_get_library_root('sprint'));
+ require_once $root.'/scripts/__init_script__.php';
+
+ $config = $this->getComputedConfiguration();
+
+ if ($config[self::PHABRICATOR_TESTCONFIG_BUILD_STORAGE_FIXTURES]) {
+ ++self::$storageFixtureReferences;
+ if (!self::$storageFixture) {
+ self::$storageFixture = $this->newStorageFixture();
+ }
+ }
+
+ ++self::$testsAreRunning;
+ }
+
+ public function didRunTestCases(array $test_cases) {
+ if (self::$storageFixture) {
+ self::$storageFixtureReferences--;
+ if (!self::$storageFixtureReferences) {
+ self::$storageFixture = null;
+ }
+ }
+
+ --self::$testsAreRunning;
+ }
+
+ protected function willRunTests() {
+ $config = $this->getComputedConfiguration();
+
+ if ($config[self::PHABRICATOR_TESTCONFIG_ISOLATE_LISK]) {
+ LiskDAO::beginIsolateAllLiskEffectsToCurrentProcess();
+ }
+
+ $this->env = PhabricatorEnv::beginScopedEnv();
+
+ // NOTE: While running unit tests, we act as though all applications are
+ // installed, regardless of the install's configuration. Tests which need
+ // to uninstall applications are responsible for adjusting state themselves
+ // (such tests are exceedingly rare).
+
+ $this->env->overrideEnvConfig(
+ 'phabricator.uninstalled-applications',
+ array());
+ $this->env->overrideEnvConfig(
+ 'phabricator.show-prototypes',
+ true);
+
+ // Reset application settings to defaults, particularly policies.
+ $this->env->overrideEnvConfig(
+ 'phabricator.application-settings',
+ array());
+
+ // We can't stub this service right now, and it's not generally useful
+ // to publish notifications about test execution.
+ $this->env->overrideEnvConfig(
+ 'notification.enabled',
+ false);
+
+ $this->env->overrideEnvConfig(
+ 'phabricator.base-uri',
+ 'http://phabricator.example.com');
+ }
+
+ protected function didRunTests() {
+ $config = $this->getComputedConfiguration();
+
+ if ($config[self::PHABRICATOR_TESTCONFIG_ISOLATE_LISK]) {
+ LiskDAO::endIsolateAllLiskEffectsToCurrentProcess();
+ }
+
+ try {
+ if (phutil_is_hiphop_runtime()) {
+ $this->env->__destruct();
+ }
+ unset($this->env);
+ } catch (Exception $ex) {
+ throw new Exception(
+ 'Some test called PhabricatorEnv::beginScopedEnv(), but is still '.
+ 'holding a reference to the scoped environment!');
+ }
+ }
+
+ protected function willRunOneTest($test) {
+ $config = $this->getComputedConfiguration();
+
+ if ($config[self::PHABRICATOR_TESTCONFIG_BUILD_STORAGE_FIXTURES]) {
+ LiskDAO::beginIsolateAllLiskEffectsToTransactions();
+ }
+ }
+
+ protected function didRunOneTest($test) {
+ $config = $this->getComputedConfiguration();
+
+ if ($config[self::PHABRICATOR_TESTCONFIG_BUILD_STORAGE_FIXTURES]) {
+ LiskDAO::endIsolateAllLiskEffectsToTransactions();
+ }
+ }
+
+ protected function newStorageFixture() {
+ $bytes = Filesystem::readRandomCharacters(24);
+ $name = self::NAMESPACE_PREFIX.$bytes;
+
+ return new PhabricatorStorageFixtureScopeGuard($name);
+ }
+
+ protected function getLink($method) {
+ $phabricator_project = 'PHID-APRJ-3f1fc779edeab89b2171';
+ return
+ 'https://secure.phabricator.com/diffusion/symbol/'.$method.
+ '/?lang=php&projects='.$phabricator_project.
+ '&jump=true&context='.get_class($this);
+ }
+
+ /**
+ * Returns an integer seed to use when building unique identifiers (e.g.,
+ * non-colliding usernames). The seed is unstable and its value will change
+ * between test runs, so your tests must not rely on it.
+ *
+ * @return int A unique integer.
+ */
+ protected function getNextObjectSeed() {
+ self::$storageFixtureObjectSeed += mt_rand(1, 100);
+ return self::$storageFixtureObjectSeed;
+ }
+
+ protected function generateNewTestUser() {
+ $seed = $this->getNextObjectSeed();
+
+ $user = id(new PhabricatorUser())
+ ->setRealName("Test User {$seed}}")
+ ->setUserName("test{$seed}")
+ ->setIsApproved(1);
+
+ $email = id(new PhabricatorUserEmail())
+ ->setAddress("testuser{$seed}@example.com")
+ ->setIsVerified(1);
+
+ $editor = new PhabricatorUserEditor();
+ $editor->setActor($user);
+ $editor->createNewUser($user, $email);
+
+ return $user;
+ }
+
+
+ /**
+ * Throws unless tests are currently executing. This method can be used to
+ * guard code which is specific to unit tests and should not normally be
+ * reachable.
+ *
+ * If tests aren't currently being executed, throws an exception.
+ */
+ public static function assertExecutingUnitTests() {
+ if (!self::$testsAreRunning) {
+ throw new Exception(
+ 'Executing test code outside of test execution! This code path can '.
+ 'only be run during unit tests.');
+ }
+ }
+
+
+}
\ No newline at end of file
diff --git a/src/controller/BurndownDataViewController.php
b/src/controller/BurndownDataViewController.php
index fa14fa6..8e1950c 100644
--- a/src/controller/BurndownDataViewController.php
+++ b/src/controller/BurndownDataViewController.php
@@ -29,7 +29,8 @@
try {
$burndown_view = id(new BurndownDataView())
->setProject($project)
- ->setViewer($viewer);
+ ->setViewer($viewer)
+ ->setRequest($request);
} catch (BurndownException $e) {
$error_box = id(new AphrontErrorView())
->setTitle(pht('Burndown could not be rendered for this project'))
diff --git a/src/query/SprintQuery.php b/src/query/SprintQuery.php
index f81d693..510340f 100644
--- a/src/query/SprintQuery.php
+++ b/src/query/SprintQuery.php
@@ -117,6 +117,15 @@
return $data;
}
+ public function getEdges ($tasks) {
+
+ // Load all edges of depends and depended on tasks
+ $edges = id(new PhabricatorEdgeQuery())
+ ->withSourcePHIDs(array_keys($tasks))
+
->withEdgeTypes(array(PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK,
PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK))
+ ->execute();
+ }
+
public function getEvents($xactions) {
$scope_phids = array($this->project->getPHID());
$events = $this->extractEvents($xactions, $scope_phids);
diff --git a/src/view/BurndownDataView.php b/src/view/BurndownDataView.php
index b8ce0f9..c0e67fd 100644
--- a/src/view/BurndownDataView.php
+++ b/src/view/BurndownDataView.php
@@ -6,6 +6,7 @@
final class BurndownDataView extends SprintView {
+ private $request;
private $timeseries;
private $sprint_data;
private $project;
@@ -25,6 +26,11 @@
return $this;
}
+ public function setRequest ($request) {
+ $this->request = $request;
+ return $this;
+ }
+
public function setTimeZone ($viewer) {
$timezone = new DateTimeZone($viewer->getTimezoneIdentifier());
return $timezone;
@@ -36,7 +42,7 @@
$pie = $this->buildC3Pie();
$burndown_table = $this->buildBurnDownTable();
$event_table = $this->buildEventTable();
- return array ($chart, $pie, $tasks_table, $burndown_table, $event_table);
+ return array ($chart, $tasks_table, $pie, $burndown_table, $event_table);
}
private function buildChartDataSet() {
@@ -196,8 +202,9 @@
* @returns PHUIObjectBoxView
*/
private function buildTasksTable() {
- $rows = $this->buildTasksTree();
-
+ $order = $this->request->getStr('order', 'name');
+ list($order, $reverse) = AphrontTableView::parseSort($order);
+ $rows = $this->buildTasksTree($order, $reverse);
$table = id(new AphrontTableView($rows))
->setHeaders(
array(
@@ -207,6 +214,19 @@
pht('Points'),
pht('Status'),
));
+ $table->makeSortable(
+ $this->request->getRequestURI(),
+ 'order',
+ $order,
+ $reverse,
+ array(
+ 'Task',
+ 'Assigned to',
+ 'Priority',
+ 'Points',
+ 'Status'
+ )
+ );
$box = id(new PHUIObjectBoxView())
->setHeaderText(pht('Tasks in this Sprint'))
@@ -215,30 +235,35 @@
return $box;
}
- /**
- * This builds a tree of the tasks in this project. Due to the acyclic nature
- * of tasks, we ntake some steps to reduce and call out duplication.
- *
- * We ignore any tasks not in this sprint.
- *
- * @return array
- */
- private function buildTasksTree() {
- // Shorter constants
- $DEPENDS_ON = PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK;
- $DEPENDED_ON = PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK;
+ private function setSortOrder ($row, $order, $task, $assigned_to, $priority,
+ $points, $status) {
+ switch ($order) {
+ case 'Task':
+ $row['sort'] = $task;
+ break;
+ case 'Assigned to':
+ $row['sort'] = $assigned_to;
+ break;
+ case 'Priority':
+ $row['sort'] = $priority;
+ break;
+ case 'Points':
+ $row['sort'] = $points;
+ break;
+ case 'Status':
+ default:
+ $row['sort'] = $status;
+ break;
+ }
+ return $row['sort'];
+ }
- // Load all edges of depends and depended on tasks
- $edges = id(new PhabricatorEdgeQuery())
- ->withSourcePHIDs(array_keys($this->tasks))
- ->withEdgeTypes(array($DEPENDS_ON, $DEPENDED_ON))
- ->execute();
- // First we build a flat map. Each task is in the map at the root level,
- // and lists it's parents and children.
+ private function buildTaskMap ($tasks, $edges) {
$map = array();
foreach ($this->tasks as $task) {
- if ($parents = $edges[$task->getPHID()][$DEPENDED_ON]) {
+ if ($parents =
+
$edges[$task->getPHID()][PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK])
{
foreach ($parents as $parent) {
// Make sure this task is in this sprint.
if (isset($this->tasks[$parent['dst']]))
@@ -246,7 +271,8 @@
}
}
- if ($children = $edges[$task->getPHID()][$DEPENDS_ON]) {
+ if ($children =
+
$edges[$task->getPHID()][PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK]) {
foreach ($children as $child) {
// Make sure this task is in this sprint.
if (isset($this->tasks[$child['dst']])) {
@@ -255,6 +281,20 @@
}
}
}
+ return $map;
+ }
+ /**
+ * This builds a tree of the tasks in this project. Due to the acyclic nature
+ * of tasks, we ntake some steps to reduce and call out duplication.
+ *
+ * We ignore any tasks not in this sprint.
+ *
+ * @return array
+ */
+ private function buildTasksTree($order, $reverse) {
+ $query = id(new SprintQuery());
+ $edges = $query->getEdges($this->tasks);
+ $map = $this->buildTaskMap($this->tasks, $edges);
// We also collect the phids we need to fetch owner information
$handle_phids = array();
@@ -262,11 +302,7 @@
// Get the owner (assigned to) phid
$handle_phids[$task->getOwnerPHID()] = $task->getOwnerPHID();
}
-
- $handles = id(new PhabricatorHandleQuery())
- ->setViewer($this->viewer)
- ->withPHIDs($handle_phids)
- ->execute();
+ $handles = $query->getViewerHandles($this->request, $handle_phids);
// Now we loop through the tasks, and add them to the output
$output = array();
@@ -277,13 +313,25 @@
continue;
}
- $this->addTaskToTree($output, $task, $map, $handles);
+ $row = $this->addTaskToTree($output, $task, $map, $handles);
+ list ($task, $assigned_to, $priority,$points, $status) = $row[0];
+ $row['sort'] = $this->setSortOrder($row, $order, $task, $assigned_to,
$priority,$points, $status);
+ $rows[] = $row;
+ }
+ $rows = isort($rows, 'sort');
+
+ foreach ($rows as $k => $row) {
+ unset($rows[$k]['sort']);
}
- return $output;
+ if ($reverse) {
+ $rows = array_reverse($rows);
+ }
+ $rows = array_map( function( $a ) { return $a['0']; }, $rows );
+ return $rows;
}
- private function addTaskToTree(&$output, $task, &$map, $handles, $depth = 0)
{
+ private function addTaskToTree($output, $task, $map, $handles, $depth = 0) {
static $included = array();
$query = id(new SprintQuery())
->setProject($this->project)
@@ -333,6 +381,7 @@
$this->addTaskToTree($output, $child, $map, $handles, $depth + 1);
}
}
+ return $output;
}
/**
--
To view, visit https://gerrit.wikimedia.org/r/172465
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7148ba1dd395696ea9f60bf861fa49d022473ba7
Gerrit-PatchSet: 1
Gerrit-Project: phabricator/extensions/Sprint
Gerrit-Branch: master
Gerrit-Owner: Christopher Johnson (WMDE) <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
