Repository: saleor/saleor
Pull request: Fix checkoutLinesAdd mutation when empty checkout
I want to merge this change because resolve #3562 

<!-- 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. [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:
Mutation checkoutLinesAdd doesn't work when empty checkout
### What I'm trying to achieve
Add a new product to empty checkout.

### Steps to reproduce the problem
1. Create new checkout.
2. If checkout has any product then remove it.
3. Add a new product to empty checkout.

Hints from issue discussion:

Commit message: Changelog

Patch to review:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8f2b612a2a7..2c7a139624c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -51,3 +51,4 @@ All notable, unreleased changes to this project will be documented in this file.
 - Unique validation SKU when default variant create in GraphQL API - #3555 by @fowczarek
 - Bump backend dependencies - #3557 by @maarcingebala. This PR removes security issue CVE-2019-3498 which was present in Django 2.1.4. Saleor however wasn't vulnerable to this issue as it doesn't use the affected `django.views.defaults.page_not_found()` view.
 - Restyle product selection dialogs - #3499 by @dominik-zeglen, @maarcingebala
+- Fix checkoutLinesAdd mutation when empty checkout - #3563 by @fowczarek
diff --git a/saleor/graphql/checkout/mutations.py b/saleor/graphql/checkout/mutations.py
index 905c8c49fbf..88be35d9aa5 100644
--- a/saleor/graphql/checkout/mutations.py
+++ b/saleor/graphql/checkout/mutations.py
@@ -63,17 +63,17 @@ def check_lines_quantity(variants, quantities):
     """Check if stock is sufficient for each line in the list of dicts.
     Return list of errors.
     """
-    # FIXME Add tests
     errors = []
     for variant, quantity in zip(variants, quantities):
         try:
             variant.check_quantity(quantity)
         except InsufficientStock as e:
             message = (
-                'Add line mutation error',
-                'Could not add item. Only %(remaining)d remaining in stock.' %
-                {'remaining': e.item.quantity_available})
-            errors.append((variant.name, message))
+                'Could not add item '
+                + '%(item_name)s. Only %(remaining)d remaining in stock.' % {
+                    'remaining': e.item.quantity_available,
+                    'item_name': e.item.display_product()})
+            errors.append(('quantity', message))
     return errors
 
 
@@ -166,34 +166,33 @@ def mutate(cls, root, info, checkout_id, lines, replace=False):
         errors = []
         checkout = cls.get_node_or_error(
             info, checkout_id, errors, 'checkout_id', only_type=Checkout)
-        if not checkout:
-            return CheckoutLinesAdd(errors=errors)
-
         variants, quantities = None, None
-        if lines:
-            variant_ids = [line.get('variant_id') for line in lines]
-            variants = cls.get_nodes_or_error(
-                ids=variant_ids, errors=errors, field='variant_id',
-                only_type=ProductVariant)
-            quantities = [line.get('quantity') for line in lines]
-            line_errors = check_lines_quantity(variants, quantities)
-            if line_errors:
-                for err in line_errors:
-                    cls.add_error(errors, field=err[0], message=err[1])
+        if checkout is not None:
+            if lines:
+                variant_ids = [line.get('variant_id') for line in lines]
+                variants = cls.get_nodes_or_error(
+                    ids=variant_ids, errors=errors, field='variant_id',
+                    only_type=ProductVariant)
+                quantities = [line.get('quantity') for line in lines]
+                line_errors = check_lines_quantity(variants, quantities)
+                if line_errors:
+                    for err in line_errors:
+                        cls.add_error(errors, field=err[0], message=err[1])
+
+            # FIXME test if below function is called
+            clean_shipping_method(
+                checkout=checkout, method=checkout.shipping_method,
+                errors=errors, discounts=info.context.discounts,
+                taxes=get_taxes_for_address(checkout.shipping_address))
+
+        if errors:
+            return CheckoutLinesAdd(errors=errors)
 
         if variants and quantities:
             for variant, quantity in zip(variants, quantities):
                 add_variant_to_cart(
                     checkout, variant, quantity, replace=replace)
 
