diff --git a/lms/djangoapps/ccx/api/v0/tests/test_views.py b/lms/djangoapps/ccx/api/v0/tests/test_views.py index 3b42b1b92d..5303196560 100644 --- a/lms/djangoapps/ccx/api/v0/tests/test_views.py +++ b/lms/djangoapps/ccx/api/v0/tests/test_views.py @@ -661,12 +661,12 @@ class CcxListTest(CcxRestApiTest): assert len(outbox) == 1 assert self.coach.email in outbox[0].recipients() - list_staff_master_course = list_with_level(self.course, 'staff') - list_instructor_master_course = list_with_level(self.course, 'instructor') + list_staff_master_course = list_with_level(self.course.id, 'staff') + list_instructor_master_course = list_with_level(self.course.id, 'instructor') course_key = CourseKey.from_string(resp.data.get('ccx_course_id')) with ccx_course_cm(course_key) as course_ccx: - list_staff_ccx_course = list_with_level(course_ccx, 'staff') - list_instructor_ccx_course = list_with_level(course_ccx, 'instructor') + list_staff_ccx_course = list_with_level(course_ccx.id, 'staff') + list_instructor_ccx_course = list_with_level(course_ccx.id, 'instructor') # The "Coach" in the parent course becomes "Staff" on the CCX, so the CCX should have 1 "Staff" # user more than the parent course diff --git a/lms/djangoapps/ccx/tests/test_utils.py b/lms/djangoapps/ccx/tests/test_utils.py index 21fbe96288..1e7a9ad326 100644 --- a/lms/djangoapps/ccx/tests/test_utils.py +++ b/lms/djangoapps/ccx/tests/test_utils.py @@ -87,15 +87,15 @@ class TestStaffOnCCX(CcxTestCase): add_master_course_staff_to_ccx(self.course, self.ccx_locator, self.ccx.display_name) # assert that staff and instructors of master course has staff and instructor roles on ccx - list_staff_master_course = list_with_level(self.course, 'staff') - list_instructor_master_course = list_with_level(self.course, 'instructor') + list_staff_master_course = list_with_level(self.course.id, 'staff') + list_instructor_master_course = list_with_level(self.course.id, 'instructor') with ccx_course(self.ccx_locator) as course_ccx: - list_staff_ccx_course = list_with_level(course_ccx, 'staff') + list_staff_ccx_course = list_with_level(course_ccx.id, 'staff') assert len(list_staff_master_course) == len(list_staff_ccx_course) assert list_staff_master_course[0].email == list_staff_ccx_course[0].email - list_instructor_ccx_course = list_with_level(course_ccx, 'instructor') + list_instructor_ccx_course = list_with_level(course_ccx.id, 'instructor') assert len(list_instructor_ccx_course) == len(list_instructor_master_course) assert list_instructor_ccx_course[0].email == list_instructor_master_course[0].email @@ -136,15 +136,15 @@ class TestStaffOnCCX(CcxTestCase): add_master_course_staff_to_ccx(self.course, self.ccx_locator, self.ccx.display_name, send_email=False) - list_staff_master_course = list_with_level(self.course, 'staff') - list_instructor_master_course = list_with_level(self.course, 'instructor') + list_staff_master_course = list_with_level(self.course.id, 'staff') + list_instructor_master_course = list_with_level(self.course.id, 'instructor') with ccx_course(self.ccx_locator) as course_ccx: - list_staff_ccx_course = list_with_level(course_ccx, 'staff') + list_staff_ccx_course = list_with_level(course_ccx.id, 'staff') assert len(list_staff_master_course) == len(list_staff_ccx_course) assert list_staff_master_course[0].email == list_staff_ccx_course[0].email - list_instructor_ccx_course = list_with_level(course_ccx, 'instructor') + list_instructor_ccx_course = list_with_level(course_ccx.id, 'instructor') assert len(list_instructor_ccx_course) == len(list_instructor_master_course) assert list_instructor_ccx_course[0].email == list_instructor_master_course[0].email @@ -152,10 +152,10 @@ class TestStaffOnCCX(CcxTestCase): remove_master_course_staff_from_ccx( self.course, self.ccx_locator, self.ccx.display_name, send_email=False ) - list_staff_ccx_course = list_with_level(course_ccx, 'staff') + list_staff_ccx_course = list_with_level(course_ccx.id, 'staff') assert len(list_staff_master_course) != len(list_staff_ccx_course) - list_instructor_ccx_course = list_with_level(course_ccx, 'instructor') + list_instructor_ccx_course = list_with_level(course_ccx.id, 'instructor') assert len(list_instructor_ccx_course) != len(list_instructor_master_course) for user in list_staff_master_course: @@ -178,15 +178,15 @@ class TestStaffOnCCX(CcxTestCase): assert len(outbox) == 0 add_master_course_staff_to_ccx(self.course, self.ccx_locator, self.ccx.display_name, send_email=False) - list_staff_master_course = list_with_level(self.course, 'staff') - list_instructor_master_course = list_with_level(self.course, 'instructor') + list_staff_master_course = list_with_level(self.course.id, 'staff') + list_instructor_master_course = list_with_level(self.course.id, 'instructor') with ccx_course(self.ccx_locator) as course_ccx: - list_staff_ccx_course = list_with_level(course_ccx, 'staff') + list_staff_ccx_course = list_with_level(course_ccx.id, 'staff') assert len(list_staff_master_course) == len(list_staff_ccx_course) assert list_staff_master_course[0].email == list_staff_ccx_course[0].email - list_instructor_ccx_course = list_with_level(course_ccx, 'instructor') + list_instructor_ccx_course = list_with_level(course_ccx.id, 'instructor') assert len(list_instructor_ccx_course) == len(list_instructor_master_course) assert list_instructor_ccx_course[0].email == list_instructor_master_course[0].email @@ -196,10 +196,10 @@ class TestStaffOnCCX(CcxTestCase): ) assert len(outbox) == (len(list_staff_master_course) + len(list_instructor_master_course)) - list_staff_ccx_course = list_with_level(course_ccx, 'staff') + list_staff_ccx_course = list_with_level(course_ccx.id, 'staff') assert len(list_staff_master_course) != len(list_staff_ccx_course) - list_instructor_ccx_course = list_with_level(course_ccx, 'instructor') + list_instructor_ccx_course = list_with_level(course_ccx.id, 'instructor') assert len(list_instructor_ccx_course) != len(list_instructor_master_course) for user in list_staff_master_course: @@ -212,10 +212,10 @@ class TestStaffOnCCX(CcxTestCase): assert len(outbox) == (len(list_staff_master_course) + len(list_instructor_master_course)) with ccx_course(self.ccx_locator) as course_ccx: - list_staff_ccx_course = list_with_level(course_ccx, 'staff') + list_staff_ccx_course = list_with_level(course_ccx.id, 'staff') assert len(list_staff_master_course) != len(list_staff_ccx_course) - list_instructor_ccx_course = list_with_level(course_ccx, 'instructor') + list_instructor_ccx_course = list_with_level(course_ccx.id, 'instructor') assert len(list_instructor_ccx_course) != len(list_instructor_master_course) for user in list_staff_master_course: @@ -238,8 +238,8 @@ class TestStaffOnCCX(CcxTestCase): outbox = self.get_outbox() # create a unique display name display_name = f'custom_display_{uuid.uuid4()}' - list_staff_master_course = list_with_level(self.course, 'staff') - list_instructor_master_course = list_with_level(self.course, 'instructor') + list_staff_master_course = list_with_level(self.course.id, 'staff') + list_instructor_master_course = list_with_level(self.course.id, 'instructor') assert len(outbox) == 0 # give access to the course staff/instructor add_master_course_staff_to_ccx(self.course, self.ccx_locator, display_name) @@ -263,8 +263,8 @@ class TestStaffOnCCX(CcxTestCase): add_master_course_staff_to_ccx(self.course, self.ccx_locator, self.ccx.display_name, send_email=False) # create a unique display name display_name = f'custom_display_{uuid.uuid4()}' - list_staff_master_course = list_with_level(self.course, 'staff') - list_instructor_master_course = list_with_level(self.course, 'instructor') + list_staff_master_course = list_with_level(self.course.id, 'staff') + list_instructor_master_course = list_with_level(self.course.id, 'instructor') assert len(outbox) == 0 # give access to the course staff/instructor remove_master_course_staff_from_ccx(self.course, self.ccx_locator, display_name) @@ -284,16 +284,16 @@ class TestStaffOnCCX(CcxTestCase): instructor = self.make_instructor() assert CourseInstructorRole(self.course.id).has_user(instructor) outbox = self.get_outbox() - list_staff_master_course = list_with_level(self.course, 'staff') - list_instructor_master_course = list_with_level(self.course, 'instructor') + list_staff_master_course = list_with_level(self.course.id, 'staff') + list_instructor_master_course = list_with_level(self.course.id, 'instructor') assert len(outbox) == 0 # run the assignment the first time add_master_course_staff_to_ccx(self.course, self.ccx_locator, self.ccx.display_name) assert len(outbox) == (len(list_staff_master_course) + len(list_instructor_master_course)) with ccx_course(self.ccx_locator) as course_ccx: - list_staff_ccx_course = list_with_level(course_ccx, 'staff') - list_instructor_ccx_course = list_with_level(course_ccx, 'instructor') + list_staff_ccx_course = list_with_level(course_ccx.id, 'staff') + list_instructor_ccx_course = list_with_level(course_ccx.id, 'instructor') assert len(list_staff_master_course) == len(list_staff_ccx_course) for user in list_staff_master_course: assert user in list_staff_ccx_course @@ -307,8 +307,8 @@ class TestStaffOnCCX(CcxTestCase): assert len(outbox) == (len(list_staff_master_course) + len(list_instructor_master_course)) # there are no duplicated staffs with ccx_course(self.ccx_locator) as course_ccx: - list_staff_ccx_course = list_with_level(course_ccx, 'staff') - list_instructor_ccx_course = list_with_level(course_ccx, 'instructor') + list_staff_ccx_course = list_with_level(course_ccx.id, 'staff') + list_instructor_ccx_course = list_with_level(course_ccx.id, 'instructor') assert len(list_staff_master_course) == len(list_staff_ccx_course) for user in list_staff_master_course: assert user in list_staff_ccx_course diff --git a/lms/djangoapps/ccx/tests/test_views.py b/lms/djangoapps/ccx/tests/test_views.py index 23d6345702..1db7780932 100644 --- a/lms/djangoapps/ccx/tests/test_views.py +++ b/lms/djangoapps/ccx/tests/test_views.py @@ -405,15 +405,15 @@ class TestCoachDashboard(CcxTestCase, LoginEnrollmentTestCase): assert role.has_user(self.coach) # assert that staff and instructors of master course has staff and instructor roles on ccx - list_staff_master_course = list_with_level(self.course, 'staff') - list_instructor_master_course = list_with_level(self.course, 'instructor') + list_staff_master_course = list_with_level(self.course.id, 'staff') + list_instructor_master_course = list_with_level(self.course.id, 'instructor') # assert that forum roles are seeded assert are_permissions_roles_seeded(course_key) assert has_forum_access(self.coach.username, course_key, FORUM_ROLE_ADMINISTRATOR) with ccx_course(course_key) as course_ccx: - list_staff_ccx_course = list_with_level(course_ccx, 'staff') + list_staff_ccx_course = list_with_level(course_ccx.id, 'staff') # The "Coach" in the parent course becomes "Staff" on the CCX, so the CCX should have 1 "Staff" # user more than the parent course assert (len(list_staff_master_course) + 1) == len(list_staff_ccx_course) @@ -421,7 +421,7 @@ class TestCoachDashboard(CcxTestCase, LoginEnrollmentTestCase): # Make sure the "Coach" on the parent course is "Staff" on the CCX assert self.coach in list_staff_ccx_course - list_instructor_ccx_course = list_with_level(course_ccx, 'instructor') + list_instructor_ccx_course = list_with_level(course_ccx.id, 'instructor') assert len(list_instructor_ccx_course) == len(list_instructor_master_course) assert list_instructor_ccx_course[0].email == list_instructor_master_course[0].email diff --git a/lms/djangoapps/ccx/utils.py b/lms/djangoapps/ccx/utils.py index 68f2d8326a..c6e088b3fb 100644 --- a/lms/djangoapps/ccx/utils.py +++ b/lms/djangoapps/ccx/utils.py @@ -332,13 +332,13 @@ def add_master_course_staff_to_ccx(master_course, ccx_key, display_name, send_em send_email (bool): flag to switch on or off email to the users on access grant. """ - list_staff = list_with_level(master_course, 'staff') - list_instructor = list_with_level(master_course, 'instructor') + list_staff = list_with_level(master_course.id, 'staff') + list_instructor = list_with_level(master_course.id, 'instructor') with ccx_course(ccx_key) as course_ccx: email_params = get_email_params(course_ccx, auto_enroll=True, course_key=ccx_key, display_name=display_name) - list_staff_ccx = list_with_level(course_ccx, 'staff') - list_instructor_ccx = list_with_level(course_ccx, 'instructor') + list_staff_ccx = list_with_level(course_ccx.id, 'staff') + list_instructor_ccx = list_with_level(course_ccx.id, 'instructor') for staff in list_staff: # this call should be idempotent if staff not in list_staff_ccx: @@ -401,12 +401,12 @@ def remove_master_course_staff_from_ccx(master_course, ccx_key, display_name, se send_email (bool): flag to switch on or off email to the users on revoke access. """ - list_staff = list_with_level(master_course, 'staff') - list_instructor = list_with_level(master_course, 'instructor') + list_staff = list_with_level(master_course.id, 'staff') + list_instructor = list_with_level(master_course.id, 'instructor') with ccx_course(ccx_key) as course_ccx: - list_staff_ccx = list_with_level(course_ccx, 'staff') - list_instructor_ccx = list_with_level(course_ccx, 'instructor') + list_staff_ccx = list_with_level(course_ccx.id, 'staff') + list_instructor_ccx = list_with_level(course_ccx.id, 'instructor') email_params = get_email_params(course_ccx, auto_enroll=True, course_key=ccx_key, display_name=display_name) for staff in list_staff: if staff in list_staff_ccx: diff --git a/lms/djangoapps/certificates/generation.py b/lms/djangoapps/certificates/generation.py index c49706dd7f..15d35f3069 100644 --- a/lms/djangoapps/certificates/generation.py +++ b/lms/djangoapps/certificates/generation.py @@ -17,7 +17,7 @@ from lms.djangoapps.certificates.models import CertificateStatuses, GeneratedCer from lms.djangoapps.certificates.queue import XQueueCertInterface from lms.djangoapps.certificates.utils import emit_certificate_event, has_html_certificates_enabled from lms.djangoapps.grades.api import CourseGradeFactory -from lms.djangoapps.instructor.access import list_with_level_from_course_key +from lms.djangoapps.instructor.access import list_with_level from openedx.core.djangoapps.content.course_overviews.api import get_course_overview log = logging.getLogger(__name__) @@ -122,7 +122,7 @@ def generate_user_certificates(student, course_key, insecure=False, generation_m forced_grade - a string indicating to replace grade parameter. if present grading will be skipped. """ - beta_testers_queryset = list_with_level_from_course_key(course_key, 'beta') + beta_testers_queryset = list_with_level(course_key, 'beta') if beta_testers_queryset.filter(username=student.username): log.info(f"Canceling Certificate Generation task for user {student.id} : {course_key}. User is a Beta Tester.") return diff --git a/lms/djangoapps/certificates/generation_handler.py b/lms/djangoapps/certificates/generation_handler.py index aa8a1e59da..978762d946 100644 --- a/lms/djangoapps/certificates/generation_handler.py +++ b/lms/djangoapps/certificates/generation_handler.py @@ -25,7 +25,7 @@ from lms.djangoapps.certificates.utils import ( has_html_certificates_enabled ) from lms.djangoapps.grades.api import CourseGradeFactory -from lms.djangoapps.instructor.access import list_with_level_from_course_key +from lms.djangoapps.instructor.access import list_with_level from lms.djangoapps.verify_student.services import IDVerificationService from openedx.core.djangoapps.content.course_overviews.api import get_course_overview from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag @@ -371,7 +371,7 @@ def _is_beta_tester(user, course_key): """ Check if the user is a beta tester in this course run """ - beta_testers_queryset = list_with_level_from_course_key(course_key, 'beta') + beta_testers_queryset = list_with_level(course_key, 'beta') return beta_testers_queryset.filter(username=user.username).exists() @@ -443,7 +443,7 @@ def generate_user_certificates(student, course_key, insecure=False, generation_m f'{student.id}.') return generate_certificate_task(student, course_key) - beta_testers_queryset = list_with_level_from_course_key(course_key, 'beta') + beta_testers_queryset = list_with_level(course_key, 'beta') if beta_testers_queryset.filter(username=student.username): log.info(f"Canceling Certificate Generation task for user {student.id} : {course_key}. User is a Beta Tester.") return diff --git a/lms/djangoapps/instructor/access.py b/lms/djangoapps/instructor/access.py index 15df31bc51..d1f8010171 100644 --- a/lms/djangoapps/instructor/access.py +++ b/lms/djangoapps/instructor/access.py @@ -33,7 +33,7 @@ ROLES = { } -def list_with_level(course, level): +def list_with_level(course_id, level): """ List users who have 'level' access. @@ -41,23 +41,7 @@ def list_with_level(course, level): There could be other levels specific to the course. If there is no Group for that course-level, returns an empty list """ - return ROLES[level](course.id).users_with_role() - - -def list_with_level_from_course_key(course_key, level): - """ - List users who have 'level' access. - - The 'level' value can be 'instructor', 'staff', or 'beta' for standard courses. - - It is possible for other levels to be defined specific to the course. If there is no group for that level we return - an empty list. - - This is a companion function to the `list_with_level` function. We are in the process of refactoring and removing - the `Certificates` apps dependence on `modulestore`. These functions will be consolidated at a later date. Progress - is being tracked in MICROBA-1178. - """ - return ROLES[level](course_key).users_with_role() + return ROLES[level](course_id).users_with_role() def allow_access(course, user, level, send_email=True): diff --git a/lms/djangoapps/instructor/tests/test_access.py b/lms/djangoapps/instructor/tests/test_access.py index 8d99ad8f95..f061066949 100644 --- a/lms/djangoapps/instructor/tests/test_access.py +++ b/lms/djangoapps/instructor/tests/test_access.py @@ -10,7 +10,6 @@ from common.djangoapps.student.tests.factories import UserFactory from lms.djangoapps.instructor.access import ( allow_access, list_with_level, - list_with_level_from_course_key, revoke_access, update_forum_role ) @@ -37,14 +36,14 @@ class TestInstructorAccessList(SharedModuleStoreTestCase): allow_access(self.course, user, 'beta') def test_list_instructors(self): - instructors = list_with_level(self.course, 'instructor') - instructors_alternative = list_with_level_from_course_key(self.course.id, 'instructor') + instructors = list_with_level(self.course.id, 'instructor') + instructors_alternative = list_with_level(self.course.id, 'instructor') assert set(instructors) == set(self.instructors) assert set(instructors_alternative) == set(self.instructors) def test_list_beta(self): - beta_testers = list_with_level(self.course, 'beta') - beta_testers_alternative = list_with_level_from_course_key(self.course.id, 'beta') + beta_testers = list_with_level(self.course.id, 'beta') + beta_testers_alternative = list_with_level(self.course.id, 'beta') assert set(beta_testers) == set(self.beta_testers) assert set(beta_testers_alternative) == set(self.beta_testers) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index c2a2e0bbe2..b0c5f687ac 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -963,7 +963,7 @@ def list_course_role_members(request, course_id): response_payload = { 'course_id': str(course_id), rolename: list(map(extract_user_info, list_with_level( - course, rolename + course.id, rolename ))), } return JsonResponse(response_payload)