Fix filter by attribute widget now working for "Any..." attributes #27508

Konamiman · 2020-08-28T08:18:03Z

All Submissions:

Have you followed the WooCommerce Contributing guideline?
Does your code follow the WordPress' coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

This pull requests fixes two bugs related to attribute filtering for variable products:

Filter by attribute widget not counting variations having an attribute value of "Any..."

When a variation has a value of "Any..." for a given attribute, then that variation isn't counted by the "Filter by attribute" widget corresponding to that attribute.

attribute_ metadata entries with empty values not handled correctly when adding or removing variation attributes

This is an old bug (present in pre-4.4 versions). When a new variation attribute is added to the main product, then all the already existing variations will display the attribute with a value of "Any...", but the corresponding attribute_ metadata entry for the variation won't be created. (If an existing variation is manually set to "Any..." and saved then it works as expected and the entry is created)

Conversely, when removing the attribute from the main product the corresponding attribute_ metadata entries for the variations won't be removed.

A data migration is added too to backfill possible missing values for these metadata entries.

Closes #27419.

How to test the changes in this Pull Request:

Start with an empty shop (no products).

Create an attribute named "Color" with three terms, e.g. "Black", "White", "Blue".
Create an attribute named "Style" with three terms, e.g. "Classic", "Sport", "Beach".
Add a "Filter by attribute" widget, to filter by "Color".
Add a variable product in which the "Color" attribute is used for all variations, with one variation per color.
Load the shop, verify that the filter by color widget is displaying all three colors, with a count of 1 for each.
Add a second variable product, but this time use the "Style" attribute to generate all variations.
After the three style variations have been created, add "Color" as an additional variation attribute with all three values. Now the existing variations should automatically get "Any..." as the value for the color attribute:

