Skip to content

Commit

Permalink
fix: linter and test updates
Browse files Browse the repository at this point in the history
  • Loading branch information
hsinkoff committed Jan 12, 2024
1 parent 396876f commit 7c94e63
Show file tree
Hide file tree
Showing 17 changed files with 68 additions and 55 deletions.
6 changes: 3 additions & 3 deletions lms/djangoapps/course_api/blocks/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -222,12 +222,12 @@ def test_query_counts_cached(self, store_type, with_storage_backing):
self._get_blocks(
course,
expected_mongo_queries=0,
expected_sql_queries=22 if with_storage_backing else 21,
expected_sql_queries=21 if with_storage_backing else 20,
)

@ddt.data(
(ModuleStoreEnum.Type.split, 2, True, 31),
(ModuleStoreEnum.Type.split, 2, False, 21),
(ModuleStoreEnum.Type.split, 2, True, 30),
(ModuleStoreEnum.Type.split, 2, False, 20),
)
@ddt.unpack
def test_query_counts_uncached(self, store_type, expected_mongo_queries, with_storage_backing, num_sql_queries):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ def test_gated(self, gated_block_ref, gating_block_ref, expected_blocks_before_c
self.course.enable_subsection_gating = True
self.setup_gated_section(self.blocks[gated_block_ref], self.blocks[gating_block_ref])

with self.assertNumQueries(5):
with self.assertNumQueries(6):
self.get_blocks_and_check_against_expected(self.user, expected_blocks_before_completion)

# clear the request cache to simulate a new request
Expand All @@ -179,7 +179,7 @@ def test_gated(self, gated_block_ref, gating_block_ref, expected_blocks_before_c
self.user,
)

with self.assertNumQueries(6):
with self.assertNumQueries(7):
self.get_blocks_and_check_against_expected(self.user, self.ALL_BLOCKS_EXCEPT_SPECIAL)

def test_staff_access(self):
Expand Down
14 changes: 8 additions & 6 deletions lms/djangoapps/courseware/access.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ def has_ccx_coach_role(user, course_key):
role = CourseCcxCoachRole(course_key)

# TODO: remove role check once course_roles is fully impelented and data is migrated
if role.has_user(user) or user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name):
if role.has_user(user) or user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name, course_key):
list_ccx = CustomCourseForEdX.objects.filter(
course_id=course_key.to_course_locator(),
coach=user
Expand Down Expand Up @@ -180,7 +180,7 @@ def has_staff_access_to_preview_mode(user, course_key):
return (
has_admin_access_to_course or
is_masquerading_as_student(user, course_key) or
user.has_perm(CourseRolesPermission.VIEW_ALL_CONTENT.perm_name)
user.has_perm(CourseRolesPermission.VIEW_ALL_CONTENT.perm_name, course_key)
)


Expand Down Expand Up @@ -728,7 +728,7 @@ def _has_access_to_course(user, access_level, course_key):
return ACCESS_DENIED

global_staff, staff_access, instructor_access = administrative_accesses_to_course_for_user(user, course_key)
permissions_access = user.has_perm(CourseRolesPermission.VIEW_ALL_CONTENT.perm_name)
permissions_access = user.has_perm(CourseRolesPermission.VIEW_ALL_CONTENT.perm_name, course_key)
if global_staff:
debug("Allow: user.is_staff")
return ACCESS_GRANTED
Expand All @@ -745,7 +745,7 @@ def _has_access_to_course(user, access_level, course_key):
if instructor_access and access_level in ('staff', 'instructor'):
debug("Allow: user has course instructor access")
return ACCESS_GRANTED

if permissions_access:
debug("Allow: user has view all content permission")
return ACCESS_GRANTED
Expand Down Expand Up @@ -892,8 +892,10 @@ def is_mobile_available_for_user(user, block):
block (CourseBlock|CourseOverview): course or overview of course in question
"""
permissions_access = (
user.has_perm(CourseRolesPermission.VIEW_LIVE_PUBLISHED_CONTENT.perm_name) and
user.has_perm(CourseRolesPermission.VIEW_ALL_PUBLISHED_CONTENT.perm_name)
user.has_perms([
CourseRolesPermission.VIEW_LIVE_PUBLISHED_CONTENT.perm_name,
CourseRolesPermission.VIEW_ALL_PUBLISHED_CONTENT.perm_name
], block.id)
)
# TODO: remove role check once course_roles is fully impelented and data is migrated
return (
Expand Down
2 changes: 1 addition & 1 deletion lms/djangoapps/courseware/access_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def adjust_start_date(user, days_early_for_beta, start, course_key):
# TODO: remove role check once course_roles is fully impelented and data is migrated
if (
CourseBetaTesterRole(course_key).has_user(user) or
user.has_perm(CourseRolesPermission.VIEW_LIVE_PUBLISHED_CONTENT.perm_name)
user.has_perm(CourseRolesPermission.VIEW_LIVE_PUBLISHED_CONTENT.perm_name, course_key)
):
debug("Adjust start time: user in beta role for %s", course_key)
delta = timedelta(days_early_for_beta)
Expand Down
20 changes: 14 additions & 6 deletions lms/djangoapps/courseware/block_render.py
Original file line number Diff line number Diff line change
Expand Up @@ -574,8 +574,10 @@ def inner_get_block(block: XBlock) -> XBlock | None:
block_wrappers.append(partial(offer_banner_wrapper, user))

has_permission = (
user.has_perm(CourseRolesPermission.VIEW_ALL_CONTENT.perm_name) and
user.has_perm(CourseRolesPermission.SPECIFIC_MASQUERADING.perm_name)
user.has_perms([
CourseRolesPermission.VIEW_ALL_CONTENT.perm_name,
CourseRolesPermission.SPECIFIC_MASQUERADING.perm_name
], course_id)
)
# TODO: remove role check once course_roles is fully impelented and data is migrated
user_is_staff = (
Expand Down Expand Up @@ -608,10 +610,16 @@ def inner_get_block(block: XBlock) -> XBlock | None:
# See the docstring of `DjangoXBlockUserService`.
deprecated_anonymous_user_id=anonymous_id_for_user(user, None),
request_country_code=user_location,
user_has_manage_content=user.has_perm(CourseRolesPermission.MANAGE_CONTENT.perm_name),
user_has_manage_grades=user.has_perm(CourseRolesPermission.MANAGE_GRADES.perm_name),
user_has_access_data_downloads=user.has_perm(CourseRolesPermission.ACCESS_DATA_DOWNLOADS.perm_name),
user_has_view_all_content=user.has_perm(CourseRolesPermission.VIEW_ALL_CONTENT.perm_name)
user_has_manage_content=user.has_perm(CourseRolesPermission.MANAGE_CONTENT.perm_name, course_id),
user_has_manage_grades=user.has_perm(CourseRolesPermission.MANAGE_GRADES.perm_name, course_id),
user_has_access_data_downloads=user.has_perm(
CourseRolesPermission.ACCESS_DATA_DOWNLOADS.perm_name,
course_id
),
user_has_view_all_content=user.has_perm(
CourseRolesPermission.VIEW_ALL_CONTENT.perm_name,
course_id
)
),
'verification': XBlockVerificationService(),
'proctoring': ProctoringService(),
Expand Down
18 changes: 9 additions & 9 deletions lms/djangoapps/courseware/masquerade.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,8 @@ def get(self, request, course_key_string):
# TODO: remove role check once course_roles is fully impelented and data is migrated
is_staff = has_staff_roles(request.user, course_key)
has_permissions = (
request.user.has_perm(CourseRolesPermission.GENERAL_MASQUERADING.perm_name) or
request.user.has_perm(CourseRolesPermission.SPECIFIC_MASQUERADING.perm_name)
request.user.has_perm(CourseRolesPermission.GENERAL_MASQUERADING.perm_name, course_key) or
request.user.has_perm(CourseRolesPermission.SPECIFIC_MASQUERADING.perm_name, course_key)
)
if not is_staff and not has_permissions:
return JsonResponse({
Expand Down Expand Up @@ -174,8 +174,8 @@ def post(self, request, course_key_string):
# TODO: remove role check once course_roles is fully impelented and data is migrated
is_staff = has_staff_roles(request.user, course_key)
has_permissions = (
request.user.has_perm(CourseRolesPermission.GENERAL_MASQUERADING.perm_name) or
request.user.has_perm(CourseRolesPermission.SPECIFIC_MASQUERADING.perm_name)
request.user.has_perm(CourseRolesPermission.GENERAL_MASQUERADING.perm_name, course_key) or
request.user.has_perm(CourseRolesPermission.SPECIFIC_MASQUERADING.perm_name, course_key)
)
if not is_staff and not has_permissions:
return JsonResponse({
Expand Down Expand Up @@ -409,12 +409,12 @@ def check_content_start_date_for_masquerade_user(course_key, user, request, cour
# TODO: remove role check once course_roles is fully impelented and data is migrated
is_staff = has_staff_roles(user, course_key)
has_permissions = (
user.has_perm(CourseRolesPermission.SPECIFIC_MASQUERADING.perm_name) and
user.has_perm(CourseRolesPermission.SPECIFIC_MASQUERADING.perm_name, course_key) and
(
user.has_perm(CourseRolesPermission.MANAGE_CONTENT.perm_name) or
user.has_perm(CourseRolesPermission.VIEW_ALL_CONTENT.perm_name) or
user.has_perm(CourseRolesPermission.VIEW_LIVE_AND_PUBLISHED_CONTENT.perm_name) or
user.has_perm(CourseRolesPermission.VIEW_ALL_PUBLISHED_CONTENT.perm_name)
user.has_perm(CourseRolesPermission.MANAGE_CONTENT.perm_name, course_key) or
user.has_perm(CourseRolesPermission.VIEW_ALL_CONTENT.perm_name, course_key) or
user.has_perm(CourseRolesPermission.VIEW_LIVE_AND_PUBLISHED_CONTENT.perm_name, course_key) or
user.has_perm(CourseRolesPermission.VIEW_ALL_PUBLISHED_CONTENT.perm_name, course_key)
)
)
if group_masquerade or (
Expand Down
4 changes: 2 additions & 2 deletions lms/djangoapps/courseware/tests/test_discussion_xblock.py
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ def test_permissions_query_load(self):
# * django_comment_client_role
# * DiscussionsConfiguration

num_queries = 6
num_queries = 9

for discussion in discussions:
discussion_xblock = get_block_for_descriptor(
Expand All @@ -466,7 +466,7 @@ def test_permissions_query_load(self):

# query to check for provider_type
# query to check waffle flag discussions.enable_new_structure_discussions
num_queries = 2
num_queries = 5

html = fragment.content
assert 'data-user-create-comment="false"' in html
Expand Down
8 changes: 4 additions & 4 deletions lms/djangoapps/grades/tests/test_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,8 @@ def test_block_structure_created_only_once(self):
assert mock_block_structure_create.call_count == 1

@ddt.data(
(ModuleStoreEnum.Type.split, 2, 49, True),
(ModuleStoreEnum.Type.split, 2, 49, False),
(ModuleStoreEnum.Type.split, 2, 48, True),
(ModuleStoreEnum.Type.split, 2, 48, False),
)
@ddt.unpack
def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, create_multiple_subsections):
Expand All @@ -165,7 +165,7 @@ def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, creat
self._apply_recalculate_subsection_grade()

@ddt.data(
(ModuleStoreEnum.Type.split, 2, 49),
(ModuleStoreEnum.Type.split, 2, 48),
)
@ddt.unpack
def test_query_counts_dont_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls):
Expand Down Expand Up @@ -210,7 +210,7 @@ def test_other_inaccessible_subsection(self, mock_subsection_signal):
)

@ddt.data(
(ModuleStoreEnum.Type.split, 2, 49),
(ModuleStoreEnum.Type.split, 2, 48),
)
@ddt.unpack
def test_persistent_grades_on_course(self, default_store, num_mongo_queries, num_sql_queries):
Expand Down
2 changes: 1 addition & 1 deletion lms/djangoapps/learner_home/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ def check_course_access(user, course_enrollments):
administrative_accesses_to_course_for_user(
user, course_enrollment.course_id
) or
user.has_perm(CourseRolesPermission.VIEW_ALL_CONTENT.perm_name)
user.has_perm(CourseRolesPermission.VIEW_ALL_CONTENT.perm_name, course_enrollment.course_id)
),
}

Expand Down
18 changes: 9 additions & 9 deletions lms/djangoapps/teams/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ def has_course_staff_privileges(user, course_key):
if CourseInstructorRole(course_key).has_user(user):
return True
# TODO: remove role checks once course_roles is fully impelented and data is migrated
if user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name):
if user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name, course_key):
return True
return False

Expand All @@ -167,7 +167,7 @@ def has_team_api_access(user, course_key, access_username=None):
# TODO: remove role checks once course_roles is fully impelented and data is migrated
if (
has_course_staff_privileges(user, course_key) or
user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name)
user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name, course_key)
):
return True
if has_discussion_privileges(user, course_key):
Expand All @@ -187,7 +187,7 @@ def user_organization_protection_status(user, course_key):
# TODO: remove role checks once course_roles is fully impelented and data is migrated
if (
has_course_staff_privileges(user, course_key) or
user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name)
user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name, course_key)
):
return OrganizationProtectionStatus.protection_exempt
enrollment = CourseEnrollment.get_enrollment(user, course_key)
Expand All @@ -213,7 +213,7 @@ def has_specific_team_access(user, team):
- be in the team if it is private
"""
# TODO: remove role checks once course_roles is fully impelented and data is migrated
user_has_manage_student_permission = user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name)
user_has_manage_student_permission = user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name, team.course_id)
return (
has_course_staff_privileges(user, team.course_id) or
user_has_manage_student_permission or (
Expand All @@ -229,10 +229,10 @@ def has_specific_teamset_access(user, course_block, teamset_id):
Non-staff users only have access to a private_managed teamset if they are in a team in that teamset
"""
# TODO: remove role checks once course_roles is fully impelented and data is migrated
user_has_manage_student_permission = user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name)
user_has_manage_student_permission = user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name, course_block.id)
return (
has_course_staff_privileges(user, course_block.id) or
user_has_manage_student_permission or \
user_has_manage_student_permission or
teamset_is_public_or_user_is_on_team_in_teamset(user, course_block, teamset_id)
)

Expand Down Expand Up @@ -348,7 +348,7 @@ def can_user_modify_team(user, team):
return (
(not is_instructor_managed_team(team)) or
has_course_staff_privileges(user, team.course_id) or
user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name)
user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name, team.course_id)
)


Expand All @@ -362,7 +362,7 @@ def can_user_create_team_in_topic(user, course_id, topic_id):
return (
(not is_instructor_managed_topic(course_id, topic_id)) or
has_course_staff_privileges(user, course_id) or
user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name)
user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name, course_id)
)


Expand Down Expand Up @@ -430,7 +430,7 @@ def anonymous_user_ids_for_team(user, team):
# TODO: remove role checks once course_roles is fully impelented and data is migrated
if (
not has_course_staff_privileges(user, team.course_id) and not
user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name)
user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name, team.course_id)
) and not user_is_a_team_member(user, team):
raise Exception("User {user} is not permitted to access team info for {team}".format(
user=user.username,
Expand Down
3 changes: 2 additions & 1 deletion lms/djangoapps/teams/tests/test_serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,8 @@ def assert_serializer_output(self, topics, num_teams_per_topic, num_queries):
request = RequestFactory().get('/api/team/v0/topics')
request.user = self.user

with self.assertNumQueries(num_queries + 2): # num_queries on teams tables, plus 2 split modulestore queries
with self.assertNumQueries(num_queries + 3):
# num_queries on teams tables, plus 2 split modulestore queries, plus a perms query
serializer = self.serializer(
topics,
context={
Expand Down
10 changes: 6 additions & 4 deletions lms/djangoapps/teams/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -1072,8 +1072,10 @@ def _filter_hidden_private_teamsets(user, teamsets, course_block):
Follows the same logic as `has_specific_teamset_access` but in bulk rather than for one teamset at a time
"""
# TODO: remove role checks once course_roles is fully impelented and data is migrated
if (has_course_staff_privileges(user, course_block.id) or
user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name)):
if (
has_course_staff_privileges(user, course_block.id) or
user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name, course_block.id)
):
return teamsets
private_teamset_ids = [teamset.teamset_id for teamset in course_block.teamsets if teamset.is_private_managed]
teamset_ids_user_has_access_to = set(
Expand Down Expand Up @@ -1390,7 +1392,7 @@ def get(self, request): # lint-amnesty, pylint: disable=too-many-statements
# TODO: remove role checks once course_roles is fully impelented and data is migrated
if (
has_course_staff_privileges(request.user, requested_course_key) or
request.user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name)
request.user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name, requested_course_key)
):
teams_with_access = list(teamset_teams)
else:
Expand Down Expand Up @@ -1699,7 +1701,7 @@ def check_access(self):
# TODO: remove role checks once course_roles is fully impelented and data is migrated
if not (
has_course_staff_privileges(self.request.user, self.course.id) or
self.request.user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name)
self.request.user.has_perm(CourseRolesPermission.MANAGE_STUDENTS.perm_name, self.course.id)
):
raise PermissionDenied(
"To manage team membership of {}, you must be course staff.".format(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,10 @@ def load_data(self, full_course_outline):
# TODO: remove role checks once course_roles is fully impelented and data is migrated
self._is_beta_tester = (
user_has_role(self.user, CourseBetaTesterRole(self.course_key)) or
self.user.has_perm(CourseRolesPermission.VIEW_ALL_PUBLISHED_CONTENT.perm_name) or
self.user.has_perm(CourseRolesPermission.VIEW_LIVE_PUBLISHED_CONTENT.perm_name)
self.user.has_perm(CourseRolesPermission.VIEW_ALL_PUBLISHED_CONTENT.perm_name, self.course_key) or
self.user.has_perm(CourseRolesPermission.VIEW_LIVE_PUBLISHED_CONTENT.perm_name, self.course_key)
)


def inaccessible_sequences(self, full_course_outline):
"""
This might include Sequences that have not yet started, or Sequences
Expand Down
2 changes: 1 addition & 1 deletion openedx/core/djangoapps/schedules/tests/test_resolvers.py
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ def create_resolver(self, user_start_date_offset=8):
def test_schedule_context(self):
resolver = self.create_resolver()
# using this to make sure the select_related stays intact
with self.assertNumQueries(38):
with self.assertNumQueries(50):
sc = resolver.get_schedules()
schedules = list(sc)
apple_logo_url = 'http://email-media.s3.amazonaws.com/edX/2021/store_apple_229x78.jpg'
Expand Down
3 changes: 2 additions & 1 deletion openedx/features/content_type_gating/tests/test_access.py
Original file line number Diff line number Diff line change
Expand Up @@ -1190,9 +1190,10 @@ def test_content_type_gate_for_block(self):
self.user, blocks_dict['not_graded_1'], course['course'].id
) is None

@patch.object(ContentTypeGatingService, '_get_user', return_value=UserFactory.build())
@patch.object(ContentTypeGatingService, '_get_user')
def test_check_children_for_content_type_gating_paywall(self, mocked_user): # pylint: disable=unused-argument
''' Verify that the method returns a content type gate when appropriate '''
mocked_user.return_value = self.user
course = self._create_course()
blocks_dict = course['blocks']
CourseEnrollmentFactory.create(
Expand Down
Loading

0 comments on commit 7c94e63

Please sign in to comment.