From 99863aebff1bd334b9273a9364eaf06b83564b03 Mon Sep 17 00:00:00 2001 From: Diana Huang Date: Fri, 3 Apr 2020 11:31:49 -0400 Subject: [PATCH] Remove `course_id` field from CourseMode. Handle this change appropriately in CourseModeFactory. --- .../tests/test_courseware_index.py | 9 ++--- .../course_modes/api/serializers.py | 6 ++++ .../course_modes/api/v1/tests/test_views.py | 3 +- common/djangoapps/course_modes/models.py | 27 +++++++------- .../course_modes/tests/factories.py | 36 +++++++++++++++++-- lms/djangoapps/certificates/tests/test_api.py | 4 +-- lms/djangoapps/courseware/tests/test_views.py | 29 --------------- .../instructor/tests/test_ecommerce.py | 7 ++-- .../verify_student/tests/test_views.py | 3 +- 9 files changed, 69 insertions(+), 55 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_courseware_index.py b/cms/djangoapps/contentstore/tests/test_courseware_index.py index cfa0ddb458..fd9c35665e 100644 --- a/cms/djangoapps/contentstore/tests/test_courseware_index.py +++ b/cms/djangoapps/contentstore/tests/test_courseware_index.py @@ -29,6 +29,7 @@ from contentstore.signals.handlers import listen_for_course_publish, listen_for_ from contentstore.tests.utils import CourseTestCase from contentstore.utils import reverse_course_url, reverse_usage_url from course_modes.models import CourseMode +from course_modes.tests.factories import CourseModeFactory from openedx.core.djangoapps.models.course_details import CourseDetails from xmodule.library_tools import normalize_key_for_search from xmodule.modulestore import ModuleStoreEnum @@ -473,14 +474,14 @@ class TestCoursewareSearchIndexer(MixedWithOptionsTestCase): def _test_course_about_mode_index(self, store): """ Test that informational properties in the course modes store end up in the course_info index """ - honour_mode = CourseMode( - course_id=six.text_type(self.course.id), + honour_mode = CourseModeFactory( + course_id=self.course.id, mode_slug=CourseMode.HONOR, mode_display_name=CourseMode.HONOR ) honour_mode.save() - verified_mode = CourseMode( - course_id=six.text_type(self.course.id), + verified_mode = CourseModeFactory( + course_id=self.course.id, mode_slug=CourseMode.VERIFIED, mode_display_name=CourseMode.VERIFIED, min_price=1 diff --git a/common/djangoapps/course_modes/api/serializers.py b/common/djangoapps/course_modes/api/serializers.py index 94e459e392..4510fd4eec 100644 --- a/common/djangoapps/course_modes/api/serializers.py +++ b/common/djangoapps/course_modes/api/serializers.py @@ -6,6 +6,7 @@ Course modes API serializers. from rest_framework import serializers from course_modes.models import CourseMode +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview class CourseModeSerializer(serializers.Serializer): @@ -37,6 +38,11 @@ class CourseModeSerializer(serializers.Serializer): This method must be implemented for use in our ListCreateAPIView. """ + if 'course_id' in validated_data: + course_overview = CourseOverview.get_from_id(validated_data['course_id']) + del validated_data['course_id'] + validated_data['course'] = course_overview + return CourseMode.objects.create(**validated_data) def update(self, instance, validated_data): diff --git a/common/djangoapps/course_modes/api/v1/tests/test_views.py b/common/djangoapps/course_modes/api/v1/tests/test_views.py index 4c066cf2dc..da5b881b80 100644 --- a/common/djangoapps/course_modes/api/v1/tests/test_views.py +++ b/common/djangoapps/course_modes/api/v1/tests/test_views.py @@ -21,6 +21,7 @@ from course_modes.tests.factories import CourseModeFactory from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from openedx.core.djangoapps.user_authn.tests.utils import JWT_AUTH_TYPES, AuthAndScopesTestMixin, AuthType from student.tests.factories import UserFactory +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @ddt.ddt @@ -96,7 +97,7 @@ class CourseModesViewTestBase(AuthAndScopesTestMixin): @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') -class TestCourseModesListViews(CourseModesViewTestBase, APITestCase): +class TestCourseModesListViews(CourseModesViewTestBase, ModuleStoreTestCase, APITestCase): """ Tests for the course modes list/create API endpoints. """ diff --git a/common/djangoapps/course_modes/models.py b/common/djangoapps/course_modes/models.py index 9b1034fa22..deaf47510c 100644 --- a/common/djangoapps/course_modes/models.py +++ b/common/djangoapps/course_modes/models.py @@ -6,6 +6,8 @@ Add and create new modes for running courses on this particular LMS from collections import defaultdict, namedtuple from datetime import timedelta +import inspect +import logging import six from config_models.models import ConfigurationModel from django.conf import settings @@ -25,6 +27,8 @@ from simple_history.models import HistoricalRecords from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.lib.cache_utils import request_cached +log = logging.getLogger(__name__) + Mode = namedtuple('Mode', [ 'slug', @@ -54,19 +58,6 @@ class CourseMode(models.Model): on_delete=models.DO_NOTHING, ) - # Django sets the `course_id` property in __init__ with the value from the database - # This pair of properties converts that into a proper CourseKey - @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 - # the reference to this mode that can be used by Enrollments to generate # similar behavior for the same slug across courses mode_slug = models.CharField(max_length=100, verbose_name=_("Mode")) @@ -199,6 +190,16 @@ class CourseMode(models.Model): app_label = "course_modes" unique_together = ('course', 'mode_slug', 'currency') + 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 CourseMode instantiation: %s", call_location) + + super(CourseMode, self).__init__(*args, **kwargs) + def clean(self): """ Object-level validation - implemented in this method so DRF serializers diff --git a/common/djangoapps/course_modes/tests/factories.py b/common/djangoapps/course_modes/tests/factories.py index 25717d8d9b..01031d99cd 100644 --- a/common/djangoapps/course_modes/tests/factories.py +++ b/common/djangoapps/course_modes/tests/factories.py @@ -4,12 +4,15 @@ Factories for course mode models. import random +import six from factory import lazy_attribute from factory.django import DjangoModelFactory -from opaque_keys.edx.locator import CourseLocator +from opaque_keys.edx.keys import CourseKey from course_modes.models import CourseMode +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory # Factories are self documenting @@ -18,12 +21,41 @@ class CourseModeFactory(DjangoModelFactory): class Meta(object): model = CourseMode - course_id = CourseLocator('MITx', '999', 'Robot_Super_Course') mode_slug = CourseMode.DEFAULT_MODE_SLUG currency = 'usd' expiration_datetime = None suggested_prices = '' + @classmethod + def _create(cls, model_class, *args, **kwargs): + manager = cls._get_manager(model_class) + course_kwargs = {} + for key in list(kwargs): + if key.startswith('course__'): + course_kwargs[key.split('__')[1]] = kwargs.pop(key) + + if 'course' not in kwargs: + course_id = kwargs.get('course_id') + course_overview = None + course_kwargs.setdefault('id', course_id) + if course_id is not None: + if isinstance(course_id, six.string_types): + course_id = CourseKey.from_string(course_id) + course_kwargs['id'] = course_id + try: + course_overview = CourseOverview.get_from_id(course_id) + except CourseOverview.DoesNotExist: + pass + + if course_overview is None: + course_overview = CourseOverviewFactory(**course_kwargs) + + kwargs['course'] = course_overview + + del kwargs['course_id'] + + return manager.create(*args, **kwargs) + @lazy_attribute def min_price(self): if CourseMode.is_verified_slug(self.mode_slug): diff --git a/lms/djangoapps/certificates/tests/test_api.py b/lms/djangoapps/certificates/tests/test_api.py index c8d00114d6..aca27adf99 100644 --- a/lms/djangoapps/certificates/tests/test_api.py +++ b/lms/djangoapps/certificates/tests/test_api.py @@ -675,7 +675,7 @@ class CertificateGenerationEnabledTest(EventTestMixin, TestCase): self.assertEqual(expect_enabled, actual_enabled) -class GenerateExampleCertificatesTest(TestCase): +class GenerateExampleCertificatesTest(ModuleStoreTestCase): """Test generation of example certificates. """ COURSE_KEY = CourseLocator(org='test', course='test', run='test') @@ -739,7 +739,7 @@ class GenerateExampleCertificatesTest(TestCase): @override_settings(FEATURES=FEATURES_WITH_CERTS_ENABLED) -class CertificatesBrandingTest(TestCase): +class CertificatesBrandingTest(ModuleStoreTestCase): """Test certificates branding. """ COURSE_KEY = CourseLocator(org='test', course='test', run='test') diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 687dbfe23c..4dfbaac28b 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -836,35 +836,6 @@ class ViewsTestCase(BaseViewsTestCase): self.assertNotContains(response, str(course.id)) - @patch.object(CourseOverview, 'load_from_module_store', return_value=None) - def test_financial_assistance_form_missing_course_overview(self, _mock_course_overview): - """ - Verify that learners can not get financial aid for the courses with no - course overview. - """ - # Create course - course = CourseFactory.create().id - - # Create Course Modes - CourseModeFactory.create(mode_slug=CourseMode.AUDIT, course_id=course) - CourseModeFactory.create(mode_slug=CourseMode.VERIFIED, course_id=course) - - # Enroll user in the course - # Don't use the CourseEnrollmentFactory since it ensures a CourseOverview is available - enrollment = CourseEnrollment.objects.create( - course_id=course, - user=self.user, - mode=CourseMode.AUDIT, - ) - - self.assertEqual(enrollment.course_overview, None) - - url = reverse('financial_assistance_form') - response = self.client.get(url) - self.assertEqual(response.status_code, 200) - - self.assertNotContains(response, str(course)) - def test_financial_assistance_form(self): """Verify that learner can get the financial aid for the course in which he/she is enrolled in audit mode whereas the course provide verified mode. diff --git a/lms/djangoapps/instructor/tests/test_ecommerce.py b/lms/djangoapps/instructor/tests/test_ecommerce.py index 7fe5e74faf..00faa943aa 100644 --- a/lms/djangoapps/instructor/tests/test_ecommerce.py +++ b/lms/djangoapps/instructor/tests/test_ecommerce.py @@ -11,6 +11,7 @@ from django.urls import reverse from six import text_type from course_modes.models import CourseMode +from course_modes.tests.factories import CourseModeFactory from openedx.core.djangoapps.site_configuration.tests.mixins import SiteMixin from shoppingcart.models import Coupon, CourseRegistrationCode from student.roles import CourseFinanceAdminRole @@ -39,7 +40,7 @@ class TestECommerceDashboardViews(SiteMixin, SharedModuleStoreTestCase): # Create instructor account self.instructor = AdminFactory.create() self.client.login(username=self.instructor.username, password="test") - mode = CourseMode( + mode = CourseModeFactory( course_id=text_type(self.course.id), mode_slug='honor', mode_display_name='honor', min_price=10, currency='usd' ) @@ -118,7 +119,7 @@ class TestECommerceDashboardViews(SiteMixin, SharedModuleStoreTestCase): price = 200 # course B course2 = CourseFactory.create(org='EDX', display_name='test_course', number='100') - mode = CourseMode( + mode = CourseModeFactory( course_id=text_type(course2.id), mode_slug='honor', mode_display_name='honor', min_price=30, currency='usd' ) @@ -365,7 +366,7 @@ class TestECommerceDashboardViews(SiteMixin, SharedModuleStoreTestCase): # Change honor mode to verified. original_mode = CourseMode.objects.get(course_id=self.course.id, mode_slug='honor') original_mode.delete() - new_mode = CourseMode( + new_mode = CourseModeFactory( course_id=six.text_type(self.course.id), mode_slug='verified', mode_display_name='verified', min_price=10, currency='usd' ) diff --git a/lms/djangoapps/verify_student/tests/test_views.py b/lms/djangoapps/verify_student/tests/test_views.py index ca10e0d660..e6ed3a6381 100644 --- a/lms/djangoapps/verify_student/tests/test_views.py +++ b/lms/djangoapps/verify_student/tests/test_views.py @@ -1271,7 +1271,8 @@ class TestCheckoutWithEcommerceService(ModuleStoreTestCase): ecommerce api, we correctly call to that api to create a basket. """ user = UserFactory.create(username="test-username") - course_mode = CourseModeFactory.create(sku="test-sku").to_tuple() + course_id = 'edX/test/test_run' + course_mode = CourseModeFactory.create(course_id=course_id, sku="test-sku").to_tuple() expected_payment_data = {'foo': 'bar'} # mock out the payment processors endpoint httpretty.register_uri(