Repository: saleor/saleor
Pull request: Disable shipping on checkout process when checkout is not shippable
- If shipping is not allowed/available on the current checkout:
  - `checkoutCreate` doesn't save the shipping address (no error);
  - `checkoutShippingMethodUpdate` now raises a validation error on the `shipping_method` field;
  - `checkoutShippingAddressUpdate` now raises a validation error on the `shipping_address` field.
- If there is an active checkout `checkoutCreate` will now return `created: False` as well as simply returning the active checkout. So now, the client is aware when the checkout was not created. In addition, the field adds some details on how to merge a cart with an active checkout.
- If the `checkoutId` is not existing when invoking `checkoutShippingAddressUpdate` it will raise an error instead of crashing.

---

Closes #4368, closes #4445 and closes #4444.
### Pull Request Checklist

<!-- Please keep this section. It will make maintainer's life easier. -->

1. [x] Privileged views and APIs are guarded by proper permission checks.
1. [x] All visible strings are translated with proper context.
1. [x] All data-formatting is locale-aware (dates, numbers, and so on).
1. [x] Database queries are optimized and the number of queries is constant.
1. [x] Database migration files are up to date.
1. [x] The changes are tested.
1. [x] The code is documented (docstrings, project documentation).
1. [x] GraphQL schema and type definitions are up to date.
1. [x] Changes are mentioned in the changelog.


Problem statement:
Do not accept shipping address if no shipping is required
We shouldn't allow the user to set a shipping address on digital orders.

Cannot capture payments without order associated to them
### What I'm trying to achieve
…

### Steps to reproduce the problem
1. Create a payment through `checkoutPaymentCreate`
2. Try to charge it through `paymentCharge`

### What happened
```
Traceback (most recent call last):
  File "/Users/Development/saleor-venv/lib/python3.7/site-packages/promise/promise.py", line 487, in _resolve_from_executor
    executor(resolve, reject)
  File "/Users/Development/saleor-venv/lib/python3.7/site-packages/promise/promise.py", line 754, in executor
    return resolve(f(*args, **kwargs))
  File "/Users/Development/saleor-venv/lib/python3.7/site-packages/graphql/execution/middleware.py", line 75, in make_it_promise
    return next(*args, **kwargs)
  File "/Users/Development/saleor/saleor/graphql/core/mutations.py", line 207, in mutate
    response = cls.perform_mutation(root, info, **data)
  File "/Users/Development/saleor/saleor/graphql/payment/mutations.py", line 129, in perform_mutation
    gateway_capture(payment, amount)
  File "/Users/Development/saleor/saleor/payment/utils.py", line 121, in func
    return view(payment, *args, **kwargs)
  File "/Users/Development/saleor/saleor/payment/utils.py", line 431, in gateway_capture
    amount=amount,
  File "/Users/Development/saleor/saleor/payment/utils.py", line 280, in call_gateway
    payment, payment_token, **extra_params
  File "/Users/Development/saleor/saleor/payment/utils.py", line 72, in create_payment_information
    if billing_address is None and payment.order.billing_address:
AttributeError: 'NoneType' object has no attribute 'billing_address'
```

Cannot complete checkout on graphql without shipping address
```
Traceback (most recent call last):
  File "/Users/Development/saleor-venv/lib/python3.7/site-packages/promise/promise.py", line 487, in _resolve_from_executor
    executor(resolve, reject)
  File "/Users/Development/saleor-venv/lib/python3.7/site-packages/promise/promise.py", line 754, in executor
    return resolve(f(*args, **kwargs))
  File "/Users/Development/saleor-venv/lib/python3.7/site-packages/graphql/execution/middleware.py", line 75, in make_it_promise
    return next(*args, **kwargs)
  File "/Users/Development/saleor/saleor/graphql/core/mutations.py", line 207, in mutate
    response = cls.perform_mutation(root, info, **data)
  File "/Users/Development/saleor/saleor/graphql/checkout/mutations.py", line 508, in perform_mutation
    shipping_address = order_data["shipping_address"]  # type: models.Address
KeyError: 'shipping_address'
```

Hints from issue discussion:

Commit message: Add prefetch over the passed variant during creation

Patch to review:
diff --git a/saleor/graphql/checkout/mutations.py b/saleor/graphql/checkout/mutations.py
index b1b6c5f9f86..132dfe8ec6c 100644
--- a/saleor/graphql/checkout/mutations.py
+++ b/saleor/graphql/checkout/mutations.py
@@ -2,7 +2,7 @@
 
 import graphene
 from django.conf import settings
-from django.core.exceptions import ValidationError
+from django.core.exceptions import ObjectDoesNotExist, ValidationError
 from django.db import transaction
 from django.utils import timezone
 
@@ -46,6 +46,8 @@
 from ..shipping.types import ShippingMethod
 from .types import Checkout, CheckoutLine
 
