feat: set collections for a library component [FC-0062] #35600

navinkarkera · 2024-10-07T06:15:58Z

Description

Adds api to set/update collections on a given component. Related to openedx/frontend-app-authoring#1373

Supporting information

Depends on:

refactor: add lazy field to LibraryCollectionData openedx-events#404

Testing instructions

See openedx/frontend-app-authoring#1373 for instructions.

Deadline

"None" if there's no rush, or provide a specific date or event (and reason) if there is one.

Other information

openedx-webhooks · 2024-10-07T06:16:03Z

Thanks for the pull request, @navinkarkera!

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 @openedx/wg-maintenance-edx-platform. 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.

navinkarkera · 2024-10-07T06:29:29Z

requirements/edx/base.txt

@@ -824,7 +824,7 @@ openedx-filters==1.10.0
    #   -r requirements/edx/kernel.in
    #   lti-consumer-xblock
    #   ora2
-openedx-learning==0.13.1
+openedx-learning @ git+https://github.com/open-craft/openedx-learning@navin/component-collection-api


TODO: update this after merging openedx/openedx-learning#238 releasing new version of openedx-learning

navinkarkera · 2024-10-07T06:29:33Z

requirements/edx/development.txt

@@ -1373,7 +1373,7 @@ openedx-filters==1.10.0
    #   -r requirements/edx/testing.txt
    #   lti-consumer-xblock
    #   ora2
-openedx-learning==0.13.1
+openedx-learning @ git+https://github.com/open-craft/openedx-learning@navin/component-collection-api


TODO: update this after merging openedx/openedx-learning#238 releasing new version of openedx-learning

navinkarkera · 2024-10-07T06:29:36Z

requirements/edx/doc.txt

@@ -983,7 +983,7 @@ openedx-filters==1.10.0
    #   -r requirements/edx/base.txt
    #   lti-consumer-xblock
    #   ora2
-openedx-learning==0.13.1
+openedx-learning @ git+https://github.com/open-craft/openedx-learning@navin/component-collection-api


TODO: update this after merging openedx/openedx-learning#238 releasing new version of openedx-learning

navinkarkera · 2024-10-07T06:29:40Z

requirements/edx/testing.txt

@@ -1034,7 +1034,7 @@ openedx-filters==1.10.0
    #   -r requirements/edx/base.txt
    #   lti-consumer-xblock
    #   ora2
-openedx-learning==0.13.1
+openedx-learning @ git+https://github.com/open-craft/openedx-learning@navin/component-collection-api


TODO: update this after merging openedx/openedx-learning#238 releasing new version of openedx-learning

navinkarkera · 2024-10-08T12:13:24Z

openedx/core/djangoapps/content_libraries/api.py

+    from ..content.search.tasks import update_library_collection_index_doc
+    for collection in affected_collections:
+        update_library_collection_index_doc.delay(str(library_key), collection.key)


Trigger collection indexing asynchronously instead of relying on sync indexing via signals. See https://github.com/openedx/openedx-learning/pull/238/files#diff-50734dffd9d06d0a9cbc318f1b298dddd25968d064b515d716dc7a0023b7190bR660-R666

You can test the performance improvement by setting CELERY_ALWAYS_EAGER = False in cms/envs/devstack.py.

I didn't notice a performance improvement here (maybe I tested it wrong?).

This call is always in the 600ms - 2500ms range (which I agree is not good), but I don't think we should call the search tasks directly here. Maybe add another parameter to the LIBRARY_COLLECTION_UPDATED event to define beforehand whether the task should run sync or async.

@bradenmacdonald, can you give us your insight here? I don't want to make things complicated near the version cut.

Maybe add another parameter to the LIBRARY_COLLECTION_UPDATED event to define beforehand whether the task should run sync or async.

Yes, that is better. I was probably feeling lazy to update the event as part of this ticket but seems like it is unavoidable.

I didn't notice a performance improvement here (maybe I tested it wrong?).

You need to add and remove multiple collections for a component to see the difference. Also set CELERY_ALWAYS_EAGER = False to run celery tasks in worker.

@rpenido Updated LibraryCollectionData to include a field call lazy and used it here instead.

rpenido

LGTM 👍
Thank you for your work, @navinkarkera!

I tested this using the instructions from feat: manage collections in component sidebar [FC-0062] frontend-app-authoring#1373
I read through the code
~~I checked for accessibility issues~~ N/A
Includes documentation

I added a small PR to circumvent the meilisearch bug below. It is missing some tests fixes:

fix: circuvent meilisearch bug with empty objects open-craft/edx-platform#695

Note
We have an unused endpoint that we created beforehand for handling components/collections here:

edx-platform/openedx/core/djangoapps/content_libraries/views_collections.py

Line 194 in 15a1295

def update_components(self, request, *args, **kwargs) -> Response:

