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. 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. |
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
|
@bradenmacdonald and @kdmccormick: this touches on what we were talking about yesterday with hierarchy modeling: High level thoughts:
So a class PublishedContainerPath(models.Model):
# level_0 is the lowest/leaf layer, e.g. the Component.
# This is a DAG, so we can have many duplicates of level_0_entity, and the
# only UNIQUE restriction would be on the entire row, i.e. exact duplicates
# of everything would be a race condition. Even having the same entity at
# multiple versions might be allowable for pinned child references in the
# future.
#
# It feels unintuitive to model the hierarchy from bottom-up like this, but
# as Braden pointed out, this would let us avoid the issue of having Units
# show up in multiple columns, since a Unit would always be at level_1 in
# this type of model. Some containers could show up at multiple levels
# though, like OutlineRoot in the form that we've most recently discussed.
level_0_entity = models.ForeignKey(models.PublishableEntity)
level_0_version = models.ForeignKey(models.PublishableEntityVersion, null=True)
level_0_order = models.PositiveSmallInteger(null=True) # or PositiveInteger?
# Continue this pattern for at least Component -> Unit -> Subsection ->
# Section -> OutlineRoot (0-4), but probably at least to 6 to give a little
# headroom?
level_1_entity = models.ForeignKey(models.PublishableEntity)
level_1_version = models.ForeignKey(models.PublishableEntityVersion, null=True)
level_1_order = models.PositiveSmallInteger(null=True)
# highest level doesn't have an ordering (because it's never a peer child
# of anything)
level_6_entity = models.ForeignKey(models.PublishableEntity)
level_6_version = models.ForeignKey(models.PublishableEntityVersion, null=True)
# This is going to be very index heavy, but hopefully the table itself
# doesn't grow too large if we constrain it to only show published and draft.
# We can do incremental updates as drafts are updated, i.e. we should never
# be rewriting a whole course's hierarchy when a single subsection changes.
# ForeignKeys mean implicit indexes already, so we can do searches for
# individual containers easily enough. We'd need to add indexes for
# efficient ordered traversal, which in this model would be a composite
# index with ordering, e.g. (level_6_entity, level_5_order, level_5_entity,
# etc.)
This model isn't fully baked, but I think the basic idea is workable. Something flat that represents hierarchy, is easily filterable, and where callers can efficiently tack joins onto the queryset to add whatever data they think is relevant. I think that we can potentially do some really good caching once I can land the publishing/draft dependency stuff, but that the caching would mostly happen at the edges (e.g. |
|
@ormsbee @kdmccormick I like where that's going!
Are these two statements ^ contradictory, or are you saying "the second-smallest level is always a unit (and so on)" but we don't make any guarantees about containers starting from the largest/root of the hierarchy ?
Do you think we'd have separate ContainerPath tables for draft and published?
I'm wondering though if it could provide better optimization and validation to make the |
My rough thoughts right now are:
I haven't thought about this enough to have firm opinions on any of these.
... I don't know. There's a lot of commonality true, but a unified table is going to have a boolean field in every index. I still feel like they're different models from the point of view of their relationships with other things, but maybe that just really means they're two proxy models on top of the same underlying one. I guess I'd start with seeing how a single table + proxy models would work out. This is likely to be one of the most confusing models we have for people to grok, even with that indirection.
I'm currently going under the assumption that we will have other things at level 0 eventually. Also, using |
|
Random, unbaked thought: In our LC-courses prototype, we mapped course usage keys to publishable entities. I think this is quite reasonable for state management. But one of the other things we use the usage keys for is jump_to links, and right now there's no single identifier for "go to this component in this particular part of the course". Instead, we do some graph traversal to find the left-most appearance and always take you there. Likewise, inheritance has to figure out the authoritative path to this component in order to map out what its attributes are going to be when it's being rendered by itself, with the weird side-effect that a component can seemingly break inheritance rules because you're not looking at it in its first instance in the course. But we could map usage keys to rows in I don't know if this is necessarily a good idea. But it's a dimension of flexibility that we haven't really had before, and it maps pretty squarely onto something like this model. |
|
Idle thought: I wonder if we can omit the publishable entities and only have publishable entity version references in this |
|
@ormsbee Since this is blocking openedx/frontend-app-authoring#2186 , do you think we can proceed with merging it as a provisional/unstable API, and perhaps even keeping the test cases as test code, for when we implement a proper hierarchy retrieval system / |
|
@ChrisChV CC @rpenido I see this PR is still marked as a draft - what's left to do here? Are we just waiting for openedx/openedx-learning#333 ? |
Yes! We can bump the |
rpenido
force-pushed
the
chris/FAL-4180-sections-subsections-publish
branch
4 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. |
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 |
| @@ -145,3 +379,22 @@ def library_container_locator( | |||
| container_type=container_type.value, | |||
| container_id=container.publishable_entity.key, | |||
| ) | |||
|
|
|||
|
|
|||
| def get_container_from_key(container_key: LibraryContainerLocator, isDeleted=False) -> Container: | |||
There was a problem hiding this comment.
| def get_container_from_key(container_key: LibraryContainerLocator, isDeleted=False) -> Container: | |
| def get_container_from_key(container_key: LibraryContainerLocator, is_deleted=False) -> Container: |
Not sure why the linter didn't catch this, but we should use snake_case for parameters
| """ | ||
| Internal method to fetch the Container object from its LibraryContainerLocator | ||
|
|
||
| Raises ContentLibraryContainerNotFound if no container found, or if the container has been soft deleted. |
There was a problem hiding this comment.
This doesn't seem to account for the is_deleted parameter. Also is_deleted is a confusing name. You can pass is_deleted=True but then get a container that is NOT deleted. I think it should be include_deleted and the docstring should say "Raises ContentLibraryContainerNotFound if no container found, or if the container has been soft deleted (unless include_deleted is True)"
There was a problem hiding this comment.
I updated the param name and put a comment to clarify here: 3e969af.
Could you let me know what you think?
|
@bradenmacdonald I updated the docstring here: 1934a7d |
rpenido
force-pushed
the
chris/FAL-4180-sections-subsections-publish
branch
from
949592e to
1934a7d
Compare
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