From 70e2aaa95bc349bd802bd15e529acbcaaeb92d88 Mon Sep 17 00:00:00 2001 From: Alex Dusenbery Date: Thu, 1 Aug 2019 14:28:56 -0400 Subject: [PATCH] EDUCATOR-4543 | Add utility functions to the catalog module that picks out all of the course_runs associated with a program, even those that live inside the curriculum of a sub-program. Use these functions to populate the course -> program index of the catalog/programs cache. Also some refactoring of a program_enrollments overview endpoint. --- .../api/v1/tests/test_views.py | 8 +- .../program_enrollments/api/v1/views.py | 172 +++++++++--------- .../management/commands/cache_programs.py | 9 +- .../commands/tests/test_cache_programs.py | 31 +++- .../djangoapps/catalog/tests/factories.py | 4 +- .../djangoapps/catalog/tests/test_utils.py | 122 +++++++++++++ openedx/core/djangoapps/catalog/utils.py | 53 ++++++ 7 files changed, 297 insertions(+), 102 deletions(-) diff --git a/lms/djangoapps/program_enrollments/api/v1/tests/test_views.py b/lms/djangoapps/program_enrollments/api/v1/tests/test_views.py index 5c17b31485..838a5e75ec 100644 --- a/lms/djangoapps/program_enrollments/api/v1/tests/test_views.py +++ b/lms/djangoapps/program_enrollments/api/v1/tests/test_views.py @@ -1327,8 +1327,8 @@ class ProgramCourseEnrollmentOverviewViewTests(ProgramCacheTestCaseMixin, Shared # only freeze time when defining these values and not on the whole test case # as test_multiple_enrollments_all_enrolled relies on actual differences in modified datetimes with freeze_time('2019-01-01'): - cls.yesterday = datetime.now() - timedelta(1) - cls.tomorrow = datetime.now() + timedelta(1) + cls.yesterday = datetime.utcnow() - timedelta(1) + cls.tomorrow = datetime.utcnow() + timedelta(1) cls.certificate_download_url = 'www.certificates.com' @@ -1670,7 +1670,7 @@ class ProgramCourseEnrollmentOverviewViewTests(ProgramCacheTestCaseMixin, Shared # course run has not ended and user has earned a passing certificate more than 30 days ago certificate = self.create_generated_certificate() - certificate.created_date = datetime.now() - timedelta(30) + certificate.created_date = datetime.utcnow() - timedelta(30) certificate.save() mock_has_ended.return_value = False @@ -1704,7 +1704,7 @@ class ProgramCourseEnrollmentOverviewViewTests(ProgramCacheTestCaseMixin, Shared # course run has not ended and user has earned a passing certificate fewer than 30 days ago certificate = self.create_generated_certificate() - certificate.created_date = datetime.now() - timedelta(5) + certificate.created_date = datetime.utcnow() - timedelta(5) certificate.save() response = self.client.get(self.get_url(self.program_uuid)) diff --git a/lms/djangoapps/program_enrollments/api/v1/views.py b/lms/djangoapps/program_enrollments/api/v1/views.py index df21fdd2d8..2334d78a7c 100644 --- a/lms/djangoapps/program_enrollments/api/v1/views.py +++ b/lms/djangoapps/program_enrollments/api/v1/views.py @@ -12,6 +12,7 @@ from pytz import UTC from django.http import Http404 from django.core.exceptions import PermissionDenied from django.urls import reverse +from django.utils.functional import cached_property from edx_rest_framework_extensions import permissions from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser @@ -567,7 +568,7 @@ class ProgramSpecificViewMixin(object): A mixin for views that operate on or within a specific program. """ - @property + @cached_property def program(self): """ The program specified by the `program_uuid` URL parameter. @@ -956,23 +957,77 @@ class ProgramCourseEnrollmentOverviewView(DeveloperErrorViewMixin, ProgramSpecif for a user as part of a program. """ user = request.user + program_enrollment = self._get_program_enrollment(user, program_uuid) - user_program_enrollment = ProgramEnrollment.objects.filter( + program_course_enrollments = ProgramCourseEnrollment.objects.filter( + program_enrollment=program_enrollment + ).select_related('course_enrollment') + + enrollments_by_course_key = { + enrollment.course_key: enrollment.course_enrollment + for enrollment in program_course_enrollments + } + + overviews = CourseOverview.get_from_ids_if_exists(enrollments_by_course_key.keys()) + + course_run_resume_urls = get_resume_urls_for_enrollments(user, enrollments_by_course_key.values()) + + course_runs = [] + + for enrollment in program_course_enrollments: + overview = overviews[enrollment.course_key] + + certificate_info = get_certificate_for_user(user.username, enrollment.course_key) or {} + + course_run_dict = { + 'course_run_id': enrollment.course_key, + 'display_name': overview.display_name_with_default, + 'course_run_status': self.get_course_run_status(overview, certificate_info), + 'course_run_url': self.get_course_run_url(request, enrollment.course_key), + 'start_date': overview.start, + 'end_date': overview.end, + 'due_dates': self.get_due_dates(request, enrollment.course_key, user), + } + + emails_enabled = self.get_emails_enabled(user, enrollment.course_key) + if emails_enabled is not None: + course_run_dict['emails_enabled'] = emails_enabled + + if certificate_info.get('download_url'): + course_run_dict['certificate_download_url'] = certificate_info['download_url'] + + if self.program['type'] == 'MicroMasters': + course_run_dict['micromasters_title'] = self.program['title'] + + if course_run_resume_urls.get(enrollment.course_key): + course_run_dict['resume_course_run_url'] = course_run_resume_urls.get(enrollment.course_key) + + course_runs.append(course_run_dict) + + serializer = CourseRunOverviewListSerializer({'course_runs': course_runs}) + return Response(serializer.data) + + @staticmethod + def _get_program_enrollment(user, program_uuid): + """ + Returns the ``ProgramEnrollment`` record for the given program UUID in which the given + user is actively enrolled. + In the unusual and unlikely case of a user having two active program enrollments + for the same program, returns the most recently modified enrollment and logs + a warning. + + Raises ``PermissionDenied`` if the user is not enrolled in the program with the given UUID. + """ + program_enrollment = ProgramEnrollment.objects.filter( program_uuid=program_uuid, user=user, status='enrolled', - ).order_by( - '-modified', - ) + ).order_by('-modified') - user_program_enrollment_count = user_program_enrollment.count() + program_enrollment_count = program_enrollment.count() - if user_program_enrollment_count > 1: - # in the unusual and unlikely case of a user having two - # active program enrollments for the same program, - # choose the most recently modified enrollment and log - # a warning - user_program_enrollment = user_program_enrollment[0] + if program_enrollment_count > 1: + program_enrollment = program_enrollment[0] logger.warning( ('User with user_id {} has more than program enrollment' 'with an enrolled status for program uuid {}.').format( @@ -980,68 +1035,10 @@ class ProgramCourseEnrollmentOverviewView(DeveloperErrorViewMixin, ProgramSpecif program_uuid, ) ) - elif user_program_enrollment_count == 0: - # if the user is not enrolled in the program, they are not authorized - # to view the information returned by this endpoint + elif program_enrollment_count == 0: raise PermissionDenied - user_program_course_enrollments = ProgramCourseEnrollment.objects.filter( - program_enrollment=user_program_enrollment - ).select_related('course_enrollment') - - enrollment_dict = {enrollment.course_key: enrollment.course_enrollment for enrollment in user_program_course_enrollments} - - overviews = CourseOverview.get_from_ids_if_exists(enrollment_dict.keys()) - - resume_course_run_urls = get_resume_urls_for_enrollments(user, enrollment_dict.values()) - - response = { - 'course_runs': [], - } - - for enrollment in user_program_course_enrollments: - overview = overviews[enrollment.course_key] - - certificate_download_url = None - is_certificate_passing = None - certificate_creation_date = None - certificate_info = get_certificate_for_user(user.username, enrollment.course_key) - - if certificate_info: - certificate_download_url = certificate_info['download_url'] - is_certificate_passing = certificate_info['is_passing'] - certificate_creation_date = certificate_info['created'] - - course_run_dict = { - 'course_run_id': enrollment.course_key, - 'display_name': overview.display_name_with_default, - 'course_run_status': self.get_course_run_status(overview, is_certificate_passing, certificate_creation_date), - 'course_run_url': self.get_course_run_url(request, enrollment.course_key), - 'start_date': overview.start, - 'end_date': overview.end, - 'due_dates': self.get_due_dates(request, enrollment.course_key, user), - } - - if certificate_download_url: - course_run_dict['certificate_download_url'] = certificate_download_url - - emails_enabled = self.get_emails_enabled(user, enrollment.course_key) - if emails_enabled is not None: - course_run_dict['emails_enabled'] = emails_enabled - - micromasters_title = self.program['title'] if self.program['type'] == 'MicroMasters' else None - if micromasters_title: - course_run_dict['micromasters_title'] = micromasters_title - - # if the url is '', then the url is None so we can omit it from the response - resume_course_run_url = resume_course_run_urls[enrollment.course_key] - if resume_course_run_url: - course_run_dict['resume_course_run_url'] = resume_course_run_url - - response['course_runs'].append(course_run_dict) - - serializer = CourseRunOverviewListSerializer(response) - return Response(serializer.data) + return program_enrollment @staticmethod def get_due_dates(request, course_key, user): @@ -1113,25 +1110,32 @@ class ProgramCourseEnrollmentOverviewView(DeveloperErrorViewMixin, ProgramSpecif """ if is_bulk_email_feature_enabled(course_id=course_id): return not is_user_opted_out_for_course(user=user, course_id=course_id) - else: - return None + return None @staticmethod - def get_course_run_status(course_overview, is_certificate_passing, certificate_creation_date): + def get_course_run_status(course_overview, certificate_info): """ - Get the progress status of a course run. + Get the progress status of a course run, given the state of a user's certificate in the course. + + In the case of self-paced course runs, the run is considered completed when either the course run has ended + OR the user has earned a passing certificate 30 days ago or longer. Arguments: course_overview (CourseOverview): the overview for the course run - is_certificate_passing (bool): True if the user has a passing certificate in - this course run; False otherwise - certificate_creation_date: the date the certificate was created + certificate_info: A dict containing the following keys: + ``is_passing``: whether the user has a passing certificate in the course run + ``created``: the date the certificate was created Returns: - status: one of CourseRunProgressStatuses.COMPLETE, + status: one of ( + CourseRunProgressStatuses.COMPLETE, CourseRunProgressStatuses.IN_PROGRESS, - or CourseRunProgressStatuses.UPCOMING + CourseRunProgressStatuses.UPCOMING, + ) """ + is_certificate_passing = certificate_info.get('is_passing', False) + certificate_creation_date = certificate_info.get('created', datetime.max) + if course_overview.pacing == 'instructor': if course_overview.has_ended(): return CourseRunProgressStatuses.COMPLETED @@ -1140,11 +1144,9 @@ class ProgramCourseEnrollmentOverviewView(DeveloperErrorViewMixin, ProgramSpecif else: return CourseRunProgressStatuses.UPCOMING elif course_overview.pacing == 'self': - has_ended = course_overview.has_ended() thirty_days_ago = datetime.now(UTC) - timedelta(30) - # a self paced course run is completed when either the course run has ended - # OR the user has earned a certificate 30 days ago or more - if has_ended or is_certificate_passing and (certificate_creation_date and certificate_creation_date <= thirty_days_ago): + certificate_completed = is_certificate_passing and (certificate_creation_date <= thirty_days_ago) + if course_overview.has_ended() or certificate_completed: return CourseRunProgressStatuses.COMPLETED elif course_overview.has_started(): return CourseRunProgressStatuses.IN_PROGRESS diff --git a/openedx/core/djangoapps/catalog/management/commands/cache_programs.py b/openedx/core/djangoapps/catalog/management/commands/cache_programs.py index b8c64f7424..8fff31845c 100644 --- a/openedx/core/djangoapps/catalog/management/commands/cache_programs.py +++ b/openedx/core/djangoapps/catalog/management/commands/cache_programs.py @@ -17,7 +17,7 @@ from openedx.core.djangoapps.catalog.cache import ( SITE_PROGRAM_UUIDS_CACHE_KEY_TPL, ) from openedx.core.djangoapps.catalog.models import CatalogIntegration -from openedx.core.djangoapps.catalog.utils import create_catalog_api_client +from openedx.core.djangoapps.catalog.utils import create_catalog_api_client, course_run_keys_for_program logger = logging.getLogger(__name__) User = get_user_model() # pylint: disable=invalid-name @@ -218,8 +218,7 @@ class Command(BaseCommand): failure = False for program in programs.values(): - for course in program['courses']: - for course_run in course['course_runs']: - course_run_cache_key = COURSE_PROGRAMS_CACHE_KEY_TPL.format(course_run_id=course_run['key']) - course_runs[course_run_cache_key].append(program['uuid']) + for course_run_key in course_run_keys_for_program(program): + course_run_cache_key = COURSE_PROGRAMS_CACHE_KEY_TPL.format(course_run_id=course_run_key) + course_runs[course_run_cache_key].append(program['uuid']) return course_runs, failure 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 062e1ea149..28806b9990 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 @@ -9,6 +9,7 @@ from django.core.cache import cache from django.core.management import call_command from openedx.core.djangoapps.catalog.cache import ( + COURSE_PROGRAMS_CACHE_KEY_TPL, PATHWAY_CACHE_KEY_TPL, PROGRAM_CACHE_KEY_TPL, SITE_PATHWAY_IDS_CACHE_KEY_TPL, @@ -24,6 +25,9 @@ from student.tests.factories import UserFactory @skip_unless_lms @httpretty.activate class TestCachePrograms(CatalogIntegrationMixin, CacheIsolationTestCase, SiteMixin): + """ + Defines tests for the ``cache_programs`` management command. + """ ENABLED_CACHES = ['default'] def setUp(self): @@ -46,6 +50,10 @@ class TestCachePrograms(CatalogIntegrationMixin, CacheIsolationTestCase, SiteMix self.programs = ProgramFactory.create_batch(3) self.pathways = PathwayFactory.create_batch(3) + self.child_program = ProgramFactory.create() + + self.programs[0]['curricula'][0]['programs'].append(self.child_program) + self.programs.append(self.child_program) for pathway in self.pathways: self.programs += pathway['programs'] @@ -57,7 +65,10 @@ class TestCachePrograms(CatalogIntegrationMixin, CacheIsolationTestCase, SiteMix self.pathways[1]['programs'].append(self.programs[0]) def mock_list(self): + """ Mock the data returned by the program listing API endpoint. """ + # pylint: disable=unused-argument def list_callback(request, uri, headers): + """ The mock listing callback. """ expected = { 'exclude_utm': ['1'], 'status': ['active', 'retired'], @@ -75,7 +86,10 @@ class TestCachePrograms(CatalogIntegrationMixin, CacheIsolationTestCase, SiteMix ) def mock_detail(self, uuid, program): + """ Mock the data returned by the program detail API endpoint. """ + # pylint: disable=unused-argument def detail_callback(request, uri, headers): + """ The mock detail callback. """ expected = { 'exclude_utm': ['1'], } @@ -91,13 +105,9 @@ class TestCachePrograms(CatalogIntegrationMixin, CacheIsolationTestCase, SiteMix ) def mock_pathways(self, pathways, page_number=1, final=True): - """ - Mock the data for discovery's credit pathways endpoint - """ + """ Mock the data for discovery's credit pathways endpoint. """ def pathways_callback(request, uri, headers): # pylint: disable=unused-argument - """ - Mocks response - """ + """ Mocks the pathways response. """ expected = { 'exclude_utm': ['1'], @@ -171,6 +181,15 @@ class TestCachePrograms(CatalogIntegrationMixin, CacheIsolationTestCase, SiteMix del program['pathway_ids'] self.assertEqual(program, programs[key]) + # the courses in the child program's first curriculum (the active one) + # should point to both the child program and the first program + # in the cache. + for course in self.child_program['curricula'][0]['courses']: + for course_run in course['course_runs']: + course_run_cache_key = COURSE_PROGRAMS_CACHE_KEY_TPL.format(course_run_id=course_run['key']) + self.assertIn(self.programs[0]['uuid'], cache.get(course_run_cache_key)) + self.assertIn(self.child_program['uuid'], cache.get(course_run_cache_key)) + def test_handle_pathways(self): """ Verify that the command requests and caches credit pathways diff --git a/openedx/core/djangoapps/catalog/tests/factories.py b/openedx/core/djangoapps/catalog/tests/factories.py index f73a78830d..f7429f3546 100644 --- a/openedx/core/djangoapps/catalog/tests/factories.py +++ b/openedx/core/djangoapps/catalog/tests/factories.py @@ -197,7 +197,7 @@ def generate_curricula(): class ProgramFactory(DictFactoryBase): authoring_organizations = factory.LazyFunction(partial(generate_instances, OrganizationFactory, count=1)) - applicable_seat_types = [] + applicable_seat_types = factory.LazyFunction(lambda: []) banner_image = factory.LazyFunction(generate_sized_stdimage) card_image_url = factory.Faker('image_url') corporate_endorsements = factory.LazyFunction(partial(generate_instances, CorporateEndorsementFactory)) @@ -232,7 +232,7 @@ class CurriculumFactory(DictFactoryBase): marketing_text_brief = factory.Faker('word') is_active = True courses = factory.LazyFunction(partial(generate_instances, CourseFactory)) - programs = [] + programs = factory.LazyFunction(lambda: []) class ProgramTypeFactory(DictFactoryBase): diff --git a/openedx/core/djangoapps/catalog/tests/test_utils.py b/openedx/core/djangoapps/catalog/tests/test_utils.py index 9708d14c22..87732fb265 100644 --- a/openedx/core/djangoapps/catalog/tests/test_utils.py +++ b/openedx/core/djangoapps/catalog/tests/test_utils.py @@ -35,6 +35,8 @@ from openedx.core.djangoapps.catalog.tests.factories import ( ) from openedx.core.djangoapps.catalog.tests.mixins import CatalogIntegrationMixin from openedx.core.djangoapps.catalog.utils import ( + child_programs, + course_run_keys_for_program, get_course_run_details, get_course_runs, get_course_runs_for_course, @@ -663,3 +665,123 @@ class TestGetCourseRunDetails(CatalogIntegrationMixin, TestCase): data = get_course_run_details(course_run['key'], ['content_language', 'weeks_to_complete', 'max_effort']) self.assertTrue(mock_get_edx_api_data.called) self.assertEqual(data, course_run_details) + + +class TestProgramCourseRunCrawling(TestCase): + @classmethod + def setUpClass(cls): + super(TestProgramCourseRunCrawling, cls).setUpClass() + cls.grandchild_1 = { + 'title': 'grandchild 1', + 'curricula': [{'is_active': True, 'courses': [], 'programs': []}], + } + cls.grandchild_2 = { + 'title': 'grandchild 2', + 'curricula': [ + { + 'is_active': True, + 'courses': [{ + 'course_runs': [ + {'key': 'course-run-4'}, + ], + }], + 'programs': [], + }, + ], + } + cls.grandchild_3 = { + 'title': 'grandchild 3', + 'curricula': [{'is_active': False}], + } + cls.child_1 = { + 'title': 'child 1', + 'curricula': [{'is_active': True, 'courses': [], 'programs': [cls.grandchild_1]}], + } + cls.child_2 = { + 'title': 'child 2', + 'curricula': [ + { + 'is_active': True, + 'courses': [{ + 'course_runs': [ + {'key': 'course-run-3'}, + ], + }], + 'programs': [cls.grandchild_2, cls.grandchild_3], + }, + ], + } + cls.complex_program = { + 'title': 'complex program', + 'curricula': [ + { + 'is_active': True, + 'courses': [{ + 'course_runs': [ + {'key': 'course-run-2'}, + ], + }], + 'programs': [cls.child_1, cls.child_2], + }, + ], + } + cls.simple_program = { + 'title': 'simple program', + 'curricula': [ + { + 'is_active': True, + 'courses': [{ + 'course_runs': [ + {'key': 'course-run-1'}, + ], + }], + 'programs': [cls.grandchild_1] + }, + ], + } + cls.empty_program = { + 'title': 'notice that I have a curriculum, but no programs inside it', + 'curricula': [ + { + 'is_active': True, + 'courses': [], + 'programs': [], + }, + ], + } + + def test_child_programs_no_curriculum(self): + program = { + 'title': 'notice that I do not have a curriculum', + } + self.assertEqual([], child_programs(program)) + + def test_child_programs_no_children(self): + self.assertEqual([], child_programs(self.empty_program)) + + def test_child_programs_one_child(self): + self.assertEqual([self.grandchild_1], child_programs(self.simple_program)) + + def test_child_programs_many_children(self): + expected_children = [ + self.child_1, + self.grandchild_1, + self.child_2, + self.grandchild_2, + self.grandchild_3, + ] + self.assertEqual(expected_children, child_programs(self.complex_program)) + + def test_course_run_keys_for_program_no_courses(self): + self.assertEqual(set(), course_run_keys_for_program(self.empty_program)) + + def test_course_run_keys_for_program_one_course(self): + self.assertEqual({'course-run-1'}, course_run_keys_for_program(self.simple_program)) + + def test_course_run_keys_for_program_many_courses(self): + expected_course_runs = { + 'course-run-2', + 'course-run-3', + 'course-run-4', + } + self.assertEqual(expected_course_runs, course_run_keys_for_program(self.complex_program)) diff --git a/openedx/core/djangoapps/catalog/utils.py b/openedx/core/djangoapps/catalog/utils.py index 191d337ef2..4a1d119973 100644 --- a/openedx/core/djangoapps/catalog/utils.py +++ b/openedx/core/djangoapps/catalog/utils.py @@ -543,3 +543,56 @@ def get_course_run_details(course_run_key, fields): course_run_details = get_edx_api_data(catalog_integration, 'course_runs', api, resource_id=course_run_key, cache_key=cache_key, many=False, traverse_pagination=False, fields=fields) return course_run_details + + +def course_run_keys_for_program(parent_program): + """ + All of the course run keys 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. + """ + keys = set() + for program in [parent_program] + child_programs(parent_program): + curriculum = _primary_active_curriculum(program) + if curriculum: + keys.update(_course_runs_from_container(curriculum)) + keys.update(_course_runs_from_container(program)) + return keys + + +def child_programs(program): + """ + Given a program, recursively find all child programs related + to this program through its curricula. + """ + curriculum = _primary_active_curriculum(program) + if not curriculum: + return [] + result = [] + for child in curriculum.get('programs', []): + result.append(child) + result.extend(child_programs(child)) + return result + + +def _primary_active_curriculum(program): + """ + Returns the first active curriculum in the given program, or None. + """ + try: + return next(c for c in program.get('curricula', []) if c.get('is_active')) + except StopIteration: + return + + +def _course_runs_from_container(container): + """ + Pluck nested course runs 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_run.get('key') + for course in container.get('courses', []) + for course_run in course.get('course_runs', []) + ]