feat: Add REST endpoints to update Components in a Collections (temp) #674

pomegranited · 2024-08-28T04:06:43Z

This PR adds REST endpoints (patch, delete) for updating a Library Collection's Components.

This PR is for code review only; see openedx#35384 for testing instructions.

Private-ref: FAL-3784

Python API: * Converts UsageKeyV2 object keys into component keys for use with the oel_collections api. * Sends a CONTENT_OBJECT_TAGS_CHANGED for each component added/removed. REST API: * Calls the python API * Receives a collection PK + a list of UsageKeys to add to the collection.

bradenmacdonald · 2024-08-28T18:48:02Z

@pomegranited Let's mark this API as unstable with a clear comment that the API interface may change once we implement the ability to store units/sections/subsections in the library (which may not be "Components"/XBlocks, but which still need to be associated with collections).

rpenido · 2024-08-28T18:57:23Z

openedx/core/djangoapps/content_libraries/api.py

+    # Emit a CONTENT_OBJECT_TAGS_CHANGED event for each of the objects added/removed
+    object_keys = contents_qset.values_list("key", flat=True)
+    for object_key in object_keys:
+        CONTENT_OBJECT_TAGS_CHANGED.send_event(
+            content_object=ContentObjectData(object_id=object_key),
+        )
+


~~I don't think I get why we are dispatching CONTENT_OBJECT_TAGS_CHANGED here. Maybe you mean LIBRARY_COLLECTION_UPDATED?~~

Note: There is a discussion about this event here.

Edit: Now I saw that this came from the issue description. But I don't see any benefit from using this event. Currently, it updates only the tag data (ref), not the metadata of the document (that we would need here)

@rpenido Yep, that's the current behaviour, but we're planning to update it with openedx/modular-learning#230. I've made a start at that here: jill/collection-components-rest...open-craft:edx-platform:jill/collection-components-search

But will see how that discussion shakes out.

@rpenido I will rename CONTENT_OBJECT_TAGS_CHANGED to CONTENT_OBJECT_ASSOCIATIONS_CHANGED, and add a field indicating whether it was the tag or collection that changed when I do openedx/modular-learning#230.

openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py

rpenido

LGTM 👍
Thank you for your work, @pomegranited!

The code is really clear and well-documented! It was a pleasure to review!
I think we will need to check the EVENT from the content update. See the comment in the PR.

I tested this using the instructions from feat: Add REST endpoints to update Components in a Collections [FC-0062] openedx/edx-platform#35384
Nit: I had to change the PATCH/DELETE param from component_keys to usage_keys.
I read through the code
~~I checked for accessibility issues~~
Includes documentation

rpenido · 2024-08-28T19:22:43Z

openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py

@@ -179,3 +182,38 @@ def destroy(self, request, *args, **kwargs):
        # TODO: Implement the deletion logic and emit event signal

        return Response(None, status=HTTP_405_METHOD_NOT_ALLOWED)
+
+    @action(detail=True, methods=['delete', 'patch'], url_path='contents', url_name='contents:update')


I would expect DELETE collection/contents to clear all content, so I am bit more inclined to use the PATCH with a additional remove parameter here. Or even:

{ "add": ["component1"], "remove": ["compoinent2"] }

The later would add need more work for a feature that we don't need now (replace a component), so please fell free to ignore this whole comment for now.

I'm fine with the way it is working.

The url name here is giving this warning:

WARNINGS: cms-1 | ?: (urls.W003) Your URL pattern '^collections/(?P<pk>[^/.]+)/contents/$' [name='library-collections-contents:update'] has a name including a ':'. Remove the colon, to avoid ambiguous namespace references. cms-1 | ?: (urls.W003) Your URL pattern '^collections/(?P<pk>[^/.]+)/contents\.(?P<format>[a-z0-9]+)/?$' [name='library-collections-contents:update'] has a name including a ':'. Remove the colon, to avoid ambiguous namespace references. ``

@rpenido Thanks for raising this.. I agree it's ambiguous, but given that we're flagging this API as UNSTABLE, I think it's ok to leave it as-is, and change it if/when we need to.

Will fix the url name though!

Addresses PR reviews. * Collection contents -> Collection components * Add warning about unstable REST APIs * Wrap update_components view in convert_exceptions * Raise api exceptions in api.update_collection_components * Use usage_key in CONTENT_OBJECT_TAGS_CHANGED event

as oel_collections.get_learning_package_collections has been removed.

…-rest-api' into jill/collection-components-rest

pomegranited · 2024-08-29T07:27:55Z

Ugh.. my commits have gone all wonky here, but I don't want to rebase while it's under review. @rpenido let me know if you need any clarity on what's being changed here?

so we can use the updated oel_collections.get_collections method.

rpenido

Ugh.. my commits have gone all wonky here,

It's fine, @pomegranited! Thank you for the changes.

Looks like we have a (not related) flaky test. Could you try to re-run the CI?

…ction-components-rest.pre-rebase

