refactor: wizards menu order handling

[miguelpeixe]

All Submissions:

• Have you followed the Newspack Contributing guideline?
• Does your code follow the WordPress' coding standards and VIP Go coding standards?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

I started investigating the different ways we're managing menu order for wizard, prompted by @ronchambers comment regarding the Newsletters' wizard menu order:

I was having a hard time setting the Newsletters CPT parent menu item to a decimal value. It seems that CPTs only support an integer value for menu_positon. So I created a function that will loop through global $menu and then move the CPT to position 3.3 or "3.33" if something is already at 3.3. See the current code in function move_cpt_menu(). You may want to look at function registered_post_type_newsletters( $post_type ) to see how I'm changing the menu_icon but when I try to change the menu_position to 3.3 in that function it doesn't work.

Turns out we have multiple strategies in place:

• hook priority
• add_menu_page() argument
• custom reordering logic

This is hard to maintain and unpredictable. In this refactor PR I propose a consolidated strategy applied in the class-wizards.php:

newspack-plugin/includes/class-wizards.php

Lines 141 to 172 in 406bc54

	/**
	* Update menu order for wizards with parent menu items.
	*
	* @param array $menu_order The current menu order.
	*
	* @return array The updated menu order.
	*/
	public static function menu_order( $menu_order ) {
	$index = array_search( 'newspack-dashboard', $menu_order, true );
	if ( false === $index ) {
	return $menu_order;
	}
	$ordered_wizards = [];
	foreach ( self::$wizards as $slug => $wizard ) {
	if ( ! empty( $wizard->parent_menu ) && ! empty( $wizard->menu_order ) ) {
	$ordered_wizards[ $wizard->menu_order ] = $wizard->parent_menu;
	}
	}
	if ( empty( $ordered_wizards ) ) {
	return $menu_order;
	}
	ksort( $ordered_wizards );
	foreach ( array_reverse( $ordered_wizards ) as $menu_item ) {
	$key = array_search( $menu_item, $menu_order, true );
	if ( false === $key ) {
	continue;
	}
	array_splice( $menu_order, $key, 1 );
	array_splice( $menu_order, $index + 1, 0, $menu_item );
	}
	return $menu_order;
	}

Each wizard with a parent menu item can use the public vars parent_menu and menu_order to determine how it should render relative to the newspack-dashboard wizard:

newspack-plugin/includes/wizards/class-network-wizard.php

Lines 33 to 45 in 406bc54

	/**
	* The parent menu item name.
	*
	* @var string
	*/
	public $parent_menu = 'newspack-network';
	/**
	* Order relative to the Newspack Dashboard menu item.
	*
	* @var int
	*/
	public $menu_order = 5;

Result:

This PR also deprecates the menu_priority variable from the abstract class. It's a bit misleading, as it only applies the hook priority to the admin_menu hook. This is not a guaranteed way of setting the menu order because it gets overridden by the add_menu_page argument.

How to test the changes in this Pull Request:

1. Checkout this PR, make sure you have Ads, Network, and Newsletters plugin active
2. Confirm the menu order reflects the image above
3. Click each menu and confirm it behaves as expected

Other information:

• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes, as applicable?
• Have you successfully ran tests with your changes locally?

[jaredrethman]

@miguelpeixe have tested the code and works as described.

Additionally: I really like your approach, first time i’ve come across custom_menu_order hook - looks like the right tool for the job!

Approved!

[miguelpeixe]

Thanks, @jaredrethman! Since this touches on a core aspect of the project, I'll also wait for @ronchambers feedback before merging.

[leogermani]

Not sure if this has any impact here, but for awareness: https://core.trac.wordpress.org/ticket/52035

I've been meaning to circle back to this and help it get to the finish line

[ronchambers]

I didn't test locally, but I trust the two of you 😃 Overall, I agree that moving the menus around for the 3 different plugins (Listings, Newsletters, and Network - my original tasks ) became cumbersome where each plugin had a different approach... So yes, a standard approach is great! Since Listings and Newsletters were re-assigned to @miguelpeixe I won't worry about testing those. I do need to do some final testing on the Network plugin wizard so I'll be sure to merge this (or epic/ia after it's merged) to my Network branch and verify it's all good there too. Approved for me! Thanks!

[miguelpeixe]

Not sure if this has any impact here, but for awareness: https://core.trac.wordpress.org/ticket/52035

Thanks for flagging! The strategy doesn't cover submenu pages. I'm assuming we have tighter control over the execution order for submenu pages so it's less problematic.