From c9be5fef2783336e38628e67a4a4c7823c0b7802 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 Jun 2015 08:53:28 -0700 Subject: [PATCH 1/2] Show spaces in policy explanation dialogs Summary: Ref T8449. When an object is in a space, explain that clearly in the policy description dialog. Test Plan: {F496126} Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T8449 Differential Revision: https://secure.phabricator.com/D13264 --- .../PhabricatorPolicyExplainController.php | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/applications/policy/controller/PhabricatorPolicyExplainController.php b/src/applications/policy/controller/PhabricatorPolicyExplainController.php index 09fd69242a..4224319d9c 100644 --- a/src/applications/policy/controller/PhabricatorPolicyExplainController.php +++ b/src/applications/policy/controller/PhabricatorPolicyExplainController.php @@ -69,7 +69,27 @@ final class PhabricatorPolicyExplainController $capability_name = $capobj->getCapabilityName(); } + $space_info = null; + if ($object instanceof PhabricatorSpacesInterface) { + if (PhabricatorSpacesNamespaceQuery::getViewerSpacesExist($viewer)) { + $space_phid = PhabricatorSpacesNamespaceQuery::getObjectSpacePHID( + $object); + + $handles = $viewer->loadHandles(array($space_phid)); + + $space_info = array( + pht( + 'This object is in %s, and can only be seen by users with '. + 'access to that space.', + $handles[$space_phid]->renderLink()), + phutil_tag('br'), + phutil_tag('br'), + ); + } + } + $content = array( + $space_info, pht('Users with the "%s" capability:', $capability_name), $auto_info, ); From 2a5cf5e1b76c7f375326e2b9eb1239b959d77138 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 Jun 2015 08:53:40 -0700 Subject: [PATCH 2/2] Separate "Revision" and "Diff" fields in Differential Summary: Fixes T5138. Some of the "revision" properties are really "diff" properties, but we only show the properties for the most recent / current diff. - Immediately, this makes it hard or impossible to review, e.g., lint/unit results for older diffs. - Longer-term, these limits will become more problematic with more data on diffs after Harbormaster. Instead, separate "revision" from "diff" properties. (In the long term, it might make sense to show more diffs in this panel -- e.g., tabs for the 8 most recent updates or something -- but I went with the simplest approach for now since I don't have a clean way to deal with 100-update revisions offhand.) Test Plan: {F500480} Reviewers: btrahan Reviewed By: btrahan Subscribers: cburroughs, epriestley Maniphest Tasks: T5138 Differential Revision: https://secure.phabricator.com/D13282 --- .../PhabricatorDifferentialConfigOptions.php | 4 +- .../DifferentialRevisionViewController.php | 108 ++++++++++++++---- .../customfield/DifferentialBranchField.php | 14 ++- .../customfield/DifferentialCustomField.php | 45 ++++++-- .../customfield/DifferentialHostField.php | 14 ++- .../customfield/DifferentialLintField.php | 29 ++++- .../customfield/DifferentialPathField.php | 14 ++- .../DifferentialRepositoryField.php | 22 ++-- .../customfield/DifferentialUnitField.php | 27 ++++- .../event/HarbormasterUIEventListener.php | 7 ++ 10 files changed, 224 insertions(+), 60 deletions(-) diff --git a/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php b/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php index 2c8f4e0bd9..7d0eae99c2 100644 --- a/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php +++ b/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php @@ -34,9 +34,7 @@ final class PhabricatorDifferentialConfigOptions new DifferentialReviewedByField(), new DifferentialSubscribersField(), new DifferentialRepositoryField(), - new DifferentialLintField(), new DifferentialProjectsField(), - new DifferentialUnitField(), new DifferentialViewPolicyField(), new DifferentialEditPolicyField(), @@ -54,6 +52,8 @@ final class PhabricatorDifferentialConfigOptions new DifferentialBlameRevisionField(), new DifferentialPathField(), new DifferentialHostField(), + new DifferentialLintField(), + new DifferentialUnitField(), new DifferentialRevertPlanField(), new DifferentialApplyPatchField(), diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 7069e5ae97..be61b6c153 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -206,27 +206,6 @@ final class DifferentialRevisionViewController extends DifferentialController { $visible_changesets = $changesets; } - - // TODO: This should be in a DiffQuery or similar. - $need_props = array(); - foreach ($field_list->getFields() as $field) { - foreach ($field->getRequiredDiffPropertiesForRevisionView() as $prop) { - $need_props[$prop] = $prop; - } - } - - if ($need_props) { - $prop_diff = $revision->getActiveDiff(); - $load_props = id(new DifferentialDiffProperty())->loadAllWhere( - 'diffID = %d AND name IN (%Ls)', - $prop_diff->getID(), - $need_props); - $load_props = mpull($load_props, 'getData', 'getName'); - foreach ($need_props as $need) { - $prop_diff->attachProperty($need, idx($load_props, $need)); - } - } - $commit_hashes = mpull($diffs, 'getSourceControlBaseRevision'); $local_commits = idx($props, 'local:commits', array()); foreach ($local_commits as $local_commit) { @@ -286,6 +265,11 @@ final class DifferentialRevisionViewController extends DifferentialController { $revision_detail_box->setInfoView($revision_warnings); } + $diff_detail_box = $this->buildDiffDetailView( + array_select_keys($diffs, array($diff_vs, $target->getID())), + $revision, + $field_list); + $comment_view = $this->buildTransactions( $revision, $diff_vs ? $diffs[$diff_vs] : $target, @@ -486,6 +470,7 @@ final class DifferentialRevisionViewController extends DifferentialController { $content = array( $revision_detail_box, + $diff_detail_box, $page_pane, ); @@ -974,4 +959,85 @@ final class DifferentialRevisionViewController extends DifferentialController { return $warnings; } + private function buildDiffDetailView( + array $diffs, + DifferentialRevision $revision, + PhabricatorCustomFieldList $field_list) { + $viewer = $this->getViewer(); + + $fields = array(); + foreach ($field_list->getFields() as $field) { + if ($field->shouldAppearInDiffPropertyView()) { + $fields[] = $field; + } + } + + if (!$fields) { + 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()); + } + + $property_lists[] = array( + $label, + $this->buildDiffPropertyList($diff, $revision, $fields), + ); + } + + $box = id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Diff Detail')) + ->setUser($viewer); + + $last_tab = null; + foreach ($property_lists as $key => $property_list) { + list($tab_name, $list_view) = $property_list; + + $tab = id(new PHUIListItemView()) + ->setKey($key) + ->setName($tab_name); + + $box->addPropertyList($list_view, $tab); + $last_tab = $tab; + } + + if ($last_tab) { + $last_tab->setSelected(true); + } + + return $box; + } + + private function buildDiffPropertyList( + DifferentialDiff $diff, + DifferentialRevision $revision, + array $fields) { + $viewer = $this->getViewer(); + + $view = id(new PHUIPropertyListView()) + ->setUser($viewer) + ->setObject($diff); + + foreach ($fields as $field) { + $label = $field->renderDiffPropertyViewLabel($diff); + $value = $field->renderDiffPropertyViewValue($diff); + if ($value !== null) { + $view->addProperty($label, $value); + } + } + + return $view; + } + + } diff --git a/src/applications/differential/customfield/DifferentialBranchField.php b/src/applications/differential/customfield/DifferentialBranchField.php index 35ed809f12..fc50943f1d 100644 --- a/src/applications/differential/customfield/DifferentialBranchField.php +++ b/src/applications/differential/customfield/DifferentialBranchField.php @@ -19,12 +19,20 @@ final class DifferentialBranchField return true; } - public function renderPropertyViewLabel() { + public function renderPropertyViewValue(array $handles) { + return null; + } + + public function shouldAppearInDiffPropertyView() { + return true; + } + + public function renderDiffPropertyViewLabel(DifferentialDiff $diff) { return $this->getFieldName(); } - public function renderPropertyViewValue(array $handles) { - return $this->getBranchDescription($this->getObject()->getActiveDiff()); + public function renderDiffPropertyViewValue(DifferentialDiff $diff) { + return $this->getBranchDescription($diff); } private function getBranchDescription(DifferentialDiff $diff) { diff --git a/src/applications/differential/customfield/DifferentialCustomField.php b/src/applications/differential/customfield/DifferentialCustomField.php index 088e7beeac..b4ef47c274 100644 --- a/src/applications/differential/customfield/DifferentialCustomField.php +++ b/src/applications/differential/customfield/DifferentialCustomField.php @@ -2,6 +2,7 @@ /** * @task commitmessage Integration with Commit Messages + * @task diff Integration with Diff Properties */ abstract class DifferentialCustomField extends PhabricatorCustomField { @@ -31,13 +32,6 @@ abstract class DifferentialCustomField return parent::shouldEnableForRole($role); } - public function getRequiredDiffPropertiesForRevisionView() { - if ($this->getProxy()) { - return $this->getProxy()->getRequiredDiffPropertiesForRevisionView(); - } - return array(); - } - protected function parseObjectList( $value, array $types, @@ -82,6 +76,7 @@ abstract class DifferentialCustomField return array(); } + /* -( Integration with Commit Messages )----------------------------------- */ @@ -217,4 +212,40 @@ abstract class DifferentialCustomField return; } + +/* -( Integration with Diff Properties )----------------------------------- */ + + + /** + * @task diff + */ + public function shouldAppearInDiffPropertyView() { + if ($this->getProxy()) { + return $this->getProxy()->shouldAppearInDiffPropertyView(); + } + return false; + } + + + /** + * @task diff + */ + public function renderDiffPropertyViewLabel(DifferentialDiff $diff) { + if ($this->proxy) { + return $this->proxy->renderDiffPropertyViewLabel($diff); + } + return $this->getFieldName(); + } + + + /** + * @task diff + */ + public function renderDiffPropertyViewValue(DifferentialDiff $diff) { + if ($this->proxy) { + return $this->proxy->renderDiffPropertyViewValue($diff); + } + throw new PhabricatorCustomFieldImplementationIncompleteException($this); + } + } diff --git a/src/applications/differential/customfield/DifferentialHostField.php b/src/applications/differential/customfield/DifferentialHostField.php index 2c5c65b8c1..af71dcf987 100644 --- a/src/applications/differential/customfield/DifferentialHostField.php +++ b/src/applications/differential/customfield/DifferentialHostField.php @@ -23,12 +23,20 @@ final class DifferentialHostField return true; } - public function renderPropertyViewLabel() { + public function renderPropertyViewValue(array $handles) { + return null; + } + + public function shouldAppearInDiffPropertyView() { + return true; + } + + public function renderDiffPropertyViewLabel(DifferentialDiff $diff) { return $this->getFieldName(); } - public function renderPropertyViewValue(array $handles) { - $host = $this->getObject()->getActiveDiff()->getSourceMachine(); + public function renderDiffPropertyViewValue(DifferentialDiff $diff) { + $host = $diff->getSourceMachine(); if (!$host) { return null; } diff --git a/src/applications/differential/customfield/DifferentialLintField.php b/src/applications/differential/customfield/DifferentialLintField.php index c43034dd06..94df1b7e07 100644 --- a/src/applications/differential/customfield/DifferentialLintField.php +++ b/src/applications/differential/customfield/DifferentialLintField.php @@ -19,20 +19,37 @@ final class DifferentialLintField return true; } - public function renderPropertyViewLabel() { + public function renderPropertyViewValue(array $handles) { + return null; + } + + public function shouldAppearInDiffPropertyView() { + return true; + } + + public function renderDiffPropertyViewLabel(DifferentialDiff $diff) { return $this->getFieldName(); } - public function getRequiredDiffPropertiesForRevisionView() { - return array( + public function renderDiffPropertyViewValue(DifferentialDiff $diff) { + // TODO: This load is slightly inefficient, but most of this is moving + // to Harbormaster and this simplifies the transition. Eat 1-2 extra + // queries for now. + $keys = array( 'arc:lint', 'arc:lint-excuse', 'arc:lint-postponed', ); - } - public function renderPropertyViewValue(array $handles) { - $diff = $this->getObject()->getActiveDiff(); + $properties = id(new DifferentialDiffProperty())->loadAllWhere( + 'diffID = %d AND name IN (%Ls)', + $diff->getID(), + $keys); + $properties = mpull($properties, 'getData', 'getName'); + + foreach ($keys as $key) { + $diff->attachProperty($key, idx($properties, $key)); + } $path_changesets = mpull($diff->loadChangesets(), 'getID', 'getFilename'); diff --git a/src/applications/differential/customfield/DifferentialPathField.php b/src/applications/differential/customfield/DifferentialPathField.php index 1eb0ffbf50..39ca8abd50 100644 --- a/src/applications/differential/customfield/DifferentialPathField.php +++ b/src/applications/differential/customfield/DifferentialPathField.php @@ -23,12 +23,20 @@ final class DifferentialPathField return true; } - public function renderPropertyViewLabel() { + public function renderPropertyViewValue(array $handles) { + return null; + } + + public function shouldAppearInDiffPropertyView() { + return true; + } + + public function renderDiffPropertyViewLabel(DifferentialDiff $diff) { return $this->getFieldName(); } - public function renderPropertyViewValue(array $handles) { - $path = $this->getObject()->getActiveDiff()->getSourcePath(); + public function renderDiffPropertyViewValue(DifferentialDiff $diff) { + $path = $diff->getSourcePath(); if (!$path) { return null; } diff --git a/src/applications/differential/customfield/DifferentialRepositoryField.php b/src/applications/differential/customfield/DifferentialRepositoryField.php index fc6c88194a..7f39d25823 100644 --- a/src/applications/differential/customfield/DifferentialRepositoryField.php +++ b/src/applications/differential/customfield/DifferentialRepositoryField.php @@ -125,20 +125,24 @@ final class DifferentialRepositoryField return true; } - public function renderPropertyViewLabel() { + public function renderPropertyViewValue(array $handles) { + return null; + } + + public function shouldAppearInDiffPropertyView() { + return true; + } + + public function renderDiffPropertyViewLabel(DifferentialDiff $diff) { return $this->getFieldName(); } - public function getRequiredHandlePHIDsForPropertyView() { - $repository_phid = $this->getObject()->getRepositoryPHID(); - if ($repository_phid) { - return array($repository_phid); + public function renderDiffPropertyViewValue(DifferentialDiff $diff) { + if (!$diff->getRepositoryPHID()) { + return null; } - return array(); - } - public function renderPropertyViewValue(array $handles) { - return $this->renderHandleList($handles); + return $this->getViewer()->renderHandle($diff->getRepositoryPHID()); } public function shouldAppearInTransactionMail() { diff --git a/src/applications/differential/customfield/DifferentialUnitField.php b/src/applications/differential/customfield/DifferentialUnitField.php index 2cc86d2660..56ea06d3ae 100644 --- a/src/applications/differential/customfield/DifferentialUnitField.php +++ b/src/applications/differential/customfield/DifferentialUnitField.php @@ -19,19 +19,34 @@ final class DifferentialUnitField return true; } - public function renderPropertyViewLabel() { + public function renderPropertyViewValue(array $handles) { + return null; + } + + public function shouldAppearInDiffPropertyView() { + return true; + } + + public function renderDiffPropertyViewLabel(DifferentialDiff $diff) { return $this->getFieldName(); } - public function getRequiredDiffPropertiesForRevisionView() { - return array( + public function renderDiffPropertyViewValue(DifferentialDiff $diff) { + // TODO: See DifferentialLintField. + $keys = array( 'arc:unit', 'arc:unit-excuse', ); - } - public function renderPropertyViewValue(array $handles) { - $diff = $this->getObject()->getActiveDiff(); + $properties = id(new DifferentialDiffProperty())->loadAllWhere( + 'diffID = %d AND name IN (%Ls)', + $diff->getID(), + $keys); + $properties = mpull($properties, 'getData', 'getName'); + + foreach ($keys as $key) { + $diff->attachProperty($key, idx($properties, $key)); + } $ustar = DifferentialRevisionUpdateHistoryView::renderDiffUnitStar($diff); $umsg = DifferentialRevisionUpdateHistoryView::getDiffUnitMessage($diff); diff --git a/src/applications/harbormaster/event/HarbormasterUIEventListener.php b/src/applications/harbormaster/event/HarbormasterUIEventListener.php index 7df5c111bd..e965e67fb6 100644 --- a/src/applications/harbormaster/event/HarbormasterUIEventListener.php +++ b/src/applications/harbormaster/event/HarbormasterUIEventListener.php @@ -31,6 +31,13 @@ final class HarbormasterUIEventListener return; } + if ($object instanceof DifferentialRevision) { + // TODO: This is a bit hacky and we could probably find a cleaner fix + // eventually, but we show build status on each diff, immediately below + // this property list, so it's redundant to show it on the revision view. + return; + } + if (!($object instanceof HarbormasterBuildableInterface)) { return; }