feat: Create `LibraryContainerLocator` for Library Containers [FC-0083] #369

ChrisChV · 2025-03-13T20:52:54Z

Description

Refactor LibraryCollectionKey to LibraryItemKey to use it as the general class for LibraryCollectionLocator and LibraryContainerLocator.
Creation of LibraryContainerLocator
Add LibraryUsageLocatorV2 also as LibraryItemKey

Support Information

Github issues:
- What should library container usage keys look like? openedx-learning#282
- Backend: implement opaque key format for structures in libraries frontend-app-authoring#1704
Used in: feat: get_library_unit_usage_key in new content_libraries.units module [FC-0083] edx-platform#36379
Internal ticket: FAL-4108

Testing instructions

Check all the code.
Verify that the test covers the changes.

openedx-webhooks · 2025-03-13T20:52:59Z

Thanks for the pull request, @ChrisChV!

This repository is currently maintained by @openedx/axim-engineering.

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.

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.

codecov · 2025-03-13T21:04:37Z

Codecov Report

Attention: Patch coverage is 97.36842% with 2 lines in your changes missing coverage. Please review.

Project coverage is 93.80%. Comparing base (b958395) to head (ec22e77).
Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
opaque_keys/edx/locator.py	92.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #369      +/-   ##
==========================================
+ Coverage   93.72%   93.80%   +0.07%     
==========================================
  Files          30       31       +1     
  Lines        2934     3004      +70     
  Branches      191      192       +1     
==========================================
+ Hits         2750     2818      +68     
- Misses        157      159       +2     
  Partials       27       27

