Skip to content
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

feat: export tagged library as csv (TEMP) #628

Closed
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
f84abd0
feat: export tagged library as csv
rpenido Feb 6, 2024
06810a2
Merge branch 'rpenido/fal-3610-download-course-tag-spreadsheet' into …
rpenido Feb 9, 2024
f750076
Merge branch 'rpenido/fal-3610-download-course-tag-spreadsheet' into …
rpenido Feb 10, 2024
f637076
Merge branch 'rpenido/fal-3610-download-course-tag-spreadsheet' into …
rpenido Feb 15, 2024
3cf462f
fix: rebasing
rpenido Feb 15, 2024
aa93510
fix: rename variable to fix typing
rpenido Feb 15, 2024
e94131f
fix: pylint
rpenido Feb 15, 2024
9a1729d
Merge branch 'rpenido/fal-3610-download-course-tag-spreadsheet' into …
rpenido Feb 15, 2024
6518a91
refactor: changing condition checking
rpenido Feb 15, 2024
8ddf875
fix: check invalidkey
rpenido Feb 15, 2024
663feed
feat: Upgrade Python dependency ora2 (#34244)
github-actions[bot] Feb 15, 2024
95b3e88
temp: add supplemental logging to debug IDV issues (#34248)
MichaelRoytman Feb 16, 2024
ed547be
fix: better error handling
rpenido Feb 16, 2024
df06d9a
docs: reverting unintended change
rpenido Feb 16, 2024
a153ec9
refactor: change functions to private
rpenido Feb 16, 2024
610c01a
refactor: cleaning build_object_tree_with_objecttags function
rpenido Feb 16, 2024
4012481
fix: explicit typing
rpenido Feb 16, 2024
4d1d82d
feat: export all course tags as csv (#34091)
rpenido Feb 16, 2024
6353bb2
feat: make FA form error messaging more descript (#34247)
christopappas Feb 16, 2024
4ad87c2
Merge branch 'master' into rpenido/fal-3611-download-library-tag-spre…
rpenido Feb 16, 2024
5caf952
fix: cleaning some types
rpenido Feb 16, 2024
df13345
docs: fix comment
rpenido Feb 16, 2024
b6366b6
perf: reduce number of queries for content tagging endpoints (#34200)
pomegranited Feb 16, 2024
e6a0dce
Merge branch 'master' into rpenido/fal-3611-download-library-tag-spre…
rpenido Feb 16, 2024
aa38f08
fix: typo
rpenido Feb 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions openedx/core/djangoapps/content_tagging/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@

import openedx_tagging.core.tagging.api as oel_tagging
from django.db.models import Exists, OuterRef, Q, QuerySet
from opaque_keys.edx.keys import CourseKey, LearningContextKey
from opaque_keys.edx.keys import CourseKey
from opaque_keys.edx.locator import LibraryLocatorV2
from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy
from organizations.models import Organization

Expand Down Expand Up @@ -131,24 +132,25 @@ def get_unassigned_taxonomies(enabled=True) -> QuerySet:


def get_all_object_tags(
content_key: LearningContextKey,
content_key: LibraryLocatorV2 | CourseKey,
) -> tuple[ObjectTagByObjectIdDict, TaxonomyDict]:
"""
Returns a tuple with a dictionary of grouped object tags for all blocks and a dictionary of taxonomies.
"""
# ToDo: Add support for other content types (like LibraryContent and LibraryBlock)
context_key_str = str(content_key)
# We use a block_id_prefix (i.e. the modified course id) to get the tags for the children of the Content
# (course/library) in a single db query.
if isinstance(content_key, CourseKey):
course_key_str = str(content_key)
# We use a block_id_prefix (i.e. the modified course id) to get the tags for the children of the Content
# (course) in a single db query.
block_id_prefix = course_key_str.replace("course-v1:", "block-v1:", 1)
block_id_prefix = context_key_str.replace("course-v1:", "block-v1:", 1)
elif isinstance(content_key, LibraryLocatorV2):
block_id_prefix = context_key_str.replace("lib:", "lb:", 1)
else:
raise NotImplementedError(f"Invalid content_key: {type(content_key)} -> {content_key}")

# There is no API method in oel_tagging.api that does this yet,
# so for now we have to build the ORM query directly.
all_object_tags = list(ObjectTag.objects.filter(
Q(object_id__startswith=block_id_prefix) | Q(object_id=course_key_str),
Q(object_id__startswith=block_id_prefix) | Q(object_id=content_key),
Q(tag__isnull=False, tag__taxonomy__isnull=False),
).select_related("tag__taxonomy"))

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,12 @@
from typing import Iterator

from attrs import define
from opaque_keys.edx.keys import CourseKey, LearningContextKey
from opaque_keys.edx.keys import CourseKey, UsageKey
from opaque_keys.edx.locator import LibraryLocatorV2
from xblock.core import XBlock

import openedx.core.djangoapps.content_libraries.api as library_api
from openedx.core.djangoapps.content_libraries.api import LibraryXBlockMetadata
from xmodule.modulestore.django import modulestore

from ...types import ObjectTagByObjectIdDict, ObjectTagByTaxonomyIdDict
Expand Down Expand Up @@ -38,51 +42,136 @@ def iterate_with_level(
yield from iterate_with_level(child, level + 1)


def build_object_tree_with_objecttags(
content_key: LearningContextKey,
object_tag_cache: ObjectTagByObjectIdDict,
) -> TaggedContent:
def get_course_tagged_object_and_children(
pomegranited marked this conversation as resolved.
Show resolved Hide resolved
course_key: CourseKey, object_tag_cache: ObjectTagByObjectIdDict
) -> tuple[TaggedContent, list[XBlock]]:
"""
Returns the object with the tags associated with it.
Returns a TaggedContent with course metadata with its tags, and its children.
"""
store = modulestore()

if isinstance(content_key, CourseKey):
course = store.get_course(content_key)
if course is None:
raise ValueError(f"Course not found: {content_key}")
else:
raise NotImplementedError(f"Invalid content_key: {type(content_key)} -> {content_key}")

display_name = course.display_name_with_default
course_id = str(course.id)
course = store.get_course(course_key)
assert course is not None
pomegranited marked this conversation as resolved.
Show resolved Hide resolved
course_id = str(course_key)

tagged_course = TaggedContent(
display_name=display_name,
display_name=course.display_name_with_default,
block_id=course_id,
category=course.category,
object_tags=object_tag_cache.get(str(content_key), {}),
object_tags=object_tag_cache.get(course_id, {}),
children=None,
)

return tagged_course, course.children if course.has_children else []


def get_library_tagged_object_and_children(
library_key: LibraryLocatorV2, object_tag_cache: ObjectTagByObjectIdDict
) -> tuple[TaggedContent, list[LibraryXBlockMetadata]]:
"""
Returns a TaggedContent with library metadata with its tags, and its children.
"""
library = library_api.get_library(library_key)
assert library is not None
pomegranited marked this conversation as resolved.
Show resolved Hide resolved

library_id = str(library_key)

tagged_library = TaggedContent(
display_name=library.title,
block_id=library_id,
category='library',
object_tags=object_tag_cache.get(library_id, {}),
children=None,
)

children = library_api.get_library_blocks(library_key)

return tagged_library, children


def get_xblock_tagged_object_and_children(
usage_key: UsageKey, object_tag_cache: ObjectTagByObjectIdDict, store
) -> tuple[TaggedContent, list[XBlock]]:
"""
Returns a TaggedContent with xblock metadata with its tags, and its children.
"""
block = store.get_item(usage_key)
block_id = str(usage_key)
tagged_block = TaggedContent(
display_name=block.display_name_with_default,
block_id=block_id,
category=block.category,
object_tags=object_tag_cache.get(block_id, {}),
children=None,
)

blocks = [(tagged_course, course)]
return tagged_block, block.children if block.has_children else []


def get_library_block_tagged_object(
library_block: LibraryXBlockMetadata, object_tag_cache: ObjectTagByObjectIdDict
) -> tuple[TaggedContent, None]:
"""
Returns a TaggedContent with library content block metadata and its tags,
and 'None' as children.
"""
block_id = str(library_block.usage_key)
tagged_library_block = TaggedContent(
display_name=library_block.display_name,
block_id=block_id,
category=library_block.usage_key.block_type,
object_tags=object_tag_cache.get(block_id, {}),
children=None,
)

return tagged_library_block, None


def build_object_tree_with_objecttags(
content_key: LibraryLocatorV2 | CourseKey,
object_tag_cache: ObjectTagByObjectIdDict,
) -> TaggedContent:
"""
Returns the object with the tags associated with it.
"""
if isinstance(content_key, CourseKey):
tagged_content, children = get_course_tagged_object_and_children(
content_key, object_tag_cache
)
elif isinstance(content_key, LibraryLocatorV2):
tagged_content, children = get_library_tagged_object_and_children(
content_key, object_tag_cache
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up here we can decide which method to use when fetching child blocks, so we don't have to do the type checks in the loop below:

Suggested change
if isinstance(content_key, CourseKey):
tagged_content, children = get_course_tagged_object_and_children(
content_key, object_tag_cache
)
elif isinstance(content_key, LibraryLocatorV2):
tagged_content, children = get_library_tagged_object_and_children(
content_key, object_tag_cache
)
if isinstance(content_key, CourseKey):
tagged_content, children = get_course_tagged_object_and_children(
content_key, object_tag_cache
)
get_tagged_children = get_xblock_tagged_object_and_children
elif isinstance(content_key, LibraryLocatorV2):
tagged_content, children = get_library_tagged_object_and_children(
content_key, object_tag_cache
)
get_tagged_children = get_library_block_tagged_object

But you'll need to remove the store parameter from get_xblock_tagged_object_and_children and just call this again (it's ok to do this, the modulestore is globally cached):

    store = modulestore()

else:
raise NotImplementedError(f"Invalid content_key: {type(content_key)} -> {content_key}")

blocks: list[tuple[TaggedContent, list | None]] = [(tagged_content, children)]

store = modulestore()

while blocks:
tagged_block, xblock = blocks.pop()
tagged_block, block_children = blocks.pop()
tagged_block.children = []

if xblock.has_children:
for child_id in xblock.children:
child_block = store.get_item(child_id)
tagged_child = TaggedContent(
display_name=child_block.display_name_with_default,
block_id=str(child_id),
category=child_block.category,
object_tags=object_tag_cache.get(str(child_id), {}),
children=None,
if not block_children:
continue

for child in block_children:
child_children: list | None

if isinstance(child, UsageKey):
tagged_child, child_children = get_xblock_tagged_object_and_children(
child, object_tag_cache, store
)
elif isinstance(child, LibraryXBlockMetadata):
tagged_child, child_children = get_library_block_tagged_object(
child, object_tag_cache
)
tagged_block.children.append(tagged_child)
else:
raise NotImplementedError(f"Invalid child: {type(child)} -> {child}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we can use the get_tagged_children variable set in my suggestion above:

Suggested change
if isinstance(child, UsageKey):
tagged_child, child_children = get_xblock_tagged_object_and_children(
child, object_tag_cache, store
)
elif isinstance(child, LibraryXBlockMetadata):
tagged_child, child_children = get_library_block_tagged_object(
child, object_tag_cache
)
tagged_block.children.append(tagged_child)
else:
raise NotImplementedError(f"Invalid child: {type(child)} -> {child}")
tagged_child, child_children = get_tagged_children(child, object_tag_cache)

Copy link
Member Author

@rpenido rpenido Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: 610c01a

At first, I thought we would still need a type guard before calling the function, but the types were inferred just right.

The code is way cleaner now! Thank you!

Edit: My lsp didn't found any issues but our static type checker didn't like it. It uses the first variable assignment to define the type:

openedx/core/djangoapps/content_tagging/rest_api/v1/objecttag_export_helpers.py:150: error: Incompatible types in assignment (expression has type "Callable[[LibraryXBlockMetadata, Dict[str, Dict[int, List[Any]]]], Tuple[TaggedContent, None]]", variable has type "Callable[[UsageKey, Dict[str, Dict[int, List[Any]]]], Tuple[TaggedContent, List[Any]]]")  [assignment]

I did an explicit typing here: 4012481
Not pretty, but I think it is better than disable the type checking. Do you know a better way to do this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpenido sorry, I don't know a better way to do this.. but I agree, having an ugly type definition is better than disabling type checking.


tagged_block.children.append(tagged_child)

blocks.append((tagged_child, child_block))
blocks.append((tagged_child, child_children))

return tagged_course
return tagged_content
Loading
Loading