feat(ia): setup wizard

[jaredrethman]

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:

Add new IA wizards instantiations into newspack_setup_complete conditional. PR also includes minor fix for Newspack Dashboard slug.

How to test the changes in this Pull Request:

1. Checkout this branch git checkout origin/feat/ia-setup
2. Delete newspack_setup_complete from db i.e. wp option delete newspack_setup_complete
3. Navigate to /wp-admin/admin.php?page=newspack-setup-wizard
   
4. Confirm no other Newspack IA menus exist.
5. Complete Setup process.
6. Once completed - refresh browser and observe new IA menus are present.

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?

[ronchambers]

This isn't an official review, just one piece of feedback since I got a ping via Asana and I had looked into the setup wizard before... Just one needed tweak I saw:

includes/wizards/class-setup-wizard.php
        public function redirect_to_setup() {
                $screen = get_current_screen();
-               if ( $screen && 'toplevel_page_newspack' === $screen->id ) {
+               if ( $screen && 'toplevel_page_newspack-dashboard' === $screen->id ) {

This will make it so that the new dashboard slug (url) will redirect to setup if setup isn't complete yet.

[jaredrethman]

This isn't an official review, just one piece of feedback since I got a ping via Asana and I had looked into the setup wizard before... Just one needed tweak I saw:

includes/wizards/class-setup-wizard.php
        public function redirect_to_setup() {
                $screen = get_current_screen();
-               if ( $screen && 'toplevel_page_newspack' === $screen->id ) {
+               if ( $screen && 'toplevel_page_newspack-dashboard' === $screen->id ) {

This will make it so that the new dashboard slug (url) will redirect to setup if setup isn't complete yet.

Thanks @ronchambers updated b466258

[jaredrethman]

@jaredrethman - One more. I just noticed the following issue after I did the setup to make sure the menu items would all show up and that the "setup" sub-menu item would go to "hidden". The is_setup_complete() function wasn't returning true even though setup was complete and the actual options database value was true ('1').

includes/class-newspack.php
        public static function is_setup_complete() {
-               return '1' === get_option( NEWSPACK_SETUP_COMPLETE, '0' );
+               return get_option( NEWSPACK_SETUP_COMPLETE );
        }

I don't quite understand all this but looking around the other places in Newspack Plugin where the option value is checked for true/false, just doing return get_option( NEWSPACK_SETUP_COMPLETE ) seems to work the best. I know it's good to be more specific with === and yes the value in the db is 1, but the function wasn't returning true. I verified with phpcs too and it didn't throw any warnings for this change above.

Thanks @ronchambers . Stepping through the code I'm unable to replicate the issue you've described, once I complete the setup is_setup_complete returns true. Can you share some reproducible steps, please.

[jaredrethman]

Reverted the status of this PR to draft as I've picked up some issue with Audience Wizards.

[ronchambers]

@jaredrethman - sorry, you're right. I can't reproduce the issue. I'll assume it had to do with cache of some sort. I retested on a clean site without cache and is_setup_complete worked fine. I marked my previous comment as "resolved".

[jaredrethman]

@ronchambers , I came across another significant bug, which required a change in approach.

The Gist: During setup there are endpoints that are used which are tied to wizard components. Previously, until newspack_setup_complete option is set no wizard classes would be instantiated, resulting in none (non-setup) of the wizard endpoints being registered. This logic was reverted in favor of conditionally running admin_menu hook instead (51d770f).

There were a number of other commits, including fixes to Newsletters & Network.

[ronchambers]

FYI: I didn't get an email notification, but luckily I noticed it in https://github.com/notifications ) I'll review this afternoon.

[ronchambers]

@jaredrethman this looks great! I really like the menu.

What I saw before setup:

And after setup:

Looks good.

The only change I'd recommend are removing the now redundant / rolled back calls to the admin_menu/add_page hook. See the 2 places I marked via this review.

I did notice a couple possible "bugs" but they have to do more with the internal setup features and not this foundational PR. I'll share them with you separately for your review.

I really like this PR as it is so I say you should merge it then the bugs I'll share via Slack can be handled in their own PRs.

Thanks!