-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: REST API for create Library Collection [FC-0062] #35447
feat: REST API for create Library Collection [FC-0062] #35447
Conversation
Thanks for the pull request, @ChrisChV! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf 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 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
@@ -119,7 +119,7 @@ openedx-calc # Library supporting mathematical calculatio | |||
openedx-django-require | |||
openedx-events # Open edX Events from Hooks Extension Framework (OEP-50) | |||
openedx-filters # Open edX Filters from Hooks Extension Framework (OEP-50) | |||
openedx-learning # Open edX Learning core (experimental) | |||
git+https://github.com/open-craft/openedx-learning.git@jill/collection-key#egg=openedx-learning # Open edX Learning core (experimental) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Bump version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good @ChrisChV! Good work!
I think we already have the event for LIBRARY_COLLECTION_CREATED
. Should we implement the index update as part of this task?
@@ -298,7 +298,8 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: | |||
|
|||
# Get the list of collections | |||
status_cb("Counting collections...") | |||
num_collections = authoring_api.get_collections().count() | |||
collections = Collection.objects.filter(enabled=True).select_related("learning_package").order_by('pk') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This is also being solved here (#35321) with a different approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! I think you don't need to worry about it here. cc @pomegranited
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: 3180f76
try: | ||
result = authoring_api.create_collection( | ||
learning_package_id=library.learning_package_id, | ||
key=key, | ||
title=title, | ||
description=serializer.validated_data['description'], | ||
created_by=request.user.id, | ||
) | ||
except IntegrityError: | ||
return Response(status=status.HTTP_409_CONFLICT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the user can't directly edit the slug, do you think it is worth an approach like this?
try: | |
result = authoring_api.create_collection( | |
learning_package_id=library.learning_package_id, | |
key=key, | |
title=title, | |
description=serializer.validated_data['description'], | |
created_by=request.user.id, | |
) | |
except IntegrityError: | |
return Response(status=status.HTTP_409_CONFLICT) | |
attempt = 0 | |
result = None | |
while not result: | |
modified_key = key if attempt = 0 else key + str(attempt) | |
try: | |
result = authoring_api.create_collection( | |
learning_package_id=library.learning_package_id, | |
key=modified_key, | |
title=title, | |
description=serializer.validated_data['description'], | |
created_by=request.user.id, | |
) | |
except IntegrityError: | |
attempt += 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Rename collection.key to "slug" because "key" is a reserved prop name in React | ||
slug = serializers.CharField(read_only=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we plan to change it in the model?
Also, this is not being returned from the rest call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That wasn't my plan. I've already updated the code to return the slug in the response: 817e3b4
…on create collection API view
Closed in flavor of open-craft#683 |
Description
Supporting information
Testing instructions