feat: Add top-level parent logic to Upstream/Dowstream links [FC-0097] #37076

ChrisChV · 2025-07-28T16:38:09Z

Description

Adds the top_level_parent_usage_key to the EntityLinkBase
This field is used to save the top-level parent of a component or container when it is imported into a course. Example: A unit with components imported into a course. The unit is the top-level parent of the components.
Updates the DownstreamListView to return the top-level parents instead of downstream child, if this parent exists.
Each time containers with children were synchronized, a new downstream block was created for each child instead of updating the existing one. This occurred because the upstream_key was incorrectly validated as an Opaquekey against a list of key strings. This was fixed by converting the upstream_key to a string before the verification. (see 34cd5a4 and 2964783)
Which edX user roles will this change impact? "Course Author", "Developer".

Supporting information

Github Issue: Library sync page: Show sections/subsections/units available for sync frontend-app-authoring#2168 (comment)
Used in: feat: Show sections/subsections/units available for sync in library sync page [FC-0097] frontend-app-authoring#2271
Internal ticket: FAL-4231

Testing instructions

Verify that the test covers the new code.
Follow the testing instructions in feat: Show sections/subsections/units available for sync in library sync page [FC-0097] frontend-app-authoring#2271

Deadline

None

Other information

TODO: It's missing some changes in the counts of the summary (see Library sync page: Show sections/subsections/units available for sync frontend-app-authoring#2168 (comment))

…vel parent in an update

openedx-webhooks · 2025-07-28T16:38:13Z

Thanks for the pull request, @ChrisChV!

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.

navinkarkera

@ChrisChV Nice work! This works as expected but I have left some suggestions and a doubt in the tests.

cms/djangoapps/contentstore/models.py

navinkarkera · 2025-07-29T06:16:42Z

cms/djangoapps/contentstore/models.py

+            top_level_keys = result.exclude(id__in=non_top_level_ids).values_list(
+                'top_level_parent_usage_key', flat=True
+            ).distinct()
+
+            top_level_objects = ContainerLink.filter_links(**{
+                "downstream_usage_key__in": top_level_keys
+            })


Same here, top_level_* variables should be the ones that are at the top, but here they are the objects that are under some other container.

Not really. The top_level_parent always represents the highest-level parent at import time, not the direct parent. This code retrieves the top-level parent keys and their objects (containers) and returns them instead of their descendant.

cms/djangoapps/contentstore/models.py

navinkarkera · 2025-07-29T09:37:17Z

cms/djangoapps/contentstore/models.py

+        `use_top_level_parents` is an special filter, replace any result with the top-level parent if exists.
+        Example: We have linkA and linkB with top-level parent as linkC and linkD without top-level parent.
+        After all other filters:
+        Case 1: `use_top_level_parents` is False, the result is [linkA, linkB, linkD]


Suggested change

Case 1: `use_top_level_parents` is False, the result is [linkA, linkB, linkD]

Case 1: `use_top_level_parents` is False, the result is [linkA, linkB, linkC, linkD]

It should list all components no?

Updated: bc484ec

@ChrisChV Just want to confirm whether linkC will not show up if use_top_level_parents is False?

cms/djangoapps/contentstore/models.py

navinkarkera · 2025-07-29T12:56:55Z

cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py

+        )
        assert response.status_code == 200
        data = response.json()
+        self.assertEqual(data["count"], 3)


After making the above suggested change, this increases to 4, which seems correct as we are also making a change to top level subsection in line 1096 no?

Not really. According to the requirements, only the top-level parents should be displayed. Even if all containers and components within a section are updated, only the top-level parent, which is the section, should be displayed. In this test, the top-level parent of the subsection and component is the section. This test checks that only the section is displayed and is not duplicated in the result.

The test fails because the code above is not replacing descendants with their top-level parent, it is adding the subsection to the response.

I added another more simple tests for this: bc484ec#diff-cdab0b43d246b3a18252af57261eefce305c7a0ed93bce4549288127324c5bf3R1085

navinkarkera

@ChrisChV Great work, it is not easy to catch all the edge cases here! 👍

I tested this: (Played around with multiple containers from library in courses)
I read through the code
I checked for accessibility issues
Includes documentation

cms/djangoapps/contentstore/models.py

navinkarkera · 2025-07-30T05:33:24Z

@ChrisChV One more request, could you update the process_list function here to something like this:

if item["last_published_at"] and merged[key]["last_published_at"] and item["last_published_at"] > merged[key]["last_published_at"]:

I was testing #37008 from @rpenido and I think it would be possible to copy unpublished blocks and this line fails in that case.

OR we should not allow users to copy unpublished blocks from library,

ChrisChV · 2025-07-30T16:21:42Z

I was testing #37008 from @rpenido and I think it would be possible to copy unpublished blocks and this line fails in that case.

OR we should not allow users to copy unpublished blocks from library,

It's a good question. I agree with not allowing users to copy unpublished blocks from libraries. @edschema @sdaitzman What do you think?

rpenido · 2025-07-30T17:21:44Z

OR we should not allow users to copy unpublished blocks from library,

For more context, when we copy/paste a component from a library, we are copying the latest draft version and linking it.
When we use the "Library Content" to add a component, we only show and add the published version of the element.

Each time containers with children were synchronized, a new downstream block was created for each child instead of updating the existing one. This occurred because the upstream_key was incorrectly validated as an Opaquekey against a list of key strings. This was fixed by converting the upstream_key to a string before the verification.

…ery sync

edschema · 2025-07-30T20:42:30Z

OR we should not allow users to copy unpublished blocks from library,

For more context, when we copy/paste a component from a library, we are copying the latest draft version and linking it. When we use the "Library Content" to add a component, we only show and add the published version of the element.

Hi @rpenido @ChrisChV and team, thanks for catching this! Users should not be able to reuse unpublished blocks. This is a good case I don't believe we have accounted for. I made an issue to track disabling copy from the context menu for unpublished blocks here: openedx/frontend-app-authoring#2344

If copying a block from a library that has been published but has draft changes, when pasted, that block should show the latest published version in the course. By this I mean:

Block C is published
Block C is edited to Block C', but Block C' is not published
If a user copies Block C' and pastes into their course, the version pasted should be Block C

rpenido · 2025-07-30T22:21:06Z

Thanks @edschema! I left a comment on the new issue.

bradenmacdonald · 2025-07-31T22:08:31Z

Adds the top_level_parent_usage_key to the EntityLinkBase
This field is used to save the top-level parent of a component or container when it is imported into a course. Example: A unit with components imported into a course. The unit is the top-level parent of the components.

I'm curious, could you achieve the same effect with an is_top_level boolean field, or is it necessary to store the full ID of the top level entity? (edit: Oh, I see - we don't really have the hierarchy available when we're querying the entity links table, so without the full ID we can't merge changes from children into the top level parent item. Makes sense.)

