From 4c664240e15a8067777561b58f679ebcabac6c2b Mon Sep 17 00:00:00 2001 From: Jason Bau Date: Tue, 15 Oct 2013 16:17:06 -0700 Subject: [PATCH 1/4] Move logic that decides whether to show shoppingcart to middleware Middleware is a better place for this, rather than navigation.html template --- lms/djangoapps/shoppingcart/middleware.py | 31 +++++++ .../shoppingcart/tests/test_middleware.py | 80 +++++++++++++++++++ lms/envs/common.py | 3 + lms/templates/navigation.html | 6 +- 4 files changed, 115 insertions(+), 5 deletions(-) create mode 100644 lms/djangoapps/shoppingcart/middleware.py create mode 100644 lms/djangoapps/shoppingcart/tests/test_middleware.py diff --git a/lms/djangoapps/shoppingcart/middleware.py b/lms/djangoapps/shoppingcart/middleware.py new file mode 100644 index 0000000000..44f968ce34 --- /dev/null +++ b/lms/djangoapps/shoppingcart/middleware.py @@ -0,0 +1,31 @@ +""" +This is the shoppingcart middleware class. +Currently the only middleware is detects whether request.user has a cart that should be displayed in the +navigation. We want to do this in the middleware to +1) keep database accesses out of templates (this led to a transaction bug with user email changes) +2) because navigation.html is "called" by being included in other templates, there's no "views.py" to put this. +""" +from django.conf import settings +import shoppingcart + +class UserHasCartMiddleware(object): + """ + Detects whether request.user has a cart and sets it as part of the request + + *** + Because this relies on request.user, it needs to enabled in settings after + django.contrib.auth.middleware.AuthenticationMiddleware (or equivalent) + *** + """ + def process_request(self, request): + """ + Checks if request has an authenticated user. If so, checks if request.user has a cart that should + be displayed. Anonymous users don't. + """ + request.display_shopping_cart = False + if (request.user.is_authenticated() and # user exists + settings.MITX_FEATURES.get('ENABLE_PAID_COURSE_REGISTRATION') and # settings are set + settings.MITX_FEATURES.get('ENABLE_SHOPPING_CART') and + shoppingcart.models.Order.user_cart_has_items(request.user)): # user's cart is non-empty + request.display_shopping_cart = True + return None \ No newline at end of file diff --git a/lms/djangoapps/shoppingcart/tests/test_middleware.py b/lms/djangoapps/shoppingcart/tests/test_middleware.py new file mode 100644 index 0000000000..0f124e6e66 --- /dev/null +++ b/lms/djangoapps/shoppingcart/tests/test_middleware.py @@ -0,0 +1,80 @@ +""" +Unit tests for shoppingcart middleware +""" +from mock import patch, Mock +from django.conf import settings +from django.contrib.auth.models import AnonymousUser +from django.test.utils import override_settings + +from courseware.tests.tests import TEST_DATA_MONGO_MODULESTORE +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory +from student.tests.factories import UserFactory +from course_modes.models import CourseMode +from shoppingcart.models import Order, PaidCourseRegistration +from shoppingcart.middleware import UserHasCartMiddleware + +@override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) +class UserCartMiddlewareUnitTest(ModuleStoreTestCase): + def setUp(self): + self.user = UserFactory.create() + self.request = Mock() + self.mw = UserHasCartMiddleware() + + def add_to_cart(self): + course = CourseFactory.create(org='MITx', number='999', display_name='Robot Super Course') + course_mode = CourseMode(course_id=course.id, + mode_slug="honor", + mode_display_name="honor cert", + min_price=40) + course_mode.save() + cart = Order.get_cart_for_user(self.user) + PaidCourseRegistration.add_to_order(cart, course.id) + + @patch.dict(settings.MITX_FEATURES, {'ENABLE_SHOPPING_CART': False, 'ENABLE_PAID_COURSE_REGISTRATION': True}) + def test_no_enable_shoppingcart(self): + """ + Tests when MITX_FEATURES['ENABLE_SHOPPING_CART'] is not set + """ + self.add_to_cart() + self.request.user = self.user + self.mw.process_request(self.request) + self.assertFalse(self.request.display_shopping_cart) + + @patch.dict(settings.MITX_FEATURES, {'ENABLE_SHOPPING_CART': True, 'ENABLE_PAID_COURSE_REGISTRATION': False}) + def test_no_enable_paid_course_registration(self): + """ + Tests when MITX_FEATURES['ENABLE_PAID_COURSE_REGISTRATION'] is not set + """ + self.add_to_cart() + self.request.user = self.user + self.mw.process_request(self.request) + self.assertFalse(self.request.display_shopping_cart) + + @patch.dict(settings.MITX_FEATURES, {'ENABLE_SHOPPING_CART': True, 'ENABLE_PAID_COURSE_REGISTRATION': True}) + def test_anonymous_user(self): + """ + Tests when request.user is anonymous + """ + self.request.user = AnonymousUser() + self.mw.process_request(self.request) + self.assertFalse(self.request.display_shopping_cart) + + @patch.dict(settings.MITX_FEATURES, {'ENABLE_SHOPPING_CART': True, 'ENABLE_PAID_COURSE_REGISTRATION': True}) + def test_no_items_in_cart(self): + """ + Tests when request.user doesn't have a cart with items + """ + self.request.user = self.user + self.mw.process_request(self.request) + self.assertFalse(self.request.display_shopping_cart) + + @patch.dict(settings.MITX_FEATURES, {'ENABLE_SHOPPING_CART': True, 'ENABLE_PAID_COURSE_REGISTRATION': True}) + def test_items_in_cart(self): + """ + Tests when request.user has a cart with items + """ + self.add_to_cart() + self.request.user = self.user + self.mw.process_request(self.request) + self.assertTrue(self.request.display_shopping_cart) diff --git a/lms/envs/common.py b/lms/envs/common.py index db66b821ca..15ff00eeaf 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -610,6 +610,9 @@ MIDDLEWARE_CLASSES = ( # For A/B testing 'waffle.middleware.WaffleMiddleware', + + # Shoppingcart middleware (detects if request.user has a cart) + 'shoppingcart.middleware.UserHasCartMiddleware', ) ############################### Pipeline ####################################### diff --git a/lms/templates/navigation.html b/lms/templates/navigation.html index e2e1d9d664..7f38561b3c 100644 --- a/lms/templates/navigation.html +++ b/lms/templates/navigation.html @@ -9,8 +9,6 @@ from django.utils.translation import ugettext as _ import branding # app that handles site status messages from status.status import get_site_status_msg -# shopping cart -import shoppingcart %> ## Provide a hook for themes to inject branding on top. @@ -83,9 +81,7 @@ site_status_msg = get_site_status_msg(course_id) - % if settings.MITX_FEATURES.get('ENABLE_PAID_COURSE_REGISTRATION') and \ - settings.MITX_FEATURES.get('ENABLE_SHOPPING_CART') and \ - shoppingcart.models.Order.user_cart_has_items(user): + % if getattr(request, 'display_shopping_cart', False): # see shoppingcart.middleware.UserHasCartMiddleware
  1. From 6e543c1470e6e66cea0ae5d4a1ec4a2c3f1a75fc Mon Sep 17 00:00:00 2001 From: Jason Bau Date: Tue, 15 Oct 2013 17:30:26 -0700 Subject: [PATCH 2/4] pep8 + pylint --- lms/djangoapps/shoppingcart/middleware.py | 13 ++++++++----- .../shoppingcart/tests/test_middleware.py | 19 +++++++++++++------ 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/lms/djangoapps/shoppingcart/middleware.py b/lms/djangoapps/shoppingcart/middleware.py index 44f968ce34..de82150354 100644 --- a/lms/djangoapps/shoppingcart/middleware.py +++ b/lms/djangoapps/shoppingcart/middleware.py @@ -8,6 +8,7 @@ navigation. We want to do this in the middleware to from django.conf import settings import shoppingcart + class UserHasCartMiddleware(object): """ Detects whether request.user has a cart and sets it as part of the request @@ -23,9 +24,11 @@ class UserHasCartMiddleware(object): be displayed. Anonymous users don't. """ request.display_shopping_cart = False - if (request.user.is_authenticated() and # user exists - settings.MITX_FEATURES.get('ENABLE_PAID_COURSE_REGISTRATION') and # settings are set - settings.MITX_FEATURES.get('ENABLE_SHOPPING_CART') and - shoppingcart.models.Order.user_cart_has_items(request.user)): # user's cart is non-empty + if ( + request.user.is_authenticated() and # user exists + settings.MITX_FEATURES.get('ENABLE_PAID_COURSE_REGISTRATION') and # settings is set + settings.MITX_FEATURES.get('ENABLE_SHOPPING_CART') and # setting is set + shoppingcart.models.Order.user_cart_has_items(request.user) # user's cart is non-empty + ): request.display_shopping_cart = True - return None \ No newline at end of file + return None diff --git a/lms/djangoapps/shoppingcart/tests/test_middleware.py b/lms/djangoapps/shoppingcart/tests/test_middleware.py index 0f124e6e66..0613845939 100644 --- a/lms/djangoapps/shoppingcart/tests/test_middleware.py +++ b/lms/djangoapps/shoppingcart/tests/test_middleware.py @@ -14,14 +14,21 @@ from course_modes.models import CourseMode from shoppingcart.models import Order, PaidCourseRegistration from shoppingcart.middleware import UserHasCartMiddleware + @override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) class UserCartMiddlewareUnitTest(ModuleStoreTestCase): + """ + Unit test for shoppingcart middleware UserHasCartMiddleware + """ def setUp(self): self.user = UserFactory.create() self.request = Mock() - self.mw = UserHasCartMiddleware() + self.middleware = UserHasCartMiddleware() def add_to_cart(self): + """ + Adds content to self.user's cart + """ course = CourseFactory.create(org='MITx', number='999', display_name='Robot Super Course') course_mode = CourseMode(course_id=course.id, mode_slug="honor", @@ -38,7 +45,7 @@ class UserCartMiddlewareUnitTest(ModuleStoreTestCase): """ self.add_to_cart() self.request.user = self.user - self.mw.process_request(self.request) + self.middleware.process_request(self.request) self.assertFalse(self.request.display_shopping_cart) @patch.dict(settings.MITX_FEATURES, {'ENABLE_SHOPPING_CART': True, 'ENABLE_PAID_COURSE_REGISTRATION': False}) @@ -48,7 +55,7 @@ class UserCartMiddlewareUnitTest(ModuleStoreTestCase): """ self.add_to_cart() self.request.user = self.user - self.mw.process_request(self.request) + self.middleware.process_request(self.request) self.assertFalse(self.request.display_shopping_cart) @patch.dict(settings.MITX_FEATURES, {'ENABLE_SHOPPING_CART': True, 'ENABLE_PAID_COURSE_REGISTRATION': True}) @@ -57,7 +64,7 @@ class UserCartMiddlewareUnitTest(ModuleStoreTestCase): Tests when request.user is anonymous """ self.request.user = AnonymousUser() - self.mw.process_request(self.request) + self.middleware.process_request(self.request) self.assertFalse(self.request.display_shopping_cart) @patch.dict(settings.MITX_FEATURES, {'ENABLE_SHOPPING_CART': True, 'ENABLE_PAID_COURSE_REGISTRATION': True}) @@ -66,7 +73,7 @@ class UserCartMiddlewareUnitTest(ModuleStoreTestCase): Tests when request.user doesn't have a cart with items """ self.request.user = self.user - self.mw.process_request(self.request) + self.middleware.process_request(self.request) self.assertFalse(self.request.display_shopping_cart) @patch.dict(settings.MITX_FEATURES, {'ENABLE_SHOPPING_CART': True, 'ENABLE_PAID_COURSE_REGISTRATION': True}) @@ -76,5 +83,5 @@ class UserCartMiddlewareUnitTest(ModuleStoreTestCase): """ self.add_to_cart() self.request.user = self.user - self.mw.process_request(self.request) + self.middleware.process_request(self.request) self.assertTrue(self.request.display_shopping_cart) From ec8871206aef2b79f9b76ddf63ad10228a224ee2 Mon Sep 17 00:00:00 2001 From: Jason Bau Date: Wed, 16 Oct 2013 12:16:10 -0700 Subject: [PATCH 3/4] use context processor instead of middleware --- .../shoppingcart/context_processor.py | 26 +++++++++++++ lms/djangoapps/shoppingcart/middleware.py | 34 ----------------- ...iddleware.py => test_context_processor.py} | 37 ++++++++----------- lms/envs/common.py | 6 +-- lms/templates/navigation.html | 2 +- 5 files changed, 46 insertions(+), 59 deletions(-) create mode 100644 lms/djangoapps/shoppingcart/context_processor.py delete mode 100644 lms/djangoapps/shoppingcart/middleware.py rename lms/djangoapps/shoppingcart/tests/{test_middleware.py => test_context_processor.py} (68%) diff --git a/lms/djangoapps/shoppingcart/context_processor.py b/lms/djangoapps/shoppingcart/context_processor.py new file mode 100644 index 0000000000..f835b589c4 --- /dev/null +++ b/lms/djangoapps/shoppingcart/context_processor.py @@ -0,0 +1,26 @@ +""" +This is the shoppingcart context_processor module. +Currently the only context_processor detects whether request.user has a cart that should be displayed in the +navigation. We want to do this in the context_processor to +1) keep database accesses out of templates (this led to a transaction bug with user email changes) +2) because navigation.html is "called" by being included in other templates, there's no "views.py" to put this. +""" +from django.conf import settings +import shoppingcart + + +def user_has_cart_context_processor(request): + """ + Checks if request has an authenticated user. If so, checks if request.user has a cart that should + be displayed. Anonymous users don't. + Adds `display_shopping_cart` to the context + """ + display_shopping_cart = False + if ( + request.user.is_authenticated() and # user exists + settings.MITX_FEATURES.get('ENABLE_PAID_COURSE_REGISTRATION') and # settings is set + settings.MITX_FEATURES.get('ENABLE_SHOPPING_CART') and # setting is set + shoppingcart.models.Order.user_cart_has_items(request.user) # user's cart is non-empty + ): + display_shopping_cart = True + return {'display_shopping_cart': display_shopping_cart} diff --git a/lms/djangoapps/shoppingcart/middleware.py b/lms/djangoapps/shoppingcart/middleware.py deleted file mode 100644 index de82150354..0000000000 --- a/lms/djangoapps/shoppingcart/middleware.py +++ /dev/null @@ -1,34 +0,0 @@ -""" -This is the shoppingcart middleware class. -Currently the only middleware is detects whether request.user has a cart that should be displayed in the -navigation. We want to do this in the middleware to -1) keep database accesses out of templates (this led to a transaction bug with user email changes) -2) because navigation.html is "called" by being included in other templates, there's no "views.py" to put this. -""" -from django.conf import settings -import shoppingcart - - -class UserHasCartMiddleware(object): - """ - Detects whether request.user has a cart and sets it as part of the request - - *** - Because this relies on request.user, it needs to enabled in settings after - django.contrib.auth.middleware.AuthenticationMiddleware (or equivalent) - *** - """ - def process_request(self, request): - """ - Checks if request has an authenticated user. If so, checks if request.user has a cart that should - be displayed. Anonymous users don't. - """ - request.display_shopping_cart = False - if ( - request.user.is_authenticated() and # user exists - settings.MITX_FEATURES.get('ENABLE_PAID_COURSE_REGISTRATION') and # settings is set - settings.MITX_FEATURES.get('ENABLE_SHOPPING_CART') and # setting is set - shoppingcart.models.Order.user_cart_has_items(request.user) # user's cart is non-empty - ): - request.display_shopping_cart = True - return None diff --git a/lms/djangoapps/shoppingcart/tests/test_middleware.py b/lms/djangoapps/shoppingcart/tests/test_context_processor.py similarity index 68% rename from lms/djangoapps/shoppingcart/tests/test_middleware.py rename to lms/djangoapps/shoppingcart/tests/test_context_processor.py index 0613845939..4cedca8d14 100644 --- a/lms/djangoapps/shoppingcart/tests/test_middleware.py +++ b/lms/djangoapps/shoppingcart/tests/test_context_processor.py @@ -1,5 +1,5 @@ """ -Unit tests for shoppingcart middleware +Unit tests for shoppingcart context_processor """ from mock import patch, Mock from django.conf import settings @@ -10,31 +10,26 @@ from courseware.tests.tests import TEST_DATA_MONGO_MODULESTORE from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory from student.tests.factories import UserFactory -from course_modes.models import CourseMode +from course_modes.tests.factories import CourseModeFactory from shoppingcart.models import Order, PaidCourseRegistration -from shoppingcart.middleware import UserHasCartMiddleware +from shoppingcart.context_processor import user_has_cart_context_processor @override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) -class UserCartMiddlewareUnitTest(ModuleStoreTestCase): +class UserCartContextProcessorUnitTest(ModuleStoreTestCase): """ - Unit test for shoppingcart middleware UserHasCartMiddleware + Unit test for shoppingcart context_processor """ def setUp(self): self.user = UserFactory.create() self.request = Mock() - self.middleware = UserHasCartMiddleware() def add_to_cart(self): """ Adds content to self.user's cart """ course = CourseFactory.create(org='MITx', number='999', display_name='Robot Super Course') - course_mode = CourseMode(course_id=course.id, - mode_slug="honor", - mode_display_name="honor cert", - min_price=40) - course_mode.save() + CourseModeFactory(course_id=course.id) cart = Order.get_cart_for_user(self.user) PaidCourseRegistration.add_to_order(cart, course.id) @@ -45,8 +40,8 @@ class UserCartMiddlewareUnitTest(ModuleStoreTestCase): """ self.add_to_cart() self.request.user = self.user - self.middleware.process_request(self.request) - self.assertFalse(self.request.display_shopping_cart) + context = user_has_cart_context_processor(self.request) + self.assertFalse(context['display_shopping_cart']) @patch.dict(settings.MITX_FEATURES, {'ENABLE_SHOPPING_CART': True, 'ENABLE_PAID_COURSE_REGISTRATION': False}) def test_no_enable_paid_course_registration(self): @@ -55,8 +50,8 @@ class UserCartMiddlewareUnitTest(ModuleStoreTestCase): """ self.add_to_cart() self.request.user = self.user - self.middleware.process_request(self.request) - self.assertFalse(self.request.display_shopping_cart) + context = user_has_cart_context_processor(self.request) + self.assertFalse(context['display_shopping_cart']) @patch.dict(settings.MITX_FEATURES, {'ENABLE_SHOPPING_CART': True, 'ENABLE_PAID_COURSE_REGISTRATION': True}) def test_anonymous_user(self): @@ -64,8 +59,8 @@ class UserCartMiddlewareUnitTest(ModuleStoreTestCase): Tests when request.user is anonymous """ self.request.user = AnonymousUser() - self.middleware.process_request(self.request) - self.assertFalse(self.request.display_shopping_cart) + context = user_has_cart_context_processor(self.request) + self.assertFalse(context['display_shopping_cart']) @patch.dict(settings.MITX_FEATURES, {'ENABLE_SHOPPING_CART': True, 'ENABLE_PAID_COURSE_REGISTRATION': True}) def test_no_items_in_cart(self): @@ -73,8 +68,8 @@ class UserCartMiddlewareUnitTest(ModuleStoreTestCase): Tests when request.user doesn't have a cart with items """ self.request.user = self.user - self.middleware.process_request(self.request) - self.assertFalse(self.request.display_shopping_cart) + context = user_has_cart_context_processor(self.request) + self.assertFalse(context['display_shopping_cart']) @patch.dict(settings.MITX_FEATURES, {'ENABLE_SHOPPING_CART': True, 'ENABLE_PAID_COURSE_REGISTRATION': True}) def test_items_in_cart(self): @@ -83,5 +78,5 @@ class UserCartMiddlewareUnitTest(ModuleStoreTestCase): """ self.add_to_cart() self.request.user = self.user - self.middleware.process_request(self.request) - self.assertTrue(self.request.display_shopping_cart) + context = user_has_cart_context_processor(self.request) + self.assertTrue(context['display_shopping_cart']) diff --git a/lms/envs/common.py b/lms/envs/common.py index 15ff00eeaf..ae37bf6202 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -272,6 +272,9 @@ TEMPLATE_CONTEXT_PROCESSORS = ( # Hack to get required link URLs to password reset templates 'mitxmako.shortcuts.marketing_link_context_processor', + + # Shoppingcart processor (detects if request.user has a cart) + 'shoppingcart.context_processor.user_has_cart_context_processor', ) # use the ratelimit backend to prevent brute force attacks @@ -610,9 +613,6 @@ MIDDLEWARE_CLASSES = ( # For A/B testing 'waffle.middleware.WaffleMiddleware', - - # Shoppingcart middleware (detects if request.user has a cart) - 'shoppingcart.middleware.UserHasCartMiddleware', ) ############################### Pipeline ####################################### diff --git a/lms/templates/navigation.html b/lms/templates/navigation.html index 7f38561b3c..b1e4c091e0 100644 --- a/lms/templates/navigation.html +++ b/lms/templates/navigation.html @@ -81,7 +81,7 @@ site_status_msg = get_site_status_msg(course_id)
- % if getattr(request, 'display_shopping_cart', False): # see shoppingcart.middleware.UserHasCartMiddleware + % if display_shopping_cart: # see shoppingcart.context_processor.user_has_cart_context_processor
  1. From 7223bb125e8fd966b09cfa4699d35ea2b29bace1 Mon Sep 17 00:00:00 2001 From: Jason Bau Date: Thu, 17 Oct 2013 14:21:44 -0700 Subject: [PATCH 4/4] make condition inline --- lms/djangoapps/shoppingcart/context_processor.py | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/lms/djangoapps/shoppingcart/context_processor.py b/lms/djangoapps/shoppingcart/context_processor.py index f835b589c4..b94f1cac2a 100644 --- a/lms/djangoapps/shoppingcart/context_processor.py +++ b/lms/djangoapps/shoppingcart/context_processor.py @@ -15,12 +15,9 @@ def user_has_cart_context_processor(request): be displayed. Anonymous users don't. Adds `display_shopping_cart` to the context """ - display_shopping_cart = False - if ( - request.user.is_authenticated() and # user exists - settings.MITX_FEATURES.get('ENABLE_PAID_COURSE_REGISTRATION') and # settings is set - settings.MITX_FEATURES.get('ENABLE_SHOPPING_CART') and # setting is set - shoppingcart.models.Order.user_cart_has_items(request.user) # user's cart is non-empty - ): - display_shopping_cart = True - return {'display_shopping_cart': display_shopping_cart} + return {'display_shopping_cart': ( + request.user.is_authenticated() and # user is logged in and + settings.MITX_FEATURES.get('ENABLE_PAID_COURSE_REGISTRATION') and # settings enable paid course reg and + settings.MITX_FEATURES.get('ENABLE_SHOPPING_CART') and # settings enable shopping cart and + shoppingcart.models.Order.user_cart_has_items(request.user) # user's cart has items + )}