[GTIN] Add WP CLI command for migration #2662

puntope · 2024-11-01T18:30:23Z

Changes proposed in this Pull Request:

Closes part of #2616 as part of pcTzPl-2p2-p2.

This PR creates a WP CLI GTIN Migration command.

Screenshots:

Detailed test instructions:

Checkout the PR and prepare some products for GTIN migration (it should be published variation or simple and have the GTIN in Google for WooCommerce tab and not in Inventory)
Run WP CLI wp wc g4wc gtin-migration start
See the GTIN migrated.

Additional details:

Changelog entry

Add - WP CLI Command for GTIN Migration

codecov · 2024-11-01T18:37:07Z

Codecov Report

Attention: Patch coverage is 0% with 81 lines in your changes missing coverage. Please review.

Project coverage is 65.1%. Comparing base (02e6224) to head (476adf7).
Report is 3 commits behind head on add/support-core-gtin-field.

Files with missing lines	Patch %	Lines
src/Utility/WPCLIMigrationGTIN.php	0.0%	80 Missing ⚠️
...ernal/DependencyManagement/CoreServiceProvider.php	0.0%	1 Missing ⚠️

Additional details and impacted files

@@                       Coverage Diff                       @@
##             add/support-core-gtin-field   #2662     +/-   ##
===============================================================
- Coverage                           65.4%   65.1%   -0.3%     
- Complexity                          4622    4641     +19     
===============================================================
  Files                                475     476      +1     
  Lines                              19337   19418     +81     
===============================================================
  Hits                               12638   12638             
- Misses                              6699    6780     +81

Flag	Coverage Δ
php-unit-tests	`65.1% <0.0%> (-0.3%)`	⬇️

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

Files with missing lines	Coverage Δ
...ernal/DependencyManagement/CoreServiceProvider.php	`0.0% <0.0%> (ø)`
src/Utility/WPCLIMigrationGTIN.php	`0.0% <0.0%> (ø)`

mikkamp

The CLI command is working, but as explained in the comments I think we should handle fatals as well.

I also found the output to be a bit odd mixing the progress bar with output for each product, as it causes the progress bar to jump to multiple lines and there are no new lines in between:

How do other CLI commands handle that? Is it possible to only output status per product when we set a verbose / debug level of output?

mikkamp · 2024-11-05T08:43:19Z

src/Utility/WPCLIMigrationGTIN.php

