From a86d29f1551a86862e0a33eb28f731ef803a50ef Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Thu, 28 Aug 2025 08:43:54 -0400 Subject: [PATCH 01/10] feat!: Drop import_from_modulestore app (2/3 -- models) (#37240) Part of: https://github.com/openedx/edx-platform/issues/37242 --- .../0002_drop_import_from_modulestore_all.py | 65 +++++++++ .../import_from_modulestore/models.py | 138 +----------------- 2 files changed, 66 insertions(+), 137 deletions(-) create mode 100644 cms/djangoapps/import_from_modulestore/migrations/0002_drop_import_from_modulestore_all.py diff --git a/cms/djangoapps/import_from_modulestore/migrations/0002_drop_import_from_modulestore_all.py b/cms/djangoapps/import_from_modulestore/migrations/0002_drop_import_from_modulestore_all.py new file mode 100644 index 0000000000..81639e71ba --- /dev/null +++ b/cms/djangoapps/import_from_modulestore/migrations/0002_drop_import_from_modulestore_all.py @@ -0,0 +1,65 @@ +# Generated by Django 4.2.23 on 2025-08-19 16:21 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('import_from_modulestore', '0001_initial'), + ] + + operations = [ + migrations.AlterUniqueTogether( + name='publishableentityimport', + unique_together=None, + ), + migrations.RemoveField( + model_name='publishableentityimport', + name='import_event', + ), + migrations.RemoveField( + model_name='publishableentityimport', + name='resulting_change', + ), + migrations.RemoveField( + model_name='publishableentityimport', + name='resulting_mapping', + ), + migrations.AlterUniqueTogether( + name='publishableentitymapping', + unique_together=None, + ), + migrations.RemoveField( + model_name='publishableentitymapping', + name='target_entity', + ), + migrations.RemoveField( + model_name='publishableentitymapping', + name='target_package', + ), + migrations.AlterUniqueTogether( + name='stagedcontentforimport', + unique_together=None, + ), + migrations.RemoveField( + model_name='stagedcontentforimport', + name='import_event', + ), + migrations.RemoveField( + model_name='stagedcontentforimport', + name='staged_content', + ), + migrations.DeleteModel( + name='Import', + ), + migrations.DeleteModel( + name='PublishableEntityImport', + ), + migrations.DeleteModel( + name='PublishableEntityMapping', + ), + migrations.DeleteModel( + name='StagedContentForImport', + ), + ] diff --git a/cms/djangoapps/import_from_modulestore/models.py b/cms/djangoapps/import_from_modulestore/models.py index 5b6122749b..a682631b08 100644 --- a/cms/djangoapps/import_from_modulestore/models.py +++ b/cms/djangoapps/import_from_modulestore/models.py @@ -1,139 +1,3 @@ """ -Models for the course to library import app. +Models for the course to library import app (TO BE DELETED) """ -import uuid as uuid_tools - -from django.contrib.auth import get_user_model -from django.db import models -from django.utils.translation import gettext_lazy as _ - -from model_utils.models import TimeStampedModel -from opaque_keys.edx.django.models import ( - LearningContextKeyField, - UsageKeyField, -) -from openedx_learning.api.authoring_models import LearningPackage, PublishableEntity - -from .data import ImportStatus - -User = get_user_model() - - -class Import(TimeStampedModel): - """ - Represents the action of a user importing a modulestore-based course or legacy - library into a learning-core based learning package (today, that is always a content library). - """ - - uuid = models.UUIDField(default=uuid_tools.uuid4, editable=False, unique=True) - status = models.CharField( - max_length=100, - choices=ImportStatus.choices, - default=ImportStatus.NOT_STARTED, - db_index=True - ) - user = models.ForeignKey(User, on_delete=models.CASCADE) - - # Note: For now, this will always be a course key. In the future, it may be a legacy library key. - source_key = LearningContextKeyField(help_text=_('The modulestore course'), max_length=255, db_index=True) - target_change = models.ForeignKey(to='oel_publishing.DraftChangeLog', on_delete=models.SET_NULL, null=True) - - class Meta: - verbose_name = _('Import from modulestore') - verbose_name_plural = _('Imports from modulestore') - - def __str__(self): - return f'{self.source_key} → {self.target_change}' - - def set_status(self, status: ImportStatus): - """ - Set import status. - """ - self.status = status - self.save() - if status in [ImportStatus.IMPORTED, ImportStatus.CANCELED]: - self.clean_related_staged_content() - - def clean_related_staged_content(self) -> None: - """ - Clean related staged content. - """ - for staged_content_for_import in self.staged_content_for_import.all(): - staged_content_for_import.staged_content.delete() - - -class PublishableEntityMapping(TimeStampedModel): - """ - Represents a mapping between a source usage key and a target publishable entity. - """ - - source_usage_key = UsageKeyField( - max_length=255, - help_text=_('Original usage key/ID of the thing that has been imported.'), - ) - target_package = models.ForeignKey(LearningPackage, on_delete=models.CASCADE) - target_entity = models.ForeignKey(PublishableEntity, on_delete=models.CASCADE) - - class Meta: - unique_together = ('source_usage_key', 'target_package') - - def __str__(self): - return f'{self.source_usage_key} → {self.target_entity}' - - -class PublishableEntityImport(TimeStampedModel): - """ - Represents a publishableentity version that has been imported into a learning package (e.g. content library) - - This is a many-to-many relationship between a container version and a course to library import. - """ - - import_event = models.ForeignKey(Import, on_delete=models.CASCADE) - resulting_mapping = models.ForeignKey(PublishableEntityMapping, on_delete=models.SET_NULL, null=True, blank=True) - resulting_change = models.OneToOneField( - to='oel_publishing.DraftChangeLogRecord', - # a changelog record can be pruned, which would set this to NULL, but not delete the - # entire import record - null=True, - on_delete=models.SET_NULL, - ) - - class Meta: - unique_together = ( - ('import_event', 'resulting_mapping'), - ) - - def __str__(self): - return f'{self.import_event} → {self.resulting_mapping}' - - -class StagedContentForImport(TimeStampedModel): - """ - Represents m2m relationship between an import and staged content created for that import. - """ - - import_event = models.ForeignKey( - Import, - on_delete=models.CASCADE, - related_name='staged_content_for_import', - ) - staged_content = models.OneToOneField( - to='content_staging.StagedContent', - on_delete=models.CASCADE, - related_name='staged_content_for_import', - ) - # Since StagedContent stores all the keys of the saved blocks, this field was added to optimize search. - source_usage_key = UsageKeyField( - max_length=255, - help_text=_( - 'The original Usage key of the highest-level component that was saved in StagedContent.' - ), - ) - - class Meta: - unique_together = ( - ('import_event', 'staged_content'), - ) - - def __str__(self): - return f'{self.import_event} → {self.staged_content}' From 052b930ef5467c4f89e5e8112137cc666d355df4 Mon Sep 17 00:00:00 2001 From: Mubbshar Anwar <78487564+mubbsharanwar@users.noreply.github.com> Date: Thu, 28 Aug 2025 19:01:19 +0500 Subject: [PATCH 02/10] fix: fix script tag quot escaped (#37296) --- .../discussion/rest_api/discussions_notifications.py | 5 ++++- .../rest_api/tests/test_discussions_notifications.py | 12 +++++------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/lms/djangoapps/discussion/rest_api/discussions_notifications.py b/lms/djangoapps/discussion/rest_api/discussions_notifications.py index 8249f17027..8efb2e8acb 100644 --- a/lms/djangoapps/discussion/rest_api/discussions_notifications.py +++ b/lms/djangoapps/discussion/rest_api/discussions_notifications.py @@ -2,6 +2,7 @@ Discussion notifications sender util. """ import re +import html from bs4 import BeautifulSoup, Tag from django.conf import settings @@ -447,7 +448,9 @@ def clean_thread_html_body(html_body): """ Get post body with tags removed and limited to 500 characters """ - html_body = BeautifulSoup(Truncator(html_body).chars(500, html=True), 'html.parser') + truncated_body = Truncator(html_body).chars(500, html=True) + truncated_body = html.unescape(truncated_body) + html_body = BeautifulSoup(truncated_body, 'html.parser') tags_to_remove = [ "a", "link", # Link Tags diff --git a/lms/djangoapps/discussion/rest_api/tests/test_discussions_notifications.py b/lms/djangoapps/discussion/rest_api/tests/test_discussions_notifications.py index 247bd23540..aaa920a0ff 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_discussions_notifications.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_discussions_notifications.py @@ -2,7 +2,6 @@ Unit tests for the DiscussionNotificationSender class """ import re -import django import unittest from unittest.mock import MagicMock, patch @@ -109,13 +108,12 @@ class TestCleanThreadHtmlBody(unittest.TestCase):

Script test:

Some other content that should remain.

""" - excepted_script_quot = 'alert(&quot;hello&quot;);' if django.VERSION >= (5, 0) else 'alert("hello");' expected_output = ( - f'

This is a link to a page.

' - f'

Here is an image:

' - f'

Embedded video:

' - f'

Script test: {excepted_script_quot}

' - f'

Some other content that should remain.

' + '

This is a link to a page.

' + '

Here is an image:

' + '

Embedded video:

' + '

Script test: alert("hello");

' + '

Some other content that should remain.

' ) result = clean_thread_html_body(html_body) From 8085bf6be45da515daec1bc45493def043a23fdb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 28 Aug 2025 13:30:17 -0300 Subject: [PATCH 03/10] feat: Add `unlinkable` to xblock actions and update `top_level_parent_key` on unlink [FC-0097] (#37215) - Adds the `unlinkable` action to the XBlock object sent to the frontend - Updates the `top_level_parent_key` reference when unlinking containers. If you unlink a Section with Subsections and Units, this updates the `top_level_parent_key` for the Subsections to `None` (they are the top level now), and the `top_level_parent_key` for the Units to the corresponding parent Subsection. --- cms/djangoapps/contentstore/models.py | 8 +-- .../rest_api/v2/views/downstreams.py | 29 +++++++++- .../v2/views/tests/test_downstreams.py | 57 ++++++++++++++++--- cms/djangoapps/contentstore/tasks.py | 6 +- cms/djangoapps/contentstore/utils.py | 18 +++--- .../xblock_storage_handlers/view_handlers.py | 9 ++- cms/lib/xblock/upstream_sync.py | 49 ++++++++++++++-- cms/static/js/views/pages/container.js | 20 +++++++ cms/templates/studio_xblock_wrapper.html | 6 ++ 9 files changed, 169 insertions(+), 33 deletions(-) diff --git a/cms/djangoapps/contentstore/models.py b/cms/djangoapps/contentstore/models.py index fc7bec1dc3..fd98ec44fe 100644 --- a/cms/djangoapps/contentstore/models.py +++ b/cms/djangoapps/contentstore/models.py @@ -128,6 +128,10 @@ class EntityLinkBase(models.Model): class Meta: abstract = True + @classmethod + def get_by_downstream_usage_key(cls, downstream_usage_key: UsageKey): + return cls.objects.get(downstream_usage_key=downstream_usage_key) + class ComponentLink(EntityLinkBase): """ @@ -523,10 +527,6 @@ class ContainerLink(EntityLinkBase): link.save() return link - @classmethod - def get_by_downstream_usage_key(cls, downstream_usage_key: UsageKey): - return cls.objects.get(downstream_usage_key=downstream_usage_key) - class LearningContextLinksStatusChoices(models.TextChoices): """ diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py index 4e168bcaf8..e95dac4890 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py @@ -455,16 +455,39 @@ class DownstreamView(DeveloperErrorViewMixin, APIView): Sever an XBlock's link to upstream content. """ downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True) + affected_blocks: list[XBlock] = [] try: - sever_upstream_link(downstream) + # Try to get the upstream key before severing the link, so we can delete + # the corresponding ComponentLink or ContainerLink below. + try: + upstream_key = UpstreamLink.get_for_block(downstream).upstream_key + except NoUpstream: + # Even if we don't have an UpstreamLink, we still need to check + # if the block has the upstream key set, so we don't want to + # raise an exception here. + upstream_key = None + + affected_blocks = sever_upstream_link(downstream) + + # Remove the ComponentLink or ContainerLink, if it exists. + if upstream_key: + if isinstance(upstream_key, LibraryUsageLocatorV2): + ComponentLink.get_by_downstream_usage_key(downstream.usage_key).delete() + elif isinstance(upstream_key, LibraryContainerLocator): + ContainerLink.get_by_downstream_usage_key(downstream.usage_key).delete() except NoUpstream: logger.exception( "Tried to DELETE upstream link of '%s', but it wasn't linked to anything in the first place. " "Will do nothing. ", usage_key_string, ) - else: - modulestore().update_item(downstream, request.user.id) + finally: + if affected_blocks: + # If we successfully severed the upstream link, then we need to update the affected blocks. + with modulestore().bulk_operations(downstream.usage_key.context_key): + for block in affected_blocks: + modulestore().update_item(block, request.user.id) + return Response(status=204) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py index 00db886240..1381e96a39 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py @@ -2,29 +2,29 @@ Unit tests for /api/contentstore/v2/downstreams/* JSON APIs. """ import json -import ddt from datetime import datetime, timezone -from unittest.mock import patch, MagicMock +from unittest.mock import MagicMock, patch +import ddt from django.conf import settings from django.urls import reverse from freezegun import freeze_time +from opaque_keys.edx.keys import ContainerKey, UsageKey +from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 from organizations.models import Organization from cms.djangoapps.contentstore.helpers import StaticFileNotices -from cms.lib.xblock.upstream_sync import BadUpstream, UpstreamLink from cms.djangoapps.contentstore.tests.utils import CourseTestCase from cms.djangoapps.contentstore.xblock_storage_handlers import view_handlers as xblock_view_handlers from cms.djangoapps.contentstore.xblock_storage_handlers.xblock_helpers import get_block_key_dict -from opaque_keys.edx.keys import ContainerKey, UsageKey -from opaque_keys.edx.locator import LibraryLocatorV2 -from common.djangoapps.student.tests.factories import UserFactory +from cms.lib.xblock.upstream_sync import BadUpstream, UpstreamLink from common.djangoapps.student.auth import add_users from common.djangoapps.student.roles import CourseStaffRole +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.content_libraries import api as lib_api from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory -from openedx.core.djangoapps.content_libraries import api as lib_api from .. import downstreams as downstreams_views @@ -42,7 +42,7 @@ URL_LIB_CONTAINER_PUBLISH = URL_LIB_CONTAINER + 'publish/' # Publish changes to def _get_upstream_link_good_and_syncable(downstream): return UpstreamLink( upstream_ref=downstream.upstream, - upstream_key=UsageKey.from_string(downstream.upstream), + upstream_key=LibraryUsageLocatorV2.from_string(downstream.upstream), downstream_key=str(downstream.usage_key), version_synced=downstream.upstream_version, version_available=(downstream.upstream_version or 0) + 1, @@ -433,6 +433,45 @@ class DeleteDownstreamViewTest(SharedErrorTestCases, SharedModuleStoreTestCase): assert response.status_code == 204 assert mock_sever.call_count == 1 + def test_unlink_parent_should_update_children_top_level_parent(self): + """ + If we unlink a parent block, do all children get the new top-level parent? + """ + self.client.login(username="superuser", password="password") + + all_downstreams = self.client.get( + "/api/contentstore/v2/downstreams/", + data={"course_id": str(self.course.id)}, + ) + assert all_downstreams.data["count"] == 11 + + response = self.call_api(self.top_level_downstream_chapter.usage_key) + assert response.status_code == 204 + + # Check that all children have their top_level_downstream_parent_key updated + subsection = modulestore().get_item(self.top_level_downstream_sequential.usage_key) + assert subsection.top_level_downstream_parent_key is None + + unit = modulestore().get_item(self.top_level_downstream_unit_2.usage_key) + # The sequential is the top-level parent for the unit + assert unit.top_level_downstream_parent_key == { + "id": str(self.top_level_downstream_sequential.usage_key.block_id), + "type": str(self.top_level_downstream_sequential.usage_key.block_type), + } + + video = modulestore().get_item(self.top_level_downstream_video_key) + # The sequential is the top-level parent for the video + assert video.top_level_downstream_parent_key == { + "id": str(self.top_level_downstream_sequential.usage_key.block_id), + "type": str(self.top_level_downstream_sequential.usage_key.block_type), + } + + all_downstreams = self.client.get( + "/api/contentstore/v2/downstreams/", + data={"course_id": str(self.course.id)}, + ) + assert all_downstreams.data["count"] == 10 + class _DownstreamSyncViewTestMixin(SharedErrorTestCases): """ @@ -562,7 +601,7 @@ class GetUpstreamViewTest( SharedModuleStoreTestCase, ): """ - Test that `GET /api/v2/contentstore/downstreams-all?...` returns list of links based on the provided filter. + Test that `GET /api/v2/contentstore/downstreams?...` returns list of links based on the provided filter. """ def call_api( diff --git a/cms/djangoapps/contentstore/tasks.py b/cms/djangoapps/contentstore/tasks.py index e3dbfe0615..419d04f571 100644 --- a/cms/djangoapps/contentstore/tasks.py +++ b/cms/djangoapps/contentstore/tasks.py @@ -1654,7 +1654,7 @@ def handle_create_xblock_upstream_link(usage_key): # The top-level parent link does not exist yet, # it is necessary to create it first. handle_create_xblock_upstream_link(str(top_level_parent_usage_key)) - create_or_update_xblock_upstream_link(xblock, xblock.course_id) + create_or_update_xblock_upstream_link(xblock) @shared_task @@ -1671,7 +1671,7 @@ def handle_update_xblock_upstream_link(usage_key): return if not xblock.upstream or not xblock.upstream_version: return - create_or_update_xblock_upstream_link(xblock, xblock.course_id) + create_or_update_xblock_upstream_link(xblock) @shared_task @@ -1711,7 +1711,7 @@ def create_or_update_upstream_links( course_status.update_status(LearningContextLinksStatusChoices.FAILED) return for xblock in xblocks: - create_or_update_xblock_upstream_link(xblock, course_key, created) + create_or_update_xblock_upstream_link(xblock, created) course_status.update_status(LearningContextLinksStatusChoices.COMPLETED) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 6204f020d9..775999ea60 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -2385,7 +2385,7 @@ def get_xblock_render_error(request, xblock): return "" -def _create_or_update_component_link(course_key: CourseKey, created: datetime | None, xblock): +def _create_or_update_component_link(created: datetime | None, xblock): """ Create or update upstream->downstream link for components in database for given xblock. """ @@ -2399,7 +2399,7 @@ def _create_or_update_component_link(course_key: CourseKey, created: datetime | top_level_parent_usage_key = None if xblock.top_level_downstream_parent_key is not None: top_level_parent_usage_key = BlockUsageLocator( - course_key, + xblock.usage_key.course_key, xblock.top_level_downstream_parent_key.get('type'), xblock.top_level_downstream_parent_key.get('id'), ) @@ -2408,7 +2408,7 @@ def _create_or_update_component_link(course_key: CourseKey, created: datetime | lib_component, upstream_usage_key=upstream_usage_key, upstream_context_key=str(upstream_usage_key.context_key), - downstream_context_key=course_key, + downstream_context_key=xblock.usage_key.course_key, downstream_usage_key=xblock.usage_key, top_level_parent_usage_key=top_level_parent_usage_key, version_synced=xblock.upstream_version, @@ -2417,7 +2417,7 @@ def _create_or_update_component_link(course_key: CourseKey, created: datetime | ) -def _create_or_update_container_link(course_key: CourseKey, created: datetime | None, xblock): +def _create_or_update_container_link(created: datetime | None, xblock): """ Create or update upstream->downstream link for containers in database for given xblock. """ @@ -2431,7 +2431,7 @@ def _create_or_update_container_link(course_key: CourseKey, created: datetime | top_level_parent_usage_key = None if xblock.top_level_downstream_parent_key is not None: top_level_parent_usage_key = BlockUsageLocator( - course_key, + xblock.usage_key.course_key, xblock.top_level_downstream_parent_key.get('type'), xblock.top_level_downstream_parent_key.get('id'), ) @@ -2440,7 +2440,7 @@ def _create_or_update_container_link(course_key: CourseKey, created: datetime | lib_component, upstream_container_key=upstream_container_key, upstream_context_key=str(upstream_container_key.context_key), - downstream_context_key=course_key, + downstream_context_key=xblock.usage_key.course_key, downstream_usage_key=xblock.usage_key, version_synced=xblock.upstream_version, top_level_parent_usage_key=top_level_parent_usage_key, @@ -2449,7 +2449,7 @@ def _create_or_update_container_link(course_key: CourseKey, created: datetime | ) -def create_or_update_xblock_upstream_link(xblock, course_key: CourseKey, created: datetime | None = None) -> None: +def create_or_update_xblock_upstream_link(xblock, created: datetime | None = None) -> None: """ Create or update upstream->downstream link in database for given xblock. """ @@ -2457,11 +2457,11 @@ def create_or_update_xblock_upstream_link(xblock, course_key: CourseKey, created return None try: # Try to create component link - _create_or_update_component_link(course_key, created, xblock) + _create_or_update_component_link(created, xblock) except InvalidKeyError: # It is possible that the upstream is a container and UsageKeyV2 parse failed # Create upstream container link and raise InvalidKeyError if xblock.upstream is a valid key. - _create_or_update_container_link(course_key, created, xblock) + _create_or_update_container_link(created, xblock) def get_previous_run_course_key(course_key): diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index afaf193e60..c0ab6e0e59 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -1118,11 +1118,14 @@ def create_xblock_info( # lint-amnesty, pylint: disable=too-many-statements # defining the default value 'True' for delete, duplicate, drag and add new child actions # in xblock_actions for each xblock. + # The unlinkable action is set to None by default, which means the action is not applicable for + # any xblock unless explicitly set to True or False for a specific xblock condition. xblock_actions = { "deletable": True, "draggable": True, "childAddable": True, "duplicable": True, + "unlinkable": None, } explanatory_message = None @@ -1320,9 +1323,13 @@ def create_xblock_info( # lint-amnesty, pylint: disable=too-many-statements # Also add upstream info upstream_info = UpstreamLink.try_get_for_block(xblock, log_error=False).to_json() xblock_info["upstream_info"] = upstream_info - # Disable adding or removing children component if xblock is imported from library + if upstream_info["upstream_ref"]: + # Disable adding or removing children component if xblock is imported from library xblock_actions["childAddable"] = False + # Enable unlinking only for top level imported components + xblock_actions["unlinkable"] = not upstream_info["has_top_level_parent"] + if is_xblock_unit: # if xblock is a Unit we add the discussion_enabled option xblock_info["discussion_enabled"] = xblock.discussion_enabled diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py index 076afef505..484e75b10a 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -28,6 +28,7 @@ from opaque_keys.edx.keys import UsageKey from xblock.exceptions import XBlockNotFoundError from xblock.fields import Scope, String, Integer, Dict from xblock.core import XBlockMixin, XBlock +from xmodule.util.keys import BlockKey if t.TYPE_CHECKING: from django.contrib.auth.models import User # pylint: disable=imported-auth-user @@ -190,14 +191,14 @@ class UpstreamLink: downstream.upstream, ) return cls( - upstream_ref=getattr(downstream, "upstream", ""), + upstream_ref=getattr(downstream, "upstream", None), upstream_key=None, downstream_key=str(getattr(downstream, "usage_key", "")), version_synced=getattr(downstream, "upstream_version", None), version_available=None, version_declined=None, error_message=str(exc), - has_top_level_parent=False, + has_top_level_parent=getattr(downstream, "top_level_downstream_parent_key", None) is not None, ) @classmethod @@ -311,7 +312,37 @@ def decline_sync(downstream: XBlock, user_id=None) -> None: store.update_item(downstream, user_id) -def sever_upstream_link(downstream: XBlock) -> None: +def _update_children_top_level_parent( + downstream: XBlock, + new_top_level_parent_key: dict[str, str] | None +) -> list[XBlock]: + """ + Given a new top-level parent block, update the `top_level_downstream_parent_key` field on the downstream block + and all of its children. + + If `new_top_level_parent_key` is None, use the current downstream block's usage_key for its children. + + Returns a list of all affected blocks. + """ + if not downstream.has_children: + return [] + + affected_blocks = [] + for child in downstream.get_children(): + child.top_level_downstream_parent_key = new_top_level_parent_key + affected_blocks.append(child) + # If the `new_top_level_parent_key` is None, the current level assume the top-level + # parent key for its children. + child_top_level_parent_key = new_top_level_parent_key if new_top_level_parent_key is not None else ( + BlockKey.from_usage_key(child.usage_key)._asdict() + ) + + affected_blocks.extend(_update_children_top_level_parent(child, child_top_level_parent_key)) + + return affected_blocks + + +def sever_upstream_link(downstream: XBlock) -> list[XBlock]: """ Given an XBlock that is linked to upstream content, disconnect the link, such that authors are never again prompted to sync upstream updates. Erase all `.upstream*` fields from the downtream block. @@ -320,10 +351,12 @@ def sever_upstream_link(downstream: XBlock) -> None: because once a downstream block has been de-linked from source (e.g., a Content Library block), it is no different than if the block had just been copy-pasted in the first place. - Does not save `downstream` to the store. That is left up to the caller. + Does not save `downstream` (or its children) to the store. That is left up to the caller. If `downstream` lacks a link, then this raises NoUpstream (though it is reasonable for callers to handle such exception and ignore it, as the end result is the same: `downstream.upstream is None`). + + Returns a list of affected blocks, which includes the `downstream` block itself and all of its children. """ if not downstream.upstream: raise NoUpstream() @@ -336,6 +369,14 @@ def sever_upstream_link(downstream: XBlock) -> None: continue setattr(downstream, fetched_upstream_field, None) # Null out upstream_display_name, et al. + # Set the top_level_dowwnstream_parent_key to None, and calls `_update_children_top_level_parent` to + # update all children with the new top_level_dowwnstream_parent_key for each of them. + downstream.top_level_downstream_parent_key = None + affected_blocks = _update_children_top_level_parent(downstream, None) + + # Return the list of affected blocks, which includes the `downstream` block itself. + return [downstream, *affected_blocks] + def _get_library_xblock_url(usage_key: LibraryUsageLocatorV2): """ diff --git a/cms/static/js/views/pages/container.js b/cms/static/js/views/pages/container.js index 7360c79317..a7aabb1096 100644 --- a/cms/static/js/views/pages/container.js +++ b/cms/static/js/views/pages/container.js @@ -30,6 +30,7 @@ function($, _, Backbone, gettext, BasePage, 'click .copy-button': 'copyXBlock', 'click .move-button': 'showMoveXBlockModal', 'click .delete-button': 'deleteXBlock', + 'click .unlink-button': 'unlinkXBlock', 'click .library-sync-button': 'showXBlockLibraryChangesPreview', 'click .problem-bank-v2-add-button': 'showSelectV2LibraryContent', 'click .show-actions-menu-button': 'showXBlockActionsMenu', @@ -922,6 +923,25 @@ function($, _, Backbone, gettext, BasePage, this.deleteComponent(this.findXBlockElement(event.target)); }, + unlinkXBlock: function(event) { + event.preventDefault(); + const primaryHeader = $(event.target).closest('.xblock-header-primary, .nav-actions'); + const usageId = encodeURI(primaryHeader.attr('data-usage-id')); + try { + if (this.options.isIframeEmbed) { + window.parent.postMessage( + { + type: 'unlinkXBlock', + message: 'Unlink the XBlock', + payload: { usageId } + }, document.referrer + ); + } + } catch (e) { + console.error(e); + } + }, + createComponent: function(template, target, iframeMessageData) { // A placeholder element is created in the correct location for the new xblock // and then onNewXBlock will replace it with a rendering of the xblock. Note that diff --git a/cms/templates/studio_xblock_wrapper.html b/cms/templates/studio_xblock_wrapper.html index f6022c6ac0..89b62cc17c 100644 --- a/cms/templates/studio_xblock_wrapper.html +++ b/cms/templates/studio_xblock_wrapper.html @@ -26,6 +26,7 @@ has_not_configured_message = messages.get('summary',{}).get('type', None) == 'no block_is_unit = is_unit(xblock) upstream_info = UpstreamLink.try_get_for_block(xblock, log_error=False) +can_unlink = upstream_info.upstream_ref and not upstream_info.has_top_level_parent %> <%namespace name='static' file='static_content.html'/> @@ -202,6 +203,11 @@ upstream_info = UpstreamLink.try_get_for_block(xblock, log_error=False) ${_("Delete")} % endif + % if can_unlink: +
  • + ${_("Unlink from Library")} +
    • + % endif From 3de21d9c41493dc0dbdb4a4dca32a34303f1d956 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 28 Aug 2025 10:24:29 -0400 Subject: [PATCH 04/10] test: Disable the Django Debug Toolbar by default. Disable the toolbar by default but make it easy to turn back on as needed. This is in response to the discussion here: https://discuss.openedx.org/t/lets-remove-django-debug-toolbar/16847 --- lms/envs/devstack.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/lms/envs/devstack.py b/lms/envs/devstack.py index 20bdba0d7d..8f25169cbd 100644 --- a/lms/envs/devstack.py +++ b/lms/envs/devstack.py @@ -22,6 +22,8 @@ ORA2_FILEUPLOAD_BACKEND = 'django' DEBUG = True +INTERNAL_IPS = ('127.0.0.1',) + USE_I18N = True DEFAULT_TEMPLATE_ENGINE['OPTIONS']['debug'] = True LMS_BASE = 'localhost:18000' @@ -81,13 +83,14 @@ DJFS = { ################################ DEBUG TOOLBAR ################################ -INSTALLED_APPS += ['debug_toolbar'] -MIDDLEWARE += [ - 'lms.djangoapps.discussion.django_comment_client.utils.QueryCountDebugMiddleware', - 'debug_toolbar.middleware.DebugToolbarMiddleware', -] - -INTERNAL_IPS = ('127.0.0.1',) +# The Django Debug Toolbar is disabled by default, if you need to enable it for +# debugging. Simply uncomment the INSTALLED_APPS and MIDDLEWARE section below. +# +# INSTALLED_APPS += ['debug_toolbar'] +# MIDDLEWARE += [ +# 'lms.djangoapps.discussion.django_comment_client.utils.QueryCountDebugMiddleware', +# 'debug_toolbar.middleware.DebugToolbarMiddleware', +# ] DEBUG_TOOLBAR_PANELS = ( 'debug_toolbar.panels.versions.VersionsPanel', From eb5ba0f87ddcdf413afff24e63a078871b837de5 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 28 Aug 2025 23:13:06 +0530 Subject: [PATCH 05/10] fix: exception while trying to check sync status of deleted upstream (#37298) Use `Upstream.try_get_for_block` instead of `Upstream.get_for_block` which raises `BadUpstream` if upstream block is deleted. --- cms/lib/xblock/upstream_sync.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py index 484e75b10a..1b78b6b366 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -125,7 +125,7 @@ class UpstreamLink: for child in downstream_children: if child.upstream: - child_upstream_link = UpstreamLink.get_for_block(child) + child_upstream_link = UpstreamLink.try_get_for_block(child) child_ready_to_sync = bool( child_upstream_link.upstream_ref and From acad883a38b49f94ffdcfc64bf5af66f934a5c4c Mon Sep 17 00:00:00 2001 From: Muhammad Adeel Tajamul <77053848+muhammadadeeltajamul@users.noreply.github.com> Date: Fri, 29 Aug 2025 11:41:28 +0500 Subject: [PATCH 06/10] fix: added info in notification preferences (#37295) --- .../notifications/tests/test_views.py | 31 +++++++++++++------ .../core/djangoapps/notifications/views.py | 6 +++- 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/openedx/core/djangoapps/notifications/tests/test_views.py b/openedx/core/djangoapps/notifications/tests/test_views.py index d5b2237303..06d615f07d 100644 --- a/openedx/core/djangoapps/notifications/tests/test_views.py +++ b/openedx/core/djangoapps/notifications/tests/test_views.py @@ -30,7 +30,10 @@ from openedx.core.djangoapps.notifications.email.utils import encrypt_string from openedx.core.djangoapps.notifications.models import ( CourseNotificationPreference, Notification, NotificationPreference ) -from openedx.core.djangoapps.notifications.serializers import add_non_editable_in_preference +from openedx.core.djangoapps.notifications.serializers import ( + add_info_to_notification_config, + add_non_editable_in_preference +) from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -711,6 +714,7 @@ class TestNotificationPreferencesView(ModuleStoreTestCase): expected_data = exclude_inaccessible_preferences(self.default_data['data'], self.user) expected_data = add_non_editable_in_preference(expected_data) + expected_data = add_info_to_notification_config(expected_data) self.assertEqual(response.data['data'], expected_data) @@ -768,25 +772,30 @@ class TestNotificationPreferencesView(ModuleStoreTestCase): "web": False, "email": False, "push": False, - "email_cadence": "Daily" + "email_cadence": "Daily", + "info": "" }, "new_question_post": { "web": False, "email": False, "push": False, - "email_cadence": "Daily" + "email_cadence": "Daily", + "info": "" }, "new_instructor_all_learners_post": { "web": False, "email": False, "push": False, - "email_cadence": "Daily" + "email_cadence": "Daily", + "info": "" }, "core": { "web": False, "email": False, "push": False, - "email_cadence": "Daily" + "email_cadence": "Daily", + "info": "Notifications for responses and comments on your posts, and the ones you’re " + "following, including endorsements to your responses and on your posts." } }, "non_editable": { @@ -803,13 +812,15 @@ class TestNotificationPreferencesView(ModuleStoreTestCase): "web": False, "email": False, "push": False, - "email_cadence": "Daily" + "email_cadence": "Daily", + "info": "" }, "core": { "web": True, "email": True, "push": True, - "email_cadence": "Daily" + "email_cadence": "Daily", + "info": "Notifications for new announcements and updates from the course team." } }, "non_editable": { @@ -824,13 +835,15 @@ class TestNotificationPreferencesView(ModuleStoreTestCase): "web": False, "email": False, "push": False, - "email_cadence": "Daily" + "email_cadence": "Daily", + "info": "" }, "core": { "web": True, "email": True, "push": True, - "email_cadence": "Daily" + "email_cadence": "Daily", + "info": "Notifications for submission grading." } }, "non_editable": { diff --git a/openedx/core/djangoapps/notifications/views.py b/openedx/core/djangoapps/notifications/views.py index 57ef88cde1..091be365d4 100644 --- a/openedx/core/djangoapps/notifications/views.py +++ b/openedx/core/djangoapps/notifications/views.py @@ -31,6 +31,7 @@ from .models import Notification from .serializers import ( NotificationSerializer, UserNotificationPreferenceUpdateAllSerializer, + add_info_to_notification_config, add_non_editable_in_preference ) from .tasks import create_notification_preference @@ -323,11 +324,14 @@ class NotificationPreferencesView(APIView): type_details['push'] = user_pref.push type_details['email_cadence'] = user_pref.email_cadence exclude_inaccessible_preferences(structured_preferences, request.user) + structured_preferences = add_non_editable_in_preference( + add_info_to_notification_config(structured_preferences) + ) return Response({ 'status': 'success', 'message': 'Notification preferences retrieved successfully.', 'show_preferences': get_show_notifications_tray(self.request.user), - 'data': add_non_editable_in_preference(structured_preferences) + 'data': structured_preferences }, status=status.HTTP_200_OK) def put(self, request): From 728a325781848d7749fc7d1e943dcc052cc3d6bd Mon Sep 17 00:00:00 2001 From: Shafqat Farhan Date: Fri, 29 Aug 2025 11:56:33 +0500 Subject: [PATCH 07/10] feat: Added pluggable override on financial_assistance (#37303) --- lms/djangoapps/courseware/views/views.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index af157e76a0..1b199df151 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -32,6 +32,7 @@ from django.views.decorators.csrf import ensure_csrf_cookie from django.views.decorators.http import require_GET, require_http_methods, require_POST from django.views.generic import View from edx_django_utils.monitoring import set_custom_attribute, set_custom_attributes_for_course_key +from edx_django_utils.plugins import pluggable_override from ipware.ip import get_client_ip from xblock.core import XBlock @@ -2082,6 +2083,7 @@ def financial_assistance(request, course_id=None): @login_required @require_POST +@pluggable_override('OVERRIDE_FINANCIAL_ASSISTANCE_REQUEST') def financial_assistance_request(request): """Submit a request for financial assistance to Zendesk.""" try: From cff5e562b82295157f830168e32c93e00c5bf9e9 Mon Sep 17 00:00:00 2001 From: edX requirements bot Date: Fri, 29 Aug 2025 10:09:02 -0400 Subject: [PATCH 08/10] chore: Upgrade Python requirements --- requirements/edx/base.txt | 4 ++-- requirements/edx/development.txt | 6 +++--- requirements/edx/doc.txt | 6 +++--- requirements/edx/semgrep.txt | 2 +- requirements/edx/testing.txt | 4 ++-- scripts/user_retirement/requirements/base.txt | 4 ++-- scripts/user_retirement/requirements/testing.txt | 4 ++-- 7 files changed, 15 insertions(+), 15 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 1e7353b145..37e002e244 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -70,14 +70,14 @@ bleach[css]==6.2.0 # xblock-poll boto==2.49.0 # via -r requirements/edx/kernel.in -boto3==1.40.19 +boto3==1.40.20 # via # -r requirements/edx/kernel.in # django-ses # fs-s3fs # ora2 # snowflake-connector-python -botocore==1.40.19 +botocore==1.40.20 # via # -r requirements/edx/kernel.in # boto3 diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index c3e13d3773..bd4cf21f84 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -140,7 +140,7 @@ boto==2.49.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt -boto3==1.40.19 +boto3==1.40.20 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt @@ -148,7 +148,7 @@ boto3==1.40.19 # fs-s3fs # ora2 # snowflake-connector-python -botocore==1.40.19 +botocore==1.40.20 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt @@ -1272,7 +1272,7 @@ meilisearch==0.37.0 # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # edx-search -mistune==3.1.3 +mistune==3.1.4 # via # -r requirements/edx/doc.txt # sphinx-mdinclude diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index e30f79dccc..63bff99f96 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -105,14 +105,14 @@ bleach[css]==6.2.0 # xblock-poll boto==2.49.0 # via -r requirements/edx/base.txt -boto3==1.40.19 +boto3==1.40.20 # via # -r requirements/edx/base.txt # django-ses # fs-s3fs # ora2 # snowflake-connector-python -botocore==1.40.19 +botocore==1.40.20 # via # -r requirements/edx/base.txt # boto3 @@ -930,7 +930,7 @@ meilisearch==0.37.0 # via # -r requirements/edx/base.txt # edx-search -mistune==3.1.3 +mistune==3.1.4 # via sphinx-mdinclude mongoengine==0.29.1 # via -r requirements/edx/base.txt diff --git a/requirements/edx/semgrep.txt b/requirements/edx/semgrep.txt index d55d6f3e35..87703c82a8 100644 --- a/requirements/edx/semgrep.txt +++ b/requirements/edx/semgrep.txt @@ -113,7 +113,7 @@ ruamel-yaml==0.18.15 # via semgrep ruamel-yaml-clib==0.2.12 # via ruamel-yaml -semgrep==1.133.0 +semgrep==1.134.0 # via -r requirements/edx/semgrep.in tomli==2.0.2 # via semgrep diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index b191338e2a..69f026496d 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -102,14 +102,14 @@ bleach[css]==6.2.0 # xblock-poll boto==2.49.0 # via -r requirements/edx/base.txt -boto3==1.40.19 +boto3==1.40.20 # via # -r requirements/edx/base.txt # django-ses # fs-s3fs # ora2 # snowflake-connector-python -botocore==1.40.19 +botocore==1.40.20 # via # -r requirements/edx/base.txt # boto3 diff --git a/scripts/user_retirement/requirements/base.txt b/scripts/user_retirement/requirements/base.txt index 471b303326..e141223acc 100644 --- a/scripts/user_retirement/requirements/base.txt +++ b/scripts/user_retirement/requirements/base.txt @@ -10,9 +10,9 @@ attrs==25.3.0 # via zeep backoff==2.2.1 # via -r scripts/user_retirement/requirements/base.in -boto3==1.40.19 +boto3==1.40.20 # via -r scripts/user_retirement/requirements/base.in -botocore==1.40.19 +botocore==1.40.20 # via # boto3 # s3transfer diff --git a/scripts/user_retirement/requirements/testing.txt b/scripts/user_retirement/requirements/testing.txt index e186a99fca..0050141b3a 100644 --- a/scripts/user_retirement/requirements/testing.txt +++ b/scripts/user_retirement/requirements/testing.txt @@ -14,11 +14,11 @@ attrs==25.3.0 # zeep backoff==2.2.1 # via -r scripts/user_retirement/requirements/base.txt -boto3==1.40.19 +boto3==1.40.20 # via # -r scripts/user_retirement/requirements/base.txt # moto -botocore==1.40.19 +botocore==1.40.20 # via # -r scripts/user_retirement/requirements/base.txt # boto3 From a4d4ddf9e300ff4f4ca43b78cb21831bbd03f0ca Mon Sep 17 00:00:00 2001 From: Deborah Kaplan Date: Fri, 29 Aug 2025 16:09:02 -0400 Subject: [PATCH 09/10] chore: removing ENABLE_V2_CERT_DISPLAY_SETTINGS (#37302) the remnants of the logic that used this setting has been gone for a long time. This removes the toggle. FIXES: APER-1405 --- cms/envs/mock.yml | 1 - lms/envs/mock.yml | 1 - 2 files changed, 2 deletions(-) diff --git a/cms/envs/mock.yml b/cms/envs/mock.yml index 1001262683..d5f48151f8 100644 --- a/cms/envs/mock.yml +++ b/cms/envs/mock.yml @@ -440,7 +440,6 @@ FEATURES: ENABLE_SERVICE_STATUS: true ENABLE_SPECIAL_EXAMS: true ENABLE_THIRD_PARTY_AUTH: true - ENABLE_V2_CERT_DISPLAY_SETTINGS: true ENABLE_VERIFIED_CERTIFICATES: true ENABLE_VIDEO_ABSTRACTION_LAYER_API: true ENABLE_VIDEO_BUMPER: true diff --git a/lms/envs/mock.yml b/lms/envs/mock.yml index 10ec3d1e26..565ad043ef 100644 --- a/lms/envs/mock.yml +++ b/lms/envs/mock.yml @@ -596,7 +596,6 @@ FEATURES: ENABLE_SERVICE_STATUS: true ENABLE_SPECIAL_EXAMS: true ENABLE_THIRD_PARTY_AUTH: true - ENABLE_V2_CERT_DISPLAY_SETTINGS: true ENABLE_VERIFIED_CERTIFICATES: true ENABLE_VIDEO_ABSTRACTION_LAYER_API: true ENABLE_VIDEO_BUMPER: true From 0bed7d7127408a20bc67746f5213a29c7dc03264 Mon Sep 17 00:00:00 2001 From: "kshitij.sobti" Date: Fri, 29 Aug 2025 18:21:03 +0530 Subject: [PATCH 10/10] feat: Add support for using LTI data to populate user profile Currently the LTI provider implementation auto-creates a random user when logging in, however, the LTI launch can include relevant user details such as their email, full name and even a username. This change makes the LTI code use the provided details if the "Use lti pii" setting is set in the Django admin. --- .../0005_lticonsumer_use_lti_pii.py | 18 +++ lms/djangoapps/lti_provider/models.py | 4 + .../lti_provider/tests/test_users.py | 107 ++++++++++++++++-- lms/djangoapps/lti_provider/users.py | 36 ++++-- lms/djangoapps/lti_provider/views.py | 4 +- 5 files changed, 151 insertions(+), 18 deletions(-) create mode 100644 lms/djangoapps/lti_provider/migrations/0005_lticonsumer_use_lti_pii.py diff --git a/lms/djangoapps/lti_provider/migrations/0005_lticonsumer_use_lti_pii.py b/lms/djangoapps/lti_provider/migrations/0005_lticonsumer_use_lti_pii.py new file mode 100644 index 0000000000..9728dd8d60 --- /dev/null +++ b/lms/djangoapps/lti_provider/migrations/0005_lticonsumer_use_lti_pii.py @@ -0,0 +1,18 @@ +# Generated by Django 4.2.23 on 2025-08-29 12:43 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('lti_provider', '0004_require_user_account'), + ] + + operations = [ + migrations.AddField( + model_name='lticonsumer', + name='use_lti_pii', + field=models.BooleanField(blank=True, default=False, help_text='When checked, the platform will automatically use any personal information provided via LTI to create an account. Otherwise an anonymous account will be created.'), + ), + ] diff --git a/lms/djangoapps/lti_provider/models.py b/lms/djangoapps/lti_provider/models.py index 94582d4bf0..f763321748 100644 --- a/lms/djangoapps/lti_provider/models.py +++ b/lms/djangoapps/lti_provider/models.py @@ -40,6 +40,10 @@ class LtiConsumer(models.Model): "in this instance. This is required only for linking learner accounts with " "the LTI consumer. See the Open edX LTI Provider documentation for more details." )) + use_lti_pii = models.BooleanField(blank=True, default=False, help_text=_( + "When checked, the platform will automatically use any personal information provided " + "via LTI to create an account. Otherwise an anonymous account will be created." + )) @staticmethod def get_or_supplement(instance_guid, consumer_key): diff --git a/lms/djangoapps/lti_provider/tests/test_users.py b/lms/djangoapps/lti_provider/tests/test_users.py index fa8274eef3..e94caae5a6 100644 --- a/lms/djangoapps/lti_provider/tests/test_users.py +++ b/lms/djangoapps/lti_provider/tests/test_users.py @@ -2,10 +2,11 @@ Tests for the LTI user management functionality """ - +import itertools import string from unittest.mock import MagicMock, PropertyMock, patch +import ddt import pytest from django.contrib.auth.models import AnonymousUser, User # lint-amnesty, pylint: disable=imported-auth-user from django.core.exceptions import PermissionDenied @@ -73,6 +74,7 @@ class UserManagementHelperTest(TestCase): f"Username has forbidden character '{username[char]}'" +@ddt.ddt @patch('lms.djangoapps.lti_provider.users.switch_user', autospec=True) @patch('lms.djangoapps.lti_provider.users.create_lti_user', autospec=True) class AuthenticateLtiUserTest(TestCase): @@ -156,7 +158,10 @@ class AuthenticateLtiUserTest(TestCase): create_user.assert_called_with(self.lti_user_id, self.lti_consumer) users.authenticate_lti_user(request, self.lti_user_id, self.auto_linking_consumer) - create_user.assert_called_with(self.lti_user_id, self.auto_linking_consumer, self.old_user.email) + create_user.assert_called_with(self.lti_user_id, self.auto_linking_consumer, { + "email": self.old_user.email, + "full_name": "", + }) def test_auto_linking_of_users_using_lis_person_contact_email_primary_case_insensitive(self, create_user, switch_user): # pylint: disable=line-too-long request = RequestFactory().post("/", {"lis_person_contact_email_primary": self.old_user.email.upper()}) @@ -166,7 +171,10 @@ class AuthenticateLtiUserTest(TestCase): create_user.assert_called_with(self.lti_user_id, self.lti_consumer) users.authenticate_lti_user(request, self.lti_user_id, self.auto_linking_consumer) - create_user.assert_called_with(self.lti_user_id, self.auto_linking_consumer, request.user.email) + create_user.assert_called_with(self.lti_user_id, self.auto_linking_consumer, { + "email": self.old_user.email, + "full_name": "", + }) def test_raise_exception_trying_to_auto_link_unauthenticate_user(self, create_user, switch_user): request = RequestFactory().post("/") @@ -190,7 +198,57 @@ class AuthenticateLtiUserTest(TestCase): assert not create_user.called switch_user.assert_called_with(self.request, lti_user, self.auto_linking_consumer) + @ddt.data( + *itertools.product( + ( + ( + { + "lis_person_contact_email_primary": "some_email@example.com", + "lis_person_name_given": "John", + "lis_person_name_family": "Doe", + }, + "some_email@example.com", + "John Doe", + ), + ( + { + "lis_person_contact_email_primary": "some_email@example.com", + "lis_person_name_full": "John Doe", + "lis_person_name_given": "Jacob", + }, + "some_email@example.com", + "John Doe", + ), + ( + {"lis_person_contact_email_primary": "some_email@example.com", "lis_person_name_full": "John Doe"}, + "some_email@example.com", + "John Doe", + ), + ({"lis_person_contact_email_primary": "some_email@example.com"}, "some_email@example.com", ""), + ({"lis_person_contact_email_primary": ""}, "", ""), + ({"lis_person_contact_email_primary": ""}, "", ""), + ({}, "", ""), + ), + [True, False], + ) + ) + @ddt.unpack + def test_create_user_when_user_account_not_required(self, params, enable_lti_pii, create_user, switch_user): + post_params, email, name = params + self.auto_linking_consumer.require_user_account = False + self.auto_linking_consumer.use_lti_pii = enable_lti_pii + self.auto_linking_consumer.save() + request = RequestFactory().post("/", post_params) + request.user = AnonymousUser() + users.authenticate_lti_user(request, self.lti_user_id, self.auto_linking_consumer) + if enable_lti_pii: + profile = {"email": email, "full_name": name, "username": self.lti_user_id} + create_user.assert_called_with(self.lti_user_id, self.auto_linking_consumer, profile) + else: + create_user.assert_called_with(self.lti_user_id, self.auto_linking_consumer) + +@ddt.ddt class CreateLtiUserTest(TestCase): """ Tests for the create_lti_user function in users.py @@ -222,22 +280,22 @@ class CreateLtiUserTest(TestCase): @patch('lms.djangoapps.lti_provider.users.generate_random_edx_username', side_effect=['edx_id', 'new_edx_id']) def test_unique_username_created(self, username_mock): User(username='edx_id').save() - users.create_lti_user('lti_user_id', self.lti_consumer) + users.create_lti_user('lti_user_id', self.lti_consumer, None) assert username_mock.call_count == 2 assert User.objects.count() == 3 user = User.objects.get(username='new_edx_id') assert user.email == 'new_edx_id@lti.example.com' def test_existing_user_is_linked(self): - lti_user = users.create_lti_user('lti_user_id', self.lti_consumer, self.existing_user.email) + lti_user = users.create_lti_user('lti_user_id', self.lti_consumer, {"email": self.existing_user.email}) assert lti_user.lti_consumer == self.lti_consumer assert lti_user.edx_user == self.existing_user def test_only_one_lti_user_edx_user_for_each_lti_consumer(self): - users.create_lti_user('lti_user_id', self.lti_consumer, self.existing_user.email) + users.create_lti_user('lti_user_id', self.lti_consumer, {"email": self.existing_user.email}) with pytest.raises(IntegrityError): - users.create_lti_user('lti_user_id', self.lti_consumer, self.existing_user.email) + users.create_lti_user('lti_user_id', self.lti_consumer, {"email": self.existing_user.email}) def test_create_multiple_lti_users_for_edx_user_if_lti_consumer_varies(self): lti_consumer_2 = LtiConsumer( @@ -247,11 +305,42 @@ class CreateLtiUserTest(TestCase): ) lti_consumer_2.save() - lti_user_1 = users.create_lti_user('lti_user_id', self.lti_consumer, self.existing_user.email) - lti_user_2 = users.create_lti_user('lti_user_id', lti_consumer_2, self.existing_user.email) + lti_user_1 = users.create_lti_user('lti_user_id', self.lti_consumer, {"email": self.existing_user.email}) + lti_user_2 = users.create_lti_user('lti_user_id', lti_consumer_2, {"email": self.existing_user.email}) assert lti_user_1.edx_user == lti_user_2.edx_user + def test_create_lti_user_with_full_profile(self): + lti_user = users.create_lti_user('lti_user_id', self.lti_consumer, { + "email": "some.user@example.com", + "full_name": "John Doe", + "username": "john_doe", + }) + assert lti_user.edx_user.email == "some.user@example.com" + assert lti_user.edx_user.username == "john_doe" + assert lti_user.edx_user.profile.name == "John Doe" + + @patch('lms.djangoapps.lti_provider.users.generate_random_edx_username', side_effect=['edx_id']) + def test_create_lti_user_with_missing_username_in_profile(self, mock): + lti_user = users.create_lti_user('lti_user_id', self.lti_consumer, { + "email": "some.user@example.com", + "full_name": "John Doe", + }) + assert lti_user.edx_user.email == "some.user@example.com" + assert lti_user.edx_user.username == "edx_id" + assert lti_user.edx_user.profile.name == "John Doe" + + @patch('lms.djangoapps.lti_provider.users.generate_random_edx_username', side_effect=['edx_id', 'edx_id123']) + def test_create_lti_user_with_duplicate_username_in_profile(self, mock): + lti_user = users.create_lti_user('lti_user_id', self.lti_consumer, { + "email": "some.user@example.com", + "full_name": "John Doe", + "username": self.existing_user.username, + }) + assert lti_user.edx_user.email == "some.user@example.com" + assert lti_user.edx_user.username == "edx_id" + assert lti_user.edx_user.profile.name == "John Doe" + class LtiBackendTest(TestCase): """ diff --git a/lms/djangoapps/lti_provider/users.py b/lms/djangoapps/lti_provider/users.py index d1ade6ead3..168d6c4c6d 100644 --- a/lms/djangoapps/lti_provider/users.py +++ b/lms/djangoapps/lti_provider/users.py @@ -19,6 +19,20 @@ from lms.djangoapps.lti_provider.models import LtiUser from openedx.core.djangoapps.safe_sessions.middleware import mark_user_change_as_expected +def get_lti_user_details(request): + """ + Returns key LTI user details from the LTI launch request. + """ + post_data = request.POST + email = post_data.get("lis_person_contact_email_primary", "").lower() + full_name = post_data.get("lis_person_name_full", "") + given_name = post_data.get("lis_person_name_given", "") + family_name = post_data.get("lis_person_name_family", "") + if not full_name and given_name: + full_name = f"{given_name} {family_name}" + return dict(email=email, full_name=full_name) + + def authenticate_lti_user(request, lti_user_id, lti_consumer): """ Determine whether the user specified by the LTI launch has an existing @@ -28,7 +42,7 @@ def authenticate_lti_user(request, lti_user_id, lti_consumer): If the currently logged-in user does not match the user specified by the LTI launch, log out the old user and log in the LTI identity. """ - lis_email = request.POST.get("lis_person_contact_email_primary") + profile = get_lti_user_details(request) try: lti_user = LtiUser.objects.get( @@ -40,11 +54,14 @@ def authenticate_lti_user(request, lti_user_id, lti_consumer): if lti_consumer.require_user_account: # Verify that the email from the LTI Launch and the logged-in user are the same # before linking the LtiUser with the edx_user. - if request.user.is_authenticated and request.user.email.lower() == lis_email.lower(): - lti_user = create_lti_user(lti_user_id, lti_consumer, request.user.email) + if request.user.is_authenticated and request.user.email.lower() == profile["email"]: + lti_user = create_lti_user(lti_user_id, lti_consumer, profile) else: # Ask the user to login before linking. raise PermissionDenied() from exc + elif lti_consumer.use_lti_pii: + profile["username"] = lti_user_id + lti_user = create_lti_user(lti_user_id, lti_consumer, profile) else: lti_user = create_lti_user(lti_user_id, lti_consumer) @@ -55,11 +72,15 @@ def authenticate_lti_user(request, lti_user_id, lti_consumer): switch_user(request, lti_user, lti_consumer) -def create_lti_user(lti_user_id, lti_consumer, email=None): +def create_lti_user(lti_user_id, lti_consumer, profile=None): """ Generate a new user on the edX platform with a random username and password, and associates that account with the LTI identity. """ + if profile is None: + profile = {} + email = profile.get("email") + edx_user_id = profile.get("username") or generate_random_edx_username() edx_user = User.objects.filter(email=email).first() if email else None if not edx_user: @@ -67,8 +88,7 @@ def create_lti_user(lti_user_id, lti_consumer, email=None): edx_password = str(uuid.uuid4()) while not created: try: - edx_user_id = generate_random_edx_username() - edx_email = f"{edx_user_id}@{settings.LTI_USER_EMAIL_DOMAIN}" + edx_email = email if email else f"{edx_user_id}@{settings.LTI_USER_EMAIL_DOMAIN}" with transaction.atomic(): edx_user = User.objects.create_user( username=edx_user_id, @@ -78,13 +98,13 @@ def create_lti_user(lti_user_id, lti_consumer, email=None): # A profile is required if PREVENT_CONCURRENT_LOGINS flag is set. # TODO: We could populate user information from the LTI launch here, # but it's not necessary for our current uses. - edx_user_profile = UserProfile(user=edx_user) + edx_user_profile = UserProfile(user=edx_user, name=profile.get("full_name", "")) edx_user_profile.save() created = True except IntegrityError: + edx_user_id = generate_random_edx_username() # The random edx_user_id wasn't unique. Since 'created' is still # False, we will retry with a different random ID. - pass lti_user = LtiUser( lti_consumer=lti_consumer, diff --git a/lms/djangoapps/lti_provider/views.py b/lms/djangoapps/lti_provider/views.py index 8515bb06e9..2da12c0960 100644 --- a/lms/djangoapps/lti_provider/views.py +++ b/lms/djangoapps/lti_provider/views.py @@ -35,7 +35,9 @@ REQUIRED_PARAMETERS = [ OPTIONAL_PARAMETERS = [ 'context_title', 'context_label', 'lis_result_sourcedid', - 'lis_outcome_service_url', 'tool_consumer_instance_guid' + 'lis_outcome_service_url', 'tool_consumer_instance_guid', + "lis_person_name_full", "lis_person_name_given", "lis_person_name_family", + "lis_person_contact_email_primary", ]