Navigation: Unable to add a Settings item

[psealock]

Discovered in https://github.com/woocommerce/woocommerce-bookings/pull/3001, A Settings item does not show up because its discarded in the getMenuItemsByCategory function.

To reproduce

Add an item in Settings:

Menu::add_plugin_item(
    array(
        'id'            => 'woocommerce-bookings-settings',
        'title'         => __( 'Bookings', 'woocommerce-bookings' ),
        'capability'    => 'read_global_availability',
        'url'           => 'edit.php?post_type=wc_booking&page=wc_bookings_settings',
        'parent'        => 'woocommerce-settings',
    )
);

This results in an item in wcNavigation with menuId of plugins.

getMenuItemsByCategory filters out items belonging to a parent that do not also have the same menuId. In this case, Settings has a menuId of secondary. This seems weird to me. A comment here would be useful.

https://github.com/woocommerce/woocommerce-admin/blob/d591271d19e18eceaf760bf8467dc2a03db9d603/client/navigation/components/container/index.js#L87-L93

Deliverables

• Adding an item with 'parent' => 'woocommerce-settings', places an item in the Settings nav
• Settings items have no menuId separation and should all be in the same section

[psealock]

One easy fix I can think of is to explicitly set menuId to secondary for items registering with a parent of woocommerce-settings. Right now that is forced to plugins.

[joshuatf]

getMenuItemsByCategory filters out items belonging to a parent that do not also have the same menuId. In this case, Settings has a menuId of secondary. This seems weird to me. A comment here would be useful.

I feel like this is expected behavior. We don't want to allow items to arbitrarily register in the core menus. Either these categories are walled off or they're not, otherwise this is too easy to circumvent.

I think the fix for this is either:

• We allow items to be registered under core categories (bypassing add_plugin_item).
• The item attempting to be registered in Settings properly registers via WC Settings.

[joelclimbsthings]

I'd tend to agree that this seems like expected behavior, although I also agree that it's unintuitive as it is. I keep losing track on what exactly we do want to be walled off; I'm collecting that we want core menus ("primary" and "secondary") mostly walled off, with the possible exceptions of items under Analytics & Settings?

In the case that we do want a 3PD to be able to add an item under Settings, I feel that addPluginItem() is an odd place to do it. If adding a Settings page is a common, supported use case, perhaps having a more explicit addPluginSetting() hook available would do the trick?

[psealock]

I feel like this is expected behavior.

Being able to add to Settings is the only Core menu extensions are allowed to add to. The exception being WCA extensions because they can add to Analytics because they've had that ability from the beginning of WCA.

I think the fix for this is either:

We allow items to be registered under core categories (bypassing add_plugin_item).

No, just Settings.

The item attempting to be registered in Settings properly registers via WC Settings.

I don't think thats great either. Lets look at Bookings as an example. Here is what the current menu looks like:

Their "Settings" page is independent of WC Settings but in the new Nav belongs under the Settings menu. I don't think they should have to do anything differently with their pages to accomplish this.

addPluginSetting

I like this because its explicit. How about add_plugin_setting_item?

[joelclimbsthings]

Sounds good @psealock.

One last question.

Do we want the ability to add a subcategory under Settings as well? In which case we'd need a add_plugin_settings_category() too. We don't have any sub-categories with Settings currently as far as I can tell, so it might be simpler to just restrict this to individual items.

[joshuatf]

Let's take a step back for a second. I think I'm confused over what we're expecting add_plugin_x to do. This IMO is a method to add items to the plugin menu, not to the core menu.

If we want to open up the gates for specific categories in the core menu, then I think we should discuss the right way to do that. But this logic of adding to the plugin menu and then showing up under certain core settings is very confusing to me.

[joelclimbsthings]

Let's take a step back for a second. I think I'm confused over what we're expecting add_plugin_x to do. This IMO is a method to add items to the plugin menu, not to the core menu.

That was my first thought as well @joshuatf , although I suppose it just depends on how you frame it. The add_plugin_x() convention could pertain to where the item is added, or what is doing the adding. Are we just providing a set of hooks for only plugins to use? If so, the use of add_plugin_x still makes sense to me even if the item is being posted to Settings instead of the actual "plugins" menu.

Are we providing an API that is intended to be more generic than that? Then maybe we could just rename the hook to make more sense, such as add_setting() (although that seems too general now).

With all this in mind, it could make more sense to change the convention entirely to be specific to the navigation:

nav_add_plugin_item()

nav_add_plugin_category()

nav_add_setting_item()

nav_add_setting_category() //If we want this ability

[psealock]

But this logic of adding to the plugin menu and then showing up under certain core settings is very confusing to me.

add_plugin_item accepts 'parent' => 'woocommerce-settings',. Can we use that instead?

Allowing extensions to register under Settings is a requirement of the Nav