API Pull - Expose Attribute mapping Rules Values

[jorgemd24]

Changes proposed in this Pull Request:

Part of #2146

As we move to a new approach for fetching products using the WC REST API, we need to make product's attribute mapping values accessible. Hence, this PR introduces a new property to the wc/v3/products endpoint, named gla_attributes. This property will contain the attribute values after applying all GLA Mapping Attribute rules, with GLA attributes taking precedence over mapping rules.

Also, I realized that most of the logic for mapping attribute rules resides within WCProductAdapter. In my view, moving this logic to the AttributeManager class would be more logical, given its existing handling of GLA attributes. Therefore, I've moved and adjusted most of the logic from the WCProductAdapter to the AttributeManager class. This adjustment will enable us to independently retrieve attributes from a product, without needing to create an instance of WCProductAdapter.

I was thinking of refactoring WCProductAdapter to use the new methods in the AttributeManager class, however, as we are not going to sync the attributes anymore I believe it's unnecessary to refactor that section as we will delete that part of the code in the next stages.

Screenshots:

Detailed test instructions:

1. Go to GLA -> Attribute -> Create some rules.
2. Make the following request GET wc/v3/products?gla_syncable=1 or GET wc/v3/products/YOUR_PRODUCT_ID?gla_syncable=1
3. See that the response contains a new property named: gla_attributes with the correct values.

4. Update the products by adding a GLA Attribute. This attribute should take precedence over any mapping rule applied to the same attribute.

5. Remove the query parameter gla_syncable=1 and the response should not contain gla_attributes.

Additional details:

Changelog entry

[codecov]

Codecov Report

Attention: Patch coverage is 93.49593% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 64.5%. Comparing base (c9e9bc7) to head (58ac67a).
Report is 1 commits behind head on feature/google-api-project.

Additional details and impacted files

@@                      Coverage Diff                       @@
##             feature/google-api-project   #2351     +/-   ##
==============================================================
+ Coverage                          64.2%   64.5%   +0.3%     
- Complexity                         4478    4521     +43     
==============================================================
  Files                               471     471             
  Lines                             18853   18958    +105     
==============================================================
+ Hits                              12110   12233    +123     
+ Misses                             6743    6725     -18     

Flag	Coverage Δ
php-unit-tests	64.5% <93.5%> (+0.3%)	⬆️

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

Files	Coverage Δ
...ernal/DependencyManagement/CoreServiceProvider.php	0.0% <0.0%> (ø)
...ependencyManagement/IntegrationServiceProvider.php	0.0% <0.0%> (ø)
src/Product/Attributes/AttributeManager.php	94.6% <98.1%> (+4.3%)	⬆️
src/Integration/WPCOMProxy.php	65.1% <77.8%> (-0.3%)	⬇️

... and 4 files with indirect coverage changes

[puntope]

Also, I realized that most of the logic for mapping attribute rules resides within WCProductAdapter. In my view, moving this logic to the AttributeManager class would be more logical, given its existing handling of GLA attributes. Therefore, I've moved and adjusted most of the logic from the WCProductAdapter to the AttributeManager class. This adjustment will enable us to independently retrieve attributes from a product, without needing to create an instance of WCProductAdapter.

I see we keep all the current code in WCAdapter as well and we just duplicate the code. Not sure if we can reuse the functions to avoid duplication.

Also, what about the rest of the filters and overrides in WC Product adapter? Are they not necessary?

[jorgemd24]

Hi @puntope, thanks for taking a look

I see we keep all the current code in WCAdapter as well and we just duplicate the code. Not sure if we can reuse the functions to avoid duplication.

Initially, I considered updating the WCAdapter to use the methods in AttributeManager to avoid duplicated code. However, I'm unsure if it's worthwhile to refactor this part since it will likely become obsolete as we are not pushing the attributes and the product data anymore in the following stages. We could simply remove the attribute part from WCAdapter, or even the entire class.

