diff --git a/ecommerce/api.py b/ecommerce/api.py index 574f222d4..ce20da78e 100644 --- a/ecommerce/api.py +++ b/ecommerce/api.py @@ -716,42 +716,53 @@ def set_coupons_to_redeemed(redeemed_email, coupon_ids): sheets.tasks.set_assignment_rows_to_enrolled.delay(sheet_update_map) -def clear_and_delete_baskets(baskets): +def clear_and_delete_baskets(user=None): """ Delete baskets and all the associated items. Args: - baskets (iterable of Baskets): A list of baskets whose associated items to be deleted + user (User, optional): The user whose baskets should be deleted. If not provided, expired baskets will be deleted. """ - log.info( - "Basket deletion requested for baskets Ids: %s", - [basket.id for basket in baskets], - ) - for basket in baskets: - log.info("Clearing and deleting basket with Id: %s", basket.id) + cutoff_date = now_in_utc() - timedelta(days=settings.BASKET_EXPIRY_DAYS) + basket_filter = {"user": user} if user else {"updated_on__lte": cutoff_date} - with transaction.atomic(): - basket_items = basket.basketitems.all() - log.info( - "Deleting basket items: %s", - list(basket_items.values_list("id", flat=True)), - ) - basket_items.delete() + with transaction.atomic(): + baskets = Basket.objects.select_for_update(skip_locked=True).filter( + **basket_filter + ) + log.info( + "Basket deletion requested for baskets Ids: %s", + [basket.id for basket in baskets], + ) + for basket in baskets: + log.info("Clearing and deleting basket with Id: %s", basket.id) + + with transaction.atomic(): + basket_items = basket.basketitems.all() + log.info( + "Deleting basket items: %s", + list(basket_items.values_list("id", flat=True)), + ) + basket_items.delete() - course_run_selections = basket.courserunselection_set.all() - course_run_selections_ids = list( - course_run_selections.values_list("id", flat=True) - ) - log.info("Deleting course run selections: %s", course_run_selections_ids) - course_run_selections.delete() + course_run_selections = basket.courserunselection_set.all() + course_run_selections_ids = list( + course_run_selections.values_list("id", flat=True) + ) + log.info( + "Deleting course run selections: %s", course_run_selections_ids + ) + course_run_selections.delete() - coupon_selections = basket.couponselection_set.all() - coupon_selections_ids = list(coupon_selections.values_list("id", flat=True)) - log.info("Deleting coupon selections: %s", coupon_selections_ids) - coupon_selections.delete() + coupon_selections = basket.couponselection_set.all() + coupon_selections_ids = list( + coupon_selections.values_list("id", flat=True) + ) + log.info("Deleting coupon selections: %s", coupon_selections_ids) + coupon_selections.delete() - log.info("Deleting basket: %s", basket.id) - basket.delete() + log.info("Deleting basket: %s", basket.id) + basket.delete() def complete_order(order): @@ -771,12 +782,8 @@ def complete_order(order): if order_coupon_ids: set_coupons_to_redeemed(order.purchaser.email, order_coupon_ids) - baskets = Basket.objects.select_for_update(skip_locked=True).filter( - user=order.purchaser - ) - - # clear the basket - clear_and_delete_baskets(baskets) + # clear and delete the basket + clear_and_delete_baskets(order.purchaser) def enroll_user_in_order_items(order): diff --git a/ecommerce/api_test.py b/ecommerce/api_test.py index ecc1f8592..38d268cfe 100644 --- a/ecommerce/api_test.py +++ b/ecommerce/api_test.py @@ -2,6 +2,7 @@ Test for ecommerce functions """ +import datetime import hashlib import hmac import ipaddress @@ -10,7 +11,7 @@ from collections import defaultdict from datetime import timedelta from decimal import Decimal -from unittest.mock import PropertyMock +from unittest.mock import PropertyMock, patch import factory import faker @@ -1071,9 +1072,9 @@ def test_company_multiple_global_consent_error(mocker, basket_and_agreement): ) -def test_clear_and_delete_baskets(user, basket_and_coupons): +def test_clear_baskets(user, basket_and_coupons): """ - Test to verify that the basket is cleared and deleted upon calling clear_and_delete_basket fn + Test to verify that the basket is cleared upon calling clear_and_delete_basket fn """ basket_and_coupons.basket.user = user basket_and_coupons.basket.save() @@ -1082,13 +1083,49 @@ def test_clear_and_delete_baskets(user, basket_and_coupons): assert CourseRunSelection.objects.filter(basket__user=user).count() > 0 assert CouponSelection.objects.filter(basket__user=user).count() > 0 - clear_and_delete_baskets([basket_and_coupons.basket]) + clear_and_delete_baskets(basket_and_coupons.basket.user) assert Basket.objects.filter(user=user).count() == 0 assert BasketItem.objects.filter(basket__user=user).count() == 0 assert CourseRunSelection.objects.filter(basket__user=user).count() == 0 assert CouponSelection.objects.filter(basket__user=user).count() == 0 +def test_delete_baskets_with_user_args(): + """ + Test to verify that the basket of the user passed in the clear_and_delete_baskets fn is deleted only + """ + baskets = BasketFactory.create_batch(2) + + assert Basket.objects.filter(user=baskets[0].user).count() == 1 + clear_and_delete_baskets(baskets[0].user) + assert Basket.objects.filter(user=baskets[0].user).count() == 0 + assert ( + Basket.objects.filter(user=baskets[1].user).count() == 1 + ) # Not deleting basket of other users + + +@patch("django.utils.timezone.now") +def test_delete_expired_basket(patch_now): + """ + Test to verify that the expired baskets are deleted on calling clear_and_delete_baskets fn without user argument + """ + patch_now.return_value = datetime.datetime.now( + tz=datetime.timezone.utc + ) - datetime.timedelta(days=settings.BASKET_EXPIRY_DAYS) + BasketFactory.create_batch(3) + patch_now.return_value = datetime.datetime.now( + tz=datetime.timezone.utc + ) + datetime.timedelta(days=settings.BASKET_EXPIRY_DAYS + 1) + unexpired_baskets = BasketFactory.create_batch(3) + patch_now.stop() + # Calling the clear baskets without user argument so it should delete the expired baskets + clear_and_delete_baskets() + assert Basket.objects.all().count() == 3 + assert list(Basket.objects.all().values_list("id", flat=True)) == [ + basket.id for basket in unexpired_baskets + ] + + def test_complete_order(mocker, user, basket_and_coupons): """ Test that complete_order enrolls a user in the items in their order and clears out checkout-related objects @@ -1106,7 +1143,8 @@ def test_complete_order(mocker, user, basket_and_coupons): patched_enroll.assert_called_once_with(order) patched_clear_and_delete_baskets.assert_called_once_with(mocker.ANY) assert ( - patched_clear_and_delete_baskets.call_args[0][0][0] == basket_and_coupons.basket + patched_clear_and_delete_baskets.call_args[0][0] + == basket_and_coupons.basket.user ) diff --git a/ecommerce/tasks.py b/ecommerce/tasks.py index 512e8568c..864b453cf 100644 --- a/ecommerce/tasks.py +++ b/ecommerce/tasks.py @@ -1,14 +1,9 @@ """Ecommerce Tasks""" import logging -from datetime import timedelta - -from django.conf import settings from ecommerce.api import clear_and_delete_baskets -from ecommerce.models import Basket from mitxpro.celery import app -from mitxpro.utils import now_in_utc log = logging.getLogger(__name__) @@ -18,12 +13,4 @@ def delete_expired_baskets(self): """Deletes the expired baskets""" log.info("Task ID: %s", self.request.id) - cutoff_date = now_in_utc() - timedelta(days=settings.BASKET_EXPIRY_DAYS) - log.info("Starting the deletion of expired baskets at %s", now_in_utc()) - - expired_baskets = Basket.objects.select_for_update(skip_locked=True).filter( - updated_on__lte=cutoff_date - ) - log.info("Found %d expired baskets to delete", len(expired_baskets)) - if expired_baskets: - clear_and_delete_baskets(expired_baskets) + clear_and_delete_baskets() diff --git a/ecommerce/tasks_test.py b/ecommerce/tasks_test.py index e4b27c9fd..e1a9b1d3e 100644 --- a/ecommerce/tasks_test.py +++ b/ecommerce/tasks_test.py @@ -1,44 +1,13 @@ """Ecommerce Tasks Tests""" -import datetime - -from django.conf import settings - from ecommerce import tasks -def test_delete_expired_baskets(mocker, user, basket_and_coupons): +def test_delete_expired_baskets(mocker): """Test that the expired baskets are deleted on task run""" patched_clear_and_delete_baskets = mocker.patch( "ecommerce.tasks.clear_and_delete_baskets" ) - basket_and_coupons.basket.user = user - basket_and_coupons.basket.save() - - now_in_utc = mocker.patch("ecommerce.tasks.now_in_utc") - now_in_utc.return_value = datetime.datetime.now( - tz=datetime.timezone.utc - ) + datetime.timedelta(days=settings.BASKET_EXPIRY_DAYS) - - tasks.delete_expired_baskets.delay() - - patched_clear_and_delete_baskets.assert_called_once_with(mocker.ANY) - assert ( - patched_clear_and_delete_baskets.call_args[0][0][0] == basket_and_coupons.basket - ) - - -def test_active_baskets_are_not_deleted(mocker, user, basket_and_coupons): - """Test that the active baskets are not deleted on task run""" - patched_clear_and_delete_baskets = mocker.patch( - "ecommerce.tasks.clear_and_delete_baskets" - ) - - basket_and_coupons.basket.user = user - basket_and_coupons.basket.save() - - mocker.patch("django.conf.settings.BASKET_EXPIRY_DAYS", 15) - tasks.delete_expired_baskets.delay() - patched_clear_and_delete_baskets.assert_not_called() + patched_clear_and_delete_baskets.assert_called_once_with()