+	 * Register service and initialize hooks.
+	 */
+	public function register(): void {
+		WP_CLI::add_hook( 'after_wp_load', [ $this, 'register_commands' ] );


I'm not sure how we can setup a environment without WP_CLI, but based on the doc here it sounds like it might not always be available. And since we aren't doing any conditional checks it seems like it would fatal here.

Should we make this class implement Conditional and add a is_needed function to check if WP_CLI is available?

Tweaked here 173824b

mikkamp · 2024-11-05T09:00:09Z

src/Utility/WPCLIMigrationGTIN.php

+
+			$gtin = $this->attribute_manager->get_value( $product, 'gtin' );
+			if ( $gtin ) {
+				$product->set_global_unique_id( $gtin );


This call can throw an error, for example in cases where the GTIN is duplicate (which we didn't previously validate in the GLA data. When I test this I see the following error:

Fatal error: Uncaught WC_Data_Exception: Invalid or duplicated GTIN, UPC, EAN or ISBN. in woocommerce/includes/abstracts/abstract-wc-data.php:943

I didn't test this scenario fully with the regular importer, but I'm pretty sure it will trigger one of the scheduled actions to fail, which should probably be okay, but maybe it should be a bit more graceful so it's not a "fatal error".

Tweaked here 31097f6

mikkamp · 2024-11-05T09:29:06Z

src/Utility/WPCLIMigrationGTIN.php

+			if ( $gtin ) {
+				$product->set_global_unique_id( $gtin );
+				$product->save();
+				WP_CLI::success( sprintf( 'GTIN [ %s ] has been migrated for Product ID: %s - %s', $gtin, $product->get_id(), $product->get_name() ) );


This success message also appears for invalid GTIN's, because set_global_unique_id filters out incorrect characters. That makes the output rather odd:

I get this in the output:

Success: GTIN [ abcd ] has been migrated for Product ID: 115 - Variable No Sync - Tiny GTIN Migration 13 % [=============> ] 0:01 / 0:14 Success: GTIN [ yoast_seo ] has been migrated for Product ID: 2556 - Aerodynamic Leather Wallet - Small

abcd wouldn't have migrated, and how are we handling yoast_seo values? Do we not import them from the Yoast data?

puntope · 2024-11-06T17:01:43Z

Hey @mikkamp Thanks for your suggestions. I tweaked the code addressing them:

Since now some of the logs are debug mode. You need to add --debug to see them.

puntope · 2024-11-06T17:02:13Z

how are we handling yoast_seo values?

Comming in another PR (we need to do it also for the AS version)

mikkamp

Thanks for the changes, it's working smoother now. I can correctly see it importing GTIN numbers now, and it doesn't halt the process when a duplicate is found.

Starting GTIN migration for 2290 products in the store.
GTIN Migration  13 % [======>                                                                                            ] 0:01 / 0:08
Error: Invalid or duplicated GTIN, UPC, EAN or ISBN.
GTIN Migration  100% [===================================================================================================] 0:01 / 0:08
Success: 1 GTIN was migrated in 1 seconds.

I just left a few small comments, but I'll go ahead and approve it since it pretty much ready to go.

mikkamp · 2024-11-07T08:47:18Z

src/Utility/WPCLIMigrationGTIN.php

+					WP_CLI::debug( sprintf( 'GTIN [ %s ] has been migrated for Product ID: %s - %s', $gtin, $product->get_id(), $product->get_name() ) );
+					++$processed;
+				} catch ( Exception $e ) {
+					WP_CLI::error( $e->getMessage(), false );


Can we combine this error with the product ID, so we can know what to fix? Right now I'm getting this as output:

Starting GTIN migration for 2290 products in the store. GTIN Migration 13 % [============> ] 0:01 / 0:15 Error: Invalid or duplicated GTIN, UPC, EAN or ISBN. GTIN Migration 100% [=========================================================================================] 0:01 / 0:15 Warning: No GTIN were migrated.

Which leaves me having to manually figure out which product I should fix.

mikkamp · 2024-11-07T08:58:37Z

src/Utility/WPCLIMigrationGTIN.php

+			}
+
+			$gtin = str_replace( '-', '', $gtin );
+			if ( is_numeric( $gtin ) ) {


Small nitpick, but I would have used ! is_numeric here with a continue like is done for the error checking above. That way we can keep the condition and error message right next to each other. And we don't have to indent the try and catch another time.

Add WP CLI command for GTIN Migration

d719907

puntope self-assigned this Nov 1, 2024

github-actions bot added type: enhancement The issue is a request for an enhancement. changelog: add A new feature, function, or functionality was added. labels Nov 1, 2024

puntope requested a review from a team November 1, 2024 18:30

PHPCS

cb6b1ae

puntope marked this pull request as ready for review November 1, 2024 18:33

mikkamp reviewed Nov 5, 2024

View reviewed changes

puntope added 3 commits November 6, 2024 16:52

Use debug to some verbose messages

e9f21a0

Add is_needed logic

173824b

Handle errors

31097f6

puntope requested a review from mikkamp November 6, 2024 17:02

PHPCS

476adf7

mikkamp approved these changes Nov 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GTIN] Add WP CLI command for migration #2662

[GTIN] Add WP CLI command for migration #2662

puntope commented Nov 1, 2024

codecov bot commented Nov 1, 2024 •

edited

Loading

mikkamp left a comment •

edited

Loading

mikkamp Nov 5, 2024

puntope Nov 6, 2024

mikkamp Nov 5, 2024

puntope Nov 6, 2024

mikkamp Nov 5, 2024

puntope commented Nov 6, 2024 •

edited

Loading

puntope commented Nov 6, 2024

mikkamp left a comment

mikkamp Nov 7, 2024

mikkamp Nov 7, 2024

[GTIN] Add WP CLI command for migration #2662

Are you sure you want to change the base?

[GTIN] Add WP CLI command for migration #2662

Conversation

puntope commented Nov 1, 2024

Changes proposed in this Pull Request:

Screenshots:

Detailed test instructions:

Additional details:

Changelog entry

codecov bot commented Nov 1, 2024 • edited Loading

Codecov Report

mikkamp left a comment • edited Loading

Choose a reason for hiding this comment

mikkamp Nov 5, 2024

Choose a reason for hiding this comment

puntope Nov 6, 2024

Choose a reason for hiding this comment

mikkamp Nov 5, 2024

Choose a reason for hiding this comment

puntope Nov 6, 2024

Choose a reason for hiding this comment

mikkamp Nov 5, 2024

Choose a reason for hiding this comment

puntope commented Nov 6, 2024 • edited Loading

puntope commented Nov 6, 2024

mikkamp left a comment

Choose a reason for hiding this comment

mikkamp Nov 7, 2024

Choose a reason for hiding this comment

mikkamp Nov 7, 2024

Choose a reason for hiding this comment

codecov bot commented Nov 1, 2024 •

edited

Loading

mikkamp left a comment •

edited

Loading

puntope commented Nov 6, 2024 •

edited

Loading