Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor & fix undesired exceptions in AS Jobs #2666

Open
wants to merge 1 commit into
base: add/support-core-gtin-field
Choose a base branch
from
Open

Refactor & fix undesired exceptions in AS Jobs #2666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions src/HelperTraits/GTINMigrationUtilities.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use Automattic\WooCommerce\GoogleListingsAndAds\Jobs\MigrateGTIN;
use Automattic\WooCommerce\GoogleListingsAndAds\Options\OptionsAwareTrait;
use Automattic\WooCommerce\GoogleListingsAndAds\Options\OptionsInterface;
use Exception;

defined( 'ABSPATH' ) || exit;

Expand Down Expand Up @@ -84,4 +85,70 @@
protected function options() {
return $this->options ?? woogle_get_container()->get( OptionsInterface::class );
}

/**
* Prepares the GTIN to be saved.
*
* @param string $gtin
* @return string
*/
protected function prepare_gtin( string $gtin ) {
return str_replace( '-', '', $gtin );

Check warning on line 96 in src/HelperTraits/GTINMigrationUtilities.php

View check run for this annotation

Codecov / codecov/patch

src/HelperTraits/GTINMigrationUtilities.php#L95-L96 

Added lines #L95 - L96 were not covered by tests
}

/**
* Gets the message when the GTIN is invalid.
*
* @param \WC_Product $product
* @param string $gtin
* @return string
*/
protected function error_gtin_invalid( \WC_Product $product, string $gtin ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big deal, and we don't seem to be consistent. But in most cases in the code we add use WC_Product to be able to remove the \. But I'm not attached to either method so both are fine.

return sprintf( 'GTIN [ %s ] has been skipped for Product ID: %s - %s. Invalid GTIN was found.', $gtin, $product->get_id(), $product->get_name() );

Check warning on line 107 in src/HelperTraits/GTINMigrationUtilities.php

View check run for this annotation

Codecov / codecov/patch

src/HelperTraits/GTINMigrationUtilities.php#L106-L107 

Added lines #L106 - L107 were not covered by tests
}

/**
* Gets the message when the GTIN is already in the Product Inventory
*
* @param \WC_Product $product
* @return string
*/
protected function error_gtin_already_set( \WC_Product $product ) {
return sprintf( 'GTIN has been skipped for Product ID: %s - %s. GTIN was found in Product Inventory tab.', $product->get_id(), $product->get_name() );

Check warning on line 117 in src/HelperTraits/GTINMigrationUtilities.php

View check run for this annotation

Codecov / codecov/patch

src/HelperTraits/GTINMigrationUtilities.php#L116-L117 

Added lines #L116 - L117 were not covered by tests
}

/**
* Gets the message when the GTIN is not found.
*
* @param \WC_Product $product
* @return string
*/
protected function error_gtin_not_found( \WC_Product $product ) {
return sprintf( 'GTIN has been skipped for Product ID: %s - %s. No GTIN was found', $product->get_id(), $product->get_name() );

Check warning on line 127 in src/HelperTraits/GTINMigrationUtilities.php

View check run for this annotation

Codecov / codecov/patch

src/HelperTraits/GTINMigrationUtilities.php#L126-L127 

Added lines #L126 - L127 were not covered by tests
}

/**
* Gets the message when the GTIN had an error when saving.
*
* @param \WC_Product $product
* @param string $gtin
* @param Exception $e
*
* @return string
*/
protected function error_gtin_not_saved( \WC_Product $product, string $gtin, Exception $e ) {
return sprintf( 'GTIN [ %s ] for Product ID: %s - %s has an error - %s', $gtin, $product->get_id(), $product->get_name(), $e->getMessage() );

Check warning on line 140 in src/HelperTraits/GTINMigrationUtilities.php

View check run for this annotation

Codecov / codecov/patch

src/HelperTraits/GTINMigrationUtilities.php#L139-L140 

Added lines #L139 - L140 were not covered by tests
}

