From 7c115cca243bc17d780b36b5498c588e1943b898 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 2 Apr 2020 13:01:50 -0400 Subject: [PATCH 1/4] Remove the course_id property on student.CourseEnrollment. It conflicts with an underlying related field on that model which seems to be getting at the same value from the related table. Add logging for incorrectly instantiating CourseEnrollment models. This is to catch any places that might break this that are outside of edx-platform. Django won't accept `course` values that aren't course_overviews so we don't need extra logic to test that `course` values are of the correct type. fixup! Remove the course_id property on student.CourseEnrollment. --- common/djangoapps/student/models.py | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index e421c4950c..75de38d459 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -13,6 +13,7 @@ file and check it in at the same time as your model changes. To do that, import hashlib +import inspect import json import logging import uuid @@ -1094,17 +1095,6 @@ class CourseEnrollment(models.Model): def course_price(self): return get_cosmetic_verified_display_price(self.course) - @property - def course_id(self): - return self._course_id - - @course_id.setter - def course_id(self, value): - if isinstance(value, six.string_types): - self._course_id = CourseKey.from_string(value) - else: - self._course_id = value - created = models.DateTimeField(auto_now_add=True, null=True, db_index=True) # If is_active is False, then the student is not considered to be enrolled @@ -1136,6 +1126,13 @@ class CourseEnrollment(models.Model): ordering = ('user', 'course') def __init__(self, *args, **kwargs): + if 'course_id' in kwargs: + course_id = kwargs['course_id'] + if isinstance(course_id, str): + kwargs['course_id'] = CourseKey.from_string(course_id) + call_location = "\n".join("%30s : %s:%d" % (t[3], t[1], t[2]) for t in inspect.stack()[::-1]) + log.warning("Forced to coerce course_id in CourseEnrollment instantiation: %s", call_location) + super(CourseEnrollment, self).__init__(*args, **kwargs) # Private variable for storing course_overview to minimize calls to the database. From 3c618ad04dd82b40267622ddbc2af43cf0abf1eb Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Fri, 3 Apr 2020 11:27:48 -0400 Subject: [PATCH 2/4] Fixup CourseEnrollmentFactory Historically, the CourseEnrollment model used to have a `course_id` field. A lot of tests still call the factory using that. Instead of a `course_id` field this model now has a `course` field which is a foreign key to the CourseOverview model. The factory still accepts the course_id but uses it to create the relevant CourseOverview object where necessary. This commit fixes two issues with the factory. 1. If the course id is passed in as`course_id` instead of `course__id` then, the generated CourseOverview does not have the correct course_id. 2. Even though the CourseEnrollment model no longer needs the `course_id` parameter, we were still passing it through. We now remove it so that it is not passed through to the CourseEnrollment model instantiation. --- common/djangoapps/student/tests/factories.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/common/djangoapps/student/tests/factories.py b/common/djangoapps/student/tests/factories.py index 6c5f34a3fb..f8649a3341 100644 --- a/common/djangoapps/student/tests/factories.py +++ b/common/djangoapps/student/tests/factories.py @@ -142,6 +142,11 @@ class CourseEnrollmentFactory(DjangoModelFactory): course_id = kwargs.get('course_id') course_overview = None if course_id is not None: + # 'course_id' is not needed by the model when course is passed. + # This arg used to be called course_id before we added the CourseOverview + # foreign key constraint to CourseEnrollment. + del kwargs['course_id'] + if isinstance(course_id, six.string_types): course_id = CourseKey.from_string(course_id) course_kwargs.setdefault('id', course_id) @@ -152,6 +157,9 @@ class CourseEnrollmentFactory(DjangoModelFactory): pass if course_overview is None: + if 'id' not in course_kwargs and course_id: + course_kwargs['id'] = course_id + course_overview = CourseOverviewFactory(**course_kwargs) kwargs['course'] = course_overview From cf976abea0bda42cb1eb276131713a5d57f7b9cc Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Fri, 3 Apr 2020 15:48:59 -0400 Subject: [PATCH 3/4] Update how dashboard represents entitlements. Pass in a CourseOverview object instead of expecting that passing id will work as expected. --- lms/templates/dashboard.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/templates/dashboard.html b/lms/templates/dashboard.html index 654d829b0c..2353fb3f83 100644 --- a/lms/templates/dashboard.html +++ b/lms/templates/dashboard.html @@ -183,7 +183,7 @@ from student.models import CourseEnrollment pseudo_session = unfulfilled_entitlement_pseudo_sessions[str(entitlement.uuid)] if not pseudo_session: continue - enrollment = CourseEnrollment(user=user, course_id=pseudo_session['key'], mode=pseudo_session['type']) + enrollment = CourseEnrollment(user=user, course=CourseOverview.get_from_id(pseudo_session['key']), mode=pseudo_session['type']) # We only show email settings for entitlement cards if the entitlement has an associated enrollment show_email_settings = is_fulfilled_entitlement and (entitlement_session.course_id in show_email_settings_for) else: From 7a52a251324a851b9aa6f135aee8cc1dee366500 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Fri, 3 Apr 2020 09:47:48 -0400 Subject: [PATCH 4/4] Fix tests that were creating Enrollments via a course_id. We now either pass in the relevant courseoverview or when creating the enrollement we use the factory which automatically creates the relevant CourseOverview object for testing purposes. --- common/djangoapps/student/tests/test_views.py | 2 +- .../rest_api/v1/tests/test_views.py | 14 +++++++------- .../core/djangoapps/catalog/tests/test_utils.py | 6 +++--- .../core/djangoapps/enrollments/tests/test_data.py | 9 ++++++--- .../external_user_ids/tests/test_signals.py | 8 ++++---- 5 files changed, 21 insertions(+), 18 deletions(-) diff --git a/common/djangoapps/student/tests/test_views.py b/common/djangoapps/student/tests/test_views.py index be6102ed4e..eaeef16d4b 100644 --- a/common/djangoapps/student/tests/test_views.py +++ b/common/djangoapps/student/tests/test_views.py @@ -341,7 +341,7 @@ class StudentDashboardTests(SharedModuleStoreTestCase, MilestonesTestCaseMixin, # If an entitlement has already been redeemed by the user for a course run, do not let the run be selectable enrollment = CourseEnrollmentFactory( - user=self.user, course_id=six.text_type(mock_course_overview.return_value.id), mode=CourseMode.VERIFIED + user=self.user, course=mock_course_overview.return_value, mode=CourseMode.VERIFIED ) CourseEntitlementFactory.create( user=self.user, course_uuid=program['courses'][0]['uuid'], enrollment_course_run=enrollment diff --git a/lms/djangoapps/program_enrollments/rest_api/v1/tests/test_views.py b/lms/djangoapps/program_enrollments/rest_api/v1/tests/test_views.py index 9a864a2a69..8c39a7b18f 100644 --- a/lms/djangoapps/program_enrollments/rest_api/v1/tests/test_views.py +++ b/lms/djangoapps/program_enrollments/rest_api/v1/tests/test_views.py @@ -1728,13 +1728,6 @@ class ProgramCourseEnrollmentOverviewGetTests( user=self.student, ) - # create course enrollment - self.course_enrollment = CourseEnrollmentFactory.create( - course_id=self.course_id, - user=self.student, - mode=CourseMode.MASTERS, - ) - # create course overview self.course_overview = CourseOverviewFactory.create( id=self.course_id, @@ -1742,6 +1735,13 @@ class ProgramCourseEnrollmentOverviewGetTests( end=self.tomorrow, ) + # create course enrollment + self.course_enrollment = CourseEnrollmentFactory.create( + course=self.course_overview, + user=self.student, + mode=CourseMode.MASTERS, + ) + # create program course enrollment self.program_course_enrollment = ProgramCourseEnrollmentFactory.create( program_enrollment=self.program_enrollment, diff --git a/openedx/core/djangoapps/catalog/tests/test_utils.py b/openedx/core/djangoapps/catalog/tests/test_utils.py index 9db239cea7..77bffb06c2 100644 --- a/openedx/core/djangoapps/catalog/tests/test_utils.py +++ b/openedx/core/djangoapps/catalog/tests/test_utils.py @@ -592,7 +592,7 @@ class TestSessionEntitlement(CatalogIntegrationMixin, TestCase): course_overview = CourseOverviewFactory.create(id=course_key, start=self.tomorrow) CourseModeFactory.create(mode_slug=CourseMode.VERIFIED, min_price=100, course_id=course_overview.id) course_enrollment = CourseEnrollmentFactory( - user=self.user, course_id=six.text_type(course_overview.id), mode=CourseMode.VERIFIED + user=self.user, course=course_overview, mode=CourseMode.VERIFIED ) entitlement = CourseEntitlementFactory( user=self.user, enrollment_course_run=course_enrollment, mode=CourseMode.VERIFIED @@ -617,7 +617,7 @@ class TestSessionEntitlement(CatalogIntegrationMixin, TestCase): expiration_datetime=now() - timedelta(days=1) ) course_enrollment = CourseEnrollmentFactory( - user=self.user, course_id=six.text_type(course_overview.id), mode=CourseMode.VERIFIED + user=self.user, course=course_overview, mode=CourseMode.VERIFIED ) entitlement = CourseEntitlementFactory( user=self.user, enrollment_course_run=course_enrollment, mode=CourseMode.VERIFIED @@ -643,7 +643,7 @@ class TestSessionEntitlement(CatalogIntegrationMixin, TestCase): expiration_datetime=now() - timedelta(days=1) ) course_enrollment = CourseEnrollmentFactory( - user=self.user, course_id=six.text_type(course_overview.id), mode=CourseMode.VERIFIED + user=self.user, course=course_overview, mode=CourseMode.VERIFIED ) entitlement = CourseEntitlementFactory( user=self.user, enrollment_course_run=course_enrollment, mode=CourseMode.VERIFIED diff --git a/openedx/core/djangoapps/enrollments/tests/test_data.py b/openedx/core/djangoapps/enrollments/tests/test_data.py index f2908e9bb8..a74047a71b 100644 --- a/openedx/core/djangoapps/enrollments/tests/test_data.py +++ b/openedx/core/djangoapps/enrollments/tests/test_data.py @@ -27,7 +27,7 @@ from openedx.core.djangoapps.enrollments.errors import ( from openedx.core.djangoapps.enrollments.serializers import CourseEnrollmentSerializer from openedx.core.lib.exceptions import CourseNotFoundError from student.models import AlreadyEnrolledError, CourseEnrollment, CourseFullError, EnrollmentClosedError -from student.tests.factories import CourseAccessRoleFactory, UserFactory +from student.tests.factories import CourseAccessRoleFactory, UserFactory, CourseEnrollmentFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -145,12 +145,15 @@ class EnrollmentDataTest(ModuleStoreTestCase): # not exist in database) for the user and check that the method # 'get_course_enrollments' ignores course enrollments for invalid # or deleted courses - CourseEnrollment.objects.create( + non_existent_course_id = 'InvalidOrg/InvalidCourse/InvalidRun' + enrollement = CourseEnrollmentFactory.create( user=self.user, - course_id='InvalidOrg/InvalidCourse/InvalidRun', + course_id=non_existent_course_id, mode='honor', is_active=True ) + enrollement.course.delete() + updated_results = data.get_course_enrollments(self.user.username) self.assertEqual(results, updated_results) 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 6f082af511..74c1e6a6cd 100644 --- a/openedx/core/djangoapps/external_user_ids/tests/test_signals.py +++ b/openedx/core/djangoapps/external_user_ids/tests/test_signals.py @@ -12,7 +12,7 @@ from openedx.core.djangoapps.catalog.tests.factories import ( CourseFactory, ProgramFactory, ) -from student.tests.factories import TEST_PASSWORD, UserFactory +from student.tests.factories import TEST_PASSWORD, UserFactory, CourseEnrollmentFactory from openedx.core.djangoapps.catalog.cache import ( CATALOG_COURSE_PROGRAMS_CACHE_KEY_TPL, COURSE_PROGRAMS_CACHE_KEY_TPL, @@ -93,7 +93,7 @@ class MicrobachelorsExternalIDTest(ModuleStoreTestCase, CacheIsolationTestCase): course_run_key = self.program['courses'][0]['course_runs'][0]['key'] # Enroll user - enrollment = CourseEnrollment.objects.create( + enrollment = CourseEnrollmentFactory.create( course_id=course_run_key, user=self.user, mode=CourseMode.VERIFIED, @@ -110,7 +110,7 @@ class MicrobachelorsExternalIDTest(ModuleStoreTestCase, CacheIsolationTestCase): course_run_key2 = self.program['courses'][1]['course_runs'][0]['key'] # Enroll user - CourseEnrollment.objects.create( + CourseEnrollmentFactory.create( course_id=course_run_key1, user=self.user, mode=CourseMode.VERIFIED, @@ -122,7 +122,7 @@ class MicrobachelorsExternalIDTest(ModuleStoreTestCase, CacheIsolationTestCase): assert external_id.external_id_type.name == ExternalIdType.MICROBACHELORS_COACHING original_external_user_uuid = external_id.external_user_id - CourseEnrollment.objects.create( + CourseEnrollmentFactory.create( course_id=course_run_key2, user=self.user, mode=CourseMode.VERIFIED,