From 98689ab32a256d0cbd02e29ce73389961441ae63 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 6 Jun 2024 17:00:39 -0700 Subject: [PATCH 01/18] fix: reindex_studio was crashing if instance had too many courses (#34936) --- openedx/core/djangoapps/content/search/api.py | 59 ++++++++++--------- .../content/search/tests/test_api.py | 3 + 2 files changed, 33 insertions(+), 29 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index bbde4fc98230..f658155eb812 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 @@ -358,30 +356,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( From 009123a019adac2cada670f41f28c520cfea5b9e Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 6 Jun 2024 17:00:48 -0700 Subject: [PATCH 02/18] fix: searching tag refinement was sometimes not working (#34935) --- openedx/core/djangoapps/content/search/api.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index f658155eb812..17824dbc2a7e 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -325,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 ############## From 4fe8180327715429c01476e76aa43c8f1129135d Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Mon, 20 May 2024 14:37:58 -0400 Subject: [PATCH 03/18] fix: libraries performance problem This is an attempt to fix a performance problem on the libraries home page. When you go to studio home and click on the libraries tab, on prod it will be quick for admins but extremely slow for course instructors (> 12 seconds) and leads to timeouts. It grows with the number of libraries that are assigned to the instructor. The Python code for the request to load libraries for a particular user goes through all existing libraries and then checks all of the user's roles for each library, which results in a complexity of O(l*r), l=libraries, r=roles. This PR improves the complexity to O(l). The BulkRoleCache and RoleCache classes were using a python set to store all roles for a particular user. A user can have a large number of roles, and lookup speed of iterating through a set is slow (O(n)). Most roles don't have the same course id, however. So if you have the course id of the role you're looking for, we can use a dict of course ids that contain related roles. The number of roles per course id is negligible, so we arrive at a lookup speed of O(1) when looking up a user's roles that belong to a specific course id. The BulkRoleCache now caches and stores user roles in a data structure like this: { 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. For example, Global Staff roles. }, user_id_2: { ... } # Similar structure for another user } While this changes the data structure used to store roles under the hood and adds the new property `roles_by_course_id` to the RoleCache, when initializing the RoleCache will store roles additionally in the previous data structure - as a flat set - in the `_roles` property accessible via `all_roles_set`. This establishes backwards compatibility. We are now storing roles twice in the RoleCache (in each of the two data structures), which means this takes twice as much memory, but only in the scope of a request. (cherry picked from commit e95d7e7e3239c0fe84448e1ef5314b4169dd6f7e) --- .../contentstore/rest_api/v1/views/home.py | 1 + cms/djangoapps/contentstore/views/course.py | 2 +- common/djangoapps/student/auth.py | 3 +- common/djangoapps/student/role_helpers.py | 2 +- common/djangoapps/student/roles.py | 85 ++++++++++++++++--- common/djangoapps/student/tests/test_roles.py | 84 +++++++++++++++++- 6 files changed, 157 insertions(+), 20 deletions(-) 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/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/common/djangoapps/student/auth.py b/common/djangoapps/student/auth.py index fb2999eca98d..08b4255feeae 100644 --- a/common/djangoapps/student/auth.py +++ b/common/djangoapps/student/auth.py @@ -23,7 +23,7 @@ OrgContentCreatorRole, OrgInstructorRole, OrgLibraryUserRole, - OrgStaffRole + OrgStaffRole, ) # Studio permissions: @@ -106,6 +106,7 @@ def get_user_permissions(user, course_key, org=None): # 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_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' From c4bdf74e127e4874a812dc11ba1295d6a30117bc Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 20 May 2024 14:50:17 -0400 Subject: [PATCH 04/18] fix: add in-line docs correct namespace for content_groups_for_teams (#34783) --- openedx/core/lib/teams_config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 From 50097c2031f997d8efc046081b1cbe195dc704c5 Mon Sep 17 00:00:00 2001 From: Glib Glugovskiy Date: Tue, 11 Jun 2024 19:00:26 +0300 Subject: [PATCH 05/18] fix: security issue limited staff have edit access through some APIs in CMS --- common/djangoapps/student/auth.py | 13 +++++++++---- common/djangoapps/student/tests/test_authz.py | 17 ++++++++++++++--- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/common/djangoapps/student/auth.py b/common/djangoapps/student/auth.py index 08b4255feeae..d3dc51616c75 100644 --- a/common/djangoapps/student/auth.py +++ b/common/djangoapps/student/auth.py @@ -95,14 +95,19 @@ 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 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): """ From 28ddf51bfca0c8c4f5b66859a39e774d60382fcc Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Wed, 19 Jul 2023 15:08:17 -0400 Subject: [PATCH 06/18] build: lms/static/css/vendor/* -> common/static/css/vendor The git-ignored target directory for LMS Sass compilation is: lms/static/css Unfortunately, that directory contains git-controlled directory of vendored-in static assets: lms/static/css/vendor This is a problem for a couple reasons: 1. In Tutor, we would like to make lms/static/css a symlink to an external location for the sake of build efficiency. This is impossible to do without clobbering lms/static/css/vendor and dirtying the git state. 2. More generally, when optimizing (or just understanding) a build system, it adds complexity when git-controlled source directories are mixed up inside git-ignored target directories. The solution is to simply merge these vendored-in assets to another existing git-controlled vendor directory: common/static/css/vendor LMS already reads assets from this folder, so no further changes need to be made. common/static/css is fully git-controlled, so we avoid the complexity described above. Backport of: 97a9f08a9fdd20f47055920cec9ec92ac359218c --- .../css/vendor/images/treeview-default-line.gif | Bin .../static/css/vendor/images/treeview-default.gif | Bin {lms => common}/static/css/vendor/indicator.gif | Bin .../static/css/vendor/jquery.autocomplete.css | 0 .../static/css/vendor/jquery.treeview.css | 0 5 files changed, 0 insertions(+), 0 deletions(-) rename {lms => common}/static/css/vendor/images/treeview-default-line.gif (100%) rename {lms => common}/static/css/vendor/images/treeview-default.gif (100%) rename {lms => common}/static/css/vendor/indicator.gif (100%) rename {lms => common}/static/css/vendor/jquery.autocomplete.css (100%) rename {lms => common}/static/css/vendor/jquery.treeview.css (100%) 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 From 6ab0b7a1190168b5ebfc73bfe9af191089b2e067 Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Mon, 3 Jun 2024 08:50:26 -0400 Subject: [PATCH 07/18] build: git-ignore static asset artifacts whether they are links or dirs Remove the trailing slashes from the .gitignore entries for static asset build artifacts. With these slashes, only directory contents are ignored. Without these slashes, the artifact is ignored whether it is a directory *or* an actual file (particularly, in our case: a symlink). This allows us to build edx-platform static assets into a separate directory, and then symlink to them from edx-platform. Also: We remove the duplicate cms/static/css/ gitignore entry. Backport of: 1f41529a763ca16f7670f65a700901443845f340 --- .dockerignore | 1 - .gitignore | 11 +++++------ 2 files changed, 5 insertions(+), 7 deletions(-) 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/ From aa70fea890ecd622d392270cd21453b9ae9fe578 Mon Sep 17 00:00:00 2001 From: Asespinel <79876430+Asespinel@users.noreply.github.com> Date: Wed, 19 Jun 2024 09:22:25 -0500 Subject: [PATCH 08/18] feat: hide the survey report banner for a month after clicking the dismiss button (#34914) This hides the survey report banner from the Django Admin for a particular user for one month after they click on the "dismiss" button. This is done completely on the client side using localStorage, so the same user could see the banner again if they're logging in with a different browser. --- .../static/survey_report/js/admin_banner.js | 46 +++++++++++++++++-- .../templates/survey_report/admin_banner.html | 3 +- 2 files changed, 45 insertions(+), 4 deletions(-) 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 %} -
+ From e5f074b45d94741a6476b3e471d2bf9042be7efe Mon Sep 17 00:00:00 2001 From: magajh Date: Mon, 15 Jul 2024 09:23:25 -0400 Subject: [PATCH 09/18] chore: upgrade Django to 4.2.14 --- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- scripts/user_retirement/requirements/base.txt | 2 +- scripts/user_retirement/requirements/testing.txt | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 2ae5ba235a89..db14ceee890f 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -177,7 +177,7 @@ defusedxml==0.7.1 # ora2 # python3-openid # social-auth-core -django==4.2.13 +django==4.2.14 # via # -c requirements/edx/../common_constraints.txt # -c requirements/edx/../constraints.txt diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 8b1bb858efe2..cda3abc1db76 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -347,7 +347,7 @@ distlib==0.3.8 # via # -r requirements/edx/testing.txt # virtualenv -django==4.2.13 +django==4.2.14 # via # -c requirements/edx/../common_constraints.txt # -c requirements/edx/../constraints.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 9e1dee112300..d766c8f0298a 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -227,7 +227,7 @@ defusedxml==0.7.1 # ora2 # python3-openid # social-auth-core -django==4.2.13 +django==4.2.14 # via # -c requirements/edx/../common_constraints.txt # -c requirements/edx/../constraints.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 452fec9a8643..67cdbe56435b 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -263,7 +263,7 @@ dill==0.3.8 # via pylint distlib==0.3.8 # via virtualenv -django==4.2.13 +django==4.2.14 # via # -c requirements/edx/../common_constraints.txt # -c requirements/edx/../constraints.txt diff --git a/scripts/user_retirement/requirements/base.txt b/scripts/user_retirement/requirements/base.txt index 934f4293d699..1e510e645926 100644 --- a/scripts/user_retirement/requirements/base.txt +++ b/scripts/user_retirement/requirements/base.txt @@ -39,7 +39,7 @@ click==8.1.6 # edx-django-utils cryptography==42.0.7 # via pyjwt -django==4.2.13 +django==4.2.14 # via # -c scripts/user_retirement/requirements/../../../requirements/common_constraints.txt # -c scripts/user_retirement/requirements/../../../requirements/constraints.txt diff --git a/scripts/user_retirement/requirements/testing.txt b/scripts/user_retirement/requirements/testing.txt index 2bae9bb1a272..15cc2d9849bf 100644 --- a/scripts/user_retirement/requirements/testing.txt +++ b/scripts/user_retirement/requirements/testing.txt @@ -56,7 +56,7 @@ cryptography==42.0.7 # pyjwt ddt==1.7.2 # via -r scripts/user_retirement/requirements/testing.in -django==4.2.13 +django==4.2.14 # via # -r scripts/user_retirement/requirements/base.txt # django-crum From 4744ea1c39f22d397a0206a4761d833496a2851d Mon Sep 17 00:00:00 2001 From: Muhammad Soban Javed Date: Mon, 10 Jun 2024 23:31:34 +0500 Subject: [PATCH 10/18] chore!: uprgade social-auth-app-django to version 5.4.1 --- requirements/constraints.txt | 8 -------- requirements/edx/base.txt | 7 +++---- requirements/edx/development.txt | 7 +++---- requirements/edx/doc.txt | 7 +++---- requirements/edx/testing.txt | 7 +++---- 5 files changed, 12 insertions(+), 24 deletions(-) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index 8f7a66f9cc2f..866ac9e3bf7d 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -62,14 +62,6 @@ pycodestyle<2.9.0 pylint<2.16.0 # greater version failing quality test. Fix them in seperate ticket. -# adding these constraints to minimize boto3 and botocore changeset -social-auth-core==4.3.0 - -# social-auth-app-django versions after 5.2.0 has a problematic migration that will cause issues deployments with large -# `social_auth_usersocialauth` tables. 5.1.0 has missing migration and 5.2.0 has that problematic migration. -social-auth-app-django==5.0.0 - - # urllib3>=2.0.0 conflicts with elastic search && snowflake-connector-python packages # which require urllib3<2 for now. # Issue for unpinning: https://github.com/openedx/edx-platform/issues/32222 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index db14ceee890f..44fbf21df2eb 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -246,6 +246,7 @@ django==4.2.14 # openedx-filters # openedx-learning # ora2 + # social-auth-app-django # super-csv # xblock-google-drive # xss-utils @@ -1097,14 +1098,12 @@ slumber==0.7.1 # edx-rest-api-client snowflake-connector-python==3.10.0 # via edx-enterprise -social-auth-app-django==5.0.0 +social-auth-app-django==5.4.1 # via - # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in # edx-auth-backends -social-auth-core==4.3.0 +social-auth-core==4.5.4 # via - # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in # edx-auth-backends # social-auth-app-django diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index cda3abc1db76..93e4b117a852 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -420,6 +420,7 @@ django==4.2.14 # openedx-filters # openedx-learning # ora2 + # social-auth-app-django # super-csv # xblock-google-drive # xss-utils @@ -1945,15 +1946,13 @@ snowflake-connector-python==3.10.0 # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # edx-enterprise -social-auth-app-django==5.0.0 +social-auth-app-django==5.4.1 # via - # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # edx-auth-backends -social-auth-core==4.3.0 +social-auth-core==4.5.4 # via - # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # edx-auth-backends diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index d766c8f0298a..d5d3136349f1 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -296,6 +296,7 @@ django==4.2.14 # openedx-filters # openedx-learning # ora2 + # social-auth-app-django # super-csv # xblock-google-drive # xss-utils @@ -1300,14 +1301,12 @@ snowflake-connector-python==3.10.0 # via # -r requirements/edx/base.txt # edx-enterprise -social-auth-app-django==5.0.0 +social-auth-app-django==5.4.1 # via - # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt # edx-auth-backends -social-auth-core==4.3.0 +social-auth-core==4.5.4 # via - # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt # edx-auth-backends # social-auth-app-django diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 67cdbe56435b..3c109ddaf98f 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -332,6 +332,7 @@ django==4.2.14 # openedx-filters # openedx-learning # ora2 + # social-auth-app-django # super-csv # xblock-google-drive # xss-utils @@ -1466,14 +1467,12 @@ snowflake-connector-python==3.10.0 # via # -r requirements/edx/base.txt # edx-enterprise -social-auth-app-django==5.0.0 +social-auth-app-django==5.4.1 # via - # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt # edx-auth-backends -social-auth-core==4.3.0 +social-auth-core==4.5.4 # via - # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt # edx-auth-backends # social-auth-app-django From 670772b2b9a677560775f9a5b4959de00f7571ab Mon Sep 17 00:00:00 2001 From: Muhammad Soban Javed Date: Fri, 28 Jun 2024 01:49:59 +0500 Subject: [PATCH 11/18] chore: add migration from social_django --- ...ricalusersocialauth_extra_data_and_more.py | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 lms/djangoapps/support/migrations/0006_alter_historicalusersocialauth_extra_data_and_more.py 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'), + ), + ] From 2165da0a344db6f1e18af530aa6c699135570793 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Tue, 23 Jul 2024 10:51:32 -0400 Subject: [PATCH 12/18] chore: Re-compile requirements. --- requirements/common_constraints.txt | 8 ++++++++ requirements/edx/base.txt | 1 + requirements/edx/development.txt | 1 + requirements/edx/doc.txt | 1 + requirements/edx/testing.txt | 1 + 5 files changed, 12 insertions(+) diff --git a/requirements/common_constraints.txt b/requirements/common_constraints.txt index 4abc9ae22cb3..ef8bc86061b7 100644 --- a/requirements/common_constraints.txt +++ b/requirements/common_constraints.txt @@ -21,6 +21,7 @@ Django<5.0 # elasticsearch>=7.14.0 includes breaking changes in it which caused issues in discovery upgrade process. # elastic search changelog: https://www.elastic.co/guide/en/enterprise-search/master/release-notes-7.14.0.html +# See https://github.com/openedx/edx-platform/issues/35126 for more info elasticsearch<7.14.0 # django-simple-history>3.0.0 adds indexing and causes a lot of migrations to be affected @@ -33,3 +34,10 @@ elasticsearch<7.14.0 # So we need to pin it globally, for now. # Ticket for unpinning: https://github.com/openedx/edx-lint/issues/407 importlib-metadata<7 + +# Cause: https://github.com/openedx/event-tracking/pull/290 +# event-tracking 2.4.1 upgrades to pymongo 4.4.0 which is not supported on edx-platform. +# We will pin event-tracking to do not break existing installations +# This can be unpinned once https://github.com/openedx/edx-platform/issues/34586 +# has been resolved and edx-platform is running with pymongo>=4.4.0 +event-tracking<2.4.1 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 44fbf21df2eb..d68c66cc6ef4 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -567,6 +567,7 @@ enmerkar-underscore==2.3.0 # via -r requirements/edx/kernel.in event-tracking==2.4.0 # via + # -c requirements/edx/../common_constraints.txt # -r requirements/edx/kernel.in # edx-completion # edx-proctoring diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 93e4b117a852..f78a27d03813 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -895,6 +895,7 @@ enmerkar-underscore==2.3.0 # -r requirements/edx/testing.txt event-tracking==2.4.0 # via + # -c requirements/edx/../common_constraints.txt # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # edx-completion diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index d5d3136349f1..de65c64de287 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -651,6 +651,7 @@ enmerkar-underscore==2.3.0 # via -r requirements/edx/base.txt event-tracking==2.4.0 # via + # -c requirements/edx/../common_constraints.txt # -r requirements/edx/base.txt # edx-completion # edx-proctoring diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 3c109ddaf98f..0baf5e170300 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -688,6 +688,7 @@ enmerkar-underscore==2.3.0 # via -r requirements/edx/base.txt event-tracking==2.4.0 # via + # -c requirements/edx/../common_constraints.txt # -r requirements/edx/base.txt # edx-completion # edx-proctoring From ed72248eb6acc785acc989148bf8f12d518d296e Mon Sep 17 00:00:00 2001 From: connorhaugh Date: Thu, 15 Feb 2024 18:04:49 +0000 Subject: [PATCH 13/18] fix: libraries across orgs --- cms/djangoapps/contentstore/utils.py | 4 +- cms/djangoapps/contentstore/views/library.py | 53 +++++++++++++++---- .../contentstore/views/tests/test_library.py | 39 +++++++++----- 3 files changed, 70 insertions(+), 26 deletions(-) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 9bcd77c3eea9..1b5af4f769bd 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,7 @@ 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/library.py b/cms/djangoapps/contentstore/views/library.py index 17aa24c5712a..8ea6bf1e3657 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 @@ -68,13 +68,10 @@ def should_redirect_to_library_authoring_mfe(): REDIRECT_TO_LIBRARY_AUTHORING_MICROFRONTEND.is_enabled() ) - -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 +81,48 @@ 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. + + users can only create libraries in orgs they are a part of. + """ + 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 +145,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..fdbb9d905c62 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) From d23b41e2898cda5a19bd2e26ffda854e75bd7829 Mon Sep 17 00:00:00 2001 From: connorhaugh Date: Thu, 15 Feb 2024 21:26:45 +0000 Subject: [PATCH 14/18] docs: imporved comment --- cms/djangoapps/contentstore/views/library.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/views/library.py b/cms/djangoapps/contentstore/views/library.py index 8ea6bf1e3657..7ba80bcab9f6 100644 --- a/cms/djangoapps/contentstore/views/library.py +++ b/cms/djangoapps/contentstore/views/library.py @@ -98,7 +98,16 @@ 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. - users can only create libraries in orgs they are a part of. + 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 From d757cfa2c7564afc479b2dfdab25b57adc068109 Mon Sep 17 00:00:00 2001 From: connorhaugh Date: Wed, 4 Oct 2023 12:46:36 -0400 Subject: [PATCH 15/18] fix: cohorts data can be private --- common/djangoapps/util/file.py | 11 ++++++++++- lms/djangoapps/instructor/views/api.py | 3 ++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/common/djangoapps/util/file.py b/common/djangoapps/util/file.py index 66fc2a6f5f35..b884ca46a703 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/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: From 62269f8c956e25f36d83e06299a90018a535b79f Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 25 Jul 2024 10:24:36 -0400 Subject: [PATCH 16/18] fix: Remove errant pluses from a bad merge. --- common/djangoapps/util/file.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/common/djangoapps/util/file.py b/common/djangoapps/util/file.py index b884ca46a703..b2892e6f42c9 100644 --- a/common/djangoapps/util/file.py +++ b/common/djangoapps/util/file.py @@ -80,10 +80,10 @@ def store_uploaded_file( 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, -+ ) + ACL='private', + Bucket=settings.AWS_STORAGE_BUCKET_NAME, + Key=stored_file_name, + ) if validator: try: From 8813e8b02cf30f2e5d619db72c4a2cdd80834eb8 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 25 Jul 2024 10:49:14 -0400 Subject: [PATCH 17/18] style: Fix a pylint and style violations. --- cms/djangoapps/contentstore/utils.py | 3 ++- cms/djangoapps/contentstore/views/library.py | 2 +- cms/djangoapps/contentstore/views/tests/test_library.py | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 1b5af4f769bd..21f605a9e440 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -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_view_create_library_button(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/library.py b/cms/djangoapps/contentstore/views/library.py index 7ba80bcab9f6..870c192653d2 100644 --- a/cms/djangoapps/contentstore/views/library.py +++ b/cms/djangoapps/contentstore/views/library.py @@ -68,6 +68,7 @@ def should_redirect_to_library_authoring_mfe(): REDIRECT_TO_LIBRARY_AUTHORING_MICROFRONTEND.is_enabled() ) + def user_can_view_create_library_button(user): """ Helper method for displaying the visibilty of the create_library_button. @@ -92,7 +93,6 @@ def user_can_view_create_library_button(user): return not disable_course_creation - def user_can_create_library(user, org): """ Helper method for returning the library creation status for a particular user, diff --git a/cms/djangoapps/contentstore/views/tests/test_library.py b/cms/djangoapps/contentstore/views/tests/test_library.py index fdbb9d905c62..fa6505419725 100644 --- a/cms/djangoapps/contentstore/views/tests/test_library.py +++ b/cms/djangoapps/contentstore/views/tests/test_library.py @@ -111,7 +111,7 @@ def test_library_creator_status_with_course_staff_role_for_enabled_creator_group 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. + # 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() From 1e0d575f5ff686ccc600e1efc1aab9e812a059ee Mon Sep 17 00:00:00 2001 From: Fateme Khodayari <55655542+FatemeKhodayari@users.noreply.github.com> Date: Tue, 30 Jul 2024 18:16:09 +0330 Subject: [PATCH 18/18] fix: course progress url returned based on course_home_mfe_progress_tab_is_active (#35149) --- lms/djangoapps/learner_home/serializers.py | 3 ++- .../learner_home/test_serializers.py | 26 ++++++++++++++++++- lms/djangoapps/learner_home/utils.py | 16 ++++++++++++ 3 files changed, 43 insertions(+), 2 deletions(-) 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})