feat: collections support for containers [FC-0083] by rpenido · Pull Request #36504 · openedx/openedx-platform

rpenido · 2025-04-08T23:39:38Z

Description

This PR adds support for adding Containers to Collections.

ToDo:

Update openedx-events once feat: add collections support for containers [FC-0083] openedx-learning#299 is merged

Supporting information

Implements part of Tags+Collections can be added to Units frontend-app-authoring#1648
Depends on feat: add collections support for containers [FC-0083] openedx-learning#299

Testing instructions

Check feat: add collections support for containers [FC-0083] frontend-app-authoring#1797

Private ref: FAL-4111

openedx-webhooks · 2025-04-08T23:39:42Z

Thanks for the pull request, @rpenido!

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.

openedx/core/djangoapps/content_libraries/signal_handlers.py

pomegranited

@rpenido This is working great, with the exception of when I "undo delete" (aka restore) a Unit that is part of a Collection.

Have left a bunch of nits, but I love the refactoring you did! Makes things clearer, and you're right, better to do now than later.

Would like to review again once my comments are addressed, but this is looking great.

openedx/core/djangoapps/content/search/api.py

pomegranited · 2025-04-11T05:44:31Z

openedx/core/djangoapps/content/search/documents.py

+        else:
+            return result


nit: I'd either remove this else entirely (and let control just fall through), or raise an error about an unexpected OpaqueKey type here?

Suggested change

else:

return result

You're right! Fixed here: 20c307b

openedx/core/djangoapps/content/search/documents.py

pomegranited · 2025-04-11T05:51:27Z

openedx/core/djangoapps/content/search/api.py

+    library_key: LibraryLocatorV2,
+    collection_key: str,


#36476 started using a single LibraryCollectionLocator key wherever we could, instead of passing the library_key with the collection_key. Can you do the same thing here?

openedx/core/djangoapps/content_libraries/api/collections.py

openedx/core/djangoapps/content_libraries/rest_api/blocks.py

openedx/core/djangoapps/content_libraries/api/collections.py

pomegranited · 2025-04-11T06:19:02Z

openedx/core/djangoapps/content_libraries/rest_api/serializers.py

+    def to_internal_value(self, value: str) -> OpaqueKey:
+        """
+        Returns a UsageKeyV2 or a LibraryContainerLocator from the string value.
+
+        Raises ValidationError if invalid UsageKeyV2 or LibraryContainerLocator.
+        """
+        try:
+            return UsageKeyV2.from_string(value)
+        except InvalidKeyError:
+            try:
+                return LibraryContainerLocator.from_string(value)
+            except InvalidKeyError as err:
+                raise ValidationError from err


If we genuinely don't care what kind of OpaqueKey we deserialize here, this can be simpler:

Suggested change

def to_internal_value(self, value: str) -> OpaqueKey:

"""

Returns a UsageKeyV2 or a LibraryContainerLocator from the string value.

Raises ValidationError if invalid UsageKeyV2 or LibraryContainerLocator.

"""

try:

return UsageKeyV2.from_string(value)

except InvalidKeyError:

try:

return LibraryContainerLocator.from_string(value)

except InvalidKeyError as err:

raise ValidationError from err

def to_internal_value(self, value: str) -> OpaqueKey:

"""

Returns an OpaqueKey from the string value.

Raises ValidationError if invalid OpaqueKey.

"""

try:

return OpaqueKey.from_string(value)

except InvalidKeyError as err:

raise ValidationError from err

We pass it through functions as OpaqueKeys, but we have some checks on the keys to check its type:
https://github.com/open-craft/edx-platform/blob/c712133864d3069cc160a6e2f41d04101fc60e30/openedx/core/djangoapps/content_libraries/api/collections.py#L132-L159

That's cool -- then we probably don't need to duplicate these checks in the serializer too, right?

openedx/core/djangoapps/content_libraries/signal_handlers.py

pomegranited · 2025-04-11T06:38:04Z

openedx/core/djangoapps/content/search/tasks.py


+@shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError))
+@set_code_owner_attribute
+def update_library_containers_collections(library_key_str: str, collection_key: str) -> None:


#36476 started using a single LibraryCollectionLocator key wherever we could, instead of passing the library_key with the collection_key. Can you do the same thing here?

Co-authored-by: Jillian <jill@opencraft.com>

pomegranited

👍 Working perfectly thank you @rpenido !

I tested this on my tutor dev stack by adding and removing units to/from collections, deleting a unit that's in a collection, and restoring that unit to ensure the collection link is also restored.
I read through the code
I checked for accessibility issues by using my keyboard to navigate
Includes documentation -- code comments
~~User-facing strings are extracted for translation~~ N/A

navinkarkera

@rpenido Nice work! 👍

I tested this: (Used frontend to add/remove/update container collections)
I read through the code
I checked for accessibility issues
Includes documentation