Also, what about the rest of the filters and overrides in WC Product adapter? Are they not necessary?

Can you point me to the other filters not included in the AttributeManager?

[puntope]

Hi @puntope, thanks for taking a look

I see we keep all the current code in WCAdapter as well and we just duplicate the code. Not sure if we can reuse the functions to avoid duplication.

Initially, I considered updating the WCAdapter to use the methods in AttributeManager to avoid duplicated code. However, I'm unsure if it's worthwhile to refactor this part since it will likely become obsolete as we are not pushing the attributes and the product data anymore in the following stages. We could simply remove the attribute part from WCAdapter, or even the entire class.

Also, what about the rest of the filters and overrides in WC Product adapter? Are they not necessary?

Can you point me to the other filters not included in the AttributeManager?

That's what I mean. Right now all some methods are duplicated between AttributeManager and WCProductAdapter.

[puntope]

Can you point me to the other filters not included in the AttributeManager?

All the methods that are not related to Attribute Mapping. For example the woocommerce_gla_product_attribute_values filter, or woocommerce_gla_product_attribute_value_{attribute} or map_woocommerce_product logic if its still relevant.

[jorgemd24]

@puntope, thanks for the clarification.

That's what I mean. Right now all some methods are duplicated between AttributeManager and WCProductAdapter.

Yes, but it is also too early to remove the methods in WCProductAdapter so we need to wait until the next stage. I added the task here: #2146

• I think the filter woocommerce_gla_product_attribute_values could be handy so merchants could override the attributes. I will add it.

• We are still relying on the woocommerce_gla_product_attribute_value_{attribute} filter because the new method get_all_aggregated_values invokes get_attribute_mapping_rules_values:

  google-listings-and-ads/src/Product/Attributes/AttributeManager.php

  Lines 215 to 222 in 58ac67a

  	$attributes[ $attribute_id ] = $this->format_attribute(
  	apply_filters(
  	"woocommerce_gla_product_attribute_value_{$attribute_id}",
  	$this->get_source( $product, $mapping_rule['source'] ),
  	$product
  	),
  	$attribute_id
  	);

• Regards map_woocommerce_product I think it will eventually be not used since we will not need to map any products anymore. A similar situation could happen with other methods from the class WCProductAdapter. However, it is too early to remove those methods.

[jorgemd24]

Actually, we can't use the same filter woocommerce_gla_product_attribute_values as the third parameter is an instance of WCProductAdapter so I guess we will need to add a new filter, for example, woocommerce_gla_aggregated_product_attribute_values before returning the response to Google. WDYT?

[puntope]

Actually, we can't use the same filter woocommerce_gla_product_attribute_values as the third parameter is an instance of WCProductAdapter so I guess we will need to add a new filter, for example, woocommerce_gla_aggregated_product_attribute_values before returning the response to Google. WDYT?

Thanks @jorgemd24 for all the explanations. What I don't fully understand is why migrate now all the code to AttributeManager instead of keeping it in WCProductAdapter, as far as I get we:

• Still need to have WCProductAdapter
• There will be some duplicated code between the two classes.
• We cannot reuse some of the filters anymore. So users with existing filters won't have that override working for the new feature. But yes for the new one. Which seems inconsistent.

As far as I read, the advantage is to not create an instance of WCProductAdapter. Not sure how critical this is and how it would be affecting the code.

[jorgemd24]

Hey @puntope. I aimed to make the attributes separate from the WCProductAdapter class and consolidate all attribute-related code into the AttributeManager class. It just seems a bit counterintuitive to me that we have to instantiate a WCProductAdapter to apply attribute mapping rules.

Anyway, I've put together an alternative PR that uses the WCProductAdapter and creates an instance of that class to fetch the attribute values after applying the rules and filters. Could you take a look at this PR #2366? so we can decide which option is the best. Thanks once again!

[jorgemd24]

closing this PR in favor of #2366.