Repository: saleor/saleor
Pull request: Fix checkout with shipping and digital product
I want to merge this change because resolve #5006 

<!-- Please mention all relevant issue numbers. -->

### Screenshots

<!-- If your changes affect the UI, providing "before" and "after" screenshots will
greatly reduce the amount of work needed to review your work. -->

### Pull Request Checklist

<!-- Please keep this section. It will make maintainer's life easier. -->

1. [ ] Privileged views and APIs are guarded by proper permission checks.
1. [ ] All visible strings are translated with proper context.
1. [ ] All data-formatting is locale-aware (dates, numbers, and so on).
1. [ ] Database queries are optimized and the number of queries is constant.
1. [ ] Database migration files are up to date.
1. [ ] The changes are tested.
1. [ ] GraphQL schema and type definitions are up to date.
1. [ ] Changes are mentioned in the changelog.


Problem statement:
unable to checkout on graphql mutation [digital product]
### What I'm trying to achieve
checkout error on when only digital product is left in the cart system, if there is shipping method assigned before

### Steps to reproduce the problem
1. add shippable product
2. add non-shippable (digital) product
4. add shipping method
3. remove all shippable product, left only non-shippable product
4. add shippable product again
5. Error "This checkout doesn't need shipping" caused by execution of graphql mutation `updateCheckoutLine`

### What I expected to happen
able to add shippable product to the checkout cart
suspected: shipping method is not removed when only digital product is left on the cart

Hints from issue discussion:
Thanks for reporting @trence ! We are aware of this issue. If we find a solution, I will let you know. And if you have any feedback or questions, we're happy to help you.

Commit message: Add test for check remove shipping for specific checkout

