Page MenuHomePhabricator

Add a basic ProjectProfileMenuItem
ClosedPublic

Authored by chad on Dec 10 2016, 4:12 AM.
Tags
None
Referenced Files
F12834152: D17021.id41040.diff
Thu, Mar 28, 2:28 PM
Unknown Object (File)
Thu, Mar 28, 1:02 AM
Unknown Object (File)
Tue, Mar 26, 7:19 PM
Unknown Object (File)
Fri, Mar 15, 10:45 PM
Unknown Object (File)
Sat, Mar 2, 4:13 PM
Unknown Object (File)
Feb 12 2024, 3:12 PM
Unknown Object (File)
Feb 10 2024, 3:07 AM
Unknown Object (File)
Feb 3 2024, 1:23 PM
Subscribers

Details

Summary

Allows you to name and set a project as a menu item navigation element.

Test Plan

Add a project, no name, see project. Remove. Add a project and give it a short name (bugs) and see project link.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

chad retitled this revision from to Add a basic ProjectProfilePanel.
chad updated this object.
chad edited the test plan for this revision. (Show Details)
chad added a reviewer: epriestley.
epriestley edited edge metadata.

The getProject() method is a performance issue: it issues an individual query for each project, which is much slower than issuing one query for all of the projects (roughly, this is the "N + 1 Queries" problem).

You can see the effects of this by adding 10 project items to a menu, then looking at the "Services" tab in DarkConsole. I expect there will be 30+ queries issued (each project will issue separate queries for the project itself, parents, images, and maybe other stuff).

Although most users probably won't add 10 or more projects to a menu, I think it's realistic to imagine that they might add a smaller number like 5 or 6 in many cases. We have at least one other panel today which suffers from this same inefficiency (PhabricatorProjectPointsProfilePanel) but in that case it's pointless to ever add more than one panel since all copies of that particular panel are always exactly identical, so users have no reason to run into this.

I think you can fix this by rewriting PhabricatorProfilePanelEngine->buildNavigation() to have a step where it allows each panel type to bulk-query objects. Currently, the method looks like this:

foreach ($panels as $panel) {
  if ($panel->isDisabled()) {
    continue;
  }

  $items = $panel->buildNavigationMenuItems();
  ...

Rewrite it to do this, instead:

  • First, throw out all the disabled panels.
  • Use mgroup to group the remaining panels by type.
  • For each group of panels of the same type, select the first one. Pass the whole group of panels to some new method like willBuildNavigationItems(array $panels).

Then provide an empty implementation of that method on the base class, and have this panel override the method. It should pull all of the project PHIDs off the panels, bulk-load them all in a single query, then attach the results back onto the panels.

When you go to render the actual items, use the attached results from the previous step instead of loading new results.

This should reduce the cost of an arbitrarily large number of projects down to a handful of queries.

src/applications/search/profilepanel/PhabricatorProjectProfilePanel.php
24 โ†—(On Diff #40948)

Maybe "getDisplayName()" so we get "Project (Milestone)" instead of just "Milestone", but I'm not sure that's necessarily better.

34 โ†—(On Diff #40948)

Maybe call this property projectPHID to be a little more explicit.

This revision now requires changes to proceed.Dec 10 2016, 5:05 AM

yeah makes sense, doesn't sound too bad

chad edited edge metadata.
  • Rebase, add Name
chad planned changes to this revision.Dec 14 2016, 9:11 PM

(still need to do perf)

chad retitled this revision from Add a basic ProjectProfilePanel to Add a basic ProjectProfileMenuItem.Dec 14 2016, 9:13 PM
chad edited the summary of this revision. (Show Details)
chad edited the test plan for this revision. (Show Details)
src/applications/search/engine/PhabricatorProfileMenuEngine.php
176

I'm not sure how to pull in the phids here without hardwiring a lot of stuff. Assume you have some abstraction in mind that's flexible?

src/applications/search/engine/PhabricatorProfileMenuEngine.php
176

Assume it needs to work for Dashboards as well.

Oh, sorry, I meant something more like this:

$filtered_items = // you're doing this right already
$filtered_groups = mgroup(...);

foreach ($filtered_groups as $group) {
  $first_item = head($group);
  $first_item->willBuildNavigationItems($group);
}

Then:

  • put an empty willBuildNavigationItems(array $items) on PhabricatorProfileMenuItem;
  • override it in PhabricatorProjectProfileMenuItem like this:
public function willBuildNavigationItems(array $items) {
  $project_phids = array();
  foreach ($items as $item) {
    $project_phids[] = $item->getMenuItemProperty('projectPHID');
  }

  $projects = // Now load all the projects.

  foreach ($items as $item) {
    $project = // ...
    $item->attachProject($project);
  }
}

put an empty willBuildNavigationItems(array $items) on PhabricatorProfileMenuItem;

So that method just needs to be public? I keep hitting Unable to resolve method 'willBuildNavigationItems'. and I've checked spelling like 20 times.

I have:

public function willBuildNavigationItems(array $items) {}

That looks right, show me the code?

Oh, right -- on PhabricatorProfileMenuItemConfiguration, add a method like this:

public function willBuildNavigationItems(array $stuff) {
  return $this->getMenuItem()->willBuildNavigationItems($stuff);
}

(It has a bunch of similar methods already, which mostly just pass stuff through from the MenuItemConfiguration to the underlying MenuItem objects.)

I don't really know what "attachProject" means. I'm super confused how many different places this gets built.

src/applications/search/storage/PhabricatorProfileMenuItemConfiguration.php
19

maybe all this goes up on PhabricatorProfileProfileMenuItem?

In PhabricatorProfileMenuEngine, we have a list of $menu_items. These are objects of class PhabricatorProfileMenuItemConfiguration which we (probably) loaded from the database.

Each PhabricatorProfileMenuItemConfiguration has an actual item implementation on it (which is some subclass of PhabricatorProfileMenuItem).

The PhabricatorProfileMenuItemConfiguration objects are generic objects which we save in the database, and have information like which menu they belong to, the order, and any configured stuff for the underlying actual item.

The actual items have different types (CatFacts, Link, Divider, etc).

So we might have something like this:

  • $menu_items
    • PhabricatorProfileMenuItemConfiguration
      • id: 1
      • order: 1
      • type: divider
      • menuItem: object of class PhabricatorDividerProfileMenuItem
    • PhabricatorProfileMenuItemConfiguration
      • id: 2
      • order: 2
      • type: link
      • properties: {name: "click me", href: "google.com"}
      • menuItem: object of class PhabricatorLinkProfileMenuItem

The generic PhabricatorProfileMenuItemConfiguration objects expose a bunch of methods which are really just wrappers around the specific item type's implementation. For example, methods like getDisplayName() and canMakeDefault() on PhabricatorProfileMenuItemConfiguration just ask the underlying PhabricatorProfileMenuItem for the result.

You can get the underlying object with PhabricatorProfileMenuItemConfiguration->getMenuItem().

The attachProject() stuff is headed in the right direction, but it should go on PhabricatorProjectProfileMenuItem (the class that specifically implements project items), not PhabricatorProfileMenuItemConfiguration (which is generic storage).

I think this will work if you make these changes:

  • Move the attachProject() stuff to PhabricatorProjectProfileMenuItem. (Note that you can't actually use self::ATTACHABLE since those classes do not extend LiskDAO -- you can use the names attachProject/getProject, but just implement them like a normal setter/getter).
  • When attaching, instead of calling $item->attachProject(...), call $item->getMenuItem()->attachProject(...).

Broadly, you're attaching projects to the generic storage container, but then trying to read them from the specific item implementation right now. Moving all the attaching/reading down a level to the specific item implementation should make things work.

$item->getMenuItem()->attachProject(...)

This was what I was missing.

Do these work OK?

Case 1:

  • Create a menu item for a project XYZ.
  • Destroy the project (bin/remove destroy XYZ).
  • Load a page with the menu item.

Case 2:

  • Create a menu item for project XYZ.
  • Change the policy for XYZ so you can't see it anymore.
  • Load a page with the menu item.
src/applications/search/engine/PhabricatorProfileMenuEngine.php
177โ€“193

Something isn't working here... it's attaching the same project to every project menu item. Can't figure this out.

Try this:

diff --git a/src/applications/search/query/PhabricatorProfileMenuItemConfigurationQuery.php b/src/applications/search/query/PhabricatorProfileMenuItemConfigurationQuery.php
index 2109ee1db..f8172638c 100644
--- a/src/applications/search/query/PhabricatorProfileMenuItemConfigurationQuery.php
+++ b/src/applications/search/query/PhabricatorProfileMenuItemConfigurationQuery.php
@@ -66,6 +66,7 @@ final class PhabricatorProfileMenuItemConfigurationQuery
         unset($page[$key]);
         continue;
       }
+      $item_type = clone $item_type;
       $item->attachMenuItem($item_type);
     }

Currently, since we don't do this kind of attachThing() on other types of MenuItems, we're attaching the same MenuItem to every MenuItemConfiguration of a given type. Adding a clone gives us a different MenuItem for each MenuItemConfiguration.

Screen Shot 2016-12-15 at 1.16.35 PM.png (303ร—463 px, 43 KB)

UNRECOVERABLE FATAL ERROR <<<

Call to a member function getName() on null

/var/www/html/dev/core/lib/phabricator/src/applications/search/menuitem/PhabricatorProjectProfileMenuItem.php:54

You can get a stack trace out of that like this:

diff --git a/src/applications/search/menuitem/PhabricatorProjectProfileMenuItem.php b/src/applications/search/menuitem/PhabricatorProjectProfileMenuItem.php
index c97a24775..365ef0298 100644
--- a/src/applications/search/menuitem/PhabricatorProjectProfileMenuItem.php
+++ b/src/applications/search/menuitem/PhabricatorProjectProfileMenuItem.php
@@ -51,6 +51,11 @@ final class PhabricatorProjectProfileMenuItem
       return $this->getName($config);
     } else {
       $project = $this->project;
+
+      if (!$project) {
+        throw new Exception("!!");
+      }
+
       return $project->getName();
     }
   }