/**
* Gets the message when the GTIN is successfully migrated.
*
* @param \WC_Product $product
* @param string $gtin
*
* @return string
*/
protected function successful_migrated_gtin( \WC_Product $product, string $gtin ) {
return sprintf( 'GTIN [ %s ] has been migrated for Product ID: %s - %s', $gtin, $product->get_id(), $product->get_name() );

Check warning on line 152 in src/HelperTraits/GTINMigrationUtilities.php

View check run for this annotation

Codecov / codecov/patch

src/HelperTraits/GTINMigrationUtilities.php#L151-L152 

Added lines #L151 - L152 were not covered by tests
}
}
37 changes: 31 additions & 6 deletions src/Jobs/MigrateGTIN.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use Automattic\WooCommerce\GoogleListingsAndAds\Options\OptionsInterface;
use Automattic\WooCommerce\GoogleListingsAndAds\Product\Attributes\AttributeManager;
use Automattic\WooCommerce\GoogleListingsAndAds\Product\ProductRepository;
use Exception;

defined( 'ABSPATH' ) || exit;

Expand Down Expand Up @@ -85,11 +86,6 @@
// update the product core GTIN using G4W GTIN
$products = $this->product_repository->find_by_ids( $items );
foreach ( $products as $product ) {
// void if core GTIN is already set.
if ( $product->get_global_unique_id() ) {
continue;
}

// process variations
if ( $product instanceof \WC_Product_Variable ) {
$variations = $product->get_children();
Expand All @@ -98,9 +94,23 @@
}

$gtin = $this->attribute_manager->get_value( $product, 'gtin' );
if ( $gtin ) {
if ( ! $gtin ) {
$this->debug( $this->error_gtin_not_found( $product ) );
continue;

Check warning on line 99 in src/Jobs/MigrateGTIN.php

View check run for this annotation

Codecov / codecov/patch

src/Jobs/MigrateGTIN.php#L97-L99 

Added lines #L97 - L99 were not covered by tests
}

$gtin = $this->prepare_gtin( $gtin );
if ( ! is_numeric( $gtin ) ) {
$this->debug( $this->error_gtin_invalid( $product, $gtin ) );
continue;

Check warning on line 105 in src/Jobs/MigrateGTIN.php

View check run for this annotation

Codecov / codecov/patch

src/Jobs/MigrateGTIN.php#L102-L105 

Added lines #L102 - L105 were not covered by tests
}

try {
$product->set_global_unique_id( $gtin );
$product->save();
$this->debug( $this->successful_migrated_gtin( $product, $gtin ) );
} catch ( Exception $e ) {
$this->debug( $this->error_gtin_not_saved( $product, $gtin, $e ) );

Check warning on line 113 in src/Jobs/MigrateGTIN.php

View check run for this annotation

Codecov / codecov/patch

src/Jobs/MigrateGTIN.php#L111-L113 

Added lines #L111 - L113 were not covered by tests
}
}
}
Expand Down Expand Up @@ -140,4 +150,19 @@
protected function get_batch( int $batch_number ): array {
return $this->product_repository->find_all_product_ids( $this->get_batch_size(), $this->get_query_offset( $batch_number ) );
}

/**
* Debug info in the logs.
*
* @param string $message
*
* @return void
*/
protected function debug( string $message ): void {
do_action(
'woocommerce_gla_debug_message',
$message,
__METHOD__
);

Check warning on line 166 in src/Jobs/MigrateGTIN.php

View check run for this annotation

Codecov / codecov/patch

src/Jobs/MigrateGTIN.php#L161-L166 

Added lines #L161 - L166 were not covered by tests
}
}
38 changes: 29 additions & 9 deletions src/Utility/WPCLIMigrationGTIN.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

namespace Automattic\WooCommerce\GoogleListingsAndAds\Utility;

