feat: add block_id field to filterable attributes of meilisearch #35493
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
force-pushed
the
navin/add-collections-to-lib-block
branch
from
0294f24
to
9f1ebd4
Compare
| @@ -1185,6 +1197,8 @@ def update_library_collection_components( | |||
| remove=False, | |||
| # As an optimization, callers may pass in a pre-fetched ContentLibrary instance | |||
| content_library: ContentLibrary | None = None, | |||
| # Similarly, users may pass component_keys instead of usage_keys | |||
| component_keys: list[str] | None = None, | |||
There was a problem hiding this comment.
I'm ok with this change since it saves re-querying data we already have. But I think we shouldn't allow passing in usage_keys if they're providing component_keys instead?
There was a problem hiding this comment.
@pomegranited Actually, I need to update the comment. We still need usage_keys as they are used below to trigger CONTENT_OBJECT_ASSOCIATIONS_CHANGED event.
Update: Updated comment
There was a problem hiding this comment.
Well, we can generate the LibraryUsageLocator keys from component_keys since we have the base library-key, e.g.
usage_key = LibraryUsageLocator(library_key, component.component_type.name, component.local_key)What do you think? Is it too much to check that either a) component_keys + no usage_keys were passed in or b) the usage_keys provided match the component_keys provided?
There was a problem hiding this comment.
There should definitely be some sort of validation on the component_keys to make sure they're from the same library.
Personally, I would probably not even have the component_keys parameter for now. It seems a bit like premature optimization, especially when we don't yet have any use of this API to change more than one component at a time (as far as I can see in this PR). But I'm not opposed to leaving it in, as long as there's some more validation.
navinkarkera
force-pushed
the
navin/add-collections-to-lib-block
branch
2 times, most recently
from
fd379a0
to
d911d3c
Compare
There was a problem hiding this comment.
👍 This works perfectly @navinkarkera . I've left an open question about the collection_keys/usage_keys param, but that doesn't have to block merging here.
- I tested this while testing feat: collections page [FC-0062] frontend-app-authoring#1281
- I read through the code
-
I checked for accessibility issuesN/A - Includes documentation in code comments
-
User-facing strings are extracted for translationN/A
| @@ -1185,6 +1197,8 @@ def update_library_collection_components( | |||
| remove=False, | |||
| # As an optimization, callers may pass in a pre-fetched ContentLibrary instance | |||
| content_library: ContentLibrary | None = None, | |||
| # Similarly, users may pass component_keys instead of usage_keys | |||
| component_keys: list[str] | None = None, | |||
There was a problem hiding this comment.
Well, we can generate the LibraryUsageLocator keys from component_keys since we have the base library-key, e.g.
usage_key = LibraryUsageLocator(library_key, component.component_type.name, component.local_key)What do you think? Is it too much to check that either a) component_keys + no usage_keys were passed in or b) the usage_keys provided match the component_keys provided?
| """ | ||
| Create a new XBlock in this library of the specified type (e.g. "html"). | ||
|
|
||
| Optionally associates the newly created xblock with given collection_key. |
There was a problem hiding this comment.
Q: Is it important to assign a collection and create a block in a single API call, as opposed to two API calls? We don't have an API to set any other things during creation, like tags, display name, content, etc. - those are done by follow-up calls. Is there something special about collections?
There was a problem hiding this comment.
That's a good point -- there's nothing special about collections or how they're indexed that requires doing this in a single step.
We'd still need the filterable fields update, but not the rest of this PR.. @navinkarkera what do you think?
There was a problem hiding this comment.
@bradenmacdonald, @pomegranited Yes, that makes sense. I have removed the commit which added collections during creation and updated frontend PR to make an additional api call.
There was a problem hiding this comment.
Thanks! I definitely feel better keeping it simple for now.
navinkarkera
force-pushed
the
navin/add-collections-to-lib-block
branch
from
d911d3c
to
610aaf3
Compare
navinkarkera
changed the title
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
Adds block_id field to filterable attributes of meilisearch allowing us to fetch a single block/collection from index under a library (
context_key).Supporting information
Testing instructions
Follow instructions in openedx/frontend-app-authoring#1281
Deadline
"None" if there's no rush, or provide a specific date or event (and reason) if there is one.
Other information
Include anything else that will help reviewers and consumers understand the change.