From 11cd2ef557aa1a1677eb9755e190c272063ec38f Mon Sep 17 00:00:00 2001 From: Cole Rogers Date: Wed, 8 Aug 2018 13:39:13 -0400 Subject: [PATCH] Update pathway caching to work with multiple pages returned from endpoint --- common/test/acceptance/fixtures/catalog.py | 2 +- .../management/commands/cache_programs.py | 9 ++- .../commands/tests/test_cache_programs.py | 77 +++++++++++++++++-- 3 files changed, 80 insertions(+), 8 deletions(-) diff --git a/common/test/acceptance/fixtures/catalog.py b/common/test/acceptance/fixtures/catalog.py index 8ee94c00b5..cb38724516 100644 --- a/common/test/acceptance/fixtures/catalog.py +++ b/common/test/acceptance/fixtures/catalog.py @@ -49,7 +49,7 @@ class CatalogFixture(object): """ requests.put( '{}/set_config'.format(CATALOG_STUB_URL), - data={'catalog.credit_pathways': json.dumps({'results': credit_pathways})} + data={'catalog.credit_pathways': json.dumps({'results': credit_pathways, 'next': None})} ) def install_program_types(self, program_types): diff --git a/openedx/core/djangoapps/catalog/management/commands/cache_programs.py b/openedx/core/djangoapps/catalog/management/commands/cache_programs.py index 586206c991..39a9d27f97 100644 --- a/openedx/core/djangoapps/catalog/management/commands/cache_programs.py +++ b/openedx/core/djangoapps/catalog/management/commands/cache_programs.py @@ -138,11 +138,16 @@ class Command(BaseCommand): """ Get all pathways for the current client """ - pathways = {} + pathways = [] failure = False logger.info('Requesting credit pathways for {domain}.'.format(domain=site.domain)) try: - pathways = client.credit_pathways.get(exclude_utm=1)['results'] + next_page = 1 + while next_page: + new_pathways = client.credit_pathways.get(exclude_utm=1, page=next_page) + pathways.extend(new_pathways['results']) + next_page = next_page + 1 if new_pathways['next'] else None + except: # pylint: disable=bare-except logger.error('Failed to retrieve credit pathways for site: {domain}.'.format(domain=site.domain)) failure = True diff --git a/openedx/core/djangoapps/catalog/management/commands/tests/test_cache_programs.py b/openedx/core/djangoapps/catalog/management/commands/tests/test_cache_programs.py index 7f46504a5b..fb769de5c5 100644 --- a/openedx/core/djangoapps/catalog/management/commands/tests/test_cache_programs.py +++ b/openedx/core/djangoapps/catalog/management/commands/tests/test_cache_programs.py @@ -86,7 +86,7 @@ class TestCachePrograms(CatalogIntegrationMixin, CacheIsolationTestCase, SiteMix content_type='application/json' ) - def mock_pathways(self): + def mock_pathways(self, pathways, page_number=1, final=True): """ Mock the data for discovery's credit pathways endpoint """ @@ -94,18 +94,31 @@ class TestCachePrograms(CatalogIntegrationMixin, CacheIsolationTestCase, SiteMix """ Mocks response """ + expected = { 'exclude_utm': ['1'], + 'page': [str(page_number)], } self.assertEqual(request.querystring, expected) - return (200, headers, json.dumps({'results': self.pathways})) + body = { + 'count': len(pathways), + 'next': None if final else 'more', # we don't actually parse this value + 'prev': None, + 'results': pathways + } + return (200, headers, json.dumps(body)) + + # NOTE: httpretty does not properly match against query strings here (using match_querystring arg) + # as such, it does not actually look at the query parameters (for page num), but returns items in a LIFO order. + # this means that for multiple pages, you must call this function starting from the last page. + # we do assert the page number query param above, however httpretty.register_uri( httpretty.GET, self.pathway_url, body=pathways_callback, - content_type='application/json' + content_type='application/json', ) def test_handle_programs(self): @@ -123,7 +136,7 @@ class TestCachePrograms(CatalogIntegrationMixin, CacheIsolationTestCase, SiteMix } self.mock_list() - self.mock_pathways() + self.mock_pathways(self.pathways) for uuid in self.uuids: program = programs[PROGRAM_CACHE_KEY_TPL.format(uuid=uuid)] @@ -170,7 +183,7 @@ class TestCachePrograms(CatalogIntegrationMixin, CacheIsolationTestCase, SiteMix } self.mock_list() - self.mock_pathways() + self.mock_pathways(self.pathways) for uuid in self.uuids: program = programs[PROGRAM_CACHE_KEY_TPL.format(uuid=uuid)] @@ -202,6 +215,60 @@ class TestCachePrograms(CatalogIntegrationMixin, CacheIsolationTestCase, SiteMix self.assertEqual(pathway, pathways[key]) + def test_pathways_multiple_pages(self): + """ + Verify that the command properly caches credit pathways when multiple pages are returned from its endpoint + """ + UserFactory(username=self.catalog_integration.service_username) + new_pathways = CreditPathwayFactory.create_batch(40) + for new_pathway in new_pathways: + new_pathway['programs'] = [] + pathways = self.pathways + new_pathways + + programs = { + PROGRAM_CACHE_KEY_TPL.format(uuid=program['uuid']): program for program in self.programs + } + + self.mock_list() + for uuid in self.uuids: + program = programs[PROGRAM_CACHE_KEY_TPL.format(uuid=uuid)] + self.mock_detail(uuid, program) + + # mock 3 pages of credit pathways, starting at the last + self.mock_pathways(pathways[40:], page_number=3, final=True) + self.mock_pathways(pathways[20:40], page_number=2, final=False) + self.mock_pathways(pathways[:20], page_number=1, final=False) + + call_command('cache_programs') + + pathways_dict = { + CREDIT_PATHWAY_CACHE_KEY_TPL.format(id=pathway['id']): pathway for pathway in pathways + } + pathway_keys = pathways_dict.keys() + + cached_pathway_keys = cache.get(SITE_CREDIT_PATHWAY_IDS_CACHE_KEY_TPL.format(domain=self.site_domain)) + self.assertEqual( + set(cached_pathway_keys), + set(pathway_keys) + ) + + cached_pathways = cache.get_many(pathway_keys) + self.assertEqual( + set(cached_pathways), + set(pathways_dict) + ) + + # We can't use a set comparison here because these values are dictionaries + # and aren't hashable. We've already verified that all pathways came out + # of the cache above, so all we need to do here is verify the accuracy of + # the data itself. + for key, pathway in cached_pathways.items(): + # cached pathways store just program uuids instead of the full programs, transform before comparing + pathways_dict[key]['program_uuids'] = [program['uuid'] for program in pathways_dict[key]['programs']] + del pathways_dict[key]['programs'] + + self.assertEqual(pathway, pathways_dict[key]) + def test_handle_missing_service_user(self): """ Verify that the command raises an exception when run without a service