Repository: saleor/saleor
Pull request: Generate voucher code if it wasn't provided in mutation
I want to merge this change because it implements generating voucher code if it wasn't provided in `voucherCreate` mutation. `code` argument is still required in the mutation; voucher code is generated only if empty string was passed as `code` value.

It closes #3704. 

<!-- 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:
Generate voucher code if it wasn't provided
When creating a voucher through API, a user has to provide the voucher code (it is required by the database model). In Dashboard 1.0 we used to generate the code for users and here we should bring back the same behavior.
Once this feature is implemented we'll add a help text in Dashboard 2.0 saying that this field would be generated is users leave it empty (that's a separate issue that we'll open later).

**Required changes**
Within the `voucherCreate` mutation generate a new code if an empty string was passed in the `VoucherInput.code` field. We should use the same function for generating codes as in Dashboard 1.0 (if this logic sits within a Django form, it should be abstracted to a utility function in the `discounts` module).

Hints from issue discussion:

Commit message: Add generating voucher code if an empty string was provided in mutation

Patch to review:
diff --git a/saleor/dashboard/discount/forms.py b/saleor/dashboard/discount/forms.py
index 240368746f5..bb0af428641 100644
--- a/saleor/dashboard/discount/forms.py
+++ b/saleor/dashboard/discount/forms.py
@@ -1,5 +1,3 @@
-import uuid
-
 from django import forms
 from django.conf import settings
 from django.urls import reverse_lazy
@@ -11,6 +9,7 @@
 from ...core.utils.taxes import ZERO_MONEY
 from ...discount import DiscountValueType
 from ...discount.models import Sale, Voucher
+from ...discount.utils import generate_voucher_code
 from ...product.models import Category, Product
 from ..forms import AjaxSelect2MultipleChoiceField
 
@@ -116,16 +115,10 @@ def __init__(self, *args, **kwargs):
         initial = kwargs.get('initial', {})
         instance = kwargs.get('instance')
         if instance and instance.id is None and not initial.get('code'):
-            initial['code'] = self._generate_code
+            initial['code'] = generate_voucher_code()
         kwargs['initial'] = initial
         super().__init__(*args, **kwargs)
 
-    def _generate_code(self):
-        while True:
-            code = str(uuid.uuid4()).replace('-', '').upper()[:12]
-            if not Voucher.objects.filter(code=code).exists():
-                return code
-
 
 class ShippingVoucherForm(forms.ModelForm):
     min_amount_spent = MinAmountSpent
diff --git a/saleor/discount/utils.py b/saleor/discount/utils.py
index 166a9b233f5..d1763063a66 100644
--- a/saleor/discount/utils.py
+++ b/saleor/discount/utils.py
@@ -1,8 +1,10 @@
+import uuid
+
 from django.db.models import F
 from django.utils.translation import pgettext
 
 from ..core.utils.taxes import ZERO_MONEY, ZERO_TAXED_MONEY
-from .models import NotApplicable
+from .models import NotApplicable, Voucher
 
 
 def increase_voucher_usage(voucher):
@@ -86,3 +88,11 @@ def get_products_voucher_discount(voucher, prices):
         voucher.get_discount_amount_for(price) for price in prices)
     total_amount = sum(discounts, ZERO_MONEY)
     return total_amount
+
+
+def generate_voucher_code():
+    """Generate new unique voucher code."""
+    while True:
+        code = str(uuid.uuid4()).replace('-', '').upper()[:12]
+        if not Voucher.objects.filter(code=code).exists():
+            return code
diff --git a/saleor/graphql/discount/mutations.py b/saleor/graphql/discount/mutations.py
index 44ed7e0baff..968e07342f4 100644
--- a/saleor/graphql/discount/mutations.py
+++ b/saleor/graphql/discount/mutations.py
@@ -2,6 +2,7 @@
 from graphql_jwt.decorators import permission_required
 
 from ...discount import VoucherType, models
+from ...discount.utils import generate_voucher_code
 from ..core.mutations import BaseMutation, ModelDeleteMutation, ModelMutation
 from ..core.scalars import Decimal
 from ..product.types import Category, Collection, Product
@@ -121,6 +122,9 @@ def user_is_allowed(cls, user, input):
 
     @classmethod
     def clean_input(cls, info, instance, input, errors):
+        code = input.get('code', None)
+        if code == '':
+            input['code'] = generate_voucher_code()
         cleaned_input = super().clean_input(info, instance, input, errors)
         voucher_errors = validate_voucher(cleaned_input)
         for err in voucher_errors:
