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

MC Product Statuses: Replace WP functions with WC functions #2257

Merged
Show file tree
Hide file tree
Changes from 15 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
4 changes: 2 additions & 2 deletions src/API/Google/MerchantReport.php
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@
* @throws Exception If the product view report data can't be retrieved.
*/
public function get_product_view_report( $next_page_token = null ): array {
$batch_size = apply_filters( 'woocommerce_gla_product_view_report_page_size', 1000 );
$batch_size = apply_filters( 'woocommerce_gla_product_view_report_page_size', 500 );

Check warning on line 68 in src/API/Google/MerchantReport.php

View check run for this annotation

Codecov / codecov/patch

src/API/Google/MerchantReport.php#L68

Added line #L68 was not covered by tests

try {
$product_view_data = [
Expand Down Expand Up @@ -97,7 +97,7 @@
continue;
}

$product_view_data['statuses'][] = [
$product_view_data['statuses'][ $wc_product_id ] = [

Check warning on line 100 in src/API/Google/MerchantReport.php

View check run for this annotation

Codecov / codecov/patch

src/API/Google/MerchantReport.php#L100

Added line #L100 was not covered by tests
'product_id' => $wc_product_id,
'status' => $mc_product_status,
'expiration_date' => $this->convert_shopping_content_date( $product_view->getExpirationDate() ),
Expand Down
95 changes: 47 additions & 48 deletions src/MerchantCenter/MerchantStatuses.php
Original file line number Diff line number Diff line change
Expand Up @@ -617,32 +617,28 @@
* @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' ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that the mapping part is moved to the ProductRepository. I was wondering why we didn't already have that and it didn't seem correctly placed handling it in the MerchantStatuses class.


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

/** @var ProductHelper $product_helper */
$product_helper = $this->container->get( ProductHelper::class );
$visibility_meta_key = $this->prefix_meta_key( ProductMetaHandler::KEY_VISIBILITY );

foreach ( $statuses as $product_status ) {

$wc_product_id = $product_status['product_id'];
$mc_product_status = $product_status['status'];

// Skip if the product does not exist or if the product previously found/validated.
if ( ! $wc_product_id || ! empty( $this->product_data_lookup[ $wc_product_id ] ) ) {
// Skip if the product does not exist in WooCommerce.
if ( ! $wc_product_id ) {
continue;
}

if ( $this->product_is_expiring( $product_status['expiration_date'] ) ) {
$mc_product_status = MCStatus::EXPIRING;
}
$wc_product = $products[ $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.
if ( ! $wc_product ) {
// Skip if the product does not exist in WooCommerce.
do_action(
'woocommerce_gla_debug_message',
sprintf( 'Merchant Center product %s not found in this WooCommerce store.', $wc_product_id ),
Expand All @@ -651,23 +647,29 @@
continue;
}

$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,
];
if ( $this->product_is_expiring( $product_status['expiration_date'] ) ) {
$mc_product_status = MCStatus::EXPIRING;
}

// Products is used later for global product status statistics.
$this->product_statuses['products'][ $wc_product_id ][ $mc_product_status ] = 1 + ( $this->product_statuses['products'][ $wc_product_id ][ $mc_product_status ] ?? 0 );

// Aggregate parent statuses for mc_status postmeta.
$wc_parent_id = $this->product_data_lookup[ $wc_product_id ]['parent_id'];
$wc_parent_id = $wc_product->get_parent_id();
if ( ! $wc_parent_id ) {
continue;
}
$this->product_statuses['parents'][ $wc_parent_id ][ $mc_product_status ] = 1 + ( $this->product_statuses['parents'][ $wc_parent_id ][ $mc_product_status ] ?? 0 );

}

$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;

// 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 );
}

/**
Expand All @@ -681,10 +683,6 @@
$this->mc_statuses = [];

$this->process_product_statuses( $statuses );

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

/**
Expand Down Expand Up @@ -713,9 +711,11 @@
* 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(): array {
protected function update_intermediate_product_statistics( &$products ): array {
$product_statistics = [
MCStatus::APPROVED => 0,
MCStatus::PARTIALLY_APPROVED => 0,
Expand Down Expand Up @@ -744,7 +744,14 @@

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

if ( ! $product ) {
continue;

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

View check run for this annotation

Codecov / codecov/patch

src/MerchantCenter/MerchantStatuses.php#L750

Added line #L750 was not covered by tests
}

$parent_id = $product->get_parent_id();

if ( ! $parent_id ) {
$product_statistics[ $status ] += $num_products;
} elseif ( ! isset( $parent_statuses[ $parent_id ] ) ) {
Expand Down Expand Up @@ -823,8 +830,10 @@

/**
* 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() {
protected function update_products_meta_with_mc_status( &$products ) {
// Generate a product_id=>mc_status array.
$new_product_statuses = [];
foreach ( $this->product_statuses as $types ) {
Expand All @@ -844,33 +853,23 @@
}
}
}
ksort( $new_product_statuses );

/** @var ProductMetaQueryHelper $product_meta_query_helper */
$product_meta_query_helper = $this->container->get( ProductMetaQueryHelper::class );

// Get all MC statuses.
$current_product_statuses = $product_meta_query_helper->get_all_values( ProductMetaHandler::KEY_MC_STATUS );

// Format: product_id=>status.
$to_insert = [];
// Format: status=>[product_ids].
$to_update = [];

foreach ( $new_product_statuses as $product_id => $new_status ) {
if ( ! isset( $current_product_statuses[ $product_id ] ) ) {
// MC status not in WC, insert.
$to_insert[ $product_id ] = $new_status;
} elseif ( $current_product_statuses[ $product_id ] !== $new_status ) {
// MC status not same as WC, update.
$to_update[ $new_status ][] = intval( $product_id );
$product = $products[ $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 ) {
do_action(
'woocommerce_gla_debug_message',
sprintf( 'Merchant Center product %s not found in this WooCommerce store.', $product_id ),
Copy link
Contributor

Choose a reason for hiding this comment

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

This error message seems to implicate that we are outputting the MC ID, but at this time we have already mapped it to a WC ID (potentially with a filter and custom mapping). Should we reword it to something like:

Suggested change
sprintf( 'Merchant Center product %s not found in this WooCommerce store.', $product_id ),
sprintf( 'Merchant Center product with WooCommerce ID %d is not found in this store.', $product_id ),

__METHOD__,
);
continue;

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

View check run for this annotation

Codecov / codecov/patch

src/MerchantCenter/MerchantStatuses.php#L862-L867

Added lines #L862 - L867 were not covered by tests
}
}

// Insert and update changed MC Status postmeta.
$product_meta_query_helper->batch_insert_values( ProductMetaHandler::KEY_MC_STATUS, $to_insert );
foreach ( $to_update as $status => $product_ids ) {
$product_meta_query_helper->batch_update_values( ProductMetaHandler::KEY_MC_STATUS, $status, $product_ids );
$product->add_meta_data( $this->prefix_meta_key( ProductMetaHandler::KEY_MC_STATUS ), $new_status, true );
// We use save_meta_data so we don't trigger the woocommerce_update_product hook and the Syncer Hooks.
$product->save_meta_data();
}
}

Expand Down
19 changes: 19 additions & 0 deletions src/Product/ProductRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,25 @@
return $this->find( $args, $limit, $offset );
}

/**
* Find and return an associative array of products with the product ID as the key.
*
* @param int[] $ids Array of WooCommerce product IDs
* @param array $args Array of WooCommerce args (except 'return'), and product metadata.
* @param int $limit Maximum number of results to retrieve or -1 for unlimited.
* @param int $offset Amount to offset product results.
*
* @return WC_Product[] Array of WooCommerce product objects
*/
public function find_by_ids_as_associative_array( array $ids, array $args = [], int $limit = -1, int $offset = 0 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we force this function to have a return type of array? The function already always returns an array. I just noticed you have the code $products + $parent_products so I wanted to confirm that there is no way that those variables are not arrays.

$products = $this->find_by_ids( $ids, $args, $limit, $offset );
$map = [];
foreach ( $products as $product ) {
$map[ $product->get_id() ] = $product;

Check warning on line 115 in src/Product/ProductRepository.php

View check run for this annotation

Codecov / codecov/patch

src/Product/ProductRepository.php#L111-L115

Added lines #L111 - L115 were not covered by tests
}
return $map;

Check warning on line 117 in src/Product/ProductRepository.php

View check run for this annotation

Codecov / codecov/patch

src/Product/ProductRepository.php#L117

Added line #L117 was not covered by tests
}

/**
* Find and return an array of WooCommerce product objects already submitted to Google Merchant Center.
*
Expand Down
Loading
Loading