That gives us this, at least for me::

Screen Shot 2016-12-15 at 1.48.10 PM.png (966ร—1 px, 199 KB)

So when building the configuration view, we don't have the project loaded.

It seems like we should, since in buildResponse() we call buildNavigation() -- which should load the projects -- before we call buildItemConfigureContent().

However, we actually use different lists of items for the two calls. Simple fix is this, I think:

diff --git a/src/applications/search/engine/PhabricatorProfileMenuEngine.php b/src/applications/search/engine/PhabricatorProfileMenuEngine.php
index 90de23f93..d930b3131 100644
--- a/src/applications/search/engine/PhabricatorProfileMenuEngine.php
+++ b/src/applications/search/engine/PhabricatorProfileMenuEngine.php
@@ -73,7 +73,7 @@ abstract class PhabricatorProfileMenuEngine extends Phobject {
     }
 
     $item_id = $request->getURIData('itemID');
-    $item_list = $this->loadItems();
+    $item_list = $this->getItems();
 
     $selected_item = null;
     if (strlen($item_id)) {

ah, good tip.

Whats.... the correct course of action here with visibility. I've got it wired that if it's missing, archived, or restricted, it does not display in the menu, but when you edit, it does say there is (restricted, archived, etc) one in the menu. This seems OK? I don't like the idea of having dead ends visible in the menu itself day to day.

I think showing some kind of "Deleted/Restricted Item" thing is definitely right for editing.

I could go either way on the main menu, but I think hiding it to start out with is reasonable. There's some risk we end up in cases like panels (T8033) where it's confusing why an item isn't showing up, but I think that's less likely with menus than with panels. If we get a bunch of admins not understanding why their junk isn't visible to users we could change it later.

  • test archive, hide, delete
epriestley edited edge metadata.
epriestley added inline comments.
src/applications/search/menuitem/PhabricatorProjectProfileMenuItem.php
32

Maybe OK to keep linking to archived projects (maybe give 'em a UI treatment down the road)?

This revision is now accepted and ready to land.Dec 15 2016, 10:26 PM
This revision was automatically updated to reflect the committed changes.

I failed this interview, didn't I.