use Automattic\WooCommerce\GoogleListingsAndAds\HelperTraits\GTINMigrationUtilities;
use Automattic\WooCommerce\GoogleListingsAndAds\Infrastructure\Conditional;
use Automattic\WooCommerce\GoogleListingsAndAds\Infrastructure\Registerable;
use Automattic\WooCommerce\GoogleListingsAndAds\Infrastructure\Service;
Expand All @@ -22,7 +23,7 @@
* @since x.x.x
*/
class WPCLIMigrationGTIN implements Service, Registerable, Conditional {

use GTINMigrationUtilities;

/** @var AttributeManager */
public AttributeManager $attribute_manager;
Expand Down Expand Up @@ -157,7 +158,6 @@
* @return int The number of items processed.
*/
protected function process_items( array $items ): int {
// todo: refactor avoiding duplication with MigrateGTIN Job after https://github.com/woocommerce/google-listings-and-ads/pull/2656 gets merged.
// update the product core GTIN using G4W GTIN
$products = $this->product_repository->find_by_ids( $items );
$processed = 0;
Expand All @@ -172,30 +172,29 @@

// void if core GTIN is already set.
if ( $product->get_global_unique_id() ) {
WP_CLI::debug( sprintf( 'GTIN has been skipped for Product ID: %s - %s. GTIN was found in Product Inventory tab.', $product->get_id(), $product->get_name() ) );
$this->debug( $this->error_gtin_already_set( $product ) );

Check warning on line 175 in src/Utility/WPCLIMigrationGTIN.php

View check run for this annotation

Codecov / codecov/patch

src/Utility/WPCLIMigrationGTIN.php#L175 

Added line #L175 was not covered by tests
continue;
}

$gtin = $this->attribute_manager->get_value( $product, 'gtin' );
if ( ! $gtin ) {
WP_CLI::debug( sprintf( 'GTIN has been skipped for Product ID: %s - %s. No GTIN was found', $product->get_id(), $product->get_name() ) );
$this->debug( $this->error_gtin_not_found( $product ) );

Check warning on line 181 in src/Utility/WPCLIMigrationGTIN.php

View check run for this annotation

Codecov / codecov/patch

src/Utility/WPCLIMigrationGTIN.php#L181 

Added line #L181 was not covered by tests
continue;
}

$gtin = str_replace( '-', '', $gtin );

$gtin = $this->prepare_gtin( $gtin );

Check warning on line 185 in src/Utility/WPCLIMigrationGTIN.php

View check run for this annotation

Codecov / codecov/patch

src/Utility/WPCLIMigrationGTIN.php#L185 

Added line #L185 was not covered by tests
if ( ! is_numeric( $gtin ) ) {
WP_CLI::debug( sprintf( 'GTIN [ %s ] has been skipped for Product ID: %s - %s. Invalid GTIN was found.', $gtin, $product->get_id(), $product->get_name() ) );
$this->debug( $this->error_gtin_invalid( $product, $gtin ) );

Check warning on line 187 in src/Utility/WPCLIMigrationGTIN.php

View check run for this annotation

Codecov / codecov/patch

src/Utility/WPCLIMigrationGTIN.php#L187 

Added line #L187 was not covered by tests
continue;
}

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

Check warning on line 195 in src/Utility/WPCLIMigrationGTIN.php

View check run for this annotation

Codecov / codecov/patch

src/Utility/WPCLIMigrationGTIN.php#L195 

Added line #L195 was not covered by tests
} catch ( Exception $e ) {
WP_CLI::error( sprintf( 'GTIN [ %s ] for Product ID: %s - %s has an error - %s', $gtin, $product->get_id(), $product->get_name(), $e->getMessage() ), false );
$this->error( $this->error_gtin_not_saved( $product, $gtin, $e ) );

Check warning on line 197 in src/Utility/WPCLIMigrationGTIN.php

View check run for this annotation

Codecov / codecov/patch

src/Utility/WPCLIMigrationGTIN.php#L197 

Added line #L197 was not covered by tests
}
}

Expand All @@ -211,4 +210,25 @@
public static function is_needed(): bool {
return defined( 'WP_CLI' ) && WP_CLI;
}

/**
* Add some info in the debug console.
* Add --debug to see this kind of logs in WP CLI
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small nitpick about the wording (this should be plural):

Suggested change
* Add --debug to see this kind of logs in WP CLI
* Add --debug to see these logs in WP CLI

*
* @param string $message
* @return void
*/
protected function debug( string $message ): void {
WP_CLI::debug( $message );

Check warning on line 222 in src/Utility/WPCLIMigrationGTIN.php

View check run for this annotation

Codecov / codecov/patch

src/Utility/WPCLIMigrationGTIN.php#L221-L222 

Added lines #L221 - L222 were not covered by tests
}

/**
* Add some info in the error console.
*
* @param string $message
* @return void
*/
protected function error( string $message ): void {
WP_CLI::error( $message, false );

Check warning on line 232 in src/Utility/WPCLIMigrationGTIN.php

View check run for this annotation

Codecov / codecov/patch

src/Utility/WPCLIMigrationGTIN.php#L231-L232 

Added lines #L231 - L232 were not covered by tests
}
}