From e1cb10b41d0a70b07d8bfa75178af8f1c80de9c4 Mon Sep 17 00:00:00 2001 From: Zia Fazal Date: Mon, 18 Nov 2019 19:57:03 +0500 Subject: [PATCH] Removed EnterpriseMiddleware EnterpriseMiddleware set value of user's enterprise customer in session. In order to get value of enterprise customer it calls `enterprise/api/v1/enterprise-learner` API. Sometimes this middleware is called many times which result in many call to the under lying API and throttling of API causes 429 http errors. We are not removing that middleware and storing value of user's enterprise customer in session inside underlying method. ENT-1849 Removed pdb statement --- lms/envs/common.py | 3 -- openedx/features/enterprise_support/api.py | 4 +- .../features/enterprise_support/middleware.py | 30 ----------- .../enterprise_support/tests/test_api.py | 31 +++++++++++ .../tests/test_middleware.py | 51 ------------------- 5 files changed, 34 insertions(+), 85 deletions(-) delete mode 100644 openedx/features/enterprise_support/middleware.py delete mode 100644 openedx/features/enterprise_support/tests/test_middleware.py diff --git a/lms/envs/common.py b/lms/envs/common.py index 8cbb27c148..cf810b6e42 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1510,9 +1510,6 @@ MIDDLEWARE_CLASSES = [ 'waffle.middleware.WaffleMiddleware', - # Inserts Enterprise content. - 'openedx.features.enterprise_support.middleware.EnterpriseMiddleware', - # Enables force_django_cache_miss functionality for TieredCache. 'edx_django_utils.cache.middleware.TieredCacheMiddleware', diff --git a/openedx/features/enterprise_support/api.py b/openedx/features/enterprise_support/api.py index fe226277ae..52aa426955 100644 --- a/openedx/features/enterprise_support/api.py +++ b/openedx/features/enterprise_support/api.py @@ -450,7 +450,9 @@ def enterprise_customer_for_request(request): if 'enterprise_customer' in request.session: return enterprise_customer_from_cache(request=request) else: - return enterprise_customer_from_api(request) + enterprise_customer = enterprise_customer_from_api(request) + request.session['enterprise_customer'] = enterprise_customer + return enterprise_customer @enterprise_is_enabled(otherwise=False) diff --git a/openedx/features/enterprise_support/middleware.py b/openedx/features/enterprise_support/middleware.py deleted file mode 100644 index 2cbe20b6d1..0000000000 --- a/openedx/features/enterprise_support/middleware.py +++ /dev/null @@ -1,30 +0,0 @@ -""" -Middleware for the Enterprise feature. - -The Enterprise feature must be turned on for this middleware to have any effect. -""" - -from __future__ import absolute_import - -from django.core.exceptions import MiddlewareNotUsed -from openedx.features.enterprise_support import api - - -class EnterpriseMiddleware(object): - """ - Middleware that adds Enterprise-related content to the request. - """ - - def __init__(self): - """ - We don't need to use this middleware if the Enterprise feature isn't enabled. - """ - if not api.enterprise_enabled(): - raise MiddlewareNotUsed() - - def process_request(self, request): - """ - Fill the request with Enterprise-related content. - """ - if 'enterprise_customer' not in request.session and request.user.is_authenticated: - request.session['enterprise_customer'] = api.enterprise_customer_for_request(request) diff --git a/openedx/features/enterprise_support/tests/test_api.py b/openedx/features/enterprise_support/tests/test_api.py index a3636dc09a..b8dfa8ea79 100644 --- a/openedx/features/enterprise_support/tests/test_api.py +++ b/openedx/features/enterprise_support/tests/test_api.py @@ -253,6 +253,7 @@ class TestEnterpriseApi(EnterpriseServiceMockMixin, CacheIsolationTestCase): # Verify that the method `enterprise_customer_for_request` returns no # enterprise customer if the enterprise customer API throws 404. + del dummy_request.session['enterprise_customer'] self.mock_get_enterprise_customer('real-ent-uuid', {'detail': 'Not found.'}, 404) enterprise_customer = enterprise_customer_for_request(dummy_request) self.assertIsNone(enterprise_customer) @@ -288,6 +289,36 @@ class TestEnterpriseApi(EnterpriseServiceMockMixin, CacheIsolationTestCase): ) self.assertEqual(enterprise_customer, {'real': 'enterprisecustomer'}) + def test_enterprise_customer_for_request_with_session(self): + """ + Verify enterprise_customer_for_request stores and retrieves data from session appropriately + """ + dummy_request = mock.MagicMock(session={}, user=self.user) + enterprise_data = {'name': 'dummy-enterprise-customer', 'uuid': '8dc65e66-27c9-447b-87ff-ede6d66e3a5d'} + + # Verify enterprise customer data fetched from API when it is not available in session + with mock.patch( + 'openedx.features.enterprise_support.api.enterprise_customer_from_api', + return_value=enterprise_data + ): + self.assertEqual(dummy_request.session.get('enterprise_customer'), None) + enterprise_customer = enterprise_customer_for_request(dummy_request) + self.assertEqual(enterprise_customer, enterprise_data) + self.assertEqual(dummy_request.session.get('enterprise_customer'), enterprise_data) + + # Verify enterprise customer data fetched from session for subsequent calls + with mock.patch( + 'openedx.features.enterprise_support.api.enterprise_customer_from_api', + return_value=enterprise_data + ) as mock_enterprise_customer_from_api, mock.patch( + 'openedx.features.enterprise_support.api.enterprise_customer_from_cache', + return_value=enterprise_data + ) as mock_enterprise_customer_from_cache: + enterprise_customer = enterprise_customer_for_request(dummy_request) + self.assertEqual(enterprise_customer, enterprise_data) + self.assertEqual(mock_enterprise_customer_from_api.called, False) + self.assertEqual(mock_enterprise_customer_from_cache.called, True) + def check_data_sharing_consent(self, consent_required=False, consent_url=None): """ Used to test the data_sharing_consent_required view decorator. diff --git a/openedx/features/enterprise_support/tests/test_middleware.py b/openedx/features/enterprise_support/tests/test_middleware.py deleted file mode 100644 index 0264dd128e..0000000000 --- a/openedx/features/enterprise_support/tests/test_middleware.py +++ /dev/null @@ -1,51 +0,0 @@ -""" -Tests for Enterprise middleware. -""" - -from __future__ import absolute_import - -import mock - -from django.test import TestCase -from django.test.utils import override_settings -from django.urls import reverse -from openedx.core.djangolib.testing.utils import skip_unless_lms -from openedx.features.enterprise_support.tests import ( - FAKE_ENTERPRISE_CUSTOMER, - FEATURES_WITH_ENTERPRISE_ENABLED, - factories -) -from student.tests.factories import UserFactory - - -@override_settings(FEATURES=FEATURES_WITH_ENTERPRISE_ENABLED) -@skip_unless_lms -class EnterpriseMiddlewareTest(TestCase): - """ - Test for `EnterpriseMiddleware`. - """ - - def setUp(self): - """Initiate commonly needed objects.""" - super(EnterpriseMiddlewareTest, self).setUp() - - # Customer & Learner details. - self.user = UserFactory.create(username='username', password='password') - self.enterprise_customer = FAKE_ENTERPRISE_CUSTOMER - self.enterprise_learner = factories.EnterpriseCustomerUserFactory(user_id=self.user.id) - - # Request details. - self.client.login(username='username', password='password') - self.dashboard = reverse('dashboard') - - # Mocks. - patcher = mock.patch('openedx.features.enterprise_support.api.enterprise_customer_from_api') - self.mock_enterprise_customer_from_api = patcher.start() - self.mock_enterprise_customer_from_api.return_value = self.enterprise_customer - self.addCleanup(patcher.stop) - - def test_anonymous_user(self): - """The `enterprise_customer` is not set in the session if the user is anonymous.""" - self.client.logout() - self.client.get(self.dashboard) - assert self.client.session.get('enterprise_customer') is None