-        # FIXME test if below function is called
-        clean_shipping_method(
-            checkout=checkout, method=checkout.shipping_method, errors=errors,
-            discounts=info.context.discounts,
-            taxes=get_taxes_for_address(checkout.shipping_address))
-        if errors:
-            return CheckoutLinesAdd(errors=errors)
-
         return CheckoutLinesAdd(checkout=checkout, errors=errors)
 
 
@@ -280,12 +279,11 @@ def mutate(cls, root, info, checkout_id):
         errors = []
         checkout = cls.get_node_or_error(
             info, checkout_id, errors, 'checkout_id', only_type=Checkout)
-        if not checkout:
-            return cls(errors=errors)
-        if not checkout.user:
+        if checkout is not None and not checkout.user:
             cls.add_error(
                 errors, field=None,
                 message='There\'s no customer assigned to this Checkout.')
+        if errors:
             return CheckoutCustomerDetach(errors=errors)
 
         checkout.user = None
@@ -311,21 +309,20 @@ def mutate(cls, root, info, checkout_id, shipping_address):
         checkout = cls.get_node_or_error(
             info, checkout_id, errors, 'checkout_id', only_type=Checkout)
 
-        shipping_address, errors = cls.validate_address(
-            shipping_address, errors, instance=checkout.shipping_address)
-        if errors:
-            return CheckoutShippingAddressUpdate(errors=errors)
-
-        clean_shipping_method(
-            checkout, checkout.shipping_method, errors,
-            info.context.discounts, get_taxes_for_address(shipping_address))
-        if errors:
-            CheckoutShippingAddressUpdate(errors=errors)
-
-        if checkout and shipping_address:
-            with transaction.atomic():
-                shipping_address.save()
-                change_shipping_address_in_cart(checkout, shipping_address)
+        if checkout:
+            shipping_address, errors = cls.validate_address(
+                shipping_address, errors, instance=checkout.shipping_address)
+            clean_shipping_method(
+                checkout, checkout.shipping_method, errors,
+                info.context.discounts,
+                get_taxes_for_address(shipping_address))
+            if errors:
+                CheckoutShippingAddressUpdate(errors=errors)
+
+            if shipping_address:
+                with transaction.atomic():
+                    shipping_address.save()
+                    change_shipping_address_in_cart(checkout, shipping_address)
         return CheckoutShippingAddressUpdate(checkout=checkout, errors=errors)
 
 
@@ -402,9 +399,10 @@ def mutate(cls, root, info, checkout_id, shipping_method_id):
             info, shipping_method_id, errors, 'shipping_method_id',
             only_type=ShippingMethod)
 
-        clean_shipping_method(
-            checkout, shipping_method, errors, info.context.discounts,
-            info.context.taxes, remove=False)
+        if checkout and shipping_method:
+            clean_shipping_method(
+                checkout, shipping_method, errors, info.context.discounts,
+                info.context.taxes, remove=False)
         if errors:
             return CheckoutShippingMethodUpdate(errors=errors)
 
@@ -429,7 +427,7 @@ def mutate(cls, root, info, checkout_id):
         errors = []
         checkout = cls.get_node_or_error(
             info, checkout_id, errors, 'checkout_id', only_type=Checkout)