(and some API methods too). I'm not sure if we should remove it right away. It could be helpful if we implement the "Remove from this collection" action in the three dot menu.

rpenido · 2024-10-08T14:31:59Z

openedx/core/djangoapps/content_libraries/api.py

+    from ..content.search.tasks import update_library_collection_index_doc
+    for collection in affected_collections:
+        update_library_collection_index_doc.delay(str(library_key), collection.key)


I didn't notice a performance improvement here (maybe I tested it wrong?).

This call is always in the 600ms - 2500ms range (which I agree is not good), but I don't think we should call the search tasks directly here. Maybe add another parameter to the LIBRARY_COLLECTION_UPDATED event to define beforehand whether the task should run sync or async.

@bradenmacdonald, can you give us your insight here? I don't want to make things complicated near the version cut.

The indexing update is already handled by m2m_changed handler.

Using `get_collection_components` does not work in case of post_remove as the component is already removed from collection and we don't really need additional filtering as we already have pk_set from the signal.

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

navinkarkera commented Oct 7, 2024

View reviewed changes

navinkarkera force-pushed the navin/component-collection-api branch from c6736b8 to eb7d2d1 Compare October 7, 2024 06:42

navinkarkera mentioned this pull request Oct 8, 2024

feat: manage collections in component sidebar [FC-0062] openedx/frontend-app-authoring#1373

Open

2 tasks

navinkarkera changed the title ~~feat: set collections for a library component~~ feat: set collections for a library component [FC-0062] Oct 8, 2024

navinkarkera commented Oct 8, 2024

View reviewed changes

navinkarkera marked this pull request as ready for review October 8, 2024 12:13

rpenido force-pushed the navin/component-collection-api branch from 93f436d to eb7d2d1 Compare October 8, 2024 13:54

rpenido approved these changes Oct 8, 2024

View reviewed changes

navinkarkera force-pushed the navin/component-collection-api branch 3 times, most recently from ce7f906 to 3e889a9 Compare October 9, 2024 11:08

navinkarkera added 12 commits October 10, 2024 11:50

feat: add & remove collections to component

bf17dbb

refactor: set collections in a component

e5d3b69

refactor: async collections indexing

40a63e4

fix: lint issues

d18d5ba

test: test set collections for component

ac59d53

chore: fix docs

fa8ad9e

chore: temporarily update openedx-learning package

0b7a782

fix: lint issues

e007398

chore: add usage_key to filterable attribute

0a438d0

feat: add collections to component api

4c8ab8a

fix: lint issues

86b8cdd

fix: remove collection-entity through model post_delete signal handler

cd9eec9

The indexing update is already handled by m2m_changed handler.

navinkarkera force-pushed the navin/component-collection-api branch from 3e889a9 to cd9eec9 Compare October 10, 2024 06:20

navinkarkera mentioned this pull request Oct 10, 2024

refactor: add lazy field to LibraryCollectionData openedx/openedx-events#404

Open

9 tasks

navinkarkera force-pushed the navin/component-collection-api branch from e2f79fe to 1debf71 Compare October 10, 2024 07:31

navinkarkera added 2 commits October 10, 2024 15:09

fix: use lazy field to control collection update task

97ad3ba

chore: temporarily point openedx-events to dev branch

baa10fc

navinkarkera force-pushed the navin/component-collection-api branch from 1debf71 to baa10fc Compare October 10, 2024 09:39

fix: use get_components in m2m_changed signal handler

e553ec0

Using `get_collection_components` does not work in case of post_remove as the component is already removed from collection and we don't really need additional filtering as we already have pk_set from the signal.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: set collections for a library component [FC-0062] #35600

feat: set collections for a library component [FC-0062] #35600

navinkarkera commented Oct 7, 2024 •

edited

Loading

openedx-webhooks commented Oct 7, 2024 •

edited

Loading

navinkarkera Oct 7, 2024

navinkarkera Oct 7, 2024

navinkarkera Oct 7, 2024

navinkarkera Oct 7, 2024

navinkarkera Oct 8, 2024 •

edited

Loading

rpenido Oct 8, 2024

navinkarkera Oct 8, 2024

navinkarkera Oct 8, 2024

navinkarkera Oct 10, 2024

rpenido left a comment •

edited

Loading

rpenido Oct 8, 2024

feat: set collections for a library component [FC-0062] #35600

Are you sure you want to change the base?

feat: set collections for a library component [FC-0062] #35600

Conversation

navinkarkera commented Oct 7, 2024 • edited Loading

Description

Supporting information

Testing instructions

Deadline

Other information

openedx-webhooks commented Oct 7, 2024 • edited Loading

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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navinkarkera Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navinkarkera commented Oct 7, 2024 •

edited

Loading

openedx-webhooks commented Oct 7, 2024 •

edited

Loading

navinkarkera Oct 8, 2024 •

edited

Loading

rpenido left a comment •

edited

Loading