Add task using TaskLists::add_task API

[tomalec]

Changes proposed in this Pull Request:

• Add task using TaskLists::add_task API, woocommerce_admin_onboarding_task_list is deprecated.
• Remove add_woocommerce_extended_task_list_item & remove_woocommerce_extended_task_list_item hooks.
• Add npm script to generate hook's docs.
  Re-generate those docs.
• Add tests for CompleteSetupTask

Screenshots:

Detailed test instructions:

1. With incomplete onboarding, go to WooCommerce > Home /wp-admin/admin.php?page=wc-admin
2. You should see a dismissable, incomplete task to "Set up Google Listings & Ads (20 minutes)"
3. Click on the task to get redirected to the GLA start page
4. Complete the onboarding
5. go back to WooCommerce > Home /wp-admin/admin.php?page=wc-admin
6. You should see the complete setup GLA task that, when clicked, redirects to the GLA dashboard.

Additional details:

1. The register method is being called a few times for some reason. I don't know how to make sure the callback is called only once in https://github.com/woocommerce/google-listings-and-ads/pull/2026/files#diff-b45e7e0b771cc2f644e3a4805a651b28213c4f6464b615c7c26e947275a3d52fR27-R39
   Currently, it does not introduce any problem, just does unnecessary runs, but it could lead to unexpected quirks in the future.
   Tests are checking it was called at least once.

Changelog entry

Tweak - Use the latest API to add an item to the WC tasks list.
Break - Remove add_woocommerce_extended_task_list_item and remove_woocommerce_extended_task_list_item hooks.

[codecov]

Codecov Report

Merging #2026 (bcab4d4) into develop (d12eb24) will increase coverage by 0.2%.
Report is 43 commits behind head on develop.
The diff coverage is 65.5%.

❗ Current head bcab4d4 differs from pull request most recent head 3733294. Consider uploading reports for the commit 3733294 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##             develop   #2026     +/-   ##
===========================================
+ Coverage       58.1%   58.2%   +0.2%     
+ Complexity      4119    4117      -2     
===========================================
  Files            455     454      -1     
  Lines          17695   17681     -14     
===========================================
+ Hits           10278   10297     +19     
+ Misses          7417    7384     -33     

Flag	Coverage Δ
php-unit-tests	58.2% <65.5%> (+0.2%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
...ernal/DependencyManagement/CoreServiceProvider.php	0.0% <0.0%> (ø)
src/TaskList/CompleteSetupTask.php	67.9% <67.9%> (ø)

[mikkamp]

Thanks for the changes. It's working well and I'm seeing the task being added.
I just left some comments on how to fix the tests as the WP hooks shouldn't be triggered twice.

[mikkamp]

All the WP hooks have already been called before the unit tests starts. So at this point it's already triggered the register code and init code which adds the task to the list.

So what we are really doing here is clearing the WC task list with our custom task added. And then we try to add it again in test_register but we shouldn't trigger the init hook a second time as that calls all kinds of code we don't want to run a second time.

My suggestion would be to remove these lines of code and leave TaskLists as is (so we are working with a non mocked version of TaskLists).

[tomalec]

Thanks for the explanation :)

Implemented in e9d7c21

But that means we will not be asserting the actual timing of the registration.

Are we fine with that?

[tomalec]

Also, CodeCov complains about no coverage for the register method https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2026/blob/src/TaskList/CompleteSetupTask.php#L27

[mikkamp]

As mentioned in the comment above we shouldn't register and call init a second time here. Instead we should just test that it was registered and the hook was called.
The task is never going to be an identical object so at most I'd compare the ID or just leave it at assertNotNull since we are already fetching a specific ID so if that returns something it should always be the right one.

Suggested change

	public function test_register() {
	$this->task->register();
	do_action( 'init', 1 );
	$this->assertGreaterThan( 0, did_action( 'add_woocommerce_extended_task_list_item' ) );
	$this->assertNotNull( TaskLists::get_list( 'extended' )->get_task( $this->task->get_id() ) );
	$this->assertEquals( $this->task, TaskLists::get_list( 'extended' )->get_task( $this->task->get_id() ) );
	}
	public function test_register() {
	$this->assertEquals( 1, did_action( 'add_woocommerce_extended_task_list_item' ) );
	$this->assertNotNull( TaskLists::get_list( 'extended' )->get_task( $this->task->get_id() ) );
	$this->assertEquals(
	$this->task->get_id(),
	TaskLists::get_list( 'extended' )->get_task( $this->task->get_id() )->get_id()
	);
	}

[tomalec]

Maybe we should not create any instance but test the behavior of the already registered one
1c7b5fb

[mikkamp]

Seems a previous comment I made about this hook got lost. But I was questioning if we still need to call this hook, we don't use it anywhere in our code and neither does WC need it to add the task to the list. It seems convenient for testing to confirm the function was called, but as long as we are testing the task got added to the list then I don't think that's necessary to test.

[tomalec]

I just kept it as it was to minimize the set of changes, but if you think we don't need that hook. I'm always for YAGNI and less code :)

[tomalec]

Thank you @mikkamp for the great suggestions. I changed the tests a bit, so everything is passing. However, Codecov still complains about the lack of coverage for register.
Would you mind another round?

[mikkamp]

Thanks that looks a lot simpler.

I can understand the desire to get 100% unit test coverage. Although this case might be a bit tricky since the register function would be better covered in an integration test. PHPunit looks for a line of code in the tests which triggers the "covered" code, but that's only happening (triggered by bootstrap.php) before it starts the actual tests. So at most you could add @codeCoverageIgnore to the register function (but we haven't done that much in other cases).

Also note that if register / init would not be called then this part of the test would fail:

		// Fetch the task from the global list.
		$this->task = TaskLists::get_list( 'extended' )->get_task( 'gla_complete_setup' );
		$this->task->set_merchant_center_object( $this->merchant_center );

Because $this->task would be false if the task is not found, so the function set_merchant_center_object can't be called. So we are testing the result of register, but just not proving it in the code coverage. One improvement we could do is assertInstanceOf to confirm that $this->task is the expected object, as that will help with troubleshooting if it ever fails.

Either way I'm going to approve the PR as the actual code changes performs as expected.

[tomalec]

Thanks for the clarifications. I agree it's more of an integration test, but I was pretty surprised that Codecov reported as "uncovered" lines that were actually executed. It's a pity it was covered before Codecov checks, but I think as long as it's actually tested, we're safe.

Also note that if register / init would not be called then this part of the test would fail:

Yep, that's why I put it there. There is a small downside that it's in setup not test, so troubleshooting could be a bit less straightforward. I've added a failure message in setUp to make it clearer 3733294
Also, I think the assertInstanceOf can help 5565e31.