fix: fixed basket deletion issue

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):

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
     )
 
 

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()

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()