diff --git a/resources/celerity/map.php b/resources/celerity/map.php --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -7,7 +7,7 @@ */ return array( 'names' => array( - 'core.pkg.css' => 'f7a91f6a', + 'core.pkg.css' => '76a3afdf', 'core.pkg.js' => '7d8faf57', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '2de124c9', @@ -25,7 +25,7 @@ 'rsrc/css/aphront/notification.css' => '7f684b62', 'rsrc/css/aphront/panel-view.css' => '8427b78d', 'rsrc/css/aphront/phabricator-nav-view.css' => 'ac79a758', - 'rsrc/css/aphront/table-view.css' => 'ec078a76', + 'rsrc/css/aphront/table-view.css' => 'aba95954', 'rsrc/css/aphront/tokenizer.css' => '056da01b', 'rsrc/css/aphront/tooltip.css' => '1a07aea8', 'rsrc/css/aphront/typeahead-browse.css' => 'd8581d2c', @@ -523,7 +523,7 @@ 'aphront-list-filter-view-css' => '5d6f0526', 'aphront-multi-column-view-css' => 'fd18389d', 'aphront-panel-view-css' => '8427b78d', - 'aphront-table-view-css' => 'ec078a76', + 'aphront-table-view-css' => 'aba95954', 'aphront-tokenizer-control-css' => '056da01b', 'aphront-tooltip-css' => '1a07aea8', 'aphront-typeahead-control-css' => 'd4f16145', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1149,6 +1149,7 @@ 'HarbormasterUnitMessageViewController' => 'applications/harbormaster/controller/HarbormasterUnitMessageViewController.php', 'HarbormasterUnitPropertyView' => 'applications/harbormaster/view/HarbormasterUnitPropertyView.php', 'HarbormasterUnitStatus' => 'applications/harbormaster/constants/HarbormasterUnitStatus.php', + 'HarbormasterUnitSummaryView' => 'applications/harbormaster/view/HarbormasterUnitSummaryView.php', 'HarbormasterUploadArtifactBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterUploadArtifactBuildStepImplementation.php', 'HarbormasterWaitForPreviousBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterWaitForPreviousBuildStepImplementation.php', 'HarbormasterWorker' => 'applications/harbormaster/worker/HarbormasterWorker.php', @@ -4624,7 +4625,7 @@ 'DifferentialTransactionEditor' => 'PhabricatorApplicationTransactionEditor', 'DifferentialTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'DifferentialTransactionView' => 'PhabricatorApplicationTransactionView', - 'DifferentialUnitField' => 'DifferentialHarbormasterField', + 'DifferentialUnitField' => 'DifferentialCustomField', 'DifferentialUnitStatus' => 'Phobject', 'DifferentialUnitTestResult' => 'Phobject', 'DifferentialUpdateRevisionConduitAPIMethod' => 'DifferentialConduitAPIMethod', @@ -5318,6 +5319,7 @@ 'HarbormasterUnitMessageViewController' => 'HarbormasterController', 'HarbormasterUnitPropertyView' => 'AphrontView', 'HarbormasterUnitStatus' => 'Phobject', + 'HarbormasterUnitSummaryView' => 'AphrontView', 'HarbormasterUploadArtifactBuildStepImplementation' => 'HarbormasterBuildStepImplementation', 'HarbormasterWaitForPreviousBuildStepImplementation' => 'HarbormasterBuildStepImplementation', 'HarbormasterWorker' => 'PhabricatorWorker', diff --git a/src/applications/differential/controller/DifferentialController.php b/src/applications/differential/controller/DifferentialController.php --- a/src/applications/differential/controller/DifferentialController.php +++ b/src/applications/differential/controller/DifferentialController.php @@ -92,4 +92,130 @@ return $toc_view; } + protected function loadDiffProperties(array $diffs) { + $diffs = mpull($diffs, null, 'getID'); + + $properties = id(new DifferentialDiffProperty())->loadAllWhere( + 'diffID IN (%Ld)', + array_keys($diffs)); + $properties = mgroup($properties, 'getDiffID'); + + foreach ($diffs as $id => $diff) { + $values = idx($properties, $id, array()); + $values = mpull($values, 'getData', 'getName'); + $diff->attachDiffProperties($values); + } + } + + + protected function loadHarbormasterData(array $diffs) { + $viewer = $this->getViewer(); + + $diffs = mpull($diffs, null, 'getPHID'); + + $buildables = id(new HarbormasterBuildableQuery()) + ->setViewer($viewer) + ->withBuildablePHIDs(array_keys($diffs)) + ->withManualBuildables(false) + ->needBuilds(true) + ->needTargets(true) + ->execute(); + + $buildables = mpull($buildables, null, 'getBuildablePHID'); + foreach ($diffs as $phid => $diff) { + $diff->attachBuildable(idx($buildables, $phid)); + } + + $target_map = array(); + foreach ($diffs as $phid => $diff) { + $target_map[$phid] = $diff->getBuildTargetPHIDs(); + } + $all_target_phids = array_mergev($target_map); + + if ($all_target_phids) { + $unit_messages = id(new HarbormasterBuildUnitMessage())->loadAllWhere( + 'buildTargetPHID IN (%Ls)', + $all_target_phids); + $unit_messages = mgroup($unit_messages, 'getBuildTargetPHID'); + } else { + $unit_messages = array(); + } + + foreach ($diffs as $phid => $diff) { + $target_phids = idx($target_map, $phid, array()); + $messages = array_select_keys($unit_messages, $target_phids); + $messages = array_mergev($messages); + $diff->attachUnitMessages($messages); + } + + // For diffs with no messages, look for legacy unit messages stored on the + // diff itself. + foreach ($diffs as $phid => $diff) { + if ($diff->getUnitMessages()) { + continue; + } + + if (!$diff->hasDiffProperty('arc:unit')) { + continue; + } + + $legacy_messages = $diff->getProperty('arc:unit'); + if (!$legacy_messages) { + continue; + } + + // Show the top 100 legacy lint messages. Previously, we showed some + // by default and let the user toggle the rest. With modern messages, + // we can send the user to the Harbormaster detail page. Just show + // "a lot" of messages in legacy cases to try to strike a balance + // between implementation simplicitly and compatibility. + $legacy_messages = array_slice($legacy_messages, 0, 100); + + $messages = array(); + foreach ($legacy_messages as $message) { + $messages[] = HarbormasterBuildUnitMessage::newFromDictionary( + new HarbormasterBuildTarget(), + $this->getModernUnitMessageDictionary($message)); + } + + $diff->attachUnitMessages($messages); + } + } + + private function getModernUnitMessageDictionary(array $map) { + // Strip out `null` values to satisfy stricter typechecks. + foreach ($map as $key => $value) { + if ($value === null) { + unset($map[$key]); + } + } + + return $map; + } + + protected function getDiffTabLabels(array $diffs) { + // Make sure we're only going to render unique diffs. + $diffs = mpull($diffs, null, 'getID'); + $labels = array(pht('Left'), pht('Right')); + + $results = array(); + + foreach ($diffs as $diff) { + if (count($diffs) == 2) { + $label = array_shift($labels); + $label = pht('%s (Diff %d)', $label, $diff->getID()); + } else { + $label = pht('Diff %d', $diff->getID()); + } + + $results[] = array( + $label, + $diff, + ); + } + + return $results; + } + + } diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -102,10 +102,8 @@ } } - $props = id(new DifferentialDiffProperty())->loadAllWhere( - 'diffID = %d', - $target_manual->getID()); - $props = mpull($props, 'getData', 'getName'); + $this->loadDiffProperties($diffs); + $props = $target_manual->getDiffProperties(); $object_phids = array_merge( $revision->getReviewers(), @@ -256,23 +254,17 @@ array($diff_vs, $target->getID())); $detail_diffs = mpull($detail_diffs, null, 'getPHID'); - $buildables = id(new HarbormasterBuildableQuery()) - ->setViewer($viewer) - ->withBuildablePHIDs(array_keys($detail_diffs)) - ->withManualBuildables(false) - ->needBuilds(true) - ->needTargets(true) - ->execute(); - $buildables = mpull($buildables, null, 'getBuildablePHID'); - foreach ($detail_diffs as $diff_phid => $detail_diff) { - $detail_diff->attachBuildable(idx($buildables, $diff_phid)); - } + $this->loadHarbormasterData($detail_diffs); $diff_detail_box = $this->buildDiffDetailView( $detail_diffs, $revision, $field_list); + $unit_box = $this->buildUnitMessagesView( + $target, + $revision); + $comment_view = $this->buildTransactions( $revision, $diff_vs ? $diffs[$diff_vs] : $target, @@ -469,6 +461,7 @@ $operations_box, $revision_detail_box, $diff_detail_box, + $unit_box, $page_pane, ); @@ -972,18 +965,9 @@ return null; } - // Make sure we're only going to render unique diffs. - $diffs = mpull($diffs, null, 'getID'); - $labels = array(pht('Left'), pht('Right')); - $property_lists = array(); - foreach ($diffs as $diff) { - if (count($diffs) == 2) { - $label = array_shift($labels); - $label = pht('%s (Diff %d)', $label, $diff->getID()); - } else { - $label = pht('Diff %d', $diff->getID()); - } + foreach ($this->getDiffTabLabels($diffs) as $tab) { + list($label, $diff) = $tab; $property_lists[] = array( $label, @@ -1083,4 +1067,44 @@ ->setOperation($operation); } + private function buildUnitMessagesView( + $diff, + DifferentialRevision $revision) { + $viewer = $this->getViewer(); + + if (!$diff->getUnitMessages()) { + return null; + } + + $interesting_messages = array(); + foreach ($diff->getUnitMessages() as $message) { + switch ($message->getResult()) { + case ArcanistUnitTestResult::RESULT_PASS: + case ArcanistUnitTestResult::RESULT_SKIP: + break; + default: + $interesting_messages[] = $message; + break; + } + } + + if (!$interesting_messages) { + return null; + } + + $excuse = null; + if ($diff->hasDiffProperty('arc:unit-excuse')) { + $excuse = $diff->getProperty('arc:unit-excuse'); + } + + return id(new HarbormasterUnitSummaryView()) + ->setUser($viewer) + ->setExcuse($excuse) + ->setBuildable($diff->getBuildable()) + ->setUnitMessages($diff->getUnitMessages()) + ->setLimit(5) + ->setShowViewAll(true); + } + + } diff --git a/src/applications/differential/customfield/DifferentialUnitField.php b/src/applications/differential/customfield/DifferentialUnitField.php --- a/src/applications/differential/customfield/DifferentialUnitField.php +++ b/src/applications/differential/customfield/DifferentialUnitField.php @@ -1,7 +1,7 @@ getFieldName(); } - protected function getLegacyProperty() { - return 'arc:unit'; - } - - protected function getDiffPropertyKeys() { - return array( - 'arc:unit', - 'arc:unit-excuse', - ); - } - - protected function loadHarbormasterTargetMessages(array $target_phids) { - return id(new HarbormasterBuildUnitMessage())->loadAllWhere( - 'buildTargetPHID IN (%Ls)', - $target_phids); - } - - protected function newModernMessage(array $message) { - return HarbormasterBuildUnitMessage::newFromDictionary( - new HarbormasterBuildTarget(), - $this->getModernUnitMessageDictionary($message)); - } - - protected function newHarbormasterMessageView(array $all_messages) { - $messages = $all_messages; - - foreach ($messages as $key => $message) { - switch ($message->getResult()) { - case ArcanistUnitTestResult::RESULT_PASS: - case ArcanistUnitTestResult::RESULT_SKIP: - // Don't show "Pass" or "Skip" in the UI since they aren't very - // interesting. The user can click through to the full results if - // they want details. - unset($messages[$key]); - break; - } - } - - if (!$messages) { - return null; - } - - $table = id(new HarbormasterUnitPropertyView()) - ->setLimit(5) - ->setUnitMessages($all_messages); - - $diff = $this->getObject()->getActiveDiff(); - $buildable = $diff->getBuildable(); - if ($buildable) { - $full_results = '/harbormaster/unit/'.$buildable->getID().'/'; - $table->setFullResultsURI($full_results); - } - - return $table; - } - public function getWarningsForDetailView() { $status = $this->getObject()->getActiveDiff()->getUnitStatus(); @@ -105,9 +49,7 @@ return $warnings; } - protected function renderHarbormasterStatus( - DifferentialDiff $diff, - array $messages) { + public function renderDiffPropertyViewValue(DifferentialDiff $diff) { $colors = array( DifferentialUnitStatus::UNIT_NONE => 'grey', @@ -121,42 +63,15 @@ $message = DifferentialRevisionUpdateHistoryView::getDiffUnitMessage($diff); - $note = array(); - - $excuse = $diff->getProperty('arc:unit-excuse'); - if (strlen($excuse)) { - $excuse = array( - phutil_tag('strong', array(), pht('Excuse:')), - ' ', - phutil_escape_html_newlines($excuse), - ); - $note[] = $excuse; - } - - $note = phutil_implode_html(" \xC2\xB7 ", $note); - $status = id(new PHUIStatusListView()) ->addItem( id(new PHUIStatusItemView()) ->setIcon(PHUIStatusItemView::ICON_STAR, $icon_color) - ->setTarget($message) - ->setNote($note)); + ->setTarget($message)); return $status; } - private function getModernUnitMessageDictionary(array $map) { - // Strip out `null` values to satisfy stricter typechecks. - foreach ($map as $key => $value) { - if ($value === null) { - unset($map[$key]); - } - } - - // TODO: Remap more stuff here? - - return $map; - } } diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php --- a/src/applications/differential/storage/DifferentialDiff.php +++ b/src/applications/differential/storage/DifferentialDiff.php @@ -40,6 +40,8 @@ private $properties = array(); private $buildable = self::ATTACHABLE; + private $unitMessages = self::ATTACHABLE; + protected function getConfiguration() { return array( self::CONFIG_AUX_PHID => true, @@ -334,6 +336,20 @@ return $this->assertAttachedKey($this->properties, $key); } + public function hasDiffProperty($key) { + $properties = $this->getDiffProperties(); + return array_key_exists($key, $properties); + } + + public function attachDiffProperties(array $properties) { + $this->properties = $properties; + return $this; + } + + public function getDiffProperties() { + return $this->assertAttached($this->properties); + } + public function attachBuildable(HarbormasterBuildable $buildable = null) { $this->buildable = $buildable; return $this; @@ -391,6 +407,17 @@ } + public function attachUnitMessages(array $unit_messages) { + $this->unitMessages = $unit_messages; + return $this; + } + + + public function getUnitMessages() { + return $this->assertAttached($this->unitMessages); + } + + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php b/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php --- a/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php @@ -336,42 +336,11 @@ } if ($unit_data) { - $unit_href = $this->getApplicationURI('unit/'.$buildable->getID().'/'); - - $unit_table = id(new HarbormasterUnitPropertyView()) - ->setUser($viewer) - ->setLimit(5) + $unit = id(new HarbormasterUnitSummaryView()) + ->setBuildable($buildable) ->setUnitMessages($unit_data) - ->setFullResultsURI($unit_href); - - $unit_data = msort($unit_data, 'getSortKey'); - $head_unit = head($unit_data); - if ($head_unit) { - $status = $head_unit->getResult(); - - $tag_text = HarbormasterUnitStatus::getUnitStatusLabel($status); - $tag_color = HarbormasterUnitStatus::getUnitStatusColor($status); - $tag_icon = HarbormasterUnitStatus::getUnitStatusIcon($status); - - } else { - $tag_text = pht('No Unit Tests'); - $tag_color = 'grey'; - $tag_icon = 'fa-ban'; - } - - $unit_header = id(new PHUIHeaderView()) - ->setHeader(pht('Unit Tests')) - ->setStatus($tag_icon, $tag_color, $tag_text) - ->addActionLink( - id(new PHUIButtonView()) - ->setTag('a') - ->setHref($unit_href) - ->setIcon('fa-list-ul') - ->setText('View All')); - - $unit = id(new PHUIObjectBoxView()) - ->setHeader($unit_header) - ->setTable($unit_table); + ->setShowViewAll(true) + ->setLimit(5); } else { $unit = null; } diff --git a/src/applications/harbormaster/controller/HarbormasterUnitMessageListController.php b/src/applications/harbormaster/controller/HarbormasterUnitMessageListController.php --- a/src/applications/harbormaster/controller/HarbormasterUnitMessageListController.php +++ b/src/applications/harbormaster/controller/HarbormasterUnitMessageListController.php @@ -34,14 +34,10 @@ $unit_data = array(); } - $unit_table = id(new HarbormasterUnitPropertyView()) - ->setUser($viewer) + $unit = id(new HarbormasterUnitSummaryView()) + ->setBuildable($buildable) ->setUnitMessages($unit_data); - $unit = id(new PHUIObjectBoxView()) - ->setHeaderText(pht('Unit Tests')) - ->setTable($unit_table); - $crumbs = $this->buildApplicationCrumbs(); $this->addBuildableCrumb($crumbs, $buildable); $crumbs->addTextCrumb(pht('Unit Tests')); diff --git a/src/applications/harbormaster/view/HarbormasterUnitPropertyView.php b/src/applications/harbormaster/view/HarbormasterUnitPropertyView.php --- a/src/applications/harbormaster/view/HarbormasterUnitPropertyView.php +++ b/src/applications/harbormaster/view/HarbormasterUnitPropertyView.php @@ -6,6 +6,7 @@ private $unitMessages = array(); private $limit; private $fullResultsURI; + private $notice; public function setPathURIMap(array $map) { $this->pathURIMap = $map; @@ -28,6 +29,12 @@ return $this; } + public function setNotice($notice) { + $this->notice = $notice; + return $this; + } + + public function render() { require_celerity_resource('harbormaster-css'); @@ -68,12 +75,14 @@ $name = $message->getUnitMessageDisplayName(); $id = $message->getID(); - $name = phutil_tag( - 'a', - array( - 'href' => "/harbormaster/unit/view/{$id}/", - ), - $name); + if ($id) { + $name = phutil_tag( + 'a', + array( + 'href' => "/harbormaster/unit/view/{$id}/", + ), + $name); + } $details = $message->getUnitMessageDetails(); if (strlen($details)) { @@ -127,8 +136,8 @@ )) ->setColumnClasses( array( - 'top', - 'top', + 'top center', + 'top right', 'top wide', )) ->setColumnWidths( @@ -142,6 +151,10 @@ $any_duration, )); + if ($this->notice) { + $table->setNotice($this->notice); + } + return $table; } diff --git a/src/applications/harbormaster/view/HarbormasterUnitSummaryView.php b/src/applications/harbormaster/view/HarbormasterUnitSummaryView.php new file mode 100644 --- /dev/null +++ b/src/applications/harbormaster/view/HarbormasterUnitSummaryView.php @@ -0,0 +1,104 @@ +buildable = $buildable; + return $this; + } + + public function setUnitMessages(array $messages) { + $this->messages = $messages; + return $this; + } + + public function setLimit($limit) { + $this->limit = $limit; + return $this; + } + + public function setExcuse($excuse) { + $this->excuse = $excuse; + return $this; + } + + public function setShowViewAll($show_view_all) { + $this->showViewAll = $show_view_all; + return $this; + } + + public function render() { + $messages = $this->messages; + $buildable = $this->buildable; + + $id = $buildable->getID(); + $full_uri = "/harbormaster/unit/{$id}/"; + + $messages = msort($messages, 'getSortKey'); + $head_unit = head($messages); + if ($head_unit) { + $status = $head_unit->getResult(); + + $tag_text = HarbormasterUnitStatus::getUnitStatusLabel($status); + $tag_color = HarbormasterUnitStatus::getUnitStatusColor($status); + $tag_icon = HarbormasterUnitStatus::getUnitStatusIcon($status); + } else { + $tag_text = pht('No Unit Tests'); + $tag_color = 'grey'; + $tag_icon = 'fa-ban'; + } + + $header = id(new PHUIHeaderView()) + ->setHeader(pht('Unit Tests')) + ->setStatus($tag_icon, $tag_color, $tag_text); + + if ($this->showViewAll) { + $view_all = id(new PHUIButtonView()) + ->setTag('a') + ->setHref($full_uri) + ->setIcon('fa-list-ul') + ->setText('View All'); + $header->addActionLink($view_all); + } + + $box = id(new PHUIObjectBoxView()) + ->setHeader($header); + + $table = id(new HarbormasterUnitPropertyView()) + ->setUnitMessages($messages); + + if ($this->showViewAll) { + $table->setFullResultsURI($full_uri); + } + + if ($this->limit) { + $table->setLimit($this->limit); + } + + $excuse = $this->excuse; + if (strlen($excuse)) { + $excuse_icon = id(new PHUIIconView()) + ->setIcon('fa-commenting-o red'); + + $table->setNotice( + array( + $excuse_icon, + ' ', + phutil_tag('strong', array(), pht('Excuse:')), + ' ', + $excuse, + )); + } + + $box->setTable($table); + + return $box; + } + +} diff --git a/src/view/control/AphrontTableView.php b/src/view/control/AphrontTableView.php --- a/src/view/control/AphrontTableView.php +++ b/src/view/control/AphrontTableView.php @@ -157,21 +157,6 @@ $sort_values[] = null; } - if ($this->notice) { - $colspan = max(count(array_filter($visibility)), 1); - $table[] = phutil_tag( - 'tr', - array(), - phutil_tag( - 'td', - array( - 'colspan' => $colspan, - 'class' => 'aphront-table-notice', - ), - $this->notice)); - - } - $tr = array(); foreach ($headers as $col_num => $header) { if (!$visibility[$col_num]) { @@ -350,13 +335,29 @@ $classes[] = 'aphront-table-view-fixed'; } + $notice = null; + if ($this->notice) { + $notice = phutil_tag( + 'div', + array( + 'class' => 'aphront-table-notice', + ), + $this->notice); + } + $html = phutil_tag( 'table', array( 'class' => implode(' ', $classes), ), $table); - return phutil_tag_div('aphront-table-wrap', $html); + + return phutil_tag_div( + 'aphront-table-wrap', + array( + $notice, + $html, + )); } public static function renderSingleDisplayLine($line) { diff --git a/webroot/rsrc/css/aphront/table-view.css b/webroot/rsrc/css/aphront/table-view.css --- a/webroot/rsrc/css/aphront/table-view.css +++ b/webroot/rsrc/css/aphront/table-view.css @@ -19,7 +19,11 @@ table-layout: fixed; } -.aphront-table-view td.aphront-table-notice { +.aphront-table-view-fixed th { + box-sizing: border-box; +} + +.aphront-table-notice { padding: 12px 16px; font-size: {$normalfontsize}; color: {$darkbluetext};