Adds Set up Facebook for WooCommerce task

[ibndawood]

Changes proposed in this Pull Request:

Closes #2298.

This PR:

• Adds a setup task that asks the user to set up Facebook for WooCommerce.
• Removes the admin notice that asked the user to connect to Facebook.

• Do the changed files pass phpcs checks? Please remove phpcs:ignore comments in changed files and fix any issues, or delete if not practical.

Screenshots:

Detailed test instructions:

1. On a fresh WordPress installation, install and activate Facebook for WooCommerce plugin.
2. Verify that the admin notice: "Facebook for WooCommerce is almost ready" banner is not there.
3. Navigate to WooCommerce > Home and verify the task: "Set up Facebook for WooCommerce" has been added.
4. Verify the time for the task is "20 minutes".
5. Verify the task is unchecked.
6. Verify clicking on the task should take you to the Marketing > Facebook page: wp-admin/admin.php?page=wc-facebook
7. Verify Grow your business on Facebook banner is present and that the action button navigates you to Facebook.
8. Connect Facebook to your website.
9. Once you are connected, navigate to WooCommerce > Home and verify the task: "Set up Facebook for WooCommerce" is checked.
10. Navigate to Marketing > Facebook and Disconnect your store.
11. Navigate to WooCommerce > Home and verify the task: "Set up Facebook for WooCommerce" is active again.

Additional details:

An earlier version of the PR used woocommerce_admin_onboarding_task_list filter and added the task through JS. However, while testing, I found that this approach was deprecated and TaskLists::add_task is the now recommended method.

I searched through all Automattic repos for examples that used the TaskLists::add_task method and found this example extension: https://github.com/woocommerce/woocommerce/tree/trunk/plugins/woocommerce-admin/docs/examples/extensions/add-task . However, this example extension was not working, so I reached out to @joshuatf who added the example and asked for help.

Joshua got back to me and clarified that the task list API had undergone a few changes and explained the reason for the changes. He provided with a working example and created this issue to correct the task example in the documentation. Thank you @joshuatf.

Changelog entry

Add - Set up Facebook task to the WooCommerce admin tasks

[rawdreeg]

I tested the changes, and they work as expected. Thanks, @ibndawood

I added a small suggestion: running add_setup_task on admin_init rather than init

[jorgemd24]

Howdy @ibndawood! Thanks for adding this, I looked at this PR as I needed to do something similar for the Google Analytics Integration and found an issue with the hook admin_init.

The hook admin_init is not triggered when the WC fetches the tasks using the following endpoint: wp-json/wc-admin/onboarding/tasks?_locale=user, I think the hook admin_init is not triggered because the request is not made through admin-ajax.php or admin-post.php therefore the function add_setup_task was never executed. If I change the hook to init the task is shown correctly.

Also, the class Setup could be renamed to something more specific as WC is using the class name to create the "id map", so it could collide with another class in the future if the name is too generic.

[ibndawood]

Hey @jorgemd24, Thank you for the catch and explanation. I really do appreciate it. It looks like I've missed testing after changing the hook. I'll be more careful next time. Thank you once again.

As regards the class name, the namespace conveys the specificity of the class. I think it should be fine here. Your thoughts?

[jorgemd24]

As regards the class name, the namespace conveys the specificity of the class. I think it should be fine here. Your thoughts?

My only concern is that the class name is used as the index of task_class_id_map, as far as I can see the namespace is removed in the following line: https://github.com/woocommerce/woocommerce/blob/11e22063caedc74459043e955bfb4368ee05ce6a/plugins/woocommerce/src/Admin/Features/OnboardingTasks/TaskList.php#L277

So that makes it easier for another class with the same name to override the index.

Minor thing, the following line can be removed as TaskLists is unused.

https://github.com/woocommerce/facebook-for-woocommerce/blob/05856938c2e6b15d830c9ce43dbec1bb16430fb1/includes/Admin/Tasks/Setup.php#L14

[ibndawood]

Thank you @jorgemd24 for your input. I can see what you mean. Thank you for the explanation, I appreciate it.

Is there a specific reason, why the core would remove the namespace from the task_class_id_map? the namespaces are there to solve precisely the concern you've raised. Renaming the class name to SetupFacebook or SetupFacebookTask would be redundant as the namespace would convey them all. I believe this should be addressed in the core. The task_class_id_map should not rely on plugin developers to ensure the class names are unique. Do you agree?

I've removed the unused TaskLists from the namespace import. Thank you for pointing this out.

[jorgemd24]

Is there a specific reason, why the core would remove the namespace from the task_class_id_map? the namespaces are there to solve precisely the concern you've raised. Renaming the class name to SetupFacebook or SetupFacebookTask would be redundant as the namespace would convey them all. I believe this should be addressed in the core.

@ibndawood I don't know the reason behind this as my point of view is similar to yours, the namespaces would avoid having a duplicate ID in the task_class_id_map. I have tested with another plugin using the same class name and it seems that is not causing any issues, so I guess that we should be ok.

I've removed the unused TaskLists from the namespace import. Thank you for pointing this out.

Thanks for the further adjustments. I tested and it is now showing the FB task.

[ibndawood]

Thank you @jorgemd24 for your feedback and time. I really appreciate it.