From 8a2bf2b60e6dfca64aaa77c5f4f95b302eed526a Mon Sep 17 00:00:00 2001 From: Alex Dusenbery Date: Tue, 6 Aug 2019 13:47:55 -0400 Subject: [PATCH] EDUCATOR-4543 | Let the program_enrollments overview endpoint return course enrollment information for verified and masters mode enrollments, even if they are not directly tied to a ProgramCourseEnrollment. --- .../api/v1/tests/test_views.py | 108 +++++------------- .../program_enrollments/api/v1/views.py | 70 +++++------- 2 files changed, 57 insertions(+), 121 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 838a5e75ec..cb6021fae1 100644 --- a/lms/djangoapps/program_enrollments/api/v1/tests/test_views.py +++ b/lms/djangoapps/program_enrollments/api/v1/tests/test_views.py @@ -35,7 +35,7 @@ from lms.djangoapps.program_enrollments.tests.factories import ProgramCourseEnro from lms.djangoapps.program_enrollments.models import ProgramEnrollment, ProgramCourseEnrollment from lms.djangoapps.program_enrollments.utils import ProviderDoesNotExistException from openedx.core.djangoapps.catalog.cache import PROGRAM_CACHE_KEY_TPL -from openedx.core.djangoapps.catalog.tests.factories import CourseFactory +from openedx.core.djangoapps.catalog.tests.factories import CourseFactory, CourseRunFactory from openedx.core.djangoapps.catalog.tests.factories import OrganizationFactory as CatalogOrganizationFactory from openedx.core.djangoapps.catalog.tests.factories import ProgramFactory from openedx.core.djangoapps.content.course_overviews.models import CourseOverview @@ -593,6 +593,7 @@ class CourseEnrollmentPostTests(BaseCourseEnrollmentTestsMixin, APITestCase): ) +# pylint: disable=no-member @ddt.ddt class CourseEnrollmentModificationTestBase(BaseCourseEnrollmentTestsMixin): """ @@ -1320,6 +1321,8 @@ class ProgramCourseEnrollmentOverviewViewTests(ProgramCacheTestCaseMixin, Shared cls.other_curriculum_uuid = 'bbbbbbbb-1111-2222-3333-444444444444' cls.course_id = CourseKey.from_string('course-v1:edX+ToyX+Toy_Course') + cls.course_run = CourseRunFactory.create(key=text_type(cls.course_id)) + cls.course = CourseFactory.create(course_runs=[cls.course_run]) cls.password = 'password' cls.student = UserFactory.create(username='student', password=cls.password) @@ -1335,9 +1338,6 @@ class ProgramCourseEnrollmentOverviewViewTests(ProgramCacheTestCaseMixin, Shared def setUp(self): super(ProgramCourseEnrollmentOverviewViewTests, self).setUp() - # create program - self.program = self.setup_catalog_cache(self.program_uuid, 'organization_key') - # create program enrollment self.program_enrollment = ProgramEnrollmentFactory.create( program_uuid=self.program_uuid, @@ -1346,9 +1346,10 @@ class ProgramCourseEnrollmentOverviewViewTests(ProgramCacheTestCaseMixin, Shared ) # create course enrollment - self.course_enrollment = CourseEnrollmentFactory( + self.course_enrollment = CourseEnrollmentFactory.create( course_id=self.course_id, user=self.student, + mode=CourseMode.MASTERS, ) # create course overview @@ -1366,6 +1367,11 @@ class ProgramCourseEnrollmentOverviewViewTests(ProgramCacheTestCaseMixin, Shared status='active', ) + # create program + self.program = self.setup_catalog_cache(self.program_uuid, 'organization_key') + self.program['curricula'][0]['courses'].append(self.course) + self.set_program_in_catalog_cache(self.program_uuid, self.program) + def create_generated_certificate(self): return GeneratedCertificateFactory.create( user=self.student, @@ -1401,91 +1407,39 @@ class ProgramCourseEnrollmentOverviewViewTests(ProgramCacheTestCaseMixin, Shared response = self.client.get(self.get_url(self.program_uuid)) assert status.HTTP_403_FORBIDDEN == response.status_code - @ddt.data( - 'pending', - 'suspended', - 'canceled', - ) - def test_multiple_enrollments_with_not_enrolled(self, program_enrollment_status): - # add a second program enrollment - program_enrollment = ProgramEnrollmentFactory.create( - program_uuid=self.program_uuid, - curriculum_uuid=self.other_curriculum_uuid, - user=self.student, - status=program_enrollment_status, - ) - - other_course_key_string = 'course-v1:edX+ToyX+Other_Course' - other_course_key = CourseKey.from_string(other_course_key_string) - - # add a second course enrollment - course_enrollment = CourseEnrollmentFactory( - course_id=other_course_key, - user=self.student, - ) - - # add a second program course enrollment - ProgramCourseEnrollmentFactory.create( - program_enrollment=program_enrollment, - course_enrollment=course_enrollment, - course_key=other_course_key, - status='active', - ) - - # add a course over view for other_course_key + def _add_new_course_to_program(self, course_run_key, program): + """ + Helper method to create another course, an overview for it, + add it to the program, and re-load the cache. + """ + other_course_run = CourseRunFactory.create(key=text_type(course_run_key)) + other_course = CourseFactory.create(course_runs=[other_course_run]) + program['courses'].append(other_course) + self.set_program_in_catalog_cache(program['uuid'], program) CourseOverviewFactory.create( - id=other_course_key, + id=course_run_key, start=self.yesterday, ) - self.client.login(username=self.student.username, password=self.password) - response = self.client.get(self.get_url(self.program_uuid)) - - self.assertEqual(status.HTTP_200_OK, response.status_code) - # we expect data associated with the last modified program enrollment - # with 'enrolled' status - self.assertEqual( - text_type(self.program_course_enrollment.course_key), - response.data['course_runs'][0]['course_run_id'] - ) - def test_multiple_enrollments_all_enrolled(self): - # add a second program enrollment - program_enrollment = ProgramEnrollmentFactory.create( - program_uuid=self.program_uuid, - curriculum_uuid=self.other_curriculum_uuid, - user=self.student, - ) + other_course_key = CourseKey.from_string('course-v1:edX+ToyX+Other_Course') + self._add_new_course_to_program(other_course_key, self.program) - other_course_key_string = 'course-v1:edX+ToyX+Other_Course' - other_course_key = CourseKey.from_string(other_course_key_string) - - # add a second course enrollment - course_enrollment = CourseEnrollmentFactory( + # add a second course enrollment, which doesn't need a ProgramCourseEnrollment + # to be returned. + CourseEnrollmentFactory.create( course_id=other_course_key, - user=self.student - ) - - # add a second program course enrollment - ProgramCourseEnrollmentFactory.create( - program_enrollment=program_enrollment, - course_enrollment=course_enrollment, - course_key=other_course_key, - status='active', - ) - - # add a course over view for other_course_key - CourseOverviewFactory.create( - id=other_course_key, - start=self.yesterday, + user=self.student, + mode=CourseMode.VERIFIED, ) self.client.login(username=self.student.username, password=self.password) response = self.client.get(self.get_url(self.program_uuid)) self.assertEqual(status.HTTP_200_OK, response.status_code) - # we expect data associated with the last modified program enrollment - self.assertEqual(other_course_key_string, response.data['course_runs'][0]['course_run_id']) + actual_course_run_ids = {run['course_run_id'] for run in response.data['course_runs']} + expected_course_run_ids = {text_type(other_course_key), text_type(self.course_id)} + self.assertEqual(expected_course_run_ids, actual_course_run_ids) @mock.patch('lms.djangoapps.program_enrollments.api.v1.views.get_resume_urls_for_enrollments') def test_resume_urls(self, mock_get_resume_urls): diff --git a/lms/djangoapps/program_enrollments/api/v1/views.py b/lms/djangoapps/program_enrollments/api/v1/views.py index 2334d78a7c..b098c2545f 100644 --- a/lms/djangoapps/program_enrollments/api/v1/views.py +++ b/lms/djangoapps/program_enrollments/api/v1/views.py @@ -27,6 +27,7 @@ from rest_framework.response import Response from six import iteritems from bulk_email.api import is_bulk_email_feature_enabled, is_user_opted_out_for_course +from course_modes.models import CourseMode from edx_when.api import get_dates_for_course from lms.djangoapps.certificates.api import get_certificate_for_user from lms.djangoapps.program_enrollments.api.v1.constants import ( @@ -46,8 +47,9 @@ from lms.djangoapps.program_enrollments.api.v1.serializers import ( from lms.djangoapps.program_enrollments.models import ProgramCourseEnrollment, ProgramEnrollment from lms.djangoapps.program_enrollments.utils import get_user_by_program_id, ProviderDoesNotExistException from student.helpers import get_resume_urls_for_enrollments +from student.models import CourseEnrollment from xmodule.modulestore.django import modulestore -from openedx.core.djangoapps.catalog.utils import get_programs +from openedx.core.djangoapps.catalog.utils import get_programs, course_run_keys_for_program from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.lib.api.authentication import OAuth2AuthenticationAllowInactiveUser from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, PaginatedAPIView, verify_course_exists @@ -957,39 +959,39 @@ class ProgramCourseEnrollmentOverviewView(DeveloperErrorViewMixin, ProgramSpecif for a user as part of a program. """ user = request.user - program_enrollment = self._get_program_enrollment(user, program_uuid) + self._check_program_enrollment_exists(user, program_uuid) - program_course_enrollments = ProgramCourseEnrollment.objects.filter( - program_enrollment=program_enrollment - ).select_related('course_enrollment') + program = get_programs(uuid=program_uuid) + course_run_keys = [CourseKey.from_string(key) for key in course_run_keys_for_program(program)] - enrollments_by_course_key = { - enrollment.course_key: enrollment.course_enrollment - for enrollment in program_course_enrollments - } + course_enrollments = CourseEnrollment.objects.filter( + user=user, + course_id__in=course_run_keys, + mode__in=[CourseMode.VERIFIED, CourseMode.MASTERS], + ) - overviews = CourseOverview.get_from_ids_if_exists(enrollments_by_course_key.keys()) + overviews = CourseOverview.get_from_ids_if_exists(course_run_keys) - course_run_resume_urls = get_resume_urls_for_enrollments(user, enrollments_by_course_key.values()) + course_run_resume_urls = get_resume_urls_for_enrollments(user, course_enrollments) course_runs = [] - for enrollment in program_course_enrollments: - overview = overviews[enrollment.course_key] + for enrollment in course_enrollments: + overview = overviews[enrollment.course_id] - certificate_info = get_certificate_for_user(user.username, enrollment.course_key) or {} + certificate_info = get_certificate_for_user(user.username, enrollment.course_id) or {} course_run_dict = { - 'course_run_id': enrollment.course_key, + 'course_run_id': enrollment.course_id, '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), + 'course_run_url': self.get_course_run_url(request, enrollment.course_id), 'start_date': overview.start, 'end_date': overview.end, - 'due_dates': self.get_due_dates(request, enrollment.course_key, user), + 'due_dates': self.get_due_dates(request, enrollment.course_id, user), } - emails_enabled = self.get_emails_enabled(user, enrollment.course_key) + emails_enabled = self.get_emails_enabled(user, enrollment.course_id) if emails_enabled is not None: course_run_dict['emails_enabled'] = emails_enabled @@ -999,8 +1001,8 @@ class ProgramCourseEnrollmentOverviewView(DeveloperErrorViewMixin, ProgramSpecif 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) + if course_run_resume_urls.get(enrollment.course_id): + course_run_dict['resume_course_run_url'] = course_run_resume_urls.get(enrollment.course_id) course_runs.append(course_run_dict) @@ -1008,38 +1010,18 @@ class ProgramCourseEnrollmentOverviewView(DeveloperErrorViewMixin, ProgramSpecif return Response(serializer.data) @staticmethod - def _get_program_enrollment(user, program_uuid): + def _check_program_enrollment_exists(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_enrollments = ProgramEnrollment.objects.filter( program_uuid=program_uuid, user=user, status='enrolled', - ).order_by('-modified') - - program_enrollment_count = program_enrollment.count() - - 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( - user.id, - program_uuid, - ) - ) - elif program_enrollment_count == 0: + ) + if not program_enrollments: raise PermissionDenied - return program_enrollment - @staticmethod def get_due_dates(request, course_key, user): """