* feat: Add Library Collections REST endpoints * test: Add tests for Collections REST APIs * chore: Add missing __init__ files * feat: Add events emitting for Collections * feat: Add REST endpoints to update Components in a Collections (temp) (#674) * feat: add/remove components to/from a collection * docs: Add warning about unstable REST APIs * chore: updates openedx-events==9.14.0 * chore: updates openedx-learning==0.11.4 * fix: assert collection doc have unique id --------- Co-authored-by: Jillian <[email protected]> Co-authored-by: Chris Chávez <[email protected]> Co-authored-by: Rômulo Penido <[email protected]>

yusuf-musleh added 6 commits August 23, 2024 08:56

feat: Add Library Collections REST endpoints

f58bb67

test: Add tests for Collections REST APIs

f490007

chore: Add missing __init__ files

17933a1

feat: Verify collection belongs to library

743291c

feat: Add events emitting for Collections

3da44d5

chore: fix pylint errors

59514cc

pomegranited changed the title ~~feat: Add REST endpoints to add/remove Components to/from Collections [FC-0062]~~ feat: Add REST endpoints to add/remove Components to/from Collections (temp) Aug 28, 2024

chore: update openedx-learning

250434f

pomegranited changed the title ~~feat: Add REST endpoints to add/remove Components to/from Collections (temp)~~ feat: Add REST endpoints to update Components in a Collections (temp) Aug 28, 2024

pomegranited force-pushed the jill/collection-components-rest branch from a13005b to 704bb11 Compare August 28, 2024 05:18

pomegranited mentioned this pull request Aug 28, 2024

Associate PublishableEntities with Collections [FC-0062] openedx/openedx-learning#216

Merged

pomegranited marked this pull request as draft August 28, 2024 05:56

pomegranited force-pushed the jill/collection-components-rest branch from eb91d5d to f4521b5 Compare August 28, 2024 10:50

rpenido reviewed Aug 28, 2024

View reviewed changes

openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py Outdated Show resolved Hide resolved

rpenido reviewed Aug 28, 2024

View reviewed changes

pomegranited and others added 12 commits August 29, 2024 11:19

refactor: use oel_collections.get_collections

e8013eb

as oel_collections.get_learning_package_collections has been removed.

fix: pylint/type error

94890db

test: adds tests for api.update_collection_components

6ded8a9

test: fixes failing test

0d8079c

feat: Add Library Collections REST endpoints

fe43b72

test: Add tests for Collections REST APIs

f5900be

chore: Add missing __init__ files

97739c6

feat: Verify collection belongs to library

2996a2e

feat: Add events emitting for Collections

fbfd983

chore: fix pylint errors

cf9e906

refactor: Use convert_exceptions + update tests

5bf9422

yusuf-musleh force-pushed the yusuf-musleh/collections-crud-rest-api branch from 59514cc to 5bf9422 Compare August 29, 2024 05:50

Merge remote-tracking branch 'opencraft/yusuf-musleh/collections-crud…

ea24e2f

…-rest-api' into jill/collection-components-rest

refactor: index collections within each library

8034861

so we can use the updated oel_collections.get_collections method.

rpenido approved these changes Aug 29, 2024

View reviewed changes

pomegranited added 2 commits August 30, 2024 14:15

test: fix flaky test

55338e1

refactor: adapt to oel_collection.update_collection_components changes

23f5d5b

yusuf-musleh force-pushed the yusuf-musleh/collections-crud-rest-api branch 2 times, most recently from 95de012 to 366a7e9 Compare August 30, 2024 06:28

pomegranited added 2 commits September 2, 2024 12:30

Merge branch 'yusuf-musleh/collections-crud-rest-api' into jill/colle…

0a1732e

…ction-components-rest.pre-rebase

refactor: moved code around after merging base

addde52

pomegranited marked this pull request as ready for review September 3, 2024 00:15

pomegranited added 2 commits September 3, 2024 10:34

refactor: simplify the REST API params and validation

51f5c1a

chore: uses openedx-learning==0.11.3

7a8e4c6

pomegranited merged commit 504aa4f into yusuf-musleh/collections-crud-rest-api Sep 3, 2024
47 checks passed

pomegranited deleted the jill/collection-components-rest branch November 1, 2024 02:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add REST endpoints to update Components in a Collections (temp) #674

feat: Add REST endpoints to update Components in a Collections (temp) #674

pomegranited commented Aug 28, 2024 •

edited

Loading

bradenmacdonald commented Aug 28, 2024

rpenido Aug 28, 2024 •

edited

Loading

pomegranited Aug 28, 2024 •

edited

Loading

pomegranited Sep 2, 2024

rpenido left a comment •

edited

Loading

rpenido Aug 28, 2024

rpenido Aug 28, 2024

pomegranited Aug 29, 2024

pomegranited commented Aug 29, 2024

rpenido left a comment

feat: Add REST endpoints to update Components in a Collections (temp) #674

feat: Add REST endpoints to update Components in a Collections (temp) #674

Conversation

pomegranited commented Aug 28, 2024 • edited Loading

bradenmacdonald commented Aug 28, 2024

rpenido Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

pomegranited Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

pomegranited Sep 2, 2024

Choose a reason for hiding this comment

rpenido left a comment • edited Loading

Choose a reason for hiding this comment

rpenido Aug 28, 2024

Choose a reason for hiding this comment

rpenido Aug 28, 2024

Choose a reason for hiding this comment

pomegranited Aug 29, 2024

Choose a reason for hiding this comment

pomegranited commented Aug 29, 2024

rpenido left a comment

Choose a reason for hiding this comment

pomegranited commented Aug 28, 2024 •

edited

Loading

rpenido Aug 28, 2024 •

edited

Loading

pomegranited Aug 28, 2024 •

edited

Loading

rpenido left a comment •

edited

Loading