feat: Preview migration api [FC-0114] #37818
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @ChrisChV! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are 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. DetailsWhere 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. |
| unsupportedBlocksChildren = fetch_block_types( | ||
| [ | ||
| f'context_key = "{source_key}"', | ||
| f'breadcrumbs.usage_key IN [{quoted_keys}]' | ||
| ], | ||
| ) | ||
| # Final unsupported blocks count | ||
| unsupported_blocks_count = len(unsupported_blocks) + unsupportedBlocksChildren["estimatedTotalHits"] | ||
| unsupported_percentage = (unsupported_blocks_count / total_blocks) * 100 |
There was a problem hiding this comment.
@ChrisChV As per latest requirements, we need to ignore children counts from the total counts and percentage. So we can skip this part. You'll still need to fetch this data to subtract the children count from total block counts.
See openedx/frontend-app-authoring#2525 (comment) and the related PR: openedx/frontend-app-authoring#2774
navinkarkera
left a comment
navinkarkera
left a comment
There was a problem hiding this comment.
@ChrisChV Looks good!
- I tested this: Played around with the rest api.
- I read through the code
- I checked for accessibility issues
- Includes documentation
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
Looks good; I just have some questions/comments to discuss before we merge.
| account for a more advanced summary. | ||
| """ | ||
| # Get all containers and components from the source key | ||
| blocks = get_all_blocks_from_context(str(source_key), ["block_type", "block_id"]) |
There was a problem hiding this comment.
So far, I think we've tried to avoid depending on Meilisearch for any of the content library python APIs / migration APIs / etc. Meilisearch is used by the frontend to fetch content, but you can use all the new content library APIs without Meilisearch running, if you wanted to. So this marks a change from that, and we should mention in the docstring that this particular API depends on the search index. Since it's just a "preview" API, I think that's fine, but it would be better if it worked without Meilisearch.
That's also a bit problematic because the actual migration doesn't use the search index, so if the search index is out of date, then "preview" won't exactly match the migration.
Usually I would prefer to implement a preview API by using the exact same "migrate" API but without actually making any changes. I guess that's too slow for our purposes here? Or did you go this route because it's easier to get the whole list of IDs for any context this way? Whatever the rationale is, please document it here.
Is there a risk that in the future, we'll make some changes to how the migration API works, and forget to update this preview API, so they'll no longer match?
There was a problem hiding this comment.
I guess that's too slow for our purposes here? Or did you go this route because it's easier to get the whole list of IDs for any context this way?
It was for both reasons. It was easier to replicate the same queries and calculations used on the frontend. And yes, I do have that concern; won't it be very slow to do a "migration" for each preview?
Is there a risk that in the future, we'll make some changes to how the migration API works, and forget to update this preview API, so they'll no longer match?
In this respect, there is indeed a risk. It has already happened with the library block limit.
| Lazily yields all blocks for a given context key using Meilisearch pagination. | ||
| Meilisearch works with limits of 1000 maximum; ensuring we obtain all blocks | ||
| requires making several queries. | ||
| """ |
There was a problem hiding this comment.
| """ | |
| This data may not always be 100% accurate / up to date because it's based | |
| on the search index, so this should only be used for analysis/estimation | |
| purposes. | |
| """ |
|
|
||
| def fetch_block_types(extra_filter: Filter | None = None): | ||
| """ | ||
| Fetch the block types facet distribution for the search results. |
There was a problem hiding this comment.
Please include an example of the return type and how to query for a single course/context in the docstring, because it's not super obvious from the description if you're not used to working with these search APIs.
5 participants
Description
Supporting information
Testing instructions
Deadline
"None"
Other information
N/A