diff --git a/.dockerignore b/.dockerignore index 1665783ba48e..c3873d33a595 100644 --- a/.dockerignore +++ b/.dockerignore @@ -102,7 +102,6 @@ common/test/data/badges/*.png ### Static assets pipeline artifacts **/*.scssc lms/static/css/ -!lms/static/css/vendor lms/static/certificates/css/ cms/static/css/ common/static/common/js/vendor/ diff --git a/.gitignore b/.gitignore index 0d020e45fc93..aaeb22eef13d 100644 --- a/.gitignore +++ b/.gitignore @@ -102,11 +102,11 @@ bin/ ### Static assets pipeline artifacts *.scssc -lms/static/css/ -lms/static/certificates/css/ -cms/static/css/ -common/static/common/js/vendor/ -common/static/common/css/vendor/ +lms/static/css +lms/static/certificates/css +cms/static/css +common/static/common/js/vendor +common/static/common/css/vendor common/static/bundles webpack-stats.json @@ -115,7 +115,6 @@ lms/static/sass/*.css lms/static/sass/*.css.map lms/static/certificates/sass/*.css lms/static/themed_sass/ -cms/static/css/ cms/static/sass/*.css cms/static/sass/*.css.map cms/static/themed_sass/ diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/home.py b/cms/djangoapps/contentstore/rest_api/v1/views/home.py index b06cec44b7d3..d41ceb2647c5 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/home.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/home.py @@ -215,4 +215,5 @@ def get(self, request: Request): library_context = get_library_context(request) serializer = LibraryTabSerializer(library_context) + return Response(serializer.data) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 9bcd77c3eea9..21f605a9e440 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -1670,7 +1670,7 @@ def get_home_context(request, no_course=False): LIBRARY_AUTHORING_MICROFRONTEND_URL, LIBRARIES_ENABLED, should_redirect_to_library_authoring_mfe, - user_can_create_library, + user_can_view_create_library_button, ) active_courses = [] @@ -1699,7 +1699,8 @@ def get_home_context(request, no_course=False): 'library_authoring_mfe_url': LIBRARY_AUTHORING_MICROFRONTEND_URL, 'taxonomy_list_mfe_url': get_taxonomy_list_url(), 'libraries': libraries, - 'show_new_library_button': user_can_create_library(user) and not should_redirect_to_library_authoring_mfe(), + 'show_new_library_button': user_can_view_create_library_button(user) + and not should_redirect_to_library_authoring_mfe(), 'user': user, 'request_course_creator_url': reverse('request_course_creator'), 'course_creator_status': _get_course_creator_status(user), diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index b56c989b411e..dce82809dfad 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -52,7 +52,7 @@ CourseStaffRole, GlobalStaff, UserBasedRole, - OrgStaffRole + OrgStaffRole, ) from common.djangoapps.util.json_request import JsonResponse, JsonResponseBadRequest, expect_json from common.djangoapps.util.string_utils import _has_non_ascii_characters diff --git a/cms/djangoapps/contentstore/views/library.py b/cms/djangoapps/contentstore/views/library.py index 17aa24c5712a..870c192653d2 100644 --- a/cms/djangoapps/contentstore/views/library.py +++ b/cms/djangoapps/contentstore/views/library.py @@ -10,7 +10,7 @@ from django.conf import settings from django.contrib.auth.decorators import login_required from django.core.exceptions import PermissionDenied -from django.http import Http404, HttpResponseForbidden, HttpResponseNotAllowed +from django.http import Http404, HttpResponseNotAllowed from django.utils.translation import gettext as _ from django.views.decorators.csrf import ensure_csrf_cookie from django.views.decorators.http import require_http_methods @@ -69,12 +69,10 @@ def should_redirect_to_library_authoring_mfe(): ) -def user_can_create_library(user, org=None): +def user_can_view_create_library_button(user): """ - Helper method for returning the library creation status for a particular user, - taking into account the value LIBRARIES_ENABLED. + Helper method for displaying the visibilty of the create_library_button. """ - if not LIBRARIES_ENABLED: return False elif user.is_staff: @@ -84,8 +82,56 @@ def user_can_create_library(user, org=None): has_org_staff_role = OrgStaffRole().get_orgs_for_user(user).exists() has_course_staff_role = UserBasedRole(user=user, role=CourseStaffRole.ROLE).courses_with_role().exists() has_course_admin_role = UserBasedRole(user=user, role=CourseInstructorRole.ROLE).courses_with_role().exists() + return is_course_creator or has_org_staff_role or has_course_staff_role or has_course_admin_role + else: + # EDUCATOR-1924: DISABLE_LIBRARY_CREATION overrides DISABLE_COURSE_CREATION, if present. + disable_library_creation = settings.FEATURES.get('DISABLE_LIBRARY_CREATION', None) + disable_course_creation = settings.FEATURES.get('DISABLE_COURSE_CREATION', False) + if disable_library_creation is not None: + return not disable_library_creation + else: + return not disable_course_creation + +def user_can_create_library(user, org): + """ + Helper method for returning the library creation status for a particular user, + taking into account the value LIBRARIES_ENABLED. + + if the ENABLE_CREATOR_GROUP value is False, then any user can create a library (in any org), + if library creation is enabled. + + if the ENABLE_CREATOR_GROUP value is true, then what a user can do varies by thier role. + + Global Staff: can make libraries in any org. + Course Creator Group Members: can make libraries in any org. + Organization Staff: Can make libraries in the organization for which they are staff. + Course Staff: Can make libraries in the organization which has courses of which they are staff. + Course Admin: Can make libraries in the organization which has courses of which they are Admin. + """ + if org is None: + return False + if not LIBRARIES_ENABLED: + return False + elif user.is_staff: + return True + if settings.FEATURES.get('ENABLE_CREATOR_GROUP', False): + is_course_creator = get_course_creator_status(user) == 'granted' + has_org_staff_role = org in OrgStaffRole().get_orgs_for_user(user) + has_course_staff_role = ( + UserBasedRole(user=user, role=CourseStaffRole.ROLE) + .courses_with_role() + .filter(org=org) + .exists() + ) + has_course_admin_role = ( + UserBasedRole(user=user, role=CourseInstructorRole.ROLE) + .courses_with_role() + .filter(org=org) + .exists() + ) return is_course_creator or has_org_staff_role or has_course_staff_role or has_course_admin_role + else: # EDUCATOR-1924: DISABLE_LIBRARY_CREATION overrides DISABLE_COURSE_CREATION, if present. disable_library_creation = settings.FEATURES.get('DISABLE_LIBRARY_CREATION', None) @@ -108,12 +154,8 @@ def library_handler(request, library_key_string=None): raise Http404 # Should never happen because we test the feature in urls.py also if request.method == 'POST': - if not user_can_create_library(request.user): - return HttpResponseForbidden() - if library_key_string is not None: return HttpResponseNotAllowed(("POST",)) - return _create_library(request) else: diff --git a/cms/djangoapps/contentstore/views/tests/test_library.py b/cms/djangoapps/contentstore/views/tests/test_library.py index f6b7a48a68e1..fa6505419725 100644 --- a/cms/djangoapps/contentstore/views/tests/test_library.py +++ b/cms/djangoapps/contentstore/views/tests/test_library.py @@ -59,55 +59,66 @@ def setUp(self): @mock.patch("cms.djangoapps.contentstore.views.library.LIBRARIES_ENABLED", False) def test_library_creator_status_libraries_not_enabled(self): _, nostaff_user = self.create_non_staff_authed_user_client() - self.assertEqual(user_can_create_library(nostaff_user), False) + self.assertEqual(user_can_create_library(nostaff_user, None), False) # When creator group is disabled, non-staff users can create libraries @mock.patch("cms.djangoapps.contentstore.views.library.LIBRARIES_ENABLED", True) def test_library_creator_status_with_no_course_creator_role(self): _, nostaff_user = self.create_non_staff_authed_user_client() - self.assertEqual(user_can_create_library(nostaff_user), True) + self.assertEqual(user_can_create_library(nostaff_user, 'An Org'), True) # When creator group is enabled, Non staff users cannot create libraries @mock.patch("cms.djangoapps.contentstore.views.library.LIBRARIES_ENABLED", True) def test_library_creator_status_for_enabled_creator_group_setting_for_non_staff_users(self): _, nostaff_user = self.create_non_staff_authed_user_client() with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): - self.assertEqual(user_can_create_library(nostaff_user), False) + self.assertEqual(user_can_create_library(nostaff_user, None), False) - # Global staff can create libraries + # Global staff can create libraries for any org, even ones that don't exist. @mock.patch("cms.djangoapps.contentstore.views.library.LIBRARIES_ENABLED", True) def test_library_creator_status_with_is_staff_user(self): - self.assertEqual(user_can_create_library(self.user), True) + print(self.user.is_staff) + self.assertEqual(user_can_create_library(self.user, 'aNyOrg'), True) - # When creator groups are enabled, global staff can create libraries + # Global staff can create libraries for any org, but an org has to be supplied. + @mock.patch("cms.djangoapps.contentstore.views.library.LIBRARIES_ENABLED", True) + def test_library_creator_status_with_is_staff_user_no_org(self): + print(self.user.is_staff) + self.assertEqual(user_can_create_library(self.user, None), False) + + # When creator groups are enabled, global staff can create libraries in any org @mock.patch("cms.djangoapps.contentstore.views.library.LIBRARIES_ENABLED", True) def test_library_creator_status_for_enabled_creator_group_setting_with_is_staff_user(self): with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): - self.assertEqual(user_can_create_library(self.user), True) + self.assertEqual(user_can_create_library(self.user, 'RandomOrg'), True) - # When creator groups are enabled, course creators can create libraries + # When creator groups are enabled, course creators can create libraries in any org. @mock.patch("cms.djangoapps.contentstore.views.library.LIBRARIES_ENABLED", True) def test_library_creator_status_with_course_creator_role_for_enabled_creator_group_setting(self): _, nostaff_user = self.create_non_staff_authed_user_client() with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): grant_course_creator_status(self.user, nostaff_user) - self.assertEqual(user_can_create_library(nostaff_user), True) + self.assertEqual(user_can_create_library(nostaff_user, 'soMeRandOmoRg'), True) # When creator groups are enabled, course staff members can create libraries + # but only in the org they are course staff for. @mock.patch("cms.djangoapps.contentstore.views.library.LIBRARIES_ENABLED", True) def test_library_creator_status_with_course_staff_role_for_enabled_creator_group_setting(self): _, nostaff_user = self.create_non_staff_authed_user_client() with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): auth.add_users(self.user, CourseStaffRole(self.course.id), nostaff_user) - self.assertEqual(user_can_create_library(nostaff_user), True) + self.assertEqual(user_can_create_library(nostaff_user, self.course.org), True) + self.assertEqual(user_can_create_library(nostaff_user, 'SomEOtherOrg'), False) # When creator groups are enabled, course instructor members can create libraries + # but only in the org they are course staff for. @mock.patch("cms.djangoapps.contentstore.views.library.LIBRARIES_ENABLED", True) def test_library_creator_status_with_course_instructor_role_for_enabled_creator_group_setting(self): _, nostaff_user = self.create_non_staff_authed_user_client() with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): auth.add_users(self.user, CourseInstructorRole(self.course.id), nostaff_user) - self.assertEqual(user_can_create_library(nostaff_user), True) + self.assertEqual(user_can_create_library(nostaff_user, self.course.org), True) + self.assertEqual(user_can_create_library(nostaff_user, 'SomEOtherOrg'), False) @ddt.data( (False, False, True), @@ -131,7 +142,7 @@ def test_library_creator_status_settings(self, disable_course, disable_library, "DISABLE_LIBRARY_CREATION": disable_library } ): - self.assertEqual(user_can_create_library(nostaff_user), expected_status) + self.assertEqual(user_can_create_library(nostaff_user, 'SomEOrg'), expected_status) @mock.patch.dict('django.conf.settings.FEATURES', {'DISABLE_COURSE_CREATION': True}) @mock.patch("cms.djangoapps.contentstore.views.library.LIBRARIES_ENABLED", True) @@ -140,7 +151,7 @@ def test_library_creator_status_with_no_course_creator_role_and_disabled_nonstaf Ensure that `DISABLE_COURSE_CREATION` feature works with libraries as well. """ nostaff_client, nostaff_user = self.create_non_staff_authed_user_client() - self.assertFalse(user_can_create_library(nostaff_user)) + self.assertFalse(user_can_create_library(nostaff_user, 'SomEOrg')) # To be explicit, this user can GET, but not POST get_response = nostaff_client.get_json(LIBRARY_REST_URL) @@ -251,7 +262,7 @@ def test_lib_create_permission_course_staff_role(self): auth.add_users(self.user, CourseStaffRole(self.course.id), ns_user) self.assertTrue(auth.user_has_role(ns_user, CourseStaffRole(self.course.id))) response = self.client.ajax_post(LIBRARY_REST_URL, { - 'org': 'org', 'library': 'lib', 'display_name': "New Library", + 'org': self.course.org, 'library': 'lib', 'display_name': "New Library", }) self.assertEqual(response.status_code, 200) diff --git a/common/djangoapps/student/auth.py b/common/djangoapps/student/auth.py index fb2999eca98d..d3dc51616c75 100644 --- a/common/djangoapps/student/auth.py +++ b/common/djangoapps/student/auth.py @@ -23,7 +23,7 @@ OrgContentCreatorRole, OrgInstructorRole, OrgLibraryUserRole, - OrgStaffRole + OrgStaffRole, ) # Studio permissions: @@ -95,17 +95,23 @@ def get_user_permissions(user, course_key, org=None): return all_perms # HACK: Limited Staff should not have studio read access. However, since many LMS views depend on the # `has_course_author_access` check and `course_author_access_required` decorator, we have to allow write access - # until the permissions become more granular. For example, there could be STUDIO_VIEW_COHORTS and - # STUDIO_EDIT_COHORTS specifically for the cohorts endpoint, which is used to display the "Cohorts" tab of the - # Instructor Dashboard. + # by returning STUDIO_EDIT_CONTENT, if the request is made from LMS, until the permissions become more granular. + # For example, there could be STUDIO_VIEW_COHORTS and STUDIO_EDIT_COHORTS specifically for the cohorts endpoint, + # which is used to display the "Cohorts" tab of the Instructor Dashboard. If the request is made from the CMS, + # then STUDIO_NO_PERMISSIONS is returned instead. # The permissions matrix from the RBAC project (https://github.com/openedx/platform-roadmap/issues/246) shows that # the LMS and Studio permissions will be separated as a part of this project. Once this is done (and this code is # not removed during its implementation), we can replace the Limited Staff permissions with more granular ones. if course_key and user_has_role(user, CourseLimitedStaffRole(course_key)): - return STUDIO_EDIT_CONTENT + if settings.SERVICE_VARIANT == 'lms': + return STUDIO_EDIT_CONTENT + else: + return STUDIO_NO_PERMISSIONS + # Staff have all permissions except EDIT_ROLES: if OrgStaffRole(org=org).has_user(user) or (course_key and user_has_role(user, CourseStaffRole(course_key))): return STUDIO_VIEW_USERS | STUDIO_EDIT_CONTENT | STUDIO_VIEW_CONTENT + # Otherwise, for libraries, users can view only: if course_key and isinstance(course_key, LibraryLocator): if OrgLibraryUserRole(org=org).has_user(user) or user_has_role(user, LibraryUserRole(course_key)): diff --git a/common/djangoapps/student/role_helpers.py b/common/djangoapps/student/role_helpers.py index 8a12bfa0ac90..64ed5cc17efb 100644 --- a/common/djangoapps/student/role_helpers.py +++ b/common/djangoapps/student/role_helpers.py @@ -75,4 +75,4 @@ def get_course_roles(user: User) -> list[CourseAccessRole]: """ # pylint: disable=protected-access role_cache = get_role_cache(user) - return list(role_cache._roles) + return list(role_cache.all_roles_set) diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index 7bbd0cf92454..971433c9c523 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -4,9 +4,9 @@ """ +from collections import defaultdict import logging from abc import ABCMeta, abstractmethod -from collections import defaultdict from contextlib import contextmanager from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user @@ -23,6 +23,9 @@ # A mapping of roles to the roles that they inherit permissions from. ACCESS_ROLES_INHERITANCE = {} +# The key used to store roles for a user in the cache that do not belong to a course or do not have a course id. +ROLE_CACHE_UNGROUPED_ROLES__KEY = 'ungrouped' + def register_access_role(cls): """ @@ -60,21 +63,52 @@ def strict_role_checking(): ACCESS_ROLES_INHERITANCE.update(OLD_ACCESS_ROLES_INHERITANCE) +def get_role_cache_key_for_course(course_key=None): + """ + Get the cache key for the course key. + """ + return str(course_key) if course_key else ROLE_CACHE_UNGROUPED_ROLES__KEY + + class BulkRoleCache: # lint-amnesty, pylint: disable=missing-class-docstring + """ + This class provides a caching mechanism for roles grouped by users and courses, + using a nested dictionary structure to optimize lookup performance. The cache structure is designed as follows: + + { + user_id_1: { + course_id_1: {role1, role2, role3}, # Set of roles associated with course_id_1 + course_id_2: {role4, role5, role6}, # Set of roles associated with course_id_2 + [ROLE_CACHE_UNGROUPED_ROLES_KEY]: {role7, role8} # Set of roles not tied to any specific course or library + }, + user_id_2: { ... } # Similar structure for another user + } + + - Each top-level dictionary entry keys by `user_id` to access role data for a specific user. + - Nested within each user's dictionary, entries are keyed by `course_id` grouping roles by course. + - The special key `ROLE_CACHE_UNGROUPED_ROLES_KEY` (a constant defined above) + stores roles that are not associated with any specific course or library. + """ + CACHE_NAMESPACE = "student.roles.BulkRoleCache" CACHE_KEY = 'roles_by_user' @classmethod def prefetch(cls, users): # lint-amnesty, pylint: disable=missing-function-docstring - roles_by_user = defaultdict(set) + roles_by_user = defaultdict(lambda: defaultdict(set)) get_cache(cls.CACHE_NAMESPACE)[cls.CACHE_KEY] = roles_by_user for role in CourseAccessRole.objects.filter(user__in=users).select_related('user'): - roles_by_user[role.user.id].add(role) + user_id = role.user.id + course_id = get_role_cache_key_for_course(role.course_id) + + # Add role to the set in roles_by_user[user_id][course_id] + user_roles_set_for_course = roles_by_user[user_id][course_id] + user_roles_set_for_course.add(role) users_without_roles = [u for u in users if u.id not in roles_by_user] for user in users_without_roles: - roles_by_user[user.id] = set() + roles_by_user[user.id] = {} @classmethod def get_user_roles(cls, user): @@ -83,15 +117,32 @@ def get_user_roles(cls, user): class RoleCache: """ - A cache of the CourseAccessRoles held by a particular user + A cache of the CourseAccessRoles held by a particular user. + Internal data structures should be accessed by getter and setter methods; + don't use `_roles_by_course_id` or `_roles` directly. + _roles_by_course_id: This is the data structure as saved in the RequestCache. + It contains all roles for a user as a dict that's keyed by course_id. + The key ROLE_CACHE_UNGROUPED_ROLES__KEY is used for all roles + that are not associated with a course. + _roles: This is a set of all roles for a user, ungrouped. It's used for some types of + lookups and collected from _roles_by_course_id on initialization + so that it doesn't need to be recalculated. + """ def __init__(self, user): try: - self._roles = BulkRoleCache.get_user_roles(user) + self._roles_by_course_id = BulkRoleCache.get_user_roles(user) except KeyError: - self._roles = set( - CourseAccessRole.objects.filter(user=user).all() - ) + self._roles_by_course_id = {} + roles = CourseAccessRole.objects.filter(user=user).all() + for role in roles: + course_id = get_role_cache_key_for_course(role.course_id) + if not self._roles_by_course_id.get(course_id): + self._roles_by_course_id[course_id] = set() + self._roles_by_course_id[course_id].add(role) + self._roles = set() + for roles_for_course in self._roles_by_course_id.values(): + self._roles.update(roles_for_course) @staticmethod def get_roles(role): @@ -100,16 +151,24 @@ def get_roles(role): """ return ACCESS_ROLES_INHERITANCE.get(role, set()) | {role} + @property + def all_roles_set(self): + return self._roles + + @property + def roles_by_course_id(self): + return self._roles_by_course_id + def has_role(self, role, course_id, org): """ Return whether this RoleCache contains a role with the specified role or a role that inherits from the specified role, course_id and org. """ + course_id_string = get_role_cache_key_for_course(course_id) + course_roles = self._roles_by_course_id.get(course_id_string, []) return any( - access_role.role in self.get_roles(role) and - access_role.course_id == course_id and - access_role.org == org - for access_role in self._roles + access_role.role in self.get_roles(role) and access_role.org == org + for access_role in course_roles ) diff --git a/common/djangoapps/student/tests/test_authz.py b/common/djangoapps/student/tests/test_authz.py index 1c79780e88c1..c0b88e6318b5 100644 --- a/common/djangoapps/student/tests/test_authz.py +++ b/common/djangoapps/student/tests/test_authz.py @@ -8,7 +8,7 @@ from ccx_keys.locator import CCXLocator from django.contrib.auth.models import AnonymousUser from django.core.exceptions import PermissionDenied -from django.test import TestCase +from django.test import TestCase, override_settings from opaque_keys.edx.locator import CourseLocator from common.djangoapps.student.auth import ( @@ -285,15 +285,26 @@ def test_remove_user_from_course_group_permission_denied(self): with pytest.raises(PermissionDenied): remove_users(self.staff, CourseStaffRole(self.course_key), another_staff) - def test_limited_staff_no_studio_read_access(self): + @override_settings(SERVICE_VARIANT='lms') + def test_limited_staff_no_studio_read_access_lms(self): """ - Verifies that course limited staff have no read, but have write access. + Verifies that course limited staff have no read, but have write access when SERVICE_VARIANT is 'lms'. """ add_users(self.global_admin, CourseLimitedStaffRole(self.course_key), self.limited_staff) assert not has_studio_read_access(self.limited_staff, self.course_key) assert has_studio_write_access(self.limited_staff, self.course_key) + @override_settings(SERVICE_VARIANT='cms') + def test_limited_staff_no_studio_access_cms(self): + """ + Verifies that course limited staff have no read and no write access when SERVICE_VARIANT is not 'lms'. + """ + add_users(self.global_admin, CourseLimitedStaffRole(self.course_key), self.limited_staff) + + assert not has_studio_read_access(self.limited_staff, self.course_key) + assert not has_studio_write_access(self.limited_staff, self.course_key) + class CourseOrgGroupTest(TestCase): """ diff --git a/common/djangoapps/student/tests/test_roles.py b/common/djangoapps/student/tests/test_roles.py index 9037eb902f61..da1aad19a803 100644 --- a/common/djangoapps/student/tests/test_roles.py +++ b/common/djangoapps/student/tests/test_roles.py @@ -6,6 +6,7 @@ import ddt from django.test import TestCase from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.locator import LibraryLocator from common.djangoapps.student.roles import ( CourseAccessRole, @@ -22,7 +23,9 @@ OrgContentCreatorRole, OrgInstructorRole, OrgStaffRole, - RoleCache + RoleCache, + get_role_cache_key_for_course, + ROLE_CACHE_UNGROUPED_ROLES__KEY ) from common.djangoapps.student.role_helpers import get_course_roles, has_staff_roles from common.djangoapps.student.tests.factories import AnonymousUserFactory, InstructorFactory, StaffFactory, UserFactory @@ -35,7 +38,7 @@ class RolesTestCase(TestCase): def setUp(self): super().setUp() - self.course_key = CourseKey.from_string('edX/toy/2012_Fall') + self.course_key = CourseKey.from_string('course-v1:course-v1:edX+toy+2012_Fall') self.course_loc = self.course_key.make_usage_key('course', '2012_Fall') self.anonymous_user = AnonymousUserFactory() self.student = UserFactory() @@ -189,8 +192,9 @@ def test_get_orgs_for_user(self): @ddt.ddt class RoleCacheTestCase(TestCase): # lint-amnesty, pylint: disable=missing-class-docstring - IN_KEY = CourseKey.from_string('edX/toy/2012_Fall') - NOT_IN_KEY = CourseKey.from_string('edX/toy/2013_Fall') + IN_KEY_STRING = 'course-v1:edX+toy+2012_Fall' + IN_KEY = CourseKey.from_string(IN_KEY_STRING) + NOT_IN_KEY = CourseKey.from_string('course-v1:edX+toy+2013_Fall') ROLES = ( (CourseStaffRole(IN_KEY), ('staff', IN_KEY, 'edX')), @@ -233,3 +237,75 @@ def test_only_in_role(self, role, target): def test_empty_cache(self, role, target): # lint-amnesty, pylint: disable=unused-argument cache = RoleCache(self.user) assert not cache.has_role(*target) + + def test_get_role_cache_key_for_course_for_course_object_gets_string(self): + """ + Given a valid course key object, get_role_cache_key_for_course + should return the string representation of the key. + """ + course_string = 'course-v1:edX+toy+2012_Fall' + key = CourseKey.from_string(course_string) + key = get_role_cache_key_for_course(key) + assert key == course_string + + def test_get_role_cache_key_for_course_for_undefined_object_returns_default(self): + """ + Given a value None, get_role_cache_key_for_course + should return the default key for ungrouped courses. + """ + key = get_role_cache_key_for_course(None) + assert key == ROLE_CACHE_UNGROUPED_ROLES__KEY + + def test_role_cache_get_roles_set(self): + """ + Test that the RoleCache.all_roles_set getter method returns a flat set of all roles for a user + and that the ._roles attribute is the same as the set to avoid legacy behavior being broken. + """ + lib0 = LibraryLocator.from_string('library-v1:edX+quizzes') + course0 = CourseKey.from_string('course-v1:edX+toy+2012_Summer') + course1 = CourseKey.from_string('course-v1:edX+toy2+2013_Fall') + role_library_v1 = LibraryUserRole(lib0) + role_course_0 = CourseInstructorRole(course0) + role_course_1 = CourseInstructorRole(course1) + + role_library_v1.add_users(self.user) + role_course_0.add_users(self.user) + role_course_1.add_users(self.user) + + cache = RoleCache(self.user) + assert cache.has_role('library_user', lib0, 'edX') + assert cache.has_role('instructor', course0, 'edX') + assert cache.has_role('instructor', course1, 'edX') + + assert len(cache.all_roles_set) == 3 + roles_set = cache.all_roles_set + for role in roles_set: + assert role.course_id.course in ('quizzes', 'toy2', 'toy') + + assert roles_set == cache._roles # pylint: disable=protected-access + + def test_role_cache_roles_by_course_id(self): + """ + Test that the RoleCache.roles_by_course_id getter method returns a dictionary of roles for a user + that are grouped by course_id or if ungrouped by the ROLE_CACHE_UNGROUPED_ROLES__KEY. + """ + lib0 = LibraryLocator.from_string('library-v1:edX+quizzes') + course0 = CourseKey.from_string('course-v1:edX+toy+2012_Summer') + course1 = CourseKey.from_string('course-v1:edX+toy2+2013_Fall') + role_library_v1 = LibraryUserRole(lib0) + role_course_0 = CourseInstructorRole(course0) + role_course_1 = CourseInstructorRole(course1) + role_org_staff = OrgStaffRole('edX') + + role_library_v1.add_users(self.user) + role_course_0.add_users(self.user) + role_course_1.add_users(self.user) + role_org_staff.add_users(self.user) + + cache = RoleCache(self.user) + roles_dict = cache.roles_by_course_id + assert len(roles_dict) == 4 + assert roles_dict.get(ROLE_CACHE_UNGROUPED_ROLES__KEY).pop().role == 'staff' + assert roles_dict.get('library-v1:edX+quizzes').pop().course_id.course == 'quizzes' + assert roles_dict.get('course-v1:edX+toy+2012_Summer').pop().course_id.course == 'toy' + assert roles_dict.get('course-v1:edX+toy2+2013_Fall').pop().course_id.course == 'toy2' diff --git a/common/djangoapps/util/file.py b/common/djangoapps/util/file.py index 66fc2a6f5f35..b2892e6f42c9 100644 --- a/common/djangoapps/util/file.py +++ b/common/djangoapps/util/file.py @@ -13,6 +13,7 @@ from django.utils.translation import gettext as _ from django.utils.translation import ngettext from pytz import UTC +from storages.backends.s3boto3 import S3Boto3Storage class FileValidationException(Exception): @@ -23,7 +24,7 @@ class FileValidationException(Exception): def store_uploaded_file( - request, file_key, allowed_file_types, base_storage_filename, max_file_size, validator=None, + request, file_key, allowed_file_types, base_storage_filename, max_file_size, validator=None, is_private=False, ): """ Stores an uploaded file to django file storage. @@ -45,6 +46,8 @@ def store_uploaded_file( a `FileValidationException` if the file is not properly formatted. If any exception is thrown, the stored file will be deleted before the exception is re-raised. Note that the implementor of the validator function should take care to close the stored file if they open it for reading. + is_private (Boolean): an optional boolean which if True and the storage backend is S3, + sets the ACL for the file object to be private. Returns: Storage: the file storage object where the file can be retrieved from @@ -75,6 +78,12 @@ def store_uploaded_file( file_storage = DefaultStorage() # If a file already exists with the supplied name, file_storage will make the filename unique. stored_file_name = file_storage.save(stored_file_name, uploaded_file) + if is_private and settings.DEFAULT_FILE_STORAGE == 'storages.backends.s3boto3.S3Boto3Storage': + S3Boto3Storage().connection.meta.client.put_object_acl( + ACL='private', + Bucket=settings.AWS_STORAGE_BUCKET_NAME, + Key=stored_file_name, + ) if validator: try: diff --git a/lms/static/css/vendor/images/treeview-default-line.gif b/common/static/css/vendor/images/treeview-default-line.gif similarity index 100% rename from lms/static/css/vendor/images/treeview-default-line.gif rename to common/static/css/vendor/images/treeview-default-line.gif diff --git a/lms/static/css/vendor/images/treeview-default.gif b/common/static/css/vendor/images/treeview-default.gif similarity index 100% rename from lms/static/css/vendor/images/treeview-default.gif rename to common/static/css/vendor/images/treeview-default.gif diff --git a/lms/static/css/vendor/indicator.gif b/common/static/css/vendor/indicator.gif similarity index 100% rename from lms/static/css/vendor/indicator.gif rename to common/static/css/vendor/indicator.gif diff --git a/lms/static/css/vendor/jquery.autocomplete.css b/common/static/css/vendor/jquery.autocomplete.css similarity index 100% rename from lms/static/css/vendor/jquery.autocomplete.css rename to common/static/css/vendor/jquery.autocomplete.css diff --git a/lms/static/css/vendor/jquery.treeview.css b/common/static/css/vendor/jquery.treeview.css similarity index 100% rename from lms/static/css/vendor/jquery.treeview.css rename to common/static/css/vendor/jquery.treeview.css diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 6d7dfe17a9d7..02a91e7d84de 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -1603,7 +1603,8 @@ def post(self, request, course_key_string): request, 'uploaded-file', ['.csv'], course_and_time_based_filename_generator(course_key, 'cohorts'), max_file_size=2000000, # limit to 2 MB - validator=_cohorts_csv_validator + validator=_cohorts_csv_validator, + is_private=True ) task_api.submit_cohort_students(request, course_key, file_name) except (FileValidationException, ValueError) as e: diff --git a/lms/djangoapps/learner_home/serializers.py b/lms/djangoapps/learner_home/serializers.py index 0ce7bf9c6977..b3471715b9dc 100644 --- a/lms/djangoapps/learner_home/serializers.py +++ b/lms/djangoapps/learner_home/serializers.py @@ -15,6 +15,7 @@ from common.djangoapps.course_modes.models import CourseMode from openedx.features.course_experience import course_home_url from xmodule.data import CertificatesDisplayBehaviors +from lms.djangoapps.learner_home.utils import course_progress_url class LiteralField(serializers.Field): @@ -116,7 +117,7 @@ def get_homeUrl(self, instance): return course_home_url(instance.course_id) def get_progressUrl(self, instance): - return reverse("progress", kwargs={"course_id": instance.course_id}) + return course_progress_url(instance.course_id) def get_unenrollUrl(self, instance): return reverse("course_run_refund_status", args=[instance.course_id]) diff --git a/lms/djangoapps/learner_home/test_serializers.py b/lms/djangoapps/learner_home/test_serializers.py index f588af58aee4..ac11a8b2990d 100644 --- a/lms/djangoapps/learner_home/test_serializers.py +++ b/lms/djangoapps/learner_home/test_serializers.py @@ -51,7 +51,7 @@ SuggestedCourseSerializer, UnfulfilledEntitlementSerializer, ) - +from lms.djangoapps.learner_home.utils import course_progress_url from lms.djangoapps.learner_home.test_utils import ( datetime_to_django_format, random_bool, @@ -224,6 +224,30 @@ def test_missing_resume_url(self): # Then the resumeUrl is None, which is allowed self.assertIsNone(output_data["resumeUrl"]) + def is_progress_url_matching_course_home_mfe_progress_tab_is_active(self): + """ + Compares the progress URL generated by CourseRunSerializer to the expected progress URL. + + :return: True if the generated progress URL matches the expected, False otherwise. + """ + input_data = self.create_test_enrollment() + input_context = self.create_test_context(input_data.course.id) + output_data = CourseRunSerializer(input_data, context=input_context).data + return output_data['progressUrl'] == course_progress_url(input_data.course.id) + + @mock.patch('lms.djangoapps.learner_home.utils.course_home_mfe_progress_tab_is_active') + def test_progress_url(self, mock_course_home_mfe_progress_tab_is_active): + """ + Tests the progress URL generated by the CourseRunSerializer. When course_home_mfe_progress_tab_is_active + is true, the generated progress URL must point to the progress page of the course home (learning) MFE. + Otherwise, it must point to the legacy progress page. + """ + mock_course_home_mfe_progress_tab_is_active.return_value = True + self.assertTrue(self.is_progress_url_matching_course_home_mfe_progress_tab_is_active()) + + mock_course_home_mfe_progress_tab_is_active.return_value = False + self.assertTrue(self.is_progress_url_matching_course_home_mfe_progress_tab_is_active()) + @ddt.ddt class TestCoursewareAccessSerializer(LearnerDashboardBaseTest): diff --git a/lms/djangoapps/learner_home/utils.py b/lms/djangoapps/learner_home/utils.py index 28e4479f9439..96af6a64452b 100644 --- a/lms/djangoapps/learner_home/utils.py +++ b/lms/djangoapps/learner_home/utils.py @@ -4,6 +4,7 @@ import logging +from django.urls import reverse from django.contrib.auth import get_user_model from django.core.exceptions import MultipleObjectsReturned from rest_framework.exceptions import PermissionDenied, NotFound @@ -11,6 +12,8 @@ from common.djangoapps.student.models import ( get_user_by_username_or_email, ) +from lms.djangoapps.course_home_api.toggles import course_home_mfe_progress_tab_is_active +from openedx.features.course_experience.url_helpers import get_learning_mfe_home_url log = logging.getLogger(__name__) User = get_user_model() @@ -54,3 +57,16 @@ def get_masquerade_user(request): ) log.info(success_msg) return masquerade_user + + +def course_progress_url(course_key) -> str: + """ + Returns the course progress page's URL for the current user. + + :param course_key: The course key for which the home url is being requested. + + :return: The course progress page URL. + """ + if course_home_mfe_progress_tab_is_active(course_key): + return get_learning_mfe_home_url(course_key, url_fragment='progress') + return reverse('progress', kwargs={'course_id': course_key}) diff --git a/lms/djangoapps/support/migrations/0006_alter_historicalusersocialauth_extra_data_and_more.py b/lms/djangoapps/support/migrations/0006_alter_historicalusersocialauth_extra_data_and_more.py new file mode 100644 index 000000000000..5f09d0cc493b --- /dev/null +++ b/lms/djangoapps/support/migrations/0006_alter_historicalusersocialauth_extra_data_and_more.py @@ -0,0 +1,23 @@ +# Generated by Django 4.2.13 on 2024-06-27 20:46 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('support', '0005_unique_course_id'), + ] + + operations = [ + migrations.AlterField( + model_name='historicalusersocialauth', + name='extra_data', + field=models.JSONField(default=dict), + ), + migrations.AlterField( + model_name='historicalusersocialauth', + name='id', + field=models.BigIntegerField(auto_created=True, blank=True, db_index=True, verbose_name='ID'), + ), + ] diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index bbde4fc98230..17824dbc2a7e 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -13,6 +13,7 @@ from django.conf import settings from django.contrib.auth import get_user_model from django.core.cache import cache +from django.core.paginator import Paginator from meilisearch import Client as MeilisearchClient from meilisearch.errors import MeilisearchError from meilisearch.models.task import TaskInfo @@ -21,10 +22,9 @@ from common.djangoapps.student.roles import GlobalStaff from rest_framework.request import Request from common.djangoapps.student.role_helpers import get_course_roles +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.content.search.models import get_access_ids_for_request - from openedx.core.djangoapps.content_libraries import api as lib_api -from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore from .documents import ( @@ -292,9 +292,7 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: # Get the list of courses status_cb("Counting courses...") - with store.branch_setting(ModuleStoreEnum.Branch.draft_preferred): - all_courses = store.get_courses() - num_courses = len(all_courses) + num_courses = CourseOverview.objects.count() # Some counters so we can track our progress as indexing progresses: num_contexts = num_courses + num_libraries @@ -327,11 +325,20 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: ]) # Mark which attributes are used for keyword search, in order of importance: client.index(temp_index_name).update_searchable_attributes([ + # Keyword search does _not_ search the course name, course ID, breadcrumbs, block type, or other fields. Fields.display_name, Fields.block_id, Fields.content, Fields.tags, - # Keyword search does _not_ search the course name, course ID, breadcrumbs, block type, or other fields. + # If we don't list the following sub-fields _explicitly_, they're only sometimes searchable - that is, they + # are searchable only if at least one document in the index has a value. If we didn't list them here and, + # say, there were no tags.level3 tags in the index, the client would get an error if trying to search for + # these sub-fields: "Attribute `tags.level3` is not searchable." + Fields.tags + "." + Fields.tags_taxonomy, + Fields.tags + "." + Fields.tags_level0, + Fields.tags + "." + Fields.tags_level1, + Fields.tags + "." + Fields.tags_level2, + Fields.tags + "." + Fields.tags_level3, ]) ############## Libraries ############## @@ -358,30 +365,33 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: ############## Courses ############## status_cb("Indexing courses...") - for course in all_courses: - status_cb( - f"{num_contexts_done + 1}/{num_contexts}. Now indexing course {course.display_name} ({course.id})" - ) - docs = [] - - # Pre-fetch the course with all of its children: - course = store.get_course(course.id, depth=None) - - def add_with_children(block): - """ Recursively index the given XBlock/component """ - doc = searchable_doc_for_course_block(block) - doc.update(searchable_doc_tags(block.usage_key)) - docs.append(doc) # pylint: disable=cell-var-from-loop - _recurse_children(block, add_with_children) # pylint: disable=cell-var-from-loop - - # Index course children - _recurse_children(course, add_with_children) - - if docs: - # Add all the docs in this course at once (usually faster than adding one at a time): - _wait_for_meili_task(client.index(temp_index_name).add_documents(docs)) - num_contexts_done += 1 - num_blocks_done += len(docs) + # To reduce memory usage on large instances, split up the CourseOverviews into pages of 1,000 courses: + paginator = Paginator(CourseOverview.objects.only('id', 'display_name'), 1000) + for p in paginator.page_range: + for course in paginator.page(p).object_list: + status_cb( + f"{num_contexts_done + 1}/{num_contexts}. Now indexing course {course.display_name} ({course.id})" + ) + docs = [] + + # Pre-fetch the course with all of its children: + course = store.get_course(course.id, depth=None) + + def add_with_children(block): + """ Recursively index the given XBlock/component """ + doc = searchable_doc_for_course_block(block) + doc.update(searchable_doc_tags(block.usage_key)) + docs.append(doc) # pylint: disable=cell-var-from-loop + _recurse_children(block, add_with_children) # pylint: disable=cell-var-from-loop + + # Index course children + _recurse_children(course, add_with_children) + + if docs: + # Add all the docs in this course at once (usually faster than adding one at a time): + _wait_for_meili_task(client.index(temp_index_name).add_documents(docs)) + num_contexts_done += 1 + num_blocks_done += len(docs) status_cb(f"Done! {num_blocks_done} blocks indexed across {num_contexts_done} courses and libraries.") diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index cd1acc31b7d9..1c78b28506fe 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -15,6 +15,7 @@ from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.content_libraries import api as library_api from openedx.core.djangoapps.content_tagging import api as tagging_api +from openedx.core.djangoapps.content.course_overviews.api import CourseOverview from openedx.core.djangolib.testing.utils import skip_unless_cms from xmodule.modulestore.tests.django_utils import TEST_DATA_SPLIT_MODULESTORE, ModuleStoreTestCase @@ -106,6 +107,8 @@ def setUp(self): "content": {}, "access_id": course_access.id, } + # Make sure the CourseOverview for the course is created: + CourseOverview.get_from_id(self.course.id) # Create a content library: self.library = library_api.create_library( diff --git a/openedx/core/lib/teams_config.py b/openedx/core/lib/teams_config.py index c4952b6cdc3a..7210eed0f02f 100644 --- a/openedx/core/lib/teams_config.py +++ b/openedx/core/lib/teams_config.py @@ -19,7 +19,7 @@ TEAM_SCHEME = "team" TEAMS_NAMESPACE = "teams" -# .. toggle_name: course_teams.content_groups_for_teams +# .. toggle_name: teams.content_groups_for_teams # .. toggle_implementation: CourseWaffleFlag # .. toggle_default: False # .. toggle_description: This flag enables content groups for teams. Content groups are virtual groupings of learners diff --git a/openedx/features/survey_report/static/survey_report/js/admin_banner.js b/openedx/features/survey_report/static/survey_report/js/admin_banner.js index d8650321ba36..8dd9ba5565e0 100644 --- a/openedx/features/survey_report/static/survey_report/js/admin_banner.js +++ b/openedx/features/survey_report/static/survey_report/js/admin_banner.js @@ -1,11 +1,51 @@ $(document).ready(function(){ + // Function to get user ID + function getUserId() { + return $('#userIdSurvey').val(); + } + + // Function to get current time in milliseconds + function getCurrentTime() { + return new Date().getTime(); + } + + // Function to set dismissal time and expiration time in local storage + function setDismissalAndExpirationTime(userId, dismissalTime) { + let expirationTime = dismissalTime + (30 * 24 * 60 * 60 * 1000); // 30 days + localStorage.setItem('bannerDismissalTime_' + userId, dismissalTime); + localStorage.setItem('bannerExpirationTime_' + userId, expirationTime); + } + + // Function to check if banner should be shown or hidden + function checkBannerVisibility() { + let userId = getUserId(); + let bannerDismissalTime = localStorage.getItem('bannerDismissalTime_' + userId); + let bannerExpirationTime = localStorage.getItem('bannerExpirationTime_' + userId); + let currentTime = getCurrentTime(); + + if (bannerDismissalTime && bannerExpirationTime && currentTime > bannerExpirationTime) { + // Banner was dismissed and it's not within the expiration period, so show it + $('#originalContent').show(); + } else if (bannerDismissalTime && bannerExpirationTime && currentTime < bannerExpirationTime) { + // Banner was dismissed and it's within the expiration period, so hide it + $('#originalContent').hide(); + } else { + // Banner has not been dismissed ever so we need to show it. + $('#originalContent').show(); + } + } + + // Click event for dismiss button $('#dismissButton').click(function() { $('#originalContent').slideUp('slow', function() { - // If you want to do something after the slide-up, do it here. - // For example, you can hide the entire div: - // $(this).hide(); + let userId = getUserId(); + let dismissalTime = getCurrentTime(); + setDismissalAndExpirationTime(userId, dismissalTime); }); }); + + // Check banner visibility on page load + checkBannerVisibility(); // When the form is submitted $("#survey_report_form").submit(function(event){ event.preventDefault(); // Prevent the form from submitting traditionally diff --git a/openedx/features/survey_report/templates/survey_report/admin_banner.html b/openedx/features/survey_report/templates/survey_report/admin_banner.html index fa0b37ecf751..e13eb655f63c 100644 --- a/openedx/features/survey_report/templates/survey_report/admin_banner.html +++ b/openedx/features/survey_report/templates/survey_report/admin_banner.html @@ -1,7 +1,7 @@ {% block survey_report_banner %} {% load static %} {% if show_survey_report_banner %} -