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 2fc3b99a1c..6687f35d66 100644 --- a/common/djangoapps/course_modes/admin.py +++ b/common/djangoapps/course_modes/admin.py @@ -1,6 +1,7 @@ from django import forms from django.conf import settings from django.contrib import admin +from django.http.request import QueryDict from django.utils.translation import ugettext_lazy as _ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey @@ -18,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 @@ -51,10 +53,23 @@ class CourseModeForm(forms.ModelForm): ) def __init__(self, *args, **kwargs): + # If args is a QueryDict, then the ModelForm addition request came in as a POST with a course ID string. + # Change the course ID string to a CourseLocator object by copying the QueryDict to make it mutable. + if len(args) > 0 and 'course' in args[0] and isinstance(args[0], QueryDict): + args_copy = args[0].copy() + args_copy['course'] = CourseKey.from_string(args_copy['course']) + args = [args_copy] + super(CourseModeForm, self).__init__(*args, **kwargs) - if self.data.get('course'): - self.data['course'] = CourseKey.from_string(self.data['course']) + try: + if self.data.get('course'): + self.data['course'] = CourseKey.from_string(self.data['course']) + except AttributeError: + # Change the course ID string to a CourseLocator. + # On a POST request, self.data is a QueryDict and is immutable - so this code will fail. + # However, the args copy above before the super() call handles this case. + pass default_tz = timezone(settings.TIME_ZONE) @@ -78,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/course_modes/models.py b/common/djangoapps/course_modes/models.py index 62edaa074d..ac93241157 100644 --- a/common/djangoapps/course_modes/models.py +++ b/common/djangoapps/course_modes/models.py @@ -202,6 +202,9 @@ class CourseMode(models.Model): # Ensure currency is always lowercase. self.clean() # ensure object-level validation is performed before we save. self.currency = self.currency.lower() + if self.id is None: + # If this model has no primary key at save time, it needs to be force-inserted. + force_insert = True super(CourseMode, self).save(force_insert, force_update, using) @property 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/common/djangoapps/student/tests/test_email.py b/common/djangoapps/student/tests/test_email.py index 36e0c2ce1c..67478c0eea 100644 --- a/common/djangoapps/student/tests/test_email.py +++ b/common/djangoapps/student/tests/test_email.py @@ -16,6 +16,7 @@ from mock import Mock, patch from edxmako.shortcuts import render_to_string from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.theming.tests.test_util import with_comprehensive_theme +from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, CacheIsolationMixin from student.models import PendingEmailChange, Registration, UserProfile from student.tests.factories import PendingEmailChangeFactory, RegistrationFactory, UserFactory from student.views import ( @@ -84,7 +85,7 @@ class EmailTestMixin(object): @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') -class ActivationEmailTests(TestCase): +class ActivationEmailTests(CacheIsolationTestCase): """ Test sending of the activation email. """ @@ -164,7 +165,7 @@ class ActivationEmailTests(TestCase): @patch('student.views.login.render_to_string', Mock(side_effect=mock_render_to_string, autospec=True)) @patch('django.contrib.auth.models.User.email_user') -class ReactivationEmailTests(EmailTestMixin, TestCase): +class ReactivationEmailTests(EmailTestMixin, CacheIsolationTestCase): """ Test sending a reactivation email to a user """ @@ -241,7 +242,7 @@ class ReactivationEmailTests(EmailTestMixin, TestCase): self.assertTrue(response_data['success']) -class EmailChangeRequestTests(EventTestMixin, TestCase): +class EmailChangeRequestTests(EventTestMixin, CacheIsolationTestCase): """ Test changing a user's email address """ @@ -365,12 +366,14 @@ class EmailChangeRequestTests(EventTestMixin, TestCase): @patch('django.contrib.auth.models.User.email_user') @patch('student.views.management.render_to_response', Mock(side_effect=mock_render_to_response, autospec=True)) @patch('student.views.management.render_to_string', Mock(side_effect=mock_render_to_string, autospec=True)) -class EmailChangeConfirmationTests(EmailTestMixin, TransactionTestCase): +class EmailChangeConfirmationTests(EmailTestMixin, CacheIsolationMixin, TransactionTestCase): """ Test that confirmation of email change requests function even in the face of exceptions thrown while sending email """ def setUp(self): super(EmailChangeConfirmationTests, self).setUp() + self.clear_caches() + self.addCleanup(self.clear_caches) self.user = UserFactory.create() self.profile = UserProfile.objects.get(user=self.user) self.req_factory = RequestFactory() @@ -380,6 +383,16 @@ class EmailChangeConfirmationTests(EmailTestMixin, TransactionTestCase): self.pending_change_request = PendingEmailChangeFactory.create(user=self.user) self.key = self.pending_change_request.activation_key + @classmethod + def setUpClass(cls): + super(EmailChangeConfirmationTests, cls).setUpClass() + cls.start_cache_isolation() + + @classmethod + def tearDownClass(cls): + cls.end_cache_isolation() + super(EmailChangeConfirmationTests, cls).tearDownClass() + def assertRolledBack(self): """ Assert that no changes to user, profile, or pending email have been made to the db 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/course_api/blocks/tests/test_api.py b/lms/djangoapps/course_api/blocks/tests/test_api.py index 3c3d3eef08..913bc1a085 100644 --- a/lms/djangoapps/course_api/blocks/tests/test_api.py +++ b/lms/djangoapps/course_api/blocks/tests/test_api.py @@ -5,8 +5,8 @@ Tests for Blocks api.py from itertools import product import ddt +import django from django.test.client import RequestFactory - from openedx.core.djangoapps.content.block_structure.api import clear_course_from_cache from openedx.core.djangoapps.content.block_structure.config import STORAGE_BACKING_FOR_CACHE, waffle from student.tests.factories import UserFactory @@ -161,8 +161,20 @@ class TestGetBlocksQueryCounts(SharedModuleStoreTestCase): with waffle().override(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing): course = self._create_course(store_type) clear_course_from_cache(course.id) + + if with_storage_backing: + # TODO: Remove Django 1.11 upgrade shim + # SHIM: Django 1.11 results in a few more SAVEPOINTs due to: + # https://github.com/django/django/commit/d44afd88#diff-5b0dda5eb9a242c15879dc9cd2121379L485 + if django.VERSION >= (1, 11): + num_sql_queries = 16 + else: + num_sql_queries = 14 + else: + num_sql_queries = 6 + self._get_blocks( course, expected_mongo_queries, - expected_sql_queries=14 if with_storage_backing else 6, + expected_sql_queries=num_sql_queries, ) diff --git a/lms/djangoapps/course_goals/api.py b/lms/djangoapps/course_goals/api.py index 3d05885f52..f1a65c0e2b 100644 --- a/lms/djangoapps/course_goals/api.py +++ b/lms/djangoapps/course_goals/api.py @@ -2,6 +2,7 @@ Course Goals Python API """ import models +from six import text_type from opaque_keys.edx.keys import CourseKey from django.conf import settings @@ -22,7 +23,7 @@ def add_course_goal(user, course_id, goal_key): goal_key (string): The goal key for the new goal. """ - course_key = CourseKey.from_string(str(course_id)) + course_key = CourseKey.from_string(text_type(course_id)) current_goal = get_course_goal(user, course_key) if current_goal: # If a course goal already exists, simply update it. diff --git a/lms/djangoapps/experiments/views.py b/lms/djangoapps/experiments/views.py index 90758e3746..e26ce73d1c 100644 --- a/lms/djangoapps/experiments/views.py +++ b/lms/djangoapps/experiments/views.py @@ -46,7 +46,6 @@ class ExperimentDataViewSet(viewsets.ModelViewSet): try: obj = self.get_queryset().get(user=self.request.user, experiment_id=experiment_id, key=key) self.kwargs['pk'] = obj.pk - self.request.data['id'] = obj.pk return self.update(request, *args, **kwargs) except ExperimentData.DoesNotExist: pass 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/grades/tests/test_course_grade_factory.py b/lms/djangoapps/grades/tests/test_course_grade_factory.py index cc2e8c683a..c762899bb1 100644 --- a/lms/djangoapps/grades/tests/test_course_grade_factory.py +++ b/lms/djangoapps/grades/tests/test_course_grade_factory.py @@ -2,6 +2,7 @@ import itertools from nose.plugins.attrib import attr import ddt +import django from courseware.access import has_access from django.conf import settings from lms.djangoapps.grades.config.tests.utils import persistent_grades_feature_flags @@ -94,25 +95,57 @@ class TestCourseGradeFactory(GradeTestBase): with self.assertNumQueries(2), mock_get_score(1, 2): _assert_read(expected_pass=False, expected_percent=0) # start off with grade of 0 - with self.assertNumQueries(29), mock_get_score(1, 2): + # TODO: Remove Django 1.11 upgrade shim + # SHIM: Django 1.11 results in a few more SAVEPOINTs due to: + # https://github.com/django/django/commit/d44afd88#diff-5b0dda5eb9a242c15879dc9cd2121379L485 + if django.VERSION >= (1, 11): + num_queries = 37 + else: + num_queries = 29 + + with self.assertNumQueries(num_queries), mock_get_score(1, 2): grade_factory.update(self.request.user, self.course, force_update_subsections=True) with self.assertNumQueries(2): _assert_read(expected_pass=True, expected_percent=0.5) # updated to grade of .5 - with self.assertNumQueries(4), mock_get_score(1, 4): + # TODO: Remove Django 1.11 upgrade shim + # SHIM: Django 1.11 results in a few more SAVEPOINTs due to: + # https://github.com/django/django/commit/d44afd88#diff-5b0dda5eb9a242c15879dc9cd2121379L485 + if django.VERSION >= (1, 11): + num_queries = 6 + else: + num_queries = 4 + + with self.assertNumQueries(num_queries), mock_get_score(1, 4): grade_factory.update(self.request.user, self.course, force_update_subsections=False) with self.assertNumQueries(2): _assert_read(expected_pass=True, expected_percent=0.5) # NOT updated to grade of .25 - with self.assertNumQueries(12), mock_get_score(2, 2): + # TODO: Remove Django 1.11 upgrade shim + # SHIM: Django 1.11 results in a few more SAVEPOINTs due to: + # https://github.com/django/django/commit/d44afd88#diff-5b0dda5eb9a242c15879dc9cd2121379L485 + if django.VERSION >= (1, 11): + num_queries = 20 + else: + num_queries = 12 + + with self.assertNumQueries(num_queries), mock_get_score(2, 2): grade_factory.update(self.request.user, self.course, force_update_subsections=True) with self.assertNumQueries(2): _assert_read(expected_pass=True, expected_percent=1.0) # updated to grade of 1.0 - with self.assertNumQueries(12), mock_get_score(0, 0): # the subsection now is worth zero + # TODO: Remove Django 1.11 upgrade shim + # SHIM: Django 1.11 results in a few more SAVEPOINTs due to: + # https://github.com/django/django/commit/d44afd88#diff-5b0dda5eb9a242c15879dc9cd2121379L485 + if django.VERSION >= (1, 11): + num_queries = 20 + else: + num_queries = 12 + + with self.assertNumQueries(num_queries), mock_get_score(0, 0): # the subsection now is worth zero grade_factory.update(self.request.user, self.course, force_update_subsections=True) with self.assertNumQueries(2): diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index 21e46e9eb1..763b6d20a4 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/lms/djangoapps/grades/tests/test_tasks.py @@ -10,6 +10,7 @@ from datetime import datetime, timedelta import ddt import pytz import six +import django from django.conf import settings from django.db.utils import IntegrityError from mock import MagicMock, patch @@ -107,6 +108,28 @@ class HasCourseWithProblemsMixin(object): # pylint: enable=attribute-defined-outside-init,no-member +# TODO: Remove Django 1.11 upgrade shim +# SHIM: Django 1.11 results in a few more SAVEPOINTs due to: +# https://github.com/django/django/commit/d44afd88#diff-5b0dda5eb9a242c15879dc9cd2121379L485 +# Get rid of this logic post-upgrade. +def _recalc_expected_query_counts(): + if django.VERSION >= (1, 11): + return 27 + else: + return 23 + + +# TODO: Remove Django 1.11 upgrade shim +# SHIM: Django 1.11 results in a few more SAVEPOINTs due to: +# https://github.com/django/django/commit/d44afd88#diff-5b0dda5eb9a242c15879dc9cd2121379L485 +# Get rid of this logic post-upgrade. +def _recalc_persistent_expected_query_counts(): + if django.VERSION >= (1, 11): + return 28 + else: + return 24 + + @patch.dict(settings.FEATURES, {'PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS': False}) @ddt.ddt class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTestCase): @@ -164,10 +187,10 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest self.assertEquals(mock_block_structure_create.call_count, 1) @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 23, True), - (ModuleStoreEnum.Type.mongo, 1, 23, False), - (ModuleStoreEnum.Type.split, 3, 23, True), - (ModuleStoreEnum.Type.split, 3, 23, False), + (ModuleStoreEnum.Type.mongo, 1, _recalc_expected_query_counts(), True), + (ModuleStoreEnum.Type.mongo, 1, _recalc_expected_query_counts(), False), + (ModuleStoreEnum.Type.split, 3, _recalc_expected_query_counts(), True), + (ModuleStoreEnum.Type.split, 3, _recalc_expected_query_counts(), False), ) @ddt.unpack def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, create_multiple_subsections): @@ -179,8 +202,8 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest self._apply_recalculate_subsection_grade() @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 23), - (ModuleStoreEnum.Type.split, 3, 23), + (ModuleStoreEnum.Type.mongo, 1, _recalc_expected_query_counts()), + (ModuleStoreEnum.Type.split, 3, _recalc_expected_query_counts()), ) @ddt.unpack def test_query_counts_dont_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls): @@ -240,8 +263,8 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest self.assertEqual(len(PersistentSubsectionGrade.bulk_read_grades(self.user.id, self.course.id)), 0) @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 24), - (ModuleStoreEnum.Type.split, 3, 24), + (ModuleStoreEnum.Type.mongo, 1, _recalc_persistent_expected_query_counts()), + (ModuleStoreEnum.Type.split, 3, _recalc_persistent_expected_query_counts()), ) @ddt.unpack def test_persistent_grades_enabled_on_course(self, default_store, num_mongo_queries, num_sql_queries): diff --git a/lms/djangoapps/instructor_analytics/tests/test_basic.py b/lms/djangoapps/instructor_analytics/tests/test_basic.py index 45cecd69af..125cd68748 100644 --- a/lms/djangoapps/instructor_analytics/tests/test_basic.py +++ b/lms/djangoapps/instructor_analytics/tests/test_basic.py @@ -536,10 +536,7 @@ class TestCourseRegistrationCodeAnalyticsBasic(ModuleStoreTestCase): CourseSalesAdminRole(self.course.id).add_users(self.instructor) # Create a paid course mode. - mode = CourseModeFactory.create() - mode.course_id = self.course.id - mode.min_price = 1 - mode.save() + mode = CourseModeFactory.create(course_id=self.course.id, min_price=1) url = reverse('generate_registration_codes', kwargs={'course_id': text_type(self.course.id)}) diff --git a/lms/djangoapps/lti_provider/tests/test_views.py b/lms/djangoapps/lti_provider/tests/test_views.py index 482dc866f6..a2b9ae92bc 100644 --- a/lms/djangoapps/lti_provider/tests/test_views.py +++ b/lms/djangoapps/lti_provider/tests/test_views.py @@ -45,14 +45,18 @@ COURSE_PARAMS = { ALL_PARAMS = dict(LTI_DEFAULT_PARAMS.items() + COURSE_PARAMS.items()) -def build_launch_request(): +def build_launch_request(extra_post_data=None, param_to_delete=None): """ Helper method to create a new request object for the LTI launch. """ - request = RequestFactory().post('/') + if extra_post_data is None: + extra_post_data = {} + post_data = dict(LTI_DEFAULT_PARAMS.items() + extra_post_data.items()) + if param_to_delete: + del post_data[param_to_delete] + request = RequestFactory().post('/', data=post_data) request.user = UserFactory.create() request.session = {} - request.POST.update(LTI_DEFAULT_PARAMS) return request @@ -110,8 +114,7 @@ class LtiLaunchTest(LtiTestMixin, TestCase): """ Helper method to remove a parameter from the LTI launch and call the view """ - request = build_launch_request() - del request.POST[missing_param] + request = build_launch_request(param_to_delete=missing_param) return views.lti_launch(request, None, None) def test_launch_with_missing_parameters(self): @@ -152,8 +155,7 @@ class LtiLaunchTest(LtiTestMixin, TestCase): def test_lti_consumer_record_supplemented_with_guid(self, _render): self.mock_verify.return_value = False - request = build_launch_request() - request.POST.update(LTI_OPTIONAL_PARAMS) + request = build_launch_request(LTI_OPTIONAL_PARAMS) with self.assertNumQueries(3): views.lti_launch(request, None, None) consumer = models.LtiConsumer.objects.get( 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/lms/envs/common.py b/lms/envs/common.py index 05c2bd52ba..3515fd855c 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -668,8 +668,7 @@ derived_collection_entry('DEFAULT_TEMPLATE_ENGINE', 'DIRS') ############################################################################################### -# use the ratelimit backend to prevent brute force attacks -AUTHENTICATION_BACKENDS = ['ratelimitbackend.backends.RateLimitModelBackend'] +AUTHENTICATION_BACKENDS = ['openedx.core.djangoapps.oauth_dispatch.dot_overrides.validators.EdxRateLimitedAllowAllUsersModelBackend'] STUDENT_FILEUPLOAD_MAX_SIZE = 4 * 1000 * 1000 # 4 MB MAX_FILEUPLOADS_PER_INPUT = 20 diff --git a/openedx/core/djangoapps/cors_csrf/authentication.py b/openedx/core/djangoapps/cors_csrf/authentication.py index 55bffe33cb..91dc5098a2 100644 --- a/openedx/core/djangoapps/cors_csrf/authentication.py +++ b/openedx/core/djangoapps/cors_csrf/authentication.py @@ -1,7 +1,8 @@ """ Django Rest Framework Authentication classes for cross-domain end-points. """ - +import django +from django.middleware.csrf import CsrfViewMiddleware from rest_framework import authentication from .helpers import is_cross_domain_request_allowed, skip_cross_domain_referer_check @@ -23,6 +24,12 @@ class SessionAuthenticationCrossDomainCsrf(authentication.SessionAuthentication) Since this subclass overrides only the `enforce_csrf()` method, it can be mixed in with other `SessionAuthentication` subclasses. """ + # TODO: Remove Django 1.11 upgrade shim + # SHIM: Call new process_request in Django 1.11 to process CSRF token in cookie. + def _process_enforce_csrf(self, request): + if django.VERSION >= (1, 11): + CsrfViewMiddleware().process_request(request) + return super(SessionAuthenticationCrossDomainCsrf, self).enforce_csrf(request) def enforce_csrf(self, request): """ @@ -30,6 +37,6 @@ class SessionAuthenticationCrossDomainCsrf(authentication.SessionAuthentication) """ if is_cross_domain_request_allowed(request): with skip_cross_domain_referer_check(request): - return super(SessionAuthenticationCrossDomainCsrf, self).enforce_csrf(request) + return self._process_enforce_csrf(request) else: - return super(SessionAuthenticationCrossDomainCsrf, self).enforce_csrf(request) + return self._process_enforce_csrf(request) diff --git a/openedx/core/djangoapps/external_auth/tests/test_shib.py b/openedx/core/djangoapps/external_auth/tests/test_shib.py index c5baf55f57..fc6b79f3bc 100644 --- a/openedx/core/djangoapps/external_auth/tests/test_shib.py +++ b/openedx/core/djangoapps/external_auth/tests/test_shib.py @@ -526,10 +526,10 @@ class ShibSPTestModifiedCourseware(ModuleStoreTestCase): # Tests the two case for courses, limited and not for course in [shib_course, open_enroll_course]: for student in [shib_student, other_ext_student, int_student]: - request = self.request_factory.post('/change_enrollment') - - request.POST.update({'enrollment_action': 'enroll', - 'course_id': text_type(course.id)}) + request = self.request_factory.post( + '/change_enrollment', + data={'enrollment_action': 'enroll', 'course_id': text_type(course.id)} + ) request.user = student response = change_enrollment(request) # If course is not limited or student has correct shib extauth then enrollment should be allowed diff --git a/openedx/core/djangoapps/oauth_dispatch/dot_overrides/validators.py b/openedx/core/djangoapps/oauth_dispatch/dot_overrides/validators.py index 702d0b4e06..978e8394af 100644 --- a/openedx/core/djangoapps/oauth_dispatch/dot_overrides/validators.py +++ b/openedx/core/djangoapps/oauth_dispatch/dot_overrides/validators.py @@ -5,12 +5,14 @@ from __future__ import unicode_literals from datetime import datetime +import django from django.contrib.auth import authenticate, get_user_model from django.db.models.signals import pre_save from django.dispatch import receiver from oauth2_provider.models import AccessToken from oauth2_provider.oauth2_validators import OAuth2Validator from pytz import utc +from ratelimitbackend.backends import RateLimitMixin from ..models import RestrictedApplication @@ -29,6 +31,30 @@ def on_access_token_presave(sender, instance, *args, **kwargs): # pylint: disab RestrictedApplication.set_access_token_as_expired(instance) +# TODO: Remove Django 1.11 upgrade shim +# SHIM: Allow users that are inactive to still authenticate while keeping rate-limiting functionality. +if django.VERSION < (1, 10): + # Old backend which allowed inactive users to authenticate prior to Django 1.10. + from django.contrib.auth.backends import ModelBackend as UserModelBackend +else: + # Django 1.10+ ModelBackend disallows inactive users from authenticating, so instead we use + # AllowAllUsersModelBackend which is the closest alternative. + from django.contrib.auth.backends import AllowAllUsersModelBackend as UserModelBackend + + +class EdxRateLimitedAllowAllUsersModelBackend(RateLimitMixin, UserModelBackend): + """ + Authentication backend needed to incorporate rate limiting of login attempts - but also + enabling users with is_active of False in the Django auth_user model to still authenticate. + This is necessary for mobile users using 3rd party auth who have not activated their accounts, + Inactive users who use 1st party auth (username/password auth) will still fail login attempts, + just at a higher layer, in the login_user view. + + See: https://openedx.atlassian.net/browse/TNL-4516 + """ + pass + + class EdxOAuth2Validator(OAuth2Validator): """ Validator class that implements edX-specific custom behavior: 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/api/tests/test_authentication.py b/openedx/core/lib/api/tests/test_authentication.py index 6a28112814..9e0475805d 100644 --- a/openedx/core/lib/api/tests/test_authentication.py +++ b/openedx/core/lib/api/tests/test_authentication.py @@ -40,13 +40,13 @@ factory = APIRequestFactory() # pylint: disable=invalid-name class MockView(APIView): # pylint: disable=missing-docstring permission_classes = (IsAuthenticated,) - def get(self, request): # pylint: disable=missing-docstring,unused-argument + def get(self, request): # pylint: disable=unused-argument return HttpResponse({'a': 1, 'b': 2, 'c': 3}) - def post(self, request): # pylint: disable=missing-docstring,unused-argument + def post(self, request): # pylint: disable=unused-argument return HttpResponse({'a': 1, 'b': 2, 'c': 3}) - def put(self, request): # pylint: disable=missing-docstring,unused-argument + def put(self, request): # pylint: disable=unused-argument return HttpResponse({'a': 1, 'b': 2, 'c': 3}) 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 diff --git a/openedx/features/course_experience/__init__.py b/openedx/features/course_experience/__init__.py index b544d264bd..a2dff74d45 100644 --- a/openedx/features/course_experience/__init__.py +++ b/openedx/features/course_experience/__init__.py @@ -86,8 +86,8 @@ class CourseHomeMessages(UserMessageCollection): NAMESPACE = 'course_home_level_messages' @classmethod - def get_namespace(self): + def get_namespace(cls): """ Returns the namespace of the message collection. """ - return self.NAMESPACE + return cls.NAMESPACE diff --git a/openedx/features/course_experience/plugins.py b/openedx/features/course_experience/plugins.py index a47cf7eb4d..50613f9b5e 100644 --- a/openedx/features/course_experience/plugins.py +++ b/openedx/features/course_experience/plugins.py @@ -6,13 +6,13 @@ This includes any locally defined CourseTools. from django.core.urlresolvers import reverse from django.utils.translation import ugettext as _ -from course_tools import CourseTool from courseware.courses import get_course_by_id from student.models import CourseEnrollment -from views.course_reviews import CourseReviewsModuleFragmentView -from views.course_updates import CourseUpdatesFragmentView from . import SHOW_REVIEWS_TOOL_FLAG, UNIFIED_COURSE_TAB_FLAG +from .course_tools import CourseTool +from .views.course_reviews import CourseReviewsModuleFragmentView +from .views.course_updates import CourseUpdatesFragmentView class CourseUpdatesTool(CourseTool): diff --git a/openedx/features/course_experience/tests/views/test_course_outline.py b/openedx/features/course_experience/tests/views/test_course_outline.py index 360034deb1..14e25b25ff 100644 --- a/openedx/features/course_experience/tests/views/test_course_outline.py +++ b/openedx/features/course_experience/tests/views/test_course_outline.py @@ -36,7 +36,6 @@ class TestCourseOutlinePage(SharedModuleStoreTestCase): Set up an array of various courses to be tested. """ # setUpClassAndTestData() already calls setUpClass on SharedModuleStoreTestCase - # pylint: disable=super-method-not-called with super(TestCourseOutlinePage, cls).setUpClassAndTestData(): cls.courses = [] course = CourseFactory.create() @@ -261,7 +260,6 @@ class TestCourseOutlineResumeCourse(SharedModuleStoreTestCase): Creates a test course that can be used for non-destructive tests """ # setUpClassAndTestData() already calls setUpClass on SharedModuleStoreTestCase - # pylint: disable=super-method-not-called with super(TestCourseOutlineResumeCourse, cls).setUpClassAndTestData(): cls.course = cls.create_test_course() @@ -369,7 +367,7 @@ class TestCourseOutlineResumeCourse(SharedModuleStoreTestCase): # remove one of the sequentials from the chapter with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course.id): - self.store.delete_item(sequential.location, self.user.id) # pylint: disable=no-member + self.store.delete_item(sequential.location, self.user.id) # check resume course buttons response = self.client.get(course_home_url(course)) @@ -398,7 +396,7 @@ class TestCourseOutlineResumeCourse(SharedModuleStoreTestCase): # remove all sequentials from chapter with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course.id): for sequential in chapter.children: - self.store.delete_item(sequential.location, self.user.id) # pylint: disable=no-member + self.store.delete_item(sequential.location, self.user.id) # check resume course buttons response = self.client.get(course_home_url(course)) diff --git a/openedx/features/course_experience/urls.py b/openedx/features/course_experience/urls.py index c9e8be4130..002d66b4af 100644 --- a/openedx/features/course_experience/urls.py +++ b/openedx/features/course_experience/urls.py @@ -4,14 +4,14 @@ Defines URLs for the course experience. from django.conf.urls import url -from views.course_dates import CourseDatesFragmentMobileView -from views.course_home import CourseHomeFragmentView, CourseHomeView -from views.course_outline import CourseOutlineFragmentView -from views.course_reviews import CourseReviewsView -from views.course_updates import CourseUpdatesFragmentView, CourseUpdatesView -from views.course_sock import CourseSockFragmentView -from views.latest_update import LatestUpdateFragmentView -from views.welcome_message import WelcomeMessageFragmentView, dismiss_welcome_message +from .views.course_dates import CourseDatesFragmentMobileView +from .views.course_home import CourseHomeFragmentView, CourseHomeView +from .views.course_outline import CourseOutlineFragmentView +from .views.course_reviews import CourseReviewsView +from .views.course_updates import CourseUpdatesFragmentView, CourseUpdatesView +from .views.course_sock import CourseSockFragmentView +from .views.latest_update import LatestUpdateFragmentView +from .views.welcome_message import WelcomeMessageFragmentView, dismiss_welcome_message urlpatterns = [ url( diff --git a/openedx/features/course_experience/views/course_home_messages.py b/openedx/features/course_experience/views/course_home_messages.py index a80c45429b..5dc49afd1d 100644 --- a/openedx/features/course_experience/views/course_home_messages.py +++ b/openedx/features/course_experience/views/course_home_messages.py @@ -8,15 +8,20 @@ from babel.dates import format_date, format_timedelta from django.contrib import auth from django.template.loader import render_to_string from django.utils.http import urlquote_plus -from pytz import UTC -from django.utils.translation import get_language, to_locale from django.utils.translation import ugettext as _ from django.utils.translation import get_language, to_locale from opaque_keys.edx.keys import CourseKey +from pytz import UTC from web_fragments.fragment import Fragment from courseware.courses import get_course_date_blocks, get_course_with_access -from lms.djangoapps.course_goals.api import get_course_goal, get_course_goal_options, valid_course_goals_ordered, get_goal_api_url, has_course_goal_permission +from lms.djangoapps.course_goals.api import ( + get_course_goal, + get_course_goal_options, + get_goal_api_url, + has_course_goal_permission, + valid_course_goals_ordered +) from lms.djangoapps.course_goals.models import GOAL_KEY_CHOICES from openedx.core.djangoapps.plugin_api.views import EdxFragmentView from openedx.core.djangolib.markup import HTML, Text @@ -52,7 +57,9 @@ class CourseHomeMessageFragmentView(EdxFragmentView): # Get time until the start date, if already started, or no start date, value will be zero or negative now = datetime.now(UTC) already_started = course.start and now > course.start - days_until_start_string = "started" if already_started else format_timedelta(course.start - now, locale=to_locale(get_language())) + days_until_start_string = "started" if already_started else format_timedelta( + course.start - now, locale=to_locale(get_language()) + ) course_start_data = { 'course_start_date': format_date(course.start, locale=to_locale(get_language())), 'already_started': already_started,