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.

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

Refactor product issues #2277

Merged
merged 20 commits into from
Mar 5, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
1 change: 1 addition & 0 deletions src/API/Google/MerchantReport.php
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ public function get_product_view_report( $next_page_token = null ): array {
}

$product_view_data['statuses'][ $wc_product_id ] = [
'mc_id' => $product_view->getId(),
'product_id' => $wc_product_id,
'status' => $mc_product_status,
'expiration_date' => $this->convert_shopping_content_date( $product_view->getExpirationDate() ),
Expand Down
5 changes: 3 additions & 2 deletions src/DB/Table/MerchantIssueTable.php
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,10 @@
* Delete stale issue records.
*
* @param DateTime $created_before Delete all records created before this.
* @param string $compare Comparison operator to use. Default is '<'.
Copy link
Contributor

Choose a reason for hiding this comment

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

💅 Maybe we can delete the extra tabulation between $compare and the description.

*/
public function delete_stale( DateTime $created_before ) {
$query = "DELETE FROM `{$this->get_sql_safe_name()}` WHERE `created_at` < '%s'";
public function delete_stale( DateTime $created_before, string $compare = '<' ) {
$query = "DELETE FROM `{$this->get_sql_safe_name()}` WHERE `created_at` {$compare} '%s'";

Check warning on line 61 in src/DB/Table/MerchantIssueTable.php

View check run for this annotation

Codecov / codecov/patch

src/DB/Table/MerchantIssueTable.php#L60-L61

Added lines #L60 - L61 were not covered by tests
$this->wpdb->query( $this->wpdb->prepare( $query, $created_before->format( 'Y-m-d H:i:s' ) ) ); // phpcs:ignore WordPress.DB.PreparedSQL
}

Expand Down
3 changes: 1 addition & 2 deletions src/Jobs/UpdateMerchantProductStatuses.php
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,7 @@ public function process_items( array $items ) {

// Clear the cache if we're starting from the beginning.
if ( ! $next_page_token ) {
$this->merchant_statuses->clear_cache();
$this->merchant_statuses->delete_product_statuses_count_intermediate_data();
$this->merchant_statuses->clear_product_statuses_cache_and_issues();
}

$results = $this->merchant_report->get_product_view_report( $next_page_token );
Expand Down
207 changes: 90 additions & 117 deletions src/MerchantCenter/MerchantStatuses.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
use Automattic\WooCommerce\GoogleListingsAndAds\Options\OptionsInterface;
use DateTime;
use Exception;
use WC_Product;
puntope marked this conversation as resolved.
Show resolved Hide resolved

/**
* Class MerchantStatuses.
Expand Down Expand Up @@ -92,7 +93,7 @@
];

/**
* @var string[] Lookup of WooCommerce Product Names.
* @var WC_Product[] Lookup of WooCommerce Product Objects.
*/
protected $product_data_lookup = [];

Expand Down Expand Up @@ -190,10 +191,32 @@
*
* @since x.x.x
*/
public function delete_product_statuses_count_intermediate_data(): void {
protected function delete_product_statuses_count_intermediate_data(): void {
$this->options->delete( OptionsInterface::PRODUCT_STATUSES_COUNT_INTERMEDIATE_DATA );
}

/**
* Delete the stale issues from the database.
*
* @param string $compare The comparison operator to use for the created_at field.
*
* @since x.x.x
*/
protected function delete_stale_issues( string $compare = '<' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

💅 Maybe we might add : void to be consistent with the rest of the class signatures.

$this->container->get( MerchantIssueTable::class )->delete_stale( $this->cache_created_time, $compare );
}

/**
* Clear the product statuses cache and delete stale issues.
*
* @since x.x.x
*/
public function clear_product_statuses_cache_and_issues(): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

💅 Maybe we can add also some tests for this new logic.

$this->clear_cache();
$this->delete_stale_issues();
$this->delete_product_statuses_count_intermediate_data();

Check warning on line 217 in src/MerchantCenter/MerchantStatuses.php

View check run for this annotation

Codecov / codecov/patch

src/MerchantCenter/MerchantStatuses.php#L214-L217

Added lines #L214 - L217 were not covered by tests
}

/**
* Check if the Merchant Center account is connected and throw an exception if it's not.
*
Expand Down Expand Up @@ -235,14 +258,6 @@
// Update account-level issues.
$this->refresh_account_issues();

// Update MC product issues and tabulate statistics in batches.
puntope marked this conversation as resolved.
Show resolved Hide resolved
$chunk_size = apply_filters( 'woocommerce_gla_merchant_status_google_ids_chunk', 1000 );
foreach ( array_chunk( $this->get_synced_google_ids(), $chunk_size ) as $google_ids ) {
$mc_product_statuses = $this->filter_valid_statuses( $google_ids );
// TODO: Get the product issues with the Product View Report.
$this->refresh_product_issues( $mc_product_statuses );
}

// Update pre-sync product validation issues.
$this->refresh_presync_product_issues();

Expand Down Expand Up @@ -354,67 +369,71 @@
}

/**
* Return only the valid statuses from a provided array of Google IDs. Invalid statuses:
* - Aren't synced by this extension (invalid ID format), or
* - Map to products no longer in WooCommerce (deleted or uploaded by a previous connection).
* Also populates the product_data_lookup used in refresh_product_issues()
* Get MC product issues from a list of Product View statuses.
*
* @param string[] $google_ids
* @param [] $statuses The list of Product View statuses.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't be array[] instead of []?

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally. PHPDoc comment doesn't contain all the necessary @throws tags derivated from $this->container->get

*
* @return GoogleProductStatus[] Statuses found to be valid.
* @return array The list of product issues.
*/
protected function filter_valid_statuses( array $google_ids ): array {
protected function get_product_issues( array $statuses ): array {
/** @var Merchant $merchant */
$merchant = $this->container->get( Merchant::class );
/** @var ProductHelper $product_helper */
$product_helper = $this->container->get( ProductHelper::class );
$visibility_meta_key = $this->prefix_meta_key( ProductMetaHandler::KEY_VISIBILITY );

$valid_statuses = [];
$google_ids = array_column( $statuses, 'mc_id' );
$product_issues = [];
$created_at = $this->cache_created_time->format( 'Y-m-d H:i:s' );
foreach ( $merchant->get_productstatuses_batch( $google_ids )->getEntries() as $response_entry ) {
$mc_product_status = $response_entry->getProductStatus();
if ( ! $mc_product_status ) {
do_action(
'woocommerce_gla_debug_message',
'A Google ID was not found in Merchant Center.',
__METHOD__
);
continue;
}
$mc_product_id = $mc_product_status->getProductId();
$wc_product_id = $product_helper->get_wc_product_id( $mc_product_id );
// Skip products not synced by this extension.
if ( ! $wc_product_id ) {
continue;
}

// Product previously found/validated.
if ( ! empty( $this->product_data_lookup[ $wc_product_id ] ) ) {
$valid_statuses[] = $response_entry->getProductStatus();
continue;
}
$mc_product_id = $mc_product_status->getProductId();
$wc_product_id = $product_helper->get_wc_product_id( $mc_product_id );
$wc_product = $this->product_data_lookup[ $wc_product_id ] ?? null;

$wc_product = $product_helper->get_wc_product_by_wp_post( $wc_product_id );
if ( ! $wc_product || 'product' !== substr( $wc_product->post_type, 0, 7 ) ) {
// Should never reach here since the products IDS are retrieved from postmeta.
// Skip products not synced by this extension.
if ( ! $wc_product ) {
do_action(
'woocommerce_gla_debug_message',
sprintf( 'Merchant Center product %s not found in this WooCommerce store.', $mc_product_id ),
__METHOD__ . ' in remove_invalid_statuses()',
);
continue;
}
// Unsynced issues shouldn't be shown.
if ( ChannelVisibility::DONT_SYNC_AND_SHOW === $wc_product->get_meta( $visibility_meta_key ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

💅 Seems that we are not covering this case in tests.

continue;

Check warning on line 405 in src/MerchantCenter/MerchantStatuses.php

View check run for this annotation

Codecov / codecov/patch

src/MerchantCenter/MerchantStatuses.php#L405

Added line #L405 was not covered by tests
}

$this->product_data_lookup[ $wc_product_id ] = [
'name' => get_the_title( $wc_product ),
'visibility' => get_post_meta( $wc_product_id, $visibility_meta_key ),
'parent_id' => $wc_product->post_parent,
$product_issue_template = [
'product' => html_entity_decode( $wc_product->get_name() ),
puntope marked this conversation as resolved.
Show resolved Hide resolved
'product_id' => $wc_product_id,
'created_at' => $created_at,
'applicable_countries' => [],
'source' => 'mc',
];
foreach ( $mc_product_status->getItemLevelIssues() as $item_level_issue ) {
if ( 'merchant_action' !== $item_level_issue->getResolution() ) {
continue;

Check warning on line 417 in src/MerchantCenter/MerchantStatuses.php

View check run for this annotation

Codecov / codecov/patch

src/MerchantCenter/MerchantStatuses.php#L417

Added line #L417 was not covered by tests
}
$hash_key = $wc_product_id . '__' . md5( $item_level_issue->getDescription() );

$this->product_issue_countries[ $hash_key ] = array_merge(
$this->product_issue_countries[ $hash_key ] ?? [],
$item_level_issue->getApplicableCountries()
);

$valid_statuses[] = $response_entry->getProductStatus();
$product_issues[ $hash_key ] = $product_issue_template + [
'code' => $item_level_issue->getCode(),
'issue' => $item_level_issue->getDescription(),
'action' => $item_level_issue->getDetail(),
'action_url' => $item_level_issue->getDocumentation(),
'severity' => $item_level_issue->getServability(),
];
}
}

return $valid_statuses;
return $product_issues;
}

/**
Expand Down Expand Up @@ -488,52 +507,11 @@
}

/**
* Retrieve all product-level issues and store them in the database.
* Refresh product issues in the merchant issues table.
*
* @param GoogleProductStatus[] $validated_mc_statuses Product statuses of validated products.
* @param [] $product_issues Array of product issues.
Copy link
Contributor

Choose a reason for hiding this comment

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

💅 This might be array instead of [] also some throw tags missing.

*/
protected function refresh_product_issues( array $validated_mc_statuses ): void {
/** @var ProductHelper $product_helper */
$product_helper = $this->container->get( ProductHelper::class );

$product_issues = [];
$created_at = $this->cache_created_time->format( 'Y-m-d H:i:s' );
foreach ( $validated_mc_statuses as $product ) {
$wc_product_id = $product_helper->get_wc_product_id( $product->getProductId() );

// Unsynced issues shouldn't be shown.
if ( ChannelVisibility::DONT_SYNC_AND_SHOW === $this->product_data_lookup[ $wc_product_id ]['visibility'] ) {
continue;
}

$product_issue_template = [
'product' => html_entity_decode( $this->product_data_lookup[ $wc_product_id ]['name'] ),
'product_id' => $wc_product_id,
'created_at' => $created_at,
'applicable_countries' => [],
'source' => 'mc',
];
foreach ( $product->getItemLevelIssues() as $item_level_issue ) {
if ( 'merchant_action' !== $item_level_issue->getResolution() ) {
continue;
}
$hash_key = $wc_product_id . '__' . md5( $item_level_issue->getDescription() );

$this->product_issue_countries[ $hash_key ] = array_merge(
$this->product_issue_countries[ $hash_key ] ?? [],
$item_level_issue->getApplicableCountries()
);

$product_issues[ $hash_key ] = $product_issue_template + [
'code' => $item_level_issue->getCode(),
'issue' => $item_level_issue->getDescription(),
'action' => $item_level_issue->getDetail(),
'action_url' => $item_level_issue->getDocumentation(),
'severity' => $item_level_issue->getServability(),
];
}
}

protected function refresh_product_issues( array $product_issues ): void {
// Alphabetize all product/issue country lists.
array_walk(
$this->product_issue_countries,
Expand Down Expand Up @@ -618,31 +596,25 @@
/**
* Process product status statistics.
*
* @param array[] $statuses statuses.
* @param array[] $product_view_statuses Product View statuses.
* @see MerchantReport::get_product_view_report
*/
public function process_product_statuses( array $statuses ): void {
$product_repository = $this->container->get( ProductRepository::class );
$products = $product_repository->find_by_ids_as_associative_array( array_column( $statuses, 'product_id' ) );
public function process_product_statuses( array $product_view_statuses ): void {
puntope marked this conversation as resolved.
Show resolved Hide resolved
$product_repository = $this->container->get( ProductRepository::class );
$this->product_data_lookup = $product_repository->find_by_ids_as_associative_array( array_column( $product_view_statuses, 'product_id' ) );

$this->product_statuses = [
'products' => [],
'parents' => [],
];

foreach ( $statuses as $product_status ) {
foreach ( $product_view_statuses as $product_status ) {

$wc_product_id = $product_status['product_id'];
$mc_product_status = $product_status['status'];
$wc_product = $this->product_data_lookup[ $wc_product_id ] ?? null;

// Skip if the product does not exist in WooCommerce.
if ( ! $wc_product_id ) {
continue;
}

$wc_product = $products[ $wc_product_id ] ?? null;

if ( ! $wc_product ) {
if ( ! $wc_product || ! $wc_product_id ) {
// Skip if the product does not exist in WooCommerce.
do_action(
'woocommerce_gla_debug_message',
Expand All @@ -668,13 +640,16 @@

}

$parent_keys = array_values( array_keys( $this->product_statuses['parents'] ) );
$parent_products = $product_repository->find_by_ids_as_associative_array( $parent_keys );
$products = $products + $parent_products;
$parent_keys = array_values( array_keys( $this->product_statuses['parents'] ) );
$parent_products = $product_repository->find_by_ids_as_associative_array( $parent_keys );
$this->product_data_lookup = $this->product_data_lookup + $parent_products;

// Update each product's mc_status and then update the global statistics.
$this->update_products_meta_with_mc_status( $products );
$this->update_intermediate_product_statistics( $products );
$this->update_products_meta_with_mc_status();
$this->update_intermediate_product_statistics();

$product_issues = $this->get_product_issues( $product_view_statuses );
puntope marked this conversation as resolved.
Show resolved Hide resolved
$this->refresh_product_issues( $product_issues );
}

/**
Expand Down Expand Up @@ -716,11 +691,9 @@
* 3. Compare if a higher priority status is found for that variable product.
* 4. Loop through the `$parent_statuses` array at the end to add the final status counts.
*
* @param WC_Product[] $products The products to update. (passed by reference).
*
* @return array Product status statistics.
*/
protected function update_intermediate_product_statistics( &$products ): array {
protected function update_intermediate_product_statistics(): array {
$product_statistics = [
MCStatus::APPROVED => 0,
MCStatus::PARTIALLY_APPROVED => 0,
Expand Down Expand Up @@ -750,7 +723,7 @@

foreach ( $this->product_statuses['products'] as $product_id => $statuses ) {
foreach ( $statuses as $status => $num_products ) {
$product = $products[ $product_id ] ?? null;
$product = $this->product_data_lookup[ $product_id ] ?? null;

if ( ! $product ) {
continue;
Expand Down Expand Up @@ -826,6 +799,8 @@
*/
public function handle_failed_mc_statuses_fetching( string $error_message = '' ) {
$this->delete_product_statuses_count_intermediate_data();
// Let's remove any issue created during the failed fetch.
$this->delete_stale_issues( '=' );

$mc_statuses = [
'timestamp' => $this->cache_created_time->getTimestamp(),
Expand Down Expand Up @@ -882,10 +857,8 @@

/**
* Update the Merchant Center status for each product.
*
* @param WC_Product[] $products The products to update. (passed by reference).
*/
protected function update_products_meta_with_mc_status( &$products ) {
protected function update_products_meta_with_mc_status() {
// Generate a product_id=>mc_status array.
$new_product_statuses = [];
foreach ( $this->product_statuses as $types ) {
Expand All @@ -907,7 +880,7 @@
}

foreach ( $new_product_statuses as $product_id => $new_status ) {
$product = $products[ $product_id ] ?? null;
$product = $this->product_data_lookup[ $product_id ] ?? null;

// At this point, the product should exist in WooCommerce but in the case that product is not found, log it.
if ( ! $product ) {
Expand Down
2 changes: 2 additions & 0 deletions tests/Unit/API/Google/MerchantReportTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -152,11 +152,13 @@ function ( $mc_id ) use ( $wc_product_id_1, $wc_product_id_2 ) {
[
'statuses' => [
$wc_product_id_1 => [
'mc_id' => 'online:en:ES:gla_' . $wc_product_id_1,
'product_id' => $wc_product_id_1,
'status' => MCStatus::APPROVED,
'expiration_date' => $this->convert_shopping_content_date( $product_view_1->getExpirationDate() ),
],
$wc_product_id_2 => [
'mc_id' => 'online:en:ES:gla_' . $wc_product_id_2,
'product_id' => $wc_product_id_2,
'status' => MCStatus::DISAPPROVED,
'expiration_date' => $this->convert_shopping_content_date( $product_view_2->getExpirationDate() ),
Expand Down
Loading
Loading