Replace old task list option calls with data store selectors #7820

joshuatf · 2021-10-19T13:01:26Z

Fixes #7751

Replaces old deprecated option calls with data store selectors.

Detailed test instructions:

For all use cases

Add a plugin like Google Listings and Ads so that the extended task list can be tested
Make sure the extended tasks aren't complete
Make sure the task list is not hidden

Abbreviated components

Navigate to a WCA page besides the homepage
Click "Inbox" in the header
Check that the the correct count of extended item todos is shown in the abbreviated notifications

Finish setup

Navigate to any WCA page besides the homepage
Check that clicking "Finish Setup" in the header navigates to the homescreen without console errors

Help highlight

Delete help_panel_highlight_shown from your user meta table
Start a task more than once
Make sure the help tip is shown if it's not a completed task

Purchase undo dismiss

Select a purchaseable product type during the profile wizard
Dismiss the purchase task on the homescreen
Navigate back to the profile wizard
Add another purchaseable product
Make sure the purchase task is no longer dismissed

louwie17

Nice work on this @joshuatf, it's nice to get rid of so many of the getOption calls :)
I just noticed a couple small things during testing and left a couple suggestions, looking good though!!

Check that the the correct count of extended item todos is shown in the abbreviated notifications

This tested well, but only on WC Admin pages, not the orders page for example, but I guess that is a plugin's problem (given this was already the case with Google listings and ads).

Also the Store Setup would show briefly while the task lists are being loaded, I left a comment in the file as well, I think this might also be a current issue (given the options are async), but would be a minimal change and nice to add to this PR.

Check that clicking "Finish Setup" in the header navigates to the homescreen without console errors

I didn't errors, but did get these warnings:

louwie17 · 2021-10-22T12:55:08Z

client/header/activity-panel/index.js

-				isResolving( 'getOption', [ 'woocommerce_task_list_hidden' ] ),
-			setupTaskListComplete:
-				getOption( 'woocommerce_task_list_complete' ) === 'yes',
+			requestingTaskListOptions: isResolving( 'getTaskLists' ),


Could we make use of this condition for the Finish setup panel as well (on line 263). Given the Finish Setup shows up when the task lists haven't been loaded yet.

louwie17 · 2021-10-22T12:56:41Z

client/header/activity-panel/display-options/index.js

+				( list ) => list.id === 'setup' && list.isComplete
+			),
+			isTaskListHidden: taskLists.find(
+				( list ) => list.id === 'setup' && list.isHidden


Would it be worth creating selectors for the above two - getTaskIsComplete & getTaskIsHidden or something similar, then we could move this logic to the data store.

That's a good idea. Added getTaskList and getTask. I think these allow a bit more flexibility and it's still trivial to call getTaskList( 'setup' ).isComplete.

louwie17 · 2021-10-22T13:10:37Z

packages/admin-e2e-tests/src/fixtures/options.ts

+		`/wc-admin/onboarding/tasks/${ id }/unhide`
+	);
+	expect( response.statusCode ).toEqual( 200 );
+}


Nice, having the API available will make E2E tests a bit easier 😄

louwie17 · 2021-10-22T13:12:33Z

src/API/OnboardingTasks.php

@@ -143,6 +143,19 @@ public function register_routes() {
 			)
 		);

