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

Conversation

rpenido
Copy link
Member

@rpenido rpenido commented Feb 5, 2024

No description provided.

@rpenido rpenido force-pushed the rpenido/fal-3611-download-library-tag-spreadsheet branch from e6e8868 to f84abd0 Compare February 6, 2024 21:20
@rpenido rpenido changed the title feat: wip feat: export tagged course as csv (TEMP) Feb 6, 2024
@rpenido rpenido changed the title feat: export tagged course as csv (TEMP) feat: export tagged library as csv (TEMP) Feb 8, 2024
@rpenido rpenido marked this pull request as draft February 15, 2024 20:24
rpenido and others added 3 commits February 15, 2024 17:29
Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`

Co-authored-by: jansenk <[email protected]>
Co-authored-by: Jansen Kantor <[email protected]>
Copy link
Member

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

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

Hi @rpenido ! This is working well, but I've suggested a few changes here.

Thank you for splitting this code into an internal PR, it really helped me see what was changed for libraries :)

Comment on lines 137 to 144
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()

Comment on lines 162 to 171
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.

MichaelRoytman and others added 6 commits February 16, 2024 08:48
This commit adds some supplemental, more verbose logging to the results_callback view in the verify_student Djangoapp. This endpoint is called by identity verification (IDV) providers to POST an IDV review to edX.

We are experiencing issues with receiving IDV reviews from our IDV provider, and these logs will help us diagnose whether there is an issue in edX's systems.

These logs will be removed after our investigation is complete.
@rpenido rpenido force-pushed the rpenido/fal-3611-download-library-tag-spreadsheet branch from e2bd601 to 4012481 Compare February 16, 2024 14:53
@rpenido
Copy link
Member Author

rpenido commented Feb 20, 2024

Closed in favor of openedx#34246

@rpenido rpenido closed this Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants