From f5d9458d1a9c9fe6eb349f1f7175c82a395ed7d6 Mon Sep 17 00:00:00 2001 From: Christopher Neugebauer Date: Wed, 6 Apr 2016 10:22:44 +1000 Subject: [PATCH 01/10] Adds a validation based on available_products to validate_cart, and a test based on simple enabling conditions --- registrasion/controllers/cart.py | 37 ++++++++++++++++--- registrasion/tests/test_enabling_condition.py | 16 ++++++++ 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/registrasion/controllers/cart.py b/registrasion/controllers/cart.py index a643863f..76c3adef 100644 --- a/registrasion/controllers/cart.py +++ b/registrasion/controllers/cart.py @@ -203,15 +203,35 @@ class CartController(object): ''' Determines whether the status of the current cart is valid; this is normally called before generating or paying an invoice ''' + cart = self.cart + user = self.cart.user + errors = [] + # TODO: validate vouchers - items = rego.ProductItem.objects.filter(cart=self.cart) + items = rego.ProductItem.objects.filter(cart=cart) + + products = set(i.product for i in items) + available = set(ProductController.available_products( + user, + products=products, + )) + + if products != available: + # Then we have products that aren't available any more. + for product in products: + if product not in available: + message = "%s is no longer available to you." % product + errors.append(ValidationError(message)) product_quantities = list((i.product, i.quantity) for i in items) - self._test_limits(product_quantities) + try: + self._test_limits(product_quantities) + except ValidationError as ve: + errors.append(ve) # Validate the discounts - discount_items = rego.DiscountItem.objects.filter(cart=self.cart) + discount_items = rego.DiscountItem.objects.filter(cart=cart) seen_discounts = set() for discount_item in discount_items: @@ -223,12 +243,17 @@ class CartController(object): pk=discount.pk) cond = ConditionController.for_condition(real_discount) - if not cond.is_met(self.cart.user): - raise ValidationError("Discounts are no longer available") + if not cond.is_met(user): + errors.append( + ValidationError("Discounts are no longer available") + ) + if errors: + raise ValidationError(errors) + + @transaction.atomic def recalculate_discounts(self): ''' Calculates all of the discounts available for this product. - NB should be transactional, and it's terribly inefficient. ''' # Delete the existing entries. diff --git a/registrasion/tests/test_enabling_condition.py b/registrasion/tests/test_enabling_condition.py index e6c090dd..2e992d7a 100644 --- a/registrasion/tests/test_enabling_condition.py +++ b/registrasion/tests/test_enabling_condition.py @@ -293,3 +293,19 @@ class EnablingConditionTestCases(RegistrationCartTestCase): self.assertTrue(self.CAT_1 in cats) self.assertTrue(self.CAT_2 in cats) + + def test_validate_cart_when_enabling_conditions_become_unmet(self): + self.add_product_enabling_condition(mandatory=False) + + cart = TestingCartController.for_user(self.USER_1) + cart.add_to_cart(self.PROD_2, 1) + cart.add_to_cart(self.PROD_1, 1) + + # Should pass + cart.validate_cart() + + cart.set_quantity(self.PROD_2, 0) + + # Should fail + with self.assertRaises(ValidationError): + cart.validate_cart() From 7d97d2d2dea12ac974ba8fdc82e55d2d0e957b50 Mon Sep 17 00:00:00 2001 From: Christopher Neugebauer Date: Wed, 6 Apr 2016 11:36:07 +1000 Subject: [PATCH 02/10] Adds fix_simple_errors to cart - it zeroes out unavailable products. Adds test that it does that. --- registrasion/controllers/cart.py | 23 ++++++++++++++- registrasion/tests/test_enabling_condition.py | 29 +++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/registrasion/controllers/cart.py b/registrasion/controllers/cart.py index 76c3adef..6fef142f 100644 --- a/registrasion/controllers/cart.py +++ b/registrasion/controllers/cart.py @@ -210,7 +210,6 @@ class CartController(object): # TODO: validate vouchers items = rego.ProductItem.objects.filter(cart=cart) - products = set(i.product for i in items) available = set(ProductController.available_products( user, @@ -251,6 +250,28 @@ class CartController(object): if errors: raise ValidationError(errors) + def fix_simple_errors(self): + ''' This attempts to fix the easy errors raised by ValidationError. + This includes removing items from the cart that are no longer + available, recalculating all of the discounts, and removing voucher + codes that are no longer available. ''' + + # TODO: fix vouchers first (this affects available discounts) + + # Fix products and discounts + items = rego.ProductItem.objects.filter(cart=self.cart) + products = set(i.product for i in items) + available = set(ProductController.available_products( + self.cart.user, + products=products, + )) + + not_available = products - available + zeros = [(product, 0) for product in not_available] + + self.set_quantities(zeros) + + @transaction.atomic def recalculate_discounts(self): ''' Calculates all of the discounts available for this product. diff --git a/registrasion/tests/test_enabling_condition.py b/registrasion/tests/test_enabling_condition.py index 2e992d7a..6dd3d102 100644 --- a/registrasion/tests/test_enabling_condition.py +++ b/registrasion/tests/test_enabling_condition.py @@ -309,3 +309,32 @@ class EnablingConditionTestCases(RegistrationCartTestCase): # Should fail with self.assertRaises(ValidationError): cart.validate_cart() + + def test_fix_simple_errors_resolves_unavailable_products(self): + self.test_validate_cart_when_enabling_conditions_become_unmet() + cart = TestingCartController.for_user(self.USER_1) + + # Should just remove all of the unavailable products + cart.fix_simple_errors() + # Should now succeed + cart.validate_cart() + + # Should keep PROD_2 in the cart + items = rego.ProductItem.objects.filter(cart=cart.cart) + self.assertFalse([i for i in items if i.product == self.PROD_1]) + + def test_fix_simple_errors_does_not_remove_limited_items(self): + cart = TestingCartController.for_user(self.USER_1) + + cart.add_to_cart(self.PROD_2, 1) + cart.add_to_cart(self.PROD_1, 10) + + # Should just remove all of the unavailable products + cart.fix_simple_errors() + # Should now succeed + cart.validate_cart() + + # Should keep PROD_2 in the cart + # and also PROD_1, which is now exhausted for user. + items = rego.ProductItem.objects.filter(cart=cart.cart) + self.assertTrue([i for i in items if i.product == self.PROD_1]) From 6f28c20b706f27feb665bdafc0dd99bce2b639a2 Mon Sep 17 00:00:00 2001 From: Christopher Neugebauer Date: Wed, 6 Apr 2016 12:09:16 +1000 Subject: [PATCH 03/10] Factors _test_voucher() method into CartController --- registrasion/controllers/cart.py | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/registrasion/controllers/cart.py b/registrasion/controllers/cart.py index 6fef142f..339d7ba1 100644 --- a/registrasion/controllers/cart.py +++ b/registrasion/controllers/cart.py @@ -175,16 +175,26 @@ class CartController(object): def apply_voucher(self, voucher_code): ''' Applies the voucher with the given code to this cart. ''' - # Is voucher exhausted? - active_carts = rego.Cart.reserved_carts() - # Try and find the voucher voucher = rego.Voucher.objects.get(code=voucher_code.upper()) + self._test_voucher(voucher) + + # If successful... + self.cart.vouchers.add(voucher) + self.end_batch() + + def _test_voucher(self, voucher): + ''' Tests whether this voucher is allowed to be applied to this cart. + Raises ValidationError if not. ''' + + # Is voucher exhausted? + active_carts = rego.Cart.reserved_carts() + # It's invalid for a user to enter a voucher that's exhausted carts_with_voucher = active_carts.filter(vouchers=voucher) if len(carts_with_voucher) >= voucher.limit: - raise ValidationError("This voucher is no longer available") + raise ValidationError("Voucher %s is no longer available" % voucher.code) # It's not valid for users to re-enter a voucher they already have user_carts_with_voucher = rego.Cart.objects.filter( @@ -195,9 +205,6 @@ class CartController(object): if len(user_carts_with_voucher) > 0: raise ValidationError("You have already entered this voucher.") - # If successful... - self.cart.vouchers.add(voucher) - self.end_batch() def validate_cart(self): ''' Determines whether the status of the current cart is valid; From 8d07518a9bc6412fece1ea0906cbf85446cef921 Mon Sep 17 00:00:00 2001 From: Christopher Neugebauer Date: Wed, 6 Apr 2016 12:22:32 +1000 Subject: [PATCH 04/10] Fixes an incorrect voucher test --- registrasion/tests/test_voucher.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/registrasion/tests/test_voucher.py b/registrasion/tests/test_voucher.py index de47c864..b9373583 100644 --- a/registrasion/tests/test_voucher.py +++ b/registrasion/tests/test_voucher.py @@ -114,6 +114,8 @@ class VoucherTestCases(RegistrationCartTestCase): inv = InvoiceController.for_cart(current_cart.cart) inv.pay("Hello!", inv.invoice.value) + current_cart = TestingCartController.for_user(self.USER_1) + with self.assertRaises(ValidationError): current_cart.apply_voucher(voucher.code) From 0d57da8d6f668550d053d156f4841456fd06c71d Mon Sep 17 00:00:00 2001 From: Christopher Neugebauer Date: Wed, 6 Apr 2016 12:48:05 +1000 Subject: [PATCH 05/10] Makes apply_voucher() idempotent, adds _test_voucher to validate_cart, and updates tests. --- registrasion/controllers/cart.py | 29 +++++++++++++++++++++++++---- registrasion/tests/test_voucher.py | 12 +++++++----- 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/registrasion/controllers/cart.py b/registrasion/controllers/cart.py index 339d7ba1..0fdc8f76 100644 --- a/registrasion/controllers/cart.py +++ b/registrasion/controllers/cart.py @@ -178,6 +178,10 @@ class CartController(object): # Try and find the voucher voucher = rego.Voucher.objects.get(code=voucher_code.upper()) + # Re-applying vouchers should be idempotent + if voucher in self.cart.vouchers.all(): + return + self._test_voucher(voucher) # If successful... @@ -193,18 +197,32 @@ class CartController(object): # It's invalid for a user to enter a voucher that's exhausted carts_with_voucher = active_carts.filter(vouchers=voucher) + carts_with_voucher = carts_with_voucher.exclude(pk=self.cart.id) if len(carts_with_voucher) >= voucher.limit: raise ValidationError("Voucher %s is no longer available" % voucher.code) # It's not valid for users to re-enter a voucher they already have - user_carts_with_voucher = rego.Cart.objects.filter( + user_carts_with_voucher = carts_with_voucher.filter( user=self.cart.user, - released=False, - vouchers=voucher, ) + if len(user_carts_with_voucher) > 0: raise ValidationError("You have already entered this voucher.") + def _test_vouchers(self, vouchers): + ''' Tests each of the vouchers against self._test_voucher() and raises + the collective ValidationError. + Future work will refactor _test_voucher in terms of this, and save some + queries. ''' + errors = [] + for voucher in vouchers: + try: + self._test_voucher(voucher) + except ValidationError as ve: + errors.append(ve) + + if errors: + raise(ValidationError(ve)) def validate_cart(self): ''' Determines whether the status of the current cart is valid; @@ -214,7 +232,10 @@ class CartController(object): user = self.cart.user errors = [] - # TODO: validate vouchers + try: + self._test_vouchers(self.cart.vouchers.all()) + except ValidationError as ve: + errors.append(ve) items = rego.ProductItem.objects.filter(cart=cart) products = set(i.product for i in items) diff --git a/registrasion/tests/test_voucher.py b/registrasion/tests/test_voucher.py index b9373583..732b19f1 100644 --- a/registrasion/tests/test_voucher.py +++ b/registrasion/tests/test_voucher.py @@ -37,11 +37,11 @@ class VoucherTestCases(RegistrationCartTestCase): cart_2.cart.active = False cart_2.cart.save() - # After the reservation duration, user 1 should not be able to apply - # voucher, as user 2 has paid for their cart. + # After the reservation duration, even though the voucher has applied, + # it exceeds the number of vouchers available. self.add_timedelta(rego.Voucher.RESERVATION_DURATION * 2) with self.assertRaises(ValidationError): - cart_1.apply_voucher(voucher.code) + cart_1.validate_cart() def test_voucher_enables_item(self): voucher = self.new_voucher() @@ -103,8 +103,10 @@ class VoucherTestCases(RegistrationCartTestCase): voucher = self.new_voucher(limit=2) current_cart = TestingCartController.for_user(self.USER_1) current_cart.apply_voucher(voucher.code) - with self.assertRaises(ValidationError): - current_cart.apply_voucher(voucher.code) + current_cart.apply_voucher(voucher.code) + + # You can apply the code twice, but it will only add to the cart once. + self.assertEqual(1, current_cart.cart.vouchers.count()) def test_voucher_can_only_be_applied_once_across_multiple_carts(self): voucher = self.new_voucher(limit=2) From c8c16072ba8790b5e4bbe13477af736d4494da74 Mon Sep 17 00:00:00 2001 From: Christopher Neugebauer Date: Wed, 6 Apr 2016 13:01:25 +1000 Subject: [PATCH 06/10] fix_simple_errors() now removes exhausted vouchers from the voucher set. --- registrasion/controllers/cart.py | 18 ++++++++++++++---- registrasion/tests/test_voucher.py | 20 ++++++++++++++++++++ 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/registrasion/controllers/cart.py b/registrasion/controllers/cart.py index 0fdc8f76..d4aa082a 100644 --- a/registrasion/controllers/cart.py +++ b/registrasion/controllers/cart.py @@ -198,7 +198,7 @@ class CartController(object): # It's invalid for a user to enter a voucher that's exhausted carts_with_voucher = active_carts.filter(vouchers=voucher) carts_with_voucher = carts_with_voucher.exclude(pk=self.cart.id) - if len(carts_with_voucher) >= voucher.limit: + if carts_with_voucher.count() >= voucher.limit: raise ValidationError("Voucher %s is no longer available" % voucher.code) # It's not valid for users to re-enter a voucher they already have @@ -206,7 +206,7 @@ class CartController(object): user=self.cart.user, ) - if len(user_carts_with_voucher) > 0: + if user_carts_with_voucher.count() > 0: raise ValidationError("You have already entered this voucher.") def _test_vouchers(self, vouchers): @@ -278,13 +278,24 @@ class CartController(object): if errors: raise ValidationError(errors) + @transaction.atomic def fix_simple_errors(self): ''' This attempts to fix the easy errors raised by ValidationError. This includes removing items from the cart that are no longer available, recalculating all of the discounts, and removing voucher codes that are no longer available. ''' - # TODO: fix vouchers first (this affects available discounts) + # Fix vouchers first (this affects available discounts) + active_carts = rego.Cart.reserved_carts() + to_remove = [] + for voucher in self.cart.vouchers.all(): + try: + self._test_voucher(voucher) + except ValidationError as ve: + to_remove.append(voucher) + + for voucher in to_remove: + self.cart.vouchers.remove(voucher) # Fix products and discounts items = rego.ProductItem.objects.filter(cart=self.cart) @@ -299,7 +310,6 @@ class CartController(object): self.set_quantities(zeros) - @transaction.atomic def recalculate_discounts(self): ''' Calculates all of the discounts available for this product. diff --git a/registrasion/tests/test_voucher.py b/registrasion/tests/test_voucher.py index 732b19f1..e3ac0d0d 100644 --- a/registrasion/tests/test_voucher.py +++ b/registrasion/tests/test_voucher.py @@ -43,6 +43,18 @@ class VoucherTestCases(RegistrationCartTestCase): with self.assertRaises(ValidationError): cart_1.validate_cart() + def test_fix_simple_errors_resolves_unavailable_voucher(self): + self.test_apply_voucher() + + # User has an exhausted voucher leftover from test_apply_voucher + cart_1 = TestingCartController.for_user(self.USER_1) + with self.assertRaises(ValidationError): + cart_1.validate_cart() + + cart_1.fix_simple_errors() + # This should work now. + cart_1.validate_cart() + def test_voucher_enables_item(self): voucher = self.new_voucher() @@ -137,3 +149,11 @@ class VoucherTestCases(RegistrationCartTestCase): inv.refund("Hello!", inv.invoice.value) current_cart.apply_voucher(voucher.code) + + def test_fix_simple_errors_does_not_remove_limited_voucher(self): + voucher = self.new_voucher(code="VOUCHER") + current_cart = TestingCartController.for_user(self.USER_1) + current_cart.apply_voucher(voucher.code) + + current_cart.fix_simple_errors() + self.assertEqual(1, current_cart.cart.vouchers.count()) From 39b130811c16c9889ce27ce22a291297b0aa0bb0 Mon Sep 17 00:00:00 2001 From: Christopher Neugebauer Date: Wed, 6 Apr 2016 14:28:37 +1000 Subject: [PATCH 07/10] Removes superfluous test --- registrasion/controllers/cart.py | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/registrasion/controllers/cart.py b/registrasion/controllers/cart.py index d4aa082a..e8310f4b 100644 --- a/registrasion/controllers/cart.py +++ b/registrasion/controllers/cart.py @@ -238,24 +238,13 @@ class CartController(object): errors.append(ve) items = rego.ProductItem.objects.filter(cart=cart) - products = set(i.product for i in items) - available = set(ProductController.available_products( - user, - products=products, - )) - - if products != available: - # Then we have products that aren't available any more. - for product in products: - if product not in available: - message = "%s is no longer available to you." % product - errors.append(ValidationError(message)) product_quantities = list((i.product, i.quantity) for i in items) try: self._test_limits(product_quantities) except ValidationError as ve: - errors.append(ve) + for error in ve.error_list: + errors.append(error.message[1]) # Validate the discounts discount_items = rego.DiscountItem.objects.filter(cart=cart) From 0340b6da20640b51386a45708b5e528ad00a4b74 Mon Sep 17 00:00:00 2001 From: Christopher Neugebauer Date: Wed, 6 Apr 2016 14:34:16 +1000 Subject: [PATCH 08/10] =?UTF-8?q?Adds=20=E2=80=9Cfix=5Ferrors=E2=80=9D=20q?= =?UTF-8?q?uery=20to=20=E2=80=9Ccheckout=E2=80=9D,=20which=20allows=20user?= =?UTF-8?q?s=20to=20have=20issues=20fix?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- registrasion/views.py | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/registrasion/views.py b/registrasion/views.py index 77c00bb3..3f9dbf4f 100644 --- a/registrasion/views.py +++ b/registrasion/views.py @@ -377,10 +377,30 @@ def checkout(request): invoice. ''' current_cart = CartController.for_user(request.user) - current_invoice = InvoiceController.for_cart(current_cart.cart) + + if "fix_errors" in request.GET and request.GET["fix_errors"] == "true": + current_cart.fix_simple_errors() + + try: + current_invoice = InvoiceController.for_cart(current_cart.cart) + except ValidationError as ve: + return checkout_errors(request, ve) return redirect("invoice", current_invoice.invoice.id) +def checkout_errors(request, errors): + + error_list = [] + for error in errors.error_list: + if isinstance(error, tuple): + error = error[1] + error_list.append(error) + + data = { + "error_list": error_list, + } + + return render(request, "registrasion/checkout_errors.html", data) @login_required def invoice(request, invoice_id): From 40bc5985f4b8149eea9ad8ad094fdde81498e017 Mon Sep 17 00:00:00 2001 From: Christopher Neugebauer Date: Wed, 6 Apr 2016 15:40:16 +1000 Subject: [PATCH 09/10] Propagates the error messages up from enabling condition testing --- registrasion/controllers/cart.py | 9 ++----- registrasion/controllers/conditions.py | 37 +++++++++++++++++++++++++- registrasion/views.py | 2 ++ 3 files changed, 40 insertions(+), 8 deletions(-) diff --git a/registrasion/controllers/cart.py b/registrasion/controllers/cart.py index e8310f4b..def22c17 100644 --- a/registrasion/controllers/cart.py +++ b/registrasion/controllers/cart.py @@ -120,14 +120,12 @@ class CartController(object): # Test each product limit here for product, quantity in product_quantities: if quantity < 0: - # TODO: batch errors errors.append((product, "Value must be zero or greater.")) prod = ProductController(product) limit = prod.user_quantity_remaining(self.cart.user) if quantity > limit: - # TODO: batch errors errors.append(( product, "You may only have %d of product: %s" % ( @@ -149,7 +147,6 @@ class CartController(object): to_add = sum(i[1] for i in by_cat[category]) if to_add > limit: - # TODO: batch errors errors.append(( category, "You may only have %d items in category: %s" % ( @@ -164,10 +161,8 @@ class CartController(object): ) if errs: - # TODO: batch errors - errors.append( - ("enabling_conditions", "An enabling condition failed") - ) + for error in errs: + errors.append(error) if errors: raise CartValidationError(errors) diff --git a/registrasion/controllers/conditions.py b/registrasion/controllers/conditions.py index 9a0b26df..55b1a7d5 100644 --- a/registrasion/controllers/conditions.py +++ b/registrasion/controllers/conditions.py @@ -44,6 +44,26 @@ class ConditionController(object): except KeyError: return ConditionController() + + SINGLE = True + PLURAL = False + NONE = True + SOME = False + MESSAGE = { + NONE: { + SINGLE: + "%(items)s is no longer available to you", + PLURAL: + "%(items)s are no longer available to you", + }, + SOME: { + SINGLE: + "Only %(remainder)d of the following item remains: %(items)s", + PLURAL: + "Only %(remainder)d of the following items remain: %(items)s" + }, + } + @classmethod def test_enabling_conditions( cls, user, products=None, product_quantities=None): @@ -83,6 +103,8 @@ class ConditionController(object): # if there are no mandatory conditions non_mandatory = defaultdict(lambda: False) + messages = {} + for condition in all_conditions: cond = cls.for_condition(condition) remainder = cond.user_quantity_remaining(user) @@ -104,12 +126,21 @@ class ConditionController(object): consumed = 1 met = consumed <= remainder + if not met: + items = ", ".join(str(product) for product in all_products) + base = cls.MESSAGE[remainder == 0][len(all_products) == 1] + message = base % {"items": items, "remainder": remainder} + for product in all_products: if condition.mandatory: mandatory[product] &= met else: non_mandatory[product] |= met + if not met and product not in messages: + messages[product] = message + + valid = defaultdict(lambda: True) for product in itertools.chain(mandatory, non_mandatory): if product in mandatory: @@ -119,7 +150,11 @@ class ConditionController(object): # Otherwise, we need just one non-mandatory condition met valid[product] = non_mandatory[product] - error_fields = [product for product in valid if not valid[product]] + error_fields = [ + (product, messages[product]) + for product in valid if not valid[product] + ] + return error_fields def user_quantity_remaining(self, user): diff --git a/registrasion/views.py b/registrasion/views.py index 3f9dbf4f..1cb4ec57 100644 --- a/registrasion/views.py +++ b/registrasion/views.py @@ -337,6 +337,8 @@ def set_quantities_from_products_form(products_form, current_cart): product, message = ve_field.message if product in field_names: field = field_names[product] + elif isinstance(product, rego.Product): + continue else: field = None products_form.add_error(field, message) From cc318dfa9ba8a38d82a2edd1490cad586560c9d3 Mon Sep 17 00:00:00 2001 From: Christopher Neugebauer Date: Wed, 6 Apr 2016 16:09:57 +1000 Subject: [PATCH 10/10] Fixes tests --- registrasion/controllers/product.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/registrasion/controllers/product.py b/registrasion/controllers/product.py index d25a79bb..b49d5569 100644 --- a/registrasion/controllers/product.py +++ b/registrasion/controllers/product.py @@ -38,9 +38,10 @@ class ProductController(object): if cls(product).user_quantity_remaining(user) > 0 ) - failed_conditions = set(ConditionController.test_enabling_conditions( + failed_and_messages = ConditionController.test_enabling_conditions( user, products=passed_limits - )) + ) + failed_conditions = set(i[0] for i in failed_and_messages) out = list(passed_limits - failed_conditions) out.sort(key=lambda product: product.order)