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..c9d3dc82 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 course_ids 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..70c7d05e --- /dev/null +++ b/test_utils/edx_platform_mocks/xmodule/modulestore/django.py @@ -0,0 +1,17 @@ +"""Mock modulestore""" +from opaque_keys.edx.keys import CourseKey + + +class DummyModuleStore: # pylint: disable=too-few-public-methods + """DUmmy module store class""" + def __init__(self): + self.ids = ['library-v1:org1+11', 'library-v1:org1+22'] + + def get_library_keys(self): + """mock modulestore library keys method""" + return [CourseKey.from_string(id) for id in self.ids] + + +def modulestore(): + """mocked modulestore""" + return DummyModuleStore() diff --git a/tests/test_helpers/test_extractors.py b/tests/test_helpers/test_extractors.py index 6f5b0a87..bd5cb7d0 100644 --- a/tests/test_helpers/test_extractors.py +++ b/tests/test_helpers/test_extractors.py @@ -49,9 +49,13 @@ def test_get_course_id_from_uri(uri, expected_course_id): assert get_course_id_from_uri(uri) == expected_course_id -def test_verify_course_ids_success(): - """Verify that verify_course_ids does not raise an error for valid course IDs.""" - course_ids = ['course-v1:edX+DemoX+Demo_Course', 'course-v1:edX+DemoX+Demo_Course2'] +@pytest.mark.parametrize('course_ids', [ + (['course-v1:edX+DemoX+Demo_Course', 'course-v1:edX+DemoX+Demo_Course2']), + (['library-v1:DemoX+11', 'library-v1:DemoX+22']), + (['course-v1:edX+DemoX+Demo_Course', 'library-v1:DemoX+22']), +]) +def test_verify_course_ids_success(course_ids): + """Verify that verify_course_ids does not raise an error for valid course and library IDs.""" verify_course_ids(course_ids) @@ -59,6 +63,8 @@ def test_verify_course_ids_success(): (None, 'course_ids must be a list of course IDs, but got None'), (['course-v1:edX+DemoX+Demo_Course', 3], 'course_id must be a string, but got int'), (['course-v1:edX+DemoX+Demo_Course+extra'], 'Invalid course ID format: course-v1:edX+DemoX+Demo_Course+extra'), + (['library-v1:edX+DemoX+extra'], 'Invalid course ID format: library-v1:edX+DemoX+extra'), + (['library-v1:invalid'], 'Invalid course ID format: library-v1:invalid'), ]) def test_verify_course_ids_fail(course_ids, error_message): """Verify that verify_course_ids raises an error for invalid course IDs.""" @@ -83,11 +89,48 @@ def test_get_orgs_of_courses(base_data): # pylint: disable=unused-argument @pytest.mark.django_db -def test_get_orgs_of_courses_invalid_course(base_data): # pylint: disable=unused-argument +@pytest.mark.parametrize('course_ids, expected_orgs', [ + (['course-v1:Org1+1+1', 'course-v1:ORG1+2+2'], { + 'course-v1:Org1+1+1': 'org1', + 'course-v1:ORG1+2+2': 'org1', + }), + (['library-v1:org1+11', 'course-v1:ORG1+5+5'], { + 'library-v1:org1+11': 'org1', + 'course-v1:ORG1+5+5': 'org1', + }), + (['library-v1:org1+11', 'library-v1:org1+22'], { + 'library-v1:org1+11': 'org1', + 'library-v1:org1+22': 'org1', + }), +]) +def test_get_orgs_of_courses_for_library_ids(course_ids, expected_orgs, base_data): # pylint: disable=unused-argument + """Verify that get_orgs_of_courses returns the expected organization for library ids.""" + result = get_orgs_of_courses(course_ids) + assert result == {'courses': expected_orgs} + + +@pytest.mark.django_db +@pytest.mark.parametrize('course_ids, expected_error_message', [ + ( + ['course-v1:ORG1+2+99'], + 'Invalid course IDs provided: [\'course-v1:ORG1+2+99\']' + ), + ( + ['library-v1:not_exist+11'], + 'Invalid course IDs provided: [\'library-v1:not_exist+11\']' + ), + ( + ['course-v1:ORG1+2+99', 'library-v1:not_exist+1'], + 'Invalid course IDs provided: [\'course-v1:ORG1+2+99\', \'library-v1:not_exist+1\']' + ) +]) +def test_get_orgs_of_courses_invalid_course( + course_ids, expected_error_message, base_data +): # pylint: disable=unused-argument """Verify that get_orgs_of_courses raises an error for an invalid course ID.""" with pytest.raises(ValueError) as exc_info: - get_orgs_of_courses(['course-v1:ORG1+2+99']) - assert str(exc_info.value) == 'Invalid course IDs provided: [\'course-v1:ORG1+2+99\']' + get_orgs_of_courses(course_ids) + assert str(exc_info.value) == expected_error_message def test_dict_hashcode_init(): diff --git a/tests/test_helpers/test_roles.py b/tests/test_helpers/test_roles.py index 188314ef..f5f6e3dd 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!'), @@ -1411,6 +1440,30 @@ def test_add_course_access_roles_success_list_contains_user_key( } +@pytest.mark.django_db +@pytest.mark.parametrize('role, library_ids, tenant_wide', [ + ('library_user', ['library-v1:org1+11'], False), + ('library_user', [], True), + ('library_user', None, True), + ('staff', ['library-v1:org1+11'], False), + ('instructor', ['library-v1:org1+11'], False) +]) +@patch('futurex_openedx_extensions.helpers.users.get_user_by_username_or_email') +def test_add_course_access_roles_success_for_library_roles( + mock_get_user, role, library_ids, tenant_wide, base_data +): # pylint: disable=unused-argument + """Verify that add_course_access_roles for library roles""" + caller = get_user_model().objects.get(username='user1') + mock_get_user.return_value = get_user_model().objects.get(id=3) + result = add_course_access_roles(caller, [1], ['user3'], role, tenant_wide, library_ids) + assert result == { + 'failed': [], + 'added': [], + 'updated': ['user3'], + 'not_updated': [], + } + + @pytest.mark.django_db def test_add_course_access_roles_success_user_key_same_as_id( roles_authorize_caller, base_data