Merge pull request #4770 from NyanKiyoshi/fix/sort-by/attributes/issues

Fixed known issues related of sorting a given attribute ID to sort by

saleor/graphql/product/resolvers.py

@@ -4,11 +4,11 @@
 import graphene_django_optimizer as gql_optimizer
 from django.db.models import Q, Sum
 from graphql import GraphQLError
+from graphql_relay import from_global_id
 
 from ...order import OrderStatus
 from ...product import models
 from ...search.backends import picker
-from ..core.utils import from_global_id_strict_type
 from ..utils import filter_by_period, filter_by_query_param, get_database_id, get_nodes
 from .enums import AttributeSortField, OrderDirection
 from .filters import (
@@ -116,17 +116,24 @@ def sort_products(qs: models.ProductsQueryset, sort_by: Optional["ProductOrder"]
     # Check if one of the required fields was provided
     if sort_by.field and sort_by.attribute_id:
         raise GraphQLError(
-            ("You must provide either `field` or `attributeId` to sort the products.")
+            "You must provide either `field` or `attributeId` to sort the products."
         )
 
+    if not sort_by.field and not sort_by.attribute_id:
+        return qs
+
     direction = sort_by.direction
     sorting_field = sort_by.field
 
+    # If an attribute ID was passed, attempt to convert it
     if sort_by.attribute_id:
+        graphene_type, attribute_pk = from_global_id(sort_by.attribute_id)
         is_ascending = direction == OrderDirection.ASC
-        attribute_pk = from_global_id_strict_type(sort_by.attribute_id, "Attribute")
-        qs = qs.sort_by_attribute(attribute_pk, ascending=is_ascending)
-    else:
+
+        # If the passed attribute ID is valid, execute the sorting
+        if attribute_pk.isnumeric() and graphene_type == "Attribute":
+            qs = qs.sort_by_attribute(attribute_pk, ascending=is_ascending)
+    elif sorting_field:
         qs = qs.order_by(f"{direction}{sorting_field}")
 
     return qs

saleor/product/models.py

@@ -169,10 +169,17 @@ def sort_by_attribute(self, attribute_pk: Union[int, str], ascending: bool = Tru
 
         # Retrieve all the products' attribute data IDs (assignments) and
         # product types that have the given attribute associated to them
+        associated_values = AttributeProduct.objects.filter(
+            attribute_id=attribute_pk
+        ).values_list("pk", "product_type_id")
+
+        if not associated_values:
+            if not ascending:
+                return qs.reverse()
+            return qs
+
         attribute_associations, product_types_associated_to_attribute = zip(
-            *AttributeProduct.objects.filter(attribute_id=attribute_pk).values_list(
-                "pk", "product_type_id"
-            )
+            *associated_values
         )
 
         qs = qs.annotate(

tests/api/test_product_sorting_attributes.py

@@ -503,3 +503,57 @@ def test_sort_product_not_having_attribute_data(api_client, category, count_quer
     # Compare the results
     sorted_results = list(qs)
     assert sorted_results == expected_results
+
+
+@pytest.mark.parametrize(
+    "attribute_id",
+    [
+        "",
+        graphene.Node.to_global_id("Attribute", "not a number"),
+        graphene.Node.to_global_id("Attribute", -1),
+    ],
+)
+def test_sort_product_by_attribute_using_invalid_attribute_id(
+    api_client, product_list_published, attribute_id
+):
+    """Ensure passing an empty attribute ID as sorting field does nothing."""
+
+    query = QUERY_SORT_PRODUCTS_BY_ATTRIBUTE
+
+    # Products are ordered in descending order to ensure we
+    # are not actually trying to sort them at all
+    variables = {"attributeId": attribute_id, "direction": "DESC"}
+
+    response = get_graphql_content(api_client.post_graphql(query, variables))
+    products = response["data"]["products"]["edges"]
+
+    assert len(products) == product_models.Product.objects.count()
+    assert products[0]["node"]["name"] == product_models.Product.objects.first().name
+
+
+@pytest.mark.parametrize("direction", ["ASC", "DESC"])
+def test_sort_product_by_attribute_using_attribute_having_no_products(
+    api_client, products_structures, direction
+):
+    """Ensure passing an empty attribute ID as sorting field does nothing."""
+
+    query = QUERY_SORT_PRODUCTS_BY_ATTRIBUTE
+    attribute_without_products = product_models.Attribute.objects.create(
+        name="Colors 2", slug="colors-2"
+    )
+
+    attribute_id: str = graphene.Node.to_global_id(
+        "Attribute", attribute_without_products.pk
+    )
+    variables = {"attributeId": attribute_id, "direction": direction}
+
+    response = get_graphql_content(api_client.post_graphql(query, variables))
+    products = response["data"]["products"]["edges"]
+
+    if direction == "ASC":
+        expected_first_product = product_models.Product.objects.first()
+    else:
+        expected_first_product = product_models.Product.objects.last()
+
+    assert len(products) == product_models.Product.objects.count()
+    assert products[0]["node"]["name"] == expected_first_product.name