From 27edca3c5e371bee517c0fa01e1178bd1aced63e Mon Sep 17 00:00:00 2001 From: John Eskew Date: Thu, 18 Jan 2018 14:25:57 -0500 Subject: [PATCH] Replace all clean_course_id form methods with common method. --- cms/djangoapps/contentstore/config/forms.py | 25 ++++++------------- cms/djangoapps/xblock_config/forms.py | 18 ++----------- common/djangoapps/course_modes/admin.py | 15 ++++------- common/djangoapps/student/admin.py | 18 +++---------- .../student/tests/test_admin_views.py | 2 +- lms/djangoapps/bulk_email/forms.py | 22 ++++------------ lms/djangoapps/bulk_email/tests/test_forms.py | 11 +++----- lms/djangoapps/grades/config/forms.py | 18 ++++--------- lms/djangoapps/support/tests/test_refund.py | 2 +- lms/djangoapps/support/views/refund.py | 13 +++------- openedx/core/djangoapps/video_config/forms.py | 18 ++----------- openedx/core/lib/courses.py | 6 ++--- 12 files changed, 42 insertions(+), 126 deletions(-) diff --git a/cms/djangoapps/contentstore/config/forms.py b/cms/djangoapps/contentstore/config/forms.py index b233e4f498..f68c7441f2 100644 --- a/cms/djangoapps/contentstore/config/forms.py +++ b/cms/djangoapps/contentstore/config/forms.py @@ -4,13 +4,13 @@ Defines a form for providing validation. import logging from django import forms +from opaque_keys import InvalidKeyError +from opaque_keys.edx.locator import CourseLocator +from six import text_type from contentstore.config.models import CourseNewAssetsPageFlag - -from opaque_keys import InvalidKeyError -from six import text_type +from openedx.core.lib.courses import clean_course_id from xmodule.modulestore.django import modulestore -from opaque_keys.edx.locator import CourseLocator log = logging.getLogger(__name__) @@ -23,16 +23,7 @@ class CourseNewAssetsPageAdminForm(forms.ModelForm): fields = '__all__' def clean_course_id(self): - """Validate the course id""" - cleaned_id = self.cleaned_data["course_id"] - try: - course_key = CourseLocator.from_string(cleaned_id) - except InvalidKeyError: - msg = u'Course id invalid. Entered course id was: "{0}."'.format(cleaned_id) - raise forms.ValidationError(msg) - - if not modulestore().has_course(course_key): - msg = u'Course not found. Entered course id was: "{0}". '.format(text_type(course_key)) - raise forms.ValidationError(msg) - - return course_key + """ + Validate the course id + """ + return clean_course_id(self) diff --git a/cms/djangoapps/xblock_config/forms.py b/cms/djangoapps/xblock_config/forms.py index 4ee49b1a60..70d91623f9 100644 --- a/cms/djangoapps/xblock_config/forms.py +++ b/cms/djangoapps/xblock_config/forms.py @@ -7,6 +7,7 @@ from django import forms from opaque_keys import InvalidKeyError from opaque_keys.edx.locator import CourseLocator +from openedx.core.lib.courses import clean_course_id from xblock_config.models import CourseEditLTIFieldsEnabledFlag from xmodule.modulestore.django import modulestore @@ -26,19 +27,4 @@ class CourseEditLTIFieldsEnabledAdminForm(forms.ModelForm): """ Validate the course id """ - cleaned_id = self.cleaned_data["course_id"] - try: - course_key = CourseLocator.from_string(cleaned_id) - except InvalidKeyError: - msg = u'Course id invalid. Entered course id was: "{course_id}."'.format( - course_id=cleaned_id - ) - raise forms.ValidationError(msg) - - if not modulestore().has_course(course_key): - msg = u'Course not found. Entered course id was: "{course_key}". '.format( - course_key=unicode(course_key) - ) - raise forms.ValidationError(msg) - - return course_key + return clean_course_id(self) diff --git a/common/djangoapps/course_modes/admin.py b/common/djangoapps/course_modes/admin.py index cae959a9c2..6687f35d66 100644 --- a/common/djangoapps/course_modes/admin.py +++ b/common/djangoapps/course_modes/admin.py @@ -19,6 +19,7 @@ from course_modes.models import CourseMode, CourseModeExpirationConfig # but the test suite for Studio will fail because # the verification deadline table won't exist. from lms.djangoapps.verify_student import models as verification_models +from openedx.core.lib.courses import clean_course_id from util.date_utils import get_time_display from xmodule.modulestore.django import modulestore @@ -92,16 +93,10 @@ class CourseModeForm(forms.ModelForm): ) def clean_course_id(self): - course_id = self.cleaned_data['course'] - try: - course_key = CourseKey.from_string(course_id) - except InvalidKeyError: - raise forms.ValidationError("Cannot make a valid CourseKey from id {}!".format(course_id)) - - if not modulestore().has_course(course_key): - raise forms.ValidationError("Cannot find course with id {} in the modulestore".format(course_id)) - - return course_key + """ + Validate the course id + """ + return clean_course_id(self) def clean__expiration_datetime(self): """ diff --git a/common/djangoapps/student/admin.py b/common/djangoapps/student/admin.py index 60ddb548bd..fdde9d00f1 100644 --- a/common/djangoapps/student/admin.py +++ b/common/djangoapps/student/admin.py @@ -9,6 +9,7 @@ from django.utils.translation import ugettext_lazy as _ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey +from openedx.core.lib.courses import clean_course_id from student.models import ( CourseAccessRole, CourseEnrollment, @@ -41,23 +42,10 @@ class CourseAccessRoleForm(forms.ModelForm): def clean_course_id(self): """ - Checking course-id format and course exists in module store. - This field can be null. + Validate the course id """ if self.cleaned_data['course_id']: - course_id = self.cleaned_data['course_id'] - - try: - course_key = CourseKey.from_string(course_id) - except InvalidKeyError: - raise forms.ValidationError(u"Invalid CourseID. Please check the format and re-try.") - - if not modulestore().has_course(course_key): - raise forms.ValidationError(u"Cannot find course with id {} in the modulestore".format(course_id)) - - return course_key - - return None + return clean_course_id(self) def clean_org(self): """If org and course-id exists then Check organization name diff --git a/common/djangoapps/student/tests/test_admin_views.py b/common/djangoapps/student/tests/test_admin_views.py index d5886dfed0..980ffcc2b0 100644 --- a/common/djangoapps/student/tests/test_admin_views.py +++ b/common/djangoapps/student/tests/test_admin_views.py @@ -124,7 +124,7 @@ class AdminCourseRolesPageTest(SharedModuleStoreTestCase): response = self.client.post(reverse('admin:student_courseaccessrole_add'), data=data) self.assertContains( response, - 'Cannot find course with id {} in the modulestore'.format( + 'Course not found. Entered course id was: "{}".'.format( course ) ) diff --git a/lms/djangoapps/bulk_email/forms.py b/lms/djangoapps/bulk_email/forms.py index db3d1b24bf..17427c6a7a 100644 --- a/lms/djangoapps/bulk_email/forms.py +++ b/lms/djangoapps/bulk_email/forms.py @@ -10,6 +10,7 @@ from opaque_keys.edx.keys import CourseKey from six import text_type from bulk_email.models import COURSE_EMAIL_MESSAGE_BODY_TAG, CourseAuthorization, CourseEmailTemplate +from openedx.core.lib.courses import clean_course_id from xmodule.modulestore.django import modulestore log = logging.getLogger(__name__) @@ -79,20 +80,7 @@ class CourseAuthorizationAdminForm(forms.ModelForm): fields = '__all__' def clean_course_id(self): - """Validate the course id""" - cleaned_id = self.cleaned_data["course_id"] - try: - course_key = CourseKey.from_string(cleaned_id) - except InvalidKeyError: - msg = u'Course id invalid.' - msg += u' --- Entered course id was: "{0}". '.format(cleaned_id) - msg += 'Please recheck that you have supplied a valid course id.' - raise forms.ValidationError(msg) - - if not modulestore().has_course(course_key): - msg = u'COURSE NOT FOUND' - msg += u' --- Entered course id was: "{0}". '.format(text_type(course_key)) - msg += 'Please recheck that you have supplied a valid course id.' - raise forms.ValidationError(msg) - - return course_key + """ + Validate the course id + """ + return clean_course_id(self) diff --git a/lms/djangoapps/bulk_email/tests/test_forms.py b/lms/djangoapps/bulk_email/tests/test_forms.py index 7bf95f69ab..91948023d4 100644 --- a/lms/djangoapps/bulk_email/tests/test_forms.py +++ b/lms/djangoapps/bulk_email/tests/test_forms.py @@ -78,9 +78,8 @@ class CourseAuthorizationFormTest(ModuleStoreTestCase): # Validation shouldn't work self.assertFalse(form.is_valid()) - msg = u'COURSE NOT FOUND' - msg += u' --- Entered course id was: "{0}". '.format(text_type(bad_id)) - msg += 'Please recheck that you have supplied a valid course id.' + msg = u'Course not found.' + msg += u' Entered course id was: "{0}".'.format(text_type(bad_id)) self.assertEquals(msg, form._errors['course_id'][0]) # pylint: disable=protected-access with self.assertRaisesRegexp( @@ -96,8 +95,7 @@ class CourseAuthorizationFormTest(ModuleStoreTestCase): self.assertFalse(form.is_valid()) msg = u'Course id invalid.' - msg += u' --- Entered course id was: "asd::**!@#$%^&*())//foobar!!". ' - msg += 'Please recheck that you have supplied a valid course id.' + msg += u' Entered course id was: "asd::**!@#$%^&*())//foobar!!".' self.assertEquals(msg, form._errors['course_id'][0]) # pylint: disable=protected-access with self.assertRaisesRegexp( @@ -114,8 +112,7 @@ class CourseAuthorizationFormTest(ModuleStoreTestCase): self.assertFalse(form.is_valid()) error_msg = form._errors['course_id'][0] # pylint: disable=protected-access - self.assertIn(u'--- Entered course id was: "{0}". '.format(self.course.id.run), error_msg) - self.assertIn(u'Please recheck that you have supplied a valid course id.', error_msg) + self.assertIn(u'Entered course id was: "{0}".'.format(self.course.id.run), error_msg) with self.assertRaisesRegexp( ValueError, diff --git a/lms/djangoapps/grades/config/forms.py b/lms/djangoapps/grades/config/forms.py index 4c72b57995..31457ea97c 100644 --- a/lms/djangoapps/grades/config/forms.py +++ b/lms/djangoapps/grades/config/forms.py @@ -9,6 +9,7 @@ from opaque_keys.edx.locator import CourseLocator from six import text_type from lms.djangoapps.grades.config.models import CoursePersistentGradesFlag +from openedx.core.lib.courses import clean_course_id from xmodule.modulestore.django import modulestore log = logging.getLogger(__name__) @@ -22,16 +23,7 @@ class CoursePersistentGradesAdminForm(forms.ModelForm): fields = '__all__' def clean_course_id(self): - """Validate the course id""" - cleaned_id = self.cleaned_data["course_id"] - try: - course_key = CourseLocator.from_string(cleaned_id) - except InvalidKeyError: - msg = u'Course id invalid. Entered course id was: "{0}."'.format(cleaned_id) - raise forms.ValidationError(msg) - - if not modulestore().has_course(course_key): - msg = u'Course not found. Entered course id was: "{0}". '.format(text_type(course_key)) - raise forms.ValidationError(msg) - - return course_key + """ + Validate the course id + """ + return clean_course_id(self) diff --git a/lms/djangoapps/support/tests/test_refund.py b/lms/djangoapps/support/tests/test_refund.py index dfe5508b7e..d2db69eb41 100644 --- a/lms/djangoapps/support/tests/test_refund.py +++ b/lms/djangoapps/support/tests/test_refund.py @@ -87,7 +87,7 @@ class RefundTests(ModuleStoreTestCase): def test_bad_courseid(self): response = self.client.post('/support/refund/', {'course_id': 'foo', 'user': self.student.email}) - self.assertContains(response, 'Invalid course id') + self.assertContains(response, 'Course id invalid') def test_bad_user(self): response = self.client.post('/support/refund/', {'course_id': str(self.course_id), 'user': 'unknown@foo.com'}) diff --git a/lms/djangoapps/support/views/refund.py b/lms/djangoapps/support/views/refund.py index 7448f17981..e88a851608 100644 --- a/lms/djangoapps/support/views/refund.py +++ b/lms/djangoapps/support/views/refund.py @@ -22,6 +22,7 @@ from django.views.generic.edit import FormView from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey +from openedx.core.lib.courses import clean_course_id from student.models import CourseEnrollment from support.decorators import require_support_permission @@ -49,17 +50,9 @@ class RefundForm(forms.Form): def clean_course_id(self): """ - validate course id field + Validate the course id """ - course_id = self.cleaned_data['course_id'] - try: - course_key = CourseKey.from_string(course_id) - except InvalidKeyError: - try: - course_key = CourseKey.from_string(course_id) - except InvalidKeyError: - raise forms.ValidationError(_("Invalid course id")) - return course_key + return clean_course_id(self) def clean(self): """ diff --git a/openedx/core/djangoapps/video_config/forms.py b/openedx/core/djangoapps/video_config/forms.py index 45fb6ad54c..e5b13d2f84 100644 --- a/openedx/core/djangoapps/video_config/forms.py +++ b/openedx/core/djangoapps/video_config/forms.py @@ -11,6 +11,7 @@ from openedx.core.djangoapps.video_config.models import ( CourseHLSPlaybackEnabledFlag, CourseVideoTranscriptEnabledFlag, ) +from openedx.core.lib.courses import clean_course_id from xmodule.modulestore.django import modulestore log = logging.getLogger(__name__) @@ -29,22 +30,7 @@ class CourseSpecificFlagAdminBaseForm(forms.ModelForm): """ Validate the course id """ - cleaned_id = self.cleaned_data["course_id"] - try: - course_key = CourseLocator.from_string(cleaned_id) - except InvalidKeyError: - msg = u'Course id invalid. Entered course id was: "{course_id}."'.format( - course_id=cleaned_id - ) - raise forms.ValidationError(msg) - - if not modulestore().has_course(course_key): - msg = u'Course not found. Entered course id was: "{course_key}". '.format( - course_key=unicode(course_key) - ) - raise forms.ValidationError(msg) - - return course_key + return clean_course_id(self) class CourseHLSPlaybackFlagAdminForm(CourseSpecificFlagAdminBaseForm): diff --git a/openedx/core/lib/courses.py b/openedx/core/lib/courses.py index 5688ff7ae0..929a4ed815 100644 --- a/openedx/core/lib/courses.py +++ b/openedx/core/lib/courses.py @@ -63,7 +63,7 @@ def clean_course_id(model_form, is_required=True): Returns: (CourseKey) The cleaned and validated course_id as a CourseKey. - NOTE: This should ultimately replace all copies of "def clean_course_id". + NOTE: Use this method in model forms instead of a custom "clean_course_id" method! """ cleaned_id = model_form.cleaned_data["course_id"] @@ -74,11 +74,11 @@ def clean_course_id(model_form, is_required=True): try: course_key = CourseKey.from_string(cleaned_id) except InvalidKeyError: - msg = u'Course id invalid. Entered course id was: "{0}."'.format(cleaned_id) + msg = u'Course id invalid. Entered course id was: "{0}".'.format(cleaned_id) raise forms.ValidationError(msg) if not modulestore().has_course(course_key): - msg = u'Course not found. Entered course id was: "{0}". '.format(text_type(course_key)) + msg = u'Course not found. Entered course id was: "{0}".'.format(text_type(course_key)) raise forms.ValidationError(msg) return course_key