From 3915c47d717375d6b68e19f1cf0cb0e8ae7549c2 Mon Sep 17 00:00:00 2001 From: John Eskew Date: Wed, 10 Jan 2018 16:27:26 -0500 Subject: [PATCH 01/13] Bump query counts for grading in Django 1.11. --- lms/djangoapps/grades/tests/test_tasks.py | 39 ++++++++++++++++++----- 1 file changed, 31 insertions(+), 8 deletions(-) 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): From 2eab707cc0ccce93aeefbfafd8d0f3421d7ae6f7 Mon Sep 17 00:00:00 2001 From: John Eskew Date: Thu, 11 Jan 2018 11:24:14 -0500 Subject: [PATCH 02/13] Change str conversion to text_type. --- lms/djangoapps/course_goals/api.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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. From c55aa9e1d5e11793eb21837d69c625a692d505b8 Mon Sep 17 00:00:00 2001 From: John Eskew Date: Thu, 11 Jan 2018 12:03:38 -0500 Subject: [PATCH 03/13] Change POST request upon creation only. --- lms/djangoapps/lti_provider/tests/test_views.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) 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( From efbde03ce085345bec97e09dd510952202be0427 Mon Sep 17 00:00:00 2001 From: John Eskew Date: Thu, 11 Jan 2018 15:33:39 -0500 Subject: [PATCH 04/13] Move param addition for POST request to creation. --- openedx/core/djangoapps/external_auth/tests/test_shib.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 From 56f9efcc4c1c601dbaab268b5fa77167cc1dc530 Mon Sep 17 00:00:00 2001 From: John Eskew Date: Fri, 12 Jan 2018 16:29:15 -0500 Subject: [PATCH 05/13] Create course mode in one line without extra save. --- lms/djangoapps/instructor_analytics/tests/test_basic.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) 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)}) From ff545e2a60cc35537626acd16fb6fc39badef560 Mon Sep 17 00:00:00 2001 From: John Eskew Date: Fri, 12 Jan 2018 18:24:00 -0500 Subject: [PATCH 06/13] Handle different cookie processing for Django 1.11 --- openedx/core/djangoapps/cors_csrf/authentication.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) 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) From 6d061a16c5358f854c5c0c6ca73895b44e193205 Mon Sep 17 00:00:00 2001 From: Troy Sankey Date: Wed, 17 Jan 2018 14:31:57 -0500 Subject: [PATCH 07/13] Enable cache isolation for test_email tests Cache isolation is provided by CacheIsolationMixin/CacheIsolationTestCase, and was enabled in these tests in order to stop the 'site' variable in the edxmako template context from bleeding into subsequent tests via the request cache. --- common/djangoapps/student/tests/test_email.py | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) 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 From b1f0b7d230272389097d981b659beda670cb7b71 Mon Sep 17 00:00:00 2001 From: John Eskew Date: Wed, 17 Jan 2018 18:56:22 -0500 Subject: [PATCH 08/13] Convert course id to locator differently to accomodate Django 1.11 --- common/djangoapps/course_modes/admin.py | 18 ++++++++++++++++-- common/djangoapps/course_modes/models.py | 3 +++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/common/djangoapps/course_modes/admin.py b/common/djangoapps/course_modes/admin.py index 2fc3b99a1c..cae959a9c2 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 @@ -51,10 +52,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) diff --git a/common/djangoapps/course_modes/models.py b/common/djangoapps/course_modes/models.py index 3105f1c050..74cb55fb4e 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 From 875a127f84cbeae44e740849e0fe7b5bb2a3d5b4 Mon Sep 17 00:00:00 2001 From: John Eskew Date: Wed, 17 Jan 2018 19:25:27 -0500 Subject: [PATCH 09/13] Pylint fixes to move number below allowed threshold. --- .../core/lib/api/tests/test_authentication.py | 6 +++--- openedx/features/course_experience/__init__.py | 4 ++-- openedx/features/course_experience/plugins.py | 6 +++--- .../tests/views/test_course_outline.py | 6 ++---- openedx/features/course_experience/urls.py | 16 ++++++++-------- .../views/course_home_messages.py | 15 +++++++++++---- 6 files changed, 29 insertions(+), 24 deletions(-) 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/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, From 27edca3c5e371bee517c0fa01e1178bd1aced63e Mon Sep 17 00:00:00 2001 From: John Eskew Date: Thu, 18 Jan 2018 14:25:57 -0500 Subject: [PATCH 10/13] 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 From 1deb26e1909d30892360a75733ad480393e85290 Mon Sep 17 00:00:00 2001 From: Troy Sankey Date: Fri, 19 Jan 2018 15:42:21 -0500 Subject: [PATCH 11/13] avoid unnecessary attempt to update an immutable dict request.data has become immutable under Django 1.11 for some reason. Regardless, setting the 'id' key should be extraneous because we already instruct the update method to use a specific pk (see kwargs) which should be sufficient to locate the database record and update it. --- lms/djangoapps/experiments/views.py | 1 - 1 file changed, 1 deletion(-) 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 From e649d2d782f744f7f610592fc7b0845564d5c2cb Mon Sep 17 00:00:00 2001 From: Troy Sankey Date: Wed, 24 Jan 2018 15:53:16 -0500 Subject: [PATCH 12/13] Shim query count assertions due to added SAVEPOINTs --- .../course_api/blocks/tests/test_api.py | 16 +++++++- .../grades/tests/test_course_grade_factory.py | 41 +++++++++++++++++-- 2 files changed, 51 insertions(+), 6 deletions(-) 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/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): From 5a71fa1e33aaa3a28938d1506cfbbfee9c19f4d1 Mon Sep 17 00:00:00 2001 From: John Eskew Date: Thu, 11 Jan 2018 15:05:02 -0500 Subject: [PATCH 13/13] Allow inactive users to authenticate in Django 1.10+ --- lms/envs/common.py | 3 +-- .../dot_overrides/validators.py | 26 +++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/lms/envs/common.py b/lms/envs/common.py index 14548102ee..e386b85374 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/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: