From 22da2d2dd51169f42c6ff02132a28c02066fd7ed Mon Sep 17 00:00:00 2001 From: Muhammad Anas Date: Thu, 15 Aug 2024 10:16:59 +0000 Subject: [PATCH 1/8] fix: fixed basket deletion issue --- ecommerce/api.py | 11 ++++++----- ecommerce/tasks.py | 15 ++++++++------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/ecommerce/api.py b/ecommerce/api.py index 574f222d4..9f46b4a6d 100644 --- a/ecommerce/api.py +++ b/ecommerce/api.py @@ -771,12 +771,13 @@ 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 - ) + with transaction.atomic(): + baskets = Basket.objects.select_for_update(skip_locked=True).filter( + user=order.purchaser + ) - # clear the basket - clear_and_delete_baskets(baskets) + # clear the basket + clear_and_delete_baskets(baskets) def enroll_user_in_order_items(order): diff --git a/ecommerce/tasks.py b/ecommerce/tasks.py index 512e8568c..3640f6850 100644 --- a/ecommerce/tasks.py +++ b/ecommerce/tasks.py @@ -4,7 +4,7 @@ from datetime import timedelta from django.conf import settings - +from django.db import transaction from ecommerce.api import clear_and_delete_baskets from ecommerce.models import Basket from mitxpro.celery import app @@ -21,9 +21,10 @@ def delete_expired_baskets(self): 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) + with transaction.atomic(): + 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) From 1db7b5442ea96689834aaf089cb6ab150b703139 Mon Sep 17 00:00:00 2001 From: Muhammad Anas Date: Fri, 16 Aug 2024 09:29:45 +0000 Subject: [PATCH 2/8] refactor: some changes to code --- ecommerce/api.py | 72 ++++++++++++++++++++++------------------- ecommerce/api_test.py | 52 ++++++++++++++++++++++++++--- ecommerce/conftest.py | 18 +++++++++++ ecommerce/tasks.py | 17 ++-------- ecommerce/tasks_test.py | 33 +------------------ 5 files changed, 108 insertions(+), 84 deletions(-) diff --git a/ecommerce/api.py b/ecommerce/api.py index 9f46b4a6d..1c9c2a7da 100644 --- a/ecommerce/api.py +++ b/ecommerce/api.py @@ -70,6 +70,7 @@ from maxmind.api import ip_to_country_code from mitxpro.utils import case_insensitive_equal, first_or_none, now_in_utc + log = logging.getLogger(__name__) ISO_8601_FORMAT = "%Y-%m-%dT%H:%M:%SZ" @@ -716,42 +717,52 @@ 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) - - with transaction.atomic(): - basket_items = basket.basketitems.all() - log.info( - "Deleting basket items: %s", - list(basket_items.values_list("id", flat=True)), + with transaction.atomic(): + if user: + baskets = Basket.objects.select_for_update(skip_locked=True).filter( + user=user ) - basket_items.delete() - - course_run_selections = basket.courserunselection_set.all() - course_run_selections_ids = list( - course_run_selections.values_list("id", flat=True) + else: + cutoff_date = now_in_utc() - timedelta(days=settings.BASKET_EXPIRY_DAYS) + baskets = Basket.objects.select_for_update(skip_locked=True).filter( + updated_on__lte=cutoff_date ) - log.info("Deleting course run selections: %s", course_run_selections_ids) - course_run_selections.delete() + 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() + 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,13 +782,8 @@ def complete_order(order): if order_coupon_ids: set_coupons_to_redeemed(order.purchaser.email, order_coupon_ids) - with transaction.atomic(): - baskets = Basket.objects.select_for_update(skip_locked=True).filter( - user=order.purchaser - ) - # clear the basket - clear_and_delete_baskets(baskets) + 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..6309b9799 100644 --- a/ecommerce/api_test.py +++ b/ecommerce/api_test.py @@ -6,6 +6,7 @@ import hmac import ipaddress import uuid +import datetime from base64 import b64encode from collections import defaultdict from datetime import timedelta @@ -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,56 @@ 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(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_expired_basket(mocker, user, basket_and_coupons): + """ + 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() + + 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) + + assert Basket.objects.filter(user=user).count() == 1 + clear_and_delete_baskets() + assert Basket.objects.filter(user=user).count() == 0 + + +def test_active_baskets_are_not_deleted(mocker, user, basket_and_coupons): + """Test that the active baskets are not deleted on calling clear_and_delete_baskets fn without user argument""" + basket_and_coupons.basket.user = user + basket_and_coupons.basket.save() + + mocker.patch("django.conf.settings.BASKET_EXPIRY_DAYS", 15) + assert Basket.objects.filter(user=user).count() == 1 + clear_and_delete_baskets() + assert Basket.objects.filter(user=user).count() == 1 + + 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 +1150,7 @@ 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][0] == basket_and_coupons.basket.user ) diff --git a/ecommerce/conftest.py b/ecommerce/conftest.py index 5641d2fd3..421a1abdd 100644 --- a/ecommerce/conftest.py +++ b/ecommerce/conftest.py @@ -9,6 +9,7 @@ from ecommerce.api import ValidatedBasket from ecommerce.constants import DISCOUNT_TYPE_PERCENT_OFF from ecommerce.factories import ( + BasketFactory, BasketItemFactory, CompanyFactory, CouponEligibilityFactory, @@ -22,12 +23,29 @@ ProductVersionFactory, ) from ecommerce.models import CourseRunSelection +from users.factories import UserFactory + CouponGroup = namedtuple( # noqa: PYI024 "CouponGroup", ["coupon", "coupon_version", "payment", "payment_version"] ) +@pytest.fixture +def baskets_with_different_users(): + """ + Multiple baskets for different users + """ + user1 = UserFactory.create() + basket1 = BasketFactory.create(user=user1) + + user2 = UserFactory.create() + basket2 = BasketFactory.create(user=user2) + + + return SimpleNamespace(baskets=[basket1, basket2]) + + @pytest.fixture def basket_and_coupons(): """ diff --git a/ecommerce/tasks.py b/ecommerce/tasks.py index 3640f6850..133798067 100644 --- a/ecommerce/tasks.py +++ b/ecommerce/tasks.py @@ -1,14 +1,10 @@ """Ecommerce Tasks""" import logging -from datetime import timedelta -from django.conf import settings -from django.db import transaction 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,13 +14,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()) - - with transaction.atomic(): - 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..dd7d0c9fa 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() From 3fcb9339252ad212cb782f3e10f0995bf6a197e1 Mon Sep 17 00:00:00 2001 From: Muhammad Anas Date: Fri, 16 Aug 2024 09:32:20 +0000 Subject: [PATCH 3/8] fix: formatting issues --- ecommerce/api.py | 1 - ecommerce/api_test.py | 6 +++--- ecommerce/conftest.py | 3 +-- ecommerce/tasks.py | 1 - 4 files changed, 4 insertions(+), 7 deletions(-) diff --git a/ecommerce/api.py b/ecommerce/api.py index 1c9c2a7da..00cc921e0 100644 --- a/ecommerce/api.py +++ b/ecommerce/api.py @@ -70,7 +70,6 @@ from maxmind.api import ip_to_country_code from mitxpro.utils import case_insensitive_equal, first_or_none, now_in_utc - log = logging.getLogger(__name__) ISO_8601_FORMAT = "%Y-%m-%dT%H:%M:%SZ" diff --git a/ecommerce/api_test.py b/ecommerce/api_test.py index 6309b9799..79fad9fab 100644 --- a/ecommerce/api_test.py +++ b/ecommerce/api_test.py @@ -2,11 +2,11 @@ Test for ecommerce functions """ +import datetime import hashlib import hmac import ipaddress import uuid -import datetime from base64 import b64encode from collections import defaultdict from datetime import timedelta @@ -1098,7 +1098,7 @@ def test_delete_baskets_with_user_args(baskets_with_different_users): 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 @@ -1132,7 +1132,7 @@ def test_active_baskets_are_not_deleted(mocker, user, basket_and_coupons): clear_and_delete_baskets() assert Basket.objects.filter(user=user).count() == 1 - + 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 diff --git a/ecommerce/conftest.py b/ecommerce/conftest.py index 421a1abdd..df1455aa7 100644 --- a/ecommerce/conftest.py +++ b/ecommerce/conftest.py @@ -25,7 +25,6 @@ from ecommerce.models import CourseRunSelection from users.factories import UserFactory - CouponGroup = namedtuple( # noqa: PYI024 "CouponGroup", ["coupon", "coupon_version", "payment", "payment_version"] ) @@ -41,7 +40,7 @@ def baskets_with_different_users(): user2 = UserFactory.create() basket2 = BasketFactory.create(user=user2) - + return SimpleNamespace(baskets=[basket1, basket2]) diff --git a/ecommerce/tasks.py b/ecommerce/tasks.py index 133798067..864b453cf 100644 --- a/ecommerce/tasks.py +++ b/ecommerce/tasks.py @@ -5,7 +5,6 @@ from ecommerce.api import clear_and_delete_baskets from mitxpro.celery import app - log = logging.getLogger(__name__) From 56e61f736b22d3e48e630fe01a3940f69f5eaae3 Mon Sep 17 00:00:00 2001 From: Muhammad Anas Date: Fri, 16 Aug 2024 09:40:37 +0000 Subject: [PATCH 4/8] refactor: expired basket test --- ecommerce/api_test.py | 27 ++++++++++----------------- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/ecommerce/api_test.py b/ecommerce/api_test.py index 79fad9fab..32d8d0227 100644 --- a/ecommerce/api_test.py +++ b/ecommerce/api_test.py @@ -1105,32 +1105,25 @@ def test_delete_baskets_with_user_args(baskets_with_different_users): assert Basket.objects.filter(user=user2).count() == 1 # Not deleting basket of other users -def test_delete_expired_basket(mocker, user, basket_and_coupons): +@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() - 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) - - assert Basket.objects.filter(user=user).count() == 1 - clear_and_delete_baskets() - assert Basket.objects.filter(user=user).count() == 0 - - -def test_active_baskets_are_not_deleted(mocker, user, basket_and_coupons): - """Test that the active baskets are not 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) - mocker.patch("django.conf.settings.BASKET_EXPIRY_DAYS", 15) assert Basket.objects.filter(user=user).count() == 1 clear_and_delete_baskets() - assert Basket.objects.filter(user=user).count() == 1 + assert bool(Basket.objects.filter(user=user).count()) != is_expired def test_complete_order(mocker, user, basket_and_coupons): From 653b552721a345f114532ebd7e31c327a934f997 Mon Sep 17 00:00:00 2001 From: Muhammad Anas Date: Fri, 16 Aug 2024 10:03:53 +0000 Subject: [PATCH 5/8] fix: tests --- ecommerce/api.py | 4 ++-- ecommerce/api_test.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ecommerce/api.py b/ecommerce/api.py index 00cc921e0..cd2561047 100644 --- a/ecommerce/api.py +++ b/ecommerce/api.py @@ -781,8 +781,8 @@ def complete_order(order): if order_coupon_ids: set_coupons_to_redeemed(order.purchaser.email, order_coupon_ids) - # clear the basket - clear_and_delete_baskets(order.purchaser) + # 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 32d8d0227..89d95011b 100644 --- a/ecommerce/api_test.py +++ b/ecommerce/api_test.py @@ -1143,7 +1143,7 @@ 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.user + patched_clear_and_delete_baskets.call_args[0][0] == basket_and_coupons.basket.user ) From 4d7efc26b05f8df3f8b1bc0d9951f831b0846d05 Mon Sep 17 00:00:00 2001 From: Muhammad Anas Date: Fri, 16 Aug 2024 10:34:29 +0000 Subject: [PATCH 6/8] fix: test --- ecommerce/tasks_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ecommerce/tasks_test.py b/ecommerce/tasks_test.py index dd7d0c9fa..e1a9b1d3e 100644 --- a/ecommerce/tasks_test.py +++ b/ecommerce/tasks_test.py @@ -10,4 +10,4 @@ def test_delete_expired_baskets(mocker): ) tasks.delete_expired_baskets.delay() - patched_clear_and_delete_baskets.assert_called_once_with(mocker.ANY) + patched_clear_and_delete_baskets.assert_called_once_with() From 6e67f590213459e5ed8cb989bcc32c2de6802db4 Mon Sep 17 00:00:00 2001 From: Muhammad Anas Date: Fri, 16 Aug 2024 15:21:03 +0000 Subject: [PATCH 7/8] fix: some code suggestions --- ecommerce/api.py | 21 ++++++++++---------- ecommerce/api_test.py | 46 +++++++++++++++++++------------------------ ecommerce/conftest.py | 15 -------------- 3 files changed, 30 insertions(+), 52 deletions(-) diff --git a/ecommerce/api.py b/ecommerce/api.py index cd2561047..6e3df7c75 100644 --- a/ecommerce/api.py +++ b/ecommerce/api.py @@ -723,16 +723,11 @@ def clear_and_delete_baskets(user=None): 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(): - if user: - baskets = Basket.objects.select_for_update(skip_locked=True).filter( - user=user - ) - else: - cutoff_date = now_in_utc() - timedelta(days=settings.BASKET_EXPIRY_DAYS) - baskets = Basket.objects.select_for_update(skip_locked=True).filter( - updated_on__lte=cutoff_date - ) + 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], @@ -752,11 +747,15 @@ def clear_and_delete_baskets(user=None): course_run_selections_ids = list( course_run_selections.values_list("id", flat=True) ) - log.info("Deleting course run selections: %s", course_run_selections_ids) + 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)) + coupon_selections_ids = list( + coupon_selections.values_list("id", flat=True) + ) log.info("Deleting coupon selections: %s", coupon_selections_ids) coupon_selections.delete() diff --git a/ecommerce/api_test.py b/ecommerce/api_test.py index 89d95011b..b4dfd1ccb 100644 --- a/ecommerce/api_test.py +++ b/ecommerce/api_test.py @@ -11,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 @@ -1090,40 +1090,34 @@ def test_clear_baskets(user, basket_and_coupons): assert CouponSelection.objects.filter(basket__user=user).count() == 0 -def test_delete_baskets_with_user_args(baskets_with_different_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 """ - 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 + 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 -@pytest.mark.parametrize("is_expired", [True, False]) -def test_delete_expired_basket(mocker, user, basket_and_coupons, 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 """ - 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 + 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 bool(Basket.objects.filter(user=user).count()) != is_expired + 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): diff --git a/ecommerce/conftest.py b/ecommerce/conftest.py index df1455aa7..a5684fb89 100644 --- a/ecommerce/conftest.py +++ b/ecommerce/conftest.py @@ -30,21 +30,6 @@ ) -@pytest.fixture -def baskets_with_different_users(): - """ - Multiple baskets for different users - """ - user1 = UserFactory.create() - basket1 = BasketFactory.create(user=user1) - - user2 = UserFactory.create() - basket2 = BasketFactory.create(user=user2) - - - return SimpleNamespace(baskets=[basket1, basket2]) - - @pytest.fixture def basket_and_coupons(): """ From 8e3d53456e6279b5d22453120a10865fe70c43f8 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 19 Aug 2024 12:08:47 +0000 Subject: [PATCH 8/8] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- ecommerce/api.py | 4 +++- ecommerce/api_test.py | 17 ++++++++++++----- ecommerce/conftest.py | 2 -- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/ecommerce/api.py b/ecommerce/api.py index 6e3df7c75..ce20da78e 100644 --- a/ecommerce/api.py +++ b/ecommerce/api.py @@ -727,7 +727,9 @@ def clear_and_delete_baskets(user=None): 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) + 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], diff --git a/ecommerce/api_test.py b/ecommerce/api_test.py index b4dfd1ccb..38d268cfe 100644 --- a/ecommerce/api_test.py +++ b/ecommerce/api_test.py @@ -1095,7 +1095,7 @@ 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 @@ -1109,15 +1109,21 @@ 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) + 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) + 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] + 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): @@ -1137,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] == basket_and_coupons.basket.user + patched_clear_and_delete_baskets.call_args[0][0] + == basket_and_coupons.basket.user ) diff --git a/ecommerce/conftest.py b/ecommerce/conftest.py index a5684fb89..5641d2fd3 100644 --- a/ecommerce/conftest.py +++ b/ecommerce/conftest.py @@ -9,7 +9,6 @@ from ecommerce.api import ValidatedBasket from ecommerce.constants import DISCOUNT_TYPE_PERCENT_OFF from ecommerce.factories import ( - BasketFactory, BasketItemFactory, CompanyFactory, CouponEligibilityFactory, @@ -23,7 +22,6 @@ ProductVersionFactory, ) from ecommerce.models import CourseRunSelection -from users.factories import UserFactory CouponGroup = namedtuple( # noqa: PYI024 "CouponGroup", ["coupon", "coupon_version", "payment", "payment_version"]