Support for syncing units from libraries to courses #36553

navinkarkera · 2025-04-19T17:00:48Z

Description

Sync unit containers from library to courses.

Related to: Backend: API to link a unit into a course frontend-app-authoring#1711
Private-ref: FAL-4077

Test instructions:

Run the new integration tests, which should cover most things:

DJANGO_SETTINGS_MODULE=cms.envs.tutor.test pytest cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py

Manual Test instructions:

Use curl or any other tool to post request, NOTE: below example does not contain csrftoken, studio_session_id etc. which you can add. TIP: use developer console to copy any request as curl request in browser.

curl 'http://studio.local.openedx.io:8001/xblock/' \
  -H 'Accept: application/json, text/plain, */*' \
  -H 'Accept-Language: en-US,en;q=0.6' \
  -H 'Connection: keep-alive' \
  -H 'Content-Type: application/json' \
  --data-raw '{"category":"vertical","parent_locator":"block-v1:OpenedX+DemoX+DemoCourse+type@sequential+block@727232bd02b54af08953c66eb2246d00","library_content_key":"lct:SampleTaxonomyOrg1:lib1:unit:images-57b324"}' \
  --insecure

openedx-webhooks · 2025-04-19T17:00:53Z

Thanks for the pull request, @navinkarkera!

This repository is currently maintained by @openedx/wg-maintenance-edx-platform.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

cms/lib/xblock/container_upstream_sync.py

openedx/core/djangoapps/xblock/api.py

cms/djangoapps/contentstore/models.py

This reverts commit 72ba6ef.

…r key" This reverts commit e8a8d58.

ormsbee · 2025-04-24T17:05:28Z

cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py

+            notices = []
+            # Store final children keys to update order of components in unit
+            children = []
+            for i in range(len(upstream_children)):


Nit for later: we can use enumerate() here.

ormsbee · 2025-04-24T17:07:34Z

cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py

+    if isinstance(upstream_key, LibraryUsageLocatorV2):
+        lib_block = sync_from_upstream_block(downstream=downstream, user=request.user)
+        static_file_notices = import_static_assets_for_library_sync(downstream, lib_block, request)
+        store.update_item(downstream, request.user.id)
+    else:


Nit for later: Probably worth pulling the container sync into a different function and having this one call that.

I agree. Also, I want to move all the sync stuff into one upstream_sync app instead of having some key bits of the code randomly in xblock_storage_handlers (?) and the overloaded /xblock/ REST API endpoint.

ormsbee · 2025-04-24T17:12:18Z

cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py

+                    # This downstream block was added, or deleted from upstream block.
+                    store.delete_item(child.usage_key, user_id=request.user.id)


Clarification: This is the part that (for the MVP) will delete any local additions to a Unit when the next upstream sync happens, right?

Yes, this deletes two different things:

local additions in the course - will get deleted on next sync

blocks deleted upstream

ormsbee · 2025-04-24T17:17:11Z

cms/lib/xblock/upstream_sync_block.py

