Merge pull request #1017 from edx/iivic/SOL-2137

[SOL-2137] Add Backend Validation for Coupon Creation - Benefit Value

ecommerce/extensions/api/v2/tests/views/test_coupons.py

@@ -262,21 +262,29 @@ class CouponViewSetFunctionalTest(CouponMixin, CourseCatalogTestMixin, CourseCat
         self.assertEqual(details_response['coupon_type'], 'Enrollment code')
         self.assertEqual(details_response['code_status'], 'ACTIVE')
 
-    @ddt.data(
-        ({'category': {'a': 'a', 'b': 'b'}}, status.HTTP_400_BAD_REQUEST),
-        ({'category': {'id': 10000, 'name': 'Category Not Found'}}, status.HTTP_404_NOT_FOUND)
-    )
-    @ddt.unpack
-    def test_create_coupon_product_invalid_category_data(self, category_data, expected_status_code):
+    def test_create_coupon_product_invalid_category_data(self):
         """Test creating coupon when provided category data is invalid."""
-        self.data.update(category_data)
+        self.data.update({'category': {'id': 10000, 'name': 'Category Not Found'}})
         response_data = self.client.post(COUPONS_LINK, json.dumps(self.data), 'application/json')
-        self.assertEqual(response_data.status_code, expected_status_code)
+        self.assertEqual(response_data.status_code, status.HTTP_404_NOT_FOUND)
 
-    @ddt.data('', 'Incorrect benefit type')
-    def test_create_coupon_product_invalid_benefit_type(self, benefit_type):
-        """Test creating coupon when provided benefit type is invalid."""
-        self.data.update({'benefit_type': benefit_type})
+    @ddt.data(
+        ('benefit_type', ['', 'Incorrect benefit type']),
+        ('benefit_value', ['', 'Incorrect benefit value', -1, 101]),
+        ('category', [{'a': 'a', 'b': 'b'}]),
+    )
+    @ddt.unpack
+    def test_create_coupon_product_invalid_data(self, key, values):
+        """Test creating coupon when provided data is invalid."""
+        for value in values:
+            self.data.update({key: value})
+            response_data = self.client.post(COUPONS_LINK, json.dumps(self.data), 'application/json')
+            self.assertEqual(response_data.status_code, status.HTTP_400_BAD_REQUEST)
+
+    @ddt.data('benefit_type', 'benefit_value')
+    def test_create_coupon_product_no_data_provided(self, key):
+        """Test creating coupon when data is not provided in json."""
+        del self.data[key]
         response_data = self.client.post(COUPONS_LINK, json.dumps(self.data), 'application/json')
         self.assertEqual(response_data.status_code, status.HTTP_400_BAD_REQUEST)
 

ecommerce/extensions/api/v2/views/coupons.py

@@ -21,7 +21,6 @@ from ecommerce.extensions.api.serializers import CategorySerializer, CouponSeria
 from ecommerce.extensions.basket.utils import prepare_basket
 from ecommerce.extensions.catalogue.utils import create_coupon_product, get_or_create_catalog
 from ecommerce.extensions.checkout.mixins import EdxOrderPlacementMixin
-from ecommerce.extensions.offer.models import VALID_BENEFIT_TYPES
 from ecommerce.extensions.payment.processors.invoice import InvoicePayment
 from ecommerce.extensions.voucher.models import CouponVouchers
 from ecommerce.extensions.voucher.utils import update_voucher_offer
@@ -35,7 +34,7 @@ Order = get_model('order', 'Order')
 Product = get_model('catalogue', 'Product')
 ProductCategory = get_model('catalogue', 'ProductCategory')
 ProductClass = get_model('catalogue', 'ProductClass')
-Range = Range = get_model('offer', 'Range')
+Range = get_model('offer', 'Range')
 StockRecord = get_model('partner', 'StockRecord')
 Voucher = get_model('voucher', 'Voucher')
 
@@ -167,9 +166,6 @@ class CouponViewSet(EdxOrderPlacementMixin, viewsets.ModelViewSet):
 
                 return Response(response_data, status=status.HTTP_200_OK)
         except ValidationError as e:
-            logger.exception(
-                'Failed to create Benefit. Benefit type must be one of the following %s.', VALID_BENEFIT_TYPES
-            )
             raise serializers.ValidationError(e.message)
 
     def create_order_for_invoice(self, basket, coupon_id, client, invoice_data=None):

ecommerce/extensions/catalogue/utils.py

 from __future__ import unicode_literals
 
-from decimal import Decimal
 from hashlib import md5
 import logging
 
@@ -78,7 +77,7 @@ def create_coupon_product(
     try:
         create_vouchers(
             benefit_type=benefit_type,
-            benefit_value=Decimal(benefit_value),
+            benefit_value=benefit_value,
             catalog=catalog,
             catalog_query=catalog_query,
             code=code or None,

ecommerce/extensions/offer/models.py

 # noinspection PyUnresolvedReferences
 import hashlib
+import logging
 import re
 
 from django.conf import settings
@@ -10,7 +11,7 @@ from django.utils.translation import ugettext_lazy as _
 from oscar.apps.offer.abstract_models import AbstractBenefit, AbstractConditionalOffer, AbstractRange
 from threadlocals.threadlocals import get_current_request
 
-
+logger = logging.getLogger(__name__)
 VALID_BENEFIT_TYPES = [AbstractBenefit.PERCENTAGE, AbstractBenefit.FIXED]
 
 
@@ -21,12 +22,21 @@ class Benefit(AbstractBenefit):
 
     def clean(self):
         self.clean_type()
+        self.clean_value()
         super(Benefit, self).clean()  # pylint: disable=bad-super-call
 
     def clean_type(self):
         if self.type not in VALID_BENEFIT_TYPES:
+            logger.exception(
+                'Failed to create Benefit. Benefit type must be one of the following %s.', VALID_BENEFIT_TYPES
+            )
             raise ValidationError(_('Unrecognised benefit type {type}'.format(type=self.type)))
 
+    def clean_value(self):
+        if self.value < 0:
+            logger.exception('Failed to create Benefit. Benefit value may not be a negative number.')
+            raise ValidationError(_('Benefit value must be a positive number or 0.'))
+
 
 class ConditionalOffer(AbstractConditionalOffer):
     email_domains = models.CharField(max_length=255, blank=True, null=True)

ecommerce/extensions/voucher/utils.py

 """Voucher Utility Methods. """
+from decimal import Decimal, DecimalException
 import base64
 import datetime
 import hashlib
@@ -7,6 +8,7 @@ import uuid
 
 from django.conf import settings
 from django.core.cache import cache
+from django.core.exceptions import ValidationError
 from django.core.urlresolvers import reverse
 from django.utils.translation import ugettext_lazy as _
 from opaque_keys.edx.keys import CourseKey
@@ -290,12 +292,16 @@ def _get_or_create_offer(
         type=Condition.COUNT,
         value=1,
     )
-    offer_benefit, __ = Benefit.objects.get_or_create(
-        range=product_range,
-        type=benefit_type,
-        value=benefit_value,
-        max_affected_items=1,
-    )
+    try:
+        offer_benefit, __ = Benefit.objects.get_or_create(
+            range=product_range,
+            type=benefit_type,
+            value=Decimal(benefit_value),
+            max_affected_items=1,
+        )
+    except (TypeError, DecimalException):  # If the benefit_value parameter is not sent TypeError will be raised
+        logger.exception('Failed to create Benefit. Benefit value may not be empty or a string.')
+        raise ValidationError(_('Benefit value must be a positive number or 0.'))
 
     offer_name = "Coupon [{}]-{}-{}".format(coupon_id, offer_benefit.type, offer_benefit.value)
     if offer_number: