diff --git a/common/djangoapps/django_comment_common/models.py b/common/djangoapps/django_comment_common/models.py index 761ea57c76..c9e3aead39 100644 --- a/common/djangoapps/django_comment_common/models.py +++ b/common/djangoapps/django_comment_common/models.py @@ -109,6 +109,13 @@ class Role(models.Model): return self.permissions.filter(name=permission).exists() + @staticmethod + def user_has_role_for_course(user, course_id, role_names): + """ + Returns True if the user has one of the given roles for the given course + """ + return Role.objects.filter(course_id=course_id, name__in=role_names, users=user).exists() + class Permission(models.Model): name = models.CharField(max_length=30, null=False, blank=False, primary_key=True) diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index 46d5d11aa7..be43f195c4 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -240,7 +240,7 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): __test__ = True # TODO: decrease query count as part of REVO-28 - QUERY_COUNT = 33 + QUERY_COUNT = 38 TEST_DATA = { # (providers, course_width, enable_ccx, view_as_ccx): ( # # of sql queries to default, @@ -269,7 +269,7 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): __test__ = True # TODO: decrease query count as part of REVO-28 - QUERY_COUNT = 33 + QUERY_COUNT = 38 TEST_DATA = { ('no_overrides', 1, True, False): (QUERY_COUNT, 3), ('no_overrides', 2, True, False): (QUERY_COUNT, 3), @@ -277,9 +277,9 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): ('ccx', 1, True, False): (QUERY_COUNT, 3), ('ccx', 2, True, False): (QUERY_COUNT, 3), ('ccx', 3, True, False): (QUERY_COUNT, 3), - ('ccx', 1, True, True): (34, 3), - ('ccx', 2, True, True): (34, 3), - ('ccx', 3, True, True): (34, 3), + ('ccx', 1, True, True): (QUERY_COUNT + 1, 3), + ('ccx', 2, True, True): (QUERY_COUNT + 1, 3), + ('ccx', 3, True, True): (QUERY_COUNT + 1, 3), ('no_overrides', 1, False, False): (QUERY_COUNT, 3), ('no_overrides', 2, False, False): (QUERY_COUNT, 3), ('no_overrides', 3, False, False): (QUERY_COUNT, 3), diff --git a/lms/djangoapps/course_api/blocks/tests/test_api.py b/lms/djangoapps/course_api/blocks/tests/test_api.py index 14eb1992b8..08df6b37af 100644 --- a/lms/djangoapps/course_api/blocks/tests/test_api.py +++ b/lms/djangoapps/course_api/blocks/tests/test_api.py @@ -162,7 +162,7 @@ class TestGetBlocksQueryCounts(TestGetBlocksQueryCountsBase): self._get_blocks( course, expected_mongo_queries=0, - expected_sql_queries=7 if with_storage_backing else 6, + expected_sql_queries=8 if with_storage_backing else 7, ) @ddt.data( @@ -179,9 +179,9 @@ class TestGetBlocksQueryCounts(TestGetBlocksQueryCountsBase): clear_course_from_cache(course.id) if with_storage_backing: - num_sql_queries = 17 + num_sql_queries = 18 else: - num_sql_queries = 7 + num_sql_queries = 8 self._get_blocks( course, @@ -211,7 +211,7 @@ class TestQueryCountsWithIndividualOverrideProvider(TestGetBlocksQueryCountsBase self._get_blocks( course, expected_mongo_queries=0, - expected_sql_queries=8 if with_storage_backing else 7, + expected_sql_queries=9 if with_storage_backing else 8, ) @ddt.data( @@ -228,9 +228,9 @@ class TestQueryCountsWithIndividualOverrideProvider(TestGetBlocksQueryCountsBase clear_course_from_cache(course.id) if with_storage_backing: - num_sql_queries = 18 + num_sql_queries = 19 else: - num_sql_queries = 8 + num_sql_queries = 9 self._get_blocks( course, diff --git a/lms/djangoapps/courseware/tests/test_access.py b/lms/djangoapps/courseware/tests/test_access.py index 6d1996cd23..6f9ad80013 100644 --- a/lms/djangoapps/courseware/tests/test_access.py +++ b/lms/djangoapps/courseware/tests/test_access.py @@ -842,12 +842,12 @@ class CourseOverviewAccessTestCase(ModuleStoreTestCase): if user_attr_name == 'user_staff' and action == 'see_exists': # always checks staff role, and if the course has started, check the duration configuration if course_attr_name == 'course_started': - num_queries = 5 + num_queries = 3 else: num_queries = 1 elif user_attr_name == 'user_normal' and action == 'see_exists': if course_attr_name == 'course_started': - num_queries = 5 + num_queries = 6 else: # checks staff role and enrollment data num_queries = 2 diff --git a/lms/djangoapps/courseware/tests/test_course_info.py b/lms/djangoapps/courseware/tests/test_course_info.py index d6c1b68991..e7749d4e57 100644 --- a/lms/djangoapps/courseware/tests/test_course_info.py +++ b/lms/djangoapps/courseware/tests/test_course_info.py @@ -436,8 +436,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, 40, 3) + self.fetch_course_info_with_queries(self.instructor_paced_course, 42, 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, 40, 3) + self.fetch_course_info_with_queries(self.self_paced_course, 42, 3) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index ac5a19e5d2..d9b97d8765 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, 171), - (ModuleStoreEnum.Type.split, 4, 167), + (ModuleStoreEnum.Type.mongo, 10, 178), + (ModuleStoreEnum.Type.split, 4, 172), ) @ddt.unpack def test_index_query_counts(self, store_type, expected_mongo_query_count, expected_mysql_query_count): @@ -1440,8 +1440,8 @@ class ProgressPageTests(ProgressPageBaseTests): self.assertContains(resp, u"Download Your Certificate") @ddt.data( - (True, 53), - (False, 52) + (True, 58), + (False, 57) ) @ddt.unpack def test_progress_queries_paced_courses(self, self_paced, query_count): @@ -1454,8 +1454,8 @@ class ProgressPageTests(ProgressPageBaseTests): @patch.dict(settings.FEATURES, {'ASSUME_ZERO_GRADE_IF_ABSENT_FOR_ALL_TESTS': False}) @ddt.data( - (False, 60, 40), - (True, 52, 36) + (False, 65, 45), + (True, 57, 41) ) @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 405054df33..432682cc17 100644 --- a/lms/djangoapps/discussion/tests/test_views.py +++ b/lms/djangoapps/discussion/tests/test_views.py @@ -431,18 +431,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, 22, 7), - (ModuleStoreEnum.Type.mongo, False, 50, 5, 2, 22, 7), + (ModuleStoreEnum.Type.mongo, False, 1, 5, 2, 23, 8), + (ModuleStoreEnum.Type.mongo, False, 50, 5, 2, 23, 8), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, False, 1, 3, 3, 22, 7), - (ModuleStoreEnum.Type.split, False, 50, 3, 3, 22, 7), + (ModuleStoreEnum.Type.split, False, 1, 3, 3, 23, 8), + (ModuleStoreEnum.Type.split, False, 50, 3, 3, 23, 8), # Enabling Enterprise integration should have no effect on the number of mongo queries made. - (ModuleStoreEnum.Type.mongo, True, 1, 5, 2, 22, 7), - (ModuleStoreEnum.Type.mongo, True, 50, 5, 2, 22, 7), + (ModuleStoreEnum.Type.mongo, True, 1, 5, 2, 23, 8), + (ModuleStoreEnum.Type.mongo, True, 50, 5, 2, 23, 8), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, True, 1, 3, 3, 22, 7), - (ModuleStoreEnum.Type.split, True, 50, 3, 3, 22, 7), + (ModuleStoreEnum.Type.split, True, 1, 3, 3, 23, 8), + (ModuleStoreEnum.Type.split, True, 50, 3, 3, 23, 8), ) @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 b7af268758..538d901da3 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, 40), - (ModuleStoreEnum.Type.split, 3, 13, 40), + (ModuleStoreEnum.Type.mongo, 3, 4, 41), + (ModuleStoreEnum.Type.split, 3, 13, 41), ) @ddt.unpack @count_queries @@ -412,8 +412,8 @@ class ViewsQueryCountTestCase( self.create_thread_helper(mock_request) @ddt.data( - (ModuleStoreEnum.Type.mongo, 3, 3, 36), - (ModuleStoreEnum.Type.split, 3, 10, 36), + (ModuleStoreEnum.Type.mongo, 3, 3, 37), + (ModuleStoreEnum.Type.split, 3, 10, 37), ) @ddt.unpack @count_queries diff --git a/lms/djangoapps/grades/tests/test_course_grade_factory.py b/lms/djangoapps/grades/tests/test_course_grade_factory.py index a205f111ff..ad257a06a9 100644 --- a/lms/djangoapps/grades/tests/test_course_grade_factory.py +++ b/lms/djangoapps/grades/tests/test_course_grade_factory.py @@ -92,35 +92,35 @@ class TestCourseGradeFactory(GradeTestBase): [self.sequence.display_name, self.sequence2.display_name] ) - with self.assertNumQueries(4), mock_get_score(1, 2): + with self.assertNumQueries(5), mock_get_score(1, 2): _assert_read(expected_pass=False, expected_percent=0) # start off with grade of 0 - num_queries = 43 + num_queries = 44 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(4): + with self.assertNumQueries(5): _assert_read(expected_pass=True, expected_percent=0.5) # updated to grade of .5 - num_queries = 8 + num_queries = 9 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(4): + with self.assertNumQueries(5): _assert_read(expected_pass=True, expected_percent=0.5) # NOT updated to grade of .25 - num_queries = 22 + num_queries = 23 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(4): + with self.assertNumQueries(5): _assert_read(expected_pass=True, expected_percent=1.0) # updated to grade of 1.0 - num_queries = 25 + num_queries = 26 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(4): + with self.assertNumQueries(5): _assert_read(expected_pass=False, expected_percent=0.0) # updated to grade of 0.0 @patch.dict(settings.FEATURES, {'ASSUME_ZERO_GRADE_IF_ABSENT_FOR_ALL_TESTS': False}) diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index 18cf2da5f6..824526dff8 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/lms/djangoapps/grades/tests/test_tasks.py @@ -176,10 +176,10 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest self.assertEquals(mock_block_structure_create.call_count, 1) @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 32, True), - (ModuleStoreEnum.Type.mongo, 1, 32, False), - (ModuleStoreEnum.Type.split, 3, 32, True), - (ModuleStoreEnum.Type.split, 3, 32, False), + (ModuleStoreEnum.Type.mongo, 1, 33, True), + (ModuleStoreEnum.Type.mongo, 1, 33, False), + (ModuleStoreEnum.Type.split, 3, 33, True), + (ModuleStoreEnum.Type.split, 3, 33, False), ) @ddt.unpack def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, create_multiple_subsections): @@ -191,8 +191,8 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest self._apply_recalculate_subsection_grade() @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 32), - (ModuleStoreEnum.Type.split, 3, 32), + (ModuleStoreEnum.Type.mongo, 1, 33), + (ModuleStoreEnum.Type.split, 3, 33), ) @ddt.unpack def test_query_counts_dont_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls): @@ -237,8 +237,8 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest ) @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 16), - (ModuleStoreEnum.Type.split, 3, 16), + (ModuleStoreEnum.Type.mongo, 1, 17), + (ModuleStoreEnum.Type.split, 3, 17), ) @ddt.unpack def test_persistent_grades_not_enabled_on_course(self, default_store, num_mongo_queries, num_sql_queries): @@ -252,8 +252,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, 33), - (ModuleStoreEnum.Type.split, 3, 33), + (ModuleStoreEnum.Type.mongo, 1, 34), + (ModuleStoreEnum.Type.split, 3, 34), ) @ddt.unpack def test_persistent_grades_enabled_on_course(self, default_store, num_mongo_queries, num_sql_queries): diff --git a/openedx/features/content_type_gating/helpers.py b/openedx/features/content_type_gating/helpers.py new file mode 100644 index 0000000000..f93f80153d --- /dev/null +++ b/openedx/features/content_type_gating/helpers.py @@ -0,0 +1,39 @@ +""" +Helper functions used by both content_type_gating and course_duration_limits. +""" + +from django_comment_common.models import ( + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_MODERATOR, + FORUM_ROLE_GROUP_MODERATOR, + FORUM_ROLE_COMMUNITY_TA, + Role +) +from student.roles import ( + CourseBetaTesterRole, + CourseInstructorRole, + CourseStaffRole, + OrgStaffRole, + OrgInstructorRole, + GlobalStaff +) + + +def has_staff_roles(user, course_key): + """ + Disable feature based enrollments for the enrollment if a user has any of the following roles + Staff, Instructor, Beta Tester, Forum Community TA, Forum Group Moderator, Forum Moderator, Forum Administrator + """ + forum_roles = [FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_GROUP_MODERATOR, + FORUM_ROLE_MODERATOR, FORUM_ROLE_ADMINISTRATOR] + is_staff = CourseStaffRole(course_key).has_user(user) + is_instructor = CourseInstructorRole(course_key).has_user(user) + is_beta_tester = CourseBetaTesterRole(course_key).has_user(user) + is_org_staff = OrgStaffRole(course_key.org).has_user(user) + is_org_instructor = OrgInstructorRole(course_key.org).has_user(user) + is_global_staff = GlobalStaff().has_user(user) + has_forum_role = Role.user_has_role_for_course(user, course_key, forum_roles) + if any([is_staff, is_instructor, is_beta_tester, is_org_staff, + is_org_instructor, is_global_staff, has_forum_role]): + return True + return False diff --git a/openedx/features/content_type_gating/models.py b/openedx/features/content_type_gating/models.py index f776647365..dbd9795059 100644 --- a/openedx/features/content_type_gating/models.py +++ b/openedx/features/content_type_gating/models.py @@ -10,16 +10,17 @@ from django.db import models from django.utils.encoding import python_2_unicode_compatible from django.utils.translation import ugettext_lazy as _ from django.utils import timezone -from lms.djangoapps.courseware.masquerade import get_course_masquerade, is_masquerading_as_specific_student +from lms.djangoapps.courseware.masquerade import get_course_masquerade, is_masquerading_as_specific_student from experiments.models import ExperimentData -from student.models import CourseEnrollment from openedx.core.djangoapps.config_model_utils.models import StackedConfigurationModel +from openedx.features.content_type_gating.helpers import has_staff_roles from openedx.features.course_duration_limits.config import ( CONTENT_TYPE_GATING_FLAG, EXPERIMENT_ID, EXPERIMENT_DATA_HOLDBACK_KEY ) +from student.models import CourseEnrollment @python_2_unicode_compatible @@ -85,6 +86,24 @@ class ContentTypeGatingConfig(StackedConfigurationModel): if enrollment is None: enrollment = CourseEnrollment.get_enrollment(user, course_key) + if user is None and enrollment is not None: + user = enrollment.user + + no_masquerade = get_course_masquerade(user, course_key) is None + student_masquerade = is_masquerading_as_specific_student(user, course_key) + # We can only use the user variable for the code below when the request is not in a masquerade state + # or is masquerading as a specific user. + # When a request is not in a masquerade state the user variable represents the correct user. + # When a request is in a masquerade state and not masquerading as a specific user, + # then then user variable will be the incorrect (original) user, not the masquerade user. + # If a request is masquerading as a specific user, the user variable will represent the correct user. + user_variable_represents_correct_user = (no_masquerade or student_masquerade) + if user and user.id: + # TODO: Move masquerade checks to enabled_for_enrollment from content_type_gating/partitions.py + # TODO: Consolidate masquerade checks into shared function like has_staff_roles below + if user_variable_represents_correct_user and has_staff_roles(user, course_key): + return False + # enrollment might be None if the user isn't enrolled. In that case, # return enablement as if the user enrolled today if enrollment is None: @@ -93,9 +112,7 @@ class ContentTypeGatingConfig(StackedConfigurationModel): # TODO: clean up as part of REV-100 experiment_data_holdback_key = EXPERIMENT_DATA_HOLDBACK_KEY.format(user) is_in_holdback = False - no_masquerade = get_course_masquerade(user, course_key) is None - student_masquerade = is_masquerading_as_specific_student(user, course_key) - if user and (no_masquerade or student_masquerade): + if user and (user_variable_represents_correct_user): try: holdback_value = ExperimentData.objects.get( user=user, diff --git a/openedx/features/content_type_gating/partitions.py b/openedx/features/content_type_gating/partitions.py index bb192ca5b0..eb6de93406 100644 --- a/openedx/features/content_type_gating/partitions.py +++ b/openedx/features/content_type_gating/partitions.py @@ -14,8 +14,8 @@ from django.apps import apps from django.conf import settings from django.template.loader import render_to_string from django.utils.translation import ugettext_lazy as _ - from web_fragments.fragment import Fragment + from lms.djangoapps.commerce.utils import EcommerceService from lms.djangoapps.courseware.masquerade import ( get_course_masquerade, @@ -25,7 +25,6 @@ from lms.djangoapps.courseware.masquerade import ( from xmodule.partitions.partitions import Group, UserPartition, UserPartitionError, ENROLLMENT_TRACK_PARTITION_ID from openedx.core.lib.mobile_utils import is_request_from_mobile_app from openedx.features.content_type_gating.models import ContentTypeGatingConfig -from student.roles import CourseBetaTesterRole LOG = logging.getLogger(__name__) @@ -171,10 +170,6 @@ class ContentTypeGatingPartitionScheme(object): if not course_mode.has_verified_mode(modes_dict): return cls.FULL_ACCESS - # If the user is a beta tester for this course they are granted FULL_ACCESS - if CourseBetaTesterRole(course_key).has_user(user): - return cls.FULL_ACCESS - course_enrollment = apps.get_model('student.CourseEnrollment') mode_slug, is_active = course_enrollment.enrollment_mode_for_user(user, course_key) diff --git a/openedx/features/content_type_gating/tests/test_access.py b/openedx/features/content_type_gating/tests/test_access.py index e923e36e41..5012b0f1f4 100644 --- a/openedx/features/content_type_gating/tests/test_access.py +++ b/openedx/features/content_type_gating/tests/test_access.py @@ -11,6 +11,14 @@ from django.urls import reverse from django.utils import timezone from mock import patch +from django_comment_common.models import ( + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_MODERATOR, + FORUM_ROLE_GROUP_MODERATOR, + FORUM_ROLE_COMMUNITY_TA, + Role +) +from django_comment_client.tests.factories import RoleFactory from course_modes.tests.factories import CourseModeFactory from experiments.models import ExperimentKeyValue from xmodule.partitions.partitions import ENROLLMENT_TRACK_PARTITION_ID @@ -39,6 +47,14 @@ from student.tests.factories import ( UserFactory, TEST_PASSWORD ) +from lms.djangoapps.courseware.tests.factories import ( + InstructorFactory, + StaffFactory, + BetaTesterFactory, + OrgStaffFactory, + OrgInstructorFactory, + GlobalStaffFactory, +) from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory @@ -424,6 +440,7 @@ class TestProblemTypeAccess(SharedModuleStoreTestCase): BetaTesterFactory, OrgStaffFactory, OrgInstructorFactory, + GlobalStaffFactory, ) def test_access_course_team_users(self, role_factory): """ @@ -432,7 +449,10 @@ class TestProblemTypeAccess(SharedModuleStoreTestCase): # There are two types of course team members: instructor and staff # they have different privileges, but for the purpose of this test the important thing is that they should both # have access to all graded content - user = role_factory.create(course_key=self.course.id) + if role_factory == GlobalStaffFactory: + user = role_factory.create() + else: + user = role_factory.create(course_key=self.course.id) # assert that course team members have access to graded content _assert_block_is_gated( block=self.blocks_dict['problem'], @@ -443,17 +463,19 @@ class TestProblemTypeAccess(SharedModuleStoreTestCase): ) @ddt.data( - GlobalStaffFactory, + FORUM_ROLE_COMMUNITY_TA, + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_MODERATOR, + FORUM_ROLE_GROUP_MODERATOR ) - def test_access_global_users(self, role_factory): + def test_access_user_with_forum_role(self, role_name): """ - Test that members of the course team do not lose access to graded content + Test that users with a given forum role do not lose access to graded content """ - # There are two types of course team members: instructor and staff - # they have different privileges, but for the purpose of this test the important thing is that they should both - # have access to all graded content - user = role_factory.create() - # assert that course team members have access to graded content + user = UserFactory.create() + role = RoleFactory(name=role_name, course_id=self.course.id) + role.users.add(user) + _assert_block_is_gated( block=self.blocks_dict['problem'], user_id=user.id, @@ -549,6 +571,71 @@ class TestProblemTypeAccess(SharedModuleStoreTestCase): self.assertEqual(response.status_code, 200) return response + @ddt.data( + InstructorFactory, + StaffFactory, + BetaTesterFactory, + OrgStaffFactory, + OrgInstructorFactory, + GlobalStaffFactory, + ) + def test_access_masquerade_as_course_team_users(self, role_factory): + """ + Test that when masquerading as members of the course team you do not lose access to graded content + """ + # There are two types of course team members: instructor and staff + # they have different privileges, but for the purpose of this test the important thing is that they should both + # have access to all graded content + staff_user = StaffFactory.create(password=TEST_PASSWORD, course_key=self.course.id) + CourseEnrollmentFactory.create( + user=staff_user, + course_id=self.course.id, + mode='audit' + ) + self.client.login(username=staff_user.username, password=TEST_PASSWORD) + + if role_factory == GlobalStaffFactory: + user = role_factory.create() + else: + user = role_factory.create(course_key=self.course.id) + self.update_masquerade(username=user.username) + + block = self.blocks_dict['problem'] + block_view_url = reverse('render_xblock', kwargs={'usage_key_string': unicode(block.scope_ids.usage_id)}) + response = self.client.get(block_view_url) + self.assertEquals(response.status_code, 200) + + @ddt.data( + FORUM_ROLE_COMMUNITY_TA, + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_MODERATOR, + FORUM_ROLE_GROUP_MODERATOR + ) + def test_access_masquerade_as_user_with_forum_role(self, role_name): + """ + Test that when masquerading as a user with a given forum role you do not lose access to graded content + """ + staff_user = StaffFactory.create(password=TEST_PASSWORD, course_key=self.course.id) + CourseEnrollmentFactory.create( + user=staff_user, + course_id=self.course.id, + mode='audit' + ) + self.client.login(username=staff_user.username, password=TEST_PASSWORD) + + user = UserFactory.create() + role = RoleFactory(name=role_name, course_id=self.course.id) + role.users.add(user) + self.update_masquerade(username=user.username) + + _assert_block_is_gated( + block=self.blocks_dict['problem'], + user_id=user.id, + course=self.course, + is_gated=False, + request_factory=self.factory, + ) + @override_settings(FIELD_OVERRIDE_PROVIDERS=( 'openedx.features.content_type_gating.field_override.ContentTypeGatingFieldOverride', diff --git a/openedx/features/content_type_gating/tests/test_models.py b/openedx/features/content_type_gating/tests/test_models.py index 693ae87daf..715eccd735 100644 --- a/openedx/features/content_type_gating/tests/test_models.py +++ b/openedx/features/content_type_gating/tests/test_models.py @@ -71,11 +71,11 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): course_key = self.course_overview.id if already_enrolled and pass_enrollment: - query_count = 4 + query_count = 7 elif not pass_enrollment and already_enrolled: - query_count = 6 + query_count = 8 else: - query_count = 5 + query_count = 7 with self.assertNumQueries(query_count): enabled = ContentTypeGatingConfig.enabled_for_enrollment( diff --git a/openedx/features/course_duration_limits/access.py b/openedx/features/course_duration_limits/access.py index 9f76a5b644..91798fb6d0 100644 --- a/openedx/features/course_duration_limits/access.py +++ b/openedx/features/course_duration_limits/access.py @@ -12,7 +12,6 @@ from django.utils.translation import ugettext as _ from util.date_utils import DEFAULT_SHORT_DATE_FORMAT, strftime_localized from course_modes.models import CourseMode - from lms.djangoapps.courseware.access_response import AccessError from lms.djangoapps.courseware.access_utils import ACCESS_GRANTED from lms.djangoapps.courseware.date_summary import verified_upgrade_deadline_link @@ -122,7 +121,7 @@ def register_course_expired_message(request, course): return if is_masquerading_as_student(request.user, course.id) and timezone.now() > expiration_date: - upgrade_message = _('This learner would not have access to this course. ' + upgrade_message = _('This learner does not have access to this course. ' 'Their access expired on {expiration_date}.') PageLevelMessages.register_warning_message( request, diff --git a/openedx/features/course_duration_limits/models.py b/openedx/features/course_duration_limits/models.py index ac476e8a13..051493e0cc 100644 --- a/openedx/features/course_duration_limits/models.py +++ b/openedx/features/course_duration_limits/models.py @@ -11,14 +11,16 @@ from django.db import models from django.utils.encoding import python_2_unicode_compatible from django.utils.translation import ugettext_lazy as _ from django.utils import timezone -from xmodule.partitions.partitions import ENROLLMENT_TRACK_PARTITION_ID from course_modes.models import CourseMode -from lms.djangoapps.courseware.masquerade import get_masquerade_role, get_course_masquerade, \ - is_masquerading_as_specific_student - from experiments.models import ExperimentData +from lms.djangoapps.courseware.masquerade import ( + get_course_masquerade, + get_masquerade_role, + is_masquerading_as_specific_student +) from openedx.core.djangoapps.config_model_utils.models import StackedConfigurationModel +from openedx.features.content_type_gating.helpers import has_staff_roles from openedx.features.content_type_gating.partitions import CONTENT_GATING_PARTITION_ID, CONTENT_TYPE_GATE_GROUP_IDS from openedx.features.course_duration_limits.config import ( CONTENT_TYPE_GATING_FLAG, @@ -26,7 +28,7 @@ from openedx.features.course_duration_limits.config import ( EXPERIMENT_DATA_HOLDBACK_KEY ) from student.models import CourseEnrollment -from student.roles import CourseBetaTesterRole, CourseInstructorRole, CourseStaffRole +from xmodule.partitions.partitions import ENROLLMENT_TRACK_PARTITION_ID @python_2_unicode_compatible @@ -48,6 +50,34 @@ class CourseDurationLimitConfig(StackedConfigurationModel): ) ) + @classmethod + def has_full_access_role_in_masquerade(cls, user, course_key, course_masquerade): + """ + When masquerading, the course duration limits will never trigger the course to expire, redirecting the user. + The roles of the masquerade user are still used to determine whether the course duration limit banner displays. + Another banner also displays if the course is expired for the masquerade user. + Both banners will appear if the masquerade user does not have any of the following roles: + Staff, Instructor, Beta Tester, Forum Community TA, Forum Group Moderator, Forum Moderator, Forum Administrator + """ + masquerade_role = get_masquerade_role(user, course_key) + verified_mode_id = settings.COURSE_ENROLLMENT_MODES.get(CourseMode.VERIFIED, {}).get('id') + # Masquerading users can select the the role of a verified users without selecting a specific user + is_verified = (course_masquerade.user_partition_id == ENROLLMENT_TRACK_PARTITION_ID + and course_masquerade.group_id == verified_mode_id) + # Masquerading users can select the role of staff without selecting a specific user + is_staff = masquerade_role == 'staff' + # Masquerading users can select other full access roles for which content type gating is disabled + is_full_access = (course_masquerade.user_partition_id == CONTENT_GATING_PARTITION_ID + and course_masquerade.group_id == CONTENT_TYPE_GATE_GROUP_IDS['full_access']) + # When masquerading as a specific user, we can check that user's staff roles as we would with a normal user + is_staff_role = False + if course_masquerade.user_name: + is_staff_role = has_staff_roles(user, course_key) + + if is_verified or is_full_access or is_staff or is_staff_role: + return True + return False + @classmethod def enabled_for_enrollment(cls, enrollment=None, user=None, course_key=None): """ @@ -83,28 +113,16 @@ class CourseDurationLimitConfig(StackedConfigurationModel): if enrollment is None: enrollment = CourseEnrollment.get_enrollment(user, course_key) - # if the user is has a role of staff, instructor or beta tester their access should not expire if user is None and enrollment is not None: user = enrollment.user - if user: + if user and user.id: course_masquerade = get_course_masquerade(user, course_key) if course_masquerade: - verified_mode_id = settings.COURSE_ENROLLMENT_MODES.get(CourseMode.VERIFIED, {}).get('id') - is_verified = (course_masquerade.user_partition_id == ENROLLMENT_TRACK_PARTITION_ID - and course_masquerade.group_id == verified_mode_id) - is_full_access = (course_masquerade.user_partition_id == CONTENT_GATING_PARTITION_ID - and course_masquerade.group_id == CONTENT_TYPE_GATE_GROUP_IDS['full_access']) - is_staff = get_masquerade_role(user, course_key) == 'staff' - if is_verified or is_full_access or is_staff: - return False - else: - staff_role = CourseStaffRole(course_key).has_user(user) - instructor_role = CourseInstructorRole(course_key).has_user(user) - beta_tester_role = CourseBetaTesterRole(course_key).has_user(user) - - if staff_role or instructor_role or beta_tester_role: + if cls.has_full_access_role_in_masquerade(user, course_key, course_masquerade): return False + elif has_staff_roles(user, course_key): + return False # enrollment might be None if the user isn't enrolled. In that case, # return enablement as if the user enrolled today diff --git a/openedx/features/course_duration_limits/tests/test_course_expiration.py b/openedx/features/course_duration_limits/tests/test_course_expiration.py index a0be81ca01..97b059f52d 100644 --- a/openedx/features/course_duration_limits/tests/test_course_expiration.py +++ b/openedx/features/course_duration_limits/tests/test_course_expiration.py @@ -11,8 +11,24 @@ import ddt import mock from course_modes.models import CourseMode +from django_comment_client.tests.factories import RoleFactory +from django_comment_common.models import ( + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_MODERATOR, + FORUM_ROLE_GROUP_MODERATOR, + FORUM_ROLE_COMMUNITY_TA +) from experiments.models import ExperimentData +from lms.djangoapps.courseware.tests.factories import ( + InstructorFactory, + StaffFactory, + BetaTesterFactory, + OrgStaffFactory, + OrgInstructorFactory, + GlobalStaffFactory, +) from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.djangoapps.schedules.tests.factories import ScheduleFactory from openedx.features.content_type_gating.partitions import CONTENT_GATING_PARTITION_ID, CONTENT_TYPE_GATE_GROUP_IDS from openedx.features.course_duration_limits.access import get_user_course_expiration_date, MIN_DURATION, MAX_DURATION from openedx.features.course_duration_limits.config import EXPERIMENT_ID, EXPERIMENT_DATA_HOLDBACK_KEY @@ -20,7 +36,7 @@ from openedx.features.course_experience.tests.views.helpers import add_course_mo from openedx.features.course_duration_limits.models import CourseDurationLimitConfig from student.models import CourseEnrollment from student.roles import CourseInstructorRole -from student.tests.factories import UserFactory, CourseEnrollmentFactory +from student.tests.factories import UserFactory, CourseEnrollmentFactory, TEST_PASSWORD from xmodule.partitions.partitions import ENROLLMENT_TRACK_PARTITION_ID from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -35,6 +51,7 @@ class CourseExpirationTestCase(ModuleStoreTestCase): start=now() - timedelta(weeks=10), ) self.user = UserFactory() + self.THREE_YEARS_AGO = now() - timedelta(days=(365 * 3)) # Make this a verified course so we can test expiration date add_course_mode(self.course, upgrade_deadline_expired=False) @@ -264,5 +281,105 @@ class CourseExpirationTestCase(ModuleStoreTestCase): response = self.client.get(course_home_url, follow=True) self.assertEqual(response.status_code, 200) self.assertItemsEqual(response.redirect_chain, []) - banner_text = 'This learner would not have access to this course. Their access expired on' + banner_text = 'This learner does not have access to this course. Their access expired on' self.assertIn(banner_text, response.content) + + @mock.patch("openedx.features.course_duration_limits.access.get_course_run_details") + @ddt.data( + InstructorFactory, + StaffFactory, + BetaTesterFactory, + OrgStaffFactory, + OrgInstructorFactory, + GlobalStaffFactory, + ) + def test_no_banner_when_masquerading_as_staff(self, role_factory, mock_get_course_run_details): + """ + When masquerading as a specific expired user, if that user has a staff role + the expired course banner will not show up. + """ + mock_get_course_run_details.return_value = {'weeks_to_complete': 1} + + if role_factory == GlobalStaffFactory: + expired_staff = role_factory.create(password=TEST_PASSWORD) + else: + expired_staff = role_factory.create(password=TEST_PASSWORD, course_key=self.course.id) + + ScheduleFactory( + start=self.THREE_YEARS_AGO, + enrollment__mode=CourseMode.AUDIT, + enrollment__course_id=self.course.id, + enrollment__user=expired_staff + ) + CourseDurationLimitConfig.objects.create( + enabled=True, + course=CourseOverview.get_from_id(self.course.id), + enabled_as_of=self.course.start, + ) + + staff_user = StaffFactory.create(password=TEST_PASSWORD, course_key=self.course.id) + CourseEnrollmentFactory.create( + user=staff_user, + course_id=self.course.id, + mode='audit' + ) + + self.client.login(username=staff_user.username, password='test') + + self.update_masquerade(username=expired_staff.username) + + course_home_url = reverse('openedx.course_experience.course_home', args=[unicode(self.course.id)]) + response = self.client.get(course_home_url, follow=True) + self.assertEqual(response.status_code, 200) + self.assertItemsEqual(response.redirect_chain, []) + banner_text = 'This learner does not have access to this course. Their access expired on' + self.assertNotIn(banner_text, response.content) + + @mock.patch("openedx.features.course_duration_limits.access.get_course_run_details") + @ddt.data( + FORUM_ROLE_COMMUNITY_TA, + FORUM_ROLE_GROUP_MODERATOR, + FORUM_ROLE_MODERATOR, + FORUM_ROLE_ADMINISTRATOR, + ) + def test_no_banner_when_masquerading_as_forum_staff(self, role_name, mock_get_course_run_details): + """ + When masquerading as a specific expired user, if that user has a forum staff role + the expired course banner will not show up. + """ + mock_get_course_run_details.return_value = {'weeks_to_complete': 1} + + expired_staff = UserFactory.create() + role = RoleFactory(name=role_name, course_id=self.course.id) + role.users.add(expired_staff) + + ScheduleFactory( + start=self.THREE_YEARS_AGO, + enrollment__mode=CourseMode.AUDIT, + enrollment__course_id=self.course.id, + enrollment__user=expired_staff + ) + + CourseDurationLimitConfig.objects.create( + enabled=True, + course=CourseOverview.get_from_id(self.course.id), + enabled_as_of=self.course.start, + ) + + staff_user = StaffFactory.create(password=TEST_PASSWORD, course_key=self.course.id) + CourseEnrollmentFactory.create( + user=staff_user, + course_id=self.course.id, + mode='audit' + ) + + self.client.login(username=staff_user.username, password='test') + + self.update_masquerade(username=expired_staff.username) + + course_home_url = reverse('openedx.course_experience.course_home', args=[unicode(self.course.id)]) + response = self.client.get(course_home_url, follow=True) + self.assertEqual(response.status_code, 200) + self.assertItemsEqual(response.redirect_chain, []) + banner_text = 'This learner does not have access to this course. Their access expired on' + self.assertNotIn(banner_text, response.content) diff --git a/openedx/features/course_duration_limits/tests/test_models.py b/openedx/features/course_duration_limits/tests/test_models.py index 6fe57c63f8..a0468b27ff 100644 --- a/openedx/features/course_duration_limits/tests/test_models.py +++ b/openedx/features/course_duration_limits/tests/test_models.py @@ -76,9 +76,9 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): user = self.user course_key = self.course_overview.id - query_count = 6 + query_count = 7 if not pass_enrollment and already_enrolled: - query_count = 7 + query_count = 8 with self.assertNumQueries(query_count): enabled = CourseDurationLimitConfig.enabled_for_enrollment( diff --git a/openedx/features/course_experience/tests/views/test_course_home.py b/openedx/features/course_experience/tests/views/test_course_home.py index ce99fa7733..0db166763d 100644 --- a/openedx/features/course_experience/tests/views/test_course_home.py +++ b/openedx/features/course_experience/tests/views/test_course_home.py @@ -15,6 +15,13 @@ from pytz import UTC from waffle.models import Flag from waffle.testutils import override_flag +from django_comment_common.models import ( + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_MODERATOR, + FORUM_ROLE_GROUP_MODERATOR, + FORUM_ROLE_COMMUNITY_TA +) +from django_comment_client.tests.factories import RoleFactory from course_modes.models import CourseMode from course_modes.tests.factories import CourseModeFactory from courseware.tests.helpers import get_expiration_banner_text @@ -195,7 +202,7 @@ class TestCourseHomePage(CourseHomePageTestCase): # Fetch the view and verify the query counts # TODO: decrease query count as part of REVO-28 - with self.assertNumQueries(78, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): + with self.assertNumQueries(86, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): with check_mongo_calls(4): url = course_home_url(self.course) self.client.get(url) @@ -374,7 +381,7 @@ class TestCourseHomePageAccess(CourseHomePageTestCase): OrgStaffFactory, OrgInstructorFactory, ) - def test_course_does_not_expire_for_course_roles(self, role_factory): + def test_course_does_not_expire_for_course_staff(self, role_factory): """ There are a number of different roles/users that should not lose access after the expiration date. Ensure that users who should not lose access get a 200 (ok) response @@ -391,7 +398,7 @@ class TestCourseHomePageAccess(CourseHomePageTestCase): enrollment__user=user ) - # ensure that the user who has indefinite access + # ensure that the user has indefinite access self.client.login(username=user.username, password=self.TEST_PASSWORD) response = self.client.get(url) self.assertEqual( @@ -400,6 +407,32 @@ class TestCourseHomePageAccess(CourseHomePageTestCase): "Should not expire access for user", ) + @ddt.data( + FORUM_ROLE_COMMUNITY_TA, + FORUM_ROLE_GROUP_MODERATOR, + FORUM_ROLE_MODERATOR, + FORUM_ROLE_ADMINISTRATOR + ) + def test_course_does_not_expire_for_user_with_course_role(self, role_name): + """ + Test that users with the above roles for a course do not lose access + """ + course = CourseFactory.create(start=THREE_YEARS_AGO) + url = course_home_url(course) + + user = UserFactory.create() + role = RoleFactory(name=role_name, course_id=course.id) + role.users.add(user) + + # ensure the user has indefinite access + self.client.login(username=user.username, password=self.TEST_PASSWORD) + response = self.client.get(url) + self.assertEqual( + response.status_code, + 200, + "Should not expire access for user" + ) + @mock.patch.dict(settings.FEATURES, {'DISABLE_START_DATES': False}) @ddt.data( GlobalStaffFactory, 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 809640d55e..51defb96ef 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(52, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): + with self.assertNumQueries(56, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): with check_mongo_calls(4): url = course_updates_url(self.course) self.client.get(url) diff --git a/openedx/tests/settings.py b/openedx/tests/settings.py index 652568f937..988ea74c36 100644 --- a/openedx/tests/settings.py +++ b/openedx/tests/settings.py @@ -54,6 +54,7 @@ DATABASES = { FEATURES = {} INSTALLED_APPS = ( + 'django_comment_common', 'django.contrib.auth', 'django.contrib.contenttypes', 'django.contrib.sessions',