diff --git a/common/test/acceptance/fixtures/catalog.py b/common/test/acceptance/fixtures/catalog.py index 32a6b8e05d6e7749a4ceaf18e66e341eac50d226..7368d2a9622045d090e369c99ca657b8355f1323 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 0000000000000000000000000000000000000000..ad1335266f0a89b7b3f0e903e9c3e5b03fe35285 --- /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 1780d715b354fdacdd493fd116f7c0a05928408b..5eada0135ed779f740856df7af23650ecb2a3a2c 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 0b11eae54991c9812721422ace376b94973e7da5..384518456347e60e18c298a5c20174015dafddc6 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 da54c65e18d38a1262623e828549bbfee2cd45e2..23d03013622422fafd34df5fc7312942bc16b538 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 18dd05541d8e4a3eb4218527ef9c0822c5d78260..924ed69f714f023113c3e0763fa007f7ce834cc6 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 c711197190598bc988c5c05c3d9636c8a9ca57b2..973250c01571dc4723d60cd140b068e1c6f57054 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 a9e1d7dcad94acbe42c37e0e3f3de42739228880..ca99a2740b3e4421386789007708ba2328f9df1c 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 22499f47b6cfe6be3c55354d63a57b34ceb14961..be9a28a2eab2059ff80a1c6fe2e8f69cd6fec7b3 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 0000000000000000000000000000000000000000..0961e7194d6b7b263691044d750fcdc98a553ce1 --- /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 eb4376cb4be8d01fb7848e5a36b82ce1c0171716..babbd1da15eeabdc865d380943ffb62c2bcf78a3 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 0000000000000000000000000000000000000000..a604127e9f9af428ebc7a36712524b77bef0c204 --- /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