Patch to review:
diff --git a/saleor/graphql/checkout/mutations.py b/saleor/graphql/checkout/mutations.py
index d8328dffa75..f0e65e9b265 100644
--- a/saleor/graphql/checkout/mutations.py
+++ b/saleor/graphql/checkout/mutations.py
@@ -85,9 +85,14 @@ def update_checkout_shipping_method_if_invalid(checkout: models.Checkout, discou
         checkout.shipping_method = None
         checkout.save(update_fields=["shipping_method"])
 
-    is_valid = clean_shipping_method(
-        checkout=checkout, method=checkout.shipping_method, discounts=discounts
-    )
+    is_valid = True
+    try:
+        is_valid = clean_shipping_method(
+            checkout=checkout, method=checkout.shipping_method, discounts=discounts
+        )
+    except ValidationError:
+        checkout.shipping_method = None
+        checkout.save(update_fields=["shipping_method"])
 
     if not is_valid:
         cheapest_alternative = get_valid_shipping_methods_for_checkout(
@@ -335,7 +340,7 @@ def perform_mutation(cls, _root, info, checkout_id, lines, replace=False):
         quantities = [line.get("quantity") for line in lines]
 
         check_lines_quantity(variants, quantities)
-        update_checkout_shipping_method_if_invalid(checkout, info.context.discounts)
+        # update_checkout_shipping_method_if_invalid(checkout, info.context.discounts)
 
         if variants and quantities:
             for variant, quantity in zip(variants, quantities):
@@ -348,6 +353,7 @@ def perform_mutation(cls, _root, info, checkout_id, lines, replace=False):
                         f"Insufficient product stock: {exc.item}", code=exc.code
                     )
 
+        update_checkout_shipping_method_if_invalid(checkout, info.context.discounts)
         recalculate_checkout_discount(checkout, info.context.discounts)
 
         return CheckoutLinesAdd(checkout=checkout)
diff --git a/tests/api/test_checkout_digital.py b/tests/api/test_checkout_digital.py
index 79f52663afd..5603ac9c8f1 100644
--- a/tests/api/test_checkout_digital.py
+++ b/tests/api/test_checkout_digital.py
@@ -1,18 +1,20 @@
 """Test the API's checkout process over full digital orders."""
+import graphene
 import pytest
-from graphene import Node
 
 from saleor.account.models import Address
 from saleor.checkout import calculations
 from saleor.checkout.error_codes import CheckoutErrorCode
 from saleor.checkout.models import Checkout
 from saleor.checkout.utils import add_variant_to_checkout
+from saleor.graphql.checkout.mutations import update_checkout_shipping_method_if_invalid
 from saleor.order.models import Order
 from tests.api.utils import get_graphql_content
 
 from .test_checkout import (
     MUTATION_CHECKOUT_COMPLETE,
     MUTATION_CHECKOUT_CREATE,
+    MUTATION_CHECKOUT_LINES_UPDATE,
     MUTATION_CHECKOUT_SHIPPING_ADDRESS_UPDATE,
     MUTATION_UPDATE_SHIPPING_METHOD,
 )
@@ -43,7 +45,7 @@ def test_create_checkout(
     address_count = Address.objects.count()
 
     variant = digital_content.product_variant
-    variant_id = Node.to_global_id("ProductVariant", variant.pk)
+    variant_id = graphene.Node.to_global_id("ProductVariant", variant.pk)
 
     checkout_input = {
         "lines": [{"quantity": 1, "variantId": variant_id}],
@@ -108,7 +110,7 @@ def test_checkout_update_shipping_address(
     """Test updating the shipping address of a digital order throws an error."""
 
     checkout = checkout_with_digital_item
-    checkout_id = Node.to_global_id("Checkout", checkout.pk)
+    checkout_id = graphene.Node.to_global_id("Checkout", checkout.pk)
     variables = {"checkoutId": checkout_id, "shippingAddress": graphql_address_data}
 
     response = api_client.post_graphql(
@@ -140,8 +142,8 @@ def test_checkout_update_shipping_method(
     """Test updating the shipping method of a digital order throws an error."""
 
     checkout = checkout_with_digital_item
-    checkout_id = Node.to_global_id("Checkout", checkout.pk)
-    method_id = Node.to_global_id("ShippingMethod", shipping_method.pk)
+    checkout_id = graphene.Node.to_global_id("Checkout", checkout.pk)
+    method_id = graphene.Node.to_global_id("ShippingMethod", shipping_method.pk)
     variables = {"checkoutId": checkout_id, "shippingMethodId": method_id}
 
     # Put a shipping address, to ensure it is still handled properly
@@ -168,7 +170,7 @@ def test_checkout_complete(
 
     order_count = Order.objects.count()
     checkout = checkout_with_digital_item
-    checkout_id = Node.to_global_id("Checkout", checkout.pk)
+    checkout_id = graphene.Node.to_global_id("Checkout", checkout.pk)
     variables = {"checkoutId": checkout_id, "redirectUrl": "https://www.example.com"}
 
     # Set a billing address
@@ -195,3 +197,47 @@ def test_checkout_complete(
     assert (
         Order.objects.count() == order_count + 1
     ), "The order should have been created"
+
+
+def test_remove_shipping_method_if_only_digital_in_checkout(
+    checkout_with_digital_item, address, shipping_method
+):
+    checkout = checkout_with_digital_item
+    checkout.shipping_address = address
+    checkout.shipping_method = shipping_method
+    checkout.save()
+
+    assert checkout.shipping_method
+    update_checkout_shipping_method_if_invalid(checkout, None)
+
+    checkout.refresh_from_db()
+    assert not checkout.shipping_method
+
+
+def test_checkout_lines_update_remove_shipping_if_removed_product_with_shipping(
+    user_api_client, checkout_with_item, digital_content, address, shipping_method
+):
+    checkout = checkout_with_item
+    digital_variant = digital_content.product_variant
+    checkout.shipping_address = address
+    checkout.shipping_method = shipping_method
+    checkout.save()
+    add_variant_to_checkout(checkout, digital_variant, 1)
+    line = checkout.lines.first()
+    variant = line.variant
+
+    variant_id = graphene.Node.to_global_id("ProductVariant", variant.pk)
+    checkout_id = graphene.Node.to_global_id("Checkout", checkout.pk)
+
+    variables = {
+        "checkoutId": checkout_id,
+        "lines": [{"variantId": variant_id, "quantity": 0}],
+    }
+    response = user_api_client.post_graphql(MUTATION_CHECKOUT_LINES_UPDATE, variables)
+    content = get_graphql_content(response)
+
+    data = content["data"]["checkoutLinesUpdate"]
+    assert not data["errors"]
+    checkout.refresh_from_db()
+    assert checkout.lines.count() == 1
+    assert not checkout.shipping_method


Code review comments:
diff_hunk: @@ -85,9 +85,14 @@ def update_checkout_shipping_method_if_invalid(checkout: models.Checkout, discou
         checkout.shipping_method = None
         checkout.save(update_fields=["shipping_method"])
 
-    is_valid = clean_shipping_method(
-        checkout=checkout, method=checkout.shipping_method, discounts=discounts
-    )
+    is_valid = True
+    try:
+        is_valid = clean_shipping_method(
+            checkout=checkout, method=checkout.shipping_method, discounts=discounts
+        )
+    except ValidationError:
path: saleor/graphql/checkout/mutations.py
text: @user1:
Not 100% sure about this - all `ValidationError`s we raise in Graphene are supposed to be returned as errors in API. If we're catching it here, it means that maybe we shouldn't raise it in `clean_shipping_method`?  Maybe resetting the shipping method should happen in `update_checkout_shipping_method_if_invalid`?

@author:
How about the current solution?