fix: fixed basket deletion issue

[Anas12091101]

What are the relevant tickets?

Followup for #3021

Description (What does it do?)

We were getting some errors while testing the functionality of this PR on RC. This PR fixes those errors.

How can this be tested?

• Validate that the checkout page is working properly

1 In the .env file set the value BASKET_EXPIRY_DAYS=0 ( warning: this will delete all your baskets )
2 For any course, open the checkout page and don't complete the order. ( You can leave this step if you have existing basket and basket items )
3 Open the mitxpro shell by running python manage.py shell in the mitxpro directory
4 Run the following code in the shell:

>>> from ecommerce.models import Basket, BasketItem
>>> Basket.objects.all().count()
< number of existing baskets >
>>> BasketItem.objects.all().count()
< number of existing basket items >

5 Now run the following code

>>> from ecommerce.tasks import delete_expired_baskets
>>> delete_expired_baskets.delay()

6 In the celery logs, see the logs of this task. The task should be executed successfully and should show some useful logs.
7 Run the code in step 4 again. This time, the count will return 0

[arslanashraf7]

How about we make it more robust giving this single and whole responsibility to clear_and_delete_baskets to do everything needed. We can pass in a user to this method. So, If there is a user the clear_and_delete_baskets would delete all the associated baskets to that user otherwise it will delete the baskets based on the cutoff time? That way would make the code cleaner and short as well

[arslanashraf7]

I made some tweaks to this method for you. These changes should make it simpler. You should be able to replace this whole method with

def clear_and_delete_baskets(user=None):
    """
    Delete baskets and all the associated items.

    Args:
       user (User, optional): The user whose baskets should be deleted. If not provided, expired baskets will be deleted.
    """
    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():
        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()

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

[arslanashraf7]

We don't need another fixture baskets_with_different_users. I've re-written the test for you that tests exactly what we want to.

Suggested change

	def test_delete_baskets_with_user_args(baskets_with_different_users):
	"""
	Test to verify that the basket of the user passed in the clear_and_delete_baskets fn is deleted only
	"""
	user1 = baskets_with_different_users.baskets[0].user
	user2 = baskets_with_different_users.baskets[1].user
	assert Basket.objects.filter(user=user1).count() == 1
	clear_and_delete_baskets(user1)
	assert Basket.objects.filter(user=user1).count() == 0
	assert (
	Basket.objects.filter(user=user2).count() == 1
	) # Not deleting basket of other users
	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

[arslanashraf7]

This test has a complex readability and it wasn't testing the thing in a way it should have been. I've re-written it for you

Suggested change

	@pytest.mark.parametrize("is_expired", [True, False])
	def test_delete_expired_basket(mocker, user, basket_and_coupons, is_expired):
	"""
	Test to verify that the expired baskets are deleted on calling clear_and_delete_baskets fn without user argument
	"""
	basket_and_coupons.basket.user = user
	basket_and_coupons.basket.save()
	if is_expired:
	now_in_utc = mocker.patch("ecommerce.api.now_in_utc")
	now_in_utc.return_value = datetime.datetime.now(
	tz=datetime.timezone.utc
	) + datetime.timedelta(days=settings.BASKET_EXPIRY_DAYS)
	else:
	mocker.patch("django.conf.settings.BASKET_EXPIRY_DAYS", 15)
	assert Basket.objects.filter(user=user).count() == 1
	clear_and_delete_baskets()
	assert bool(Basket.objects.filter(user=user).count()) != is_expired
	@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]

[arslanashraf7]

This fixture can now be removed.

[Anas12091101]

@arslanashraf7 thanks for all the suggestions

[arslanashraf7]

Thanks for making all the changes 👍