From 7ad4c9c056d1f03bc2a2b87f3f8a52e037b67446 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 21 Jun 2015 09:00:00 -0700 Subject: [PATCH 01/10] Replace Harbormaster "BuildItem" with Lint/Unit messages Summary: Ref T8095. Harbormaster has a `BuildItem` class, but it has no table and is unused. This was an earlier idea about representing lint/unit results and some other possible types of messages, but I think we want to be more specific than this. Remove `BuildItem` and add `Lint` and `Unit` storage. These tables roughly parallel how we store lint/unit messages today, with some guesses about how where they'll go in the future. Test Plan: Ran `bin/storage upgrade` and got a clean adjust out of it. Reviewers: btrahan Reviewed By: btrahan Subscribers: hach-que, epriestley Maniphest Tasks: T8095 Differential Revision: https://secure.phabricator.com/D13329 --- .../autopatches/20150617.harbor.1.lint.sql | 14 +++ .../autopatches/20150617.harbor.2.unit.sql | 13 +++ src/__phutil_library_map__.php | 10 +- .../phid/HarbormasterBuildItemPHIDType.php | 33 ------ .../query/HarbormasterBuildItemQuery.php | 60 ----------- .../storage/build/HarbormasterBuildItem.php | 19 ---- .../build/HarbormasterBuildLintMessage.php | 98 +++++++++++++++++ .../build/HarbormasterBuildUnitMessage.php | 100 ++++++++++++++++++ 8 files changed, 229 insertions(+), 118 deletions(-) create mode 100644 resources/sql/autopatches/20150617.harbor.1.lint.sql create mode 100644 resources/sql/autopatches/20150617.harbor.2.unit.sql delete mode 100644 src/applications/harbormaster/phid/HarbormasterBuildItemPHIDType.php delete mode 100644 src/applications/harbormaster/query/HarbormasterBuildItemQuery.php delete mode 100644 src/applications/harbormaster/storage/build/HarbormasterBuildItem.php create mode 100644 src/applications/harbormaster/storage/build/HarbormasterBuildLintMessage.php create mode 100644 src/applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php diff --git a/resources/sql/autopatches/20150617.harbor.1.lint.sql b/resources/sql/autopatches/20150617.harbor.1.lint.sql new file mode 100644 index 0000000000..ff23386509 --- /dev/null +++ b/resources/sql/autopatches/20150617.harbor.1.lint.sql @@ -0,0 +1,14 @@ +CREATE TABLE {$NAMESPACE}_harbormaster.harbormaster_buildlintmessage ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + buildTargetPHID VARBINARY(64) NOT NULL, + path LONGTEXT NOT NULL, + line INT UNSIGNED, + characterOffset INT UNSIGNED, + code VARCHAR(32) COLLATE {$COLLATE_TEXT} NOT NULL, + severity VARCHAR(32) COLLATE {$COLLATE_TEXT} NOT NULL, + name VARCHAR(255) COLLATE {$COLLATE_TEXT} NOT NULL, + properties LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL, + KEY `key_target` (buildTargetPHID) +) ENGINE=INNODB, COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20150617.harbor.2.unit.sql b/resources/sql/autopatches/20150617.harbor.2.unit.sql new file mode 100644 index 0000000000..3140947652 --- /dev/null +++ b/resources/sql/autopatches/20150617.harbor.2.unit.sql @@ -0,0 +1,13 @@ +CREATE TABLE {$NAMESPACE}_harbormaster.harbormaster_buildunitmessage ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + buildTargetPHID VARBINARY(64) NOT NULL, + engine VARCHAR(255) COLLATE {$COLLATE_TEXT} NOT NULL, + namespace VARCHAR(255) COLLATE {$COLLATE_TEXT} NOT NULL, + name VARCHAR(255) COLLATE {$COLLATE_TEXT} NOT NULL, + result VARCHAR(32) COLLATE {$COLLATE_TEXT} NOT NULL, + duration DOUBLE, + properties LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL, + KEY `key_target` (buildTargetPHID) +) ENGINE=INNODB, COLLATE {$COLLATE_TEXT}; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 3454bae920..a449b84b28 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -833,9 +833,7 @@ phutil_register_library_map(array( 'HarbormasterBuildEngine' => 'applications/harbormaster/engine/HarbormasterBuildEngine.php', 'HarbormasterBuildFailureException' => 'applications/harbormaster/exception/HarbormasterBuildFailureException.php', 'HarbormasterBuildGraph' => 'applications/harbormaster/engine/HarbormasterBuildGraph.php', - 'HarbormasterBuildItem' => 'applications/harbormaster/storage/build/HarbormasterBuildItem.php', - 'HarbormasterBuildItemPHIDType' => 'applications/harbormaster/phid/HarbormasterBuildItemPHIDType.php', - 'HarbormasterBuildItemQuery' => 'applications/harbormaster/query/HarbormasterBuildItemQuery.php', + 'HarbormasterBuildLintMessage' => 'applications/harbormaster/storage/build/HarbormasterBuildLintMessage.php', 'HarbormasterBuildLog' => 'applications/harbormaster/storage/build/HarbormasterBuildLog.php', 'HarbormasterBuildLogPHIDType' => 'applications/harbormaster/phid/HarbormasterBuildLogPHIDType.php', 'HarbormasterBuildLogQuery' => 'applications/harbormaster/query/HarbormasterBuildLogQuery.php', @@ -867,6 +865,7 @@ phutil_register_library_map(array( 'HarbormasterBuildTransaction' => 'applications/harbormaster/storage/HarbormasterBuildTransaction.php', 'HarbormasterBuildTransactionEditor' => 'applications/harbormaster/editor/HarbormasterBuildTransactionEditor.php', 'HarbormasterBuildTransactionQuery' => 'applications/harbormaster/query/HarbormasterBuildTransactionQuery.php', + 'HarbormasterBuildUnitMessage' => 'applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php', 'HarbormasterBuildViewController' => 'applications/harbormaster/controller/HarbormasterBuildViewController.php', 'HarbormasterBuildWorker' => 'applications/harbormaster/worker/HarbormasterBuildWorker.php', 'HarbormasterBuildable' => 'applications/harbormaster/storage/HarbormasterBuildable.php', @@ -4253,9 +4252,7 @@ phutil_register_library_map(array( 'HarbormasterBuildEngine' => 'Phobject', 'HarbormasterBuildFailureException' => 'Exception', 'HarbormasterBuildGraph' => 'AbstractDirectedGraph', - 'HarbormasterBuildItem' => 'HarbormasterDAO', - 'HarbormasterBuildItemPHIDType' => 'PhabricatorPHIDType', - 'HarbormasterBuildItemQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'HarbormasterBuildLintMessage' => 'HarbormasterDAO', 'HarbormasterBuildLog' => array( 'HarbormasterDAO', 'PhabricatorPolicyInterface', @@ -4309,6 +4306,7 @@ phutil_register_library_map(array( 'HarbormasterBuildTransaction' => 'PhabricatorApplicationTransaction', 'HarbormasterBuildTransactionEditor' => 'PhabricatorApplicationTransactionEditor', 'HarbormasterBuildTransactionQuery' => 'PhabricatorApplicationTransactionQuery', + 'HarbormasterBuildUnitMessage' => 'HarbormasterDAO', 'HarbormasterBuildViewController' => 'HarbormasterController', 'HarbormasterBuildWorker' => 'HarbormasterWorker', 'HarbormasterBuildable' => array( diff --git a/src/applications/harbormaster/phid/HarbormasterBuildItemPHIDType.php b/src/applications/harbormaster/phid/HarbormasterBuildItemPHIDType.php deleted file mode 100644 index 08c657fe56..0000000000 --- a/src/applications/harbormaster/phid/HarbormasterBuildItemPHIDType.php +++ /dev/null @@ -1,33 +0,0 @@ -withPHIDs($phids); - } - - public function loadHandles( - PhabricatorHandleQuery $query, - array $handles, - array $objects) { - - foreach ($handles as $phid => $handle) { - $build_item = $objects[$phid]; - } - } - -} diff --git a/src/applications/harbormaster/query/HarbormasterBuildItemQuery.php b/src/applications/harbormaster/query/HarbormasterBuildItemQuery.php deleted file mode 100644 index 1c54cf1fa7..0000000000 --- a/src/applications/harbormaster/query/HarbormasterBuildItemQuery.php +++ /dev/null @@ -1,60 +0,0 @@ -ids = $ids; - return $this; - } - - public function withPHIDs(array $phids) { - $this->phids = $phids; - return $this; - } - - protected function loadPage() { - $table = new HarbormasterBuildItem(); - $conn_r = $table->establishConnection('r'); - - $data = queryfx_all( - $conn_r, - 'SELECT * FROM %T %Q %Q %Q', - $table->getTableName(), - $this->buildWhereClause($conn_r), - $this->buildOrderClause($conn_r), - $this->buildLimitClause($conn_r)); - - return $table->loadAllFromArray($data); - } - - protected function buildWhereClause(AphrontDatabaseConnection $conn_r) { - $where = array(); - - if ($this->ids) { - $where[] = qsprintf( - $conn_r, - 'id IN (%Ld)', - $this->ids); - } - - if ($this->phids) { - $where[] = qsprintf( - $conn_r, - 'phid in (%Ls)', - $this->phids); - } - - $where[] = $this->buildPagingClause($conn_r); - - return $this->formatWhereClause($where); - } - - public function getQueryApplicationClass() { - return 'PhabricatorHarbormasterApplication'; - } - -} diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildItem.php b/src/applications/harbormaster/storage/build/HarbormasterBuildItem.php deleted file mode 100644 index 0d3ce7fcad..0000000000 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildItem.php +++ /dev/null @@ -1,19 +0,0 @@ - true, - self::CONFIG_NO_TABLE => true, - ) + parent::getConfiguration(); - } - - public function generatePHID() { - return PhabricatorPHID::generateNewPHID( - HarbormasterBuildItemPHIDType::TYPECONST); - } - -} diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildLintMessage.php b/src/applications/harbormaster/storage/build/HarbormasterBuildLintMessage.php new file mode 100644 index 0000000000..27b0948af7 --- /dev/null +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildLintMessage.php @@ -0,0 +1,98 @@ +setBuildTargetPHID($build_target->getPHID()); + } + + public static function newFromDictionary( + HarbormasterBuildTarget $build_target, + array $dict) { + + $obj = self::initializeNewLintMessage($build_target); + + $spec = array( + 'path' => 'string', + 'line' => 'optional int', + 'char' => 'optional int', + 'code' => 'string', + 'severity' => 'string', + 'name' => 'string', + 'description' => 'optional string', + ); + + // We're just going to ignore extra keys for now, to make it easier to + // add stuff here later on. + $dict = array_select_keys($dict, array_keys($spec)); + PhutilTypeSpec::checkMap($dict, $spec); + + $obj->setPath($dict['path']); + $obj->setLine(idx($dict, 'line')); + $obj->setCharacterOffset(idx($dict, 'char')); + $obj->setCode($dict['code']); + $obj->setSeverity($dict['severity']); + $obj->setName($dict['name']); + + $description = idx($dict, 'description'); + if (strlen($description)) { + $obj->setProperty('description', $description); + } + + return $obj; + } + + protected function getConfiguration() { + return array( + self::CONFIG_SERIALIZATION => array( + 'properties' => self::SERIALIZATION_JSON, + ), + self::CONFIG_COLUMN_SCHEMA => array( + 'path' => 'text', + 'line' => 'uint32?', + 'characterOffset' => 'uint32?', + 'code' => 'text32', + 'severity' => 'text32', + 'name' => 'text255', + ), + self::CONFIG_KEY_SCHEMA => array( + 'key_target' => array( + 'columns' => array('buildTargetPHID'), + ), + ), + ) + parent::getConfiguration(); + } + + public function attachBuildTarget(HarbormasterBuildTarget $build_target) { + $this->buildTarget = $build_target; + return $this; + } + + public function getBuildTarget() { + return $this->assertAttached($this->buildTarget); + } + + public function getProperty($key, $default = null) { + return idx($this->properties, $key, $default); + } + + public function setProperty($key, $value) { + $this->properties[$key] = $value; + return $this; + } + +} diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php b/src/applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php new file mode 100644 index 0000000000..d21fc2f718 --- /dev/null +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php @@ -0,0 +1,100 @@ +setBuildTargetPHID($build_target->getPHID()); + } + + public static function newFromDictionary( + HarbormasterBuildTarget $build_target, + array $dict) { + + $obj = self::initializeNewUnitMessage($build_target); + + $spec = array( + 'engine' => 'optional string', + 'namespace' => 'optional string', + 'name' => 'string', + 'result' => 'string', + 'duration' => 'optional float', + 'path' => 'optional string', + 'coverage' => 'optional string', + ); + + // We're just going to ignore extra keys for now, to make it easier to + // add stuff here later on. + $dict = array_select_keys($dict, array_keys($spec)); + PhutilTypeSpec::checkMap($dict, $spec); + + $obj->setEngine(idx($dict, 'engine', '')); + $obj->setNamespace(idx($dict, 'namespace', '')); + $obj->setName($dict['name']); + $obj->setResult($dict['result']); + $obj->setDuration(idx($dict, 'duration')); + + $path = idx($dict, 'path'); + if (strlen($path)) { + $obj->setProperty('path', $path); + } + + $coverage = idx($dict, 'coverage'); + if (strlen($coverage)) { + $obj->setProperty('coverage', $coverage); + } + + return $obj; + } + + protected function getConfiguration() { + return array( + self::CONFIG_SERIALIZATION => array( + 'properties' => self::SERIALIZATION_JSON, + ), + self::CONFIG_COLUMN_SCHEMA => array( + 'engine' => 'text255', + 'namespace' => 'text255', + 'name' => 'text255', + 'result' => 'text32', + 'duration' => 'double?', + ), + self::CONFIG_KEY_SCHEMA => array( + 'key_target' => array( + 'columns' => array('buildTargetPHID'), + ), + ), + ) + parent::getConfiguration(); + } + + public function attachBuildTarget(HarbormasterBuildTarget $build_target) { + $this->buildTarget = $build_target; + return $this; + } + + public function getBuildTarget() { + return $this->assertAttached($this->buildTarget); + } + + public function getProperty($key, $default = null) { + return idx($this->properties, $key, $default); + } + + public function setProperty($key, $value) { + $this->properties[$key] = $value; + return $this; + } + +} From 081300c7f861b858b852674746a08b0dc012679d Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 21 Jun 2015 09:02:44 -0700 Subject: [PATCH 02/10] Modernize some Harbormaster Controller/Policy infrastructure Summary: Ref T8095. This is just general groundwork for more exciting changes: - Use more modern conventions around controllers, UI elements, and dialogs. - Provide real CAN_EDIT policies and policy checks (they just don't do anything yet). Test Plan: - Used all affected controllers. - Faked CAN_EDIT to POLICY_NOONE and verified everything was greyed out and unselectable. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T8095 Differential Revision: https://secure.phabricator.com/D13344 --- .../controller/HarbormasterController.php | 17 ----- .../HarbormasterPlanDisableController.php | 23 +++--- .../HarbormasterPlanEditController.php | 22 +++--- .../HarbormasterPlanListController.php | 28 +++++-- .../HarbormasterPlanRunController.php | 23 +++--- .../HarbormasterPlanViewController.php | 73 ++++++++++++------- .../HarbormasterStepAddController.php | 18 ++--- .../HarbormasterStepDeleteController.php | 39 ++++------ .../HarbormasterStepEditController.php | 44 +++++------ .../configuration/HarbormasterBuildPlan.php | 6 ++ .../configuration/HarbormasterBuildStep.php | 1 + 11 files changed, 148 insertions(+), 146 deletions(-) diff --git a/src/applications/harbormaster/controller/HarbormasterController.php b/src/applications/harbormaster/controller/HarbormasterController.php index 7202e356a2..c946cf7002 100644 --- a/src/applications/harbormaster/controller/HarbormasterController.php +++ b/src/applications/harbormaster/controller/HarbormasterController.php @@ -2,21 +2,4 @@ abstract class HarbormasterController extends PhabricatorController { - protected function buildApplicationCrumbs() { - $crumbs = parent::buildApplicationCrumbs(); - - $can_create = $this->hasApplicationCapability( - HarbormasterManagePlansCapability::CAPABILITY); - - $crumbs->addAction( - id(new PHUIListItemView()) - ->setName(pht('New Build Plan')) - ->setHref($this->getApplicationURI('plan/edit/')) - ->setDisabled(!$can_create) - ->setWorkflow(!$can_create) - ->setIcon('fa-plus-square')); - - return $crumbs; - } - } diff --git a/src/applications/harbormaster/controller/HarbormasterPlanDisableController.php b/src/applications/harbormaster/controller/HarbormasterPlanDisableController.php index 3937495343..28cb8340a6 100644 --- a/src/applications/harbormaster/controller/HarbormasterPlanDisableController.php +++ b/src/applications/harbormaster/controller/HarbormasterPlanDisableController.php @@ -3,22 +3,20 @@ final class HarbormasterPlanDisableController extends HarbormasterPlanController { - private $id; - - public function willProcessRequest(array $data) { - $this->id = $data['id']; - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); $this->requireApplicationCapability( HarbormasterManagePlansCapability::CAPABILITY); $plan = id(new HarbormasterBuildPlanQuery()) ->setViewer($viewer) - ->withIDs(array($this->id)) + ->withIDs(array($request->getURIData('id'))) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) ->executeOne(); if (!$plan) { return new Aphront404Response(); @@ -63,14 +61,11 @@ final class HarbormasterPlanDisableController $button = pht('Disable Plan'); } - $dialog = id(new AphrontDialogView()) - ->setUser($viewer) + return $this->newDialog() ->setTitle($title) ->appendChild($body) ->addSubmitButton($button) ->addCancelButton($plan_uri); - - return id(new AphrontDialogResponse())->setDialog($dialog); } } diff --git a/src/applications/harbormaster/controller/HarbormasterPlanEditController.php b/src/applications/harbormaster/controller/HarbormasterPlanEditController.php index 8679d9ce4c..dd43aa0bee 100644 --- a/src/applications/harbormaster/controller/HarbormasterPlanEditController.php +++ b/src/applications/harbormaster/controller/HarbormasterPlanEditController.php @@ -2,23 +2,22 @@ final class HarbormasterPlanEditController extends HarbormasterPlanController { - private $id; - - public function willProcessRequest(array $data) { - $this->id = idx($data, 'id'); - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); $this->requireApplicationCapability( HarbormasterManagePlansCapability::CAPABILITY); - if ($this->id) { + $id = $request->getURIData('id'); + if ($id) { $plan = id(new HarbormasterBuildPlanQuery()) ->setViewer($viewer) - ->withIDs(array($this->id)) + ->withIDs(array($id)) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) ->executeOne(); if (!$plan) { return new Aphront404Response(); @@ -43,6 +42,7 @@ final class HarbormasterPlanEditController extends HarbormasterPlanController { $editor = id(new HarbormasterBuildPlanEditor()) ->setActor($viewer) + ->setContinueOnNoEffect(true) ->setContentSourceFromRequest($request); try { diff --git a/src/applications/harbormaster/controller/HarbormasterPlanListController.php b/src/applications/harbormaster/controller/HarbormasterPlanListController.php index 0e2cb3e233..e0f9ce07c7 100644 --- a/src/applications/harbormaster/controller/HarbormasterPlanListController.php +++ b/src/applications/harbormaster/controller/HarbormasterPlanListController.php @@ -2,19 +2,13 @@ final class HarbormasterPlanListController extends HarbormasterPlanController { - private $queryKey; - public function shouldAllowPublic() { return true; } - public function willProcessRequest(array $data) { - $this->queryKey = idx($data, 'queryKey'); - } - - public function processRequest() { + public function handleRequest(AphrontRequest $request) { $controller = id(new PhabricatorApplicationSearchController()) - ->setQueryKey($this->queryKey) + ->setQueryKey($request->getURIData('queryKey')) ->setSearchEngine(new HarbormasterBuildPlanSearchEngine()) ->setNavigation($this->buildSideNavView()); @@ -44,4 +38,22 @@ final class HarbormasterPlanListController extends HarbormasterPlanController { return $this->buildSideNavView(true)->getMenu(); } + protected function buildApplicationCrumbs() { + $crumbs = parent::buildApplicationCrumbs(); + + $can_create = $this->hasApplicationCapability( + HarbormasterManagePlansCapability::CAPABILITY); + + $crumbs->addAction( + id(new PHUIListItemView()) + ->setName(pht('New Build Plan')) + ->setHref($this->getApplicationURI('plan/edit/')) + ->setDisabled(!$can_create) + ->setWorkflow(!$can_create) + ->setIcon('fa-plus-square')); + + return $crumbs; + } + + } diff --git a/src/applications/harbormaster/controller/HarbormasterPlanRunController.php b/src/applications/harbormaster/controller/HarbormasterPlanRunController.php index e4f94e05d1..0f5673087d 100644 --- a/src/applications/harbormaster/controller/HarbormasterPlanRunController.php +++ b/src/applications/harbormaster/controller/HarbormasterPlanRunController.php @@ -2,20 +2,18 @@ final class HarbormasterPlanRunController extends HarbormasterController { - private $id; - - public function willProcessRequest(array $data) { - $this->id = $data['id']; - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); $this->requireApplicationCapability( HarbormasterManagePlansCapability::CAPABILITY); - $plan_id = $this->id; + $plan_id = $request->getURIData('id'); + + // NOTE: At least for now, this only requires the "Can Manage Plans" + // capability, not the "Can Edit" capability. Possibly it should have + // a more stringent requirement, though. + $plan = id(new HarbormasterBuildPlanQuery()) ->setViewer($viewer) ->withIDs(array($plan_id)) @@ -87,15 +85,12 @@ final class HarbormasterPlanRunController extends HarbormasterController { ->setError($e_name) ->setValue($v_name)); - $dialog = id(new AphrontDialogView()) + return $this->newDialog() ->setWidth(AphrontDialogView::WIDTH_FULL) - ->setUser($viewer) ->setTitle($title) ->appendChild($form) ->addCancelButton($cancel_uri) ->addSubmitButton($save_button); - - return id(new AphrontDialogResponse())->setDialog($dialog); } } diff --git a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php index 8db8eaf118..3cde3ab5c8 100644 --- a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php @@ -2,17 +2,10 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { - private $id; + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getviewer(); - public function willProcessRequest(array $data) { - $this->id = $data['id']; - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); - - $id = $this->id; + $id = $request->getURIData('id'); $plan = id(new HarbormasterBuildPlanQuery()) ->setViewer($viewer) @@ -79,11 +72,9 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { } private function buildStepList(HarbormasterBuildPlan $plan) { - $request = $this->getRequest(); - $viewer = $request->getUser(); + $viewer = $this->getViewer(); - $run_order = - HarbormasterBuildGraph::determineDependencyExecution($plan); + $run_order = HarbormasterBuildGraph::determineDependencyExecution($plan); $steps = id(new HarbormasterBuildStepQuery()) ->setViewer($viewer) @@ -91,9 +82,16 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { ->execute(); $steps = mpull($steps, null, 'getPHID'); - $can_edit = $this->hasApplicationCapability( + $has_manage = $this->hasApplicationCapability( HarbormasterManagePlansCapability::CAPABILITY); + $has_edit = PhabricatorPolicyFilter::hasCapability( + $viewer, + $plan, + PhabricatorPolicyCapability::CAN_EDIT); + + $can_edit = ($has_manage && $has_edit); + $step_list = id(new PHUIObjectItemListView()) ->setUser($viewer) ->setNoDataString( @@ -222,12 +220,32 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { $step_list->addItem($item); } - return array($step_list, $has_any_conflicts, $is_deadlocking); + $step_list->setFlush(true); + + $plan_id = $plan->getID(); + + $header = id(new PHUIHeaderView()) + ->setHeader(pht('Build Steps')) + ->addActionLink( + id(new PHUIButtonView()) + ->setText(pht('Add Build Step')) + ->setHref($this->getApplicationURI("step/add/{$plan_id}/")) + ->setTag('a') + ->setIcon( + id(new PHUIIconView()) + ->setIconFont('fa-plus')) + ->setDisabled(!$can_edit) + ->setWorkflow(true)); + + $step_box = id(new PHUIObjectBoxView()) + ->setHeader($header) + ->appendChild($step_list); + + return array($step_box, $has_any_conflicts, $is_deadlocking); } private function buildActionList(HarbormasterBuildPlan $plan) { - $request = $this->getRequest(); - $viewer = $request->getUser(); + $viewer = $this->getViewer(); $id = $plan->getID(); $list = id(new PhabricatorActionListView()) @@ -235,9 +253,16 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { ->setObject($plan) ->setObjectURI($this->getApplicationURI("plan/{$id}/")); - $can_edit = $this->hasApplicationCapability( + $has_manage = $this->hasApplicationCapability( HarbormasterManagePlansCapability::CAPABILITY); + $has_edit = PhabricatorPolicyFilter::hasCapability( + $viewer, + $plan, + PhabricatorPolicyCapability::CAN_EDIT); + + $can_edit = ($has_manage && $has_edit); + $list->addAction( id(new PhabricatorActionView()) ->setName(pht('Edit Plan')) @@ -264,20 +289,12 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { ->setIcon('fa-ban')); } - $list->addAction( - id(new PhabricatorActionView()) - ->setName(pht('Add Build Step')) - ->setHref($this->getApplicationURI("step/add/{$id}/")) - ->setWorkflow(true) - ->setDisabled(!$can_edit) - ->setIcon('fa-plus')); - $list->addAction( id(new PhabricatorActionView()) ->setName(pht('Run Plan Manually')) ->setHref($this->getApplicationURI("plan/run/{$id}/")) ->setWorkflow(true) - ->setDisabled(!$can_edit) + ->setDisabled(!$has_manage) ->setIcon('fa-play-circle')); return $list; diff --git a/src/applications/harbormaster/controller/HarbormasterStepAddController.php b/src/applications/harbormaster/controller/HarbormasterStepAddController.php index 9b5817e074..2d72c02b0f 100644 --- a/src/applications/harbormaster/controller/HarbormasterStepAddController.php +++ b/src/applications/harbormaster/controller/HarbormasterStepAddController.php @@ -2,22 +2,20 @@ final class HarbormasterStepAddController extends HarbormasterController { - private $id; - - public function willProcessRequest(array $data) { - $this->id = $data['id']; - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); $this->requireApplicationCapability( HarbormasterManagePlansCapability::CAPABILITY); $plan = id(new HarbormasterBuildPlanQuery()) ->setViewer($viewer) - ->withIDs(array($this->id)) + ->withIDs(array($request->getURIData('id'))) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) ->executeOne(); if (!$plan) { return new Aphront404Response(); diff --git a/src/applications/harbormaster/controller/HarbormasterStepDeleteController.php b/src/applications/harbormaster/controller/HarbormasterStepDeleteController.php index c0df595fad..94afff9d3b 100644 --- a/src/applications/harbormaster/controller/HarbormasterStepDeleteController.php +++ b/src/applications/harbormaster/controller/HarbormasterStepDeleteController.php @@ -2,27 +2,25 @@ final class HarbormasterStepDeleteController extends HarbormasterController { - private $id; - - public function willProcessRequest(array $data) { - $this->id = $data['id']; - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); $this->requireApplicationCapability( HarbormasterManagePlansCapability::CAPABILITY); - $id = $this->id; + $id = $request->getURIData('id'); $step = id(new HarbormasterBuildStepQuery()) ->setViewer($viewer) ->withIDs(array($id)) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) ->executeOne(); - if ($step === null) { - throw new Exception(pht('Build step not found!')); + if (!$step) { + return new Aphront404Response(); } $plan_id = $step->getBuildPlan()->getID(); @@ -33,19 +31,14 @@ final class HarbormasterStepDeleteController extends HarbormasterController { return id(new AphrontRedirectResponse())->setURI($done_uri); } - $dialog = new AphrontDialogView(); - $dialog->setTitle(pht('Really Delete Step?')) - ->setUser($viewer) - ->addSubmitButton(pht('Delete Build Step')) - ->addCancelButton($done_uri); - $dialog->appendChild( - phutil_tag( - 'p', - array(), + return $this->newDialog() + ->setTitle(pht('Really Delete Step?')) + ->appendParagraph( pht( "Are you sure you want to delete this step? ". - "This can't be undone!"))); - return id(new AphrontDialogResponse())->setDialog($dialog); + "This can't be undone!")) + ->addCancelButton($done_uri) + ->addSubmitButton(pht('Delete Build Step')); } } diff --git a/src/applications/harbormaster/controller/HarbormasterStepEditController.php b/src/applications/harbormaster/controller/HarbormasterStepEditController.php index c753bd2181..9734f9652f 100644 --- a/src/applications/harbormaster/controller/HarbormasterStepEditController.php +++ b/src/applications/harbormaster/controller/HarbormasterStepEditController.php @@ -2,27 +2,22 @@ final class HarbormasterStepEditController extends HarbormasterController { - private $id; - private $planID; - private $className; - - public function willProcessRequest(array $data) { - $this->id = idx($data, 'id'); - $this->planID = idx($data, 'plan'); - $this->className = idx($data, 'class'); - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); $this->requireApplicationCapability( HarbormasterManagePlansCapability::CAPABILITY); - if ($this->id) { + $id = $request->getURIData('id'); + if ($id) { $step = id(new HarbormasterBuildStepQuery()) ->setViewer($viewer) - ->withIDs(array($this->id)) + ->withIDs(array($id)) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) ->executeOne(); if (!$step) { return new Aphront404Response(); @@ -31,23 +26,30 @@ final class HarbormasterStepEditController extends HarbormasterController { $is_new = false; } else { + $plan_id = $request->getURIData('plan'); + $class = $request->getURIData('class'); + $plan = id(new HarbormasterBuildPlanQuery()) - ->setViewer($viewer) - ->withIDs(array($this->planID)) - ->executeOne(); + ->setViewer($viewer) + ->withIDs(array($plan_id)) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->executeOne(); if (!$plan) { return new Aphront404Response(); } - $impl = HarbormasterBuildStepImplementation::getImplementation( - $this->className); + $impl = HarbormasterBuildStepImplementation::getImplementation($class); if (!$impl) { return new Aphront404Response(); } $step = HarbormasterBuildStep::initializeNewStep($viewer) ->setBuildPlanPHID($plan->getPHID()) - ->setClassName($this->className); + ->setClassName($class); $is_new = true; } diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php index fffc86c656..b63927b9d9 100644 --- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php +++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php @@ -102,6 +102,7 @@ final class HarbormasterBuildPlan extends HarbormasterDAO public function getCapabilities() { return array( PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, ); } @@ -109,6 +110,10 @@ final class HarbormasterBuildPlan extends HarbormasterDAO switch ($capability) { case PhabricatorPolicyCapability::CAN_VIEW: return PhabricatorPolicies::getMostOpenPolicy(); + case PhabricatorPolicyCapability::CAN_EDIT: + // NOTE: In practice, this policy is always limited by the "Mangage + // Build Plans" policy. + return PhabricatorPolicies::getMostOpenPolicy(); } } @@ -119,4 +124,5 @@ final class HarbormasterBuildPlan extends HarbormasterDAO public function describeAutomaticCapability($capability) { return null; } + } diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php index 8446fa542c..90f4c33077 100644 --- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php +++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php @@ -118,6 +118,7 @@ final class HarbormasterBuildStep extends HarbormasterDAO public function getCapabilities() { return array( PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, ); } From 76194a0dc1dda27838d2780d8d1156271fc7b429 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 21 Jun 2015 09:04:21 -0700 Subject: [PATCH 03/10] Add "Autoplans" to Harbormaster Summary: Ref T8095. Two general problems: - I want Harbormaster to own all lint and unit test results. - I don't want users to have to configure anything for `arc` to keep working automatically. These are in conflict because generic lint/unit test ownership in Harbormaster requires that build targets exist which we can attach build results to. However, we can't currently create build targets on demand: Harbormaster assumes it is responsible for creating targets, then running code or making third-party service calls to actually run the builds. I considered two broad approaches to let `arc` push results into Harbormaster without requiring administrators to configure some kind of "arc results" build plan: # Add magic target PHIDs like `PHID-MAGIC-this-is-really-arc-unit`. # Add new code to build real targets with real PHIDs. (1) is probably a bit less work to get off the ground, but I think it's worse overall and very likely to create more problems in the long run. I particularly worry that it will lead to a small amount of special casing in a very large number of places, which seems more fragile. (2) is more work upfront but I think does a better job of putting all the special casing in one place that we can, e.g., more reasonably unit test, and letting the rest of the code rarely/never care about this case since it's just dealing with normal plans/steps/targets as far as it can tell. This diff introduces "autoplans", which are source templates for plans/steps. This let us "push" these targets into Harbormaster. Hypthetically, any process "like" arc can use autoplans to upload test/lint/etc results. In practice, probably only `arc` will ever use this, but I think it's still quite a bit cleaner than the alternative despite all the generality. Workflow is basically: - `arc` creates a diff. - `arc` calls `harbormaster.queryautotargets`, passing the diff PHID and saying "I have some lint and unit results I want to stick on this thing". - Harbormaster builds the plan, steps, and targets (if any of them don't already exist), and hands back the target PHIDs so `arc` has a completely standard-looking place to put results. - `arc` uploads the test results to the right targets, as though Harbormaster had asked it to run unit/lint in the first place. (This doesn't actually do any of that yet, just sets things up.) I'll maybe doc turn that ^^^^^^ into a doc for posterity since I think it's hard to guess what an "autotarget" is, but I'm going to grab some lunch first. Test Plan: - Added unit tests to make sure we can build these things properly. - Used `harbormaster.queryautotargets` to build autotargets for a bunch of diffs. - Verified targets come up in "waiting for message" state. - Verified plans and steps are not editable. Reviewers: btrahan Reviewed By: btrahan Subscribers: hach-que, epriestley Maniphest Tasks: T8095 Differential Revision: https://secure.phabricator.com/D13345 --- .../20150618.harbor.1.planauto.sql | 2 + .../20150618.harbor.2.stepauto.sql | 2 + .../20150618.harbor.3.buildauto.sql | 2 + src/__phutil_library_map__.php | 16 ++ .../DifferentialDiffTransactionQuery.php | 10 + .../differential/storage/DifferentialDiff.php | 16 +- .../HarbormasterAutotargetsTestCase.php | 61 +++++ .../HarbormasterBuildArcanistAutoplan.php | 16 ++ .../autoplan/HarbormasterBuildAutoplan.php | 44 +++ ...masterQueryAutotargetsConduitAPIMethod.php | 76 ++++++ .../HarbormasterStepAddController.php | 10 +- .../HarbormasterStepEditController.php | 5 + .../engine/HarbormasterTargetEngine.php | 251 ++++++++++++++++++ .../query/HarbormasterBuildPlanQuery.php | 39 +++ .../HarbormasterBuildPlanSearchEngine.php | 4 + ...ormasterArcLintBuildStepImplementation.php | 38 +++ ...ormasterArcUnitBuildStepImplementation.php | 38 +++ .../HarbormasterBuildStepImplementation.php | 19 ++ .../storage/HarbormasterBuildable.php | 10 +- .../storage/build/HarbormasterBuild.php | 6 + .../storage/build/HarbormasterBuildTarget.php | 10 +- .../configuration/HarbormasterBuildPlan.php | 59 +++- .../configuration/HarbormasterBuildStep.php | 14 +- 23 files changed, 733 insertions(+), 15 deletions(-) create mode 100644 resources/sql/autopatches/20150618.harbor.1.planauto.sql create mode 100644 resources/sql/autopatches/20150618.harbor.2.stepauto.sql create mode 100644 resources/sql/autopatches/20150618.harbor.3.buildauto.sql create mode 100644 src/applications/differential/query/DifferentialDiffTransactionQuery.php create mode 100644 src/applications/harbormaster/__tests__/HarbormasterAutotargetsTestCase.php create mode 100644 src/applications/harbormaster/autoplan/HarbormasterBuildArcanistAutoplan.php create mode 100644 src/applications/harbormaster/autoplan/HarbormasterBuildAutoplan.php create mode 100644 src/applications/harbormaster/conduit/HarbormasterQueryAutotargetsConduitAPIMethod.php create mode 100644 src/applications/harbormaster/engine/HarbormasterTargetEngine.php create mode 100644 src/applications/harbormaster/step/HarbormasterArcLintBuildStepImplementation.php create mode 100644 src/applications/harbormaster/step/HarbormasterArcUnitBuildStepImplementation.php diff --git a/resources/sql/autopatches/20150618.harbor.1.planauto.sql b/resources/sql/autopatches/20150618.harbor.1.planauto.sql new file mode 100644 index 0000000000..915006824c --- /dev/null +++ b/resources/sql/autopatches/20150618.harbor.1.planauto.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_harbormaster.harbormaster_buildplan + ADD planAutoKey VARCHAR(32) COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20150618.harbor.2.stepauto.sql b/resources/sql/autopatches/20150618.harbor.2.stepauto.sql new file mode 100644 index 0000000000..6b95c9db3d --- /dev/null +++ b/resources/sql/autopatches/20150618.harbor.2.stepauto.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_harbormaster.harbormaster_buildstep + ADD stepAutoKey VARCHAR(32) COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20150618.harbor.3.buildauto.sql b/resources/sql/autopatches/20150618.harbor.3.buildauto.sql new file mode 100644 index 0000000000..660577728f --- /dev/null +++ b/resources/sql/autopatches/20150618.harbor.3.buildauto.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_harbormaster.harbormaster_build + ADD planAutoKey VARCHAR(32) COLLATE {$COLLATE_TEXT}; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index a449b84b28..fa5272bb32 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -357,6 +357,7 @@ phutil_register_library_map(array( 'DifferentialDiffTableOfContentsView' => 'applications/differential/view/DifferentialDiffTableOfContentsView.php', 'DifferentialDiffTestCase' => 'applications/differential/storage/__tests__/DifferentialDiffTestCase.php', 'DifferentialDiffTransaction' => 'applications/differential/storage/DifferentialDiffTransaction.php', + 'DifferentialDiffTransactionQuery' => 'applications/differential/query/DifferentialDiffTransactionQuery.php', 'DifferentialDiffViewController' => 'applications/differential/controller/DifferentialDiffViewController.php', 'DifferentialDoorkeeperRevisionFeedStoryPublisher' => 'applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php', 'DifferentialDraft' => 'applications/differential/storage/DifferentialDraft.php', @@ -823,11 +824,16 @@ phutil_register_library_map(array( 'FundInitiativeTransactionQuery' => 'applications/fund/query/FundInitiativeTransactionQuery.php', 'FundInitiativeViewController' => 'applications/fund/controller/FundInitiativeViewController.php', 'FundSchemaSpec' => 'applications/fund/storage/FundSchemaSpec.php', + 'HarbormasterArcLintBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterArcLintBuildStepImplementation.php', + 'HarbormasterArcUnitBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterArcUnitBuildStepImplementation.php', + 'HarbormasterAutotargetsTestCase' => 'applications/harbormaster/__tests__/HarbormasterAutotargetsTestCase.php', 'HarbormasterBuild' => 'applications/harbormaster/storage/build/HarbormasterBuild.php', 'HarbormasterBuildAbortedException' => 'applications/harbormaster/exception/HarbormasterBuildAbortedException.php', 'HarbormasterBuildActionController' => 'applications/harbormaster/controller/HarbormasterBuildActionController.php', + 'HarbormasterBuildArcanistAutoplan' => 'applications/harbormaster/autoplan/HarbormasterBuildArcanistAutoplan.php', 'HarbormasterBuildArtifact' => 'applications/harbormaster/storage/build/HarbormasterBuildArtifact.php', 'HarbormasterBuildArtifactQuery' => 'applications/harbormaster/query/HarbormasterBuildArtifactQuery.php', + 'HarbormasterBuildAutoplan' => 'applications/harbormaster/autoplan/HarbormasterBuildAutoplan.php', 'HarbormasterBuildCommand' => 'applications/harbormaster/storage/HarbormasterBuildCommand.php', 'HarbormasterBuildDependencyDatasource' => 'applications/harbormaster/typeahead/HarbormasterBuildDependencyDatasource.php', 'HarbormasterBuildEngine' => 'applications/harbormaster/engine/HarbormasterBuildEngine.php', @@ -897,6 +903,7 @@ phutil_register_library_map(array( 'HarbormasterPlanRunController' => 'applications/harbormaster/controller/HarbormasterPlanRunController.php', 'HarbormasterPlanViewController' => 'applications/harbormaster/controller/HarbormasterPlanViewController.php', 'HarbormasterPublishFragmentBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterPublishFragmentBuildStepImplementation.php', + 'HarbormasterQueryAutotargetsConduitAPIMethod' => 'applications/harbormaster/conduit/HarbormasterQueryAutotargetsConduitAPIMethod.php', 'HarbormasterQueryBuildablesConduitAPIMethod' => 'applications/harbormaster/conduit/HarbormasterQueryBuildablesConduitAPIMethod.php', 'HarbormasterQueryBuildsConduitAPIMethod' => 'applications/harbormaster/conduit/HarbormasterQueryBuildsConduitAPIMethod.php', 'HarbormasterRemarkupRule' => 'applications/harbormaster/remarkup/HarbormasterRemarkupRule.php', @@ -907,6 +914,7 @@ phutil_register_library_map(array( 'HarbormasterStepAddController' => 'applications/harbormaster/controller/HarbormasterStepAddController.php', 'HarbormasterStepDeleteController' => 'applications/harbormaster/controller/HarbormasterStepDeleteController.php', 'HarbormasterStepEditController' => 'applications/harbormaster/controller/HarbormasterStepEditController.php', + 'HarbormasterTargetEngine' => 'applications/harbormaster/engine/HarbormasterTargetEngine.php', 'HarbormasterTargetWorker' => 'applications/harbormaster/worker/HarbormasterTargetWorker.php', 'HarbormasterThrowExceptionBuildStep' => 'applications/harbormaster/step/HarbormasterThrowExceptionBuildStep.php', 'HarbormasterUIEventListener' => 'applications/harbormaster/event/HarbormasterUIEventListener.php', @@ -3704,6 +3712,7 @@ phutil_register_library_map(array( 'DifferentialDiffTableOfContentsView' => 'AphrontView', 'DifferentialDiffTestCase' => 'PhutilTestCase', 'DifferentialDiffTransaction' => 'PhabricatorApplicationTransaction', + 'DifferentialDiffTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'DifferentialDiffViewController' => 'DifferentialController', 'DifferentialDoorkeeperRevisionFeedStoryPublisher' => 'DoorkeeperFeedStoryPublisher', 'DifferentialDraft' => 'DifferentialDAO', @@ -4235,6 +4244,9 @@ phutil_register_library_map(array( 'FundInitiativeTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'FundInitiativeViewController' => 'FundController', 'FundSchemaSpec' => 'PhabricatorConfigSchemaSpec', + 'HarbormasterArcLintBuildStepImplementation' => 'HarbormasterBuildStepImplementation', + 'HarbormasterArcUnitBuildStepImplementation' => 'HarbormasterBuildStepImplementation', + 'HarbormasterAutotargetsTestCase' => 'PhabricatorTestCase', 'HarbormasterBuild' => array( 'HarbormasterDAO', 'PhabricatorApplicationTransactionInterface', @@ -4242,11 +4254,13 @@ phutil_register_library_map(array( ), 'HarbormasterBuildAbortedException' => 'Exception', 'HarbormasterBuildActionController' => 'HarbormasterController', + 'HarbormasterBuildArcanistAutoplan' => 'HarbormasterBuildAutoplan', 'HarbormasterBuildArtifact' => array( 'HarbormasterDAO', 'PhabricatorPolicyInterface', ), 'HarbormasterBuildArtifactQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'HarbormasterBuildAutoplan' => 'Phobject', 'HarbormasterBuildCommand' => 'HarbormasterDAO', 'HarbormasterBuildDependencyDatasource' => 'PhabricatorTypeaheadDatasource', 'HarbormasterBuildEngine' => 'Phobject', @@ -4342,6 +4356,7 @@ phutil_register_library_map(array( 'HarbormasterPlanRunController' => 'HarbormasterController', 'HarbormasterPlanViewController' => 'HarbormasterPlanController', 'HarbormasterPublishFragmentBuildStepImplementation' => 'HarbormasterBuildStepImplementation', + 'HarbormasterQueryAutotargetsConduitAPIMethod' => 'HarbormasterConduitAPIMethod', 'HarbormasterQueryBuildablesConduitAPIMethod' => 'HarbormasterConduitAPIMethod', 'HarbormasterQueryBuildsConduitAPIMethod' => 'HarbormasterConduitAPIMethod', 'HarbormasterRemarkupRule' => 'PhabricatorObjectRemarkupRule', @@ -4352,6 +4367,7 @@ phutil_register_library_map(array( 'HarbormasterStepAddController' => 'HarbormasterController', 'HarbormasterStepDeleteController' => 'HarbormasterController', 'HarbormasterStepEditController' => 'HarbormasterController', + 'HarbormasterTargetEngine' => 'Phobject', 'HarbormasterTargetWorker' => 'HarbormasterWorker', 'HarbormasterThrowExceptionBuildStep' => 'HarbormasterBuildStepImplementation', 'HarbormasterUIEventListener' => 'PhabricatorEventListener', diff --git a/src/applications/differential/query/DifferentialDiffTransactionQuery.php b/src/applications/differential/query/DifferentialDiffTransactionQuery.php new file mode 100644 index 0000000000..c9da0bab61 --- /dev/null +++ b/src/applications/differential/query/DifferentialDiffTransactionQuery.php @@ -0,0 +1,10 @@ +getID(); - $revision = $this->getRevision(); - $results['buildable.revision'] = $revision->getID(); - $repo = $revision->getRepository(); + if ($this->revisionID) { + $revision = $this->getRevision(); + $results['buildable.revision'] = $revision->getID(); + $repo = $revision->getRepository(); - if ($repo) { - $results['repository.callsign'] = $repo->getCallsign(); - $results['repository.vcs'] = $repo->getVersionControlSystem(); - $results['repository.uri'] = $repo->getPublicCloneURI(); + if ($repo) { + $results['repository.callsign'] = $repo->getCallsign(); + $results['repository.vcs'] = $repo->getVersionControlSystem(); + $results['repository.uri'] = $repo->getPublicCloneURI(); + } } return $results; diff --git a/src/applications/harbormaster/__tests__/HarbormasterAutotargetsTestCase.php b/src/applications/harbormaster/__tests__/HarbormasterAutotargetsTestCase.php new file mode 100644 index 0000000000..33487c0bfd --- /dev/null +++ b/src/applications/harbormaster/__tests__/HarbormasterAutotargetsTestCase.php @@ -0,0 +1,61 @@ + true, + ); + } + + public function testGenerateHarbormasterAutotargets() { + $viewer = $this->generateNewTestUser(); + + $raw_diff = <<parseDiff($raw_diff); + + $diff = DifferentialDiff::newFromRawChanges($viewer, $changes) + ->setLintStatus(DifferentialLintStatus::LINT_AUTO_SKIP) + ->setUnitStatus(DifferentialUnitStatus::UNIT_AUTO_SKIP) + ->attachRevision(null) + ->save(); + + $params = array( + 'objectPHID' => $diff->getPHID(), + 'targetKeys' => array( + HarbormasterArcLintBuildStepImplementation::STEPKEY, + HarbormasterArcUnitBuildStepImplementation::STEPKEY, + ), + ); + + // Creation of autotargets should work from an empty state. + $result = id(new ConduitCall('harbormaster.queryautotargets', $params)) + ->setUser($viewer) + ->execute(); + + $targets = idx($result, 'targetMap'); + foreach ($params['targetKeys'] as $target_key) { + $this->assertTrue((bool)$result['targetMap'][$target_key]); + } + + // Querying the same autotargets again should produce the same results, + // not make new ones. + $retry = id(new ConduitCall('harbormaster.queryautotargets', $params)) + ->setUser($viewer) + ->execute(); + + $this->assertEqual($result, $retry); + } + +} diff --git a/src/applications/harbormaster/autoplan/HarbormasterBuildArcanistAutoplan.php b/src/applications/harbormaster/autoplan/HarbormasterBuildArcanistAutoplan.php new file mode 100644 index 0000000000..7b4d5f8433 --- /dev/null +++ b/src/applications/harbormaster/autoplan/HarbormasterBuildArcanistAutoplan.php @@ -0,0 +1,16 @@ +setAncestorClass(__CLASS__) + ->loadObjects(); + + $map = array(); + foreach ($objects as $object) { + $key = $object->getAutoplanPlanKey(); + if (!empty($map[$key])) { + $other = $map[$key]; + throw new Exception( + pht( + 'Two build autoplans (of classes "%s" and "%s") define the same '. + 'key ("%s"). Each autoplan must have a unique key.', + get_class($other), + get_class($object), + $key)); + } + $map[$key] = $object; + } + + ksort($map); + + $plans = $map; + } + + return $plans; + } + +} diff --git a/src/applications/harbormaster/conduit/HarbormasterQueryAutotargetsConduitAPIMethod.php b/src/applications/harbormaster/conduit/HarbormasterQueryAutotargetsConduitAPIMethod.php new file mode 100644 index 0000000000..a00324f674 --- /dev/null +++ b/src/applications/harbormaster/conduit/HarbormasterQueryAutotargetsConduitAPIMethod.php @@ -0,0 +1,76 @@ + 'phid', + 'targetKeys' => 'list', + ); + } + + protected function defineReturnType() { + return 'map'; + } + + protected function execute(ConduitAPIRequest $request) { + $viewer = $request->getUser(); + + $phid = $request->getValue('objectPHID'); + + // NOTE: We use withNames() to let monograms like "D123" work, which makes + // this a little easier to test. Real PHIDs will still work as expected. + + $object = id(new PhabricatorObjectQuery()) + ->setViewer($viewer) + ->withNames(array($phid)) + ->executeOne(); + if (!$object) { + throw new Exception( + pht( + 'No such object "%s" exists.', + $phid)); + } + + if (!($object instanceof HarbormasterBuildableInterface)) { + throw new Exception( + pht( + 'Object "%s" does not implement interface "%s". Autotargets may '. + 'only be queried for buildable objects.', + $phid, + 'HarbormasterBuildableInterface')); + } + + $autotargets = $request->getValue('targetKeys', array()); + + if ($autotargets) { + $targets = id(new HarbormasterTargetEngine()) + ->setViewer($viewer) + ->setObject($object) + ->setAutoTargetKeys($autotargets) + ->buildTargets(); + } else { + $targets = array(); + } + + // Reorder the results according to the request order so we can make test + // assertions that subsequent calls return the same results. + + $map = mpull($targets, 'getPHID'); + $map = array_select_keys($map, $autotargets); + + return array( + 'targetMap' => $map, + ); + } + +} diff --git a/src/applications/harbormaster/controller/HarbormasterStepAddController.php b/src/applications/harbormaster/controller/HarbormasterStepAddController.php index 2d72c02b0f..371a1d1db2 100644 --- a/src/applications/harbormaster/controller/HarbormasterStepAddController.php +++ b/src/applications/harbormaster/controller/HarbormasterStepAddController.php @@ -24,10 +24,17 @@ final class HarbormasterStepAddController extends HarbormasterController { $plan_id = $plan->getID(); $cancel_uri = $this->getApplicationURI("plan/{$plan_id}/"); + $all = HarbormasterBuildStepImplementation::getImplementations(); + foreach ($all as $key => $impl) { + if ($impl->shouldRequireAutotargeting()) { + unset($all[$key]); + } + } + $errors = array(); if ($request->isFormPost()) { $class = $request->getStr('class'); - if (!HarbormasterBuildStepImplementation::getImplementation($class)) { + if (empty($all[$class])) { $errors[] = pht('Choose the type of build step you want to add.'); } if (!$errors) { @@ -39,7 +46,6 @@ final class HarbormasterStepAddController extends HarbormasterController { $control = id(new AphrontFormRadioButtonControl()) ->setName('class'); - $all = HarbormasterBuildStepImplementation::getImplementations(); foreach ($all as $class => $implementation) { $control->addButton( $class, diff --git a/src/applications/harbormaster/controller/HarbormasterStepEditController.php b/src/applications/harbormaster/controller/HarbormasterStepEditController.php index 9734f9652f..9d742740d1 100644 --- a/src/applications/harbormaster/controller/HarbormasterStepEditController.php +++ b/src/applications/harbormaster/controller/HarbormasterStepEditController.php @@ -47,6 +47,11 @@ final class HarbormasterStepEditController extends HarbormasterController { return new Aphront404Response(); } + if ($impl->shouldRequireAutotargeting()) { + // No manual creation of autotarget steps. + return new Aphront404Response(); + } + $step = HarbormasterBuildStep::initializeNewStep($viewer) ->setBuildPlanPHID($plan->getPHID()) ->setClassName($class); diff --git a/src/applications/harbormaster/engine/HarbormasterTargetEngine.php b/src/applications/harbormaster/engine/HarbormasterTargetEngine.php new file mode 100644 index 0000000000..a8403385ac --- /dev/null +++ b/src/applications/harbormaster/engine/HarbormasterTargetEngine.php @@ -0,0 +1,251 @@ +viewer = $viewer; + return $this; + } + + public function getViewer() { + return $this->viewer; + } + + public function setObject(HarbormasterBuildableInterface $object) { + $this->object = $object; + return $this; + } + + public function getObject() { + return $this->object; + } + + public function setAutoTargetKeys(array $auto_keys) { + $this->autoTargetKeys = $auto_keys; + return $this; + } + + public function getAutoTargetKeys() { + return $this->autoTargetKeys; + } + + public function buildTargets() { + $object = $this->getObject(); + $viewer = $this->getViewer(); + + $step_map = $this->generateBuildStepMap($this->getAutoTargetKeys()); + + $buildable = HarbormasterBuildable::createOrLoadExisting( + $viewer, + $object->getHarbormasterBuildablePHID(), + $object->getHarbormasterContainerPHID()); + + $target_map = $this->generateBuildTargetMap($buildable, $step_map); + + return $target_map; + } + + + /** + * Get a map of the @{class:HarbormasterBuildStep} objects for a list of + * autotarget keys. + * + * This method creates the steps if they do not yet exist. + * + * @param list Autotarget keys, like `"core.arc.lint"`. + * @return map Map of keys to step objects. + */ + private function generateBuildStepMap(array $autotargets) { + $viewer = $this->getViewer(); + + $autosteps = $this->getAutosteps($autotargets); + $autosteps = mgroup($autosteps, 'getBuildStepAutotargetPlanKey'); + + $plans = id(new HarbormasterBuildPlanQuery()) + ->setViewer($viewer) + ->withPlanAutoKeys(array_keys($autosteps)) + ->needBuildSteps(true) + ->execute(); + $plans = mpull($plans, null, 'getPlanAutoKey'); + + // NOTE: When creating the plan and steps, we save the autokeys as the + // names. These won't actually be shown in the UI, but make the data more + // consistent for secondary consumers like typeaheads. + + $step_map = array(); + foreach ($autosteps as $plan_key => $steps) { + $plan = idx($plans, $plan_key); + if (!$plan) { + $plan = HarbormasterBuildPlan::initializeNewBuildPlan($viewer) + ->setName($plan_key) + ->setPlanAutoKey($plan_key); + } + + $current = $plan->getBuildSteps(); + $current = mpull($current, null, 'getStepAutoKey'); + $new_steps = array(); + + foreach ($steps as $step_key => $step) { + if (isset($current[$step_key])) { + $step_map[$step_key] = $current[$step_key]; + continue; + } + + $new_step = HarbormasterBuildStep::initializeNewStep($viewer) + ->setName($step_key) + ->setClassName(get_class($step)) + ->setStepAutoKey($step_key); + + $new_steps[$step_key] = $new_step; + } + + if ($new_steps) { + $plan->openTransaction(); + if (!$plan->getPHID()) { + $plan->save(); + } + foreach ($new_steps as $step_key => $step) { + $step->setBuildPlanPHID($plan->getPHID()); + $step->save(); + + $step->attachBuildPlan($plan); + $step_map[$step_key] = $step; + } + $plan->saveTransaction(); + } + } + + return $step_map; + } + + + /** + * Get all of the @{class:HarbormasterBuildStepImplementation} objects for + * a list of autotarget keys. + * + * @param list Autotarget keys, like `"core.arc.lint"`. + * @return map Map of keys to implementations. + */ + private function getAutosteps(array $autotargets) { + $all_steps = HarbormasterBuildStepImplementation::getImplementations(); + $all_steps = mpull($all_steps, null, 'getBuildStepAutotargetStepKey'); + + // Make sure all the targets really exist. + foreach ($autotargets as $autotarget) { + if (empty($all_steps[$autotarget])) { + throw new Exception( + pht( + 'No build step provides autotarget "%s"!', + $autotarget)); + } + } + + return array_select_keys($all_steps, $autotargets); + } + + + /** + * Get a list of @{class:HarbormasterBuildTarget} objects for a list of + * autotarget keys. + * + * If some targets or builds do not exist, they are created. + * + * @param HarbormasterBuildable A buildable. + * @param map Map of keys to steps. + * @return map Map of keys to targets. + */ + private function generateBuildTargetMap( + HarbormasterBuildable $buildable, + array $step_map) { + + $viewer = $this->getViewer(); + $plan_map = mgroup($step_map, 'getBuildPlanPHID'); + + $builds = id(new HarbormasterBuildQuery()) + ->setViewer($viewer) + ->withBuildablePHIDs(array($buildable->getPHID())) + ->withBuildPlanPHIDs(array_keys($plan_map)) + ->needBuildTargets(true) + ->execute(); + + $autobuilds = array(); + foreach ($builds as $build) { + $plan_key = $build->getBuildPlan()->getPlanAutoKey(); + $autobuilds[$plan_key] = $build; + } + + $new_builds = array(); + foreach ($plan_map as $plan_phid => $steps) { + $plan = head($steps)->getBuildPlan(); + $plan_key = $plan->getPlanAutoKey(); + + $build = idx($autobuilds, $plan_key); + if ($build) { + // We already have a build for this set of targets, so we don't need + // to do any work. (It's possible the build is an older build that + // doesn't have all of the right targets if new autotargets were + // recently introduced, but we don't currently try to construct them.) + continue; + } + + // NOTE: Normally, `applyPlan()` does not actually generate targets. + // We need to apply the plan in-process to perform target generation. + // This is fine as long as autotargets are empty containers that don't + // do any work, which they always should be. + + PhabricatorWorker::setRunAllTasksInProcess(true); + try { + + // NOTE: We might race another process here to create the same build + // with the same `planAutoKey`. The database will prevent this and + // using autotargets only currently makes sense if you just created the + // resource and "own" it, so we don't try to handle this, but may need + // to be more careful here if use of autotargets expands. + + $build = $buildable->applyPlan($plan); + PhabricatorWorker::setRunAllTasksInProcess(false); + } catch (Exception $ex) { + PhabricatorWorker::setRunAllTasksInProcess(false); + throw $ex; + } + + $new_builds[] = $build; + } + + if ($new_builds) { + $all_targets = id(new HarbormasterBuildTargetQuery()) + ->setViewer($viewer) + ->withBuildPHIDs(mpull($new_builds, 'getPHID')) + ->execute(); + } else { + $all_targets = array(); + } + + foreach ($builds as $build) { + foreach ($build->getBuildTargets() as $target) { + $all_targets[] = $target; + } + } + + $target_map = array(); + foreach ($all_targets as $target) { + $target_key = $target + ->getImplementation() + ->getBuildStepAutotargetStepKey(); + if (!$target_key) { + continue; + } + $target_map[$target_key] = $target; + } + + $target_map = array_select_keys($target_map, array_keys($step_map)); + + return $target_map; + } + + +} diff --git a/src/applications/harbormaster/query/HarbormasterBuildPlanQuery.php b/src/applications/harbormaster/query/HarbormasterBuildPlanQuery.php index 9caf01f54e..efa04b037b 100644 --- a/src/applications/harbormaster/query/HarbormasterBuildPlanQuery.php +++ b/src/applications/harbormaster/query/HarbormasterBuildPlanQuery.php @@ -7,6 +7,8 @@ final class HarbormasterBuildPlanQuery private $phids; private $statuses; private $datasourceQuery; + private $planAutoKeys; + private $needBuildSteps; public function withIDs(array $ids) { $this->ids = $ids; @@ -28,6 +30,16 @@ final class HarbormasterBuildPlanQuery return $this; } + public function withPlanAutoKeys(array $keys) { + $this->planAutoKeys = $keys; + return $this; + } + + public function needBuildSteps($need) { + $this->needBuildSteps = $need; + return $this; + } + public function newResultObject() { return new HarbormasterBuildPlan(); } @@ -36,6 +48,26 @@ final class HarbormasterBuildPlanQuery return $this->loadStandardPage($this->newResultObject()); } + protected function didFilterPage(array $page) { + if ($this->needBuildSteps) { + $plan_phids = mpull($page, 'getPHID'); + + $steps = id(new HarbormasterBuildStepQuery()) + ->setParentQuery($this) + ->setViewer($this->getViewer()) + ->withBuildPlanPHIDs($plan_phids) + ->execute(); + $steps = mgroup($steps, 'getBuildPlanPHID'); + + foreach ($page as $plan) { + $plan_steps = idx($steps, $plan->getPHID(), array()); + $plan->attachBuildSteps($plan_steps); + } + } + + return $page; + } + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { $where = parent::buildWhereClauseParts($conn); @@ -67,6 +99,13 @@ final class HarbormasterBuildPlanQuery $this->datasourceQuery); } + if ($this->planAutoKeys !== null) { + $where[] = qsprintf( + $conn, + 'planAutoKey IN (%Ls)', + $this->planAutoKeys); + } + return $where; } diff --git a/src/applications/harbormaster/query/HarbormasterBuildPlanSearchEngine.php b/src/applications/harbormaster/query/HarbormasterBuildPlanSearchEngine.php index 7555fdc258..2fdde53dbc 100644 --- a/src/applications/harbormaster/query/HarbormasterBuildPlanSearchEngine.php +++ b/src/applications/harbormaster/query/HarbormasterBuildPlanSearchEngine.php @@ -88,6 +88,10 @@ final class HarbormasterBuildPlanSearchEngine $item->setDisabled(true); } + if ($plan->isAutoplan()) { + $item->addIcon('fa-lock grey', pht('Autoplan')); + } + $item->setHref($this->getApplicationURI("plan/{$id}/")); $list->addItem($item); diff --git a/src/applications/harbormaster/step/HarbormasterArcLintBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterArcLintBuildStepImplementation.php new file mode 100644 index 0000000000..93b6e498c7 --- /dev/null +++ b/src/applications/harbormaster/step/HarbormasterArcLintBuildStepImplementation.php @@ -0,0 +1,38 @@ +setBuildablePHID($this->getPHID()) ->setBuildPlanPHID($plan->getPHID()) - ->setBuildStatus(HarbormasterBuild::STATUS_PENDING) - ->save(); + ->setBuildStatus(HarbormasterBuild::STATUS_PENDING); + + $auto_key = $plan->getPlanAutoKey(); + if ($auto_key) { + $build->setPlanAutoKey($auto_key); + } + + $build->save(); PhabricatorWorker::scheduleTask( 'HarbormasterBuildWorker', diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuild.php b/src/applications/harbormaster/storage/build/HarbormasterBuild.php index 5ca60bb323..439780e466 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuild.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuild.php @@ -9,6 +9,7 @@ final class HarbormasterBuild extends HarbormasterDAO protected $buildPlanPHID; protected $buildStatus; protected $buildGeneration; + protected $planAutoKey; private $buildable = self::ATTACHABLE; private $buildPlan = self::ATTACHABLE; @@ -148,6 +149,7 @@ final class HarbormasterBuild extends HarbormasterDAO self::CONFIG_COLUMN_SCHEMA => array( 'buildStatus' => 'text32', 'buildGeneration' => 'uint32', + 'planAutoKey' => 'text32?', ), self::CONFIG_KEY_SCHEMA => array( 'key_buildable' => array( @@ -159,6 +161,10 @@ final class HarbormasterBuild extends HarbormasterDAO 'key_status' => array( 'columns' => array('buildStatus'), ), + 'key_planautokey' => array( + 'columns' => array('buildablePHID', 'planAutoKey'), + 'unique' => true, + ), ), ) + parent::getConfiguration(); } diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php b/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php index 3236fc50fa..b2e0a2aa83 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php @@ -175,8 +175,16 @@ final class HarbormasterBuildTarget extends HarbormasterDAO return $this->implementation; } + public function isAutotarget() { + try { + return (bool)$this->getImplementation()->getBuildStepAutotargetPlanKey(); + } catch (Exception $e) { + return false; + } + } + public function getName() { - if (strlen($this->name)) { + if (strlen($this->name) && !$this->isAutotarget()) { return $this->name; } diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php index b63927b9d9..7b15b0034c 100644 --- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php +++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php @@ -1,5 +1,8 @@ setPlanStatus(self::STATUS_ACTIVE); + ->setName('') + ->setPlanStatus(self::STATUS_ACTIVE) + ->attachBuildSteps(array()); } protected function getConfiguration() { @@ -25,6 +31,7 @@ final class HarbormasterBuildPlan extends HarbormasterDAO self::CONFIG_COLUMN_SCHEMA => array( 'name' => 'sort128', 'planStatus' => 'text32', + 'planAutoKey' => 'text32?', ), self::CONFIG_KEY_SCHEMA => array( 'key_status' => array( @@ -33,6 +40,10 @@ final class HarbormasterBuildPlan extends HarbormasterDAO 'key_name' => array( 'columns' => array('name'), ), + 'key_planautokey' => array( + 'columns' => array('planAutoKey'), + 'unique' => true, + ), ), ) + parent::getConfiguration(); } @@ -57,6 +68,33 @@ final class HarbormasterBuildPlan extends HarbormasterDAO } +/* -( Autoplans )---------------------------------------------------------- */ + + + public function isAutoplan() { + return ($this->getPlanAutoKey() !== null); + } + + + public function getAutoplan() { + if (!$this->isAutoplan()) { + return null; + } + + return HarbormasterBuildAutoplan::getAutoplan($this->getPlanAutoKey()); + } + + + public function getName() { + $autoplan = $this->getAutoplan(); + if ($autoplan) { + return $autoplan->getAutoplanName(); + } + + return parent::getName(); + } + + /* -( PhabricatorSubscribableInterface )----------------------------------- */ @@ -113,6 +151,11 @@ final class HarbormasterBuildPlan extends HarbormasterDAO case PhabricatorPolicyCapability::CAN_EDIT: // NOTE: In practice, this policy is always limited by the "Mangage // Build Plans" policy. + + if ($this->isAutoplan()) { + return PhabricatorPolicies::POLICY_NOONE; + } + return PhabricatorPolicies::getMostOpenPolicy(); } } @@ -122,7 +165,19 @@ final class HarbormasterBuildPlan extends HarbormasterDAO } public function describeAutomaticCapability($capability) { - return null; + $messages = array(); + + switch ($capability) { + case PhabricatorPolicyCapability::CAN_EDIT: + if ($this->isAutoplan()) { + $messages[] = pht( + 'This is an autoplan (a builtin plan provided by an application) '. + 'so it can not be edited.'); + } + break; + } + + return $messages; } } diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php index 90f4c33077..cdb2044cc5 100644 --- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php +++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php @@ -12,13 +12,16 @@ final class HarbormasterBuildStep extends HarbormasterDAO protected $className; protected $details = array(); protected $sequence = 0; + protected $stepAutoKey; private $buildPlan = self::ATTACHABLE; private $customFields = self::ATTACHABLE; private $implementation; public static function initializeNewStep(PhabricatorUser $actor) { - return id(new HarbormasterBuildStep()); + return id(new HarbormasterBuildStep()) + ->setName('') + ->setDescription(''); } protected function getConfiguration() { @@ -37,11 +40,16 @@ final class HarbormasterBuildStep extends HarbormasterDAO // which predated editable names. These should be backfilled with // default names, then the code for handling `null` shoudl be removed. 'name' => 'text255?', + 'stepAutoKey' => 'text32?', ), self::CONFIG_KEY_SCHEMA => array( 'key_plan' => array( 'columns' => array('buildPlanPHID'), ), + 'key_stepautokey' => array( + 'columns' => array('buildPlanPHID', 'stepAutoKey'), + 'unique' => true, + ), ), ) + parent::getConfiguration(); } @@ -88,6 +96,10 @@ final class HarbormasterBuildStep extends HarbormasterDAO return $this->implementation; } + public function isAutostep() { + return ($this->getStepAutoKey() !== null); + } + /* -( PhabricatorApplicationTransactionInterface )------------------------- */ From bc6d0478b4d3ca0ddd5a5fed9f0c97a120f8e6e8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 22 Jun 2015 11:28:15 -0700 Subject: [PATCH 04/10] Move Passphrase to SearchField Summary: Prepares for bringing spaces and a new object policy here. Test Plan: Used all search controls dozens of times. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Differential Revision: https://secure.phabricator.com/D13384 --- .../query/PassphraseCredentialQuery.php | 51 ++++++--------- .../PassphraseCredentialSearchEngine.php | 65 +++++++------------ 2 files changed, 44 insertions(+), 72 deletions(-) diff --git a/src/applications/passphrase/query/PassphraseCredentialQuery.php b/src/applications/passphrase/query/PassphraseCredentialQuery.php index 9cae8b1f85..9411fa8a77 100644 --- a/src/applications/passphrase/query/PassphraseCredentialQuery.php +++ b/src/applications/passphrase/query/PassphraseCredentialQuery.php @@ -53,19 +53,12 @@ final class PassphraseCredentialQuery return $this; } + public function newResultObject() { + return new PassphraseCredential(); + } + protected function loadPage() { - $table = new PassphraseCredential(); - $conn_r = $table->establishConnection('r'); - - $rows = queryfx_all( - $conn_r, - 'SELECT * FROM %T %Q %Q %Q', - $table->getTableName(), - $this->buildWhereClause($conn_r), - $this->buildOrderClause($conn_r), - $this->buildLimitClause($conn_r)); - - return $table->loadAllFromArray($rows); + return $this->loadStandardPage($this->newResultObject()); } protected function willFilterPage(array $page) { @@ -99,61 +92,59 @@ final class PassphraseCredentialQuery return $page; } - protected function buildWhereClause(AphrontDatabaseConnection $conn_r) { - $where = array(); + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); - $where[] = $this->buildPagingClause($conn_r); - - if ($this->ids) { + if ($this->ids !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'id IN (%Ld)', $this->ids); } - if ($this->phids) { + if ($this->phids !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'phid IN (%Ls)', $this->phids); } - if ($this->credentialTypes) { + if ($this->credentialTypes !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'credentialType in (%Ls)', $this->credentialTypes); } - if ($this->providesTypes) { + if ($this->providesTypes !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'providesType IN (%Ls)', $this->providesTypes); } if ($this->isDestroyed !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'isDestroyed = %d', (int)$this->isDestroyed); } if ($this->allowConduit !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'allowConduit = %d', (int)$this->allowConduit); } if (strlen($this->nameContains)) { $where[] = qsprintf( - $conn_r, - 'name LIKE %~', - $this->nameContains); + $conn, + 'LOWER(name) LIKE %~', + phutil_utf8_strtolower($this->nameContains)); } - return $this->formatWhereClause($where); + return $where; } public function getQueryApplicationClass() { diff --git a/src/applications/passphrase/query/PassphraseCredentialSearchEngine.php b/src/applications/passphrase/query/PassphraseCredentialSearchEngine.php index f503510e5a..be4305bcff 100644 --- a/src/applications/passphrase/query/PassphraseCredentialSearchEngine.php +++ b/src/applications/passphrase/query/PassphraseCredentialSearchEngine.php @@ -11,58 +11,39 @@ final class PassphraseCredentialSearchEngine return 'PhabricatorPassphraseApplication'; } - public function buildSavedQueryFromRequest(AphrontRequest $request) { - $saved = new PhabricatorSavedQuery(); - - $saved->setParameter( - 'isDestroyed', - $this->readBoolFromRequest($request, 'isDestroyed')); - $saved->setParameter('name', $request->getStr('name')); - - return $saved; + public function newQuery() { + return new PassphraseCredentialQuery(); } - public function buildQueryFromSavedQuery(PhabricatorSavedQuery $saved) { - $query = id(new PassphraseCredentialQuery()); + protected function buildCustomSearchFields() { + return array( + id(new PhabricatorSearchThreeStateField()) + ->setLabel(pht('Status')) + ->setKey('isDestroyed') + ->setOptions( + pht('Show All'), + pht('Show Only Destroyed Credentials'), + pht('Show Only Active Credentials')), + id(new PhabricatorSearchTextField()) + ->setLabel(pht('Name Contains')) + ->setKey('name'), + ); + } - $destroyed = $saved->getParameter('isDestroyed'); - if ($destroyed !== null) { - $query->withIsDestroyed($destroyed); + protected function buildQueryFromParameters(array $map) { + $query = $this->newQuery(); + + if ($map['isDestroyed'] !== null) { + $query->withIsDestroyed($map['isDestroyed']); } - $name = $saved->getParameter('name'); - if (strlen($name)) { - $query->withNameContains($name); + if (strlen($map['name'])) { + $query->withNameContains($map['name']); } return $query; } - public function buildSearchForm( - AphrontFormView $form, - PhabricatorSavedQuery $saved_query) { - - $name = $saved_query->getParameter('name'); - - $form - ->appendChild( - id(new AphrontFormSelectControl()) - ->setName('isDestroyed') - ->setLabel(pht('Status')) - ->setValue($this->getBoolFromQuery($saved_query, 'isDestroyed')) - ->setOptions( - array( - '' => pht('Show All Credentials'), - 'false' => pht('Show Only Active Credentials'), - 'true' => pht('Show Only Destroyed Credentials'), - ))) - ->appendChild( - id(new AphrontFormTextControl()) - ->setName('name') - ->setLabel(pht('Name Contains')) - ->setValue($name)); - } - protected function getURI($path) { return '/passphrase/'.$path; } From 85af4b01b962445d0628a4f1147eed38a52e1a3c Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 22 Jun 2015 11:28:33 -0700 Subject: [PATCH 05/10] Save authorPHID on Passphrase Credentials to support "Credential Author" object policy Summary: Fixes T5135. Currently, when you create a credential, we default the policies to your PHID. This means we can't have an application-level configurable default because there's no way to select "the actor's PHID" as a policy. Start tracking the credential author's PHID and add an object policy for it, so there is such a setting. Then, add policy defaults. This mostly unblocks T6787. This obsoletes T6860. Test Plan: - Created a credential with "Credential Author" policy. - Verified I can see/edit it, but other users can not. - Changed default policies to something else. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T5135 Differential Revision: https://secure.phabricator.com/D13385 --- .../sql/autopatches/20150621.phrase.1.sql | 2 + src/__phutil_library_map__.php | 6 +++ .../PhabricatorPassphraseApplication.php | 18 +++++++ .../PassphraseDefaultEditCapability.php | 12 +++++ .../PassphraseDefaultViewCapability.php | 16 +++++++ .../PassphraseCredentialAuthorPolicyRule.php | 48 +++++++++++++++++++ .../storage/PassphraseCredential.php | 14 +++++- 7 files changed, 114 insertions(+), 2 deletions(-) create mode 100644 resources/sql/autopatches/20150621.phrase.1.sql create mode 100644 src/applications/passphrase/capability/PassphraseDefaultEditCapability.php create mode 100644 src/applications/passphrase/capability/PassphraseDefaultViewCapability.php create mode 100644 src/applications/passphrase/policyrule/PassphraseCredentialAuthorPolicyRule.php diff --git a/resources/sql/autopatches/20150621.phrase.1.sql b/resources/sql/autopatches/20150621.phrase.1.sql new file mode 100644 index 0000000000..323b2b2208 --- /dev/null +++ b/resources/sql/autopatches/20150621.phrase.1.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_passphrase.passphrase_credential + ADD authorPHID VARBINARY(64) NOT NULL; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index fa5272bb32..61fa9d03bb 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1267,6 +1267,7 @@ phutil_register_library_map(array( 'PassphraseConduitAPIMethod' => 'applications/passphrase/conduit/PassphraseConduitAPIMethod.php', 'PassphraseController' => 'applications/passphrase/controller/PassphraseController.php', 'PassphraseCredential' => 'applications/passphrase/storage/PassphraseCredential.php', + 'PassphraseCredentialAuthorPolicyRule' => 'applications/passphrase/policyrule/PassphraseCredentialAuthorPolicyRule.php', 'PassphraseCredentialConduitController' => 'applications/passphrase/controller/PassphraseCredentialConduitController.php', 'PassphraseCredentialControl' => 'applications/passphrase/view/PassphraseCredentialControl.php', 'PassphraseCredentialCreateController' => 'applications/passphrase/controller/PassphraseCredentialCreateController.php', @@ -1286,6 +1287,8 @@ phutil_register_library_map(array( 'PassphraseCredentialTypeTestCase' => 'applications/passphrase/credentialtype/__tests__/PassphraseCredentialTypeTestCase.php', 'PassphraseCredentialViewController' => 'applications/passphrase/controller/PassphraseCredentialViewController.php', 'PassphraseDAO' => 'applications/passphrase/storage/PassphraseDAO.php', + 'PassphraseDefaultEditCapability' => 'applications/passphrase/capability/PassphraseDefaultEditCapability.php', + 'PassphraseDefaultViewCapability' => 'applications/passphrase/capability/PassphraseDefaultViewCapability.php', 'PassphraseNoteCredentialType' => 'applications/passphrase/credentialtype/PassphraseNoteCredentialType.php', 'PassphrasePasswordCredentialType' => 'applications/passphrase/credentialtype/PassphrasePasswordCredentialType.php', 'PassphrasePasswordKey' => 'applications/passphrase/keys/PassphrasePasswordKey.php', @@ -4779,6 +4782,7 @@ phutil_register_library_map(array( 'PhabricatorPolicyInterface', 'PhabricatorDestructibleInterface', ), + 'PassphraseCredentialAuthorPolicyRule' => 'PhabricatorPolicyRule', 'PassphraseCredentialConduitController' => 'PassphraseController', 'PassphraseCredentialControl' => 'AphrontFormControl', 'PassphraseCredentialCreateController' => 'PassphraseController', @@ -4798,6 +4802,8 @@ phutil_register_library_map(array( 'PassphraseCredentialTypeTestCase' => 'PhabricatorTestCase', 'PassphraseCredentialViewController' => 'PassphraseController', 'PassphraseDAO' => 'PhabricatorLiskDAO', + 'PassphraseDefaultEditCapability' => 'PhabricatorPolicyCapability', + 'PassphraseDefaultViewCapability' => 'PhabricatorPolicyCapability', 'PassphraseNoteCredentialType' => 'PassphraseCredentialType', 'PassphrasePasswordCredentialType' => 'PassphraseCredentialType', 'PassphrasePasswordKey' => 'PassphraseAbstractKey', diff --git a/src/applications/passphrase/application/PhabricatorPassphraseApplication.php b/src/applications/passphrase/application/PhabricatorPassphraseApplication.php index df21ba8be1..92e9abe9d0 100644 --- a/src/applications/passphrase/application/PhabricatorPassphraseApplication.php +++ b/src/applications/passphrase/application/PhabricatorPassphraseApplication.php @@ -63,4 +63,22 @@ final class PhabricatorPassphraseApplication extends PhabricatorApplication { ); } + protected function getCustomCapabilities() { + $policy_key = id(new PassphraseCredentialAuthorPolicyRule()) + ->getObjectPolicyFullKey(); + + return array( + PassphraseDefaultViewCapability::CAPABILITY => array( + 'caption' => pht('Default view policy for newly created credentials.'), + 'template' => PassphraseCredentialPHIDType::TYPECONST, + 'default' => $policy_key, + ), + PassphraseDefaultEditCapability::CAPABILITY => array( + 'caption' => pht('Default edit policy for newly created credentials.'), + 'template' => PassphraseCredentialPHIDType::TYPECONST, + 'default' => $policy_key, + ), + ); + } + } diff --git a/src/applications/passphrase/capability/PassphraseDefaultEditCapability.php b/src/applications/passphrase/capability/PassphraseDefaultEditCapability.php new file mode 100644 index 0000000000..0b80782698 --- /dev/null +++ b/src/applications/passphrase/capability/PassphraseDefaultEditCapability.php @@ -0,0 +1,12 @@ +getAuthorPHID(); + if (!$author_phid) { + return false; + } + + $viewer_phid = $viewer->getPHID(); + if (!$viewer_phid) { + return false; + } + + return ($viewer_phid == $author_phid); + } + + public function getValueControlType() { + return self::CONTROL_TYPE_NONE; + } + +} diff --git a/src/applications/passphrase/storage/PassphraseCredential.php b/src/applications/passphrase/storage/PassphraseCredential.php index b867323f53..b2977a9c3b 100644 --- a/src/applications/passphrase/storage/PassphraseCredential.php +++ b/src/applications/passphrase/storage/PassphraseCredential.php @@ -17,17 +17,27 @@ final class PassphraseCredential extends PassphraseDAO protected $isDestroyed; protected $isLocked = 0; protected $allowConduit = 0; + protected $authorPHID; private $secret = self::ATTACHABLE; public static function initializeNewCredential(PhabricatorUser $actor) { + $app = id(new PhabricatorApplicationQuery()) + ->setViewer($actor) + ->withClasses(array('PhabricatorPassphraseApplication')) + ->executeOne(); + + $view_policy = $app->getPolicy(PassphraseDefaultViewCapability::CAPABILITY); + $edit_policy = $app->getPolicy(PassphraseDefaultEditCapability::CAPABILITY); + return id(new PassphraseCredential()) ->setName('') ->setUsername('') ->setDescription('') ->setIsDestroyed(0) - ->setViewPolicy($actor->getPHID()) - ->setEditPolicy($actor->getPHID()); + ->setAuthorPHID($actor->getPHID()) + ->setViewPolicy($view_policy) + ->setEditPolicy($edit_policy); } public function getMonogram() { From e6b7f655ee805f8903f6e13db04506a1dc05db13 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 22 Jun 2015 11:28:54 -0700 Subject: [PATCH 06/10] Support Spaces in Passphrase Summary: Ref T8493. This stuff mostly takes care of itself now. Test Plan: Shifted stuff between spaces, verified transactions/headers showed up correctly. Queried by space. Reviewers: btrahan Reviewed By: btrahan Subscribers: joshuaspence, epriestley Maniphest Tasks: T8493 Differential Revision: https://secure.phabricator.com/D13386 --- resources/sql/autopatches/20150621.phrase.2.sql | 2 ++ src/__phutil_library_map__.php | 1 + .../PassphraseCredentialEditController.php | 11 +++++++++-- .../passphrase/storage/PassphraseCredential.php | 16 ++++++++++++++-- 4 files changed, 26 insertions(+), 4 deletions(-) create mode 100644 resources/sql/autopatches/20150621.phrase.2.sql diff --git a/resources/sql/autopatches/20150621.phrase.2.sql b/resources/sql/autopatches/20150621.phrase.2.sql new file mode 100644 index 0000000000..06fa7e2bf2 --- /dev/null +++ b/resources/sql/autopatches/20150621.phrase.2.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_passphrase.passphrase_credential + ADD spacePHID VARBINARY(64); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 61fa9d03bb..deeda11c91 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4781,6 +4781,7 @@ phutil_register_library_map(array( 'PhabricatorApplicationTransactionInterface', 'PhabricatorPolicyInterface', 'PhabricatorDestructibleInterface', + 'PhabricatorSpacesInterface', ), 'PassphraseCredentialAuthorPolicyRule' => 'PhabricatorPolicyRule', 'PassphraseCredentialConduitController' => 'PassphraseController', diff --git a/src/applications/passphrase/controller/PassphraseCredentialEditController.php b/src/applications/passphrase/controller/PassphraseCredentialEditController.php index abde525382..36033c7c01 100644 --- a/src/applications/passphrase/controller/PassphraseCredentialEditController.php +++ b/src/applications/passphrase/controller/PassphraseCredentialEditController.php @@ -60,6 +60,7 @@ final class PassphraseCredentialEditController extends PassphraseController { $e_name = true; $v_desc = $credential->getDescription(); + $v_space = $credential->getSpacePHID(); $v_username = $credential->getUsername(); $e_username = true; @@ -93,6 +94,7 @@ final class PassphraseCredentialEditController extends PassphraseController { $v_is_locked = $request->getStr('lock'); $v_secret = $request->getStr('secret'); + $v_space = $request->getStr('spacePHID'); $v_password = $request->getStr('password'); $v_decrypt = $v_secret; @@ -127,6 +129,7 @@ final class PassphraseCredentialEditController extends PassphraseController { $type_is_locked = PassphraseCredentialTransaction::TYPE_LOCK; $type_view_policy = PhabricatorTransactions::TYPE_VIEW_POLICY; $type_edit_policy = PhabricatorTransactions::TYPE_EDIT_POLICY; + $type_space = PhabricatorTransactions::TYPE_SPACE; $xactions = array(); @@ -146,6 +149,10 @@ final class PassphraseCredentialEditController extends PassphraseController { ->setTransactionType($type_edit_policy) ->setNewValue($v_edit_policy); + $xactions[] = id(new PassphraseCredentialTransaction()) + ->setTransactionType($type_space) + ->setNewValue($v_space); + // Open a transaction in case we're writing a new secret; this limits // the amount of code which handles secret plaintexts. $credential->openTransaction(); @@ -244,13 +251,13 @@ final class PassphraseCredentialEditController extends PassphraseController { ->setValue($type->getCredentialTypeName())) ->appendChild( id(new AphrontFormDividerControl())) - ->appendChild( + ->appendControl( id(new AphrontFormPolicyControl()) ->setName('viewPolicy') ->setPolicyObject($credential) ->setCapability(PhabricatorPolicyCapability::CAN_VIEW) ->setPolicies($policies)) - ->appendChild( + ->appendControl( id(new AphrontFormPolicyControl()) ->setName('editPolicy') ->setPolicyObject($credential) diff --git a/src/applications/passphrase/storage/PassphraseCredential.php b/src/applications/passphrase/storage/PassphraseCredential.php index b2977a9c3b..90eb32725d 100644 --- a/src/applications/passphrase/storage/PassphraseCredential.php +++ b/src/applications/passphrase/storage/PassphraseCredential.php @@ -4,7 +4,8 @@ final class PassphraseCredential extends PassphraseDAO implements PhabricatorApplicationTransactionInterface, PhabricatorPolicyInterface, - PhabricatorDestructibleInterface { + PhabricatorDestructibleInterface, + PhabricatorSpacesInterface { protected $name; protected $credentialType; @@ -18,6 +19,7 @@ final class PassphraseCredential extends PassphraseDAO protected $isLocked = 0; protected $allowConduit = 0; protected $authorPHID; + protected $spacePHID; private $secret = self::ATTACHABLE; @@ -37,7 +39,8 @@ final class PassphraseCredential extends PassphraseDAO ->setIsDestroyed(0) ->setAuthorPHID($actor->getPHID()) ->setViewPolicy($view_policy) - ->setEditPolicy($edit_policy); + ->setEditPolicy($edit_policy) + ->setSpacePHID($actor->getDefaultSpacePHID()); } public function getMonogram() { @@ -158,4 +161,13 @@ final class PassphraseCredential extends PassphraseDAO $this->delete(); $this->saveTransaction(); } + + +/* -( PhabricatorSpacesInterface )----------------------------------------- */ + + + public function getSpacePHID() { + return $this->spacePHID; + } + } From d1983560a6bc02035756d087b7469893f0091838 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 22 Jun 2015 11:46:59 -0700 Subject: [PATCH 07/10] Show when objects have a non-default policy Summary: Fixes T6787. I'm kind of cheating a little bit here by not unifying default selection with `initializeNew(...)` methods, but I figure we can let this settle for a bit and then go do that later. It's pretty minor. Since we're not doing templates I kind of want to swap the `'template'` key to `'type'` so maybe I'll do that too at some point. @chad, freel free to change these, I was just trying to make them pretty obvious. I //do// think it's good for them to stand out, but my approach is probably a bit inconsistent/heavy-handed in the new design. Test Plan: {F525024} {F525025} {F525026} {F525027} Reviewers: btrahan, chad Reviewed By: btrahan Subscribers: johnny-bit, joshuaspence, chad, epriestley Maniphest Tasks: T6787 Differential Revision: https://secure.phabricator.com/D13387 --- resources/celerity/map.php | 6 +- .../base/PhabricatorApplication.php | 17 +++++ .../PhabricatorCountdownApplication.php | 1 + .../PhabricatorDifferentialApplication.php | 1 + .../PhabricatorDiffusionApplication.php | 2 + .../PhabricatorDivinerApplication.php | 2 + .../PhabricatorDrydockApplication.php | 2 + .../PhabricatorFilesApplication.php | 1 + .../PhabricatorLegalpadApplication.php | 2 + .../PhabricatorManiphestApplication.php | 2 + .../PhabricatorNuanceApplication.php | 2 + .../PhabricatorPassphraseApplication.php | 2 + .../PhabricatorPasteApplication.php | 2 + .../PhabricatorPholioApplication.php | 2 + .../PhabricatorPolicyEditController.php | 4 -- .../PhabricatorPolicyExplainController.php | 65 +++++++++++++++---- .../policy/query/PhabricatorPolicyQuery.php | 41 ++++++++++++ .../PhabricatorProjectApplication.php | 3 + .../PhabricatorSlowvoteApplication.php | 1 + .../PhabricatorSpacesApplication.php | 2 + src/view/phui/PHUIHeaderView.php | 41 +++++++++++- webroot/rsrc/css/phui/phui-header-view.css | 35 ++++++++++ 22 files changed, 214 insertions(+), 22 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 6cb0832e1d..90b596da99 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -7,7 +7,7 @@ */ return array( 'names' => array( - 'core.pkg.css' => 'd7ecac6d', + 'core.pkg.css' => 'eb51e6dc', 'core.pkg.js' => 'e0117d99', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '02273347', @@ -136,7 +136,7 @@ return array( 'rsrc/css/phui/phui-fontkit.css' => 'dd8ddf27', 'rsrc/css/phui/phui-form-view.css' => '808329f2', 'rsrc/css/phui/phui-form.css' => '25876baf', - 'rsrc/css/phui/phui-header-view.css' => '2dd74fe0', + 'rsrc/css/phui/phui-header-view.css' => 'a8e1d0ac', 'rsrc/css/phui/phui-icon.css' => 'bc766998', 'rsrc/css/phui/phui-image-mask.css' => '5a8b09c8', 'rsrc/css/phui/phui-info-panel.css' => '27ea50a1', @@ -779,7 +779,7 @@ return array( 'phui-fontkit-css' => 'dd8ddf27', 'phui-form-css' => '25876baf', 'phui-form-view-css' => '808329f2', - 'phui-header-view-css' => '2dd74fe0', + 'phui-header-view-css' => 'a8e1d0ac', 'phui-icon-view-css' => 'bc766998', 'phui-image-mask-css' => '5a8b09c8', 'phui-info-panel-css' => '27ea50a1', diff --git a/src/applications/base/PhabricatorApplication.php b/src/applications/base/PhabricatorApplication.php index 1f605476bc..d91ccad4c7 100644 --- a/src/applications/base/PhabricatorApplication.php +++ b/src/applications/base/PhabricatorApplication.php @@ -606,6 +606,23 @@ abstract class PhabricatorApplication return idx($spec, 'template'); } + final public function getDefaultObjectTypePolicyMap() { + $map = array(); + + foreach ($this->getCustomCapabilities() as $capability => $spec) { + if (empty($spec['template'])) { + continue; + } + if (empty($spec['capability'])) { + continue; + } + $default = $this->getPolicy($capability); + $map[$spec['template']][$spec['capability']] = $default; + } + + return $map; + } + public function getApplicationSearchDocumentTypes() { return array(); } diff --git a/src/applications/countdown/application/PhabricatorCountdownApplication.php b/src/applications/countdown/application/PhabricatorCountdownApplication.php index 24e796a4a6..d4b99f9ed7 100644 --- a/src/applications/countdown/application/PhabricatorCountdownApplication.php +++ b/src/applications/countdown/application/PhabricatorCountdownApplication.php @@ -53,6 +53,7 @@ final class PhabricatorCountdownApplication extends PhabricatorApplication { PhabricatorCountdownDefaultViewCapability::CAPABILITY => array( 'caption' => pht('Default view policy for new countdowns.'), 'template' => PhabricatorCountdownCountdownPHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_VIEW, ), ); } diff --git a/src/applications/differential/application/PhabricatorDifferentialApplication.php b/src/applications/differential/application/PhabricatorDifferentialApplication.php index d73236668a..2cc611027f 100644 --- a/src/applications/differential/application/PhabricatorDifferentialApplication.php +++ b/src/applications/differential/application/PhabricatorDifferentialApplication.php @@ -187,6 +187,7 @@ final class PhabricatorDifferentialApplication extends PhabricatorApplication { DifferentialDefaultViewCapability::CAPABILITY => array( 'caption' => pht('Default view policy for newly created revisions.'), 'template' => DifferentialRevisionPHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_VIEW, ), ); } diff --git a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php index 271efc2a53..0314318643 100644 --- a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php +++ b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php @@ -142,10 +142,12 @@ final class PhabricatorDiffusionApplication extends PhabricatorApplication { return array( DiffusionDefaultViewCapability::CAPABILITY => array( 'template' => PhabricatorRepositoryRepositoryPHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_VIEW, ), DiffusionDefaultEditCapability::CAPABILITY => array( 'default' => PhabricatorPolicies::POLICY_ADMIN, 'template' => PhabricatorRepositoryRepositoryPHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_EDIT, ), DiffusionDefaultPushCapability::CAPABILITY => array( 'template' => PhabricatorRepositoryRepositoryPHIDType::TYPECONST, diff --git a/src/applications/diviner/application/PhabricatorDivinerApplication.php b/src/applications/diviner/application/PhabricatorDivinerApplication.php index 4d0691d990..23a1e91a7c 100644 --- a/src/applications/diviner/application/PhabricatorDivinerApplication.php +++ b/src/applications/diviner/application/PhabricatorDivinerApplication.php @@ -57,10 +57,12 @@ final class PhabricatorDivinerApplication extends PhabricatorApplication { return array( DivinerDefaultViewCapability::CAPABILITY => array( 'template' => DivinerBookPHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_VIEW, ), DivinerDefaultEditCapability::CAPABILITY => array( 'default' => PhabricatorPolicies::POLICY_ADMIN, 'template' => DivinerBookPHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_EDIT, ), ); } diff --git a/src/applications/drydock/application/PhabricatorDrydockApplication.php b/src/applications/drydock/application/PhabricatorDrydockApplication.php index 6e7b0fcfdc..7919f1c9cf 100644 --- a/src/applications/drydock/application/PhabricatorDrydockApplication.php +++ b/src/applications/drydock/application/PhabricatorDrydockApplication.php @@ -74,10 +74,12 @@ final class PhabricatorDrydockApplication extends PhabricatorApplication { return array( DrydockDefaultViewCapability::CAPABILITY => array( 'template' => DrydockBlueprintPHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_VIEW, ), DrydockDefaultEditCapability::CAPABILITY => array( 'default' => PhabricatorPolicies::POLICY_ADMIN, 'template' => DrydockBlueprintPHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_EDIT, ), DrydockCreateBlueprintsCapability::CAPABILITY => array( 'default' => PhabricatorPolicies::POLICY_ADMIN, diff --git a/src/applications/files/application/PhabricatorFilesApplication.php b/src/applications/files/application/PhabricatorFilesApplication.php index 1714719f8e..6fed230fd2 100644 --- a/src/applications/files/application/PhabricatorFilesApplication.php +++ b/src/applications/files/application/PhabricatorFilesApplication.php @@ -61,6 +61,7 @@ final class PhabricatorFilesApplication extends PhabricatorApplication { FilesDefaultViewCapability::CAPABILITY => array( 'caption' => pht('Default view policy for newly created files.'), 'template' => PhabricatorFileFilePHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_VIEW, ), ); } diff --git a/src/applications/legalpad/application/PhabricatorLegalpadApplication.php b/src/applications/legalpad/application/PhabricatorLegalpadApplication.php index e1394e0507..e47049ef30 100644 --- a/src/applications/legalpad/application/PhabricatorLegalpadApplication.php +++ b/src/applications/legalpad/application/PhabricatorLegalpadApplication.php @@ -77,9 +77,11 @@ final class PhabricatorLegalpadApplication extends PhabricatorApplication { LegalpadCreateDocumentsCapability::CAPABILITY => array(), LegalpadDefaultViewCapability::CAPABILITY => array( 'template' => PhabricatorLegalpadDocumentPHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_VIEW, ), LegalpadDefaultEditCapability::CAPABILITY => array( 'template' => PhabricatorLegalpadDocumentPHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_EDIT, ), ); } diff --git a/src/applications/maniphest/application/PhabricatorManiphestApplication.php b/src/applications/maniphest/application/PhabricatorManiphestApplication.php index 3debdb44ca..758d85f879 100644 --- a/src/applications/maniphest/application/PhabricatorManiphestApplication.php +++ b/src/applications/maniphest/application/PhabricatorManiphestApplication.php @@ -132,10 +132,12 @@ final class PhabricatorManiphestApplication extends PhabricatorApplication { ManiphestDefaultViewCapability::CAPABILITY => array( 'caption' => pht('Default view policy for newly created tasks.'), 'template' => ManiphestTaskPHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_VIEW, ), ManiphestDefaultEditCapability::CAPABILITY => array( 'caption' => pht('Default edit policy for newly created tasks.'), 'template' => ManiphestTaskPHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_EDIT, ), ManiphestEditStatusCapability::CAPABILITY => array(), ManiphestEditAssignCapability::CAPABILITY => array(), diff --git a/src/applications/nuance/application/PhabricatorNuanceApplication.php b/src/applications/nuance/application/PhabricatorNuanceApplication.php index 20c58f841c..2d82f3f0b0 100644 --- a/src/applications/nuance/application/PhabricatorNuanceApplication.php +++ b/src/applications/nuance/application/PhabricatorNuanceApplication.php @@ -73,10 +73,12 @@ final class PhabricatorNuanceApplication extends PhabricatorApplication { NuanceSourceDefaultViewCapability::CAPABILITY => array( 'caption' => pht('Default view policy for newly created sources.'), 'template' => NuanceSourcePHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_VIEW, ), NuanceSourceDefaultEditCapability::CAPABILITY => array( 'caption' => pht('Default edit policy for newly created sources.'), 'template' => NuanceSourcePHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_EDIT, ), NuanceSourceManageCapability::CAPABILITY => array(), ); diff --git a/src/applications/passphrase/application/PhabricatorPassphraseApplication.php b/src/applications/passphrase/application/PhabricatorPassphraseApplication.php index 92e9abe9d0..daf794180f 100644 --- a/src/applications/passphrase/application/PhabricatorPassphraseApplication.php +++ b/src/applications/passphrase/application/PhabricatorPassphraseApplication.php @@ -71,11 +71,13 @@ final class PhabricatorPassphraseApplication extends PhabricatorApplication { PassphraseDefaultViewCapability::CAPABILITY => array( 'caption' => pht('Default view policy for newly created credentials.'), 'template' => PassphraseCredentialPHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_VIEW, 'default' => $policy_key, ), PassphraseDefaultEditCapability::CAPABILITY => array( 'caption' => pht('Default edit policy for newly created credentials.'), 'template' => PassphraseCredentialPHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_EDIT, 'default' => $policy_key, ), ); diff --git a/src/applications/paste/application/PhabricatorPasteApplication.php b/src/applications/paste/application/PhabricatorPasteApplication.php index f9cd03dd74..a3f11bb9a8 100644 --- a/src/applications/paste/application/PhabricatorPasteApplication.php +++ b/src/applications/paste/application/PhabricatorPasteApplication.php @@ -65,10 +65,12 @@ final class PhabricatorPasteApplication extends PhabricatorApplication { PasteDefaultViewCapability::CAPABILITY => array( 'caption' => pht('Default view policy for newly created pastes.'), 'template' => PhabricatorPastePastePHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_VIEW, ), PasteDefaultEditCapability::CAPABILITY => array( 'caption' => pht('Default edit policy for newly created pastes.'), 'template' => PhabricatorPastePastePHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_EDIT, ), ); } diff --git a/src/applications/pholio/application/PhabricatorPholioApplication.php b/src/applications/pholio/application/PhabricatorPholioApplication.php index 52309ba078..e019adf04b 100644 --- a/src/applications/pholio/application/PhabricatorPholioApplication.php +++ b/src/applications/pholio/application/PhabricatorPholioApplication.php @@ -73,9 +73,11 @@ final class PhabricatorPholioApplication extends PhabricatorApplication { return array( PholioDefaultViewCapability::CAPABILITY => array( 'template' => PholioMockPHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_VIEW, ), PholioDefaultEditCapability::CAPABILITY => array( 'template' => PholioMockPHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_EDIT, ), ); } diff --git a/src/applications/policy/controller/PhabricatorPolicyEditController.php b/src/applications/policy/controller/PhabricatorPolicyEditController.php index 30c9836557..abd9c5b41b 100644 --- a/src/applications/policy/controller/PhabricatorPolicyEditController.php +++ b/src/applications/policy/controller/PhabricatorPolicyEditController.php @@ -6,9 +6,6 @@ final class PhabricatorPolicyEditController public function handleRequest(AphrontRequest $request) { $viewer = $this->getViewer(); - // TODO: This doesn't do anything yet, but sets up template policies; see - // T6860. - $is_template = false; $object_phid = $request->getURIData('objectPHID'); if ($object_phid) { @@ -23,7 +20,6 @@ final class PhabricatorPolicyEditController $object_type = $request->getURIData('objectType'); if (!$object_type) { $object_type = $request->getURIData('templateType'); - $is_template = true; } $phid_types = PhabricatorPHIDType::getAllInstalledTypes($viewer); diff --git a/src/applications/policy/controller/PhabricatorPolicyExplainController.php b/src/applications/policy/controller/PhabricatorPolicyExplainController.php index 46dfad3f57..2639c967d4 100644 --- a/src/applications/policy/controller/PhabricatorPolicyExplainController.php +++ b/src/applications/policy/controller/PhabricatorPolicyExplainController.php @@ -3,24 +3,15 @@ final class PhabricatorPolicyExplainController extends PhabricatorPolicyController { - private $phid; - private $capability; - public function shouldAllowPublic() { return true; } - public function willProcessRequest(array $data) { - $this->phid = $data['phid']; - $this->capability = $data['capability']; - } + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); - - $phid = $this->phid; - $capability = $this->capability; + $phid = $request->getURIData('phid'); + $capability = $request->getURIData('capability'); $object = id(new PhabricatorObjectQuery()) ->setViewer($viewer) @@ -84,11 +75,15 @@ final class PhabricatorPolicyExplainController $handle->getTypeName(), $handle->getObjectName()); - return $dialog + $dialog ->setTitle(pht('Policy Details: %s', $object_name)) ->appendParagraph($intro) ->appendChild($auto_info) ->addCancelButton($object_uri, pht('Done')); + + $this->appendStrengthInformation($dialog, $object, $policy, $capability); + + return $dialog; } private function appendSpaceInformation( @@ -180,4 +175,46 @@ final class PhabricatorPolicyExplainController 'object policy checks.')); } + private function appendStrengthInformation( + AphrontDialogView $dialog, + PhabricatorPolicyInterface $object, + PhabricatorPolicy $policy, + $capability) { + $viewer = $this->getViewer(); + + $default_policy = PhabricatorPolicyQuery::getDefaultPolicyForObject( + $viewer, + $object, + $capability); + if (!$default_policy) { + return; + } + + if ($default_policy->getPHID() == $policy->getPHID()) { + return; + } + + if ($default_policy->isStrongerThan($policy)) { + $info = pht( + 'This object has a less restrictive policy ("%s") than the default '. + 'policy for similar objects (which is "%s").', + $policy->getShortName(), + $default_policy->getShortName()); + } else if ($policy->isStrongerThan($default_policy)) { + $info = pht( + 'This object has a more restrictive policy ("%s") than the default '. + 'policy for similar objects (which is "%s").', + $policy->getShortName(), + $default_policy->getShortName()); + } else { + $info = pht( + 'This object has a different policy ("%s") than the default policy '. + 'for similar objects (which is "%s").', + $policy->getShortName(), + $default_policy->getShortName()); + } + + $dialog->appendParagraph($info); + } + } diff --git a/src/applications/policy/query/PhabricatorPolicyQuery.php b/src/applications/policy/query/PhabricatorPolicyQuery.php index 496c704b71..daa7012223 100644 --- a/src/applications/policy/query/PhabricatorPolicyQuery.php +++ b/src/applications/policy/query/PhabricatorPolicyQuery.php @@ -342,5 +342,46 @@ final class PhabricatorPolicyQuery return $results; } + public static function getDefaultPolicyForObject( + PhabricatorUser $viewer, + PhabricatorPolicyInterface $object, + $capability) { + + $phid = $object->getPHID(); + if (!$phid) { + return null; + } + + $type = phid_get_type($phid); + + $map = self::getDefaultObjectTypePolicyMap(); + + if (empty($map[$type][$capability])) { + return null; + } + + $policy_phid = $map[$type][$capability]; + + return id(new PhabricatorPolicyQuery()) + ->setViewer($viewer) + ->withPHIDs(array($policy_phid)) + ->executeOne(); + } + + private static function getDefaultObjectTypePolicyMap() { + static $map; + + if ($map === null) { + $map = array(); + + $apps = PhabricatorApplication::getAllApplications(); + foreach ($apps as $app) { + $map += $app->getDefaultObjectTypePolicyMap(); + } + } + + return $map; + } + } diff --git a/src/applications/project/application/PhabricatorProjectApplication.php b/src/applications/project/application/PhabricatorProjectApplication.php index adf6378e2a..0a2ae190d9 100644 --- a/src/applications/project/application/PhabricatorProjectApplication.php +++ b/src/applications/project/application/PhabricatorProjectApplication.php @@ -121,14 +121,17 @@ final class PhabricatorProjectApplication extends PhabricatorApplication { ProjectDefaultViewCapability::CAPABILITY => array( 'caption' => pht('Default view policy for newly created projects.'), 'template' => PhabricatorProjectProjectPHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_VIEW, ), ProjectDefaultEditCapability::CAPABILITY => array( 'caption' => pht('Default edit policy for newly created projects.'), 'template' => PhabricatorProjectProjectPHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_EDIT, ), ProjectDefaultJoinCapability::CAPABILITY => array( 'caption' => pht('Default join policy for newly created projects.'), 'template' => PhabricatorProjectProjectPHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_JOIN, ), ); } diff --git a/src/applications/slowvote/application/PhabricatorSlowvoteApplication.php b/src/applications/slowvote/application/PhabricatorSlowvoteApplication.php index 40791659b1..e7ea30dfed 100644 --- a/src/applications/slowvote/application/PhabricatorSlowvoteApplication.php +++ b/src/applications/slowvote/application/PhabricatorSlowvoteApplication.php @@ -65,6 +65,7 @@ final class PhabricatorSlowvoteApplication extends PhabricatorApplication { PhabricatorSlowvoteDefaultViewCapability::CAPABILITY => array( 'caption' => pht('Default view policy for new polls.'), 'template' => PhabricatorSlowvotePollPHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_VIEW, ), ); } diff --git a/src/applications/spaces/application/PhabricatorSpacesApplication.php b/src/applications/spaces/application/PhabricatorSpacesApplication.php index 7942caf919..2f4ba394d1 100644 --- a/src/applications/spaces/application/PhabricatorSpacesApplication.php +++ b/src/applications/spaces/application/PhabricatorSpacesApplication.php @@ -74,11 +74,13 @@ final class PhabricatorSpacesApplication extends PhabricatorApplication { PhabricatorSpacesCapabilityDefaultView::CAPABILITY => array( 'caption' => pht('Default view policy for newly created spaces.'), 'template' => PhabricatorSpacesNamespacePHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_VIEW, ), PhabricatorSpacesCapabilityDefaultEdit::CAPABILITY => array( 'caption' => pht('Default edit policy for newly created spaces.'), 'default' => PhabricatorPolicies::POLICY_ADMIN, 'template' => PhabricatorSpacesNamespacePHIDType::TYPECONST, + 'capability' => PhabricatorPolicyCapability::CAN_EDIT, ), ); } diff --git a/src/view/phui/PHUIHeaderView.php b/src/view/phui/PHUIHeaderView.php index 100ee93d4b..0557424f7b 100644 --- a/src/view/phui/PHUIHeaderView.php +++ b/src/view/phui/PHUIHeaderView.php @@ -292,6 +292,7 @@ final class PHUIHeaderView extends AphrontTagView { // NOTE: We'll do this even if the viewer has access to only one space, and // show them information about the existence of spaces if they click // through. + $use_space_policy = false; if ($object instanceof PhabricatorSpacesInterface) { $space_phid = PhabricatorSpacesNamespaceQuery::getObjectSpacePHID( $object); @@ -306,13 +307,46 @@ final class PHUIHeaderView extends AphrontTagView { if ($space_policy) { if ($space_policy->isStrongerThan($policy)) { $policy = $space_policy; + $use_space_policy = true; } } } } + $container_classes = array(); + $container_classes[] = 'policy-header-callout'; $phid = $object->getPHID(); + // If we're going to show the object policy, try to determine if the object + // policy differs from the default policy. If it does, we'll call it out + // as changed. + if (!$use_space_policy) { + $default_policy = PhabricatorPolicyQuery::getDefaultPolicyForObject( + $viewer, + $object, + $view_capability); + if ($default_policy) { + if ($default_policy->getPHID() != $policy->getPHID()) { + $container_classes[] = 'policy-adjusted'; + if ($default_policy->isStrongerThan($policy)) { + // The policy has strictly been weakened. For example, the + // default might be "All Users" and the current policy is "Public". + $container_classes[] = 'policy-adjusted-weaker'; + } else if ($policy->isStrongerThan($default_policy)) { + // The policy has strictly been strengthened, and is now more + // restrictive than the default. For example, "All Users" has + // been replaced with "No One". + $container_classes[] = 'policy-adjusted-stronger'; + } else { + // The policy has been adjusted but not strictly strengthened + // or weakened. For example, "Members of X" has been replaced with + // "Members of Y". + $container_classes[] = 'policy-adjusted-different'; + } + } + } + } + $icon = id(new PHUIIconView()) ->setIconFont($policy->getIcon().' bluegrey'); @@ -325,7 +359,12 @@ final class PHUIHeaderView extends AphrontTagView { ), $policy->getShortName()); - return array($icon, $link); + return phutil_tag( + 'span', + array( + 'class' => implode(' ', $container_classes), + ), + array($icon, $link)); } } diff --git a/webroot/rsrc/css/phui/phui-header-view.css b/webroot/rsrc/css/phui/phui-header-view.css index 842441dae0..6a80aaa8c3 100644 --- a/webroot/rsrc/css/phui/phui-header-view.css +++ b/webroot/rsrc/css/phui/phui-header-view.css @@ -116,6 +116,41 @@ body.device-phone .phui-header-view { color: {$darkbluetext}; } +.policy-header-callout.policy-adjusted { + padding: 0 4px; + border-radius: 3px; +} + +.policy-header-callout.policy-adjusted-weaker { + background: {$lightgreen}; + border: 1px solid {$green}; +} + +.policy-header-callout.policy-adjusted-weaker .policy-link, +.policy-header-callout.policy-adjusted-weaker .phui-icon-view { + color: {$green}; +} + +.policy-header-callout.policy-adjusted-stronger { + background: {$lightred}; + border: 1px solid {$red}; +} + +.policy-header-callout.policy-adjusted-stronger .policy-link, +.policy-header-callout.policy-adjusted-stronger .phui-icon-view { + color: {$red}; +} + +.policy-header-callout.policy-adjusted-different { + background: {$lightorange}; + border: 1px solid {$orange}; +} + +.policy-header-callout.policy-adjusted-different .policy-link, +.policy-header-callout.policy-adjusted-different .phui-icon-view { + color: {$orange}; +} + .phui-header-subheader .phui-header-status-dark { color: {$indigo}; text-shadow: 0 1px #fff; From 0597aba33ec18502843b50dac7bb558bba337f37 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 22 Jun 2015 11:47:58 -0700 Subject: [PATCH 08/10] Add hard stops on empty batch edit sets Summary: Ref T8637. If a user tries to batch edit a list of tasks which can't be edited, we fall through to `withIDs(array())`, which can affect //everything//. Explicitly stop batch editing if we don't have valid IDs or valid tasks. The UI sort-of warns you that something is wrong, but this is ultimately a pretty severe UX issue. I'll fix the underlying Query in the next diff. Test Plan: Tried to batch edit a list of tasks I didn't have permission to edit. Reviewers: btrahan Reviewed By: btrahan Subscribers: lloyd.oliver, epriestley Maniphest Tasks: T8637 Differential Revision: https://secure.phabricator.com/D13388 --- .../controller/ManiphestBatchEditController.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/applications/maniphest/controller/ManiphestBatchEditController.php b/src/applications/maniphest/controller/ManiphestBatchEditController.php index 2859eaae57..e4086b2d18 100644 --- a/src/applications/maniphest/controller/ManiphestBatchEditController.php +++ b/src/applications/maniphest/controller/ManiphestBatchEditController.php @@ -25,6 +25,12 @@ final class ManiphestBatchEditController extends ManiphestController { $task_ids = $request->getStrList('batch'); } + if (!$task_ids) { + throw new Exception( + pht( + 'No tasks are selected.')); + } + $tasks = id(new ManiphestTaskQuery()) ->setViewer($viewer) ->withIDs($task_ids) @@ -37,6 +43,12 @@ final class ManiphestBatchEditController extends ManiphestController { ->needProjectPHIDs(true) ->execute(); + if (!$tasks) { + throw new Exception( + pht( + "You don't have permission to edit any of the selected tasks.")); + } + if ($project) { $cancel_uri = '/project/board/'.$project->getID().'/'; $redirect_uri = $cancel_uri; From 487e12101abaa2362ba77ea087a0f25e52c202c8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 22 Jun 2015 11:54:51 -0700 Subject: [PATCH 09/10] Make ManiphestTaskQuery more modern and safe Summary: Ref T8637. This class has some really old parameter handling which can send `withIDs(array())` down a "fetch everything" pathway. Clean up most of it. Test Plan: Issued every ApplicationSearch query. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T8637 Differential Revision: https://secure.phabricator.com/D13390 --- .../maniphest/query/ManiphestTaskQuery.php | 208 ++++++++---------- 1 file changed, 96 insertions(+), 112 deletions(-) diff --git a/src/applications/maniphest/query/ManiphestTaskQuery.php b/src/applications/maniphest/query/ManiphestTaskQuery.php index 841a917312..b104ec5dab 100644 --- a/src/applications/maniphest/query/ManiphestTaskQuery.php +++ b/src/applications/maniphest/query/ManiphestTaskQuery.php @@ -6,13 +6,13 @@ */ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { - private $taskIDs = array(); - private $taskPHIDs = array(); - private $authorPHIDs = array(); - private $ownerPHIDs = array(); + private $taskIDs; + private $taskPHIDs; + private $authorPHIDs; + private $ownerPHIDs; private $noOwner; private $anyOwner; - private $subscriberPHIDs = array(); + private $subscriberPHIDs; private $dateCreatedAfter; private $dateCreatedBefore; private $dateModifiedAfter; @@ -216,75 +216,7 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { $task_dao = new ManiphestTask(); $conn = $task_dao->establishConnection('r'); - $where = array(); - $where[] = $this->buildTaskIDsWhereClause($conn); - $where[] = $this->buildTaskPHIDsWhereClause($conn); - $where[] = $this->buildStatusWhereClause($conn); - $where[] = $this->buildStatusesWhereClause($conn); - $where[] = $this->buildDependenciesWhereClause($conn); - $where[] = $this->buildAuthorWhereClause($conn); - $where[] = $this->buildOwnerWhereClause($conn); - $where[] = $this->buildFullTextWhereClause($conn); - - if ($this->dateCreatedAfter) { - $where[] = qsprintf( - $conn, - 'task.dateCreated >= %d', - $this->dateCreatedAfter); - } - - if ($this->dateCreatedBefore) { - $where[] = qsprintf( - $conn, - 'task.dateCreated <= %d', - $this->dateCreatedBefore); - } - - if ($this->dateModifiedAfter) { - $where[] = qsprintf( - $conn, - 'task.dateModified >= %d', - $this->dateModifiedAfter); - } - - if ($this->dateModifiedBefore) { - $where[] = qsprintf( - $conn, - 'task.dateModified <= %d', - $this->dateModifiedBefore); - } - - if ($this->priorities) { - $where[] = qsprintf( - $conn, - 'task.priority IN (%Ld)', - $this->priorities); - } - - if ($this->subpriorities) { - $where[] = qsprintf( - $conn, - 'task.subpriority IN (%Lf)', - $this->subpriorities); - } - - if ($this->subpriorityMin) { - $where[] = qsprintf( - $conn, - 'task.subpriority >= %f', - $this->subpriorityMin); - } - - if ($this->subpriorityMax) { - $where[] = qsprintf( - $conn, - 'task.subpriority <= %f', - $this->subpriorityMax); - } - - $where[] = $this->buildWhereClauseParts($conn); - - $where = $this->formatWhereClause($where); + $where = $this->buildWhereClause($conn); $group_column = ''; switch ($this->groupBy) { @@ -392,26 +324,99 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { return $tasks; } - private function buildTaskIDsWhereClause(AphrontDatabaseConnection $conn) { - if (!$this->taskIDs) { - return null; + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); + + $where[] = $this->buildStatusWhereClause($conn); + $where[] = $this->buildDependenciesWhereClause($conn); + $where[] = $this->buildOwnerWhereClause($conn); + $where[] = $this->buildFullTextWhereClause($conn); + + if ($this->taskIDs !== null) { + $where[] = qsprintf( + $conn, + 'task.id in (%Ld)', + $this->taskIDs); } - return qsprintf( - $conn, - 'task.id in (%Ld)', - $this->taskIDs); - } - - private function buildTaskPHIDsWhereClause(AphrontDatabaseConnection $conn) { - if (!$this->taskPHIDs) { - return null; + if ($this->taskPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'task.phid in (%Ls)', + $this->taskPHIDs); } - return qsprintf( - $conn, - 'task.phid in (%Ls)', - $this->taskPHIDs); + if ($this->statuses !== null) { + $where[] = qsprintf( + $conn, + 'task.status IN (%Ls)', + $this->statuses); + } + + if ($this->authorPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'task.authorPHID in (%Ls)', + $this->authorPHIDs); + } + + if ($this->dateCreatedAfter) { + $where[] = qsprintf( + $conn, + 'task.dateCreated >= %d', + $this->dateCreatedAfter); + } + + if ($this->dateCreatedBefore) { + $where[] = qsprintf( + $conn, + 'task.dateCreated <= %d', + $this->dateCreatedBefore); + } + + if ($this->dateModifiedAfter) { + $where[] = qsprintf( + $conn, + 'task.dateModified >= %d', + $this->dateModifiedAfter); + } + + if ($this->dateModifiedBefore) { + $where[] = qsprintf( + $conn, + 'task.dateModified <= %d', + $this->dateModifiedBefore); + } + + if ($this->priorities !== null) { + $where[] = qsprintf( + $conn, + 'task.priority IN (%Ld)', + $this->priorities); + } + + if ($this->subpriorities !== null) { + $where[] = qsprintf( + $conn, + 'task.subpriority IN (%Lf)', + $this->subpriorities); + } + + if ($this->subpriorityMin !== null) { + $where[] = qsprintf( + $conn, + 'task.subpriority >= %f', + $this->subpriorityMin); + } + + if ($this->subpriorityMax !== null) { + $where[] = qsprintf( + $conn, + 'task.subpriority <= %f', + $this->subpriorityMax); + } + + return $where; } private function buildStatusWhereClause(AphrontDatabaseConnection $conn) { @@ -448,27 +453,6 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { } } - private function buildStatusesWhereClause(AphrontDatabaseConnection $conn) { - if ($this->statuses) { - return qsprintf( - $conn, - 'task.status IN (%Ls)', - $this->statuses); - } - return null; - } - - private function buildAuthorWhereClause(AphrontDatabaseConnection $conn) { - if (!$this->authorPHIDs) { - return null; - } - - return qsprintf( - $conn, - 'task.authorPHID in (%Ls)', - $this->authorPHIDs); - } - private function buildOwnerWhereClause(AphrontDatabaseConnection $conn) { $subclause = array(); @@ -590,7 +574,7 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { id(new ManiphestTask())->getTableName()); } - if ($this->subscriberPHIDs) { + if ($this->subscriberPHIDs !== null) { $joins[] = qsprintf( $conn_r, 'JOIN %T e_ccs ON e_ccs.src = task.phid '. From 95fe4f94511f579f3aec12b554b414f325834900 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 22 Jun 2015 12:26:18 -0700 Subject: [PATCH 10/10] Add a repository import troubleshooting guide Summary: Fixes T8615. Basic overview of the pipeline, where to look for errors, and how to identify/understand/fix/force your way through. Test Plan: Read documentation. Reviewers: btrahan, joshuaspence Reviewed By: joshuaspence Subscribers: epriestley Maniphest Tasks: T8615 Differential Revision: https://secure.phabricator.com/D13376 --- .../user/field/repository_imports.diviner | 247 ++++++++++++++++++ src/docs/user/userguide/diffusion.diviner | 22 +- .../userguide/diffusion_autoclose.diviner | 8 + .../user/userguide/diffusion_updates.diviner | 8 + 4 files changed, 281 insertions(+), 4 deletions(-) create mode 100644 src/docs/user/field/repository_imports.diviner diff --git a/src/docs/user/field/repository_imports.diviner b/src/docs/user/field/repository_imports.diviner new file mode 100644 index 0000000000..a59bee051f --- /dev/null +++ b/src/docs/user/field/repository_imports.diviner @@ -0,0 +1,247 @@ +@title Troubleshooting Repository Imports +@group fieldmanual + +Guide to the troubleshooting repositories which import incompletely. + +Overview +======== + +When you first import an external source code repository (or push new commits to +a hosted repository), Phabricator imports those commits in the background. + +While a repository is initially importing, some features won't work. While +individual commits are importing, some of their metadata won't be available in +the web UI. + +Sometimes, the import process may hang or fail to complete. This document can +help you understand the import process and troubleshoot problems with it. + + +Understanding the Import Pipeline +================================= + +Phabricator first performs commit discovery on repositories. This examines +a repository and identifies all the commits in it at a very shallow level, +then creates stub objects for them. These stub objects primarily serve to +assign various internal IDs to each commit. + +Commit discovery occurs in the update phase, and you can learn more about +updates in @{article:Diffusion User Guide: Repository Updates}. + +After commits are discovered, background tasks are queued to actually import +commits. These tasks do things like look at commit messages, trigger mentions +and autoclose rules, cache changes, trigger Herald, publish feed stories and +email, and apply Owners rules. You can learn more about some of these steps in +@{article:Diffusion User Guide: Autoclose}. + +Specifically, the import pipeline has four steps: + + - **Message**: Parses the commit message and author metadata. + - **Change**: Caches the paths the commit affected. + - **Owners**: Runs Owners rules. + - **Herald**: Runs Herald rules and publishes notifications. + +These steps run in sequence for each commit, but all discovered commits import +in parallel. + + +Identifying Missing Steps +========================= + +There are a few major pieces of information you can look at to understand where +the import process is stuck. + +First, to identify which commits have missing import steps, run this command: + +``` +phabricator/ $ ./bin/repository importing rXYZ +``` + +That will show what work remains to be done. Each line shows a commit which +is discovered but not imported, and the import steps that are remaining for +that commit. Generally, the commit is stuck on the first step in the list. + +Second, load the Daemon Console (at `/daemon/` in the web UI). This will show +what work is currently being done and waiting to complete. The most important +sections are "Queued Tasks" (work waiting in queue) and "Leased Tasks" +(work currently being done). + +Third, run this command to look at the daemon logs: + +``` +phabricator/ $ ./bin/phd log +``` + +This can show you any errors the daemons have encountered recently. + +The next sections will walk through how to use this information to understand +and resolve the issue. + + +Handling Permanent Failures +=========================== + +Some commits can not be imported, which will permanently stop a repository from +fully importing. These are rare, but can be caused by unusual data in a +repository, version peculiarities, or bugs in the importer. + +Permanent failures usually look like a small number of commits stuck on the +"Message" or "Change" steps in the output of `repository importing`. If you +have a larger number of commits, it is less likely that there are any permanent +problems. + +In the Daemon console, permanent failures usually look like a small number of +tasks in "Leased Tasks" with a large failure count. These tasks are retrying +until they succeed, but a bug is permanently preventing them from succeeding, +so they'll rack up a large number of retries over time. + +In the daemon log, these commits usually emit specific errors showing why +they're failing to import. + +These failures are the easiest to identify and understand, and can often be +resolved quickly. Choose some failing commit from the output of `bin/repository +importing` and use this command to re-run any missing steps manually in the +foreground: + +``` +phabricator/ $ ./bin/repository reparse --importing --trace rXYZabcdef012... +``` + +This command is always safe to run, no matter what the actual root cause of +the problem is. + +If this fails with an error, you've likely identified a problem with +Phabricator. Collect as much information as you can about what makes the commit +special and file a bug in the upstream by following the instructions in +@{article:Contributing Bug Reports}. + +If the commit imports cleanly, this is more likely to be caused by some other +issue. + + +Handling Temporary Failures +=========================== + +Some commits may temporarily fail to import: perhaps the network or services +may have briefly been down, or some configuration wasn't quite right, or the +daemons were killed halfway through the work. + +These commits will retry eventually and usually succeed, but some of the retry +time limits are very conserative (up to 24 hours) and you might not want to +wait that long. + +In the Daemon console, temporarily failures usually look like tasks in the +"Leased Tasks" column with a large "Expires" value but a low "Failures" count +(usually 0 or 1). The "Expires" column is showing how long Phabricator is +waiting to retry these tasks. + +In the daemon log, these temporary failures might have created log entries, but +might also not have. For example, if the failure was rooted in a network issue +that probably will create a log entry, but if the faiulre was rooted in the +daemons being abruptly killed that may not create a log entry. + +You can follow the instructions from "Handling Permanent Failures" above to +reparse steps individually to look for an error that represents a root cause, +but sometimes this can happen because of some transient issue which won't be +identifiable. + +The easiest way to fix this is to restart the daemons. When you restart +daemons, all task leases are immediately expired, so any tasks waiting for a +long time will run right away: + +``` +phabricator/ $ ./bin/phd restart +``` + +This command is always safe to run, no matter what the actual root cause of +the problem is. + +After restarting the daemons, any pending tasks should be able to retry +immediately. + +For more information on managing the daemons, see +@{article:Managing Daemons with phd}. + + +Forced Parsing +============== + +In rare cases, the actual tasks may be lost from the task queue. Usually, they +have been stolen by gremlins or spritied away by ghosts, or someone may have +been too ambitious with running manual SQL commands and deleted a bunch of +extra things they shouldn't have. + +There is no normal set of conditions under which this should occur, but you can +force Phabricator to re-queue the tasks to recover from it if it does occur. + +This will look like missing steps in `repository importing`, but nothing in the +"Queued Tasks" or "Leased Tasks" sections of the daemon console. The daemon +logs will also be empty, since the tasks have vanished. + +To re-queue parse tasks for a repository, run this command, which will queue +up all of the missing work in `repository importing`: + +``` +phabricator/ $ ./bin/repository reparse --importing --all rXYZ +``` + +This command may cause duplicate work to occur if you have misdiagnosed the +problem and the tasks aren't actually lost. For example, it could queue a +second task to perform publishing, which could cause Phabricator to send a +second copy of email about the commit. Other than that, it is safe to run even +if this isn't the problem. + +After running this command, you should see tasks in "Queued Tasks" and "Leased +Tasks" in the console which correspond to the commits in `repository +importing`, and progress should resume. + + +Forced Imports +============== + +In some cases, you might want to force a repository to be flagged as imported +even though the import isn't complete. The most common and reasonable case +where you might want to do this is if you've identified a permanent failure +with a small number of commits (maybe just one) and reported it upstream, and +are waiting for a fix. You might want to start using the repository immediately, +even if a few things can't import yet. + +You should be cautious about doing this. The "importing" flag controls +publishing of notifications and email, so if you flag a repository as imported +but it still has a lot of work queued, it may send an enormous amount of email +as that work completes. + +To mark a repository as imported even though it really isn't, run this +command: + +``` +phabricator/ $ ./bin/repository mark-imported rXYZ +``` + +If you do this by mistake, you can reverse it later by using the +`--mark-not-imported` flag. + + +General Tips +============ + +Broadly, `bin/repository` contains several useful debugging commands which +let you figure out where failures are occuring. You can add the `--trace` flag +to any command to get more details about what it is doing. For any command, +you can use `help` to learn more about what it does and which flag it takes: + +``` +phabricator/ $ bin/repository help +``` + +In particular, you can use flags with the the `repository reparse` command to +manually run parse steps in the foreground, including re-running steps and +running steps out of order. + + +Next Steps +========== + +Continue by: + + - returning to the @{article:Diffusion User Guide}. diff --git a/src/docs/user/userguide/diffusion.diviner b/src/docs/user/userguide/diffusion.diviner index 1f4f08295f..f3fa53da3b 100644 --- a/src/docs/user/userguide/diffusion.diviner +++ b/src/docs/user/userguide/diffusion.diviner @@ -94,9 +94,23 @@ repositories. = Next Steps = - - Learn about creating a symbol index at +Continue by: + + - learning how to creating a symbol index at @{article:Diffusion User Guide: Symbol Indexes}; or - - set up repository hosting with + - setting up repository hosting with @{article:Diffusion User Guide: Repository Hosting}; or - - understand daemons in detail with @{article:Managing Daemons with phd}; or - - give us feedback at @{article:Give Feedback! Get Support!}. + - managing repository hooks with + @{article:Diffusion User Guide: Commit Hooks}; or + - understanding daemons in more detail with + @{article:Managing Daemons with phd}. + +If you're having trouble getting things working, these topic guides may be +helpful: + + - get details about automatically closing tasks and revisions in response + to commits in @{article:Diffusion User Guide: Autoclose}; or + - understand how Phabricator updates repositories with + @{article:Diffusion User Guide: Repository Updates}; or + - fix issues with repository imports with + @{article:Troubleshooting Repository Imports}. diff --git a/src/docs/user/userguide/diffusion_autoclose.diviner b/src/docs/user/userguide/diffusion_autoclose.diviner index adbc753aad..6d6f76bf87 100644 --- a/src/docs/user/userguide/diffusion_autoclose.diviner +++ b/src/docs/user/userguide/diffusion_autoclose.diviner @@ -58,3 +58,11 @@ not necessarily that the repository is still importing. no containing branch was configured to autoclose. - //Field Not Present// This commit was processed before we implemented this diagnostic feature, and no information is available. + +Next Steps +========== + +Continue by: + + - troubleshooting in greater depth with + @{article:Troubleshooting Repository Imports}. diff --git a/src/docs/user/userguide/diffusion_updates.diviner b/src/docs/user/userguide/diffusion_updates.diviner index 6b19c14480..3dae59b3a5 100644 --- a/src/docs/user/userguide/diffusion_updates.diviner +++ b/src/docs/user/userguide/diffusion_updates.diviner @@ -113,3 +113,11 @@ issues, using the `--trace` flag to get full details: To catch potential issues with permissions, run this command as the same user that the daemons run as. + +Next Steps +========== + +Continue by: + + - troubleshooting in greater depth with + @{article:Troubleshooting Repository Imports}.