Fix/8251 woocommerce payments task list logic (woocommerce#8332)

* Allow the support of multiple task lists with overlapping tasks * Add filter by task ids * Revert getTaskListById call * Fix notices * Move prefix event to task and task list classes instead * Fix track events and extended task list * Fix php unit tests * Remove the seperate task list classes as this was unnecessary. * Fix tests * Remove unneeded allowed-tasks * Change onboarding task list redux store structure * Add extra id for handling hidden param for experimental task lists * Fix lint errors * Fix forgotten change * Add changelog
thomasplevy · Mar 1, 2022 · 6820ede · 6820ede
1 parent 1446f26
commit 6820ede
Show file tree

Hide file tree

Showing 36 changed files with 625 additions and 396 deletions.
diff --git a/changelogs/fix-8251_woocommerce_payments_task_list_logic b/changelogs/fix-8251_woocommerce_payments_task_list_logic
@@ -0,0 +1,4 @@
+Significance: minor
+Type: Dev
+
+Update task list data structure to better handle new designs. #8332
diff --git a/client/homescreen/layout.js b/client/homescreen/layout.js
@@ -337,7 +337,7 @@ export default compose(
 			shouldShowWelcomeModal,
 			shouldShowWelcomeFromCalypsoModal,
 			isTaskListHidden: getTaskList( 'setup' )?.isHidden,
-			hasTaskList: taskLists.find( ( list ) => list.isVisible ),
+			hasTaskList: !! taskLists.find( ( list ) => list.isVisible ),
 			taskListComplete: getTaskList( 'setup' )?.isComplete,
 			installTimestamp,
 			installTimestampHasResolved,

diff --git a/client/tasks/task-list.tsx b/client/tasks/task-list.tsx
@@ -17,15 +17,6 @@ import { TaskListItem } from './task-list-item';
 import { TaskListMenu } from './task-list-menu';
 import './task-list.scss';
 
-export const prefixEvent = ( id: string, eventName: string ): string => {
-	// This helps retain backwards compatibility with the old event naming.
-	if ( id === 'setup' ) {
-		return `tasklist_${ eventName }`;
-	}
-
-	return `${ id }_tasklist_${ eventName }`;
-};
-
 export type TaskListProps = TaskListType & {
 	query: {
 		task: string;
@@ -34,6 +25,7 @@ export type TaskListProps = TaskListType & {
 
 export const TaskList: React.FC< TaskListProps > = ( {
 	id,
+	eventPrefix,
 	tasks,
 	title: listTitle,
 	isCollapsible = false,
@@ -64,7 +56,7 @@ export const TaskList: React.FC< TaskListProps > = ( {
 	);
 
 	const recordTaskListView = () => {
-		recordEvent( prefixEvent( id, 'view' ), {
+		recordEvent( eventPrefix + 'view', {
 			number_tasks: visibleTasks.length,
 			store_connected: profileItems.wccom_connected,
 		} );
@@ -106,9 +98,8 @@ export const TaskList: React.FC< TaskListProps > = ( {
 				collapseLabel,
 				expandLabel,
 				show: 2,
-				onCollapse: () =>
-					recordEvent( prefixEvent( id, 'collapse' ), {} ),
-				onExpand: () => recordEvent( prefixEvent( id, 'expand' ), {} ),
+				onCollapse: () => recordEvent( eventPrefix + 'collapse', {} ),
+				onExpand: () => recordEvent( eventPrefix + 'expand', {} ),
 		  }
 		: {};
 

diff --git a/client/tasks/tasks.tsx b/client/tasks/tasks.tsx
@@ -60,11 +60,11 @@ export const Tasks: React.FC< TasksProps > = ( { query } ) => {
 	};
 
 	const toggleTaskList = ( taskList ) => {
-		const { id, isHidden } = taskList;
+		const { id, eventPrefix, isHidden } = taskList;
 		const newValue = ! isHidden;
 
 		recordEvent(
-			newValue ? `${ id }_tasklist_hide` : `${ id }_tasklist_show`,
+			newValue ? `${ eventPrefix }hide` : `${ eventPrefix }show`,
 			{}
 		);
 
@@ -101,62 +101,63 @@ export const Tasks: React.FC< TasksProps > = ( { query } ) => {
 		return <TasksPlaceholder query={ query } />;
 	}
 
-	return taskLists.map( ( taskList ) => {
-		const {
-			id,
-			isComplete,
-			isHidden,
-			isVisible,
-			isToggleable,
-			title,
-		} = taskList;
-
-		if ( ! isVisible ) {
-			return null;
-		}
-		const hasMultiplePaymentTasks =
-			( taskList.tasks || [] ).filter(
-				( t ) => t.id === 'payments' || t.id === 'woocommerce-payments'
-			).length > 1;
-		const tasks =
-			id === 'setup' && hasMultiplePaymentTasks && taskList.tasks
-				? taskList.tasks.filter( ( t ) => t.id !== 'payments' )
-				: taskList.tasks;
-
-		return (
-			<Fragment key={ id }>
-				<TaskList
-					id={ id }
-					isComplete={ isComplete }
-					isExpandable={
-						experimentAssignment?.variationName === 'treatment'
-					}
-					query={ query }
-					tasks={ tasks }
-					title={ title }
-				/>
-				{ isToggleable && (
-					<DisplayOption>
-						<MenuGroup
-							className="woocommerce-layout__homescreen-display-options"
-							label={ __( 'Display', 'woocommerce-admin' ) }
-						>
-							<MenuItem
-								className="woocommerce-layout__homescreen-extension-tasklist-toggle"
-								icon={ ! isHidden && check }
-								isSelected={ ! isHidden }
-								role="menuitemcheckbox"
-								onClick={ () => toggleTaskList( taskList ) }
+	return taskLists
+		.filter( ( { id } ) =>
+			experimentAssignment?.variationName === 'treatment'
+				? id.endsWith( 'two_column' )
+				: ! id.endsWith( 'two_column' )
+		)
+		.map( ( taskList ) => {
+			const {
+				id,
+				eventPrefix,
+				isComplete,
+				isHidden,
+				isVisible,
+				isToggleable,
+				title,
+				tasks,
+			} = taskList;
+
+			if ( ! isVisible ) {
+				return null;
+			}
+
+			return (
+				<Fragment key={ id }>
+					<TaskList
+						id={ id }
+						eventPrefix={ eventPrefix }
+						isComplete={ isComplete }
+						isExpandable={
+							experimentAssignment?.variationName === 'treatment'
+						}
+						query={ query }
+						tasks={ tasks }
+						title={ title }
+					/>
+					{ isToggleable && (
+						<DisplayOption>
+							<MenuGroup
+								className="woocommerce-layout__homescreen-display-options"
+								label={ __( 'Display', 'woocommerce-admin' ) }
 							>
-								{ __(
-									'Show things to do next',
-									'woocommerce-admin'
-								) }
-							</MenuItem>
-						</MenuGroup>
-					</DisplayOption>
-				) }
-			</Fragment>
-		);
-	} );
+								<MenuItem
+									className="woocommerce-layout__homescreen-extension-tasklist-toggle"
+									icon={ ! isHidden && check }
+									isSelected={ ! isHidden }
+									role="menuitemcheckbox"
+									onClick={ () => toggleTaskList( taskList ) }
+								>
+									{ __(
+										'Show things to do next',
+										'woocommerce-admin'
+									) }
+								</MenuItem>
+							</MenuGroup>
+						</DisplayOption>
+					) }
+				</Fragment>
+			);
+		} );
 };
diff --git a/client/tasks/test/task-list.test.tsx b/client/tasks/test/task-list.test.tsx
@@ -7,7 +7,7 @@ import { recordEvent } from '@woocommerce/tracks';
 /**
  * Internal dependencies
  */
-import { TaskList, prefixEvent } from '../task-list';
+import { TaskList } from '../task-list';
 
 jest.mock( '@woocommerce/tracks', () => ( {
 	recordEvent: jest.fn(),
@@ -90,7 +90,13 @@ describe( 'TaskList', () => {
 
 	it( 'should trigger tasklist_view event on initial render for setup task list', () => {
 		render(
-			<TaskList id="setup" tasks={ [] } title="List title" query={ {} } />
+			<TaskList
+				id="setup"
+				eventPrefix="tasklist_"
+				tasks={ [] }
+				title="List title"
+				query={ {} }
+			/>
 		);
 		expect( recordEvent ).toHaveBeenCalledTimes( 1 );
 		expect( recordEvent ).toHaveBeenCalledWith( 'tasklist_view', {
@@ -103,6 +109,7 @@ describe( 'TaskList', () => {
 		render(
 			<TaskList
 				id="extended"
+				eventPrefix="extended_tasklist_"
 				tasks={ [] }
 				title="List title"
 				query={ {} }
@@ -182,18 +189,4 @@ describe( 'TaskList', () => {
 		);
 		expect( queryByText( dismissedTask[ 0 ].title ) ).toBeInTheDocument();
 	} );
-
-	describe( 'prefixEvent', () => {
-		it( 'should not prefix tasklist id if id is setup', () => {
-			expect( prefixEvent( 'setup', 'action' ) ).toEqual(
-				'tasklist_action'
-			);
-		} );
-
-		it( 'should prefix tasklist id if id does not equal setup', () => {
-			expect( prefixEvent( 'extended', 'action' ) ).toEqual(
-				'extended_tasklist_action'
-			);
-		} );
-	} );
 } );
diff --git a/client/tasks/test/tasks.test.tsx b/client/tasks/test/tasks.test.tsx
@@ -46,6 +46,7 @@ describe( 'Task', () => {
 			taskLists: [
 				{
 					id: 'main',
+					eventPrefix: 'main_tasklist_',
 					isVisible: true,
 					tasks: [ { id: 'main-task-1' }, { id: 'main-task-2' } ],
 				},
@@ -124,6 +125,7 @@ describe( 'Task', () => {
 			taskLists: [
 				{
 					id: 'main',
+					eventPrefix: 'main_tasklist_',
 					isVisible: true,
 					isToggleable: true,
 					isHidden: false,
@@ -157,6 +159,7 @@ describe( 'Task', () => {
 				taskLists: [
 					{
 						id: 'main',
+						eventPrefix: 'main_tasklist_',
 						isVisible: true,
 						isToggleable: true,
 						isHidden: false,
@@ -185,6 +188,7 @@ describe( 'Task', () => {
 				taskLists: [
 					{
 						id: 'main',
+						eventPrefix: 'main_tasklist_',
 						isVisible: true,
 						isToggleable: true,
 						isHidden: true,

diff --git a/client/two-column-tasks/allowed-tasks.js b/client/two-column-tasks/allowed-tasks.js
diff --git a/client/two-column-tasks/extended-task.tsx b/client/two-column-tasks/extended-task.tsx
@@ -16,7 +16,6 @@ import { DisplayOption } from '~/activity-panel/display-options';
 import { TaskList } from '../tasks/task-list';
 import { TasksPlaceholder } from '../tasks/placeholder';
 import '../tasks/tasks.scss';
-import allowedTasks from './allowed-tasks';
 
 export type TasksProps = {
 	query: { task?: string };
@@ -30,9 +29,11 @@ const ExtendedTask: React.FC< TasksProps > = ( { query } ) => {
 	const { isResolving, taskLists } = useSelect( ( select ) => {
 		return {
 			isResolving: select( ONBOARDING_STORE_NAME ).isResolving(
-				'getTaskLists'
+				'getTaskListsByIds'
 			),
-			taskLists: select( ONBOARDING_STORE_NAME ).getTaskLists(),
+			taskLists: select( ONBOARDING_STORE_NAME ).getTaskListsByIds( [
+				'extended_two_column',
+			] ),
 		};
 	} );
 
@@ -63,37 +64,9 @@ const ExtendedTask: React.FC< TasksProps > = ( { query } ) => {
 		return <TasksPlaceholder query={ query } />;
 	}
 
-	const extendedTaskList = taskLists.find( ( list ) => {
-		return list.id === 'extended';
-	} );
-
-	// See if we need to move any of the main tasks to the extended task list
-	const setupTaskList = taskLists.find( ( list ) => {
-		return list.id === 'setup';
-	} );
-	const hasSetupPaymentsTask = setupTaskList.tasks.find(
-		( t ) => t.id === 'payments'
-	);
+	const extendedTaskList = taskLists[ 0 ];
 
-	extendedTaskList.tasks = [
-		...new Set(
-			// Filter out the additional payments task if it is already present in the setup tasks.
-			extendedTaskList.tasks
-				.filter(
-					( t ) => t.id !== 'payments' || ! hasSetupPaymentsTask
-				)
-				.concat(
-					setupTaskList?.tasks.filter( ( unallowedTask ) => {
-						return (
-							! allowedTasks.includes( unallowedTask.id ) &&
-							unallowedTask.id !== 'store_details'
-						);
-					} ) || []
-				)
-		),
-	];
-
-	if ( extendedTaskList.tasks.length === 0 ) {
+	if ( ! extendedTaskList || extendedTaskList.tasks.length === 0 ) {
 		return null;
 	}
 
@@ -109,6 +82,7 @@ const ExtendedTask: React.FC< TasksProps > = ( { query } ) => {
 
 	const {
 		id,
+		eventPrefix,
 		isHidden,
 		isVisible,
 		isToggleable,
@@ -124,6 +98,7 @@ const ExtendedTask: React.FC< TasksProps > = ( { query } ) => {
 		<Fragment key={ id }>
 			<TaskList
 				id={ id }
+				eventPrefix={ eventPrefix }
 				isComplete={ isComplete }
 				query={ query }
 				tasks={ tasks }