Add migration to move orders from wp_posts to custom tables #32034

vedanshujain · 2022-03-07T11:39:39Z

All Submissions:

Have you followed the WooCommerce Contributing guideline?
Does your code follow the WordPress' coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

To add migrations, we add a general migration class that targets any combination of an entity table along with it's metadata table, described in the MetaToCustomTableMigrator class. We then use objects of this class in WPPostToCOTMigrator class providing the necessary configuration for each custom table.

How to test the changes in this Pull Request:

This PR should be tested along with #32052, #32053 and #32054. For code review, I have split it into four PRs, but for testing, it should be fine to only test the cot/migration-operational-data as it has all the commits.

We have now merged the separate PRs into this one, so all code is here and reviewing and testing just this one should be good enough.

Checkout the cot/migration-operational-data branch (see PR Add migrations for operational data to custom order tables #32054).
Make sure that you have {prefix}_wc_orders, {prefix}_wc_order_addresses and {prefix}_wc_order_operational_data created by following steps from Add DB table structure for custom order tables. #31811.
Run this snippet:

namespace Automattic\WooCommerce\DataBase\Migrations\CustomOrderTable;

$done = false;
$migrator = new WPPostToCOTMigrator();
delete_option( 'wc_cot_migration' );
$done = $migrator->process_next_migration_batch( 500 );

Check contents of {prefix}_wc_orders, {prefix}_wc_order_addresses and {prefix}_wc_order_operational_data table and verify that first 500 order records are migrated.

After the above commands you can also optionally run this to migrate all the records:

while( ! $done ) {
	$done = $migrator->process_next_migration_batch( 500 );
}

Note the PR has many TODO comments, which will be handled by different PRs as the resolution for them is not clear at this time.

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you successfully run tests with your changes locally?

Changelog entry

Enhancement - Add migration for moving data from wp_posts to custom order table.

FOR PR REVIEWER ONLY:

I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

jorgeatorres

Hi @vedanshujain!

This is brilliant work 💯. I particularly like the way you've abstracted migration of metadata in a way that can be easily re-used for each table.

Ignoring the TODO's for now, this looks good to me, with just one tiny exception (a typo) that I've flagged as that prevents the queries from working correctly.

I do have some general questions, which I'll leave not as part of the review but a comment in the PR.

plugins/woocommerce/src/Database/Migrations/CustomOrderTable/WPPostToCOTMigrator.php

jorgeatorres · 2022-03-22T18:36:50Z

plugins/woocommerce/src/Database/Migrations/CustomOrderTable/WPPostToCOTMigrator.php

+	 *
+	 * @return bool|\WP_Error
+	 */
+	public function process_single( $post_id ) {


This might be intentional but thought I'd ask: shouldn't process_single() also migrate other meta (addresses and op data) for the migrated order?
I know this is not being used anywhere right now, but it's still a public function and might become handy later.

Agreed, I was hoping to do it for later, but let me do it now.

jorgeatorres · 2022-03-22T18:52:31Z

plugins/woocommerce/src/Database/Migrations/CustomOrderTable/WPPostToCOTMigrator.php

+		if ( count( $data['data'] ) !== $result ) {
+			// Some rows were not inserted.
+			// TODO: Find and log the entity ids that were not inserted.
+			echo 'error';


Even at this point, would it make sense to make this an exception or WP_Error instead?

hmm, so raising exceptions will break the entire migration flow, which is not ideal, and leave us in a weird state. but you are also right that logging might not be enough, let me think of better solutions here.

plugins/woocommerce/src/Database/Migrations/CustomOrderTable/WPPostToCOTMigrator.php

jorgeatorres · 2022-03-22T18:54:47Z

plugins/woocommerce/src/Database/Migrations/CustomOrderTable/WPPostToCOTMigrator.php

+		if ( count( $order_data['data'] ) !== $result ) {
+			// Some rows were not inserted.
+			// TODO: Find and log the entity ids that were not inserted.
+			echo ' error ';


See other related comments.

jorgeatorres · 2022-03-22T19:02:52Z

Questions:

Please keep in mind that the items below might very well be already scoped,coming soon or even part of in-progress PRs, but given I'm new to the team and the COT project in particular, I thought I'd still ask them. Feel free to point me in the right direction if this has already been discussed or if there's a better place for these questions.

Even though migrators have support for REPLACE operations, there's no logic right now that would choose the correct operation based on any state. For instance, deleting wc_cot_migration and re-running the migration will happily add duplicates to the tables.
Related to the above, because wc_cot_migration only cares about the highest order ID that was migrated, what happens when an order with ID < highest order ID gets changed? I'm assuming eventually the COT code will add or update things on-demand, but then the point above is even more important or duplicates could be inserted.
Given "orders" are migrated first and then their metadata, we could end up with orders in the orders table with no corresponding metadata if something fails. Is transaction support being worked into the migration at a later point?

Thanks!

vedanshujain · 2022-03-24T07:33:38Z

Even though migrators have support for REPLACE operations, there's no logic right now that would choose the correct operation based on any state.

I was planning to do this later, if you are happy with the overall approach than I can handle this now 👍🏽

Related to the above, because wc_cot_migration only cares about the highest order ID that was migrated, what happens when an order with ID < highest order ID gets changed?

So that would be handled by a different flow ideally which is not implemented yet. Basically, we will also hook on every order save if migrations are running in the background to handle updates one at a time.

Given "orders" are migrated first and then their metadata, we could end up with orders in the orders table with no corresponding metadata if something fails. Is transaction support being worked into the migration at a later point?

Not at this point, but it's a good callout, let me see if we can handle it now. The biggest blocker in doing it is getting the order ID that was just inserted, but there should be a way.

vedanshujain · 2022-03-24T07:35:41Z

Marking the PR as draft again to address feedback.

vedanshujain · 2022-04-01T12:55:48Z

@jorgeatorres This is ready for review again with some caveats:

Migrating metadata is implemented, but it's not yet enabled as it is not resilient currently (for example it will add duplicates).
There are still a bunch of TODOs especially for surfacing errored orders, but I will be handling them in a different PR as it was getting too big.

Other than that, this PR should take care of not adding duplicates, updating instead of inserting when needed, handling interrupted migrations, etc. There is a code sniff failing although violations are fixed, I am looking into it but it should not be a blocker for review.

jorgeatorres

Hi @vedanshujain! 👋

Thanks for all your hard work on this 💯. Overall, it looks good to me, but I left a couple tiny observations. Please let me know what you think about those.

As for testing, I tested with a couple thousand orders in different configurations and didn't notice anything out of the ordinary, though we would have to test again once error handling is implemented as well as metadata migration.

jorgeatorres · 2022-04-05T17:15:11Z

plugins/woocommerce/src/Database/Migrations/MigrationHelper.php

+
+namespace Automattic\WooCommerce\DataBase\Migrations;
+
+use function DeliciousBrains\WP_Offload_Media\Aws3\JmesPath\search;


This seems to have been added by mistake as this is not in WC core nor used in this class.

jorgeatorres · 2022-04-05T17:20:20Z

plugins/woocommerce/src/Database/Migrations/CustomOrderTable/MetaToCustomTableMigrator.php

+ * Generic migration class to move any entity, entity_meta table combination to custom table.
+ */
+
+namespace Automattic\WooCommerce\DataBase\Migrations\CustomOrderTable;


Might be a nitpick, but can we maybe use "Database" instead of "DataBase" in the namespace? It'd not only match the folder structure but also how the word is commonly used (i.e. "database"). If agreed, this would have to be modified in other classes too.

jorgeatorres · 2022-04-05T19:07:41Z

plugins/woocommerce/src/Database/Migrations/CustomOrderTable/WPPostToCOTMigrator.php

+	}
+
+	/**
+	 * Process migration for metadata for given post ids.\


"\" at the end of the line.

vedanshujain · 2022-04-11T06:56:07Z

@jorgeatorres Feedback addressed in c5e3150 . Do you think if we can merge this PR to the trunk, that way it will unblock work on UX implementation? I am meanwhile working on meta and better error logging in separate PRs.

jorgeatorres · 2022-04-11T12:52:50Z

Hi @vedanshujain!

Changes look good. Thank you!

Do you think if we can merge this PR to the trunk, that way it will unblock work on UX implementation?

I'm sure you have a better overview on all things COT than me, but the way I see it, it'd be best to merge this ASAP. Not only because of the UX work but also because it provides classes that might be useful for the other migrations that are still to be implemented. It also provides a small fix to the SQL schema which makes sense to get in soon.

Even if we need to make further adjustments, I'm good with merging this right now. Does that sound good to you?

jorgeatorres

LGTM 🚢. Beautiful work @vedanshujain! Thanks.

github-actions · 2022-04-11T17:30:21Z

Hi @jorgeatorres, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

Add the release: add changelog label
Add the release: add testing instructions label

exonianp · 2022-05-01T20:29:21Z

Have you tested it with any of us working with this: https://github.com/liquidweb/woocommerce-custom-orders-table

github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Mar 7, 2022

vedanshujain marked this pull request as ready for review March 15, 2022 11:06

This was referenced Mar 15, 2022

Add migration for billing address to custom order table #32052

Merged

Add migration for shipping address to custom order table #32053

Merged

Add migrations for operational data to custom order tables #32054

Merged

vedanshujain requested review from a team and jorgeatorres and removed request for a team March 15, 2022 11:17

vedanshujain force-pushed the cot/31766 branch from e42de11 to 9c90a5b Compare March 21, 2022 08:25

jorgeatorres requested changes Mar 22, 2022

View reviewed changes

jorgeatorres reviewed Mar 22, 2022

View reviewed changes

plugins/woocommerce/src/Database/Migrations/CustomOrderTable/WPPostToCOTMigrator.php Show resolved Hide resolved

jorgeatorres reviewed Mar 22, 2022

View reviewed changes

jorgeatorres added the needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. label Mar 23, 2022

github-actions bot added needs: triage feedback Issues for which we requested feedback from the author and received it. and removed needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. labels Mar 24, 2022

vedanshujain marked this pull request as draft March 24, 2022 07:35

vedanshujain removed the needs: triage feedback Issues for which we requested feedback from the author and received it. label Mar 24, 2022

vedanshujain marked this pull request as ready for review April 1, 2022 12:44

vedanshujain requested a review from jorgeatorres April 1, 2022 12:56

jorgeatorres requested changes Apr 5, 2022

View reviewed changes

jorgeatorres added the needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. label Apr 5, 2022

github-actions bot added needs: triage feedback Issues for which we requested feedback from the author and received it. and removed needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. labels Apr 11, 2022

vedanshujain added 16 commits April 11, 2022 19:21

Applied coding standards.

d5b1646

Change parent class reference to try to fix CI.

a220e4f

Applied coding standards + minor fixups.

2270bfb

Applied coding standards.

5b3e9f7

Applied coding standards

f8d5746

Applied coding standards.

8e4287d

Force type to try to fix CI.

d7434ce

Remove incorrectly added file.

b403fa0

Force type to try to fix CI.

81f918a

Minor fixes in bulk migration logic.

bb10c88

Add errors to log.

c5df64c

Applied coding standards.

909a57e

Applied coding standards.

c06d8dd

Applied coding standards.

891a943

Typos and DataBase -> Database.

b14b34a

Add changelog.

63ae96a

vedanshujain force-pushed the cot/31766 branch from ee0c714 to 63ae96a Compare April 11, 2022 13:52

jorgeatorres self-requested a review April 11, 2022 17:29

jorgeatorres approved these changes Apr 11, 2022

View reviewed changes

jorgeatorres merged commit e9e382a into trunk Apr 11, 2022

jorgeatorres deleted the cot/31766 branch April 11, 2022 17:30

github-actions bot added this to the 6.5.0 milestone Apr 11, 2022

Konamiman mentioned this pull request Apr 29, 2022

[COT] Effectively synchronize orders from the posts table to the custom orders table #32817

Merged

8 tasks


		namespace Automattic\WooCommerce\DataBase\Migrations;

		use function DeliciousBrains\WP_Offload_Media\Aws3\JmesPath\search;

Add migration to move orders from wp_posts to custom tables #32034

Add migration to move orders from wp_posts to custom tables #32034

Uh oh!

Conversation

vedanshujain commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

Changelog entry

FOR PR REVIEWER ONLY:

Uh oh!

jorgeatorres left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgeatorres commented Mar 22, 2022

Uh oh!

vedanshujain commented Mar 24, 2022

Uh oh!

vedanshujain commented Mar 24, 2022

Uh oh!

vedanshujain commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jorgeatorres left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vedanshujain commented Apr 11, 2022

Uh oh!

jorgeatorres commented Apr 11, 2022

Uh oh!

jorgeatorres left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 11, 2022 • edited by jorgeatorres Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

exonianp commented May 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vedanshujain commented Mar 7, 2022 •

edited

Loading

vedanshujain commented Apr 1, 2022 •

edited

Loading

github-actions bot commented Apr 11, 2022 •

edited by jorgeatorres

Loading