From 2f976b94e53b4a0b53e3e665878fe1c12f259d10 Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Thu, 16 May 2019 09:48:18 -0400 Subject: [PATCH] Additional kwargs, better efficiency for get_valid_coupon_versions query (#243) --- ecommerce/api.py | 132 +++++++++++++++++----------------- ecommerce/api_test.py | 111 +++++++++++----------------- ecommerce/conftest.py | 2 +- ecommerce/serializers.py | 28 +++++--- ecommerce/serializers_test.py | 7 +- 5 files changed, 131 insertions(+), 149 deletions(-) diff --git a/ecommerce/api.py b/ecommerce/api.py index a09815986..8ba244669 100644 --- a/ecommerce/api.py +++ b/ecommerce/api.py @@ -10,7 +10,7 @@ import uuid from django.conf import settings -from django.db.models import Q, Max, F, Count +from django.db.models import Q, Max, F, Count, Subquery from django.db import transaction from rest_framework.exceptions import ValidationError @@ -153,7 +153,9 @@ def latest_coupon_version(coupon): return coupon.versions.order_by("-created_on").first() -def get_valid_coupon_versions(product, user, auto_only=False, code=None): +def get_valid_coupon_versions( + product, user, auto_only=False, code=None, full_discount=False, company=None +): # pylint:disable=too-many-arguments """ Given a list of coupon ids, determine which of them are valid based on payment version dates and redemptions. @@ -162,85 +164,83 @@ def get_valid_coupon_versions(product, user, auto_only=False, code=None): user (User): User of coupons auto_only (bool): Whether or not to filter by automatic=True code (str): A coupon code to filter by + full_discount (bool): If true, only include 100% off coupons + company (Company): a company to filter by Returns: list of CouponVersion: CouponVersion objects sorted by discount, highest first. """ - valid_coupons = [] now = now_in_utc() - with transaction.atomic(): - # Get the ids of the latest coupon versions - product_coupon_query = CouponEligibility.objects.select_related( - "coupon" - ).filter(product=product, coupon__enabled=True) - if code: - product_coupon_query = product_coupon_query.filter(coupon__coupon_code=code) - - cv_latest = ( - CouponVersion.objects.select_related("coupon") - .filter(coupon__in=product_coupon_query.values_list("coupon", flat=True)) - .order_by("coupon", "-created_on") - .distinct("coupon") - .values_list("pk") - ) - - # filter by expiration and activation dates - query = ( - CouponVersion.objects.select_related("payment_version") - .filter(pk__in=cv_latest) - .filter( - Q(payment_version__expiration_date__gte=now) - | Q(payment_version__expiration_date__isnull=True) - ) - .filter( - Q(payment_version__activation_date__lte=now) - | Q(payment_version__activation_date__isnull=True) - ) + # Get enabled coupons eligible for the product + product_coupon_subquery = CouponEligibility.objects.select_related("coupon").filter( + product=product, coupon__enabled=True + ) + if code: + product_coupon_subquery = product_coupon_subquery.filter( + coupon__coupon_code=code ) - if auto_only: - query = query.filter(payment_version__automatic=True) + # Get the latest versions for product coupons + coupon_version_subquery = CouponVersion.objects.filter( + coupon__in=Subquery(product_coupon_subquery.values_list("coupon", flat=True)) + ) - # filter by redemption counts - for coupon_version in query: - redemptions_global = ( - CouponRedemption.objects.select_related("coupon_version", "order") - .filter(coupon_version=coupon_version) - .filter(order__status=Order.FULFILLED) - ) - redemptions_user = redemptions_global.filter(order__purchaser=user) - if ( - coupon_version.payment_version.max_redemptions - > redemptions_global.count() - and coupon_version.payment_version.max_redemptions_per_user - > redemptions_user.count() - ): - valid_coupons.append(coupon_version) - return sorted( - valid_coupons, key=lambda x: x.payment_version.amount, reverse=True + if full_discount: + coupon_version_subquery = coupon_version_subquery.filter( + payment_version__amount=decimal.Decimal(1) ) + if auto_only: + coupon_version_subquery = coupon_version_subquery.filter( + payment_version__automatic=True + ) -def best_coupon_for_basket(basket, auto_only=False, code=None): - """ - Get the best eligible coupon for the basket. - Assumes that the basket only contains one item/product. + if company is not None: + coupon_version_subquery = coupon_version_subquery.filter( + payment_version__company=company + ) - Args: - basket (Basket): the basket Object - auto_only (bool): Only retrieve `automatic` Coupons - code (str): A coupon code to filter by + coupon_version_subquery = coupon_version_subquery.order_by( + "coupon", "-created_on" + ).distinct("coupon") - Returns: - CouponVersion: the CouponVersion with the highest discount, or None - """ - basket_item = basket.basketitems.first() - if basket_item: - return best_coupon_for_product( - basket_item.product, basket.user, auto_only=auto_only, code=code + # Exclude versions with too many redemptions or active dates outside of today. + query = ( + CouponVersion.objects.select_related("coupon", "payment_version") + .filter(pk__in=Subquery(coupon_version_subquery.values_list("pk", flat=True))) + .filter( + Q(payment_version__expiration_date__gte=now) + | Q(payment_version__expiration_date__isnull=True) ) - return None + .filter( + Q(payment_version__activation_date__lte=now) + | Q(payment_version__activation_date__isnull=True) + ) + .annotate( + user_redemptions=( + Count( + "couponredemption", + filter=( + Q(couponredemption__order__purchaser=user) + & Q(couponredemption__order__status=Order.FULFILLED) + ), + ) + ) + ) + .annotate( + global_redemptions=( + Count( + "couponredemption", + filter=(Q(couponredemption__order__status=Order.FULFILLED)), + ) + ) + ) + .exclude(user_redemptions__gte=F("payment_version__max_redemptions_per_user")) + .exclude(global_redemptions__gte=F("payment_version__max_redemptions")) + ) + + return query.order_by("-payment_version__amount") def best_coupon_for_product(product, user, auto_only=False, code=None): diff --git a/ecommerce/api_test.py b/ecommerce/api_test.py index 82709ed84..abe61308c 100644 --- a/ecommerce/api_test.py +++ b/ecommerce/api_test.py @@ -22,7 +22,6 @@ ISO_8601_FORMAT, make_reference_id, redeem_coupon, - best_coupon_for_basket, get_new_order_by_reference_number, get_product_price, get_product_version_price_with_discount, @@ -37,7 +36,6 @@ from ecommerce.exceptions import EcommerceException, ParseException from ecommerce.factories import ( BasketFactory, - BasketItemFactory, CouponRedemptionFactory, CouponSelectionFactory, CouponVersionFactory, @@ -223,10 +221,12 @@ def test_get_valid_coupon_versions(basket_and_coupons, auto_only): """ Verify that the correct valid CouponPaymentVersions are returned for a list of coupons """ - best_versions = get_valid_coupon_versions( - basket_and_coupons.basket_item.product, - basket_and_coupons.basket_item.basket.user, - auto_only, + best_versions = list( + get_valid_coupon_versions( + basket_and_coupons.basket_item.product, + basket_and_coupons.basket_item.basket.user, + auto_only, + ) ) expected_versions = [basket_and_coupons.coupongroup_worst.coupon_version] if not auto_only: @@ -246,13 +246,39 @@ def test_get_valid_coupon_versions_bad_dates(basket_and_coupons): civ_best.expiration_date = today - timedelta(days=1) civ_best.save() - best_versions = get_valid_coupon_versions( - basket_and_coupons.basket_item.product, - basket_and_coupons.basket_item.basket.user, + best_versions = list( + get_valid_coupon_versions( + basket_and_coupons.basket_item.product, + basket_and_coupons.basket_item.basket.user, + ) ) assert best_versions == [] +def test_get_valid_coupon_versions_full_discount(basket_and_coupons): + """Verify that only 100% coupons are returned if full_discount kwarg is True""" + assert list( + get_valid_coupon_versions( + basket_and_coupons.basket_item.product, + basket_and_coupons.basket_item.basket.user, + full_discount=True, + ) + ) == [basket_and_coupons.coupongroup_best.coupon_version] + assert basket_and_coupons.coupongroup_best.payment_version.amount == Decimal(1.0) + + +def test_get_valid_coupon_versions_by_company(basket_and_coupons): + """Verify that valid coupons are filtered by company""" + company = basket_and_coupons.coupongroup_worst.payment_version.company + assert list( + get_valid_coupon_versions( + basket_and_coupons.basket_item.product, + basket_and_coupons.basket_item.basket.user, + company=company, + ) + ) == [basket_and_coupons.coupongroup_worst.coupon_version] + + @pytest.mark.parametrize("order_status", [Order.FULFILLED, Order.FAILED]) def test_get_valid_coupon_versions_over_redeemed(basket_and_coupons, order_status): """ @@ -276,9 +302,11 @@ def test_get_valid_coupon_versions_over_redeemed(basket_and_coupons, order_statu ), ) - best_versions = get_valid_coupon_versions( - basket_and_coupons.basket_item.product, - basket_and_coupons.basket_item.basket.user, + best_versions = list( + get_valid_coupon_versions( + basket_and_coupons.basket_item.product, + basket_and_coupons.basket_item.basket.user, + ) ) if order_status == Order.FULFILLED: assert best_versions == [] @@ -289,65 +317,6 @@ def test_get_valid_coupon_versions_over_redeemed(basket_and_coupons, order_statu ] -@pytest.mark.parametrize("auto_only", [True, False]) -def test_get_best_coupon_for_basket(basket_and_coupons, auto_only): - """ - Verify that the CouponPaymentVersion with the best price is returned for a bucket based on auto filter - """ - best_cv = best_coupon_for_basket( - basket_and_coupons.basket_item.basket, auto_only=auto_only - ) - if auto_only: - assert best_cv == basket_and_coupons.coupongroup_worst.coupon_version - else: - assert best_cv == basket_and_coupons.coupongroup_best.coupon_version - - -@pytest.mark.parametrize("code", ["WORST", None]) -def test_get_best_coupon_for_basket_by_code(basket_and_coupons, code): - """ - Verify that the CouponPaymentVersion with the best price is returned for a bucket based on coupon code - """ - best_cv = best_coupon_for_basket( - basket_and_coupons.basket_item.basket, auto_only=False, code=code - ) - if code: - assert best_cv == basket_and_coupons.coupongroup_worst.coupon_version - else: - assert best_cv == basket_and_coupons.coupongroup_best.coupon_version - - -def test_get_best_coupon_for_basket_empty_basket(): - """ - Verify that the best_coupon_version() returns None if the basket has no product - """ - assert best_coupon_for_basket(BasketFactory()) is None - - -def test_get_best_coupon_for_basket_no_coupons(): - """ - Verify that best_coupon_version() returns None if the product has no coupons - """ - basket_item = BasketItemFactory() - ProductVersionFactory(product=basket_item.product, price=Decimal(25.00)) - assert best_coupon_for_basket(basket_item.basket) is None - - -def test_get_best_coupon_for_basket_no_valid_coupons(basket_and_coupons): - """ - Verify that best_coupon_version() returns None if the product coupons are invalid - """ - today = now_in_utc() - civ_worst = basket_and_coupons.coupongroup_worst.coupon_version.payment_version - civ_worst.activation_date = today + timedelta(days=1) - civ_worst.save() - - assert ( - best_coupon_for_basket(basket_and_coupons.basket_item.basket, code="WORST") - is None - ) - - def test_latest_coupon_version(basket_and_coupons): """ Verify that the most recent coupon version is returned diff --git a/ecommerce/conftest.py b/ecommerce/conftest.py index 51a5a5f3b..106b4e364 100644 --- a/ecommerce/conftest.py +++ b/ecommerce/conftest.py @@ -58,7 +58,7 @@ def basket_and_coupons(): payment=payment_best, amount=Decimal("0.50") ) # Coupon payment for best coupon, more recent than previous so takes precedence - civ_best = CouponPaymentVersionFactory(payment=payment_best, amount=Decimal("0.40")) + civ_best = CouponPaymentVersionFactory(payment=payment_best, amount=Decimal("1.00")) # Coupon version for worst coupon cv_worst = CouponVersionFactory(payment_version=civ_worst, coupon=coupon_worst) diff --git a/ecommerce/serializers.py b/ecommerce/serializers.py index f7a830469..99c4b91e9 100644 --- a/ecommerce/serializers.py +++ b/ecommerce/serializers.py @@ -544,18 +544,28 @@ def create(self, validated_data): payment_type=validated_data.get("payment_type"), payment_transaction=validated_data.get("payment_transaction"), ) - for coupon in range(validated_data.get("num_coupon_codes")): - coupon = models.Coupon.objects.create( + + eligibilities = [] + + coupons = [ + models.Coupon( coupon_code=validated_data.get("coupon_code", uuid4().hex), payment=payment, ) - models.CouponVersion.objects.create( - coupon=coupon, payment_version=payment_version - ) - for product_id in validated_data.get("product_ids"): - models.CouponEligibility.objects.create( - coupon=coupon, product_id=product_id - ) + for _ in range(validated_data.get("num_coupon_codes")) + ] + coupon_objs = models.Coupon.objects.bulk_create(coupons) + versions = [ + models.CouponVersion(coupon=obj, payment_version=payment_version) + for obj in coupon_objs + ] + eligibilities = [ + models.CouponEligibility(coupon=obj, product_id=product_id) + for obj in coupon_objs + for product_id in validated_data.get("product_ids") + ] + models.CouponVersion.objects.bulk_create(versions) + models.CouponEligibility.objects.bulk_create(eligibilities) return payment_version diff --git a/ecommerce/serializers_test.py b/ecommerce/serializers_test.py index efd2f9ea0..13f8942e2 100644 --- a/ecommerce/serializers_test.py +++ b/ecommerce/serializers_test.py @@ -2,6 +2,8 @@ Tests for ecommerce serializers """ # pylint: disable=unused-argument, redefined-outer-name +from decimal import Decimal + import pytest from mitxpro.test_utils import any_instance_of @@ -227,8 +229,9 @@ def test_serialize_coupon_payment_version_serializer(basket_and_coupons): assert serializer.data.get(attr) == getattr( basket_and_coupons.coupongroup_best.payment_version, attr ).strftime(datetime_format) - assert serializer.data.get("amount") == "{0:.2}".format( - basket_and_coupons.coupongroup_best.payment_version.amount + assert ( + Decimal(serializer.data.get("amount")) + == basket_and_coupons.coupongroup_best.payment_version.amount )