When picking a default menu item to render, don't pick disabled items

Summary:
Depends on D20358. Fixes T12871. After refactoring, we can now tell when a "storage" menu item generated only disabled "display" menu items, and not pick any of them as the default rendering.

This means that if you're looking at a portal/menu with several dashboards, but can't see some at the top, you'll get the first one you can see.

Also clean up a lot of minor issues with less-common states.

Test Plan:
  - Created a portal with two private dashboards and a public dashboard.
  - Viewed it as another user, saw the default view show the dashboard I can actually see.
  - Minor fix: Disabled and enabled the hard-coded "Home" item, now worked cleanly with the right menu state.
  - Minor fix: added a motivator panel.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12871

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

src/applications/dashboard/engine/PhabricatorDashboardPortalProfileMenuEngine.php

@@ -30,10 +30,21 @@ final class PhabricatorDashboardPortalProfileMenuEngine
     return $items;
   }
 
-  protected function newNoMenuItemsView() {
-    return $this->newEmptyView(
-      pht('New Portal'),
-      pht('Use "Edit Menu" to add menu items to this portal.'));
+  protected function newNoMenuItemsView(array $items) {
+    $object = $this->getProfileObject();
+    $builtins = $this->getBuiltinProfileItems($object);
+
+    if (count($items) <= count($builtins)) {
+      return $this->newEmptyView(
+        pht('New Portal'),
+        pht('Use "Edit Menu" to add menu items to this portal.'));
+    } else {
+      return $this->newEmptyValue(
+        pht('No Portal Content'),
+        pht(
+          'None of the visible menu items in this portal can render any '.
+          'content.'));
+    }
   }
 
 }

src/applications/search/engine/PhabricatorProfileMenuEngine.php

