Associate PublishableEntities with Collections [FC-0062] by pomegranited · Pull Request #216 · openedx/openedx-core

pomegranited · 2024-08-21T07:40:31Z

Description

Adds models and APIs for linking objects (PublishableEntities) to Collections, and for fetching the Collections associated with a given entity.

Supporting information

Part of : openedx/modular-learning#227
Based on : #131

Private-ref: FAL-3784

Testing instructions

See open-craft/openedx-platform#674

openedx-webhooks · 2024-08-21T07:40:35Z

Thanks for the pull request, @pomegranited!

What's next?

Please work through the following steps to get your changes 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.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @axim-engineering. Tag them in a comment and let them know that your changes are ready for review.

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.

@ormsbee

copied and modified slightly from @ormsbee's #131

to associate PublishableEntities with Collections. Collection.modified timestamp is updated whenever its contents are changed.

bradenmacdonald · 2024-08-28T18:46:04Z

@ormsbee Can you take a quick look at this when you get some time? In particular, if possible, I want to make sure that this is sufficiently general that we can link units/sections/subsections to collections with the same API, even if they're not implemented as XBlocks/Components. Is PublishableEntity the right "thing" to use here then?

ormsbee · 2024-08-28T19:09:29Z

@bradenmacdonald: I'll do a full review now this afternoon, but yes, PublishableEntity is the correct generic thing to join against to get all the different publishable things.

ormsbee

Some questions and requests. Most of these are minor changes to match openedx_learning package coding conventions that I have failed to document anywhere, and there is no reasonable way for you to have been aware of them. 😛 Sorry about that, and thank you for your patience.

ormsbee · 2024-08-28T20:36:35Z

+        added = add_to_collections(
+            Collection.objects.filter(id=collection.id),
+            contents_qset,
+        )
+        if added:
+            collection.refresh_from_db()  # fetch updated modified date


[nitpick, optional]: It's a little weird to me that this is going to cause multiple writes to the same Collection with just slightly differing datetimes, but in the same transaction. I can't quite place my finger on why this bothers me, except some vague sense that something tracking the Collection lifecycle might get confused.

Instead of invoking add_to_collections and doing the refresh to get around the timestamp change, can we instead copy some of logic from there that does the adding of the records?

So something like:

records_to_add = [ CollectionPublishableEntity( collection_id=collection.id, entity_id=entity_id, ) for entity_id in entities_qset.values_list("pk", flat=True) ] CollectionPublishableEntity.bulk_add(records_to_add)

@ormsbee I don't have a use case for creating a collection with components in it, I was just copying that code from your PR. Maybe it's better just to keep them as separate API calls, to avoid this duplication, and the need for a transaction?

I'm fine either way. I think that use case will come up when we do v1 library import functionality, but I'm fine with punting until then.

I ended up removing the entities_qset param from create_collection, we can add it back later if needed.

rpenido

Hi @pomegranited!

Thank you for your work here! I just left some comments.

rpenido · 2024-08-28T18:29:19Z

+If you're trying to do either of these things, you probably want a new model or
+app. For more details, read on.


This adds a lot of context!! Thank you! ❤️

Oh definitely.. @ormsbee 's notes are always worth preserving :)

Addresses PR review. * CollectionObject -> CollectionPublishableEntity, with added created_by and created fields * contents -> entities, contents_qset -> entities_qset * get_object_collections -> get_entity_collections, with added learning_package_id argument

pomegranited

Thank you for your reviews, @ormsbee and @rpenido :) I addressed everything I could, but there's a couple open questions, if you could take another look?

We ended up not needing a "get all collections" function; we always fetch them for a given learning package. So we merged them into a single api.get_collections method that takes a `learning_package_id` + optional `enabled` (defaults to True).

rpenido

Thank you for the changes, @pomegranited!
LGTM 👍

API: * create_collection -- removed entities_qset param, added enabled param * add/remove entities from a single collection, not a qset * raise ValidationError if adding entities to a collection with mismatching learning packages * add/remove returns updated collection object (not count) Tests: * use authoring API * simplified data used by test classes

pomegranited · 2024-09-02T03:25:25Z

Thanks for your review @rpenido :) How's this looking now, @ormsbee ?

ormsbee

Optional nitpick and some speculation, but nothing that would block merging. Thank you!

(Also, this is a three-day weekend in the U.S., so I may be a bit slow on reviews.)

ormsbee · 2024-09-02T03:31:58Z

+    # Disallow adding entities outside the collection's learning package
+    for entity in entities_qset.all():
+        if entity.learning_package_id != learning_package_id:


[Nit (optional)]: You can also push this to the database layer with something like:

invalid_entity = entities_qset.filter(~Q(learning_package_id=learning_package_id)).first() if invalid_entity: # ... validation error with specifics

