diff --git a/openedx/core/djangoapps/catalog/tests/test_utils.py b/openedx/core/djangoapps/catalog/tests/test_utils.py index 791d2aae71..22499f47b6 100644 --- a/openedx/core/djangoapps/catalog/tests/test_utils.py +++ b/openedx/core/djangoapps/catalog/tests/test_utils.py @@ -5,9 +5,11 @@ import uuid 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 from openedx.core.djangoapps.catalog.tests.factories import CourseRunFactory, ProgramFactory, ProgramTypeFactory from openedx.core.djangoapps.catalog.tests.mixins import CatalogIntegrationMixin @@ -17,13 +19,107 @@ from openedx.core.djangoapps.catalog.utils import ( get_programs_with_type, get_course_runs, ) -from openedx.core.djangolib.testing.utils import skip_unless_lms +from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms from student.tests.factories import UserFactory + UTILS_MODULE = 'openedx.core.djangoapps.catalog.utils' User = get_user_model() # pylint: disable=invalid-name +@skip_unless_lms +@mock.patch(UTILS_MODULE + '.logger.warning') +class TestGetCachedPrograms(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) + + # Cache details for 2 of 3 programs. + partial_programs = { + PROGRAM_CACHE_KEY_TPL.format(uuid=program['uuid']): program for program in programs[:2] + } + cache.set_many(partial_programs, None) + + # When called before UUIDs are cached, the function should return an empty + # list and log a warning. + self.assertEqual(get_programs(), []) + mock_warning.assert_called_once_with('Program UUIDs are not cached.') + mock_warning.reset_mock() + + # Cache UUIDs for all 3 programs. + cache.set( + PROGRAM_UUIDS_CACHE_KEY, + [program['uuid'] for program in programs], + None + ) + + actual_programs = get_programs() + + # The 2 cached programs should be returned while a warning should be logged + # for the missing one. + self.assertEqual( + set(program['uuid'] for program in actual_programs), + set(program['uuid'] for program in partial_programs.values()) + ) + mock_warning.assert_called_with( + 'Details for program {uuid} are not cached.'.format(uuid=programs[2]['uuid']) + ) + mock_warning.reset_mock() + + # We can't use a set comparison here because these values are dictionaries + # and aren't hashable. We've already verified that all programs came out + # of the cache above, so all we need to do here is verify the accuracy of + # the data itself. + for program in actual_programs: + key = PROGRAM_CACHE_KEY_TPL.format(uuid=program['uuid']) + self.assertEqual(program, partial_programs[key]) + + # Cache details for all 3 programs. + all_programs = { + PROGRAM_CACHE_KEY_TPL.format(uuid=program['uuid']): program for program in programs + } + cache.set_many(all_programs, None) + + actual_programs = get_programs() + + # All 3 programs should be returned. + self.assertEqual( + set(program['uuid'] for program in actual_programs), + set(program['uuid'] for program in all_programs.values()) + ) + self.assertFalse(mock_warning.called) + + for program in actual_programs: + key = PROGRAM_CACHE_KEY_TPL.format(uuid=program['uuid']) + self.assertEqual(program, all_programs[key]) + + def test_get_one(self, mock_warning): + expected_program = ProgramFactory() + expected_uuid = expected_program['uuid'] + + self.assertEqual(get_programs(uuid=expected_uuid), None) + mock_warning.assert_called_once_with( + 'Details for program {uuid} are not cached.'.format(uuid=expected_uuid) + ) + mock_warning.reset_mock() + + cache.set( + PROGRAM_CACHE_KEY_TPL.format(uuid=expected_uuid), + expected_program, + None + ) + + actual_program = get_programs(uuid=expected_uuid) + self.assertEqual(actual_program, expected_program) + self.assertFalse(mock_warning.called) + + @skip_unless_lms @mock.patch(UTILS_MODULE + '.get_edx_api_data') class TestGetPrograms(CatalogIntegrationMixin, TestCase): @@ -89,15 +185,6 @@ class TestGetPrograms(CatalogIntegrationMixin, TestCase): self.assert_contract(mock_get_edx_api_data.call_args, program_uuid=self.uuid) self.assertEqual(data, program) - def test_get_programs_by_types(self, mock_get_edx_api_data): - programs = ProgramFactory.create_batch(2) - mock_get_edx_api_data.return_value = programs - - data = get_programs(types=self.types) - - self.assert_contract(mock_get_edx_api_data.call_args, types=self.types) - self.assertEqual(data, programs) - def test_programs_unavailable(self, mock_get_edx_api_data): mock_get_edx_api_data.return_value = [] @@ -222,7 +309,7 @@ class TestGetCourseRuns(CatalogIntegrationMixin, TestCase): self.assertFalse(mock_get_edx_api_data.called) self.assertEqual(data, []) - @mock.patch(UTILS_MODULE + '.log.error') + @mock.patch(UTILS_MODULE + '.logger.error') def test_service_user_missing(self, mock_log_error, mock_get_edx_api_data): """ Verify that no errors occur when the catalog service user is missing. diff --git a/openedx/core/djangoapps/catalog/utils.py b/openedx/core/djangoapps/catalog/utils.py index dc7d6c94aa..37b7dbe9c3 100644 --- a/openedx/core/djangoapps/catalog/utils.py +++ b/openedx/core/djangoapps/catalog/utils.py @@ -2,16 +2,19 @@ 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 from edx_rest_api_client.client import EdxRestApiClient +from openedx.core.djangoapps.catalog.cache import PROGRAM_CACHE_KEY_TPL, PROGRAM_UUIDS_CACHE_KEY from openedx.core.djangoapps.catalog.models import CatalogIntegration from openedx.core.lib.edx_api_utils import get_edx_api_data from openedx.core.lib.token_utils import JwtBuilder -log = logging.getLogger(__name__) +logger = logging.getLogger(__name__) User = get_user_model() # pylint: disable=invalid-name @@ -24,55 +27,74 @@ def create_catalog_api_client(user, catalog_integration): return EdxRestApiClient(catalog_integration.internal_api_url, jwt=jwt) -def get_programs(uuid=None, types=None): # pylint: disable=redefined-builtin - """Retrieve marketable programs from the catalog service. +def get_programs(uuid=None): + """Read programs from the cache. + + The cache is populated by a management command, cache_programs. Keyword Arguments: - uuid (string): UUID identifying a specific program. - types (list of string): List of program type names used to filter programs by type - (e.g., ["MicroMasters"] will only return MicroMasters programs, - ["MicroMasters", "XSeries"] will return MicroMasters and XSeries programs). + uuid (string): UUID identifying a specific program to read from the cache. Returns: list of dict, representing programs. dict, if a specific program is requested. """ - 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) - types_param = ','.join(types) if types else None - - cache_key = '{base}.programs{types}'.format( - base=catalog_integration.CACHE_KEY, - types='.' + types_param if types_param else '' - ) - - querystring = { - 'exclude_utm': 1, - 'status': ('active', 'retired',), - } + if waffle.switch_is_active('read_cached_programs'): + missing_details_msg_tpl = 'Details for program {uuid} are not cached.' if uuid: - querystring['use_full_course_serializer'] = 1 + program = cache.get(PROGRAM_CACHE_KEY_TPL.format(uuid=uuid)) + if not program: + logger.warning(missing_details_msg_tpl.format(uuid=uuid)) - if types_param: - querystring['types'] = types_param + return program - 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, - ) + 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()) + + 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: - return [] + # 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 [] def get_program_types(name=None): @@ -121,11 +143,19 @@ def get_programs_with_type(types=None): list of dict, representing the active programs. """ programs_with_type = [] - programs = get_programs(types=types) + programs = get_programs() if programs: program_types = {program_type['name']: program_type for program_type in get_program_types()} for program in programs: + # This limited type filtering is sufficient for current needs and + # helps us avoid additional complexity when caching program data. + # However, if the need for additional filtering of programs should + # arise, consider using the cache_programs management command to + # cache the filtered lists of UUIDs. + if types and program['type'] not in types: + continue + # deepcopy the program dict here so we are not adding # the type to the cached object program_with_type = copy.deepcopy(program) @@ -148,7 +178,7 @@ def get_course_runs(): try: user = User.objects.get(username=catalog_integration.service_username) except User.DoesNotExist: - log.error( + logger.error( 'Catalog service user with username [%s] does not exist. Course runs will not be retrieved.', catalog_integration.service_username, )