Reload the shop. Without the fix, it still displays 1 for all colors. With the fix, it displays 2 for each color (it's counting the "Any..." product now).
Set one of the variations to "Out of stock". Reload the shop and verify that the value of the counters hasn't changed.
Set all the variations to "Out of stock". Reload the shop and verify that the value of the counters is now 1 (and now the product doesn't display in the shop). Set the variations to "In stock" again.
Remove the "Color" attribute from the product, reload the shop and verify that all counters go back to 1.

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you successfully run tests with your changes locally?

Changelog entry

Fix - Filter by attribute widget not working properly for variations having attribute values of "Any..."
Fix - "attribute_" metadata entries not created and deleted properly for variations

When a variation product has an attribute with a value of "Any...", and there's a filter by attribute widget for that attribute, then that product won't be included in the counts displayed in the widget (and if the count ends up being zero, the attribute won't be shown in the widget). This happens before since Woo 4.4, this widget works by looking at entries in the term relationships table for varitions too (used to do so for simple products and for "main" variable products only), see PR #26260; but there are no such entries for "Any..." attributes. This commit fixes that by extending the SQL query used by the widget to look for variations that have empty attribute values in the meta table too.

…r removed. When a new variation attribute is added, the corresponding 'attribute_' meta entries are added for all variations with an empty value; and when an existing variation attribute is removed, the existing 'attribute_' meta entries are deleted for all variations. This is necessary for the filter by attribute widget to work properly when variations exist with a value of "Any..." for attributes.

The previous commit fixes a bug that causes the "attribute_" metadata with an empty value to not be created when a new variation attribute is added to the product (so that all variations have the attribute with a value of "Any..."). This commit adds a data migration to backfill the missing metadata for existing variations.

rrennick · 2020-08-28T13:06:40Z

@Konamiman You've got a unit test failure in PHP 7.4.

rrennick · 2020-08-28T13:28:46Z

@Konamiman In the screenshot below the Test 25133 product has 5 variations that are Blue so the count of 8 is technically correct. However, it's inconsistent with the number showing in the sorting and the number of products shown on the page.

rrennick · 2020-08-28T13:49:29Z

The other thing I wanted to test was adding an attribute term after the variable product had been created. I added Extra Large.

The first test product has Any size

The widget on refresh

A hook needs to be added on updating attribute terms to sync the product lookup table.

Konamiman · 2020-08-31T09:02:41Z

Hi @rrennick. After you add a new term you need to manually add that new term to the list of values used for variations here:

"Any..." value in the context of variations means "any of the values that are used for variations in this product", not "any of all the existing values for the attribute", if I understood correctly how that works.

If what you mean is that adding a new term should automatically add the term to the list of attributes used for variations on all variable products using that attribute: maybe. But I think that's a separate discussion, not really related to this PR as that's the way it worked before.

Right now the filter by attributes widget counts available variations (for variation products). This is confusing since the counter shows numbers that are higher than the actual count of products displayed. This commit changes the query used by the widget so that instead of counting variations it returns the parent product ids, and then counts the distinct values. This also covers the case of products where some of the variations have concrete values and some have "Any..." values.

When set_attributes is used on WC_Product to remove existing attributes the wc_product_attribute_uasort_comparison ends up being called with a null argument, and this breaks tests in PHP 7.4 since null is used as an array. This commit modifies the function so that if null is passed no array access is attempted.

vedanshujain

I am still reviewing and testing the PR, but adding some initial comments that will be helpful.

includes/class-wc-product-variable.php

includes/widgets/class-wc-widget-layered-nav.php

vedanshujain · 2020-08-31T17:19:31Z

includes/widgets/class-wc-widget-layered-nav.php

+		// This one will return non-variable products and variable products with concrete values for the attributes.
 		$query           = array();
-		$query['select'] = "SELECT COUNT( DISTINCT {$wpdb->posts}.ID ) as term_count, terms.term_id as term_count_id";
+		$query['select'] = "SELECT {$wpdb->posts}.post_parent as product_id, terms.term_id as term_count_id";


are we sure about this change? post_parent is set to 0 for normal products, i think this is applicable only for variations.

includes/wc-update-functions.php

ObliviousHarmony

Thanks for this @Konamiman, I've left some comments I think we should address.

Regarding the PR itself however I don't think this is the route that we should take. I'm worried that automatically synchronizing the attributes to variations is a little too destructive and might be considered backwards incompatible. Right now (without this PR), when you remove an attribute from a variable product, none of the terms are removed from the variations. This allows you to add back the attribute without needing to restore a backup.

It also appears that the behavior of the widget isn’t exactly the same as without the PR when things are handled correctly, but that may just be a misunderstanding on my part.

From what I can tell it seems like mostly everything other than this widget is resilient to the missing entries and so perhaps we should just correct the count query and leave the rest as it is for now?

ObliviousHarmony · 2020-08-31T19:37:32Z

includes/class-wc-product-variable.php


+		if ( $this->get_id() ) {
+			$previous_product              = wc_get_product( $this );
+			$previous_variation_attributes = array_keys( $this->data_store->read_variation_attributes( $previous_product ) );


Since the WC_Data class maintains a dirty list in the $data array, could we just pull the current attributes from there instead of loading them like this?

ObliviousHarmony · 2020-08-31T19:39:18Z

includes/class-wc-product-variable.php

 		$this->data_store->sync_managed_variation_stock_status( $this );

+		// Get variation attributes after save.
+		$current_attributes           = array_filter( array_values( $this->get_attributes() ) );


It's a small nitpick but given how we're iterating over $current_attributes how would you feel about performing these transformations there instead?

ObliviousHarmony · 2020-08-31T20:19:45Z

includes/data-stores/class-wc-product-variable-data-store-cpt.php

+	 * @param array      $current_variation_attributes Names of the variation attributes the product has now.
+	 */
+	public function sync_meta_for_variation_attributes( $product, $previous_variation_attributes, $current_variation_attributes ) {
+		$added_attributes   = array_diff( $current_variation_attributes, $previous_variation_attributes );


Another small nitpick but what about just iterating over the input arrays and checking for existence in the other?

ObliviousHarmony · 2020-08-31T20:23:46Z

includes/data-stores/class-wc-product-variable-data-store-cpt.php

+	 * @param array      $previous_variation_attributes Names of the variation attributes the product had before saving.
+	 * @param array      $current_variation_attributes Names of the variation attributes the product has now.
+	 */
+	public function sync_meta_for_variation_attributes( $product, $previous_variation_attributes, $current_variation_attributes ) {


Looking at WC_Product_Variation_Data_Store_CPT::update_attributes it looks like the attributes are located in other places too. Do you think we should replicate some of that behavior as well?

ObliviousHarmony · 2020-08-31T20:46:04Z

includes/widgets/class-wc-widget-layered-nav.php

 * Layered nav widget
 *
- * @package WooCommerce\Widgets
+ * @package WooCommerce/Widgets


We'll need to leave this as a backslash. If some style rule is telling us to change it then we should address that!

includes/widgets/class-wc-widget-layered-nav.php

…ring widget The new query doesn't need empty attribute entries in the meta table, therefore the code that generates them and the migration to backfill the missing existing ones have been removed.

ObliviousHarmony

I've tested this out with various product types and it seems to do what it says on the tin!

One nitpick is that we're now counting variable products using the post_parent instead of their own ID. This behavior is different than what it was doing before as it was previously considering each in-stock variation in the count. This might lead to strange behavior when combining multiple widgets and I think we should use the ID for variations as well.

Konamiman · 2020-09-02T06:51:06Z

One nitpick is that we're now counting variable products using the post_parent instead of their own ID. This behavior is different than what it was doing before as it was previously considering each in-stock variation in the count. This might lead to strange behavior when combining multiple widgets and I think we should use the ID for variations as well.

Originally it did count variations, but as @rrennick pointed out, that resulted in the widget displaying a number that was higher than the actual amount of products being displayed, which was quite confusing. So it now looks at which variations are supposed to be visible, keeps its parent id, and counts how many different parent ids are in the result; this also blends nicely with the counts for simple products.

vedanshujain

I have left a comment about splitting the query, I found out a couple more issues but let's tackle them in different PR:

Looks like we have introduced performance regressions in get_available_variations and then calling it in is_visible_core for every product. This makes the whole page slow if there are many products with a large number of variations.
I am not sure if using IF in select statement and selecting post_parent is the best way because we are applying the rest of the checks to post.ID. to explain, if a variable product is marked out of stock, then the variations are still counted and shown if the particular variations are not marked out of stock. This is because the checking is done on the post.ID but we are bubbling up post.post_parent in the SELECT statement.

Let's deal with these two changes later on, but I think we should still split this query into two and run them separately.

vedanshujain · 2020-09-02T07:23:20Z

includes/widgets/class-wc-widget-layered-nav.php

+		$query_sql = "
+			SELECT COUNT(DISTINCT(product_id)) AS term_count, term_count_id FROM (
+				{$main_query_sql}
+				UNION ALL


Let's not unionize this as mentioned in https://github.com/woocommerce/woocommerce/pull/27508/files#r480269210 and use two separate queries. There are multiple reasons to not go this way:

Performance and locking implications as mentioned in the previous comment.

Easier to kill performance if an extension or theme adds a LIMIT or ORDER BY in any of the union subquery.

Please then cache these queries separately.

Konamiman · 2020-09-02T09:00:46Z

if a variable product is marked out of stock, then the variations are still counted and shown if the particular variations are not marked out of stock

This is the point of the whole thing. Take in account only variations that are in stock, but in the end, get the main product id so that counters are consistent with the list of products shown (which are always the main products).

E.g. assuming one single product, if two variations are in stock for the same attribute, say "blue-big" and "blue-small", then count only one for "blue". Otherwise we would be displaying a count of 2 but there would be only one product in the displayed list, which would be confusing.

Or am I misunderstanding you?

vedanshujain · 2020-09-02T09:20:52Z

Take in account only variations that are in stock, but in the end, get the main product id so that counters are consistent with the list of products shown (which are always the main products).

So this becomes inconsistent with the main query when the parent product is out of stock but variations are in stock. Then we will count it in the widget, but main query won't as the product is tagged out of stock. That said, I mentioned that we will deal with this later because currently the only way to repro this scenario is to convert the product into simple, then make it out of stock, and then convert it back to the variable. This is definitely an edge case, let's not block the whole PR for this. (You can also manually add out_of_stock term relationship on a variable product to repro).

For performance reasons the query is split in two: one for simple products and variations with a concrete attribute value, and another one for variations having "Any..." as the attribute value.

vedanshujain

I have left a comment, but pre-approving since its minor. Note that this whole widget needs perf improvements, but I think we can merge it for now.

vedanshujain · 2020-09-02T16:35:00Z

includes/widgets/class-wc-widget-layered-nav.php

+					{$query}
+				) AS x GROUP BY term_count_id";
+
+			$results = $wpdb->get_results( $query, ARRAY_A ); // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared


lets cache here instead of in callee function, so that both the queries can be cached separately

Do we really need to cache the queries separately? We have split the original query in two for performance reasons, but conceptually they are still a single one; they will always be used together and with the same set of parameters.

jonathansadowski · 2020-09-02T18:33:14Z

Thanks for the reviews and work to get this in to 4.5

This commit reverts the functionality introduced in PR #26260 (later refined by #27175, #27190, #27508) in which filtering by attribute using the layered nav widget was improved to handle the cases of variations out of stock. The revert is a response to the numerous problems reported by users in Woo 4.4 and 4.5 Not all the code has been reverted, only the code that resulted in visible functionality changes. Thus, the code that generates term relationships for variations is still in place to keep database consistency and to keep the reverting changes to the minimum needed.

Gasper90 · 2020-09-10T17:49:16Z

This is still not working on our side. The filter doesn't show up on the page.

jonathansadowski · 2020-09-10T18:39:01Z

Thanks for the report @Gasper90. This PR hasn't been included in a release yet. We're planning on pushing a release with this fix early next week. Are you using a WooCommerce release version? If so, can I ask you to add your report to #27419 so that we can be sure that we're tracking it appropriately?

Gasper90 · 2020-09-10T21:50:49Z

Ok, because I saw this in the changelog:

Fix – Filter by attribute widget not working properly for variations having attribute values of “Any…”. Fix filter by attribute widget now working for "Any..." attributes #27508

jonathansadowski · 2020-09-11T13:54:45Z

Sorry @Gasper90, I thought I was looking at the revert commit (#27625). You're correct. We originally included this PR in hopes that it would improve things, and have a revert queued for early next week.

Gasper90 · 2020-09-14T20:01:50Z

It works now! 👍

Konamiman added 3 commits August 27, 2020 10:59

Konamiman added status: needs review release: cherry-pick Commits from this PR also needs to be added to current release branch. labels Aug 28, 2020

Konamiman added this to the 4.5.0 milestone Aug 28, 2020

Konamiman self-assigned this Aug 28, 2020

Konamiman requested a review from vedanshujain August 28, 2020 08:18

Konamiman added 2 commits August 31, 2020 12:22

vedanshujain reviewed Aug 31, 2020

View reviewed changes

includes/class-wc-product-variable.php Outdated Show resolved Hide resolved

includes/widgets/class-wc-widget-layered-nav.php Outdated Show resolved Hide resolved

vedanshujain reviewed Aug 31, 2020

View reviewed changes

includes/wc-update-functions.php Outdated Show resolved Hide resolved

ObliviousHarmony self-requested a review August 31, 2020 18:44

ObliviousHarmony requested changes Sep 1, 2020

View reviewed changes

Improve the query to retrieve "Any..."-attributed variations in filte…

07b62da

…ring widget The new query doesn't need empty attribute entries in the meta table, therefore the code that generates them and the migration to backfill the missing existing ones have been removed.

ObliviousHarmony approved these changes Sep 1, 2020

View reviewed changes

vedanshujain suggested changes Sep 2, 2020

View reviewed changes

Split the query for counting terms in the filter by attributes widget.

417bcf8

For performance reasons the query is split in two: one for simple products and variations with a concrete attribute value, and another one for variations having "Any..." as the attribute value.

vedanshujain approved these changes Sep 2, 2020

View reviewed changes

jonathansadowski merged commit 53c22ba into master Sep 2, 2020

jonathansadowski deleted the fix/27419 branch September 2, 2020 18:32

woocommercebot added release: add changelog Mark all PRs that have not had their changelog entries added. [auto] release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] labels Sep 2, 2020

jonathansadowski added cherry picked and removed release: cherry-pick Commits from this PR also needs to be added to current release branch. labels Sep 2, 2020

Konamiman removed the status: needs review label Sep 3, 2020

Konamiman mentioned this pull request Sep 4, 2020

Enhance the query for "Any" variations in filter by attribute widget #27589

Merged

6 tasks

Konamiman mentioned this pull request Sep 9, 2020

Revert improved filtering for variations #27625

Merged

6 tasks

vedanshujain restored the fix/27419 branch September 10, 2020 08:28

kalessil deleted the fix/27419 branch September 24, 2024 06:54

Fix filter by attribute widget now working for "Any..." attributes #27508

Fix filter by attribute widget now working for "Any..." attributes #27508

Uh oh!

Conversation

Konamiman commented Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

Changelog entry

Uh oh!

rrennick commented Aug 28, 2020

Uh oh!

rrennick commented Aug 28, 2020

Uh oh!

rrennick commented Aug 28, 2020

Uh oh!

Konamiman commented Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vedanshujain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ObliviousHarmony left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ObliviousHarmony left a comment

Choose a reason for hiding this comment

Uh oh!

Konamiman commented Sep 2, 2020

Uh oh!

vedanshujain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Konamiman commented Sep 2, 2020

Uh oh!

vedanshujain commented Sep 2, 2020

Uh oh!

vedanshujain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathansadowski commented Sep 2, 2020

Uh oh!

Gasper90 commented Sep 10, 2020

Uh oh!

jonathansadowski commented Sep 10, 2020

Uh oh!

Gasper90 commented Sep 10, 2020

Uh oh!

jonathansadowski commented Sep 11, 2020

Uh oh!

Gasper90 commented Sep 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Konamiman commented Aug 28, 2020 •

edited

Loading

Konamiman commented Aug 31, 2020 •

edited

Loading

ObliviousHarmony left a comment •

edited

Loading