From 78a9b1f0aee63c493f2a4e47e43d4df7679cde84 Mon Sep 17 00:00:00 2001 From: Renzo Lucioni <renzo@lucioni.xyz> Date: Mon, 8 May 2017 18:50:24 -0400 Subject: [PATCH] Remove use of read_cached_programs switch The read_cached_programs switch was used to control the release of changes for reading programs exclusively from the cache. With those changes stable, the switch is no longer necessary. LEARNER-382 --- common/test/acceptance/fixtures/catalog.py | 31 ++-- common/test/acceptance/pages/lms/catalog.py | 22 +++ .../acceptance/tests/lms/test_programs.py | 34 +++-- lms/djangoapps/courseware/tests/test_views.py | 21 ++- .../learner_dashboard/tests/test_programs.py | 12 +- lms/envs/bok_choy.env.json | 1 + lms/envs/common.py | 4 + lms/urls.py | 2 + .../djangoapps/catalog/tests/test_utils.py | 132 +----------------- openedx/core/djangoapps/catalog/urls.py | 8 ++ openedx/core/djangoapps/catalog/utils.py | 66 ++------- openedx/core/djangoapps/catalog/views.py | 21 +++ 12 files changed, 135 insertions(+), 219 deletions(-) create mode 100644 common/test/acceptance/pages/lms/catalog.py create mode 100644 openedx/core/djangoapps/catalog/urls.py create mode 100644 openedx/core/djangoapps/catalog/views.py diff --git a/common/test/acceptance/fixtures/catalog.py b/common/test/acceptance/fixtures/catalog.py index 32a6b8e05d6..7368d2a9622 100644 --- a/common/test/acceptance/fixtures/catalog.py +++ b/common/test/acceptance/fixtures/catalog.py @@ -13,23 +13,34 @@ class CatalogFixture(object): """ Interface to set up mock responses from the Catalog stub server. """ - def install_programs(self, data): - """Set response data for the catalog's course run API.""" + def install_programs(self, programs): + """ + Stub the discovery service's program list and detail API endpoints. + + Arguments: + programs (list): A list of programs. Both list and detail endpoints + will be stubbed using data from this list. + """ key = 'catalog.programs' - if isinstance(data, dict): - key += '.' + data['uuid'] + uuids = [] + for program in programs: + uuid = program['uuid'] + uuids.append(uuid) + + program_key = '{base}.{uuid}'.format(base=key, uuid=uuid) requests.put( '{}/set_config'.format(CATALOG_STUB_URL), - data={key: json.dumps(data)}, - ) - else: - requests.put( - '{}/set_config'.format(CATALOG_STUB_URL), - data={key: json.dumps({'results': data})}, + data={program_key: json.dumps(program)}, ) + # Stub list endpoint as if the uuids_only query param had been passed. + requests.put( + '{}/set_config'.format(CATALOG_STUB_URL), + data={key: json.dumps(uuids)}, + ) + class CatalogIntegrationMixin(object): """Mixin providing a method used to configure the catalog integration.""" diff --git a/common/test/acceptance/pages/lms/catalog.py b/common/test/acceptance/pages/lms/catalog.py new file mode 100644 index 00000000000..ad1335266f0 --- /dev/null +++ b/common/test/acceptance/pages/lms/catalog.py @@ -0,0 +1,22 @@ +import re + +from bok_choy.page_object import PageObject + +from common.test.acceptance.pages.lms import BASE_URL + + +class CacheProgramsPage(PageObject): + """ + Visit this page to call the cache_programs management command. + + This page makes a GET request to a view which is only meant to be enabled in + testing contexts where the LMS can only be reached over HTTP. Stub the + discovery service before visiting this page. + """ + url = BASE_URL + '/catalog/management/cache_programs/' + + def is_browser_on_page(self): + body = self.q(css='body').text[0] + match = re.search(r'programs cached', body, flags=re.IGNORECASE) + + return True if match else False diff --git a/common/test/acceptance/tests/lms/test_programs.py b/common/test/acceptance/tests/lms/test_programs.py index 1780d715b35..5eada0135ed 100644 --- a/common/test/acceptance/tests/lms/test_programs.py +++ b/common/test/acceptance/tests/lms/test_programs.py @@ -6,6 +6,7 @@ from common.test.acceptance.fixtures.programs import ProgramsConfigMixin from common.test.acceptance.fixtures.course import CourseFixture from common.test.acceptance.tests.helpers import UniqueCourseTest from common.test.acceptance.pages.lms.auto_auth import AutoAuthPage +from common.test.acceptance.pages.lms.catalog import CacheProgramsPage from common.test.acceptance.pages.lms.programs import ProgramListingPage, ProgramDetailsPage from openedx.core.djangoapps.catalog.tests.factories import ( ProgramFactory, CourseFactory, CourseRunFactory @@ -39,10 +40,19 @@ class ProgramPageBase(ProgramsConfigMixin, CatalogIntegrationMixin, UniqueCourse return ProgramFactory(courses=[course]) - def stub_catalog_api(self, data=None): - """Stub out the catalog API's program and course run endpoints.""" + def stub_catalog_api(self, programs): + """ + Stub the discovery service's program list and detail API endpoints. + """ self.set_catalog_integration(is_enabled=True, service_username=self.username) - CatalogFixture().install_programs(data or self.programs) + CatalogFixture().install_programs(programs) + + def cache_programs(self): + """ + Populate the LMS' cache of program data. + """ + cache_programs_page = CacheProgramsPage(self.browser) + cache_programs_page.visit() class ProgramListingPageTest(ProgramPageBase): @@ -55,7 +65,8 @@ class ProgramListingPageTest(ProgramPageBase): def test_no_enrollments(self): """Verify that no cards appear when the user has no enrollments.""" self.auth(enroll=False) - self.stub_catalog_api() + self.stub_catalog_api(self.programs) + self.cache_programs() self.listing_page.visit() @@ -68,7 +79,8 @@ class ProgramListingPageTest(ProgramPageBase): but none are included in an active program. """ self.auth() - self.stub_catalog_api() + self.stub_catalog_api(self.programs) + self.cache_programs() self.listing_page.visit() @@ -83,7 +95,8 @@ class ProgramListingPageTest(ProgramPageBase): self.auth() program = self.create_program() - self.stub_catalog_api(data=[program]) + self.stub_catalog_api(programs=[program]) + self.cache_programs() self.listing_page.visit() @@ -104,7 +117,8 @@ class ProgramListingPageA11yTest(ProgramPageBase): def test_empty_a11y(self): """Test a11y of the page's empty state.""" self.auth(enroll=False) - self.stub_catalog_api(data=[self.program]) + self.stub_catalog_api(programs=[self.program]) + self.cache_programs() self.listing_page.visit() @@ -115,7 +129,8 @@ class ProgramListingPageA11yTest(ProgramPageBase): def test_cards_a11y(self): """Test a11y when program cards are present.""" self.auth() - self.stub_catalog_api(data=[self.program]) + self.stub_catalog_api(programs=[self.program]) + self.cache_programs() self.listing_page.visit() @@ -138,7 +153,8 @@ class ProgramDetailsPageA11yTest(ProgramPageBase): def test_a11y(self): """Test the page's a11y compliance.""" self.auth() - self.stub_catalog_api(data=self.program) + self.stub_catalog_api(programs=[self.program]) + self.cache_programs() self.details_page.visit() diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 0b11eae5499..38451845634 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -50,12 +50,10 @@ from lms.djangoapps.grades.config.waffle import waffle as grades_waffle, ASSUME_ from milestones.tests.utils import MilestonesTestCaseMixin from openedx.core.djangoapps.catalog.tests.factories import CourseFactory as CatalogCourseFactory from openedx.core.djangoapps.catalog.tests.factories import ProgramFactory, CourseRunFactory -from openedx.core.djangoapps.catalog.tests.mixins import CatalogIntegrationMixin from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.crawlers.models import CrawlersConfig from openedx.core.djangoapps.credit.api import set_credit_requirements from openedx.core.djangoapps.credit.models import CreditCourse, CreditProvider -from openedx.core.djangoapps.programs.tests.mixins import ProgramsApiConfigMixin from openedx.core.djangoapps.self_paced.models import SelfPacedConfiguration from openedx.core.lib.gating import api as gating_api from openedx.features.enterprise_support.tests.mixins.enterprise import EnterpriseTestConsentRequired @@ -966,8 +964,10 @@ class ViewsTestCase(ModuleStoreTestCase): @attr(shard=2) -@patch('openedx.core.djangoapps.catalog.utils.get_edx_api_data') -class TestProgramMarketingView(ProgramsApiConfigMixin, CatalogIntegrationMixin, SharedModuleStoreTestCase): +# Patching 'lms.djangoapps.courseware.views.views.get_programs' would be ideal, +# but for some unknown reason that patch doesn't seem to be applied. +@patch('openedx.core.djangoapps.catalog.utils.cache') +class TestProgramMarketingView(SharedModuleStoreTestCase): """Unit tests for the program marketing page.""" program_uuid = str(uuid4()) url = reverse('program_marketing_view', kwargs={'program_uuid': program_uuid}) @@ -982,25 +982,20 @@ class TestProgramMarketingView(ProgramsApiConfigMixin, CatalogIntegrationMixin, cls.data = ProgramFactory(uuid=cls.program_uuid, courses=[course]) - def test_404_if_no_data(self, _mock_get_edx_api_data): + def test_404_if_no_data(self, mock_cache): """ Verify that the page 404s if no program data is found. """ - self.create_programs_config() + mock_cache.get.return_value = None response = self.client.get(self.url) self.assertEqual(response.status_code, 404) - def test_200(self, mock_get_edx_api_data): + def test_200(self, mock_cache): """ Verify the view returns a 200. """ - self.create_programs_config() - - catalog_integration = self.create_catalog_integration() - UserFactory(username=catalog_integration.service_username) - - mock_get_edx_api_data.return_value = self.data + mock_cache.get.return_value = self.data response = self.client.get(self.url) self.assertEqual(response.status_code, 200) diff --git a/lms/djangoapps/learner_dashboard/tests/test_programs.py b/lms/djangoapps/learner_dashboard/tests/test_programs.py index da54c65e18d..23d03013622 100644 --- a/lms/djangoapps/learner_dashboard/tests/test_programs.py +++ b/lms/djangoapps/learner_dashboard/tests/test_programs.py @@ -228,7 +228,7 @@ class TestProgramListing(ProgramsApiConfigMixin, CredentialsApiConfigMixin, Shar @skip_unless_lms -@mock.patch(CATALOG_UTILS_MODULE + '.get_edx_api_data') +@mock.patch(PROGRAMS_UTILS_MODULE + '.get_programs') class TestProgramDetails(ProgramsApiConfigMixin, CatalogIntegrationMixin, SharedModuleStoreTestCase): """Unit tests for the program details page.""" program_uuid = str(uuid4()) @@ -266,7 +266,7 @@ class TestProgramDetails(ProgramsApiConfigMixin, CatalogIntegrationMixin, Shared any(soup.find_all('a', class_='tab-nav-link', href=reverse('program_listing_view'))) ) - def test_login_required(self, mock_get_edx_api_data): + def test_login_required(self, mock_get_programs): """ Verify that login is required to access the page. """ @@ -275,7 +275,7 @@ class TestProgramDetails(ProgramsApiConfigMixin, CatalogIntegrationMixin, Shared catalog_integration = self.create_catalog_integration() UserFactory(username=catalog_integration.service_username) - mock_get_edx_api_data.return_value = self.data + mock_get_programs.return_value = self.data self.client.logout() @@ -290,7 +290,7 @@ class TestProgramDetails(ProgramsApiConfigMixin, CatalogIntegrationMixin, Shared response = self.client.get(self.url) self.assert_program_data_present(response) - def test_404_if_disabled(self, _mock_get_edx_api_data): + def test_404_if_disabled(self, _mock_get_programs): """ Verify that the page 404s if disabled. """ @@ -299,9 +299,11 @@ class TestProgramDetails(ProgramsApiConfigMixin, CatalogIntegrationMixin, Shared response = self.client.get(self.url) self.assertEqual(response.status_code, 404) - def test_404_if_no_data(self, _mock_get_edx_api_data): + def test_404_if_no_data(self, mock_get_programs): """Verify that the page 404s if no program data is found.""" self.create_programs_config() + mock_get_programs.return_value = None + response = self.client.get(self.url) self.assertEqual(response.status_code, 404) diff --git a/lms/envs/bok_choy.env.json b/lms/envs/bok_choy.env.json index 18dd05541d8..924ed69f714 100644 --- a/lms/envs/bok_choy.env.json +++ b/lms/envs/bok_choy.env.json @@ -84,6 +84,7 @@ "ALLOW_AUTOMATED_SIGNUPS": true, "AUTOMATIC_AUTH_FOR_TESTING": true, "MODE_CREATION_FOR_TESTING": true, + "EXPOSE_CACHE_PROGRAMS_ENDPOINT": true, "AUTOMATIC_VERIFY_STUDENT_IDENTITY_FOR_TESTING": true, "ENABLE_COURSE_DISCOVERY": true, "ENABLE_SPECIAL_EXAMS": true, diff --git a/lms/envs/common.py b/lms/envs/common.py index c7111971905..973250c0157 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -290,6 +290,10 @@ FEATURES = { # For easily adding modes to courses during acceptance testing 'MODE_CREATION_FOR_TESTING': False, + # For caching programs in contexts where the LMS can only + # be reached over HTTP. + 'EXPOSE_CACHE_PROGRAMS_ENDPOINT': False, + # Courseware search feature 'ENABLE_COURSEWARE_SEARCH': False, diff --git a/lms/urls.py b/lms/urls.py index a9e1d7dcad9..ca99a2740b3 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -86,6 +86,8 @@ urlpatterns = ( url(r'^rss_proxy/', include('rss_proxy.urls', namespace='rss_proxy')), url(r'^api/organizations/', include('organizations.urls', namespace='organizations')), + url(r'^catalog/', include('openedx.core.djangoapps.catalog.urls', namespace='catalog')), + # Update session view url( r'^lang_pref/session_language', diff --git a/openedx/core/djangoapps/catalog/tests/test_utils.py b/openedx/core/djangoapps/catalog/tests/test_utils.py index 22499f47b6c..be9a28a2eab 100644 --- a/openedx/core/djangoapps/catalog/tests/test_utils.py +++ b/openedx/core/djangoapps/catalog/tests/test_utils.py @@ -7,7 +7,6 @@ import mock from django.contrib.auth import get_user_model from django.core.cache import cache from django.test import TestCase -from waffle.models import Switch from openedx.core.djangoapps.catalog.cache import PROGRAM_CACHE_KEY_TPL, PROGRAM_UUIDS_CACHE_KEY from openedx.core.djangoapps.catalog.models import CatalogIntegration @@ -29,14 +28,9 @@ User = get_user_model() # pylint: disable=invalid-name @skip_unless_lms @mock.patch(UTILS_MODULE + '.logger.warning') -class TestGetCachedPrograms(CacheIsolationTestCase): +class TestGetPrograms(CacheIsolationTestCase): ENABLED_CACHES = ['default'] - def setUp(self): - super(TestGetCachedPrograms, self).setUp() - - Switch.objects.create(name='read_cached_programs', active=True) - def test_get_many(self, mock_warning): programs = ProgramFactory.create_batch(3) @@ -120,130 +114,6 @@ class TestGetCachedPrograms(CacheIsolationTestCase): self.assertFalse(mock_warning.called) -@skip_unless_lms -@mock.patch(UTILS_MODULE + '.get_edx_api_data') -class TestGetPrograms(CatalogIntegrationMixin, TestCase): - """Tests covering retrieval of programs from the catalog service.""" - def setUp(self): - super(TestGetPrograms, self).setUp() - - self.uuid = str(uuid.uuid4()) - self.types = ['Foo', 'Bar', 'FooBar'] - self.catalog_integration = self.create_catalog_integration(cache_ttl=1) - - UserFactory(username=self.catalog_integration.service_username) - - def assert_contract(self, call_args, program_uuid=None, types=None): - """Verify that API data retrieval utility is used correctly.""" - args, kwargs = call_args - - for arg in (self.catalog_integration, 'programs'): - self.assertIn(arg, args) - - self.assertEqual(kwargs['resource_id'], program_uuid) - - types_param = ','.join(types) if types and isinstance(types, list) else None - cache_key = '{base}.programs{types}'.format( - base=self.catalog_integration.CACHE_KEY, - types='.' + types_param if types_param else '' - ) - self.assertEqual( - kwargs['cache_key'], - cache_key if self.catalog_integration.is_cache_enabled else None - ) - - self.assertEqual(kwargs['api']._store['base_url'], self.catalog_integration.internal_api_url) # pylint: disable=protected-access - - querystring = { - 'exclude_utm': 1, - 'status': ('active', 'retired',), - } - if program_uuid: - querystring['use_full_course_serializer'] = 1 - if types: - querystring['types'] = types_param - - self.assertEqual(kwargs['querystring'], querystring) - - return args, kwargs - - def test_get_programs(self, mock_get_edx_api_data): - programs = ProgramFactory.create_batch(3) - mock_get_edx_api_data.return_value = programs - - data = get_programs() - - self.assert_contract(mock_get_edx_api_data.call_args) - self.assertEqual(data, programs) - - def test_get_one_program(self, mock_get_edx_api_data): - program = ProgramFactory() - mock_get_edx_api_data.return_value = program - - data = get_programs(uuid=self.uuid) - - self.assert_contract(mock_get_edx_api_data.call_args, program_uuid=self.uuid) - self.assertEqual(data, program) - - def test_programs_unavailable(self, mock_get_edx_api_data): - mock_get_edx_api_data.return_value = [] - - data = get_programs() - - self.assert_contract(mock_get_edx_api_data.call_args) - self.assertEqual(data, []) - - def test_cache_disabled(self, mock_get_edx_api_data): - self.catalog_integration = self.create_catalog_integration(cache_ttl=0) - get_programs() - self.assert_contract(mock_get_edx_api_data.call_args) - - def test_config_missing(self, _mock_get_edx_api_data): - """ - Verify that no errors occur if this method is called when catalog config - is missing. - """ - CatalogIntegration.objects.all().delete() - - data = get_programs() - self.assertEqual(data, []) - - def test_service_user_missing(self, _mock_get_edx_api_data): - """ - Verify that no errors occur if this method is called when the catalog - service user is missing. - """ - # Note: Deleting the service user would be ideal, but causes mysterious - # errors on Jenkins. - self.create_catalog_integration(service_username='nonexistent-user') - - data = get_programs() - self.assertEqual(data, []) - - @mock.patch(UTILS_MODULE + '.get_programs') - @mock.patch(UTILS_MODULE + '.get_program_types') - def test_get_programs_with_type(self, mock_get_program_types, mock_get_programs, _mock_get_edx_api_data): - """Verify get_programs_with_type returns the expected list of programs.""" - programs_with_program_type = [] - programs = ProgramFactory.create_batch(2) - program_types = [] - - for program in programs: - program_type = ProgramTypeFactory(name=program['type']) - program_types.append(program_type) - - program_with_type = copy.deepcopy(program) - program_with_type['type'] = program_type - programs_with_program_type.append(program_with_type) - - mock_get_programs.return_value = programs - mock_get_program_types.return_value = program_types - - actual = get_programs_with_type() - - self.assertEqual(actual, programs_with_program_type) - - @skip_unless_lms @mock.patch(UTILS_MODULE + '.get_edx_api_data') class TestGetProgramTypes(CatalogIntegrationMixin, TestCase): diff --git a/openedx/core/djangoapps/catalog/urls.py b/openedx/core/djangoapps/catalog/urls.py new file mode 100644 index 00000000000..0961e7194d6 --- /dev/null +++ b/openedx/core/djangoapps/catalog/urls.py @@ -0,0 +1,8 @@ +from django.conf.urls import url + +from . import views + + +urlpatterns = [ + url(r'^management/cache_programs/$', views.cache_programs, name='cache_programs'), +] diff --git a/openedx/core/djangoapps/catalog/utils.py b/openedx/core/djangoapps/catalog/utils.py index eb4376cb4be..babbd1da15e 100644 --- a/openedx/core/djangoapps/catalog/utils.py +++ b/openedx/core/djangoapps/catalog/utils.py @@ -2,7 +2,6 @@ import copy import logging -import waffle from django.conf import settings from django.contrib.auth import get_user_model from django.core.cache import cache @@ -39,62 +38,27 @@ def get_programs(uuid=None): list of dict, representing programs. dict, if a specific program is requested. """ - if waffle.switch_is_active('read_cached_programs'): - missing_details_msg_tpl = 'Details for program {uuid} are not cached.' + missing_details_msg_tpl = 'Details for program {uuid} are not cached.' - if uuid: - program = cache.get(PROGRAM_CACHE_KEY_TPL.format(uuid=uuid)) - if not program: - logger.warning(missing_details_msg_tpl.format(uuid=uuid)) + if uuid: + program = cache.get(PROGRAM_CACHE_KEY_TPL.format(uuid=uuid)) + if not program: + logger.warning(missing_details_msg_tpl.format(uuid=uuid)) - return program + return program - uuids = cache.get(PROGRAM_UUIDS_CACHE_KEY, []) - if not uuids: - logger.warning('Program UUIDs are not cached.') + uuids = cache.get(PROGRAM_UUIDS_CACHE_KEY, []) + if not uuids: + logger.warning('Program UUIDs are not cached.') - programs = cache.get_many([PROGRAM_CACHE_KEY_TPL.format(uuid=uuid) for uuid in uuids]) - programs = list(programs.values()) + programs = cache.get_many([PROGRAM_CACHE_KEY_TPL.format(uuid=uuid) for uuid in uuids]) + programs = list(programs.values()) - missing_uuids = set(uuids) - set(program['uuid'] for program in programs) - for uuid in missing_uuids: - logger.warning(missing_details_msg_tpl.format(uuid=uuid)) + missing_uuids = set(uuids) - set(program['uuid'] for program in programs) + for uuid in missing_uuids: + logger.warning(missing_details_msg_tpl.format(uuid=uuid)) - return programs - else: - # Old implementation which may request programs in-process. To be removed - # as part of LEARNER-382. - catalog_integration = CatalogIntegration.current() - if catalog_integration.enabled: - try: - user = User.objects.get(username=catalog_integration.service_username) - except User.DoesNotExist: - return [] - - api = create_catalog_api_client(user, catalog_integration) - - cache_key = '{base}.programs'.format( - base=catalog_integration.CACHE_KEY - ) - - querystring = { - 'exclude_utm': 1, - 'status': ('active', 'retired',), - } - - if uuid: - querystring['use_full_course_serializer'] = 1 - - return get_edx_api_data( - catalog_integration, - 'programs', - api=api, - resource_id=uuid, - cache_key=cache_key if catalog_integration.is_cache_enabled else None, - querystring=querystring, - ) - else: - return [] + return programs def get_program_types(name=None): diff --git a/openedx/core/djangoapps/catalog/views.py b/openedx/core/djangoapps/catalog/views.py new file mode 100644 index 00000000000..a604127e9f9 --- /dev/null +++ b/openedx/core/djangoapps/catalog/views.py @@ -0,0 +1,21 @@ +from django.conf import settings +from django.core.management import call_command +from django.http import Http404, HttpResponse +from django.views.decorators.http import require_GET + + +@require_GET +def cache_programs(request): + """ + Call the cache_programs management command. + + This view is intended for use in testing contexts where the LMS can only be + reached over HTTP (e.g., Selenium-based browser tests). The discovery service + API the management command attempts to call should be stubbed out first. + """ + if settings.FEATURES.get('EXPOSE_CACHE_PROGRAMS_ENDPOINT'): + call_command('cache_programs') + + return HttpResponse('Programs cached.') + + raise Http404 -- GitLab