Repository: saleor/saleor
Pull request: Check if sale contains product, category, collection before run celery
closes #5351

# Impact

* [ ] New migrations
* [ ] New/Updated API fields or mutations
* [ ] Deprecated API fields or mutations
* [ ] Removed API types, fields, or mutations

# Pull Request Checklist

<!-- Please keep this section. It will make maintainer's life easier. -->

* [x] Privileged queries and mutations are guarded by proper permission checks
* [x] Database queries are optimized and the number of queries is constant
* [x] Database migration files are up to date
* [x] The changes are tested
* [x] GraphQL schema and type definitions are up to date
* [ ] Changes are mentioned in the changelog


Problem statement:
Creating a new sale raises error in Celery task
### Steps to reproduce the problem
1. Run the following mutation as an admin user (with `MANAGE_DISCOUNTS` permission):
```
mutation {
  saleCreate(input: {name: "Test"}) {
    errors {
      field
      message
    }
    sale {
      id
      name
    }
  }
}
```

The response from API is successful, but in the Django server console I'm getting the following error:

```
ERROR celery.app.trace Task saleor.product.tasks.update_products_minimal_variant_prices_of_discount_task[4ec46245-d1f1-47ae-ab23-0c0ab73a9981] raised unexpected: ValueError('Provide at least one of the ID lists:\n\tproduct_ids,\n\tcategory_ids,\n\tcollection_ids.') [PID:31316:Thread-175]
Traceback (most recent call last):
  File "/Users/marcin/.pyenv/versions/saleor3.8.1/lib/python3.8/site-packages/celery/app/trace.py", line 385, in trace_task
    R = retval = fun(*args, **kwargs)
  File "/Users/marcin/mirumee/saleor-platform/saleor/saleor/product/tasks.py", line 64, in update_products_minimal_variant_prices_of_discount_task
    update_products_minimal_variant_prices_of_discount(discount)
  File "/Users/marcin/mirumee/saleor-platform/saleor/saleor/product/utils/variant_prices.py", line 76, in update_products_minimal_variant_prices_of_discount
    update_products_minimal_variant_prices_of_catalogues(
  File "/Users/marcin/mirumee/saleor-platform/saleor/saleor/product/utils/variant_prices.py", line 62, in update_products_minimal_variant_prices_of_catalogues
    raise ValueError(
ValueError: Provide at least one of the ID lists:
	product_ids,
	category_ids,
	collection_ids.
```

I suppose that the Celery task that recalculates minimal variant prices is run even there are no products to update. Probably an additional check needs to be added to not run the task in this case.

Hints from issue discussion:

Commit message: Fix test

Patch to review:
diff --git a/saleor/graphql/discount/mutations.py b/saleor/graphql/discount/mutations.py
index 8fc8dd3a02d..9ea3be8de34 100644
--- a/saleor/graphql/discount/mutations.py
+++ b/saleor/graphql/discount/mutations.py
@@ -270,7 +270,12 @@ class SaleUpdateMinimalVariantPriceMixin:
     def success_response(cls, instance):
         # Update the "minimal_variant_prices" of the associated, discounted
         # products (including collections and categories).
-        update_products_minimal_variant_prices_of_discount_task.delay(instance.pk)
+        if (
+            instance.products.all()
+            or instance.categories.all()
+            or instance.collections.all()
+        ):
+            update_products_minimal_variant_prices_of_discount_task.delay(instance.pk)
         return super().success_response(instance)
 
 
diff --git a/tests/api/test_product_minimal_variant_price.py b/tests/api/test_product_minimal_variant_price.py
index ee58a4acbbf..364ba170dac 100644
--- a/tests/api/test_product_minimal_variant_price.py
+++ b/tests/api/test_product_minimal_variant_price.py
@@ -499,6 +499,7 @@ def test_sale_create_updates_products_minimal_variant_prices(
     mock_update_minimal_variant_prices_task,
     staff_api_client,
     permission_manage_discounts,
+    product,
 ):
     query = """
     mutation SaleCreate(
@@ -527,6 +528,7 @@ def test_sale_create_updates_products_minimal_variant_prices(
         "name": "Half price product",
         "type": DiscountValueTypeEnum.PERCENTAGE.name,
         "value": "50",
+        "products": [to_global_id("Product", product.id)],
     }
     response = staff_api_client.post_graphql(
         query, variables, permissions=[permission_manage_discounts]
@@ -585,6 +587,7 @@ def test_sale_delete_updates_products_minimal_variant_prices(
     mock_update_minimal_variant_prices_task,
     staff_api_client,
     sale,
+    product,
     permission_manage_discounts,
 ):
     query = """
@@ -600,6 +603,7 @@ def test_sale_delete_updates_products_minimal_variant_prices(
         }
     }
     """
+    sale.products.add(product)
     variables = {"id": to_global_id("Sale", sale.pk)}
     response = staff_api_client.post_graphql(
         query, variables, permissions=[permission_manage_discounts]
@@ -609,7 +613,7 @@ def test_sale_delete_updates_products_minimal_variant_prices(
     content = get_graphql_content(response)
     assert content["data"]["saleDelete"]["errors"] == []
 
-    mock_update_minimal_variant_prices_task.delay.assert_called_once_with(sale.pk)
+    mock_update_minimal_variant_prices_task.delay.assert_not_called()
 
 
 @patch(


Code review comments:
diff_hunk: @@ -270,7 +270,12 @@ class SaleUpdateMinimalVariantPriceMixin:
     def success_response(cls, instance):
         # Update the "minimal_variant_prices" of the associated, discounted
         # products (including collections and categories).
-        update_products_minimal_variant_prices_of_discount_task.delay(instance.pk)
+        if (
+            instance.products.all()
path: saleor/graphql/discount/mutations.py
text: @user1:
Why not `instance.products.exists()`?  I think it should be much faster. Also, why not moving this entire check to `update_products_minimal_variant_prices_of_discount_task`? Then we would be sure that it is checked in every case that you actually use this task.

@user2:
Depends whether `all()` evaluates to `exists()`, that was my initial question. I don't know if @author checked or not what behavior Django adopts in this case

@author:
I didn't move this to task, because in task I only have pk, which later is used to get instance, then it checks if have any products, categories, collections, if not raise error


@author:
@user2 sorry I thought it was statement not question, I checked it and `.all()` doesn't evaluates to `.exists()`