feat: adds api to retrieve library block/container hierarchy [FC-0090] #36813
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. |
ChrisChV
force-pushed
the
chris/FAL-4180-sections-subsections-publish
branch
from
984a830 to
f2c339f
Compare
| # Removing the unit with components because the components (children of children) are not published. | ||
| # If the unit is kept, the subsection continues to have changes even after it is published. | ||
| self._remove_container_children( | ||
| self.subsection_with_units["id"], | ||
| children_ids=[ | ||
| self.unit_with_components["id"], | ||
| ] | ||
| ) |
There was a problem hiding this comment.
@bradenmacdonald @ormsbee Currently, when publishing a container, the children of the children aren't published. In the case of a subsection, the components of a unit that is a child of a subsection aren't published when the subsection is published. This is something that needs to be fixed, right?
There was a problem hiding this comment.
Yeah, I have a fairly major revamp of openedx/openedx-learning#307 that I want to get into reviewable shape next week. My hope is that the new models I'm introducing (openedx/openedx-learning#317) will make this more straightforward.
…ions-subsections-publish
in anticipation of publishing container children
These are really high, but highlight the need for future optimizations.
openedx/core/djangoapps/content_libraries/tests/test_containers.py
Outdated
Show resolved
Hide resolved
| # Fill in hierarchy up through parents | ||
| while level := hierarchy.parent_level(level): | ||
| items = list(_get_containers_with_entities(items).all()) | ||
| hierarchy.append(level, *items) |
There was a problem hiding this comment.
No action needed: Via openedx/openedx-learning#316 we will soon be adding a new OutlineRoot container type that's used for courses and which can have child containers of any type, including a mix. I don't think this code will need to support that (since they're only for courses not libraries), but thought I should mention it, especially if we ever move this "get hierarchy" functionality into learning core instead of just keeping it in content_libraries.
There was a problem hiding this comment.
We might have to move this into openedx-learning anyway to optimize the queries, so I'll keep this in mind.
pomegranited
changed the title
pomegranited
changed the title
…ions-subsections-publish
Required a refactor of the approach to avoid using the Metadata classes.
…ions-subsections-publish
pomegranited
force-pushed
the
chris/FAL-4180-sections-subsections-publish
branch
from
6fedfbb to
195c73e
Compare
|
@ormsbee Could you please look at this PR together with openedx/openedx-learning#333 (and openedx/frontend-app-authoring#2186 if you want a UI for testing), and let me know your thoughts about the performance and the high query count? As we discussed, it's working fine - just need a read on whether we're going to have perf issues bite us in the future if we merge it like this. |
rpenido
force-pushed
the
chris/FAL-4180-sections-subsections-publish
branch
from
131b00a to
b2a0cd1
Compare
rpenido
force-pushed
the
chris/FAL-4180-sections-subsections-publish
branch
3 times, most recently
from
f6f32c0 to
9d5805d
Compare
rpenido
force-pushed
the
chris/FAL-4180-sections-subsections-publish
branch
from
9d5805d to
addb0b5
Compare
|
Hi @bradenmacdonald! We have some duplicated queries, but I couldn't determine the cause after some time debugging. 😞 Should we merge it, or should I spend more time trying to find the culprit? |
|
@ChrisChV Can you move this from |
|
@rpenido I think it's fine for now. Let's merge it and we can keep an eye on the performance. Earlier testing on the sandbox seemed to show it was still quite fast. And we do have an eventual plan to do much more aggressive optimization of the performance by caching the whole course tree in learning core. |
ChrisChV
left a comment
ChrisChV
left a comment
There was a problem hiding this comment.
Looks good @rpenido Great! Thanks for this work! 👍
- I tested this: I followed the testing instructions in openedx/frontend-app-authoring#2186
- I read through the code and considered the security, stability and performance implications of the changes.
- I tested that the UI can be used with a keyboard only (tab order, keyboard controls).
- Includes tests for bugfixes and/or features added.
- Includes documentation
|
@bradenmacdonald could you review and merge this as CC? Since I created the PR, it needs another CC. |
|
@ChrisChV Sure! Could you or @rpenido just add something to the |
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
Conditional approval, if you mark the APIs as unstable and rename this one parameter. Otherwise looks good - thanks!
openedx/core/djangoapps/content_libraries/api/container_metadata.py
Outdated
Show resolved
Hide resolved
|
@bradenmacdonald I updated the docstring here: 1934a7d |
rpenido
force-pushed
the
chris/FAL-4180-sections-subsections-publish
branch
from
949592e to
1934a7d
Compare
bradenmacdonald
deleted the
chris/FAL-4180-sections-subsections-publish
branch
github-project-automation
bot
moved this from Waiting on Author
to Done
in Contributions
|
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. |
* test: Test for publish section/subsection * test: published_by is now None for unpublished containers * test: adds TODO comments to the tests in anticipation of publishing container children * feat: adds api to retrieve library block/container hierarchy * test: adds query counts for hierarchy API tests These are really high, but highlight the need for future optimizations. * perf: reduce hierarchy API query counts * perf: cut query counts in half Required a refactor of the approach to avoid using the Metadata classes. * chore: trigger ci * chore: update openedx-learning constraint * chore: compile requirements * test: updating query count * style: Add missing comment in kernel.in * fix: get_container_from_key param and comments * docs: mark api as UNSTABLE and add comment about get_library_object_hierarchy implementation --------- Co-authored-by: Jillian Vogel <jill@opencraft.com> Co-authored-by: Rômulo Penido <romulo.penido@gmail.com>
7 participants
Description
Noneas default ofpublished_byinContainerMetadatato match the same behaviour inLibraryXBlockMetadata_remove_container_componentsto_remove_container_children_patch_container_componentsto_patch_container_childrenSupporting information
Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.
Depends on: openedx/openedx-learning#333
Testing instructions
See openedx/frontend-app-authoring#2186
Deadline
None