From 4642efbf0847900b2ce3e9eadf870aa51294555c Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 3 Oct 2018 09:21:24 -0400 Subject: [PATCH 1/6] Create shell django app for course duration feature --- cms/envs/common.py | 2 ++ lms/envs/common.py | 1 + openedx/features/course_duration_limits/__init__.py | 0 openedx/features/course_duration_limits/admin.py | 6 ++++++ openedx/features/course_duration_limits/apps.py | 8 ++++++++ .../course_duration_limits/migrations/__init__.py | 0 openedx/features/course_duration_limits/models.py | 6 ++++++ openedx/features/course_duration_limits/tests.py | 6 ++++++ openedx/features/course_duration_limits/views.py | 6 ++++++ 9 files changed, 35 insertions(+) create mode 100644 openedx/features/course_duration_limits/__init__.py create mode 100644 openedx/features/course_duration_limits/admin.py create mode 100644 openedx/features/course_duration_limits/apps.py create mode 100644 openedx/features/course_duration_limits/migrations/__init__.py create mode 100644 openedx/features/course_duration_limits/models.py create mode 100644 openedx/features/course_duration_limits/tests.py create mode 100644 openedx/features/course_duration_limits/views.py diff --git a/cms/envs/common.py b/cms/envs/common.py index d6b9f33d8e..980e7adb93 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1161,6 +1161,8 @@ INSTALLED_APPS = [ # API Documentation 'rest_framework_swagger', + + 'openedx.features.course_duration_limits', ] diff --git a/lms/envs/common.py b/lms/envs/common.py index 889228d95e..2503421aca 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2282,6 +2282,7 @@ INSTALLED_APPS = [ 'openedx.features.learner_profile', 'openedx.features.learner_analytics', 'openedx.features.portfolio_project', + 'openedx.features.course_duration_limits', 'experiments', diff --git a/openedx/features/course_duration_limits/__init__.py b/openedx/features/course_duration_limits/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/openedx/features/course_duration_limits/admin.py b/openedx/features/course_duration_limits/admin.py new file mode 100644 index 0000000000..13be29d96f --- /dev/null +++ b/openedx/features/course_duration_limits/admin.py @@ -0,0 +1,6 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.contrib import admin + +# Register your models here. diff --git a/openedx/features/course_duration_limits/apps.py b/openedx/features/course_duration_limits/apps.py new file mode 100644 index 0000000000..a0f1f4b2d7 --- /dev/null +++ b/openedx/features/course_duration_limits/apps.py @@ -0,0 +1,8 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.apps import AppConfig + + +class CourseDurationLimitsConfig(AppConfig): + name = 'openedx.features.course_duration_limits' diff --git a/openedx/features/course_duration_limits/migrations/__init__.py b/openedx/features/course_duration_limits/migrations/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/openedx/features/course_duration_limits/models.py b/openedx/features/course_duration_limits/models.py new file mode 100644 index 0000000000..1dfab76043 --- /dev/null +++ b/openedx/features/course_duration_limits/models.py @@ -0,0 +1,6 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import models + +# Create your models here. diff --git a/openedx/features/course_duration_limits/tests.py b/openedx/features/course_duration_limits/tests.py new file mode 100644 index 0000000000..5982e6bcd2 --- /dev/null +++ b/openedx/features/course_duration_limits/tests.py @@ -0,0 +1,6 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.test import TestCase + +# Create your tests here. diff --git a/openedx/features/course_duration_limits/views.py b/openedx/features/course_duration_limits/views.py new file mode 100644 index 0000000000..e784a0bd2c --- /dev/null +++ b/openedx/features/course_duration_limits/views.py @@ -0,0 +1,6 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.shortcuts import render + +# Create your views here. From 97a659c416444c25e553061accdb40a28e781b16 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 2 Oct 2018 13:58:20 -0400 Subject: [PATCH 2/6] Limit access for Audit users to courses after a specified period of time --- lms/djangoapps/courseware/access.py | 57 +++++++++++++++---- .../features/course_duration_limits/access.py | 41 +++++++++++++ 2 files changed, 88 insertions(+), 10 deletions(-) create mode 100644 openedx/features/course_duration_limits/access.py diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 7e40b4f0c8..91360ef180 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -11,7 +11,7 @@ Note: The access control logic in this file does NOT check for enrollment in It is a wrapper around has_access that additionally checks for enrollment. """ import logging -from datetime import datetime +from datetime import datetime, timedelta from django.conf import settings from django.contrib.auth.models import AnonymousUser @@ -40,6 +40,7 @@ from lms.djangoapps.ccx.models import CustomCourseForEdX from mobile_api.models import IgnoreMobileAvailableFlagConfig from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.external_auth.models import ExternalAuthMap +from openedx.features.course_duration_limits.access import check_course_expired from student import auth from student.models import CourseEnrollmentAllowed from student.roles import ( @@ -318,16 +319,52 @@ def _has_access_course(user, action, courselike): NOTE: this is not checking whether user is actually enrolled in the course. """ - response = ( - _visible_to_nonstaff_users(courselike) and - check_course_open_for_learner(user, courselike) and - _can_view_courseware_with_prerequisites(user, courselike) - ) + # N.B. I'd love a better way to handle this pattern, without breaking the + # shortcircuiting logic. Maybe AccessResponse needs to grow a + # fluent interface? + # + # return ( + # _visible_to_nonstaff_users(courselike).and( + # check_course_open_for_learner, user, courselike + # ).and( + # _can_view_courseware_with_prerequisites, user, courselike + # ) + # ).or( + # _has_staff_access_to_descriptor, user, courselike, courselike.id + # ) + visible_to_nonstaff = _visible_to_nonstaff_users(courselike) + if not visible_to_nonstaff: + staff_access = _has_staff_access_to_descriptor(user, courselike, courselike.id) + if staff_access: + return staff_access + else: + return visible_to_nonstaff - return ( - ACCESS_GRANTED if (response or _has_staff_access_to_descriptor(user, courselike, courselike.id)) - else response - ) + open_for_learner = check_course_open_for_learner(user, courselike) + if not open_for_learner: + staff_access = _has_staff_access_to_descriptor(user, courselike, courselike.id) + if staff_access: + return staff_access + else: + return open_for_learner + + view_with_prereqs = _can_view_courseware_with_prerequisites(user, courselike) + if not view_with_prereqs: + staff_access = _has_staff_access_to_descriptor(user, courselike, courselike.id) + if staff_access: + return staff_access + else: + return view_with_prereqs + + has_not_expired = check_course_expired(user, courselike) + if not has_not_expired: + staff_access = _has_staff_access_to_descriptor(user, courselike, courselike.id) + if staff_access: + return staff_access + else: + return has_not_expired + + return ACCESS_GRANTED def can_enroll(): """ diff --git a/openedx/features/course_duration_limits/access.py b/openedx/features/course_duration_limits/access.py new file mode 100644 index 0000000000..866da292a9 --- /dev/null +++ b/openedx/features/course_duration_limits/access.py @@ -0,0 +1,41 @@ +from datetime import timedelta + +from django.apps import apps +from django.utils import timezone +from django.utils.translation import ugettext as _ + + +from lms.djangoapps.courseware.access_response import AccessError +from lms.djangoapps.courseware.access_utils import ACCESS_GRANTED + + +class AuditExpiredError(AccessError): + """ + Access denied because the user's audit timespan has expired + """ + def __init__(self, user, course, end_date): + error_code = "audit_expired" + developer_message = "User {} had access to {} until {}".format(user, course, end_date) + user_message = _("Your audit access period has expired.") + super(AuditExpiredError, self).__init__(error_code, developer_message, user_message) + + +def check_course_expired(user, course): + # TODO: Only limit audit users + # TODO: Limit access to instructor paced courses based on end-date, rather than content availability date + CourseEnrollment = apps.get_model('student.CourseEnrollment') + enrollment = CourseEnrollment.get_enrollment(user, course.id) + if enrollment is None: + return ACCESS_GRANTED + + try: + start_date = enrollment.schedule.start + except CourseEnrollment.schedule.RelatedObjectDoesNotExist: + start_date = max(enrollment.created, course.start) + + end_date = start_date + timedelta(days=28) + + if timezone.now() > end_date: + return AuditExpiredError(user, course, end_date) + + return ACCESS_GRANTED From 3e87cb7c7d4a569fe2ae587bc428733ee61cd708 Mon Sep 17 00:00:00 2001 From: Matthew Piatetsky Date: Fri, 12 Oct 2018 13:48:57 -0400 Subject: [PATCH 3/6] fix expired message on dashboard and some of the tests --- .../tests/test_field_override_performance.py | 54 +++++++++---------- lms/djangoapps/courseware/access.py | 2 +- .../courseware/tests/test_access.py | 14 ++--- .../courseware/tests/test_course_info.py | 4 +- lms/djangoapps/courseware/tests/test_views.py | 12 ++--- lms/djangoapps/discussion/tests/test_views.py | 16 +++--- .../django_comment_client/base/tests.py | 8 +-- .../dashboard/_dashboard_course_listing.html | 27 +++++----- .../features/course_duration_limits/access.py | 11 +++- .../features/course_duration_limits/admin.py | 6 --- .../features/course_duration_limits/apps.py | 3 ++ .../migrations/__init__.py | 0 .../features/course_duration_limits/models.py | 6 --- .../features/course_duration_limits/tests.py | 6 --- .../features/course_duration_limits/views.py | 6 --- .../tests/views/test_course_home.py | 2 +- .../tests/views/test_course_updates.py | 2 +- 17 files changed, 84 insertions(+), 95 deletions(-) delete mode 100644 openedx/features/course_duration_limits/admin.py delete mode 100644 openedx/features/course_duration_limits/migrations/__init__.py delete mode 100644 openedx/features/course_duration_limits/models.py delete mode 100644 openedx/features/course_duration_limits/tests.py delete mode 100644 openedx/features/course_duration_limits/views.py diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index 672c9bc38c..1e91d7e15f 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -235,18 +235,18 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): # # of sql queries to default, # # of mongo queries, # ) - ('no_overrides', 1, True, False): (18, 1), - ('no_overrides', 2, True, False): (18, 1), - ('no_overrides', 3, True, False): (18, 1), - ('ccx', 1, True, False): (18, 1), - ('ccx', 2, True, False): (18, 1), - ('ccx', 3, True, False): (18, 1), - ('no_overrides', 1, False, False): (18, 1), - ('no_overrides', 2, False, False): (18, 1), - ('no_overrides', 3, False, False): (18, 1), - ('ccx', 1, False, False): (18, 1), - ('ccx', 2, False, False): (18, 1), - ('ccx', 3, False, False): (18, 1), + ('no_overrides', 1, True, False): (20, 1), + ('no_overrides', 2, True, False): (20, 1), + ('no_overrides', 3, True, False): (20, 1), + ('ccx', 1, True, False): (20, 1), + ('ccx', 2, True, False): (20, 1), + ('ccx', 3, True, False): (20, 1), + ('no_overrides', 1, False, False): (20, 1), + ('no_overrides', 2, False, False): (20, 1), + ('no_overrides', 3, False, False): (20, 1), + ('ccx', 1, False, False): (20, 1), + ('ccx', 2, False, False): (20, 1), + ('ccx', 3, False, False): (20, 1), } @@ -258,19 +258,19 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): __test__ = True TEST_DATA = { - ('no_overrides', 1, True, False): (18, 3), - ('no_overrides', 2, True, False): (18, 3), - ('no_overrides', 3, True, False): (18, 3), - ('ccx', 1, True, False): (18, 3), - ('ccx', 2, True, False): (18, 3), - ('ccx', 3, True, False): (18, 3), - ('ccx', 1, True, True): (19, 3), - ('ccx', 2, True, True): (19, 3), - ('ccx', 3, True, True): (19, 3), - ('no_overrides', 1, False, False): (18, 3), - ('no_overrides', 2, False, False): (18, 3), - ('no_overrides', 3, False, False): (18, 3), - ('ccx', 1, False, False): (18, 3), - ('ccx', 2, False, False): (18, 3), - ('ccx', 3, False, False): (18, 3), + ('no_overrides', 1, True, False): (20, 3), + ('no_overrides', 2, True, False): (20, 3), + ('no_overrides', 3, True, False): (20, 3), + ('ccx', 1, True, False): (20, 3), + ('ccx', 2, True, False): (20, 3), + ('ccx', 3, True, False): (20, 3), + ('ccx', 1, True, True): (21, 3), + ('ccx', 2, True, True): (21, 3), + ('ccx', 3, True, True): (21, 3), + ('no_overrides', 1, False, False): (20, 3), + ('no_overrides', 2, False, False): (20, 3), + ('no_overrides', 3, False, False): (20, 3), + ('ccx', 1, False, False): (20, 3), + ('ccx', 2, False, False): (20, 3), + ('ccx', 3, False, False): (20, 3), } diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 91360ef180..ae822e836b 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -11,7 +11,7 @@ Note: The access control logic in this file does NOT check for enrollment in It is a wrapper around has_access that additionally checks for enrollment. """ import logging -from datetime import datetime, timedelta +from datetime import datetime from django.conf import settings from django.contrib.auth.models import AnonymousUser diff --git a/lms/djangoapps/courseware/tests/test_access.py b/lms/djangoapps/courseware/tests/test_access.py index 260101cb74..9020bcaf53 100644 --- a/lms/djangoapps/courseware/tests/test_access.py +++ b/lms/djangoapps/courseware/tests/test_access.py @@ -836,15 +836,15 @@ class CourseOverviewAccessTestCase(ModuleStoreTestCase): user = getattr(self, user_attr_name) user = User.objects.get(id=user.id) - if (user_attr_name == 'user_staff' and - action == 'see_exists' and - course_attr_name in - ['course_default', 'course_not_started']): + if user_attr_name == 'user_staff' and action == 'see_exists': # checks staff role num_queries = 1 - elif user_attr_name == 'user_normal' and action == 'see_exists' and course_attr_name != 'course_started': - # checks staff role and enrollment data - num_queries = 2 + elif user_attr_name == 'user_normal' and action == 'see_exists': + if course_attr_name == 'course_started': + num_queries = 1 + else: + # checks staff role and enrollment data + num_queries = 2 else: num_queries = 0 diff --git a/lms/djangoapps/courseware/tests/test_course_info.py b/lms/djangoapps/courseware/tests/test_course_info.py index 1d15f3b60f..8b2ead3fa5 100644 --- a/lms/djangoapps/courseware/tests/test_course_info.py +++ b/lms/djangoapps/courseware/tests/test_course_info.py @@ -431,7 +431,7 @@ class SelfPacedCourseInfoTestCase(LoginEnrollmentTestCase, SharedModuleStoreTest self.assertEqual(resp.status_code, 200) def test_num_queries_instructor_paced(self): - self.fetch_course_info_with_queries(self.instructor_paced_course, 28, 3) + self.fetch_course_info_with_queries(self.instructor_paced_course, 29, 3) def test_num_queries_self_paced(self): - self.fetch_course_info_with_queries(self.self_paced_course, 28, 3) + self.fetch_course_info_with_queries(self.self_paced_course, 29, 3) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index be6d9d92ff..f86a926430 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -205,8 +205,8 @@ class IndexQueryTestCase(ModuleStoreTestCase): NUM_PROBLEMS = 20 @ddt.data( - (ModuleStoreEnum.Type.mongo, 10, 147), - (ModuleStoreEnum.Type.split, 4, 147), + (ModuleStoreEnum.Type.mongo, 10, 157), + (ModuleStoreEnum.Type.split, 4, 153), ) @ddt.unpack def test_index_query_counts(self, store_type, expected_mongo_query_count, expected_mysql_query_count): @@ -1430,8 +1430,8 @@ class ProgressPageTests(ProgressPageBaseTests): self.assertContains(resp, u"Download Your Certificate") @ddt.data( - (True, 38), - (False, 37) + (True, 40), + (False, 39) ) @ddt.unpack def test_progress_queries_paced_courses(self, self_paced, query_count): @@ -1442,8 +1442,8 @@ class ProgressPageTests(ProgressPageBaseTests): @patch.dict(settings.FEATURES, {'ASSUME_ZERO_GRADE_IF_ABSENT_FOR_ALL_TESTS': False}) @ddt.data( - (False, 45, 28), - (True, 37, 24) + (False, 47, 30), + (True, 39, 26) ) @ddt.unpack def test_progress_queries(self, enable_waffle, initial, subsequent): diff --git a/lms/djangoapps/discussion/tests/test_views.py b/lms/djangoapps/discussion/tests/test_views.py index 375fb79800..b6a4c2be33 100644 --- a/lms/djangoapps/discussion/tests/test_views.py +++ b/lms/djangoapps/discussion/tests/test_views.py @@ -430,18 +430,18 @@ class SingleThreadQueryCountTestCase(ForumsEnableMixin, ModuleStoreTestCase): # course is outside the context manager that is verifying the number of queries, # and with split mongo, that method ends up querying disabled_xblocks (which is then # cached and hence not queried as part of call_single_thread). - (ModuleStoreEnum.Type.mongo, False, 1, 5, 2, 17, 5), - (ModuleStoreEnum.Type.mongo, False, 50, 5, 2, 17, 5), + (ModuleStoreEnum.Type.mongo, False, 1, 5, 2, 19, 7), + (ModuleStoreEnum.Type.mongo, False, 50, 5, 2, 19, 7), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, False, 1, 3, 3, 17, 5), - (ModuleStoreEnum.Type.split, False, 50, 3, 3, 17, 5), + (ModuleStoreEnum.Type.split, False, 1, 3, 3, 19, 7), + (ModuleStoreEnum.Type.split, False, 50, 3, 3, 19, 7), # Enabling Enterprise integration should have no effect on the number of mongo queries made. - (ModuleStoreEnum.Type.mongo, True, 1, 5, 2, 17, 5), - (ModuleStoreEnum.Type.mongo, True, 50, 5, 2, 17, 5), + (ModuleStoreEnum.Type.mongo, True, 1, 5, 2, 19, 7), + (ModuleStoreEnum.Type.mongo, True, 50, 5, 2, 19, 7), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, True, 1, 3, 3, 17, 5), - (ModuleStoreEnum.Type.split, True, 50, 3, 3, 17, 5), + (ModuleStoreEnum.Type.split, True, 1, 3, 3, 19, 7), + (ModuleStoreEnum.Type.split, True, 50, 3, 3, 19, 7), ) @ddt.unpack def test_number_of_mongo_queries( diff --git a/lms/djangoapps/django_comment_client/base/tests.py b/lms/djangoapps/django_comment_client/base/tests.py index 07d60509ee..26ae5a59d9 100644 --- a/lms/djangoapps/django_comment_client/base/tests.py +++ b/lms/djangoapps/django_comment_client/base/tests.py @@ -403,8 +403,8 @@ class ViewsQueryCountTestCase( return inner @ddt.data( - (ModuleStoreEnum.Type.mongo, 3, 4, 35), - (ModuleStoreEnum.Type.split, 3, 13, 35), + (ModuleStoreEnum.Type.mongo, 3, 4, 37), + (ModuleStoreEnum.Type.split, 3, 13, 37), ) @ddt.unpack @count_queries @@ -412,8 +412,8 @@ class ViewsQueryCountTestCase( self.create_thread_helper(mock_request) @ddt.data( - (ModuleStoreEnum.Type.mongo, 3, 3, 31), - (ModuleStoreEnum.Type.split, 3, 10, 31), + (ModuleStoreEnum.Type.mongo, 3, 3, 33), + (ModuleStoreEnum.Type.split, 3, 10, 33), ) @ddt.unpack @count_queries diff --git a/lms/templates/dashboard/_dashboard_course_listing.html b/lms/templates/dashboard/_dashboard_course_listing.html index 1b16d0ba33..e1472f3a8f 100644 --- a/lms/templates/dashboard/_dashboard_course_listing.html +++ b/lms/templates/dashboard/_dashboard_course_listing.html @@ -33,6 +33,8 @@ from util.course import get_link_for_about_page, get_encoded_course_sharing_utm_ if cert_name_long == "": cert_name_long = settings.CERT_NAME_LONG billing_email = settings.PAYMENT_SUPPORT_EMAIL + + is_course_expired = getattr(show_courseware_link, 'error_code') == 'audit_expired' %> <%namespace name='static' file='../static_content.html'/> @@ -65,7 +67,7 @@ from util.course import get_link_for_about_page, get_encoded_course_sharing_utm_

${_('Course details')}