Also, when we get to unlinking (openedx/frontend-app-authoring#2164) we'll need to account for recursively updating the top_level_parent_usage_key. I'll add a note to that issue.

ChrisChV · 2025-08-05T23:17:34Z

@bradenmacdonald This and openedx/frontend-app-authoring#2271 are ready for another review

I will do this openedx/frontend-app-authoring#2168 (comment) in a follow-up PR. I would like to merge this PR to unlock other tasks.

bradenmacdonald

Thanks @ChrisChV ! Looking good, just one concern about the name "definition" and then once you've renamed that and @navinkarkera confirms he's tested the final version, I will approve.

ChrisChV · 2025-08-08T22:39:28Z

Thanks @ChrisChV ! Looking good, just one concern about the name "definition" and then once you've renamed that and @navinkarkera confirms he's tested the final version, I will approve.

@navinkarkera It's ready for another review

ChrisChV · 2025-08-11T20:03:39Z

@bradenmacdonald @navinkarkera I need some help here.

These tests are failing because of this change 633e050#diff-5b6284478c70bbd62aaaf2b39e1d5b4b526214ecf8aea0cea9fefc84fdab79e7R277

Adding the delay solves the problem, but it is not what is needed now. It seems that some configuration is missing in the tests, but I can't find the solution.

bradenmacdonald · 2025-08-12T04:16:40Z

@ChrisChV Are you talking about this?

https://github.com/openedx/edx-platform/blob/633e0500ba0d53476f7df714903ce3843d3c2193/cms/djangoapps/contentstore/signals/handlers.py#L273-L277

What I would suggest is that in the child handler, when you go to create the link and find that the parent link doesn't exist yet, then create the parent link synchronously and idempotently (call handle_create_or_update_xblock_upstream_link for the parent from within the child handle_create_or_update_xblock_upstream_link handler). Later, the handler will get called to create the parent link and it will already exist, so it will be a no-op, but that's fine. Would something like that work?

ChrisChV · 2025-08-13T02:31:05Z

@bradenmacdonald It works! Thanks! Those changes are ready for review if you wish to take a look: 6cc840a

Thanks @ChrisChV ! Looking good, just one concern about the name "definition" and then once you've renamed that and @navinkarkera confirms he's tested the final version, I will approve.

@navinkarkera It's ready for a final review

bradenmacdonald · 2025-08-13T18:39:21Z

@ChrisChV Glad to hear! I'll let @navinkarkera do a final check and then you can go ahead and merge. If he's not able to review by tomorrow though let me know.

navinkarkera

@ChrisChV Nice work! Tested this locally with subsections and units.

navinkarkera · 2025-08-14T11:23:35Z

cms/djangoapps/contentstore/models.py

+        `use_top_level_parents` is an special filter, replace any result with the top-level parent if exists.
+        Example: We have linkA and linkB with top-level parent as linkC and linkD without top-level parent.
+        After all other filters:
+        Case 1: `use_top_level_parents` is False, the result is [linkA, linkB, linkD]


@ChrisChV Just want to confirm whether linkC will not show up if use_top_level_parents is False?

ChrisChV · 2025-08-14T16:46:17Z

Just want to confirm whether linkC will not show up if use_top_level_parents is False?

Yes, I updated the comment. Thanks! 5d806b0

edx-pipeline-bot · 2025-08-14T18:52:06Z

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

edx-pipeline-bot · 2025-08-14T19:06:46Z

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

openedx#37076) - Adds the `top_level_parent_usage_key` to the `EntityLinkBase` - This field is used to save the top-level parent of a component or container when it is imported into a course. Example: A unit with components imported into a course. The unit is the top-level parent of the components. - Updates the `DownstreamListView` to return the top-level parents instead of downstream child, if this parent exists. - Each time containers with children were synchronized, a new downstream block was created for each child instead of updating the existing one. This occurred because the `upstream_key` was incorrectly validated as an `Opaquekey` against a list of key strings. This was fixed by converting the `upstream_key` to a string before the verification. (see openedx@34cd5a4 and openedx@2964783) - Which edX user roles will this change impact? "Course Author", "Developer".

kdmccormick · 2025-09-26T15:02:16Z

cms/lib/xblock/upstream_sync.py

+    top_level_downstream_parent_key = Dict(
+        help=(
+            "The block key ('block_type@block_id') of the downstream block that is the top-level parent of "
+            "this block. This is present if the creation of this block is a consequence of "
+            "importing a container that has one or more levels of children. "
+            "This represents the parent (container) in the top level "
+            "at the moment of the import."
+        ),
+        default=None, scope=Scope.settings, hidden=True, enforce_type=True,
+    )


Dicts serialize to OLX in a very hard-to-read way:

<problem top_level_downstream_parent_key="{"type": "html", "id": "my-id"}">...</problem>

This field could instead be a String, joined with a :, like this:

<problem top_level_downstream_parent_key="html:my-id">...</problem>

@ChrisChV @bradenmacdonald Do you think it'd be worth making this change now? After the Ulmo cutoff, we're locked in to the Dict field.

Yes I would much prefer something like that.

@kdmccormick @bradenmacdonald Created a PR: #37448 which converts this field to a string in this format html@my-id.

feat: Add top_level_parent field and updates API to optain the top la…

36cb15c

…vel parent in an update

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

openedx-webhooks added this to Contributions Jul 28, 2025

github-project-automation bot moved this to Needs Triage in Contributions Jul 28, 2025

ChrisChV changed the title ~~feat: Add top_level_parent field and updates the API~~ feat: Add top_level_parent field and updates the API [FC-0097] Jul 28, 2025

ChrisChV marked this pull request as draft July 28, 2025 16:38

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Jul 28, 2025

mphilbrick211 moved this from Needs Triage to Waiting on Author in Contributions Jul 28, 2025

ChrisChV added 2 commits July 28, 2025 14:39

test: Added tests for top-level parents logic

51e3927

style: Fix broken lint

c34bf9d

ChrisChV mentioned this pull request Jul 28, 2025

feat: Show sections/subsections/units available for sync in library sync page [FC-0097] openedx/frontend-app-authoring#2271

Merged

8 tasks

ChrisChV changed the title ~~feat: Add top_level_parent field and updates the API [FC-0097]~~ feat: Add top-level parent logic to Upstream/Dowstream links [FC-0097] Jul 28, 2025

ChrisChV marked this pull request as ready for review July 28, 2025 21:05

navinkarkera requested changes Jul 29, 2025

View reviewed changes

ChrisChV added 2 commits July 29, 2025 13:37

style: Nits on the code

bc484ec

style: fix broken lints

806cb4f

navinkarkera approved these changes Jul 30, 2025

View reviewed changes

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

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

ChrisChV added 4 commits July 30, 2025 12:31

feat: Add _annotate_query_with_ready_to_sync to ContainerLink

966c998

Merge branch 'master' into chris/FAL-4231-top-level-parent

84ac4ba

test: Add tests to verify the fix of the bug of creating blocks on ev…

2964783

…ery sync

edschema mentioned this pull request Jul 30, 2025

Disable copying for unpublished blocks openedx/frontend-app-authoring#2344

Open

style: Fix broken lint

9c4192c

Merge branch 'master' into chris/FAL-4231-top-level-parent

4b6855a

bradenmacdonald reviewed Aug 7, 2025

View reviewed changes

ChrisChV added 2 commits August 8, 2025 14:46

Merge branch 'master' into chris/FAL-4231-top-level-parent

96d1469

refactor: Use BlockType in top_level_downstream_parent_key

c664a7a

style: Fix broken lint

1baf99b

ChrisChV added 2 commits August 12, 2025 20:42

refactor: Verify creation of top-level parents

6cc840a

Merge branch 'master' into chris/FAL-4231-top-level-parent

d51aaee

navinkarkera approved these changes Aug 14, 2025

View reviewed changes

style: Update comment

5d806b0

ChrisChV added 2 commits August 14, 2025 11:57

Merge branch 'master' into chris/FAL-4231-top-level-parent

eca0334

Merge branch 'master' into chris/FAL-4231-top-level-parent

a68379c

ChrisChV enabled auto-merge (squash) August 14, 2025 16:59

ChrisChV merged commit ec72dc7 into openedx:master Aug 14, 2025
48 checks passed

ChrisChV deleted the chris/FAL-4231-top-level-parent branch August 14, 2025 17:36

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

kdmccormick reviewed Sep 26, 2025

View reviewed changes

ChrisChV mentioned this pull request Sep 26, 2025

Bug: top_level_downstream_parent_key is hard to read in OLX openedx/frontend-app-authoring#2483

Closed

kdmccormick mentioned this pull request Oct 7, 2025

Convert UpstreamMixin.top_level_downstream_parent to a String #37445

Closed

navinkarkera mentioned this pull request Oct 8, 2025

refactor!: use String field instead of Dict field to store top_level_downstream_parent_key #37448

Merged

ormsbee mentioned this pull request Nov 7, 2025

fix: bump learning-core to 0.30.0 #37614

Merged

	Case 1: `use_top_level_parents` is False, the result is [linkA, linkB, linkD]
	Case 1: `use_top_level_parents` is False, the result is [linkA, linkB, linkC, linkD]

feat: Add top-level parent logic to Upstream/Dowstream links [FC-0097] #37076

feat: Add top-level parent logic to Upstream/Dowstream links [FC-0097] #37076

Uh oh!

Conversation

ChrisChV commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Deadline

Other information

Uh oh!

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

Uh oh!

Uh oh!

navinkarkera left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navinkarkera left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

ChrisChV commented Jul 30, 2025

Uh oh!

rpenido commented Jul 30, 2025

Uh oh!

edschema commented Jul 30, 2025

Uh oh!

rpenido commented Jul 30, 2025

Uh oh!

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

Uh oh!

Uh oh!

ChrisChV commented Aug 5, 2025

Uh oh!

bradenmacdonald left a comment

Choose a reason for hiding this comment

Uh oh!

ChrisChV commented Aug 8, 2025

Uh oh!

ChrisChV commented Aug 11, 2025

Uh oh!

bradenmacdonald commented Aug 12, 2025

Uh oh!

ChrisChV commented Aug 13, 2025

Uh oh!

bradenmacdonald commented Aug 13, 2025

Uh oh!

navinkarkera left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisChV commented Aug 14, 2025

Uh oh!

Uh oh!

edx-pipeline-bot commented Aug 14, 2025

Uh oh!

edx-pipeline-bot commented Aug 14, 2025

ChrisChV commented Jul 28, 2025 •

edited

Loading

openedx-webhooks commented Jul 28, 2025 •

edited

Loading

navinkarkera commented Jul 30, 2025 •

edited

Loading

bradenmacdonald commented Jul 31, 2025 •

edited

Loading

kdmccormick Sep 26, 2025 •

edited

Loading