From a1431e53cc0110c8076328d452d17c48ef3b050a Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 10 Aug 2015 14:15:19 -0700 Subject: [PATCH] Link to Harbormaster build targets from the Daemon worker page Summary: Fixes T7370. Two changes: - Make the default to show nothing, instead of showing all the data. This is a better default because the data is sometimes sensitive. Workers should have to opt in to revealing it. - For TargetWorkers, link to the target (technically the build, for now, since there's no dedicated target detail page). Test Plan: {F698325} Reviewers: chad Reviewed By: chad Maniphest Tasks: T7370 Differential Revision: https://secure.phabricator.com/D13845 --- .../PhabricatorWorkerTaskDetailController.php | 6 ++--- .../phid/HarbormasterBuildPHIDType.php | 11 ++++---- .../phid/HarbormasterBuildTargetPHIDType.php | 12 ++++++++- .../worker/HarbormasterBuildWorker.php | 27 +++++++++++++++---- .../worker/HarbormasterTargetWorker.php | 10 +++++++ .../PhabricatorRepositoryPushMailWorker.php | 5 ---- .../daemon/workers/PhabricatorWorker.php | 3 +-- .../sms/worker/PhabricatorSMSWorker.php | 5 ---- 8 files changed, 52 insertions(+), 27 deletions(-) diff --git a/src/applications/daemon/controller/PhabricatorWorkerTaskDetailController.php b/src/applications/daemon/controller/PhabricatorWorkerTaskDetailController.php index a0c8608935..ad8f673fd7 100644 --- a/src/applications/daemon/controller/PhabricatorWorkerTaskDetailController.php +++ b/src/applications/daemon/controller/PhabricatorWorkerTaskDetailController.php @@ -149,9 +149,9 @@ final class PhabricatorWorkerTaskDetailController $worker = $task->getWorkerInstance(); $data = $worker->renderForDisplay($viewer); - $view->addProperty( - pht('Data'), - $data); + if ($data !== null) { + $view->addProperty(pht('Data'), $data); + } return $view; } diff --git a/src/applications/harbormaster/phid/HarbormasterBuildPHIDType.php b/src/applications/harbormaster/phid/HarbormasterBuildPHIDType.php index 5ca684a098..5669df4ee9 100644 --- a/src/applications/harbormaster/phid/HarbormasterBuildPHIDType.php +++ b/src/applications/harbormaster/phid/HarbormasterBuildPHIDType.php @@ -27,12 +27,11 @@ final class HarbormasterBuildPHIDType extends PhabricatorPHIDType { foreach ($handles as $phid => $handle) { $build = $objects[$phid]; - $handles[$phid]->setName(pht( - 'Build %d: %s', - $build->getID(), - $build->getName())); - $handles[$phid]->setURI( - '/harbormaster/build/'.$build->getID()); + $build_id = $build->getID(); + $name = $build->getName(); + + $handle->setName(pht('Build %d: %s', $build_id, $name)); + $handle->setURI("/harbormaster/build/{$build_id}/"); } } diff --git a/src/applications/harbormaster/phid/HarbormasterBuildTargetPHIDType.php b/src/applications/harbormaster/phid/HarbormasterBuildTargetPHIDType.php index 8aa69abad6..e16db13ccb 100644 --- a/src/applications/harbormaster/phid/HarbormasterBuildTargetPHIDType.php +++ b/src/applications/harbormaster/phid/HarbormasterBuildTargetPHIDType.php @@ -26,7 +26,17 @@ final class HarbormasterBuildTargetPHIDType extends PhabricatorPHIDType { array $objects) { foreach ($handles as $phid => $handle) { - $build_target = $objects[$phid]; + $target = $objects[$phid]; + $target_id = $target->getID(); + + // Build target don't currently have their own page, so just point + // the user at the build until we have one. + $build = $target->getBuild(); + $build_id = $build->getID(); + $uri = "/harbormaster/build/{$build_id}/"; + + $handle->setName(pht('Build Target %d', $target_id)); + $handle->setURI($uri); } } diff --git a/src/applications/harbormaster/worker/HarbormasterBuildWorker.php b/src/applications/harbormaster/worker/HarbormasterBuildWorker.php index c71563a8a5..67eb6b3e78 100644 --- a/src/applications/harbormaster/worker/HarbormasterBuildWorker.php +++ b/src/applications/harbormaster/worker/HarbormasterBuildWorker.php @@ -5,11 +5,31 @@ */ final class HarbormasterBuildWorker extends HarbormasterWorker { + public function renderForDisplay(PhabricatorUser $viewer) { + try { + $build = $this->loadBuild(); + } catch (Exception $ex) { + return null; + } + + return $viewer->renderHandle($build->getPHID()); + } + protected function doWork() { + $viewer = $this->getViewer(); + $build = $this->loadBuild(); + + id(new HarbormasterBuildEngine()) + ->setViewer($viewer) + ->setBuild($build) + ->continueBuild(); + } + + private function loadBuild() { $data = $this->getTaskData(); $id = idx($data, 'buildID'); - $viewer = $this->getViewer(); + $viewer = $this->getViewer(); $build = id(new HarbormasterBuildQuery()) ->setViewer($viewer) ->withIDs(array($id)) @@ -19,10 +39,7 @@ final class HarbormasterBuildWorker extends HarbormasterWorker { pht('Invalid build ID "%s".', $id)); } - id(new HarbormasterBuildEngine()) - ->setViewer($viewer) - ->setBuild($build) - ->continueBuild(); + return $build; } } diff --git a/src/applications/harbormaster/worker/HarbormasterTargetWorker.php b/src/applications/harbormaster/worker/HarbormasterTargetWorker.php index a13a9de163..db355b145c 100644 --- a/src/applications/harbormaster/worker/HarbormasterTargetWorker.php +++ b/src/applications/harbormaster/worker/HarbormasterTargetWorker.php @@ -11,6 +11,16 @@ final class HarbormasterTargetWorker extends HarbormasterWorker { return phutil_units('24 hours in seconds'); } + public function renderForDisplay(PhabricatorUser $viewer) { + try { + $target = $this->loadBuildTarget(); + } catch (Exception $ex) { + return null; + } + + return $viewer->renderHandle($target->getPHID()); + } + private function loadBuildTarget() { $data = $this->getTaskData(); $id = idx($data, 'targetID'); diff --git a/src/applications/repository/worker/PhabricatorRepositoryPushMailWorker.php b/src/applications/repository/worker/PhabricatorRepositoryPushMailWorker.php index 49f5fbe293..17226a1377 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryPushMailWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryPushMailWorker.php @@ -130,11 +130,6 @@ final class PhabricatorRepositoryPushMailWorker return $target->willSendMail($mail); } - public function renderForDisplay(PhabricatorUser $viewer) { - // This data has some sensitive stuff, so don't show it. - return null; - } - private function renderRefs(array $logs) { $ref_lines = array(); $ref_list = array(); diff --git a/src/infrastructure/daemon/workers/PhabricatorWorker.php b/src/infrastructure/daemon/workers/PhabricatorWorker.php index 29f3e0ae39..629e688b90 100644 --- a/src/infrastructure/daemon/workers/PhabricatorWorker.php +++ b/src/infrastructure/daemon/workers/PhabricatorWorker.php @@ -210,8 +210,7 @@ abstract class PhabricatorWorker extends Phobject { } public function renderForDisplay(PhabricatorUser $viewer) { - $data = PhutilReadableSerializer::printableValue($this->data); - return phutil_tag('pre', array(), $data); + return null; } /** diff --git a/src/infrastructure/sms/worker/PhabricatorSMSWorker.php b/src/infrastructure/sms/worker/PhabricatorSMSWorker.php index a999fe5e82..145a371e7d 100644 --- a/src/infrastructure/sms/worker/PhabricatorSMSWorker.php +++ b/src/infrastructure/sms/worker/PhabricatorSMSWorker.php @@ -3,9 +3,4 @@ abstract class PhabricatorSMSWorker extends PhabricatorWorker { - public function renderForDisplay(PhabricatorUser $viewer) { - // This data has some sensitive stuff, so don't show it. - return null; - } - }