+    """
+    syncable_fields = _get_synchronizable_fields(upstream, downstream)
+    customizable_fields = set(downstream.get_customizable_fields().keys())
+    isVideoBlock = downstream.usage_key.block_type == "video"


Nit: I'm surprised our linting didn't complain about camel-cased var here.

Fixed. But yeah, that's suspicious....

when viewing a unit in Studio

bradenmacdonald · 2025-04-24T17:38:44Z

cms/lib/xblock/upstream_sync.py

+            # Note: if we expect that an upstream may not be set at all (i.e. we're just inspecting a random
+            # unit that may be a regular course unit), we don't want to log this, so set log_error=False then.
+            if log_error:


Simply viewing a regular unit in Studio (even before this PR) was resulting in a ton of these warnings:

upstream_sync.py:133 - Tried to inspect an unsupported, broken, or missing downstream->upstream link: 'block-v1:BradenX+CP200+23+type@vertical+block@9d1d7e5c0b4a42958f7e4174879ec1e2'->'None' ... cms.lib.xblock.upstream_sync.NoUpstream: Content is not linked to a Content Library.

So I updated the code to suppress the logging in general. Most things don't have an upstream so it's expected.

ormsbee

I have a lot of nits for follow-ups, but the only blocking concern I have in the short term is the part in the models.py file where it looks like we're always saying that there are changes:

https://github.com/openedx/edx-platform/blob/4a69f30e229ad4ff44126bdada5a31ba3736fd19/cms/djangoapps/contentstore/models.py#L247-L256

@bradenmacdonald: If you've determined that this still gives us the correct behavior, then I'd really like to see some comment explaining what's going on. It's either a bug or it's really confusing in its current state.

Because I misunderstood the comment.

ormsbee

I have a number of nits for follow-up, but nothing blocking.

bradenmacdonald · 2025-04-24T18:20:07Z

@ormsbee

If you've determined that this still gives us the correct behavior, then I'd really like to see some comment explaining what's going on. It's either a bug or it's really confusing in its current state.

I didn't really get into the ComponentLink / link tracking personally while working on this PR. It's kind of an ancillary index just used to quickly retrieve all the library content in the course, but doesn't directly affect the core upstream sync system.

But what I believe is going on here is this function is ultimately only ever called during XBLOCK_CREATED/XBLOCK_UPDATED signal handlers (where downstream has changed). So there are always changes of some sort when this is called.

The code is very confusing but I think what it's trying to say is this:

Currently, whenever you modify a downstream XBlock, it sets the ComponentLink "last modified at" time to the time the XBlock was modified, even if you just modified a random field like max_attempts which is excluded from syncing.
Ideally, in the future, whenever you modify a downstream XBlock, the code will change the ComponentLink "last modified at" time only if something relevant to the upstream sync was modified, like the upstream source was changed, or the sync was dismissed (skip the update for this particular version), or ????????? .

bradenmacdonald · 2025-04-24T18:30:22Z

Thanks everyone who collaborated on this PR, @navinkarkera @pomegranited @ormsbee !

@navinkarkera can you please work on a follow-up PR(s) for some of the cleanups that @ormsbee has identified?

edx-pipeline-bot · 2025-04-24T20:44:19Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

navinkarkera · 2025-04-25T05:49:13Z

@ormsbee

If you've determined that this still gives us the correct behavior, then I'd really like to see some comment explaining what's going on. It's either a bug or it's really confusing in its current state.

I didn't really get into the ComponentLink / link tracking personally while working on this PR. It's kind of an ancillary index just used to quickly retrieve all the library content in the course, but doesn't directly affect the core upstream sync system.

But what I believe is going on here is this function is ultimately only ever called during XBLOCK_CREATED/XBLOCK_UPDATED signal handlers (where downstream has changed). So there are always changes of some sort when this is called.

The code is very confusing but I think what it's trying to say is this:
1. Currently, whenever you modify a downstream XBlock, it sets the `ComponentLink` "last modified at" time to the time the XBlock was modified, even if you just modified a random field like `max_attempts` which is excluded from syncing.

2. Ideally, in the future, whenever you modify a downstream XBlock, the code will change the `ComponentLink` "last modified at" time _only if_ something relevant to the upstream sync was modified, like the `upstream` source was changed, or the sync was dismissed (skip the update for this particular version), or ????????? .

@bradenmacdonald @ormsbee Sorry for this very confusing piece of code. You guessed it right @bradenmacdonald. At first, has_changes was set to False before the loop: 711d6aa#diff-47a27860758e70cabb21ef00cb29fb90efa277773d45546adb679ba05587f3bfR178-R185

But while working on the frontend, I came across the requirement of sorting the links based on recently modified downstream blocks. XBlocks do not store modified dates so I updated this part to update the links modified time any time downstream block is updated and use it as base for sorting the links in frontend.

navinkarkera · 2025-04-25T06:33:12Z

@bradenmacdonald @ormsbee Created #36599 to address comments.

ormsbee · 2025-04-25T14:01:06Z

XBlocks do not store modified dates

If you need this from the modulestore, can you call block.edited_on from the EditInfoMixin?

https://github.com/openedx/edx-platform/blob/e5f7a027e4ff1695454664923128ff4f4426ad91/xmodule/modulestore/edit_info.py#L22-L27

edx-pipeline-bot · 2025-04-25T14:09:04Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2025-04-25T14:09:04Z

2U Release Notice: This PR has been deployed to the edX production environment.

navinkarkera · 2025-04-25T14:30:17Z

@ormsbee Nice! That resolves it: ac708ab

navinkarkera · 2025-04-25T14:59:14Z

Created openedx/frontend-app-authoring#1865 to compliment above change in frontend.

* feat: library unit sync * feat: create component link only for component xblocks * feat: container link model * feat: update downstream api views * feat: delete extra components in container on sync (not working) * fix: duplicate definitions of LibraryXBlockMetadata * test: add a new integration test suite for syncing * feat: partially implement container+child syncing * fix: blockserializer wasn't always serializing all HTML block fields * feat: handle reorder, addition and deletion of components in sync Updates children components of unit in course based on upstream unit, deletes removed component, adds new ones and updates order as per upstream. * feat: return unit upstreamInfo and disallow edits to units in courses that are sourced from a library (#773) * feat: Add upstream_info to unit * feat: disallow edits to units in courses that are sourced from a library (#774) --------- Co-authored-by: Jillian Vogel <jill@opencraft.com> Co-authored-by: Rômulo Penido <romulo.penido@gmail.com> * docs: capitalization of XBlock Co-authored-by: David Ormsbee <dave@axim.org> * refactor: (minor) change python property name to reflect type better * fix: lots of "Tried to inspect a missing...upstream link" warnings when viewing a unit in Studio * docs: mention potential REST API for future refactor * fix: check if upstream actually exists before making unit read-only * chore: fix camel-case var * fix: test failure when mocked XBlock doesn't have UpstreamSyncMixin --------- Co-authored-by: Braden MacDonald <braden@opencraft.com> Co-authored-by: Chris Chávez <xnpiochv@gmail.com> Co-authored-by: Jillian Vogel <jill@opencraft.com> Co-authored-by: Rômulo Penido <romulo.penido@gmail.com> Co-authored-by: Braden MacDonald <mail@bradenm.com> Co-authored-by: David Ormsbee <dave@axim.org>

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Apr 19, 2025

openedx-webhooks added this to Contributions Apr 19, 2025

github-project-automation bot moved this to Needs Triage in Contributions Apr 19, 2025

navinkarkera force-pushed the navin/fal-4077/link-unit branch from 5cc709d to 818d6c8 Compare April 20, 2025 13:02

navinkarkera mentioned this pull request Apr 20, 2025

Backend: API to link a unit into a course openedx/frontend-app-authoring#1711

Closed

navinkarkera marked this pull request as draft April 21, 2025 15:04

ChrisChV mentioned this pull request Apr 21, 2025

feat: Add unit from library in course [FC-0083] openedx/frontend-app-authoring#1829

Merged

navinkarkera force-pushed the navin/fal-4077/link-unit branch from a2cfb06 to 647d733 Compare April 22, 2025 14:52

bradenmacdonald reviewed Apr 22, 2025

View reviewed changes

cms/lib/xblock/container_upstream_sync.py Outdated Show resolved Hide resolved

bradenmacdonald reviewed Apr 22, 2025

View reviewed changes

openedx/core/djangoapps/xblock/api.py Outdated Show resolved Hide resolved

ormsbee reviewed Apr 22, 2025

View reviewed changes

cms/djangoapps/contentstore/models.py Outdated Show resolved Hide resolved

kdmccormick mentioned this pull request Apr 23, 2025

feat: context key property for new container locator keys openedx/opaque-keys#375

Closed

navinkarkera force-pushed the navin/fal-4077/link-unit branch 6 times, most recently from 5ffcfb2 to d9e6e16 Compare April 23, 2025 11:46

navinkarkera added 11 commits April 23, 2025 19:50

feat: add support for lct in library_context

03bc65b

feat: library unit sync

157f690

fixup! feat: library unit sync

04adc26

fixup! feat: library unit sync

ce0686e

fixup! feat: library unit sync

5549ee4

feat: unit sync using emulated library container usage locator key

b87bc7b

Revert "feat: add support for lct in library_context"

87762b5

This reverts commit 72ba6ef.

Revert "feat: unit sync using emulated library container usage locato…

fcfc0ee

…r key" This reverts commit e8a8d58.

feat: separate managers for component and container upstream sync

8f89cd6

fixup! feat: separate managers for component and container upstream sync

b3c0fc3

feat: remove upstream links from unit child components on sync

95395cd

ormsbee reviewed Apr 24, 2025

View reviewed changes

bradenmacdonald added 3 commits April 24, 2025 10:33

fix: lots of "Tried to inspect a missing...upstream link" warnings

fbbae0b

when viewing a unit in Studio

docs: mention potential REST API for future refactor

7d301a6

fix: check if upstream actually exists before making unit read-only

ea13143

bradenmacdonald reviewed Apr 24, 2025

View reviewed changes

bradenmacdonald added 2 commits April 24, 2025 10:43

chore: fix camel-case var

3f14cbf

fix: test failure when mocked XBlock doesn't have UpstreamSyncMixin

affff41

ormsbee previously requested changes Apr 24, 2025

View reviewed changes

ormsbee approved these changes Apr 24, 2025

View reviewed changes

bradenmacdonald merged commit 1cd73d1 into openedx:master Apr 24, 2025
49 checks passed

github-project-automation bot moved this from Waiting on Author to Done in Contributions Apr 24, 2025

bradenmacdonald deleted the navin/fal-4077/link-unit branch April 24, 2025 18:41

navinkarkera mentioned this pull request Apr 25, 2025

refactor: update sync model helper function docs and minor cleanup #36599

Merged

bradenmacdonald mentioned this pull request May 20, 2025

Double Edit Buttons on Unit Course Authoring Page openedx/frontend-app-authoring#1986

Closed

		# This downstream block was added, or deleted from upstream block.
		store.delete_item(child.usage_key, user_id=request.user.id)

Support for syncing units from libraries to courses #36553

Support for syncing units from libraries to courses #36553

Uh oh!

Conversation

navinkarkera commented Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Test instructions:

Manual Test instructions:

Uh oh!

openedx-webhooks commented Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ormsbee Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

ormsbee Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ormsbee Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

ormsbee Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

ormsbee left a comment

Choose a reason for hiding this comment

Uh oh!

ormsbee left a comment

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bradenmacdonald commented Apr 24, 2025

Uh oh!

Uh oh!

edx-pipeline-bot commented Apr 24, 2025

Uh oh!

navinkarkera commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

navinkarkera commented Apr 25, 2025

Uh oh!

ormsbee commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edx-pipeline-bot commented Apr 25, 2025

Uh oh!

edx-pipeline-bot commented Apr 25, 2025

Uh oh!

navinkarkera commented Apr 25, 2025

Uh oh!

navinkarkera commented Apr 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

navinkarkera commented Apr 19, 2025 •

edited

Loading

openedx-webhooks commented Apr 19, 2025 •

edited

Loading

bradenmacdonald Apr 24, 2025 •

edited

Loading

bradenmacdonald commented Apr 24, 2025 •

edited

Loading

navinkarkera commented Apr 25, 2025 •

edited

Loading

ormsbee commented Apr 25, 2025 •

edited

Loading