diff --git a/tests/api/test_discount.py b/tests/api/test_discount.py
index f292185d7b5..4fb36cdcc67 100644
--- a/tests/api/test_discount.py
+++ b/tests/api/test_discount.py
@@ -96,37 +96,39 @@ def test_sale_query(staff_api_client, sale, permission_manage_discounts):
     assert data['startDate'] == sale.start_date.isoformat()
 
 
-def test_create_voucher(staff_api_client, permission_manage_discounts):
-    query = """
-    mutation  voucherCreate(
-        $type: VoucherTypeEnum, $name: String, $code: String,
-        $discountValueType: DiscountValueTypeEnum,
-        $discountValue: Decimal, $minAmountSpent: Decimal,
-        $startDate: Date, $endDate: Date) {
-            voucherCreate(input: {
-                    name: $name, type: $type, code: $code,
-                    discountValueType: $discountValueType,
-                    discountValue: $discountValue,
-                    minAmountSpent: $minAmountSpent,
-                    startDate: $startDate, endDate: $endDate}) {
-                errors {
-                    field
-                    message
-                }
-                voucher {
-                    type
-                    minAmountSpent {
-                        amount
-                    }
-                    name
-                    code
-                    discountValueType
-                    startDate
-                    endDate
+CREATE_VOUCHER_MUTATION = """
+mutation  voucherCreate(
+    $type: VoucherTypeEnum, $name: String, $code: String,
+    $discountValueType: DiscountValueTypeEnum,
+    $discountValue: Decimal, $minAmountSpent: Decimal,
+    $startDate: Date, $endDate: Date) {
+        voucherCreate(input: {
+                name: $name, type: $type, code: $code,
+                discountValueType: $discountValueType,
+                discountValue: $discountValue,
+                minAmountSpent: $minAmountSpent,
+                startDate: $startDate, endDate: $endDate}) {
+            errors {
+                field
+                message
+            }
+            voucher {
+                type
+                minAmountSpent {
+                    amount
                 }
+                name
+                code
+                discountValueType
+                startDate
+                endDate
             }
         }
-    """
+    }
+"""
+
+
+def test_create_voucher(staff_api_client, permission_manage_discounts):
     start_date = date(day=1, month=1, year=2018)
     end_date = date(day=1, month=1, year=2019)
     variables = {
@@ -140,7 +142,9 @@ def test_create_voucher(staff_api_client, permission_manage_discounts):
         'endDate': end_date.isoformat()}
 
     response = staff_api_client.post_graphql(
-        query, variables, permissions=[permission_manage_discounts])
+        CREATE_VOUCHER_MUTATION,
+        variables,
+        permissions=[permission_manage_discounts])
     content = get_graphql_content(response)
     data = content['data']['voucherCreate']['voucher']
     assert data['type'] == VoucherType.VALUE.upper()
@@ -152,6 +156,30 @@ def test_create_voucher(staff_api_client, permission_manage_discounts):
     assert data['endDate'] == end_date.isoformat()
 
 
+def test_create_voucher_with_empty_code(
+        staff_api_client, permission_manage_discounts):
+    start_date = date(day=1, month=1, year=2018)
+    end_date = date(day=1, month=1, year=2019)
+    variables = {
+        'name': 'test voucher',
+        'type': VoucherTypeEnum.VALUE.name,
+        'code': '',
+        'discountValueType': DiscountValueTypeEnum.FIXED.name,
+        'discountValue': 10.12,
+        'minAmountSpent': 1.12,
+        'startDate': start_date.isoformat(),
+        'endDate': end_date.isoformat()}
+
+    response = staff_api_client.post_graphql(
+        CREATE_VOUCHER_MUTATION,
+        variables,
+        permissions=[permission_manage_discounts])
+    content = get_graphql_content(response)
+    data = content['data']['voucherCreate']['voucher']
+    assert data['name'] == variables['name']
+    assert data['code'] != ''
+
+
 def test_update_voucher(
         staff_api_client, voucher, permission_manage_discounts):
     query = """


Code review comments:
diff_hunk: @@ -86,3 +88,11 @@ def get_products_voucher_discount(voucher, prices):
         voucher.get_discount_amount_for(price) for price in prices)
     total_amount = sum(discounts, ZERO_MONEY)
     return total_amount
+
+
+def generate_voucher_code():
+    """Generate new unique voucher code."""
+    while True:
path: saleor/discount/utils.py
text: @user1:
😱 I just realized that we've had this ugly `while True` for so long. Can we get rid of this line? Chances of collision in UUID are so low that we could ignore it (we do for generating order tokens for example).

@author:
In addition, if an impossible was happen, we wouldn't store the duplicate in database (due to `unique=True`). 
I've removed the loop.