+ERROR_DOES_NOT_SHIP = "This checkout doesn't have shipping"
+
 
 def clean_shipping_method(
     checkout: models.Checkout, method: Optional[models.ShippingMethod], discounts
@@ -126,12 +128,25 @@ class CheckoutCreateInput(graphene.InputObjectType):
     )
     email = graphene.String(description="The customer's email address.")
     shipping_address = AddressInput(
-        description="The mailing address to where the checkout will be shipped."
+        description=(
+            "The mailing address to where the checkout will be shipped. "
+            "Note: the address will be ignored if the checkout "
+            "doesn't contain shippable items."
+        )
     )
     billing_address = AddressInput(description="Billing address of the customer.")
 
 
 class CheckoutCreate(ModelMutation, I18nMixin):
+    created = graphene.Field(
+        graphene.Boolean,
+        description=(
+            "Whether the checkout was created or the current active one was returned. "
+            "Refer to checkoutLinesAdd and checkoutLinesUpdate to merge a cart "
+            "with an active checkout."
+        ),
+    )
+
     class Arguments:
         input = CheckoutCreateInput(
             required=True, description="Fields required to create checkout."
@@ -147,7 +162,14 @@ def process_checkout_lines(
         cls, lines
     ) -> Tuple[List[product_models.ProductVariant], List[int]]:
         variant_ids = [line.get("variant_id") for line in lines]
-        variants = cls.get_nodes_or_error(variant_ids, "variant_id", ProductVariant)
+        variants = cls.get_nodes_or_error(
+            variant_ids,
+            "variant_id",
+            ProductVariant,
+            qs=product_models.ProductVariant.objects.prefetch_related(
+                "product__product_type"
+            ),
+        )
         quantities = [line.get("quantity") for line in lines]
 
         check_lines_quantity(variants, quantities)
@@ -171,7 +193,7 @@ def retrieve_billing_address(cls, user, data: dict) -> Optional[models.Address]:
         return None
 
     @classmethod
-    def clean_input(cls, info, instance, data):
+    def clean_input(cls, info, instance: models.Checkout, data):
         cleaned_input = super().clean_input(info, instance, data)
         user = info.context.user
 
@@ -193,23 +215,36 @@ def clean_input(cls, info, instance, data):
         return cleaned_input
 
     @classmethod
-    @transaction.atomic()
-    def save(cls, info, instance, cleaned_input):
+    def save_addresses(
+        cls,
+        instance: models.Checkout,
+        cleaned_input: dict,
+        variants: List[product_models.ProductVariant],
+    ):
         shipping_address = cleaned_input.get("shipping_address")
         billing_address = cleaned_input.get("billing_address")
 
-        if shipping_address:
+        if shipping_address and any(
+            variant.is_shipping_required() for variant in variants
+        ):
             shipping_address.save()
             instance.shipping_address = shipping_address.get_copy()
         if billing_address:
             billing_address.save()
             instance.billing_address = billing_address.get_copy()
 
-        instance.save()
-
+    @classmethod
+    @transaction.atomic()
+    def save(cls, info, instance: models.Checkout, cleaned_input):
         variants = cleaned_input.get("variants")
         quantities = cleaned_input.get("quantities")
 
+        # Save provided addresses and associate them to the checkout
+        cls.save_addresses(instance, cleaned_input, variants)
+
+        # Create the checkout object
+        instance.save()
+
         # Update/create checkout lines
         if variants and quantities:
             for variant, quantity in zip(variants, quantities):
@@ -230,7 +265,7 @@ def perform_mutation(cls, _root, info, **data):
             if checkout is not None:
                 # If user has an active checkout, return it without any
                 # modifications.
-                return CheckoutCreate(checkout=checkout)
+                return CheckoutCreate(checkout=checkout, created=False)
 
             checkout = models.Checkout(user=user)
         else:
@@ -241,7 +276,7 @@ def perform_mutation(cls, _root, info, **data):
         cls.clean_instance(checkout)
         cls.save(info, checkout, cleaned_input)
         cls._save_m2m(info, checkout, cleaned_input)
-        return CheckoutCreate(checkout=checkout)
+        return CheckoutCreate(checkout=checkout, created=True)
 
 
 class CheckoutLinesAdd(BaseMutation):
@@ -384,9 +419,22 @@ class Meta:
 
     @classmethod
     def perform_mutation(cls, _root, info, checkout_id, shipping_address):
-        checkout = cls.get_node_or_error(
-            info, checkout_id, only_type=Checkout, field="checkout_id"
+        pk = from_global_id_strict_type(
+            info, checkout_id, Checkout, field="checkout_id"
         )
+
+        try:
+            checkout = models.Checkout.objects.prefetch_related(
+                "lines__variant__product__product_type"
+            ).get(pk=pk)
+        except ObjectDoesNotExist:
+            raise ValidationError(
+                {"checkout_id": f"Couldn't resolve to a node: {checkout_id}"}
+            )
+
+        if not checkout.is_shipping_required():
+            raise ValidationError({"shipping_address": ERROR_DOES_NOT_SHIP})
+
         shipping_address = cls.validate_address(
             shipping_address, instance=checkout.shipping_address
         )
@@ -407,7 +455,7 @@ class CheckoutBillingAddressUpdate(CheckoutShippingAddressUpdate):
     class Arguments:
         checkout_id = graphene.ID(required=True, description="ID of the Checkout.")
         billing_address = AddressInput(
-            required=True, description=("The billing address of the checkout.")
+            required=True, description="The billing address of the checkout."
         )
 
     class Meta:
@@ -462,12 +510,23 @@ class Meta:
 
     @classmethod
     def perform_mutation(cls, _root, info, checkout_id, shipping_method_id):
-        checkout_id = from_global_id_strict_type(
+        pk = from_global_id_strict_type(
             info, checkout_id, only_type=Checkout, field="checkout_id"
         )
-        checkout = models.Checkout.objects.prefetch_related(
-            "lines__variant__product__collections"
-        ).get(pk=checkout_id)
+
+        try:
+            checkout = models.Checkout.objects.prefetch_related(
+                "lines__variant__product__collections",
+                "lines__variant__product__product_type",
+            ).get(pk=pk)
+        except ObjectDoesNotExist:
+            raise ValidationError(
+                {"checkout_id": f"Couldn't resolve to a node: {checkout_id}"}
+            )
+
+        if not checkout.is_shipping_required():
+            raise ValidationError({"shipping_method": ERROR_DOES_NOT_SHIP})
+
         shipping_method = cls.get_node_or_error(
             info,
             shipping_method_id,
diff --git a/saleor/graphql/core/mutations.py b/saleor/graphql/core/mutations.py
index 1d8f7e0a6cf..f9af4e6c226 100644
--- a/saleor/graphql/core/mutations.py
+++ b/saleor/graphql/core/mutations.py
@@ -120,9 +120,9 @@ def get_node_or_error(cls, info, node_id, field="id", only_type=None):
         return node
 
     @classmethod
-    def get_nodes_or_error(cls, ids, field, only_type=None):
+    def get_nodes_or_error(cls, ids, field, only_type=None, qs=None):
         try:
-            instances = get_nodes(ids, only_type)
+            instances = get_nodes(ids, only_type, qs=qs)
         except GraphQLError as e:
             raise ValidationError({field: str(e)})
         return instances
diff --git a/saleor/graphql/utils.py b/saleor/graphql/utils.py
index a60c5ce01cb..67e7ae87762 100644
--- a/saleor/graphql/utils.py
+++ b/saleor/graphql/utils.py
@@ -62,7 +62,7 @@ def _resolve_graphene_type(type_name):
     raise AssertionError("Could not resolve the type {}".format(type_name))
 
 
-def get_nodes(ids, graphene_type=None):
+def get_nodes(ids, graphene_type=None, qs=None):
     """Return a list of nodes.
 
     If the `graphene_type` argument is provided, the IDs will be validated
@@ -78,7 +78,10 @@ def get_nodes(ids, graphene_type=None):
     if nodes_type and not graphene_type:
         graphene_type = _resolve_graphene_type(nodes_type)
 
-    nodes = list(graphene_type._meta.model.objects.filter(pk__in=pks))
+    if qs is None:
+        qs = graphene_type._meta.model.objects
+
+    nodes = list(qs.filter(pk__in=pks))
     nodes.sort(key=lambda e: pks.index(str(e.pk)))  # preserve order in pks
 
     if not nodes:


Code review comments:
diff_hunk: @@ -193,23 +215,36 @@ def clean_input(cls, info, instance, data):
         return cleaned_input
 
     @classmethod
-    @transaction.atomic()
-    def save(cls, info, instance, cleaned_input):
+    def save_addresses(
+        cls,
+        instance: models.Checkout,
+        cleaned_input: dict,
+        variants: List[product_models.ProductVariant],
+    ):
         shipping_address = cleaned_input.get("shipping_address")
         billing_address = cleaned_input.get("billing_address")
 
-        if shipping_address:
+        if shipping_address and any(
+            variant.is_shipping_required() for variant in variants
path: saleor/graphql/checkout/mutations.py
text: @user1:
What about `checkout.is_shipping_required()`? Could we use it here?

@author:
The issue with this is that there are no lines yet in the checkout object. We could add the lines first, but then it would add an additional query as we would have to:
1. create checkout
1. add the lines
1. add the addresses
1. save checkout

instead of
1. add the addresses
1. create the checkout
1. add the lines

@user1:
I'm only worried that if we changed the logic of `checkout.is_shipping_required` function for some reason, this is one more place in the code that needs to be updated as well. I think one additional query when saving is not a big deal, it's still O(1).