feat: component set collection api [FC-0062] #238
Conversation
openedx-webhooks
added
the
open-source-contribution
|
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 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. |
navinkarkera
changed the title
navinkarkera
force-pushed
the
navin/component-collection-api
branch
3 times, most recently
from
d474390 to
02ecc1a
Compare
navinkarkera
force-pushed
the
navin/component-collection-api
branch
3 times, most recently
from
deea38c to
7caf76d
Compare
| if relations.exists(): | ||
| relation = relations[0] | ||
| removed_collections = set(relation.collection for relation in relations) | ||
| # The normal `delete()` function deletes objects and triggers post delete signal for each object which is not | ||
| # required in this case as executing the signal handler once at the end should be more than enough. | ||
| # HACK: use private method `_raw_delete` deletes the objects without triggering the signals. | ||
| relations._raw_delete(relations.db) # type: ignore[attr-defined] # pylint: disable=protected-access | ||
| # Trigger the signal manually for a single CollectionPublishableEntity object | ||
| post_delete.send( | ||
| sender=CollectionPublishableEntity, | ||
| instance=relation, | ||
| using=relations.db, | ||
| origin=relation, | ||
| ) |
There was a problem hiding this comment.
In edx-platform we have a post_delete signal handler for CollectionPublishableEntity which updates components collections in meilisearch index. In this specific case where we are operating on a single component and updating multiple collections we don't need to trigger reindexing on delete of each collection relation object as it slows down the whole operation a lot.
I know that using private method to go around this issue is not the best solution but other option was to manage the collection indexing without post_delete signal of CollectionPublishableEntity model which I plan to investigate soon. Any advice will be appreciated.
There was a problem hiding this comment.
I think this is good enough for now, but in the future, we could implement some mechanism to deduplicate tasks so we don't have to handle each case that it happens manually.
There was a problem hiding this comment.
Instead of a signal handler for receiver(post_delete, sender=CollectionPublishableEntity, why don't you ignore the post_delete event (and emit several of them - no problem). Then, here in the code, instead of manually emitting one post_delete for CollectionPublishableEntity (which isn't really correct, because several were actually delete, and other parts of the system may want to know that), you can emit a custom signal for the entity itself, like collection_changed. Then the platform can listen to that single signal, and you don't have to suppress any "real" events or use private methods.
There was a problem hiding this comment.
Instead of a signal handler for receiver(post_delete, sender=CollectionPublishableEntity, why don't you ignore the post_delete event
@bradenmacdonald I thought the post_delete event is used to update component index (collections key) whenever a component is removed from a collection or a collection is deleted (all components are updated) but it seems like we also have post_remove action in m2m_changed signal doing the same thing. I'll investigate and test a bit more and update.
There was a problem hiding this comment.
Turns out that we can just remove post_delete handler for CollectionPublishableEntity as it is not required. The post_remove action in m2m_changed signal triggers single event for multiple removals in single remove call which is what we want.
There was a problem hiding this comment.
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 issuesN/A - Includes documentation
| if relations.exists(): | ||
| relation = relations[0] | ||
| removed_collections = set(relation.collection for relation in relations) | ||
| # The normal `delete()` function deletes objects and triggers post delete signal for each object which is not | ||
| # required in this case as executing the signal handler once at the end should be more than enough. | ||
| # HACK: use private method `_raw_delete` deletes the objects without triggering the signals. | ||
| relations._raw_delete(relations.db) # type: ignore[attr-defined] # pylint: disable=protected-access | ||
| # Trigger the signal manually for a single CollectionPublishableEntity object | ||
| post_delete.send( | ||
| sender=CollectionPublishableEntity, | ||
| instance=relation, | ||
| using=relations.db, | ||
| origin=relation, | ||
| ) |
There was a problem hiding this comment.
I think this is good enough for now, but in the future, we could implement some mechanism to deduplicate tasks so we don't have to handle each case that it happens manually.
navinkarkera
force-pushed
the
navin/component-collection-api
branch
from
89260ba to
83aec7f
Compare
Adds api for updating collections for a given component. See openedx/frontend-app-authoring#1373 for more information.