edx-pipeline-bot · 2025-04-15T20:01:32Z

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

edx-pipeline-bot · 2025-04-15T20:24:59Z

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

Adds support for adding Containers to Collections.

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

openedx-webhooks added this to Contributions Apr 8, 2025

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

rpenido force-pushed the rpenido/fal-4111/units-collections branch 3 times, most recently from 111684c to 3a7ee98 Compare April 9, 2025 00:37

rpenido changed the title ~~feat: container collections support~~ feat: container collections support [FC-0083] Apr 9, 2025

rpenido force-pushed the rpenido/fal-4111/units-collections branch from 3a7ee98 to b0854a1 Compare April 9, 2025 11:29

rpenido closed this Apr 9, 2025

github-project-automation bot moved this from Needs Triage to Done in Contributions Apr 9, 2025

rpenido reopened this Apr 9, 2025

rpenido closed this Apr 9, 2025

rpenido reopened this Apr 9, 2025

rpenido force-pushed the rpenido/fal-4111/units-collections branch 7 times, most recently from 0f63f14 to 88fdc7f Compare April 9, 2025 22:45

rpenido commented Apr 10, 2025

View reviewed changes

openedx/core/djangoapps/content_libraries/signal_handlers.py Outdated Show resolved Hide resolved

rpenido force-pushed the rpenido/fal-4111/units-collections branch 5 times, most recently from c372da4 to 8e80240 Compare April 10, 2025 15:26

rpenido changed the title ~~feat: container collections support [FC-0083]~~ feat: collections support for containers [FC-0083] Apr 10, 2025

rpenido force-pushed the rpenido/fal-4111/units-collections branch 2 times, most recently from fa6bc82 to ffcc45d Compare April 10, 2025 15:58

rpenido force-pushed the rpenido/fal-4111/units-collections branch 2 times, most recently from 353d2fd to 8255a8d Compare April 10, 2025 23:18

feat: collections support for containers

3ad01b2

rpenido force-pushed the rpenido/fal-4111/units-collections branch from 8255a8d to 3ad01b2 Compare April 10, 2025 23:41

rpenido marked this pull request as ready for review April 10, 2025 23:48

rpenido mentioned this pull request Apr 10, 2025

feat: add collections support for containers [FC-0083] openedx/frontend-app-authoring#1797

Merged

pomegranited reviewed Apr 11, 2025

View reviewed changes

rpenido and others added 5 commits April 11, 2025 20:56

fix: apply suggestions from code review

057cb37

Co-authored-by: Jillian <jill@opencraft.com>

fix: remove early return

20c307b

Merge branch 'master' into rpenido/fal-4111/units-collections

9aa8c37

fix: openedx-learning api change

43fb533

fix: missing import and lint

881292f

rpenido force-pushed the rpenido/fal-4111/units-collections branch from c712133 to 881292f Compare April 12, 2025 00:42

rpenido added 2 commits April 11, 2025 22:26

fix: library collection key

e8fd27d

fix: container restore search index update

8d92de4

rpenido marked this pull request as draft April 12, 2025 04:48

rpenido requested a review from pomegranited April 12, 2025 04:49

pomegranited approved these changes Apr 15, 2025

View reviewed changes

rpenido added 2 commits April 15, 2025 09:19

chore: update openedx-learning version

68204ba

Merge branch 'master' into rpenido/fal-4111/units-collections

ee9347f

navinkarkera approved these changes Apr 15, 2025

View reviewed changes

rpenido marked this pull request as ready for review April 15, 2025 15:34

ChrisChV merged commit 1047ed4 into openedx:master Apr 15, 2025
49 checks passed

ChrisChV deleted the rpenido/fal-4111/units-collections branch April 15, 2025 18:12

ormsbee mentioned this pull request Apr 23, 2025

Remove py2neo dependency #36586

Merged

tonybusa pushed a commit to tonybusa/edx-platform that referenced this pull request Apr 23, 2025

feat: collections support for containers [FC-0083] (openedx#36504)

256d89b

Adds support for adding Containers to Collections.

bradenmacdonald mentioned this pull request Apr 30, 2025

feat: store content.child_usage_keys in Container search document [FC-0083] #36528

Merged

UsamaSadiq pushed a commit that referenced this pull request May 14, 2025

feat: collections support for containers [FC-0083] (#36504)

2ec615d

Adds support for adding Containers to Collections.

Conversation

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

Uh oh!

Description

ToDo:

Supporting information

Testing instructions

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpenido Apr 12, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

navinkarkera left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

edx-pipeline-bot commented Apr 15, 2025

Uh oh!

edx-pipeline-bot commented Apr 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rpenido commented Apr 8, 2025 •

edited

Loading

openedx-webhooks commented Apr 8, 2025 •

edited

Loading

rpenido Apr 12, 2025 •

edited

Loading