diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index 3dd8407b4c..36aeb39ee9 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -239,7 +239,7 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): __test__ = True # TODO: decrease query count as part of REVO-28 - QUERY_COUNT = 36 + QUERY_COUNT = 30 TEST_DATA = { # (providers, course_width, enable_ccx, view_as_ccx): ( # # of sql queries to default, @@ -268,7 +268,7 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): __test__ = True # TODO: decrease query count as part of REVO-28 - QUERY_COUNT = 36 + QUERY_COUNT = 30 TEST_DATA = { ('no_overrides', 1, True, False): (QUERY_COUNT, 3), ('no_overrides', 2, True, False): (QUERY_COUNT, 3), diff --git a/lms/djangoapps/courseware/tests/test_course_info.py b/lms/djangoapps/courseware/tests/test_course_info.py index 26d9807a63..dd66ac573c 100644 --- a/lms/djangoapps/courseware/tests/test_course_info.py +++ b/lms/djangoapps/courseware/tests/test_course_info.py @@ -430,8 +430,8 @@ class SelfPacedCourseInfoTestCase(LoginEnrollmentTestCase, SharedModuleStoreTest def test_num_queries_instructor_paced(self): # TODO: decrease query count as part of REVO-28 - self.fetch_course_info_with_queries(self.instructor_paced_course, 44, 3) + self.fetch_course_info_with_queries(self.instructor_paced_course, 41, 3) def test_num_queries_self_paced(self): # TODO: decrease query count as part of REVO-28 - self.fetch_course_info_with_queries(self.self_paced_course, 44, 3) + self.fetch_course_info_with_queries(self.self_paced_course, 41, 3) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index a020a5af79..344a78b4fd 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -213,8 +213,8 @@ class IndexQueryTestCase(ModuleStoreTestCase): NUM_PROBLEMS = 20 @ddt.data( - (ModuleStoreEnum.Type.mongo, 10, 179), - (ModuleStoreEnum.Type.split, 4, 173), + (ModuleStoreEnum.Type.mongo, 10, 176), + (ModuleStoreEnum.Type.split, 4, 170), ) @ddt.unpack def test_index_query_counts(self, store_type, expected_mongo_query_count, expected_mysql_query_count): @@ -1465,8 +1465,8 @@ class ProgressPageTests(ProgressPageBaseTests): self.assertContains(resp, u"Download Your Certificate") @ddt.data( - (True, 56), - (False, 55) + (True, 50), + (False, 49) ) @ddt.unpack def test_progress_queries_paced_courses(self, self_paced, query_count): @@ -1479,8 +1479,8 @@ class ProgressPageTests(ProgressPageBaseTests): @patch.dict(settings.FEATURES, {'ASSUME_ZERO_GRADE_IF_ABSENT_FOR_ALL_TESTS': False}) @ddt.data( - (False, 64, 44), - (True, 55, 39) + (False, 58, 38), + (True, 49, 33) ) @ddt.unpack def test_progress_queries(self, enable_waffle, initial, subsequent): diff --git a/lms/djangoapps/grades/tests/test_course_grade_factory.py b/lms/djangoapps/grades/tests/test_course_grade_factory.py index 7206db0bcb..2a64c2b0fd 100644 --- a/lms/djangoapps/grades/tests/test_course_grade_factory.py +++ b/lms/djangoapps/grades/tests/test_course_grade_factory.py @@ -95,7 +95,7 @@ class TestCourseGradeFactory(GradeTestBase): [self.sequence.display_name, self.sequence2.display_name] ) - with self.assertNumQueries(5), mock_get_score(1, 2): + with self.assertNumQueries(4), mock_get_score(1, 2): _assert_read(expected_pass=False, expected_percent=0) # start off with grade of 0 num_queries = 47 @@ -313,7 +313,7 @@ class TestGradeIteration(SharedModuleStoreTestCase): else mock_course_grade.return_value for student in self.students ] - with self.assertNumQueries(7): + with self.assertNumQueries(8): all_course_grades, all_errors = self._course_grades_and_errors_for(self.course, self.students) self.assertEqual( {student: text_type(all_errors[student]) for student in all_errors}, diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index d7d0baf5e4..fbf8047118 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -413,7 +413,7 @@ class TestInstructorGradeReport(InstructorGradeReportTestCase): RequestCache.clear_all_namespaces() - expected_query_count = 48 + expected_query_count = 49 with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'): with check_mongo_calls(mongo_count): with self.assertNumQueries(expected_query_count): diff --git a/lms/djangoapps/support/views/feature_based_enrollments.py b/lms/djangoapps/support/views/feature_based_enrollments.py index 1a73f81540..89c3ab5583 100644 --- a/lms/djangoapps/support/views/feature_based_enrollments.py +++ b/lms/djangoapps/support/views/feature_based_enrollments.py @@ -44,27 +44,25 @@ class FeatureBasedEnrollmentsSupportView(View): course = CourseOverview.objects.values('display_name').get(id=key) duration_config = CourseDurationLimitConfig.current(course_key=key) gating_config = ContentTypeGatingConfig.current(course_key=key) - partially_enabled = bool(duration_config.enabled) != bool(gating_config.enabled) + duration_enabled = CourseDurationLimitConfig.enabled_for_course(course_key=key) + gating_enabled = ContentTypeGatingConfig.enabled_for_course(course_key=key) - if partially_enabled: - if duration_config.enabled: - enabled = 'Course Duration Limits Only' - enabled_as_of = str(duration_config.enabled_as_of) if duration_config.enabled_as_of else 'N/A' - reason = 'Course duration limits are enabled for this course, but content type gating is disabled.' - elif gating_config.enabled: - enabled = 'Content Type Gating Only' - enabled_as_of = str(gating_config.enabled_as_of) if gating_config.enabled_as_of else 'N/A' - reason = 'Content type gating is enabled for this course, but course duration limits are disabled.' - else: - enabled = duration_config.enabled or False - enabled_as_of = str(duration_config.enabled_as_of) if duration_config.enabled_as_of else 'N/A' - reason = duration_config.provenances['enabled'] + gating_dict = { + 'enabled': gating_enabled, + 'enabled_as_of': str(gating_config.enabled_as_of) if gating_config.enabled_as_of else 'N/A', + 'reason': gating_config.provenances['enabled'].value + } + duration_dict = { + 'enabled': duration_enabled, + 'enabled_as_of': str(duration_config.enabled_as_of) if duration_config.enabled_as_of else 'N/A', + 'reason': duration_config.provenances['enabled'].value + } return { 'course_id': course_key, 'course_name': course.get('display_name'), - 'gating_config': gating_config, - 'duration_config': duration_config, + 'gating_config': gating_dict, + 'duration_config': duration_dict, } except (ObjectDoesNotExist, InvalidKeyError): diff --git a/lms/templates/support/feature_based_enrollments.html b/lms/templates/support/feature_based_enrollments.html index ed28720be4..159e63b487 100644 --- a/lms/templates/support/feature_based_enrollments.html +++ b/lms/templates/support/feature_based_enrollments.html @@ -49,17 +49,17 @@ ${_("Feature Based Enrollments")} ${results['course_id']} ${results['course_name']} - ${results['gating_config'].enabled if results['gating_config'].enabled is not None else 'Unknown'} + ${results['gating_config']['enabled']} - ${results['gating_config'].enabled_as_of if results['gating_config'].enabled_as_of is not None else 'N/A'} + ${results['gating_config'].get('enabled_as_of', 'N/A') if results['gating_config']['enabled'] else 'N/A'} - ${results['gating_config'].provenances['enabled'].value} + ${results['gating_config']['reason']} - ${results['duration_config'].enabled if results['duration_config'].enabled is not None else 'Unknown'} - ${results['duration_config'].enabled_as_of if results['duration_config'].enabled_as_of is not None else 'N/A'} - ${results['duration_config'].provenances['enabled'].value} + ${results['duration_config']['enabled']} + ${results['duration_config'].get('enabled_as_of', 'N/A') if results['duration_config']['enabled'] else 'N/A'} + ${results['duration_config']['reason']} diff --git a/openedx/features/content_type_gating/helpers.py b/openedx/features/content_type_gating/helpers.py index c4b2977f1e..f1cf20d3e0 100644 --- a/openedx/features/content_type_gating/helpers.py +++ b/openedx/features/content_type_gating/helpers.py @@ -1,8 +1,10 @@ """ Helper functions used by both content_type_gating and course_duration_limits. """ +import logging from xmodule.partitions.partitions import Group +from course_modes.models import CourseMode # Studio generates partition IDs starting at 100. There is already a manually generated # partition for Enrollment Track that uses ID 50, so we'll use 51. @@ -14,3 +16,42 @@ CONTENT_TYPE_GATE_GROUP_IDS = { } LIMITED_ACCESS = Group(CONTENT_TYPE_GATE_GROUP_IDS['limited_access'], 'Limited-access Users') FULL_ACCESS = Group(CONTENT_TYPE_GATE_GROUP_IDS['full_access'], 'Full-access Users') +LOG = logging.getLogger(__name__) + + +def correct_modes_for_fbe(course_key, enrollment=None, user=None): + """ + If CONTENT_TYPE_GATING is enabled use the following logic to determine whether + enabled_for_enrollment should be false + """ + if course_key is None: + return True + + modes = CourseMode.modes_for_course(course_key, include_expired=True, only_selectable=False) + modes_dict = {mode.slug: mode for mode in modes} + + # If there is no audit mode or no verified mode, FBE will not be enabled + if (CourseMode.AUDIT not in modes_dict) or (CourseMode.VERIFIED not in modes_dict): + return False + + if enrollment and user: + mode_slug = enrollment.mode + if enrollment.is_active: + course_mode = CourseMode.mode_for_course( + course_key, + mode_slug, + modes=modes, + ) + if course_mode is None: + LOG.error( + u"User %s is in an unknown CourseMode '%s'" + u" for course %s. Granting full access to content for this user", + user.username, + mode_slug, + course_key, + ) + return False + + if mode_slug != CourseMode.AUDIT: + return False + return True diff --git a/openedx/features/content_type_gating/models.py b/openedx/features/content_type_gating/models.py index 26bb3f4118..6817e6d2b4 100644 --- a/openedx/features/content_type_gating/models.py +++ b/openedx/features/content_type_gating/models.py @@ -20,7 +20,7 @@ from lms.djangoapps.courseware.masquerade import ( ) from openedx.core.djangoapps.config_model_utils.models import StackedConfigurationModel from openedx.core.djangoapps.config_model_utils.utils import is_in_holdback -from openedx.features.content_type_gating.helpers import FULL_ACCESS, LIMITED_ACCESS +from openedx.features.content_type_gating.helpers import FULL_ACCESS, LIMITED_ACCESS, correct_modes_for_fbe from student.models import CourseEnrollment from student.role_helpers import has_staff_roles from xmodule.partitions.partitions import ENROLLMENT_TRACK_PARTITION_ID @@ -137,14 +137,18 @@ class ContentTypeGatingConfig(StackedConfigurationModel): if user_variable_represents_correct_user and is_in_holdback(user): return False + if not correct_modes_for_fbe(course_key, enrollment, user): + return False + # enrollment might be None if the user isn't enrolled. In that case, # return enablement as if the user enrolled today # Also, ignore enrollment creation date if the user is masquerading. if enrollment is None or course_masquerade: - return cls.enabled_for_course(course_key=course_key, target_datetime=timezone.now()) + target_datetime = timezone.now() else: - current_config = cls.current(course_key=enrollment.course_id) - return current_config.enabled_as_of_datetime(target_datetime=enrollment.created) + target_datetime = enrollment.created + current_config = cls.current(course_key=course_key) + return current_config.enabled_as_of_datetime(target_datetime=target_datetime) @classmethod def enabled_for_course(cls, course_key, target_datetime=None): @@ -161,6 +165,8 @@ class ContentTypeGatingConfig(StackedConfigurationModel): course_key: The CourseKey of the course being queried. target_datetime: The datetime to checked enablement as of. Defaults to the current date and time. """ + if not correct_modes_for_fbe(course_key): + return False if target_datetime is None: target_datetime = timezone.now() diff --git a/openedx/features/content_type_gating/partitions.py b/openedx/features/content_type_gating/partitions.py index b640a19fab..ce1b988988 100644 --- a/openedx/features/content_type_gating/partitions.py +++ b/openedx/features/content_type_gating/partitions.py @@ -177,51 +177,10 @@ class ContentTypeGatingPartitionScheme(object): """ Returns the Group for the specified user. """ - - # For now, treat everyone as a Full-access user, until we have the rest of the - # feature gating logic in place. - if not ContentTypeGatingConfig.enabled_for_enrollment(user=user, course_key=course_key, user_partition=user_partition): return FULL_ACCESS - - # If CONTENT_TYPE_GATING is enabled use the following logic to determine whether a user should have FULL_ACCESS - # or LIMITED_ACCESS - - course_mode = apps.get_model('course_modes.CourseMode') - modes = course_mode.modes_for_course(course_key, include_expired=True, only_selectable=False) - modes_dict = {mode.slug: mode for mode in modes} - - # If there is no verified mode, all users are granted FULL_ACCESS - if not course_mode.has_verified_mode(modes_dict): - return FULL_ACCESS - - course_enrollment = apps.get_model('student.CourseEnrollment') - - mode_slug, is_active = course_enrollment.enrollment_mode_for_user(user, course_key) - - if mode_slug and is_active: - course_mode = course_mode.mode_for_course( - course_key, - mode_slug, - modes=modes, - ) - if course_mode is None: - LOG.error( - u"User %s is in an unknown CourseMode '%s'" - u" for course %s. Granting full access to content for this user", - user.username, - mode_slug, - course_key, - ) - return FULL_ACCESS - - if mode_slug == CourseMode.AUDIT: - return LIMITED_ACCESS - else: - return FULL_ACCESS else: - # Unenrolled users don't get gated content return LIMITED_ACCESS @classmethod diff --git a/openedx/features/content_type_gating/tests/test_access.py b/openedx/features/content_type_gating/tests/test_access.py index 980bd59b8f..1e0afac189 100644 --- a/openedx/features/content_type_gating/tests/test_access.py +++ b/openedx/features/content_type_gating/tests/test_access.py @@ -365,7 +365,7 @@ class TestProblemTypeAccess(SharedModuleStoreTestCase): category=component_type, display_name=component_type, graded=True, - metadata={} if component_type == 'html' else METADATA + metadata={} if (component_type == 'html' or len(modes) == 1) else METADATA ) blocks_dict[component_type] = block diff --git a/openedx/features/content_type_gating/tests/test_models.py b/openedx/features/content_type_gating/tests/test_models.py index 16c013a812..bc5fbb8861 100644 --- a/openedx/features/content_type_gating/tests/test_models.py +++ b/openedx/features/content_type_gating/tests/test_models.py @@ -8,6 +8,8 @@ import pytz from edx_django_utils.cache import RequestCache from opaque_keys.edx.locator import CourseLocator + +from course_modes.tests.factories import CourseModeFactory from openedx.core.djangoapps.config_model_utils.models import Provenance from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory @@ -24,6 +26,8 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): def setUp(self): self.course_overview = CourseOverviewFactory.create() + CourseModeFactory.create(course_id=self.course_overview.id, mode_slug='audit') + CourseModeFactory.create(course_id=self.course_overview.id, mode_slug='verified') self.user = UserFactory.create() super(TestContentTypeGatingConfig, self).setUp() @@ -73,9 +77,9 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): user = self.user course_key = self.course_overview.id - query_count = 6 + query_count = 7 if not already_enrolled or not pass_enrollment and already_enrolled: - query_count = 7 + query_count = 8 with self.assertNumQueries(query_count): enabled = ContentTypeGatingConfig.enabled_for_enrollment( diff --git a/openedx/features/content_type_gating/tests/test_partitions.py b/openedx/features/content_type_gating/tests/test_partitions.py index de08ef914e..71b10f3d02 100644 --- a/openedx/features/content_type_gating/tests/test_partitions.py +++ b/openedx/features/content_type_gating/tests/test_partitions.py @@ -25,6 +25,8 @@ class TestContentTypeGatingPartition(CacheIsolationTestCase): def test_create_content_gating_partition_happy_path(self): mock_course = Mock(id=self.course_key, user_partitions={}) + CourseModeFactory.create(course_id=mock_course.id, mode_slug='audit') + CourseModeFactory.create(course_id=mock_course.id, mode_slug='verified') ContentTypeGatingConfig.objects.create(enabled=True, enabled_as_of=datetime(2018, 1, 1)) with patch('openedx.features.content_type_gating.partitions.ContentTypeGatingPartitionScheme.create_user_partition') as mock_create: @@ -133,6 +135,7 @@ class TestContentTypeGatingPartition(CacheIsolationTestCase): mock_request = None mock_course = Mock(id=self.course_key, user_partitions={}) mock_block = Mock(scope_ids=Mock(usage_id=Mock(course_key=mock_course.id))) + CourseModeFactory.create(course_id=mock_course.id, mode_slug='audit') CourseModeFactory.create(course_id=mock_course.id, mode_slug='verified') global_staff = GlobalStaffFactory.create() ContentTypeGatingConfig.objects.create(enabled=True, enabled_as_of=datetime(2018, 1, 1)) diff --git a/openedx/features/course_duration_limits/models.py b/openedx/features/course_duration_limits/models.py index 1dbf659db6..4a9b67b0e2 100644 --- a/openedx/features/course_duration_limits/models.py +++ b/openedx/features/course_duration_limits/models.py @@ -20,7 +20,11 @@ from lms.djangoapps.courseware.masquerade import ( ) from openedx.core.djangoapps.config_model_utils.models import StackedConfigurationModel from openedx.core.djangoapps.config_model_utils.utils import is_in_holdback -from openedx.features.content_type_gating.helpers import CONTENT_GATING_PARTITION_ID, CONTENT_TYPE_GATE_GROUP_IDS +from openedx.features.content_type_gating.helpers import ( + CONTENT_GATING_PARTITION_ID, + CONTENT_TYPE_GATE_GROUP_IDS, + correct_modes_for_fbe +) from student.models import CourseEnrollment from student.role_helpers import has_staff_roles from xmodule.partitions.partitions import ENROLLMENT_TRACK_PARTITION_ID @@ -136,10 +140,14 @@ class CourseDurationLimitConfig(StackedConfigurationModel): # When masquerading as a specific learner, course duration limits # will be on if they are currently on for the learner. if enrollment is None or not_student_masquerade: - return cls.enabled_for_course(course_key=course_key, target_datetime=timezone.now()) + # we bypass enabled_for_course here and use enabled_as_of_datetime directly + # because the correct_modes_for_fbe for FBE check contained in enabled_for_course + # is redundant with checks done upstream of this code + target_datetime = timezone.now() else: - current_config = cls.current(course_key=enrollment.course_id) - return current_config.enabled_as_of_datetime(target_datetime=enrollment.created) + target_datetime = enrollment.created + current_config = cls.current(course_key=course_key) + return current_config.enabled_as_of_datetime(target_datetime=target_datetime) @classmethod def enabled_for_course(cls, course_key, target_datetime=None): @@ -156,6 +164,8 @@ class CourseDurationLimitConfig(StackedConfigurationModel): course_key: The CourseKey of the course being queried. target_datetime: The datetime to checked enablement as of. Defaults to the current date and time. """ + if not correct_modes_for_fbe(course_key): + return False if target_datetime is None: target_datetime = timezone.now() diff --git a/openedx/features/course_duration_limits/tests/test_models.py b/openedx/features/course_duration_limits/tests/test_models.py index 0f7f1ef6fe..27bbce4bb9 100644 --- a/openedx/features/course_duration_limits/tests/test_models.py +++ b/openedx/features/course_duration_limits/tests/test_models.py @@ -11,6 +11,7 @@ from mock import Mock import pytz from edx_django_utils.cache import RequestCache +from course_modes.tests.factories import CourseModeFactory from opaque_keys.edx.locator import CourseLocator from openedx.core.djangoapps.config_model_utils.models import Provenance from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory @@ -31,6 +32,8 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): def setUp(self): self.course_overview = CourseOverviewFactory.create() + CourseModeFactory.create(course_id=self.course_overview.id, mode_slug='audit') + CourseModeFactory.create(course_id=self.course_overview.id, mode_slug='verified') self.user = UserFactory.create() super(TestCourseDurationLimitConfig, self).setUp() diff --git a/openedx/features/course_experience/tests/views/test_course_updates.py b/openedx/features/course_experience/tests/views/test_course_updates.py index e48984e821..41f5f8932f 100644 --- a/openedx/features/course_experience/tests/views/test_course_updates.py +++ b/openedx/features/course_experience/tests/views/test_course_updates.py @@ -130,7 +130,7 @@ class TestCourseUpdatesPage(SharedModuleStoreTestCase): # Fetch the view and verify that the query counts haven't changed # TODO: decrease query count as part of REVO-28 - with self.assertNumQueries(53, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): + with self.assertNumQueries(50, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): with check_mongo_calls(4): url = course_updates_url(self.course) self.client.get(url)