From 1eb00da4bf07ca0a2c79b7944bc589a62d857685 Mon Sep 17 00:00:00 2001 From: Tehreem Sadat Date: Sat, 2 Nov 2024 01:03:30 +1300 Subject: [PATCH] feat: add library roles support for post and update roles --- .../helpers/constants.py | 11 +++++-- .../helpers/extractors.py | 9 +++++- futurex_openedx_extensions/helpers/roles.py | 22 +++++++++++++ .../xmodule/modulestore/django.py | 6 ++++ tests/test_helpers/test_roles.py | 31 ++++++++++++++++++- 5 files changed, 74 insertions(+), 5 deletions(-) create mode 100644 test_utils/edx_platform_mocks/xmodule/modulestore/django.py diff --git a/futurex_openedx_extensions/helpers/constants.py b/futurex_openedx_extensions/helpers/constants.py index a175e49e..0aee9a38 100644 --- a/futurex_openedx_extensions/helpers/constants.py +++ b/futurex_openedx_extensions/helpers/constants.py @@ -29,11 +29,15 @@ CLICKHOUSE_QUERY_SLUG_PATTERN = r'[a-z0-9_\-.]+' -COURSE_ID_PART = r'[a-zA-Z0-9_-]+' +ID_PART = r'[a-zA-Z0-9_-]+' COURSE_ID_REGX = \ - fr'(?Pcourse-v1:(?P{COURSE_ID_PART})\+(?P{COURSE_ID_PART})\+(?P{COURSE_ID_PART}))' + fr'(?Pcourse-v1:(?P{ID_PART})\+(?P{ID_PART})\+(?P{ID_PART}))' COURSE_ID_REGX_EXACT = rf'^{COURSE_ID_REGX}$' +LIBRARY_ID_REGX = \ + fr'(?Plibrary-v1:(?P{ID_PART})\+(?P{ID_PART}))' +LIBRARY_ID_REGX_EXACT = rf'^{LIBRARY_ID_REGX}$' + COURSE_STATUSES = { 'active': 'active', 'archived': 'archived', @@ -57,10 +61,12 @@ ] COURSE_ACCESS_ROLES_STAFF_EDITOR = 'staff' +COURSE_ACCESS_ROLES_LIBRARY_USER = 'library_user' COURSE_ACCESS_ROLES_TENANT_OR_COURSE = [ 'data_researcher', 'instructor', + COURSE_ACCESS_ROLES_LIBRARY_USER, COURSE_ACCESS_ROLES_STAFF_EDITOR, ] COURSE_ACCESS_ROLES_GLOBAL = [ @@ -78,7 +84,6 @@ COURSE_ACCESS_ROLES_ACCEPT_COURSE_ID = COURSE_ACCESS_ROLES_COURSE_ONLY + COURSE_ACCESS_ROLES_TENANT_OR_COURSE COURSE_ACCESS_ROLES_UNSUPPORTED = [ - 'library_user', # not supported yet, it requires a library ID instead of a course ID 'sales_admin', # won't be supported, looks like a deprecated role, there is no useful code for it ] diff --git a/futurex_openedx_extensions/helpers/extractors.py b/futurex_openedx_extensions/helpers/extractors.py index 676b9fc9..8a7fe8fd 100644 --- a/futurex_openedx_extensions/helpers/extractors.py +++ b/futurex_openedx_extensions/helpers/extractors.py @@ -7,6 +7,7 @@ from urllib.parse import urlparse from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from xmodule.modulestore.django import modulestore from futurex_openedx_extensions.helpers import constants as cs @@ -122,7 +123,7 @@ def verify_course_ids(course_ids: List[str]) -> None: for course_id in course_ids: if not isinstance(course_id, str): raise ValueError(f'course_id must be a string, but got {type(course_id).__name__}') - if not re.search(cs.COURSE_ID_REGX_EXACT, course_id): + if not re.search(cs.COURSE_ID_REGX_EXACT, course_id) and not re.search(cs.LIBRARY_ID_REGX_EXACT, course_id): raise ValueError(f'Invalid course ID format: {course_id}') @@ -144,6 +145,12 @@ def get_orgs_of_courses(course_ids: List[str]) -> Dict[str, Any]: 'courses': {str(course.id): course.org.lower() for course in courses}, } + result['courses'].update({ + str(lib_key): lib_key.org + for lib_key in modulestore().get_library_keys() + if str(lib_key) in course_ids + }) + invalid_course_ids = [course_id for course_id in course_ids if course_id not in result['courses']] if invalid_course_ids: raise ValueError(f'Invalid course IDs provided: {invalid_course_ids}') diff --git a/futurex_openedx_extensions/helpers/roles.py b/futurex_openedx_extensions/helpers/roles.py index a67cc4ee..7d7ecea3 100644 --- a/futurex_openedx_extensions/helpers/roles.py +++ b/futurex_openedx_extensions/helpers/roles.py @@ -3,6 +3,7 @@ from __future__ import annotations import logging +import re from copy import deepcopy from enum import Enum from typing import Any, Dict, List, Tuple @@ -44,6 +45,19 @@ logger = logging.getLogger(__name__) +def are_all_library_ids(course_ids: List[str] | None) -> bool: + """ + Verify if all given ids are library ids + """ + if not course_ids: + return False + + for course_id in course_ids: + if not re.match(cs.LIBRARY_ID_REGX, course_id): + return False + return True + + def validate_course_access_role(course_access_role: dict) -> None: """ Validate the course access role. @@ -60,6 +74,11 @@ def check_broken_rule(broken_rule: bool, code: FXExceptionCodes, message: str) - org = course_access_role['org'].strip() course_id = course_access_role['course_id'] role = course_access_role['role'] + check_broken_rule( + role == cs.COURSE_ACCESS_ROLES_LIBRARY_USER and course_id and not are_all_library_ids([course_id]), + FXExceptionCodes.ROLE_INVALID_ENTRY, + f'role {role} is only valid with library_id', + ) check_broken_rule( role not in cs.COURSE_ACCESS_ROLES_ALL, @@ -1155,6 +1174,9 @@ def add_course_access_roles( # pylint: disable=too-many-arguments, too-many-bra if (tenant_wide and course_ids) or (not tenant_wide and not course_ids): raise ValueError('Conflict between tenant_wide and course_ids') + if role == cs.COURSE_ACCESS_ROLES_LIBRARY_USER and not are_all_library_ids(course_ids): + raise ValueError(f'Role: {role} is only valid for library ids.') + if not user_keys: raise ValueError('No users provided!') diff --git a/test_utils/edx_platform_mocks/xmodule/modulestore/django.py b/test_utils/edx_platform_mocks/xmodule/modulestore/django.py new file mode 100644 index 00000000..a0dcf00f --- /dev/null +++ b/test_utils/edx_platform_mocks/xmodule/modulestore/django.py @@ -0,0 +1,6 @@ +"""Mock modulestore""" + + +class modulestore: # pylint: disable=invalid-name, too-few-public-methods + def get_library_keys(self): # pylint: disable=no-self-use + return [] diff --git a/tests/test_helpers/test_roles.py b/tests/test_helpers/test_roles.py index 188314ef..f48839ca 100644 --- a/tests/test_helpers/test_roles.py +++ b/tests/test_helpers/test_roles.py @@ -30,6 +30,7 @@ add_course_access_roles, add_missing_signup_source_record, add_org_course_creator, + are_all_library_ids, cache_name_user_course_access_roles, cache_refresh_course_access_roles, check_tenant_access, @@ -65,6 +66,20 @@ def reset_fx_views_with_roles(): FXViewRoleInfoMetaClass._fx_views_with_roles = {'_all_view_names': {}} # pylint: disable=protected-access +@pytest.mark.parametrize('course_ids, expected_return, usecase', [ + (None, False, 'course_ids list is None'), + ([], False, 'course_ids list is empty'), + (['course-v1:ORG1+1+1', 'course-v1:ORG1+1+1'], False, 'course_ids contain courses'), + (['library-v1:ORG1+1', 'course-v1:ORG1+1+1'], False, 'course_ids contain libraries and courses'), + (['library-v1:ORG1+11', 'library-v1:ORG1+22'], True, 'course_ids contain valid library'), + (['library-v1:ORG1+11', 'invalid-library-id'], False, 'course_ids contain invalid library'), +]) +def test_are_all_library_ids(course_ids, expected_return, usecase): + """test are_all_librarary_ids""" + return_value = are_all_library_ids(course_ids) + assert return_value is expected_return, f'Failed usecase: {usecase}' + + @pytest.mark.parametrize('update_course_access_role, error_msg, error_code', [ ({'role': 'bad_role'}, 'invalid role ({role})!', FXExceptionCodes.ROLE_INVALID_ENTRY), ( @@ -117,6 +132,11 @@ def reset_fx_views_with_roles(): 'missing course-creator record for {role} role!', FXExceptionCodes.ROLE_INVALID_ENTRY, ), + ( + {'role': cs.COURSE_ACCESS_ROLES_LIBRARY_USER, 'course_id': 'yes'}, + 'role {role} is only valid with library_id', + FXExceptionCodes.ROLE_INVALID_ENTRY + ), ]) @pytest.mark.django_db def test_validate_course_access_role_invalid( @@ -132,7 +152,7 @@ def test_validate_course_access_role_invalid( } bad_course_access_role.update(update_course_access_role) if bad_course_access_role['course_id']: - bad_course_access_role['course_id'] = CourseKey.from_string('course-v1:ORG1+1+1') + bad_course_access_role['course_id'] = 'course-v1:ORG1+1+1' if error_msg and '{role}' in error_msg: error_msg = error_msg.format(role=update_course_access_role['role']) @@ -1316,6 +1336,15 @@ def test_add_course_access_roles_tenant_wide_role(roles_authorize_caller): # py assert str(exc_info.value) == f'Role ({role}) can only be tenant-wide!' +@pytest.mark.django_db +def test_add_course_access_roles_for_library_user(roles_authorize_caller): # pylint: disable=unused-argument + """Verify that add_course_access_roles raises an error adding library_user role with course_id""" + role = cs.COURSE_ACCESS_ROLES_LIBRARY_USER + with pytest.raises(FXCodedException) as exc_info: + add_course_access_roles(None, [1, 8], ['user1'], role, False, ['course-v1:ORG3+1+1']) + assert str(exc_info.value) == f'Role: {role} is only valid for library ids.' + + @pytest.mark.django_db @pytest.mark.parametrize('user_keys, error_message', [ (None, 'No users provided!'),