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

yusuf-musleh · 2024-08-19T04:38:48Z

Description

This PR adds REST endpoints (list/create/update/delete) for Library Collections, in addition to emitting events when a library is created/updated. The deleted endpoint and emitting its event will be implemented in a later ticket, however the implementation skeleton is there.

Supporting information

Related Tickets:

[Libraries] [Collections] [Backend] REST CRUD API for Collections in v2 Libraries modular-learning#226

Depends on / blocked by:

Testing instructions

Go over new tests, make sure they cover most cases and make sure they are passing
For additional manual testing:
1. Checkout this branch in your local dev environment
2. Make sure you have feat: Add Content Library Collections signals [FC-0062] openedx-events#386 + feat: adds Collection.key openedx-learning#223 installed and running locally
3. Make sure you already have a V2 library or created one if not, e.g lib:SampleTaxonomyOrg1:AL1
4. Add some blocks to that library.
5. In an external HTTP client (like curl/postman or similar) make the following requests (make sure you have the correct headers for authentication, you can copy them from the network tab in your browser), replace the library key in the URL with your actual library key:
  1. GET http://studio.local.edly.io:8001/api/libraries/v2/lib:SampleTaxonomyOrg1:AL1/collections/ - Should return a list of all collections that belong to the library, initially it will be empty
  2. GET http://studio.local.edly.io:8001/api/libraries/v2/lib:SampleTaxonomyOrg1:AL1/collections/COL1/ - Should return a specific collection (with key="COL1") that belongs to the library, initially this should return 404, not found
  3. POST http://studio.local.edly.io:8001/api/libraries/v2/lib:SampleTaxonomyOrg1:AL1/collections/
    1. Payload: {"key": "COL1", "title":"Collection 1", "description": "Desc for Collection 1"}
    2. This should return the newly created collection
  4. PATCH http://studio.local.edly.io:8001/api/libraries/v2/lib:SampleTaxonomyOrg1:AL1/collections/COL1/
    1. Payload: {"title": "Collection 1 updated title"}
    2. This should return the updated collection
  5. POST http://studio.local.edly.io:8001/api/libraries/v2/lib:SampleTaxonomyOrg1:AL1/collections/
    1. Payload: {"key": "COL2", "title":"Collection 2", "description": "Desc for Collection 2"}
    2. Should return another newly created collection
  6. GET http://studio.local.edly.io:8001/api/libraries/v2/lib:SampleTaxonomyOrg1:AL1/collections/ - This should now return the 2 collections created above
  7. GET http://studio.local.edly.io:8001/api/libraries/v2/lib:SampleTaxonomyOrg1:AL1/collections/COL1/ - Should return the first collection we created
  8. DELETE http://studio.local.edly.io:8001/api/libraries/v2/lib:SampleTaxonomyOrg1:AL1/collections/COL1/ - Should not delete and return a 405
  9. GET http://studio.local.edly.io:8001/api/libraries/v2/lib:SampleTaxonomyOrg1:AL1/blocks/
    1. Should return the list of blocks added to this library
    2. Note their usage key values for the steps below.
  10. PATCH http://studio.local.edly.io:8001/api/libraries/v2/lib:SampleTaxonomyOrg1:AL1/collections/COL1/contents/
    1. Payload: {"usage_keys": [ <list of usage keys from the library blocks> ]
    2. Should return {"count": <number of components in list>}
    3. Check the logs and confirm that the event was emitted for each component, triggering an "upsert" into Meilisearch.
  11. GET http://studio.local.edly.io:8001/api/libraries/v2/lib:SampleTaxonomyOrg1:AL1/collections/COL1/
    1. The modified timestamp should be updated.
  12. DELETE http://studio.local.edly.io:8001/api/libraries/v2/lib:SampleTaxonomyOrg1:AL1/collections/COL1/contents/
    1. Payload: {"usage_keys": [ <list of usage keys from the library blocks> ]
    2. Should return {"count": <number of components in list>}
    3. Check the logs and confirm that the event was emitted for each component, triggering an "upsert" into Meilisearch.
  13. GET http://studio.local.edly.io:8001/api/libraries/v2/lib:SampleTaxonomyOrg1:AL1/collections/COL1/
    1. The modified timestamp should be updated.
  14. GET http://studio.local.edly.io:8001/api/libraries/v2/lib:SampleTaxonomyOrg1:AL1/collections/COL1/contents/ - will raise a 405, as an object's collections will be fetched from Meilisearch.

Private-Ref: FAL-3783

openedx-webhooks · 2024-08-19T04:38:52Z

Thanks for the pull request, @yusuf-musleh!

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.

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

rpenido

Good work here @yusuf-musleh!

I found a small issue while passing invalid library keys and added some other comments.
Let me know what you think!

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

openedx/core/djangoapps/content_libraries/api.py

docs/hooks/events.rst

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

openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py

yusuf-musleh · 2024-08-29T06:04:04Z

@rpenido @pomegranited Thanks for the reviews/comments! I've addressed them all here: 5bf9422

pomegranited

Hi @yusuf-musleh , this is working great, but I've requested some structural changes to where the code lives.

openedx/core/djangoapps/content_libraries/views.py

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

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

rpenido

LGTM 👍
Thank you for your work, @pomegranited!

Only a nit about updating the key and improving our error handling.

I tested this using the instructions from the PR
I read through the code
~~I checked for accessibility issues~~
Includes documentation

rpenido · 2024-09-06T00:51:45Z

openedx/core/djangoapps/content_libraries/views.py

@@ -136,12 +137,18 @@ def convert_exceptions(fn):
    def wrapped_fn(*args, **kwargs):
        try:
            return fn(*args, **kwargs)
+        except InvalidKeyError as exc:
+            log.exception(str(exc))
+            raise NotFound  # lint-amnesty, pylint: disable=raise-missing-from


Adding a new collection with the same key/library pair throws django.db.utils.IntegrityError. Maybe also add it here re-raising as ValidationError

openedx/core/djangoapps/content_libraries/serializers.py

and re-raise as api.LibraryCollectionAlreadyExists

…tions-crud-rest-api

and fixes event types documented in the hooks.

requirements/constraints.txt

navinkarkera · 2024-09-10T13:28:49Z

openedx/core/djangoapps/content_libraries/views_collections.py

+    """
+
+    serializer_class = ContentLibraryCollectionSerializer
+    lookup_field = 'key'


@pomegranited Since we plan to make use of object_key i.e. LibraryCollectionKey in frontend urls, we should also convert this view to accept LibraryCollectionKey instead of key similar to libraries. Provided we really want to include it, as we already have library key in the url in both frontend and backend.

I can do it in a follow up PR if we don't want to block this one.

Same, there are different places where LibraryCollectionKey can be used. I agree with creating a task for it and doing it when opaque-keys is updated in edx-platform

For now I would prefer to not use the opaque keys in too many places, other than for tagging. We actually tried to avoid using the opaque key completely, except that it's required for tagging. As I mentioned in #35321 (comment) for the URLs and APIs we can generally just accept both a library ID and a key/slug (not the full opaque key, just the key part) as separate URL parameters. The python APIs can just use the collection integer IDs which is more efficient than huge string keys anyways.

@bradenmacdonald Yes, that is much better.

ChrisChV · 2024-09-10T17:41:52Z

@pomegranited I created open-craft#683 with some small changes. Those changes are used in openedx/frontend-app-authoring#1259, I've seen that it's easier to add them in this PR. Let me know what you think

ChrisChV

@pomegranited Looks good 👍 I will wait for openedx/openedx-learning#223 and for your review in open-craft#683

* Update description as optional in ContentLibraryCollectionUpdateSerializer * Create collection Rest API to auto-generate key

rpenido · 2024-09-11T16:29:18Z

Hi @ChrisChV!
I found a small issue in the id for the document in the meilisearch index.
I updated it here: 1738447

Our key field is not unique (we can have the same key for multiple libraries).
As this id is for meilisearch only and not exported or used elsewhere, I think it is safe to use the Collection id instead of the key.

ChrisChV · 2024-09-11T17:24:46Z

@rpenido Looks good and works well 👍

edx-pipeline-bot · 2024-09-11T19:40:55Z

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

edx-pipeline-bot · 2024-09-11T20:00:31Z

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

edx-pipeline-bot · 2024-09-11T20:00:31Z

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

edx-pipeline-bot · 2024-09-11T20:18:08Z

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

edx-pipeline-bot · 2024-09-11T20:35:26Z

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

edx-pipeline-bot · 2024-09-11T20:35:27Z

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

pomegranited · 2024-09-12T04:03:48Z

@rpenido

As this id is for meilisearch only and not exported or used elsewhere, I think it is safe to use the Collection id instead of the key.

Good catch on the id -- but we still need the Collection.key to be present in that data, because that's required for accessing the REST APIs.

Will follow up with on this with FAL-3787 CC @ChrisChV

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

yusuf-musleh force-pushed the yusuf-musleh/collections-crud-rest-api branch from fcff07b to 8c1d5c8 Compare August 22, 2024 05:39

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

yusuf-musleh force-pushed the yusuf-musleh/collections-crud-rest-api branch from 8c1d5c8 to f490007 Compare August 23, 2024 06:14

yusuf-musleh mentioned this pull request Aug 23, 2024

feat: Add Library Collections REST endpoints (temp) open-craft/edx-platform#671

Closed

pomegranited reviewed Aug 23, 2024

View reviewed changes

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

yusuf-musleh force-pushed the yusuf-musleh/collections-crud-rest-api branch 3 times, most recently from d3f751f to 743291c Compare August 23, 2024 11:39

yusuf-musleh mentioned this pull request Aug 25, 2024

feat: Add Content Library Collections signals [FC-0062] openedx/openedx-events#386

Merged

11 tasks

yusuf-musleh force-pushed the yusuf-musleh/collections-crud-rest-api branch 3 times, most recently from 4f56674 to 59514cc Compare August 26, 2024 19:49

yusuf-musleh marked this pull request as ready for review August 27, 2024 04:36

This was referenced Aug 28, 2024

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

Merged

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

Closed

rpenido suggested changes Aug 28, 2024

View reviewed changes

pomegranited mentioned this pull request Aug 29, 2024

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

Merged

pomegranited reviewed Aug 29, 2024

View reviewed changes

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

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

pomegranited requested changes Aug 29, 2024

View reviewed changes

yusuf-musleh force-pushed the yusuf-musleh/collections-crud-rest-api branch from 5bf9422 to 95de012 Compare August 30, 2024 06:21

yusuf-musleh added 6 commits August 30, 2024 09:28

feat: Add Library Collections REST endpoints

ed30ce2

test: Add tests for Collections REST APIs

3d64ea4

chore: Add missing __init__ files

931f688

feat: Verify collection belongs to library

b2b38cb

feat: Add events emitting for Collections

a16b398

chore: fix pylint errors

be39d03

navinkarkera mentioned this pull request Sep 5, 2024

feat: collections tab [FC-0062] openedx/frontend-app-authoring#1257

Merged

rpenido reviewed Sep 6, 2024

View reviewed changes

pomegranited added 2 commits September 6, 2024 14:45

fix: catch IntegrityError in create_library_collection

007f80a

and re-raise as api.LibraryCollectionAlreadyExists

Merge remote-tracking branch 'origin/master' into yusuf-musleh/collec…

197733b

…tions-crud-rest-api

rpenido mentioned this pull request Sep 9, 2024

feat: REST API for create Library Collection [FC-0062] #35447

Closed

chore: updates openedx-events==9.14.0

d312ff3

and fixes event types documented in the hooks.

pomegranited reviewed Sep 10, 2024

View reviewed changes

requirements/constraints.txt Outdated Show resolved Hide resolved

navinkarkera reviewed Sep 10, 2024

View reviewed changes

ChrisChV approved these changes Sep 10, 2024

View reviewed changes

pomegranited and others added 3 commits September 11, 2024 08:55

Merge branch 'master' into yusuf-musleh/collections-crud-rest-api

715527c

chore: updates openedx-learning==0.11.4

d5aeff8

refactor: Update collections crud rest api (#683)

e6b469d

* Update description as optional in ContentLibraryCollectionUpdateSerializer * Create collection Rest API to auto-generate key

pomegranited force-pushed the yusuf-musleh/collections-crud-rest-api branch from 48a1667 to e6b469d Compare September 11, 2024 01:18

pomegranited mentioned this pull request Sep 11, 2024

Store content object collections in search index [FC-0062] #35469

Merged

navinkarkera mentioned this pull request Sep 11, 2024

Collections Page - Bare bones openedx/frontend-app-authoring#1101

Closed

fix: assert collection doc have unique id

1738447

Merge branch 'master' into yusuf-musleh/collections-crud-rest-api

b645044

ChrisChV merged commit 7665f13 into openedx:master Sep 11, 2024
49 checks passed

ormsbee mentioned this pull request Sep 27, 2024

XBlock asset support for new libraries experience (WIP) #35557

Closed

rpenido deleted the yusuf-musleh/collections-crud-rest-api branch October 25, 2024 13:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

yusuf-musleh commented Aug 19, 2024 •

edited by pomegranited

Loading

openedx-webhooks commented Aug 19, 2024 •

edited

Loading

rpenido left a comment

yusuf-musleh commented Aug 29, 2024

pomegranited left a comment

rpenido left a comment

rpenido Sep 6, 2024

navinkarkera Sep 10, 2024 •

edited

Loading

ChrisChV Sep 10, 2024

bradenmacdonald Sep 10, 2024

navinkarkera Sep 11, 2024

ChrisChV commented Sep 10, 2024

ChrisChV left a comment •

edited

Loading

rpenido commented Sep 11, 2024

ChrisChV commented Sep 11, 2024

edx-pipeline-bot commented Sep 11, 2024

edx-pipeline-bot commented Sep 11, 2024

edx-pipeline-bot commented Sep 11, 2024

edx-pipeline-bot commented Sep 11, 2024

edx-pipeline-bot commented Sep 11, 2024

edx-pipeline-bot commented Sep 11, 2024

pomegranited commented Sep 12, 2024 •

edited

Loading

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

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

Conversation

yusuf-musleh commented Aug 19, 2024 • edited by pomegranited Loading

Description

Supporting information

Testing instructions

openedx-webhooks commented Aug 19, 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?

rpenido left a comment

Choose a reason for hiding this comment

yusuf-musleh commented Aug 29, 2024

pomegranited left a comment

Choose a reason for hiding this comment

rpenido left a comment

Choose a reason for hiding this comment

rpenido Sep 6, 2024

Choose a reason for hiding this comment

navinkarkera Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

ChrisChV Sep 10, 2024

Choose a reason for hiding this comment

bradenmacdonald Sep 10, 2024

Choose a reason for hiding this comment

navinkarkera Sep 11, 2024

Choose a reason for hiding this comment

ChrisChV commented Sep 10, 2024

ChrisChV left a comment • edited Loading

Choose a reason for hiding this comment

rpenido commented Sep 11, 2024

ChrisChV commented Sep 11, 2024

edx-pipeline-bot commented Sep 11, 2024

edx-pipeline-bot commented Sep 11, 2024

edx-pipeline-bot commented Sep 11, 2024

edx-pipeline-bot commented Sep 11, 2024

edx-pipeline-bot commented Sep 11, 2024

edx-pipeline-bot commented Sep 11, 2024

pomegranited commented Sep 12, 2024 • edited Loading

yusuf-musleh commented Aug 19, 2024 •

edited by pomegranited

Loading

openedx-webhooks commented Aug 19, 2024 •

edited

Loading

navinkarkera Sep 10, 2024 •

edited

Loading

ChrisChV left a comment •

edited

Loading

pomegranited commented Sep 12, 2024 •

edited

Loading