That's much cleaner, thank you, done with e03daaa

ormsbee · 2024-09-02T04:13:58Z

+    return collection
+
+
+def get_entity_collections(learning_package_id: int, entity_key: str) -> QuerySet[Collection]:


[speculation (please do not block PR for this)]: It might be nice if we could a custom Manager on the through-model so that if someone has an entity, they could do something like entity.collections.active().

Oh absolutely -- I am interested to see the other use cases for Collections besides what we're supporting here. But will leave that for a day when it's needed, thank you :)

being added to a collection

openedx-webhooks · 2024-09-03T00:31:11Z

@pomegranited 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

As get_all_collections api was refactored [1] to only support fetching collections by learning_package, update reindex function to get collections by learning_package. 1. openedx/openedx-core#216

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Aug 21, 2024

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Aug 22, 2024

pomegranited force-pushed the jill/collection-components branch 2 times, most recently from 341efc4 to 56c0a99 Compare August 23, 2024 07:51

pomegranited commented Aug 23, 2024

View reviewed changes

Comment thread openedx_learning/apps/authoring/collections/api.py Outdated

pomegranited force-pushed the jill/collection-components branch from db902ab to dfbb40f Compare August 27, 2024 05:59

pomegranited mentioned this pull request Aug 28, 2024

feat: Add REST endpoints to update Components in a Collections (temp) open-craft/openedx-platform#674

Merged

pomegranited added 4 commits August 28, 2024 14:59

docs: adds guidelines for the Collection models

534e8d9

copied and modified slightly from @ormsbee's #131

feat: adds CollectionObject model and APIs

1fda3e5

to associate PublishableEntities with Collections. Collection.modified timestamp is updated whenever its contents are changed.

feat: adds get_object_collections API and tests

a2cd234

chore: bumps version to 0.11.3

7ec9ba5

pomegranited force-pushed the jill/collection-components branch from 35629c3 to 7ec9ba5 Compare August 28, 2024 05:34

pomegranited marked this pull request as ready for review August 28, 2024 05:34

pomegranited mentioned this pull request Aug 28, 2024

feat: Add REST endpoints to update Components in a Collections [FC-0062] openedx/openedx-platform#35384

Closed

pomegranited requested a review from rpenido August 28, 2024 06:02

fix: use PublishableEntity.key instead of IDs

0e15dd0

ormsbee requested changes Aug 28, 2024

View reviewed changes

rpenido approved these changes Aug 28, 2024

View reviewed changes

pomegranited mentioned this pull request Aug 29, 2024

feat: Add Library Collections REST endpoints [FC-0062] openedx/openedx-platform#35321

Merged

pomegranited commented Aug 29, 2024

View reviewed changes

Comment thread openedx_learning/apps/authoring/collections/api.py Outdated

rpenido approved these changes Aug 29, 2024

View reviewed changes

Comment thread openedx_learning/apps/authoring/collections/api.py Outdated

Comment thread openedx_learning/apps/authoring/collections/api.py Outdated

pomegranited requested a review from ormsbee September 2, 2024 03:24

ormsbee approved these changes Sep 2, 2024

View reviewed changes

fix: use fewer queries to discover invalid entities

e03daaa

being added to a collection

pomegranited merged commit 4779ec9 into main Sep 3, 2024

pomegranited deleted the jill/collection-components branch September 3, 2024 00:31

navinkarkera mentioned this pull request Sep 4, 2024

refactor: index collection per learning_package openedx/openedx-platform#35424

Closed

ormsbee mentioned this pull request Mar 24, 2025

add vscode pytest settings open-craft/openedx-learning#19

Closed

ormsbee mentioned this pull request Sep 4, 2025

version bump 0.28.0 open-craft/openedx-learning#20

Closed

		If you're trying to do either of these things, you probably want a new model or
		app. For more details, read on.

		return collection


		def get_entity_collections(learning_package_id: int, entity_key: str) -> QuerySet[Collection]:

Uh oh!

Conversation

pomegranited commented Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Uh oh!

openedx-webhooks commented Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

Uh oh!

Uh oh!

bradenmacdonald commented Aug 28, 2024

Uh oh!

ormsbee commented Aug 28, 2024

Uh oh!

ormsbee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pomegranited Aug 28, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rpenido left a comment

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!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rpenido left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pomegranited commented Sep 2, 2024

Uh oh!

ormsbee left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openedx-webhooks commented Sep 3, 2024

Uh oh!

Reviewers

Assignees

pomegranited commented Aug 21, 2024 •

edited

Loading

openedx-webhooks commented Aug 21, 2024 •

edited

Loading

pomegranited Aug 28, 2024 •

edited

Loading