-        if not checkout:
+        if checkout is None:
             return CheckoutComplete(errors=errors)
         taxes = get_taxes_for_cart(checkout, info.context.taxes)
         ready, checkout_error = ready_to_place_order(
diff --git a/tests/api/test_checkout.py b/tests/api/test_checkout.py
index 62778b783d3..787c6e1f8bb 100644
--- a/tests/api/test_checkout.py
+++ b/tests/api/test_checkout.py
@@ -9,18 +9,24 @@
 from tests.api.utils import get_graphql_content
 
 
-def test_checkout_create(user_api_client, variant, graphql_address_data):
-    """Create checkout object using GraphQL API."""
-    query = """
+MUTATION_CHECKOUT_CREATE = """
     mutation createCheckout($checkoutInput: CheckoutCreateInput!) {
         checkoutCreate(input: $checkoutInput) {
             checkout {
                 token,
                 id
             }
+            errors {
+                field
+                message
+            }
         }
     }
     """
+
+
+def test_checkout_create(user_api_client, variant, graphql_address_data):
+    """Create checkout object using GraphQL API."""
     variant_id = graphene.Node.to_global_id('ProductVariant', variant.id)
     test_email = 'test@example.com'
     shipping_address = graphql_address_data
@@ -32,7 +38,8 @@ def test_checkout_create(user_api_client, variant, graphql_address_data):
             'email': test_email,
             'shippingAddress': shipping_address}}
     assert not Cart.objects.exists()
-    response = user_api_client.post_graphql(query, variables)
+    response = user_api_client.post_graphql(
+        MUTATION_CHECKOUT_CREATE, variables)
     content = get_graphql_content(response)
 
     new_cart = Cart.objects.first()
@@ -57,39 +64,58 @@ def test_checkout_create(user_api_client, variant, graphql_address_data):
     assert new_cart.shipping_address.city == shipping_address['city']
 
 
-def test_checkout_lines_add(user_api_client, cart_with_item, variant):
-    """
-    Create checkout object using GraphQL API
-    """
-    cart = cart_with_item
-    line = cart.lines.first()
-    assert line.quantity == 3
-    query = """
-        mutation checkoutLinesAdd($checkoutId: ID!, $lines: [CheckoutLineInput!]!) {
-            checkoutLinesAdd(checkoutId: $checkoutId, lines: $lines) {
-                checkout {
-                    token
-                    lines {
-                        quantity
-                        variant {
-                            id
-                        }
+def test_checkout_create_check_lines_quantity(
+        user_api_client, variant, graphql_address_data):
+    variant_id = graphene.Node.to_global_id('ProductVariant', variant.id)
+    test_email = 'test@example.com'
+    shipping_address = graphql_address_data
+    variables = {
+        'checkoutInput': {
+            'lines': [{
+                'quantity': 3,
+                'variantId': variant_id}],
+            'email': test_email,
+            'shippingAddress': shipping_address}}
+    response = user_api_client.post_graphql(
+        MUTATION_CHECKOUT_CREATE, variables)
+    content = get_graphql_content(response)
+    data = content['data']['checkoutCreate']
+    assert data['errors'][0]['message'] == 'Could not add item Test product (SKU_A). Only 2 remaining in stock.'
+    assert data['errors'][0]['field'] == 'quantity'
+
+
+MUTATION_CHECKOUT_LINES_ADD = """
+    mutation checkoutLinesAdd($checkoutId: ID!, $lines: [CheckoutLineInput!]!) {
+        checkoutLinesAdd(checkoutId: $checkoutId, lines: $lines) {
+            checkout {
+                token
+                lines {
+                    quantity
+                    variant {
+                        id
                     }
                 }
-                errors {
-                    field
-                    message
-                }
+            }
+            errors {
+                field
+                message
             }
         }
-    """
+    }"""
+
+
+def test_checkout_lines_add(user_api_client, cart_with_item, variant):
+    cart = cart_with_item
+    line = cart.lines.first()
+    assert line.quantity == 3
     variant_id = graphene.Node.to_global_id('ProductVariant', variant.pk)
     checkout_id = graphene.Node.to_global_id('Checkout', cart.pk)
 
     variables = {
         'checkoutId': checkout_id,
         'lines': [{'variantId': variant_id, 'quantity': 1}]}
-    response = user_api_client.post_graphql(query, variables)
+    response = user_api_client.post_graphql(
+        MUTATION_CHECKOUT_LINES_ADD, variables)
     content = get_graphql_content(response)
     data = content['data']['checkoutLinesAdd']
     assert not data['errors']
@@ -99,31 +125,69 @@ def test_checkout_lines_add(user_api_client, cart_with_item, variant):
     assert line.quantity == 1
 
 
-def test_checkout_lines_update(user_api_client, cart_with_item):
-    cart = cart_with_item
-    assert cart.lines.count() == 1
+def test_checkout_lines_add_empty_checkout(
+        user_api_client, cart, variant):
+    variant_id = graphene.Node.to_global_id('ProductVariant', variant.pk)
+    checkout_id = graphene.Node.to_global_id('Checkout', cart.pk)
+
+    variables = {
+        'checkoutId': checkout_id,
+        'lines': [{'variantId': variant_id, 'quantity': 1}]}
+    response = user_api_client.post_graphql(
+        MUTATION_CHECKOUT_LINES_ADD, variables)
+    content = get_graphql_content(response)
+    data = content['data']['checkoutLinesAdd']
+    assert not data['errors']
+    cart.refresh_from_db()
     line = cart.lines.first()
-    variant = line.variant
-    assert line.quantity == 3
-    query = """
-        mutation checkoutLinesUpdate($checkoutId: ID!, $lines: [CheckoutLineInput!]!) {
-            checkoutLinesUpdate(checkoutId: $checkoutId, lines: $lines) {
-                checkout {
-                    token
-                    lines {
-                        quantity
-                        variant {
-                            id
-                        }
+    assert line.variant == variant
+    assert line.quantity == 1
+
+
+def test_checkout_lines_add_check_lines_quantity(
+        user_api_client, cart, variant):
+    variant_id = graphene.Node.to_global_id('ProductVariant', variant.pk)
+    checkout_id = graphene.Node.to_global_id('Checkout', cart.pk)
+
+    variables = {
+        'checkoutId': checkout_id,
+        'lines': [{'variantId': variant_id, 'quantity': 3}]}
+    response = user_api_client.post_graphql(
+        MUTATION_CHECKOUT_LINES_ADD, variables)
+    content = get_graphql_content(response)
+    data = content['data']['checkoutLinesAdd']
+    assert data['errors'][0]['message'] == 'Could not add item Test product (SKU_A). Only 2 remaining in stock.'
+    assert data['errors'][0]['field'] == 'quantity'
+
+
+MUTATION_CHECKOUT_LINES_UPDATE = """
+    mutation checkoutLinesUpdate($checkoutId: ID!, $lines: [CheckoutLineInput!]!) {
+        checkoutLinesUpdate(checkoutId: $checkoutId, lines: $lines) {
+            checkout {
+                token
+                lines {
+                    quantity
+                    variant {
+                        id
                     }
                 }
-                errors {
-                    field
-                    message
-                }
+            }
+            errors {
+                field
+                message
             }
         }
+    }
     """
+
+
+def test_checkout_lines_update(user_api_client, cart_with_item):
+    cart = cart_with_item
+    assert cart.lines.count() == 1
+    line = cart.lines.first()
+    variant = line.variant
+    assert line.quantity == 3
+
     variant_id = graphene.Node.to_global_id('ProductVariant', variant.pk)
     checkout_id = graphene.Node.to_global_id('Checkout', cart.pk)
 
@@ -132,7 +196,8 @@ def test_checkout_lines_update(user_api_client, cart_with_item):
         'lines': [{
             'variantId': variant_id,
             'quantity': 1}]}
-    response = user_api_client.post_graphql(query, variables)
+    response = user_api_client.post_graphql(
+        MUTATION_CHECKOUT_LINES_UPDATE, variables)
     content = get_graphql_content(response)
 
     data = content['data']['checkoutLinesUpdate']
@@ -143,11 +208,38 @@ def test_checkout_lines_update(user_api_client, cart_with_item):
     assert line.variant == variant
     assert line.quantity == 1
 
-    # test invalid checkout_id results in error
+
+def test_checkout_lines_update_invalid_checkout_id(
+        user_api_client):
     variables = {'checkoutId': 'test', 'lines': []}
-    response = user_api_client.post_graphql(query, variables)
+    response = user_api_client.post_graphql(
+        MUTATION_CHECKOUT_LINES_UPDATE, variables)
     content = get_graphql_content(response)
-    assert content['data']['checkoutLinesUpdate']['errors'][0]['field'] == 'checkoutId'
+    data = content['data']['checkoutLinesUpdate']
+    assert data['errors'][0]['field'] == 'checkoutId'
+
+
+def test_checkout_lines_update_check_lines_quantity(
+        user_api_client, cart_with_item):
+    cart = cart_with_item
+    line = cart.lines.first()
+    variant = line.variant
+
+    variant_id = graphene.Node.to_global_id('ProductVariant', variant.pk)
+    checkout_id = graphene.Node.to_global_id('Checkout', cart.pk)
+
+    variables = {
+        'checkoutId': checkout_id,
+        'lines': [{
+            'variantId': variant_id,
+            'quantity': 10}]}
+    response = user_api_client.post_graphql(
+        MUTATION_CHECKOUT_LINES_UPDATE, variables)
+    content = get_graphql_content(response)
+
+    data = content['data']['checkoutLinesUpdate']
+    assert data['errors'][0]['message'] == 'Could not add item Test product (123). Only 9 remaining in stock.'
+    assert data['errors'][0]['field'] == 'quantity'
 
 
 def test_checkout_line_delete(user_api_client, cart_with_item):
@@ -218,29 +310,32 @@ def test_checkout_customer_attach(
     assert cart.user == customer_user
 
 
+MUTATION_CHECKOUT_CUSTOMER_DETACH = """
+    mutation checkoutCustomerDetach($checkoutId: ID!) {
+        checkoutCustomerDetach(checkoutId: $checkoutId) {
+            checkout {
+                token
+            }
+            errors {
+                field
+                message
+            }
+        }
+    }
+    """
+
+
 def test_checkout_customer_detach(
         user_api_client, cart_with_item, customer_user):
     cart = cart_with_item
     cart.user = customer_user
     cart.save(update_fields=['user'])
 
-    query = """
-        mutation checkoutCustomerDetach($checkoutId: ID!) {
-            checkoutCustomerDetach(checkoutId: $checkoutId) {
-                checkout {
-                    token
-                }
-                errors {
-                    field
-                    message
-                }
-            }
-        }
-    """
     checkout_id = graphene.Node.to_global_id('Checkout', cart.pk)
     variables = {
         'checkoutId': checkout_id, }
-    response = user_api_client.post_graphql(query, variables)
+    response = user_api_client.post_graphql(
+        MUTATION_CHECKOUT_CUSTOMER_DETACH, variables)
     content = get_graphql_content(response)
 
     data = content['data']['checkoutCustomerDetach']
@@ -249,6 +344,20 @@ def test_checkout_customer_detach(
     assert cart.user is None
 
 
+def test_checkout_customer_detach_without_customer(
+        user_api_client, cart_with_item, customer_user):
+    cart = cart_with_item
+
+    checkout_id = graphene.Node.to_global_id('Checkout', cart.pk)
+    variables = {
+        'checkoutId': checkout_id, }
+    response = user_api_client.post_graphql(
+        MUTATION_CHECKOUT_CUSTOMER_DETACH, variables)
+    content = get_graphql_content(response)
+    data = content['data']['checkoutCustomerDetach']
+    assert data['errors'][0]['message'] == 'There\'s no customer assigned to this Checkout.'
+
+
 def test_checkout_shipping_address_update(
         user_api_client, cart_with_item, graphql_address_data):
     cart = cart_with_item


Code review comments:
diff_hunk: @@ -311,21 +309,20 @@ def mutate(cls, root, info, checkout_id, shipping_address):
         checkout = cls.get_node_or_error(
             info, checkout_id, errors, 'checkout_id', only_type=Checkout)
 
-        shipping_address, errors = cls.validate_address(
-            shipping_address, errors, instance=checkout.shipping_address)
-        if errors:
-            return CheckoutShippingAddressUpdate(errors=errors)
-
-        clean_shipping_method(
-            checkout, checkout.shipping_method, errors,
-            info.context.discounts, get_taxes_for_address(shipping_address))
-        if errors:
-            CheckoutShippingAddressUpdate(errors=errors)
-
-        if checkout and shipping_address:
-            with transaction.atomic():
-                shipping_address.save()
-                change_shipping_address_in_cart(checkout, shipping_address)
+        if checkout:
path: saleor/graphql/checkout/mutations.py
text: @user1:
Shouldn't it be `checkout is not None` in this check?