Migrated "import" store to "wp.data" #4982

octaedro · 2020-08-13T16:18:40Z

Fixes #3382

This PR migrates the import store to wp.data.

Also added error handling.

Screenshots

Detailed test instructions:

Go to WooCommerce | Analytics | Settings (URL: /wp-admin/admin.php?page=wc-admin&path=/analytics/settings)
If you need to add orders to test you should turn WC-Admin off, add a new order and turn it on again.
The count of customers and orders and refunds available to import should be visible.
Verify no HTTP/JS errors occur.
Press Start.
Everything should work normally.

Re-import data

After the process finishes, it should show the button Re-import data. Press it.
Start the process again.
Refresh the browser.
The button Stop should be visible until the importation finishes.

Be careful when you press Delete Previously Imported Data. It could show a false positive since it's a process that runs under the hood

Changelog Note:

Dev: migrate import store to wp.data (don't include this in plugin changelog)

psealock

Looking good @octaedro, this is quite the refactor!

I noted a few things, and found some items in testing but I'm not sure if they how they relate to these changes:

I had more orders than the total, which led to an odd UI

Here is a shot of the API response showing the same thing.

It also showed is_importing: false and the UI said "Finalizing" with the spinner showing, but it remained that way for a long time and continued to poll the server until I refreshed the page. I would have expected it to stop and show some kind of indication of success.

"Delete Previously Imported Data" didn't really do anything until I refreshed the page some time later. I don't know if thats a different issue.

I also got this warning:

Property setImportFinished returned from dispatchMap in useDispatchWithMap must be a function.`

Finally, when the button received the disabled prop the UI doesn't reflect that, so it can be misleading to the user because the button looks enabled. This obviously is a separate issue to tackle later. Issue here: #5046

psealock · 2020-08-23T21:48:03Z

client/analytics/settings/constants.js

+	noteTypes: 'info,warning,marketing,survey',
+};
+
+export const TIMEOUT = 3 * SECOND;


We should use the constants found in packages/data/src/constants.js.

Good idea! The constants were moved in the commit 2b79e38

psealock · 2020-08-23T21:49:45Z

client/analytics/settings/historical-data/actions.js

+ * Internal dependencies
+ */
+import { formatParams } from './utils';
+import { recordEvent } from '../../../lib/tracks';


If you rebase, this code is now at import { recordEvent } from '@woocommerce/tracks';

psealock · 2020-08-23T21:53:11Z

client/wc-api/wc-api-spec.js

-					...imports.operations.read( resourceNames ),
-					...items.operations.read( resourceNames ),
-				];
+				return [ ...items.operations.read( resourceNames ) ];
 			},
 			update( resourceNames, data ) {
 				return [ ...items.operations.update( resourceNames, data ) ];


Items have now been removed, so this whole file can be removed after a rebase.

Would removing the file client/wc-api/wc-api-spec.js generate some problems here?

This commit migrates the "import" store to "wp.data" # Conflicts: # client/analytics/settings/historical-data/layout.js # packages/data/src/index.js # Conflicts: # packages/data/src/index.js # Conflicts: # packages/data/src/index.js

This commit adds error handling to the apiFetch

This commit removes old import store files and "wc-api-spec" import references # Conflicts: # client/wc-api/wc-api-spec.js # Conflicts: # client/wc-api/wc-api-spec.js

# Conflicts: # client/analytics/settings/historical-data/index.js

octaedro · 2020-08-28T00:58:06Z

Thank you @psealock for your review. I answered the questions you asked below.
Could you give this PR another look?

I had more orders than the total, which led to an odd UI

I couldn't replicate this issue. I tried with 20 orders and 20 customers and everything worked ok.

It also showed is_importing: false and the UI said "Finalizing" with the spinner showing

This is a preexisting issue, I solved it in the commit d736c7f

but it remained that way for a long time and continued to poll the server until I refreshed the page.

Weird, this process usually takes a lot of time but I never experienced something like that. Did refreshing the page fix the problem for you?

I would have expected it to stop and show some kind of indication of success.

The current behavior is inherited from the old version. When the importation process finishes the user will see Status: All historical data imported and only the buttons Re-import data and Delete Previously Imported Data should be visible.

"Delete Previously Imported Data" didn't really do anything until I refreshed the page some time later. I don't know if thats a different issue.

This is also inherited. It's working but it doesn't tell the user what is going on in the API side.

I also got this warning:
Property setImportFinished returned from dispatchMap in useDispatchWithMap must be a function.

Solved in the commit bee9f92

Finally, when the button received the disabled prop the UI doesn't reflect that, so it can be misleading to the user because the button looks enabled. This obviously is a separate issue to tackle later. Issue here: #5046

I solved this problem in the commit aada1d3

becdetat · 2020-08-31T03:21:45Z

client/analytics/settings/historical-data/index.js


 class HistoricalData extends Component {
 	constructor() {
 		super( ...arguments );

 		this.dateFormat = __( 'MM/DD/YYYY', 'woocommerce-admin' );
+		this.intervalId = -1;
+		this.lastImportStopTimestamp = 0;
+		this.cacheNeedsClearning = true;


Good eyes @becdetat! I corrected it in the commit 2637750

becdetat · 2020-08-31T03:28:26Z

client/analytics/settings/historical-data/layout.js

-		lastImportStartTimestamp
+
+	const isError = Boolean(
+		getImportError( lastImportStartTimestamp ) ||


is this a || a?

Nice catch! This was fixed in the commit e417088

becdetat · 2020-08-31T03:29:04Z

client/analytics/settings/historical-data/layout.js

-		! isStatusLoading &&
-			inProgress &&
+		inProgress &&
+			! cacheNeedsClearning &&


same typo as above

becdetat

Everything looked fine in the first step until it seemed to get stuck when almost completed importing orders and refunds. When I refreshed the page everything was zero-ed out but the re-import data button wasn't present so I couldn't test the second step.

Assuming that this is just a quirk with my setup (I have thousands of customers and orders) this looks fine.

psealock

Changes look good @octaedro ! Nice work. I left a comment about the CSS for disabled button, otherwise looking good.

Would removing the file client/wc-api/wc-api-spec.js generate some problems here?

yes probably. I deleted the entire client/wc-api folder to see what would happen. Theres a lot of conflicts with code still using parts of wc-api. Once this is merged, I think it would good to address those in a follow up.

psealock · 2020-09-01T02:06:35Z

client/analytics/settings/historical-data/style.scss

@@ -112,4 +112,7 @@
 .woocommerce-settings-historical-data__actions {
 	align-items: center;
 	display: flex;
+	button:disabled {
+		opacity: 0.5 !important;
+	}


This works great, but I think should be removed because its a problem across all wc-admin. Issue to track here: #5046

Thank you @psealock, got it!
This was removed in the commit b481d41

psealock

That last comment was meant to be an approval 🎉

octaedro · 2020-09-01T13:20:55Z

As all the suggested changes were addressed I'll proceed to merge this PR.

octaedro added status: in progress Packages labels Aug 13, 2020

octaedro self-assigned this Aug 13, 2020

octaedro added this to In Progress PRs (for automation purposes) in Isotope via automation Aug 13, 2020

octaedro force-pushed the add/3382 branch 2 times, most recently from 4871e9f to 13d20d8 Compare August 19, 2020 13:04

octaedro added [Status] Needs Review and removed status: in progress labels Aug 22, 2020

octaedro requested a review from a team August 22, 2020 19:54

psealock reviewed Aug 23, 2020

View reviewed changes

psealock 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 Aug 23, 2020

Fernando Marichal added 17 commits August 26, 2020 11:53

Migrated the "import" store to "wp.data"

ee3ce3f

This commit migrates the "import" store to "wp.data" # Conflicts: # client/analytics/settings/historical-data/layout.js # packages/data/src/index.js # Conflicts: # packages/data/src/index.js # Conflicts: # packages/data/src/index.js

Added error handling

e9e086b

This commit adds error handling to the apiFetch

Fixred reducer.js

cdd27a4

Added reducer.js tests

c50f38c

Removed "endpoint" references

566a119

Tests fixed

2d6ee8c

Removed old import store files and wc-api-spec import references

090e862

This commit removes old import store files and "wc-api-spec" import references # Conflicts: # client/wc-api/wc-api-spec.js # Conflicts: # client/wc-api/wc-api-spec.js

Added interval to invalidate resolution of getImportStatus

d569b61

Fixed timestamp

7ad0ca8

Store dependency removed

b448049

Fixed reimport functionality

7c84b9e

# Conflicts: # client/analytics/settings/historical-data/index.js

Refactored layout.js

84e7bb8

Refactored clearCache method

fa652e5

inProgress check refactored

6c4e163

Using activeImport from store

daa7ee0

# Conflicts: # client/analytics/settings/historical-data/index.js

Fixed console warning

a3524a5

# Conflicts: # client/analytics/settings/historical-data/index.js

Migration done!

43c01b5

# Conflicts: # client/analytics/settings/historical-data/index.js

Fernando Marichal added 2 commits August 26, 2020 12:21

Fixed hasImportFinished

95c352f

Test fixed

241bd82

octaedro force-pushed the add/3382 branch from 5720621 to 241bd82 Compare August 26, 2020 15:43

Fernando Marichal added 6 commits August 26, 2020 12:55

Removed "setImportFinished" from actions.js and index.js

bee9f92

Added style to disabled button

aada1d3

Fixed incorrect "Finalizing" status

d736c7f

Fixed tests

9fad710

Fixed notes update on importation start

ed6cf06

Removed local constants

2b79e38

octaedro 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 Aug 28, 2020

becdetat reviewed Aug 31, 2020

View reviewed changes

becdetat approved these changes Aug 31, 2020

View reviewed changes

psealock reviewed Sep 1, 2020

View reviewed changes

psealock approved these changes Sep 1, 2020

View reviewed changes

psealock 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 Sep 1, 2020

Fernando Marichal added 3 commits September 1, 2020 09:49

Removed css for disabled button

b481d41

Typo corrected

2637750

Fixed param for getImportError method

e417088

octaedro added [Status] Ready to Merge and removed needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. labels Sep 1, 2020

octaedro merged commit 917ce3c into main Sep 1, 2020

Isotope automation moved this from In Progress PRs (for automation purposes) to Done Cycle 4 Sep 1, 2020

octaedro deleted the add/3382 branch September 1, 2020 13:21

timmyc added this to the 1.6 milestone Sep 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrated "import" store to "wp.data" #4982

Migrated "import" store to "wp.data" #4982

octaedro commented Aug 13, 2020 •

edited

psealock left a comment •

edited

psealock Aug 23, 2020

octaedro Aug 28, 2020

psealock Aug 23, 2020

psealock Aug 23, 2020

octaedro Aug 28, 2020

octaedro commented Aug 28, 2020

becdetat Aug 31, 2020

octaedro Sep 1, 2020

becdetat Aug 31, 2020

octaedro Sep 1, 2020

becdetat Aug 31, 2020

becdetat left a comment

psealock left a comment

psealock Sep 1, 2020

octaedro Sep 1, 2020

psealock left a comment

octaedro commented Sep 1, 2020

Migrated "import" store to "wp.data" #4982

Migrated "import" store to "wp.data" #4982

Conversation

octaedro commented Aug 13, 2020 • edited

Screenshots

Detailed test instructions:

Changelog Note:

psealock left a comment • edited

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

octaedro commented Aug 28, 2020

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

becdetat left a comment

Choose a reason for hiding this comment

psealock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psealock left a comment

Choose a reason for hiding this comment

octaedro commented Sep 1, 2020

octaedro commented Aug 13, 2020 •

edited

psealock left a comment •

edited