@@ -142,10 +142,14 @@ abstract class PhabricatorProfileMenuEngine extends Phobject {
 
     $view_list = $this->newProfileMenuItemViewList();
 
-    $selected_item = $this->selectItem(
-      $view_list,
-      $item_id,
-      $is_view);
+    if ($is_view) {
+      $selected_item = $this->selectViewItem($view_list, $item_id);
+    } else {
+      if (!strlen($item_id)) {
+        $item_id = self::ITEM_MANAGE;
+      }
+      $selected_item = $this->selectEditItem($view_list, $item_id);
+    }
 
     switch ($item_action) {
       case 'view':
@@ -177,8 +181,6 @@ abstract class PhabricatorProfileMenuEngine extends Phobject {
     $crumbs = $controller->buildApplicationCrumbsForEditEngine();
 
     if (!$is_view) {
-      $navigation->selectFilter(self::ITEM_MANAGE);
-
       if ($selected_item) {
         if ($selected_item->getCustomPHID()) {
           $edit_mode = 'custom';
@@ -222,7 +224,7 @@ abstract class PhabricatorProfileMenuEngine extends Phobject {
 
           $crumbs->addTextCrumb($selected_item->getDisplayName());
         } else {
-          $content = $this->newNoMenuItemsView();
+          $content = $this->newNoContentView($this->getItems());
         }
 
         if (!$content) {
@@ -320,8 +322,6 @@ abstract class PhabricatorProfileMenuEngine extends Phobject {
     return $page;
   }
 
-
-
   private function getItems() {
     if ($this->items === null) {
       $this->items = $this->loadItems(self::MODE_COMBINED);
@@ -1258,10 +1258,10 @@ abstract class PhabricatorProfileMenuEngine extends Phobject {
         ));
   }
 
-  protected function newNoMenuItemsView() {
+  protected function newNoContentView(array $items) {
     return $this->newEmptyView(
-      pht('No Menu Items'),
-      pht('There are no menu items.'));
+      pht('No Content'),
+      pht('No visible menu items can render content.'));
   }
 
 
@@ -1298,15 +1298,13 @@ abstract class PhabricatorProfileMenuEngine extends Phobject {
     return $view_list;
   }
 
-  private function selectItem(
+  private function selectViewItem(
     PhabricatorProfileMenuItemViewList $view_list,
-    $item_id,
-    $want_default) {
+    $item_id) {
 
     // Figure out which view's content we're going to render. In most cases,
     // the URI tells us. If we don't have an identifier in the URI, we'll
-    // render the default view instead if this is a workflow that falls back
-    // to default rendering.
+    // render the default view instead.
 
     $selected_view = null;
     if (strlen($item_id)) {
@@ -1315,11 +1313,9 @@ abstract class PhabricatorProfileMenuEngine extends Phobject {
         $selected_view = head($item_views);
       }
     } else {
-      if ($want_default) {
-        $default_views = $view_list->getDefaultViews();
-        if ($default_views) {
-          $selected_view = head($default_views);
-        }
+      $default_views = $view_list->getDefaultViews();
+      if ($default_views) {
+        $selected_view = head($default_views);
       }
     }
 
@@ -1333,5 +1329,32 @@ abstract class PhabricatorProfileMenuEngine extends Phobject {
     return $selected_item;
   }
 
+  private function selectEditItem(
+    PhabricatorProfileMenuItemViewList $view_list,
+    $item_id) {
+
+    // First, try to select a visible item using the normal view selection
+    // pathway. If this works, it also highlights the menu properly.
+
+    if ($item_id) {
+      $selected_item = $this->selectViewItem($view_list, $item_id);
+      if ($selected_item) {
+        return $selected_item;
+      }
+    }
+
+    // If we didn't find an item in the view list, we may be enabling an item
+    // which is currently disabled or editing an item which is not generating
+    // any actual items in the menu.
+
+    foreach ($this->getItems() as $item) {
+      if ($item->matchesIdentifier($item_id)) {
+        return $item;
+      }
+    }
+
+    return null;
+  }
+
 
 }

src/applications/search/engine/PhabricatorProfileMenuItemViewList.php

@@ -62,34 +62,15 @@ final class PhabricatorProfileMenuItemViewList
   public function getViewsWithItemIdentifier($identifier) {
     $views = $this->getItemViews();
 
-    if (!strlen($identifier)) {
-      return array();
-    }
-
-    if (ctype_digit($identifier)) {
-      $identifier_int = (int)$identifier;
-    } else {
-      $identifier_int = null;
-    }
-
-    $identifier_str = (string)$identifier;
-
     $results = array();
     foreach ($views as $view) {
       $config = $view->getMenuItemConfiguration();
 
-      if ($identifier_int !== null) {
-        $config_id = (int)$config->getID();
-        if ($config_id === $identifier_int) {
-          $results[] = $view;
-          continue;
-        }
-      }
-
-      if ($config->getBuiltinKey() === $identifier_str) {
-        $results[] = $view;
+      if (!$config->matchesIdentifier($identifier)) {
         continue;
       }
+
+      $results[] = $view;
     }
 
     return $results;
@@ -112,6 +93,13 @@ final class PhabricatorProfileMenuItemViewList
       }
     }
 
+    // Remove disabled views.
+    foreach ($views as $key => $view) {
+      if ($view->getDisabled()) {
+        unset($views[$key]);
+      }
+    }
+
     // If this engine supports pinning items and we have candidate views from a
     // valid pinned item, they are the default views.
     if ($can_pin) {

src/applications/search/menuitem/PhabricatorMotivatorProfileMenuItem.php

@@ -70,7 +70,7 @@ final class PhabricatorMotivatorProfileMenuItem
       ->setName($fact_name)
       ->setIcon($fact_icon)
       ->setTooltip($fact_text)
-      ->setHref('#');
+      ->setURI('#');
 
     return array(
       $item,

src/applications/search/storage/PhabricatorProfileMenuItemConfiguration.php

@@ -237,6 +237,24 @@ final class PhabricatorProfileMenuItemConfiguration
     return $this->isTailItem;
   }
 
+  public function matchesIdentifier($identifier) {
+    if (!strlen($identifier)) {
+      return false;
+    }
+
+    if (ctype_digit($identifier)) {
+      if ((int)$this->getID() === (int)$identifier) {
+        return true;
+      }
+    }
+
+    if ((string)$this->getBuiltinKey() === (string)$identifier) {
+      return true;
+    }
+
+    return false;
+  }
+
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */