Smooth out some UI/UX issues in Harbormaster

Summary:
Ref T8096. Fixes a few bugs and glitches.

  - Set build completion time when handling a message.
  - Format duration information in a more human-readable way.
  - Use a table for build variables.
  - Fix up container PHIDs on diffs (a touch hacky, should be OK for now though).

Test Plan: Browsed around the UI.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8096

Differential Revision: https://secure.phabricator.com/D13382

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -587,6 +587,21 @@ final class DifferentialTransactionEditor
 
         $diff->setRevisionID($object->getID());
         $diff->save();
+
+        // Update Harbormaster to set the containerPHID correctly for any
+        // existing buildables. We may otherwise have buildables stuck with
+        // the old (`null`) container.
+
+        // TODO: This is a bit iffy, maybe we can find a cleaner approach?
+        $table = new HarbormasterBuildable();
+        $conn_w = $table->establishConnection('w');
+        queryfx(
+          $conn_w,
+          'UPDATE %T SET containerPHID = %s WHERE buildablePHID = %s',
+          $table->getTableName(),
+          $object->getPHID(),
+          $diff->getPHID());
+
         return;
     }
 

src/applications/harbormaster/controller/HarbormasterBuildViewController.php

@@ -48,9 +48,7 @@ final class HarbormasterBuildViewController
     $this->buildPropertyLists($box, $build, $actions);
 
     $crumbs = $this->buildApplicationCrumbs();
-    $crumbs->addTextCrumb(
+    $this->addBuildableCrumb($crumbs, $build->getBuildable());
-      $build->getBuildable()->getMonogram(),
-      '/'.$build->getBuildable()->getMonogram());
     $crumbs->addTextCrumb($title);
 
     if ($generation === null || $generation > $build->getBuildGeneration() ||
@@ -99,29 +97,52 @@ final class HarbormasterBuildViewController
       $item->setIcon($icon, $color);
       $status_view->addItem($item);
 
-      $properties->addProperty(pht('Name'), $build_target->getName());
+      $when = array();
+      $started = $build_target->getDateStarted();
+      $now = PhabricatorTime::getNow();
+      if ($started) {
+        $ended = $build_target->getDateCompleted();
+        if ($ended) {
+          $when[] = pht(
+            'Completed at %s',
+            phabricator_datetime($started, $viewer));
 
-      if ($build_target->getDateStarted() !== null) {
+          $duration = ($ended - $started);
-        $properties->addProperty(
+          if ($duration) {
-          pht('Started'),
+            $when[] = pht(
-          phabricator_datetime($build_target->getDateStarted(), $viewer));
+              'Built for %s',
-        if ($build_target->isComplete()) {
+              phutil_format_relative_time_detailed($duration));
-          $properties->addProperty(
+          } else {
-            pht('Completed'),
+            $when[] = pht('Built instantly');
-            phabricator_datetime($build_target->getDateCompleted(), $viewer));
+          }
-          $properties->addProperty(
-            pht('Duration'),
-            phutil_format_relative_time_detailed(
-              $build_target->getDateCompleted() -
-              $build_target->getDateStarted()));
         } else {
-          $properties->addProperty(
+          $when[] = pht(
-            pht('Elapsed'),
+            'Started at %s',
-            phutil_format_relative_time_detailed(
+            phabricator_datetime($started, $viewer));
-              time() - $build_target->getDateStarted()));
+          $duration = ($now - $started);
+          if ($duration) {
+            $when[] = pht(
+              'Running for %s',
+              phutil_format_relative_time_detailed($duration));
+          }
+        }
+      } else {
+        $created = $build_target->getDateCreated();
+        $when[] = pht(
+          'Queued at %s',
+          phabricator_datetime($started, $viewer));
+        $duration = ($now - $created);
+        if ($duration) {
+          $when[] = pht(
+            'Waiting for %s',
+            phutil_format_relative_time_detailed($duration));
         }
       }
 
+      $properties->addProperty(
+        pht('When'),
+        phutil_implode_html(" \xC2\xB7 ", $when));
+
       $properties->addProperty(pht('Status'), $status_view);
 
       $target_box->addPropertyList($properties, pht('Overview'));
@@ -162,9 +183,7 @@ final class HarbormasterBuildViewController
       $variables = $build_target->getVariables();
       if ($variables) {
         $properties = new PHUIPropertyListView();
-        foreach ($variables as $key => $value) {
+        $properties->addRawContent($this->buildProperties($variables));
-          $properties->addProperty($key, $value);
-        }
         $target_box->addPropertyList($properties, pht('Variables'));
       }
 
@@ -183,7 +202,12 @@ final class HarbormasterBuildViewController
       }
 
       $properties = new PHUIPropertyListView();
-      $properties->addProperty(pht('Build Target ID'), $build_target->getID());
+      $properties->addProperty(
+        pht('Build Target ID'),
+        $build_target->getID());
+      $properties->addProperty(
+        pht('Build Target PHID'),
+        $build_target->getPHID());
       $target_box->addPropertyList($properties, pht('Metadata'));
 
       $targets[] = $target_box;
@@ -528,4 +552,30 @@ final class HarbormasterBuildViewController
     return $table;
   }
 
+  private function buildProperties(array $properties) {
+    ksort($properties);
+
+    $rows = array();
+    foreach ($properties as $key => $value) {
+      $rows[] = array(
+        $key,
+        $value,
+      );
+    }
+
+    $table = id(new AphrontTableView($rows))
+      ->setHeaders(
+        array(
+          pht('Key'),
+          pht('Value'),
+        ))
+      ->setColumnClasses(
+        array(
+          'pri right',
+          'wide',
+        ));
+
+    return $table;
+  }
+
 }

src/applications/harbormaster/controller/HarbormasterBuildableViewController.php

@@ -3,21 +3,12 @@
 final class HarbormasterBuildableViewController
   extends HarbormasterController {
 
-  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;
 
     $buildable = id(new HarbormasterBuildableQuery())
       ->setViewer($viewer)
-      ->withIDs(array($id))
+      ->withIDs(array($request->getURIData('id')))
       ->needBuildableHandles(true)
       ->needContainerHandles(true)
       ->executeOne();
@@ -25,6 +16,8 @@ final class HarbormasterBuildableViewController
       return new Aphront404Response();
     }
 
+    $id = $buildable->getID();
+
     // Pull builds and build targets.
     $builds = id(new HarbormasterBuildQuery())
       ->setViewer($viewer)
@@ -33,6 +26,7 @@ final class HarbormasterBuildableViewController
       ->execute();
 
     $buildable->attachBuilds($builds);
+    $object = $buildable->getBuildableObject();
 
     $build_list = $this->buildBuildList($buildable);
 
@@ -55,7 +49,7 @@ final class HarbormasterBuildableViewController
     $this->buildPropertyLists($box, $buildable, $actions);
 
     $crumbs = $this->buildApplicationCrumbs();
-    $crumbs->addTextCrumb("B{$id}");
+    $crumbs->addTextCrumb($buildable->getMonogram());
 
     return $this->buildApplicationPage(
       array(
@@ -144,16 +138,16 @@ final class HarbormasterBuildableViewController
       ->setActionList($actions);
     $box->addPropertyList($properties);
 
-    $properties->addProperty(
-      pht('Buildable'),
-      $buildable->getBuildableHandle()->renderLink());
-
     if ($buildable->getContainerHandle() !== null) {
       $properties->addProperty(
         pht('Container'),
         $buildable->getContainerHandle()->renderLink());
     }
 
+    $properties->addProperty(
+      pht('Buildable'),
+      $buildable->getBuildableHandle()->renderLink());
+
     $properties->addProperty(
       pht('Origin'),
       $buildable->getIsManualBuildable()

src/applications/harbormaster/controller/HarbormasterController.php

@@ -2,4 +2,14 @@
 
 abstract class HarbormasterController extends PhabricatorController {
 
+  protected function addBuildableCrumb(
+    PHUICrumbsView $crumbs,
+    HarbormasterBuildable $buildable) {
+
+    $monogram = $buildable->getMonogram();
+    $uri = '/'.$monogram;
+
+    $crumbs->addTextCrumb($monogram, $uri);
+  }
+
 }

src/applications/harbormaster/engine/HarbormasterBuildEngine.php

@@ -332,6 +332,11 @@ final class HarbormasterBuildEngine extends Phobject {
         $message->save();
 
         $target->setTargetStatus($new_status);
+
+        if ($target->isComplete()) {
+          $target->setDateCompleted(PhabricatorTime::getNow());
+        }
+
         $target->save();
       }
     }

src/applications/harbormaster/worker/HarbormasterTargetWorker.php

@@ -59,7 +59,7 @@ final class HarbormasterTargetWorker extends HarbormasterWorker {
       $target->setTargetStatus($next_status);
 
       if ($target->isComplete()) {
-        $target->setDateCompleted(time());
+        $target->setDateCompleted(PhabricatorTime::getNow());
       }
 
       $target->save();
@@ -70,12 +70,12 @@ final class HarbormasterTargetWorker extends HarbormasterWorker {
     } catch (HarbormasterBuildFailureException $ex) {
       // A build step wants to fail explicitly.
       $target->setTargetStatus(HarbormasterBuildTarget::STATUS_FAILED);
-      $target->setDateCompleted(time());
+      $target->setDateCompleted(PhabricatorTime::getNow());
       $target->save();
     } catch (HarbormasterBuildAbortedException $ex) {
       // A build step is aborting because the build has been restarted.
       $target->setTargetStatus(HarbormasterBuildTarget::STATUS_ABORTED);
-      $target->setDateCompleted(time());
+      $target->setDateCompleted(PhabricatorTime::getNow());
       $target->save();
     } catch (Exception $ex) {
       phlog($ex);