Flag	Coverage Δ
unittests	`93.80% <97.36%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bradenmacdonald · 2025-03-14T00:47:44Z

Is it possible for LibraryUsageLocatorV2 to also be a LibraryElementkey, or can each locator only be a single type of key? (I guess it's more important that it be a UsageKey if it can only be one).

bradenmacdonald · 2025-03-14T00:48:30Z

CHANGELOG.rst

@@ -1,3 +1,8 @@
+# 2.12.0
+
+* Refactor: Rename LibraryCollectionKey to LibraryElementKey.


Nit: we have been using the term "item" in a few place to refer to any thing in a library (block/collection/container) so I would slightly prefer that over "element"

Updated here fc0cff1

navinkarkera

@ChrisChV Looks good.
👍

I tested this: Verified tests.
I read through the code
I checked for accessibility issues
Includes documentation

navinkarkera · 2025-03-14T04:53:56Z

opaque_keys/edx/locator.py

+    When serialized, these keys look like:
+        lct:org:lib:ct-type:ct-id
+    """
+    CANONICAL_NAMESPACE = 'lct'  # "Library Container"


Question: Do we need this to be short? If size doesn't matter, maybe change this to something readable.

@navinkarkera Yes, other larger possibilities were ruled out; you can see that conversation here: openedx/openedx-learning#282 (comment)

ChrisChV · 2025-03-14T23:32:55Z

Is it possible for LibraryUsageLocatorV2 to also be a LibraryElementkey, or can each locator only be a single type of key?

@bradenmacdonald I've tried this, and it causes conflicts with the KEY_TYPE (it inherits two different values for this property). Apparently, a locator can only have one KEY_TYPE.

ChrisChV · 2025-03-14T23:33:57Z

@bradenmacdonald It's ready for your review

bradenmacdonald

Looks good! I tested this out in openedx/edx-platform#36371 though I haven't tried it with the latest rename from Element -> Item

pomegranited · 2025-03-15T22:24:26Z

Hi @ormsbee or @kdmccormick -- can you help get this merged and a new release created?

pomegranited · 2025-03-15T22:55:47Z

opaque_keys/edx/keys.py

    """
-    KEY_TYPE = 'collection_key'
+    KEY_TYPE = 'library_element_key'


If we're renaming this to LibraryItemKey instead of LibraryElementKey, shouldn't this key type be library_item_key?

You are right. Updated: ec22e77

kdmccormick

Don't we need LibraryContainerLocators to be UsageKeys so that we can store xblock state against units, sections, and subsections?

ChrisChV · 2025-03-18T18:52:28Z

Don't we need LibraryContainerLocators to be UsageKeys so that we can store xblock state against units, sections, and subsections?

Hi @kdmccormick, could you give me more context about this? What is the xblock state that we need to store in the containers?

kdmccormick · 2025-03-18T21:14:37Z

Sure @ChrisChV . Honestly, I actually cannot think of a reason to store state against containers in libraries right now. Still, I think that this would be good future-proofing, and it is fairly harmless if we don't end up needing it. Here are some of the ways I think it could pay off:

It is not clear yet how courses will be represented in Learning Core. One idea is that courses will be stored within libraries. That would mean...
- we would store a student's position as user state against an lct:. key.
- furthermore, we will need to store student state for various selectors (e.g. ItemBank.selected); I'm not sure if that state will get stored against the selector or its container, but if the latter, then we that will be state stored against lct:.
- Many many edx-platform Python APIs and HTTP APIs a expect UsageKeys as a reference to units and sequences. Examples include the CourseBlocks (block transformers) and SequenceMetadata APIs.
We know that we want to move away from the idea that "containers are XBlocks", but we are several years away from that. In the meantime, I believe that the Learning Core runtime will need to be able to return containers as if they are XBlocks, and that is only possible if they have usage keys.

kdmccormick · 2025-03-18T21:15:10Z

@ormsbee ^ I could use a sanity check from you on this, if you have time.

bradenmacdonald · 2025-03-19T05:34:52Z

Jumping in with some quick thoughts on this: I'm not super opposed to making these UsageKeys, but it doesn't make sense to me and I think it's cleaner not to do so.

One idea is that courses will be stored within libraries. That would mean...

If you mean courses will be stored within libraries and learners will learn from those courses within libraries, I haven't heard about that and don't like it in principle. I do see how having a course within a library is arguably a natural extension of having units, subsections, and sections - but learners wouldn't be accessing any of those things within the library generally; only when they're copied into a course. "True" independent courses might be in the same learning package, but I don't think they'd be accessed directly by learners within a library. What's more, courses aren't really "containers" and generally have their own keys course-v1 etc.

furthermore, we will need to store student state for various selectors (e.g. ItemBank.selected);

That's only required for learning in courses. We don't support ItemBank in libraries at all, because it's an XBlock with children, and the runtime doesn't support it. Eventually we will implement ItemBank-like functionality on top of containers (and "selectors") and then it will be allowed in libraries and support student state, but it wouldn't be XBlock student state. It will be a new "container"/"outline" student state, and would work fine with lct: keys that are not UsageKeys.

Many many edx-platform Python APIs and HTTP APIs a expect UsageKeys as a reference to units and sequences. Examples include the CourseBlocks (block transformers) and SequenceMetadata APIs.

None of those are APIs that we use with content libraries, and if they were, then they'd need to be updated to support non-XBlock containers anyways, since that's what we want to move toward. I see this (having lct: as a non-UsageKey) as a forcing function to help us aim toward the new architecture (containers aren't XBlocks).

the Learning Core runtime will need to be able to return containers as if they are XBlocks, and that is only possible if they have usage keys.

I don't think we need that (in libraries)? They aren't implemented as XBlocks, and can't be used as XBlocks in the Library / Learning Core runtime. That runtime doesn't support XBlocks with children so it can't support Units as XBlocks.

ormsbee · 2025-03-19T06:27:35Z

I'll try to write more tomorrow, but my quick take on this is that I don't think this needs to be a UsageKey. Also, I don't want to get in the mindset that we need a parallel set of container UsageKeys because of where the data is being stored. It's true that at some point we're going to be serving container content data out of Learning Core, if nothing else when we move course content to it. But we're going to need some mapping of existing block-v1: style UsageKeys to the corresponding course content in a LearningPackage anyhow, for backwards compatibility. That mapping doesn't even have to be 1:1 (e.g. the CCX use case).

I am fuzzy on exactly how we transition from a world where it's XBlocks all the way down to one where we shove the containers out of the XBlock runtime. The Unit seems like a great place to start because it has no user state for the most part (ignoring custom asides and the fact that technically it has a position field that I'm almost sure is never actually used). I expect that working out how to sequence that transition will be one of the thorniest pieces of this whole affair.

Edit: To be clear, I don't mean that we have to cross this particular bridge to get to courses-on-learning-core. We're going to be in XBlock-shim-land for a while. But this is going to be the next hill of confusion in the codebase. Though I'm hopeful that we can make something that looks like an XBlock for read purposes that hits 95% of the backwards compatibility use cases without things being too crazy. But now I have completely rambled on a tangent....

kdmccormick

OK, that all makes sense. Then let's merge this 🚀

ChrisChV · 2025-03-19T17:12:48Z

OK, that all makes sense. Then let's merge this 🚀

Thanks! 🚀 🚀 @kdmccormick @ormsbee could you help me merge this and create a new release?

kdmccormick · 2025-03-21T16:05:34Z

@ormsbee any last-minute objections?

I still think these might be UsageKeys, but we can retroactively add that base class in the future if it turns out to be true.

ormsbee · 2025-03-21T18:23:51Z

No objections. Agreed that we can add it in the future if we need to.

kdmccormick · 2025-03-21T19:08:27Z

Can you merge @ChrisChV or should I?

rpenido · 2025-03-21T19:43:24Z

Can you merge @ChrisChV or should I?

He doesn't have merge rights on this repo, so it would be great if you could merge it.

kdmccormick · 2025-03-21T20:07:01Z

Released: https://github.com/openedx/opaque-keys/releases/tag/2.12.0

ChrisChV added 2 commits March 13, 2025 15:38

refactor: Convert LibraryCollectionKey to LibraryElementKey

4fda18c

feat: Add LibraryContainerLocator

2f0dae3

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

refactor: Convert LibraryElementKey as abstract class

e1d5a18

chore: Bump version of the package

2419bd7

ChrisChV mentioned this pull request Mar 13, 2025

feat: get_library_unit_usage_key in new content_libraries.units module [FC-0083] openedx/edx-platform#36379

Closed

1 task

bradenmacdonald reviewed Mar 14, 2025

View reviewed changes

bradenmacdonald mentioned this pull request Mar 14, 2025

Basic CRUD REST Endpoints for units in content libraries [FC-0083] openedx/edx-platform#36371

Merged

10 tasks

navinkarkera approved these changes Mar 14, 2025

View reviewed changes

ChrisChV force-pushed the chris/FAL-4108-create-container-keys branch from 265a303 to 63141f7 Compare March 14, 2025 23:21

refactor: Rename LibraryElementKey to LibraryItemKey

fc0cff1

ChrisChV force-pushed the chris/FAL-4108-create-container-keys branch from 63141f7 to fc0cff1 Compare March 14, 2025 23:30

bradenmacdonald approved these changes Mar 14, 2025

View reviewed changes

pomegranited requested review from ormsbee and kdmccormick March 15, 2025 22:24

pomegranited reviewed Mar 15, 2025

View reviewed changes

refactor: Rename library_element_key to library_item_key

ec22e77

ChrisChV force-pushed the chris/FAL-4108-create-container-keys branch from 75ecbe6 to ec22e77 Compare March 17, 2025 16:01

kdmccormick requested changes Mar 17, 2025

View reviewed changes

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Mar 17, 2025

kdmccormick mentioned this pull request Mar 17, 2025

What should library container usage keys look like? openedx/openedx-learning#282

Closed

pomegranited mentioned this pull request Mar 17, 2025

feat: adds events related to content library containers [FC-0083] openedx/openedx-events#479

Merged

15 tasks

kdmccormick approved these changes Mar 19, 2025

View reviewed changes

pomegranited mentioned this pull request Mar 21, 2025

Epic 12: Units can be created and edited in Libraries openedx/frontend-app-authoring#1594

Open

kdmccormick merged commit 519721a into openedx:master Mar 21, 2025
13 checks passed

rpenido deleted the chris/FAL-4108-create-container-keys branch March 21, 2025 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Create `LibraryContainerLocator` for Library Containers [FC-0083] #369

feat: Create `LibraryContainerLocator` for Library Containers [FC-0083] #369

ChrisChV commented Mar 13, 2025 •

edited

Loading

openedx-webhooks commented Mar 13, 2025

codecov bot commented Mar 13, 2025 •

edited

Loading

bradenmacdonald commented Mar 14, 2025

bradenmacdonald Mar 14, 2025

ChrisChV Mar 14, 2025

navinkarkera left a comment

navinkarkera Mar 14, 2025

ChrisChV Mar 14, 2025

ChrisChV commented Mar 14, 2025

ChrisChV commented Mar 14, 2025

bradenmacdonald left a comment

pomegranited commented Mar 15, 2025

pomegranited Mar 15, 2025

ChrisChV Mar 17, 2025 •

edited

Loading

kdmccormick left a comment

ChrisChV commented Mar 18, 2025

kdmccormick commented Mar 18, 2025

kdmccormick commented Mar 18, 2025

bradenmacdonald commented Mar 19, 2025

ormsbee commented Mar 19, 2025 •

edited

Loading

kdmccormick left a comment

ChrisChV commented Mar 19, 2025

kdmccormick commented Mar 21, 2025

ormsbee commented Mar 21, 2025

kdmccormick commented Mar 21, 2025

rpenido commented Mar 21, 2025

kdmccormick commented Mar 21, 2025

		@@ -1,3 +1,8 @@
		# 2.12.0

		* Refactor: Rename LibraryCollectionKey to LibraryElementKey.

feat: Create LibraryContainerLocator for Library Containers [FC-0083] #369

feat: Create LibraryContainerLocator for Library Containers [FC-0083] #369

Conversation

ChrisChV commented Mar 13, 2025 • edited Loading

Description

Support Information

Testing instructions

openedx-webhooks commented Mar 13, 2025

codecov bot commented Mar 13, 2025 • edited Loading

Codecov Report

bradenmacdonald commented Mar 14, 2025

bradenmacdonald Mar 14, 2025

Choose a reason for hiding this comment

ChrisChV Mar 14, 2025

Choose a reason for hiding this comment

navinkarkera left a comment

Choose a reason for hiding this comment

navinkarkera Mar 14, 2025

Choose a reason for hiding this comment

ChrisChV Mar 14, 2025

Choose a reason for hiding this comment

ChrisChV commented Mar 14, 2025

ChrisChV commented Mar 14, 2025

bradenmacdonald left a comment

Choose a reason for hiding this comment

pomegranited commented Mar 15, 2025

pomegranited Mar 15, 2025

Choose a reason for hiding this comment

ChrisChV Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

kdmccormick left a comment

Choose a reason for hiding this comment

ChrisChV commented Mar 18, 2025

kdmccormick commented Mar 18, 2025

kdmccormick commented Mar 18, 2025

bradenmacdonald commented Mar 19, 2025

ormsbee commented Mar 19, 2025 • edited Loading

kdmccormick left a comment

Choose a reason for hiding this comment

ChrisChV commented Mar 19, 2025

kdmccormick commented Mar 21, 2025

ormsbee commented Mar 21, 2025

kdmccormick commented Mar 21, 2025

rpenido commented Mar 21, 2025

kdmccormick commented Mar 21, 2025

feat: Create `LibraryContainerLocator` for Library Containers [FC-0083] #369

feat: Create `LibraryContainerLocator` for Library Containers [FC-0083] #369

ChrisChV commented Mar 13, 2025 •

edited

Loading

codecov bot commented Mar 13, 2025 •

edited

Loading

ChrisChV Mar 17, 2025 •

edited

Loading

ormsbee commented Mar 19, 2025 •

edited

Loading