+		register_rest_route(
+			$this->namespace,
+			'/' . $this->rest_base . '/(?P<id>[a-z0-9_\-]+)/unhide',


Instead of calling this unhide should we call it show instead?

louwie17 · 2021-10-22T13:13:33Z

src/Features/OnboardingTasks/DeprecatedOptions.php

@@ -59,15 +59,15 @@ public static function update_deprecated_options( $value, $old_value, $option )
 				if ( ! $task_list ) {
 					return;
 				}
-				$update = 'yes' === $value ? $task_list->hide() : $task_list->show();
+				$update = 'yes' === $value ? $task_list->hide() : $task_list->unhide();


I guess my previous comment suggested to use show, was there a particular reason you decided to change it to unhide?

I felt show was a bit ambiguous in meaning. It sounds like it will either render the task list, redirect to the task list, or not hide it anymore (what we actually do). Whereas unhide I can only see one meaning.

I don't have a strong opinion on this though, so if you feel show is better, I'm happy to revert.

joshuatf · 2021-10-28T15:37:06Z

Thanks for the feedback, @louwie17!

I wasn't able to reproduce those console warnings. Is that also on main or only this branch?

I addressed all your feedback with the exception of the show/hide change. I left some comments on why I updated this, but I don't have very strong feelings on this so I'm happy to revert if you feel it makes more sense as show.

louwie17 · 2021-11-01T19:36:47Z

packages/data/src/onboarding/resolvers.js

+
+export function* getTask() {
+	yield resolveSelect( STORE_NAME, 'getTaskLists' );
+}


Nice, that's a clever way!

louwie17

@joshuatf just noticed one issue in the home screen layout.

louwie17 · 2021-11-01T19:52:02Z

client/homescreen/activity-panel/index.js

-			isTaskListHidden: Boolean( taskLists.length )
-				? ! taskLists.find( ( list ) => list.id === 'setup' ).isVisible
-				: null,
+			isTaskListHidden: taskList?.isVisible,


@joshuatf this should be isHidden right?

Thanks for catching this! It actually is supposed to be isVisible but the references to isTaskListHidden needed to be updated.

I think it's better to check visibility in this case since that's really the dependency we're checking and we know the data has been fully loaded if this value is true.

Updated in b957d70.

joshuatf · 2021-11-12T19:16:17Z

Thanks for the review, @louwie17! Rebased and fixed the visibility based on your comment. This is ready for another review.

louwie17

Thanks for the updates @joshuatf, I am fine with the isTaskListVisible rename, although this will also need to be updated in the panels.js file as mentioned in the comment below.

louwie17 · 2021-11-15T14:54:50Z

client/homescreen/activity-panel/index.js

-			isTaskListHidden: Boolean( taskLists.length )
-				? ! taskLists.find( ( list ) => list.id === 'setup' ).isVisible
-				: null,
+			isTaskListVisible: taskList?.isVisible,


Changing the isTaskListHidden prop here, will also require a change in the getAllPanels function here:

woocommerce-admin/client/homescreen/activity-panel/panels.js

Line 18 in b957d70

isTaskListHidden,

None of the panels are showing up at the moment.

joshuatf · 2021-11-15T19:05:19Z

Great catch, @louwie17! I completely missed the panelData being passed to the child.

Reverted back to checking for hidden task list so that we don't prematurely show the wrong panels. The usage of the task list in this controller has a check for undefined so it should still be okay.

louwie17

All seems good now, thanks for the updates @joshuatf 👍

…erce/woocommerce-admin#7820) * Add endpoint to unhide task list * Rename show method to unhide * Replace requests for hidden task list options * Replace unhide task list requests * Replace task list complete checks * Replace dismiss option requests * Remove complete task option check * Fix up panel buttons * Update finished setup to wait for task list resolution * Create selector for getting a single task list * Add selector for single task * Fix task list panel visibility * Add changelog entry * Fix empty product types in profiler data * Revert to checking for hidden task list

joshuatf added the status: in progress label Oct 19, 2021

joshuatf self-assigned this Oct 19, 2021

probot-autolabeler bot added the Packages label Oct 19, 2021

github-actions bot added the needs changelog entry label Oct 19, 2021

joshuatf requested a review from a team October 19, 2021 18:29

joshuatf added status: needs review and removed status: in progress labels Oct 19, 2021

joshuatf force-pushed the update/7751 branch from ea433e4 to 19eb9a3 Compare October 19, 2021 18:38

louwie17 reviewed Oct 22, 2021

View reviewed changes

louwie17 added needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. and removed status: needs review labels Oct 22, 2021

joshuatf added status: needs review and removed needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. labels Oct 28, 2021

louwie17 reviewed Nov 1, 2021

View reviewed changes

louwie17 added needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. and removed status: needs review labels Nov 1, 2021

joshuatf added 12 commits November 12, 2021 13:23

Add endpoint to unhide task list

59d12a9

Rename show method to unhide

7525405

Replace requests for hidden task list options

a3fc156

Replace unhide task list requests

4600c82

Replace task list complete checks

69d5bf1

Replace dismiss option requests

f9f2f7b

Remove complete task option check

8ff3f3f

Fix up panel buttons

022dd82

Update finished setup to wait for task list resolution

195f3e1

Create selector for getting a single task list

74f25f2

Add selector for single task

7fdc2db

Fix task list panel visibility

b957d70

joshuatf force-pushed the update/7751 branch from 28e4a0a to b957d70 Compare November 12, 2021 19:13

Add changelog entry

1230de2

joshuatf added status: needs review and removed needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. labels Nov 12, 2021

github-actions bot removed the needs changelog entry label Nov 12, 2021

Fix empty product types in profiler data

4a8ae38

louwie17 suggested changes Nov 15, 2021

View reviewed changes

louwie17 added needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. and removed status: needs review labels Nov 15, 2021

Revert to checking for hidden task list

87b08c9

joshuatf added status: needs review and removed needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. labels Nov 15, 2021

louwie17 approved these changes Nov 15, 2021

View reviewed changes

louwie17 added status: ready to merge and removed status: needs review labels Nov 15, 2021

joshuatf merged commit e9d94f6 into main Nov 15, 2021

joshuatf deleted the update/7751 branch November 15, 2021 20:57

moon0326 mentioned this pull request Dec 17, 2021

[GlobalStep - 2.9.2] "Store Management" panel fails to appear after hiding the "Setup Task List". #8037

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace old task list option calls with data store selectors #7820

Replace old task list option calls with data store selectors #7820

joshuatf commented Oct 19, 2021 •

edited

Loading

louwie17 left a comment

louwie17 Oct 22, 2021

louwie17 Oct 22, 2021

joshuatf Oct 28, 2021

louwie17 Oct 22, 2021

louwie17 Oct 22, 2021

louwie17 Oct 22, 2021

joshuatf Oct 27, 2021

joshuatf commented Oct 28, 2021

louwie17 Nov 1, 2021

louwie17 left a comment

louwie17 Nov 1, 2021

joshuatf Nov 12, 2021

joshuatf commented Nov 12, 2021

louwie17 left a comment

louwie17 Nov 15, 2021

joshuatf commented Nov 15, 2021

louwie17 left a comment

Replace old task list option calls with data store selectors #7820

Replace old task list option calls with data store selectors #7820

Conversation

joshuatf commented Oct 19, 2021 • edited Loading

Detailed test instructions:

louwie17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuatf commented Oct 28, 2021

Choose a reason for hiding this comment

louwie17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuatf commented Nov 12, 2021

louwie17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuatf commented Nov 15, 2021

louwie17 left a comment

Choose a reason for hiding this comment

joshuatf commented Oct 19, 2021 •

edited

Loading