Repository navigation
feat: store content.child_usage_keys in Container search document [FC-0083] #36528
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
892caa8
15fcff5
0bc1a86
eccc4f4
49557f5
2e7be75
57e0071
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,7 +45,7 @@ def sync_from_upstream_container( | |
| user, | ||
| permission=lib_api.permissions.CAN_VIEW_THIS_CONTENT_LIBRARY, | ||
| ) | ||
| upstream_meta = lib_api.get_container(link.upstream_key, user) | ||
| upstream_meta = lib_api.get_container(link.upstream_key) | ||
| upstream_children = lib_api.get_container_children(link.upstream_key, published=True) | ||
| _update_customizable_fields(upstream=upstream_meta, downstream=downstream, only_fetch=False) | ||
| _update_non_customizable_fields(upstream=upstream_meta, downstream=downstream) | ||
|
|
@@ -54,15 +54,15 @@ def sync_from_upstream_container( | |
| return upstream_children | ||
|
|
||
|
|
||
| def fetch_customizable_fields_from_container(*, downstream: XBlock, user: User) -> None: | ||
| def fetch_customizable_fields_from_container(*, downstream: XBlock) -> None: | ||
| """ | ||
| Fetch upstream-defined value of customizable fields and save them on the downstream. | ||
|
|
||
| The container version only retrieves values from *published* containers. | ||
|
|
||
| Basically, this sets the value of "upstream_display_name" on the downstream block. | ||
| """ | ||
| upstream = lib_api.get_container(LibraryContainerLocator.from_string(downstream.upstream), user) | ||
| upstream = lib_api.get_container(LibraryContainerLocator.from_string(downstream.upstream)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was this even working before?! Thanks for fixing.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank |
||
| _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=True) | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,6 +71,7 @@ class Fields: | |
| # The "content" field is a dictionary of arbitrary data, depending on the block_type. | ||
| # It comes from each XBlock's index_dictionary() method (if present) plus some processing. | ||
| # Text (html) blocks have an "html_content" key in here, capa has "capa_content" and "problem_types", and so on. | ||
| # Containers store their list of child usage keys here. | ||
| content = "content" | ||
|
|
||
| # Collections use this field to communicate how many entities/components they contain. | ||
|
|
@@ -87,6 +88,7 @@ class Fields: | |
| published = "published" | ||
| published_display_name = "display_name" | ||
| published_description = "description" | ||
| published_content = "content" | ||
| published_num_children = "num_children" | ||
|
|
||
| # Note: new fields or values can be added at any time, but if they need to be indexed for filtering or keyword | ||
|
|
@@ -345,13 +347,10 @@ def _collections_for_content_object(object_id: OpaqueKey) -> dict: | |
| collections = authoring_api.get_entity_collections( | ||
| component.learning_package_id, | ||
| component.key, | ||
| ) | ||
| ).values('key', 'title') | ||
| elif isinstance(object_id, LibraryContainerLocator): | ||
| container = lib_api.get_container_from_key(object_id) | ||
| collections = authoring_api.get_entity_collections( | ||
| container.publishable_entity.learning_package_id, | ||
| container.key, | ||
| ) | ||
| container = lib_api.get_container(object_id, include_collections=True) | ||
| collections = container.collections | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a change for this PR: but in the future, Edit: never mind, this just seems to be a problem with the REST API. I don't understand why we have |
||
| else: | ||
| log.warning(f"Unexpected key type for {object_id}") | ||
|
|
||
|
|
@@ -362,8 +361,8 @@ def _collections_for_content_object(object_id: OpaqueKey) -> dict: | |
| return result | ||
|
|
||
| for collection in collections: | ||
| result[Fields.collections][Fields.collections_display_name].append(collection.title) | ||
| result[Fields.collections][Fields.collections_key].append(collection.key) | ||
| result[Fields.collections][Fields.collections_display_name].append(collection["title"]) | ||
| result[Fields.collections][Fields.collections_key].append(collection["key"]) | ||
|
|
||
| return result | ||
|
|
||
|
|
@@ -579,9 +578,13 @@ def searchable_doc_for_container( | |
| container = lib_api.get_container(container_key) | ||
| except lib_api.ContentLibraryContainerNotFound: | ||
| # Container not found, so we can only return the base doc | ||
| log.error(f"Container {container_key} not found") | ||
| return doc | ||
|
|
||
| draft_num_children = lib_api.get_container_children_count(container_key, published=False) | ||
| draft_children = lib_api.get_container_children( | ||
| container_key, | ||
| published=False, | ||
| ) | ||
| publish_status = PublishStatus.published | ||
| if container.last_published is None: | ||
| publish_status = PublishStatus.never | ||
|
|
@@ -592,7 +595,13 @@ def searchable_doc_for_container( | |
| Fields.display_name: container.display_name, | ||
| Fields.created: container.created.timestamp(), | ||
| Fields.modified: container.modified.timestamp(), | ||
| Fields.num_children: draft_num_children, | ||
| Fields.num_children: len(draft_children), | ||
| Fields.content: { | ||
| "child_usage_keys": [ | ||
| str(child.usage_key) | ||
| for child in draft_children | ||
| ], | ||
| }, | ||
| Fields.publish_status: publish_status, | ||
| Fields.last_published: container.last_published.timestamp() if container.last_published else None, | ||
| }) | ||
|
|
@@ -601,10 +610,19 @@ def searchable_doc_for_container( | |
| doc[Fields.breadcrumbs] = [{"display_name": library.title}] | ||
|
|
||
| if container.published_version_num is not None: | ||
| published_num_children = lib_api.get_container_children_count(container_key, published=True) | ||
| published_children = lib_api.get_container_children( | ||
| container_key, | ||
| published=True, | ||
| ) | ||
| doc[Fields.published] = { | ||
| Fields.published_num_children: published_num_children, | ||
| Fields.published_display_name: container.published_display_name, | ||
| Fields.published_num_children: len(published_children), | ||
| Fields.published_content: { | ||
| "child_usage_keys": [ | ||
| str(child.usage_key) | ||
| for child in published_children | ||
| ], | ||
| }, | ||
| } | ||
|
|
||
| return doc | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this working? I would think that below on line 322 you need to set
upstream_container_pkto this value then, notupstream_container. But maybe the Django ORM is sorting it out?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry, I had that on my list to fix before pushing, but forgot.
I've fixed it, but I would have expected tests to fail without this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember seeing a similar thing before where assigning an ID to an object field still worked silently. In any case while I'm testing this I'll make sure the
ContainerLinkis getting created.