Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #27646 from edx/jhynes/microba-1209_function-cleanup

Browse Source 
refactor: consolidate (mostly) duplicated function
This commit is contained in:
Justin Hynes
2021-05-17 12:59:42 -04:00
committed by GitHub
parent 563a762625 d4f3e11092
commit e0cfae745f
9 changed files with 56 additions and 73 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
lms/djangoapps/ccx/api/v0/tests/test_views.py
View File

@@ -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

56
lms/djangoapps/ccx/tests/test_utils.py
View File

@@ -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

8
lms/djangoapps/ccx/tests/test_views.py
View File

@@ -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

16
lms/djangoapps/ccx/utils.py
View File

@@ -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:

4
lms/djangoapps/certificates/generation.py
View File

@@ -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

6
lms/djangoapps/certificates/generation_handler.py
View File

@@ -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

20
lms/djangoapps/instructor/access.py
View File

@@ -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):

9
lms/djangoapps/instructor/tests/test_access.py
View File

@@ -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)

2
lms/djangoapps/instructor/views/api.py
View File

@@ -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)