From 0fab41ff3c075d457470e3085bc173174b36ab0a Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 23 Apr 2019 06:55:15 -0700 Subject: [PATCH] Show "hold reasons" on commit page, not on "Edit" page Summary: Depends on D20465. Ref T13277. Currently, when a commit is unpublished, we put a single line about it on the "Edit Commit" page. This is pretty much impossible to find. Move it to the main page. This treatment is more big/bold than I'd probably like to end up, but we should probably overshoot on the explanatory text until users get used to this behavior. Also, allow searching for only published / unpublished commits. Test Plan: {F6395705} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13277 Differential Revision: https://secure.phabricator.com/D20466 --- src/__phutil_library_map__.php | 2 + .../query/PhabricatorCommitSearchEngine.php | 15 ++++ .../controller/DiffusionCommitController.php | 80 +++++++++++++++---- .../editor/DiffusionCommitEditEngine.php | 24 ------ .../diffusion/query/DiffusionCommitQuery.php | 21 +++++ .../diffusion/view/DiffusionHistoryView.php | 7 ++ .../query/PhabricatorRepositoryPublisher.php | 28 ------- ...abricatorRepositoryPublisherHoldReason.php | 78 ++++++++++++++++++ .../PhabricatorRepositoryCommitData.php | 9 ++- src/view/phui/PHUIInfoView.php | 2 +- 10 files changed, 194 insertions(+), 72 deletions(-) create mode 100644 src/applications/repository/query/PhabricatorRepositoryPublisherHoldReason.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 624096055d..9e4fd6884b 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4408,6 +4408,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryParsedChange' => 'applications/repository/data/PhabricatorRepositoryParsedChange.php', 'PhabricatorRepositoryPermanentRefsTransaction' => 'applications/repository/xaction/PhabricatorRepositoryPermanentRefsTransaction.php', 'PhabricatorRepositoryPublisher' => 'applications/repository/query/PhabricatorRepositoryPublisher.php', + 'PhabricatorRepositoryPublisherHoldReason' => 'applications/repository/query/PhabricatorRepositoryPublisherHoldReason.php', 'PhabricatorRepositoryPullEngine' => 'applications/repository/engine/PhabricatorRepositoryPullEngine.php', 'PhabricatorRepositoryPullEvent' => 'applications/repository/storage/PhabricatorRepositoryPullEvent.php', 'PhabricatorRepositoryPullEventPHIDType' => 'applications/repository/phid/PhabricatorRepositoryPullEventPHIDType.php', @@ -10690,6 +10691,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryParsedChange' => 'Phobject', 'PhabricatorRepositoryPermanentRefsTransaction' => 'PhabricatorRepositoryTransactionType', 'PhabricatorRepositoryPublisher' => 'Phobject', + 'PhabricatorRepositoryPublisherHoldReason' => 'Phobject', 'PhabricatorRepositoryPullEngine' => 'PhabricatorRepositoryEngine', 'PhabricatorRepositoryPullEvent' => array( 'PhabricatorRepositoryDAO', diff --git a/src/applications/audit/query/PhabricatorCommitSearchEngine.php b/src/applications/audit/query/PhabricatorCommitSearchEngine.php index e286573e93..35db1270ce 100644 --- a/src/applications/audit/query/PhabricatorCommitSearchEngine.php +++ b/src/applications/audit/query/PhabricatorCommitSearchEngine.php @@ -54,6 +54,10 @@ final class PhabricatorCommitSearchEngine $query->withUnreachable($map['unreachable']); } + if ($map['unpublished'] !== null) { + $query->withUnpublished($map['unpublished']); + } + if ($map['ancestorsOf']) { $query->withAncestorsOf($map['ancestorsOf']); } @@ -127,6 +131,17 @@ final class PhabricatorCommitSearchEngine pht( 'Find or exclude unreachable commits which are not ancestors of '. 'any branch, tag, or ref.')), + id(new PhabricatorSearchThreeStateField()) + ->setLabel(pht('Unpublished')) + ->setKey('unpublished') + ->setOptions( + pht('(Show All)'), + pht('Show Only Unpublished Commits'), + pht('Hide Unpublished Commits')) + ->setDescription( + pht( + 'Find or exclude unpublished commits which are not ancestors of '. + 'any permanent branch, tag, or ref.')), id(new PhabricatorSearchStringListField()) ->setLabel(pht('Ancestors Of')) ->setKey('ancestorsOf') diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index 013d87ee68..9891eb50d6 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -116,6 +116,7 @@ final class DiffusionCommitController extends DiffusionController { $commit_data = $commit->getCommitData(); $is_foreign = $commit_data->getCommitDetail('foreign-svn-stub'); $error_panel = null; + $unpublished_panel = null; $hard_limit = 1000; @@ -248,9 +249,41 @@ final class DiffusionCommitController extends DiffusionController { $header->addTag($nonpermanent_tag); - $this->commitErrors[] = pht( - 'This commit is not reachable from any permanent branch, tag, '. - 'or ref.'); + $holds = $commit_data->newPublisherHoldReasons(); + + $reasons = array(); + foreach ($holds as $hold) { + $reasons[] = array( + phutil_tag('strong', array(), pht('%s:', $hold->getName())), + ' ', + $hold->getSummary(), + ); + } + + if (!$holds) { + $reasons[] = pht('No further details are available.'); + } + + $doc_href = PhabricatorEnv::getDoclink( + 'Diffusion User Guide: Permanent Refs'); + $doc_link = phutil_tag( + 'a', + array( + 'href' => $doc_href, + 'target' => '_blank', + ), + pht('Learn More')); + + $title = array( + pht('Unpublished Commit'), + pht(" \xC2\xB7 "), + $doc_link, + ); + + $unpublished_panel = id(new PHUIInfoView()) + ->setTitle($title) + ->setErrors($reasons) + ->setSeverity(PHUIInfoView::SEVERITY_WARNING); } @@ -445,23 +478,36 @@ final class DiffusionCommitController extends DiffusionController { ->setWidth((int)$width); } + $description_box = id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Description')) + ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->appendChild($detail_list); + + $detail_box = id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Details')) + ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->appendChild($details); + $view = id(new PHUITwoColumnView()) ->setHeader($header) ->setSubheader($subheader) - ->setMainColumn(array( - $error_panel, - $timeline, - $merge_table, - $info_panel, - )) - ->setFooter(array( - $change_table, - $change_list, - $add_comment, - )) - ->addPropertySection(pht('Description'), $detail_list) - ->addPropertySection(pht('Details'), $details) - ->setCurtain($curtain); + ->setCurtain($curtain) + ->setMainColumn( + array( + $unpublished_panel, + $error_panel, + $description_box, + $detail_box, + $timeline, + $merge_table, + $info_panel, + )) + ->setFooter( + array( + $change_table, + $change_list, + $add_comment, + )); $page = $this->newPage() ->setTitle($commit->getDisplayName()) diff --git a/src/applications/diffusion/editor/DiffusionCommitEditEngine.php b/src/applications/diffusion/editor/DiffusionCommitEditEngine.php index 84a389a421..5b7277000f 100644 --- a/src/applications/diffusion/editor/DiffusionCommitEditEngine.php +++ b/src/applications/diffusion/editor/DiffusionCommitEditEngine.php @@ -113,30 +113,6 @@ final class DiffusionCommitEditEngine ->setConduitTypeDescription(pht('New auditors.')) ->setValue($object->getAuditorPHIDsForEdit()); - $holds = $data->getPublisherHoldReasons(); - if ($holds) { - $hold_names = array(); - foreach ($holds as $hold) { - $hold_names[] = id(new PhabricatorRepositoryPublisher()) - ->getHoldName($hold); - } - $desc = implode('; ', $hold_names); - - $doc_href = PhabricatorEnv::getDoclink( - 'Diffusion User Guide: Permanent Refs'); - $doc_link = phutil_tag( - 'a', - array( - 'href' => $doc_href, - 'target' => '_blank', - ), - pht('Learn More')); - - $fields[] = id(new PhabricatorStaticEditField()) - ->setLabel(pht('Unpublished')) - ->setValue(array($desc, " \xC2\xB7 ", $doc_link)); - } - $actions = DiffusionCommitActionTransaction::loadAllActions(); $actions = msortv($actions, 'getCommitActionOrderVector'); diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php index bc555a4fb4..2854aecbdd 100644 --- a/src/applications/diffusion/query/DiffusionCommitQuery.php +++ b/src/applications/diffusion/query/DiffusionCommitQuery.php @@ -15,6 +15,7 @@ final class DiffusionCommitQuery private $statuses; private $packagePHIDs; private $unreachable; + private $unpublished; private $needAuditRequests; private $needAuditAuthority; @@ -153,6 +154,11 @@ final class DiffusionCommitQuery return $this; } + public function withUnpublished($unpublished) { + $this->unpublished = $unpublished; + return $this; + } + public function withStatuses(array $statuses) { $this->statuses = $statuses; return $this; @@ -853,6 +859,21 @@ final class DiffusionCommitQuery } } + if ($this->unpublished !== null) { + if ($this->unpublished) { + $where[] = qsprintf( + $conn, + '(commit.importStatus & %d) = 0', + PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE); + } else { + $where[] = qsprintf( + $conn, + '(commit.importStatus & %d) = %d', + PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE, + PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE); + } + } + return $where; } diff --git a/src/applications/diffusion/view/DiffusionHistoryView.php b/src/applications/diffusion/view/DiffusionHistoryView.php index d7ae662ab6..9fd760b2cc 100644 --- a/src/applications/diffusion/view/DiffusionHistoryView.php +++ b/src/applications/diffusion/view/DiffusionHistoryView.php @@ -98,6 +98,13 @@ abstract class DiffusionHistoryView extends DiffusionView { foreach ($history as $item) { $commit = $item->getCommit(); if ($commit) { + + // NOTE: The "commit" objects in the history list may be undiscovered, + // and thus not yet have PHIDs. Only load data for commits with PHIDs. + if (!$commit->getPHID()) { + continue; + } + $commits[] = $commit; } } diff --git a/src/applications/repository/query/PhabricatorRepositoryPublisher.php b/src/applications/repository/query/PhabricatorRepositoryPublisher.php index 43feaf6bf0..ad374bd034 100644 --- a/src/applications/repository/query/PhabricatorRepositoryPublisher.php +++ b/src/applications/repository/query/PhabricatorRepositoryPublisher.php @@ -89,32 +89,4 @@ final class PhabricatorRepositoryPublisher return $reasons; } -/* -( Rendering )---------------------------------------------------------- */ - - public function getHoldName($hold) { - $map = array( - self::HOLD_IMPORTING => array( - 'name' => pht('Repository Importing'), - ), - self::HOLD_PUBLISHING_DISABLED => array( - 'name' => pht('Repository Publishing Disabled'), - ), - self::HOLD_REF_NOT_BRANCH => array( - 'name' => pht('Not a Branch'), - ), - self::HOLD_NOT_REACHABLE_FROM_PERMANENT_REF => array( - 'name' => pht('Not Reachable from Permanent Ref'), - ), - self::HOLD_UNTRACKED => array( - 'name' => pht('Untracked Ref'), - ), - self::HOLD_NOT_PERMANENT_REF => array( - 'name' => pht('Not a Permanent Ref'), - ), - ); - - $spec = idx($map, $hold, array()); - return idx($spec, 'name', pht('Unknown ("%s")', $hold)); - } - } diff --git a/src/applications/repository/query/PhabricatorRepositoryPublisherHoldReason.php b/src/applications/repository/query/PhabricatorRepositoryPublisherHoldReason.php new file mode 100644 index 0000000000..bdf776c83a --- /dev/null +++ b/src/applications/repository/query/PhabricatorRepositoryPublisherHoldReason.php @@ -0,0 +1,78 @@ +key = $key; + $hold->spec = $spec; + + return $hold; + } + + private static function getSpecForHoldKey($key) { + $specs = self::getHoldReasonSpecs(); + + $spec = idx($specs, $key); + + if (!$spec) { + $spec = array( + 'name' => pht('Unknown Hold ("%s")', $key), + ); + } + + return $spec; + } + + public function getName() { + return $this->getProperty('name'); + } + + public function getSummary() { + return $this->getProperty('summary'); + } + + private function getProperty($key, $default = null) { + return idx($this->spec, $key, $default); + } + + private static function getHoldReasonSpecs() { + $map = array( + PhabricatorRepositoryPublisher::HOLD_IMPORTING => array( + 'name' => pht('Repository Importing'), + 'summary' => pht('This repository is still importing.'), + ), + PhabricatorRepositoryPublisher::HOLD_PUBLISHING_DISABLED => array( + 'name' => pht('Publishing Disabled'), + 'summary' => pht('All publishing is disabled for this repository.'), + ), + PhabricatorRepositoryPublisher::HOLD_NOT_REACHABLE_FROM_PERMANENT_REF => + array( + 'name' => pht('Not On Permanent Ref'), + 'summary' => pht( + 'This commit is not an ancestor of any permanent ref.'), + ), + PhabricatorRepositoryPublisher::HOLD_REF_NOT_BRANCH => array( + 'name' => pht('Not a Branch'), + 'summary' => pht('This ref is not a branch.'), + ), + PhabricatorRepositoryPublisher::HOLD_UNTRACKED => array( + 'name' => pht('Untracked Ref'), + 'summary' => pht('This ref is configured as untracked.'), + ), + PhabricatorRepositoryPublisher::HOLD_NOT_PERMANENT_REF => array( + 'name' => pht('Not a Permanent Ref'), + 'summary' => pht('This ref is not configured as a permanent ref.'), + ), + ); + + return $map; + } + +} diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommitData.php b/src/applications/repository/storage/PhabricatorRepositoryCommitData.php index e4dd62a5f8..2f2fc3986f 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommitData.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommitData.php @@ -68,7 +68,7 @@ final class PhabricatorRepositoryCommitData extends PhabricatorRepositoryDAO { ->loadFromArray($dict); } - public function getPublisherHoldReasons() { + public function newPublisherHoldReasons() { $holds = $this->getCommitDetail('holdReasons'); // Look for the legacy "autocloseReason" if we don't have a modern list @@ -84,7 +84,12 @@ final class PhabricatorRepositoryCommitData extends PhabricatorRepositoryDAO { $holds = array(); } - return $holds; + foreach ($holds as $key => $reason) { + $holds[$key] = PhabricatorRepositoryPublisherHoldReason::newForHoldKey( + $reason); + } + + return array_values($holds); } } diff --git a/src/view/phui/PHUIInfoView.php b/src/view/phui/PHUIInfoView.php index af984f583e..cefeedbe5f 100644 --- a/src/view/phui/PHUIInfoView.php +++ b/src/view/phui/PHUIInfoView.php @@ -147,7 +147,7 @@ final class PHUIInfoView extends AphrontTagView { } $title = $this->title; - if (strlen($title)) { + if ($title || strlen($title)) { $title = phutil_tag( 'h1', array(