diff --git a/openedx/core/djangoapps/catalog/cache.py b/openedx/core/djangoapps/catalog/cache.py index bb62b65308..25fa7a2515 100644 --- a/openedx/core/djangoapps/catalog/cache.py +++ b/openedx/core/djangoapps/catalog/cache.py @@ -10,9 +10,12 @@ PATHWAY_CACHE_KEY_TPL = 'pathway-{id}' # Cache key used to locate an item containing a list of all credit pathway ids for a site. SITE_PATHWAY_IDS_CACHE_KEY_TPL = 'pathway-ids-{domain}' -# Template used to create cache keys for individual courses to program uuids. +# Template used to create cache keys for individual course runs to program uuids. COURSE_PROGRAMS_CACHE_KEY_TPL = 'course-programs-{course_run_id}' +# Template used to create cache keys for individual Courses to program uuids. +CATALOG_COURSE_PROGRAMS_CACHE_KEY_TPL = 'catalog-course-programs-{course_uuid}' + # Site-aware cache key template used to locate an item containing # a list of all program UUIDs with a certain program type (the Site is required # because program_type values are likely to be shared between different sites diff --git a/openedx/core/djangoapps/catalog/management/commands/cache_programs.py b/openedx/core/djangoapps/catalog/management/commands/cache_programs.py index 8e099e7c1e..b8c78b371f 100644 --- a/openedx/core/djangoapps/catalog/management/commands/cache_programs.py +++ b/openedx/core/djangoapps/catalog/management/commands/cache_programs.py @@ -13,6 +13,7 @@ from six import text_type from openedx.core.djangoapps.catalog.cache import ( COURSE_PROGRAMS_CACHE_KEY_TPL, + CATALOG_COURSE_PROGRAMS_CACHE_KEY_TPL, PATHWAY_CACHE_KEY_TPL, PROGRAM_CACHE_KEY_TPL, PROGRAMS_BY_ORGANIZATION_CACHE_KEY_TPL, @@ -23,6 +24,7 @@ from openedx.core.djangoapps.catalog.cache import ( from openedx.core.djangoapps.catalog.models import CatalogIntegration from openedx.core.djangoapps.catalog.utils import ( course_run_keys_for_program, + course_uuids_for_program, create_catalog_api_client, normalize_program_type ) @@ -60,6 +62,7 @@ class Command(BaseCommand): programs = {} pathways = {} courses = {} + catalog_courses = {} programs_by_type = {} organizations = {} for site in Site.objects.all(): @@ -88,6 +91,7 @@ class Command(BaseCommand): programs.update(new_programs) pathways.update(new_pathways) courses.update(self.get_courses(new_programs)) + catalog_courses.update(self.get_catalog_courses(new_programs)) programs_by_type.update(self.get_programs_by_type(site, new_programs)) organizations.update(self.get_programs_by_organization(new_programs)) @@ -113,6 +117,9 @@ class Command(BaseCommand): logger.info(u'Caching programs uuids for {} courses.'.format(len(courses))) cache.set_many(courses, None) + logger.info(u'Caching programs uuids for {} catalog courses.'.format(len(catalog_courses))) + cache.set_many(catalog_courses, None) + logger.info(text_type('Caching program UUIDs by {} program types.'.format(len(programs_by_type)))) cache.set_many(programs_by_type, None) @@ -218,7 +225,7 @@ class Command(BaseCommand): def get_courses(self, programs): """ - Get all courses for the programs. + Get all course runs for programs. TODO: when course discovery can handle it, use that instead. That will allow us to put all course runs in the cache not just the course runs in a program. Therefore, a cache miss would be different from a @@ -232,6 +239,18 @@ class Command(BaseCommand): course_runs[course_run_cache_key].append(program['uuid']) return course_runs + def get_catalog_courses(self, programs): + """ + Get all catalog courses for the programs. + """ + courses = defaultdict(list) + + for program in programs.values(): + for course_uuid in course_uuids_for_program(program): + course_cache_key = CATALOG_COURSE_PROGRAMS_CACHE_KEY_TPL.format(course_uuid=course_uuid) + courses[course_cache_key].append(program['uuid']) + return courses + def get_programs_by_type(self, site, programs): """ Returns a dictionary mapping site-aware cache keys corresponding to program types diff --git a/openedx/core/djangoapps/catalog/tests/test_utils.py b/openedx/core/djangoapps/catalog/tests/test_utils.py index ce65058301..9db239cea7 100644 --- a/openedx/core/djangoapps/catalog/tests/test_utils.py +++ b/openedx/core/djangoapps/catalog/tests/test_utils.py @@ -19,6 +19,7 @@ from course_modes.tests.factories import CourseModeFactory from entitlements.tests.factories import CourseEntitlementFactory from openedx.core.constants import COURSE_UNPUBLISHED from openedx.core.djangoapps.catalog.cache import ( + CATALOG_COURSE_PROGRAMS_CACHE_KEY_TPL, COURSE_PROGRAMS_CACHE_KEY_TPL, PATHWAY_CACHE_KEY_TPL, PROGRAM_CACHE_KEY_TPL, @@ -238,6 +239,28 @@ class TestGetPrograms(CacheIsolationTestCase): assert second_program in results assert not mock_warning.called + def test_get_from_catalog_course(self, mock_warning, _mock_info): + expected_program = ProgramFactory() + expected_catalog_course = expected_program['courses'][0] + + self.assertEqual(get_programs(catalog_course_uuid=expected_catalog_course['uuid']), []) + + cache.set( + CATALOG_COURSE_PROGRAMS_CACHE_KEY_TPL.format(course_uuid=expected_catalog_course['uuid']), + [expected_program['uuid']], + None + ) + cache.set( + PROGRAM_CACHE_KEY_TPL.format(uuid=expected_program['uuid']), + expected_program, + None + ) + + actual_program = get_programs(catalog_course_uuid=expected_catalog_course['uuid']) + + self.assertEqual(actual_program, [expected_program]) + self.assertFalse(mock_warning.called) + @skip_unless_lms @mock.patch(UTILS_MODULE + '.logger.info') diff --git a/openedx/core/djangoapps/catalog/utils.py b/openedx/core/djangoapps/catalog/utils.py index 90164b2f2e..60a1cf34c9 100644 --- a/openedx/core/djangoapps/catalog/utils.py +++ b/openedx/core/djangoapps/catalog/utils.py @@ -18,6 +18,7 @@ from entitlements.utils import is_course_run_entitlement_fulfillable from openedx.core.constants import COURSE_PUBLISHED from openedx.core.djangoapps.catalog.cache import ( COURSE_PROGRAMS_CACHE_KEY_TPL, + CATALOG_COURSE_PROGRAMS_CACHE_KEY_TPL, PROGRAMS_BY_ORGANIZATION_CACHE_KEY_TPL, PATHWAY_CACHE_KEY_TPL, PROGRAM_CACHE_KEY_TPL, @@ -87,7 +88,7 @@ def check_catalog_integration_and_get_user(error_message_field): # pylint: disable=redefined-outer-name -def get_programs(site=None, uuid=None, uuids=None, course=None, organization=None): +def get_programs(site=None, uuid=None, uuids=None, course=None, catalog_course_uuid=None, organization=None): """Read programs from the cache. The cache is populated by a management command, cache_programs. @@ -96,14 +97,15 @@ def get_programs(site=None, uuid=None, uuids=None, course=None, organization=Non site (Site): django.contrib.sites.models object to fetch programs of. uuid (string): UUID identifying a specific program to read from the cache. uuids (list of string): UUIDs identifying a specific programs to read from the cache. - course (string): course id identifying a specific course run to read from the cache. + course (string): course run id identifying a specific course run to read from the cache. + catalog_course_uuid (string): Catalog Course UUID organization (string): short name for specific organization to read from the cache. Returns: list of dict, representing programs. dict, if a specific program is requested. """ - if len([arg for arg in (site, uuid, uuids, course, organization) if arg is not None]) != 1: + if len([arg for arg in (site, uuid, uuids, course, catalog_course_uuid, organization) if arg is not None]) != 1: raise TypeError('get_programs takes exactly one argument') if uuid: @@ -118,6 +120,12 @@ def get_programs(site=None, uuid=None, uuids=None, course=None, organization=Non # Currently, the cache does not differentiate between a cache miss and a course # without programs. After this is changed, log any cache misses here. return [] + elif catalog_course_uuid: + uuids = cache.get(CATALOG_COURSE_PROGRAMS_CACHE_KEY_TPL.format(course_uuid=catalog_course_uuid)) + if not uuids: + # Currently, the cache does not differentiate between a cache miss and a course + # without programs. After this is changed, log any cache misses here. + return [] elif site: site_config = getattr(site, 'configuration', None) catalog_url = site_config.get_value('COURSE_CATALOG_API_URL') if site_config else None @@ -616,6 +624,21 @@ def course_run_keys_for_program(parent_program): return keys +def course_uuids_for_program(parent_program): + """ + All of the course uuids associated with this ``parent_program``, either + via its ``curriculum`` field (looking at both the curriculum's courses + and child programs), or through the many-to-many ``courses`` field on the program. + """ + uuids = set() + for program in [parent_program] + child_programs(parent_program): + curriculum = _primary_active_curriculum(program) + if curriculum: + uuids.update(_courses_from_container(curriculum)) + uuids.update(_courses_from_container(program)) + return uuids + + def child_programs(program): """ Given a program, recursively find all child programs related @@ -654,6 +677,18 @@ def _course_runs_from_container(container): ] +def _courses_from_container(container): + """ + Pluck nested courses out of a ``container`` dictionary, + which is either the ``curriculum`` field of a program, or + a program itself (since either may contain a ``courses`` list). + """ + return [ + course.get('uuid') + for course in container.get('courses', []) + ] + + def normalize_program_type(program_type): """ Function that normalizes a program type string for use in a cache key. """ return str(program_type).lower() diff --git a/openedx/core/djangoapps/external_user_ids/models.py b/openedx/core/djangoapps/external_user_ids/models.py index 0f1c5e4f0d..3bf0980c85 100644 --- a/openedx/core/djangoapps/external_user_ids/models.py +++ b/openedx/core/djangoapps/external_user_ids/models.py @@ -61,10 +61,6 @@ class ExternalId(TimeStampedModel): user=user, external_id_type__name=type_name ).exists(): - LOGGER.info('No external id for user id {user} with type of {type}'.format( - user=user.id, - type=type_name - )) return False return True diff --git a/openedx/core/djangoapps/external_user_ids/signals.py b/openedx/core/djangoapps/external_user_ids/signals.py index 8cca4ff051..45f7f711dc 100644 --- a/openedx/core/djangoapps/external_user_ids/signals.py +++ b/openedx/core/djangoapps/external_user_ids/signals.py @@ -13,6 +13,16 @@ from .models import ExternalId, ExternalIdType LOGGER = getLogger(__name__) +def _user_needs_external_id(instance, created): + return ( + created and + instance.user and + not ExternalId.user_has_external_id( + user=instance.user, + type_name=ExternalIdType.MICROBACHELORS_COACHING) + ) + + @receiver(post_save, sender='student.CourseEnrollment') def create_external_id_for_microbachelors_program( sender, instance, created, **kwargs # pylint: disable=unused-argument @@ -21,16 +31,30 @@ def create_external_id_for_microbachelors_program( Watches for post_save signal for creates on the CourseEnrollment table. Generate an External ID if the Enrollment is in a MicroBachelors Program """ - if ( - created and - instance.user and - not ExternalId.user_has_external_id( - user=instance.user, - type_name=ExternalIdType.MICROBACHELORS_COACHING) - ): + if _user_needs_external_id(instance, created): mb_programs = [ program for program in get_programs(course=instance.course_id) - if program.get('type_attrs', None) and program['type_attrs']['coaching_supported'] + if program.get('type_attrs', {}).get('coaching_supported') + ] + if mb_programs: + ExternalId.add_new_user_id( + user=instance.user, + type_name=ExternalIdType.MICROBACHELORS_COACHING + ) + + +@receiver(post_save, sender='entitlements.CourseEntitlement') +def create_external_id_for_microbachelors_program_entitlement( + sender, instance, created, **kwargs # pylint: disable=unused-argument +): + """ + Watches for post_save signal for creates on the CourseEntitlement table. + Generate an External ID if the Entitlement is in a MicroBachelors Program + """ + if _user_needs_external_id(instance, created): + mb_programs = [ + program for program in get_programs(catalog_course_uuid=instance.course_uuid) + if program.get('type_attrs', {}).get('coaching_supported') ] if mb_programs: ExternalId.add_new_user_id( diff --git a/openedx/core/djangoapps/external_user_ids/tests/test_signals.py b/openedx/core/djangoapps/external_user_ids/tests/test_signals.py index e25ddc6bed..6f082af511 100644 --- a/openedx/core/djangoapps/external_user_ids/tests/test_signals.py +++ b/openedx/core/djangoapps/external_user_ids/tests/test_signals.py @@ -7,12 +7,17 @@ from django.conf import settings from django.core.cache import cache from edx_django_utils.cache import RequestCache +from entitlements.models import CourseEntitlement from openedx.core.djangoapps.catalog.tests.factories import ( CourseFactory, ProgramFactory, ) from student.tests.factories import TEST_PASSWORD, UserFactory -from openedx.core.djangoapps.catalog.cache import PROGRAM_CACHE_KEY_TPL, COURSE_PROGRAMS_CACHE_KEY_TPL +from openedx.core.djangoapps.catalog.cache import ( + CATALOG_COURSE_PROGRAMS_CACHE_KEY_TPL, + COURSE_PROGRAMS_CACHE_KEY_TPL, + PROGRAM_CACHE_KEY_TPL, +) from student.models import CourseEnrollment from course_modes.models import CourseMode from openedx.core.djangolib.testing.utils import skip_unless_lms @@ -64,6 +69,12 @@ class MicrobachelorsExternalIDTest(ModuleStoreTestCase, CacheIsolationTestCase): program['type_attrs']['coaching_supported'] = True for course in program['courses']: + cache.set( + CATALOG_COURSE_PROGRAMS_CACHE_KEY_TPL.format(course_uuid=course['uuid']), + [program['uuid']], + None + ) + course_run = course['course_runs'][0]['key'] cache.set( COURSE_PROGRAMS_CACHE_KEY_TPL.format(course_run_id=course_run), @@ -127,3 +138,61 @@ class MicrobachelorsExternalIDTest(ModuleStoreTestCase, CacheIsolationTestCase): assert len(external_ids) == 1 assert external_ids[0].external_id_type.name == ExternalIdType.MICROBACHELORS_COACHING assert original_external_user_uuid == external_ids[0].external_user_id + + def test_entitlement_mb_create_external_id(self): + catalog_course = self.program['courses'][0] + + assert ExternalId.objects.filter( + user=self.user + ).count() == 0 + + entitlement = CourseEntitlement.objects.create( + course_uuid=catalog_course['uuid'], + mode=CourseMode.VERIFIED, + user=self.user, + order_number='TEST-12345' + ) + entitlement.save() + + external_id = ExternalId.objects.get( + user=self.user + ) + assert external_id is not None + assert external_id.external_id_type.name == ExternalIdType.MICROBACHELORS_COACHING + + def test_second_entitlement_mb_no_new_external_id(self): + catalog_course1 = self.program['courses'][0] + catalog_course2 = self.program['courses'][1] + + # Enroll user + entitlement = CourseEntitlement.objects.create( + course_uuid=catalog_course1['uuid'], + mode=CourseMode.VERIFIED, + user=self.user, + order_number='TEST-12345' + ) + entitlement.save() + external_id = ExternalId.objects.get( + user=self.user + ) + assert external_id is not None + assert external_id.external_id_type.name == ExternalIdType.MICROBACHELORS_COACHING + original_external_user_uuid = external_id.external_user_id + + CourseEntitlement.objects.create( + course_uuid=catalog_course2['uuid'], + mode=CourseMode.VERIFIED, + user=self.user, + order_number='TEST-12345' + ) + entitlements = CourseEntitlement.objects.filter(user=self.user) + + assert len(entitlements) == 2 + + external_ids = ExternalId.objects.filter( + user=self.user + ) + + assert len(external_ids) == 1 + assert external_ids[0].external_id_type.name == ExternalIdType.MICROBACHELORS_COACHING + assert original_external_user_uuid == external_ids[0].external_user_id