Don't use OLX when copying/duplicating blocks [FC-0049] #34386

pomegranited · 2024-03-19T07:37:40Z

Description

Refactors the block/library copy/paste and duplicate functions to ensure that content tags are copy/pasted/duplicated to use signals/events to trigger copying tags instead of using the OLX as intermediate storage.

Supporting information

Closes openedx/modular-learning#203

Testing instructions

USE latest master:

Create a course with units that contain all the different types of blocks, including a library content block.
Add tags to your units, and to the actual blocks.
Copy the tagged unit to staged content clipboard.

USE this branch:

Run migrations
Paste the block you copied into your course.

Ensure the paste works without errors, and creates the expected content.
Note that no tags will be pasted though.
Copy your tagged unit again.
Paste the tagged unit.

Check that the tags pasted as expected in the parent and all the different types of blocks.
Duplicate a subsection/unit.
Check that the tags duplicated as expected in the parent and child blocks.

Deadline

Need to get this merged before Redwood is cut to avoid OLX incompatibilities in exported courses or staged content.

Private-ref: FAL-3688

openedx-webhooks · 2024-03-19T07:37:44Z

Thanks for the pull request, @pomegranited! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

instead of using TaggedBlockMixin.studio_post_duplicate Adds api.copy_tags method and tests to support this.

to avoid duplicating logic. This required updating get_all_object_tags to accept ordinary ContentKey object_ids, not just Course or Library keys.

openedx/core/djangoapps/content_tagging/handlers.py

Adds a new JSON field to StagedContent to store a dict of serialized object tags from the copied block and its children, if any. These tags are deserialized and applied to the new block(s) on paste. To support this, we had to make 2 changes to XBlockSerializer: * XBlockSerializer now serializes the block's (and it's child block's) tags into a dict, keyed off the block usage key. These serialized tags are stored in StagedContent.tags * XBlockSerializer now inserts a url_name attribute when serializing blocks that don't provide one. When pasting serialized blocks, the url_name is used to reconstruct the original usage keys of any serialized children, so we can identify their tags. Other related changes: * Updates import_staged_content_from_user_clipboard's helper method to reconstruct child usage keys, so we can determine which tags apply to the pasted block(s). * Updates ClipboardPasteTestCase to test copying child tags * Removes TaggedBlockMixin, and moves its object tag serialization logic into content_tagging.api

yusuf-musleh

@pomegranited Great work on this! After looking into the code, it's alot more clear to me why this approach is much cleaner than the previous one storing tags in the OLX.

Just had a small nit (in addition to addressing the failing tests). I've tested out the functionality as described, everything worked really well. I did face an error when I tried duplicating a unit that contained a library block (but I think you mentioned that you're still getting to that), here's the error message to give you some insights:

 2024-03-25 06:18:18,154 ERROR 128 [django.request] [user None] [ip None] log.py:241 - Internal Server Error: /xblock/
 Traceback (most recent call last):
   File "/openedx/edx-platform/xmodule/library_content_block.py", line 185, in source_library_key
     return LibraryLocator.from_string(self.source_library_id)
   File "/openedx/venv/lib/python3.8/site-packages/opaque_keys/__init__.py", line 183, in from_string
     raise InvalidKeyError(cls, serialized)
 opaque_keys.InvalidKeyError: <class 'opaque_keys.edx.locator.LibraryLocator'>: None

 During handling of the above exception, another exception occurred:

 Traceback (most recent call last):
   File "/openedx/venv/lib/python3.8/site-packages/django/core/handlers/exception.py", line 55, in inner
     response = get_response(request)
   File "/openedx/venv/lib/python3.8/site-packages/django/core/handlers/base.py", line 197, in _get_response
     response = wrapped_callback(request, *callback_args, **callback_kwargs)
   File "/openedx/venv/lib/python3.8/site-packages/django/views/decorators/http.py", line 43, in inner
     return func(request, *args, **kwargs)
   File "/openedx/venv/lib/python3.8/site-packages/django/contrib/auth/decorators.py", line 23, in _wrapper_view
     return view_func(request, *args, **kwargs)
   File "/openedx/edx-platform/common/djangoapps/util/json_request.py", line 57, in parse_json_into_request
     return view_function(request, *args, **kwargs)
   File "/openedx/edx-platform/cms/djangoapps/contentstore/views/block.py", line 140, in xblock_handler
     return handle_xblock(request, usage_key_string)
   File "/openedx/edx-platform/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py", line 227, in handle_xblock
     dest_usage_key = duplicate_block(
   File "/openedx/edx-platform/cms/djangoapps/contentstore/utils.py", line 1137, in duplicate_block
     dupe = duplicate_block(dest_block.location, child, user=user, is_child=True)
   File "/openedx/edx-platform/cms/djangoapps/contentstore/utils.py", line 1130, in duplicate_block
     children_handled = dest_block.studio_post_duplicate(store, source_item)
   File "/openedx/edx-platform/xmodule/library_content_block.py", line 599, in studio_post_duplicate
     self._validate_sync_permissions()
   File "/openedx/edx-platform/xmodule/library_content_block.py", line 518, in _validate_sync_permissions
     if self.source_library_key:
   File "/openedx/edx-platform/xmodule/library_content_block.py", line 187, in source_library_key
     return LibraryLocatorV2.from_string(self.source_library_id)
   File "/openedx/venv/lib/python3.8/site-packages/opaque_keys/__init__.py", line 183, in from_string
     raise InvalidKeyError(cls, serialized)
 opaque_keys.InvalidKeyError: <class 'opaque_keys.edx.locator.LibraryLocatorV2'>: None
 [25/Mar/2024 06:18:18] "POST /xblock/ HTTP/1.1" 500 305190

cms/djangoapps/contentstore/helpers.py

to avoid circular imports when using these APIs.

and use taxonomy.id instead of taxonomy.external_id. To achieve this without adding functions to content_tagging.api, these related changes were made: * get_all_object_tags: now returns a list of tag values instead of ObjectTag instances, grouped by object_id and Taxonomy.id * set_object_tags: renamed to `set_all_object_tags`, to clarify its relationship to get_all_object_tags * set_all_object_tags: now accepts a list of tag values grouped by Taxonomy.id, instead of by Taxonomy.external_id * tag_object: added a content_tagging version of this oel_tagging method which checks that the given taxonomy is enabled for the given object_id's org before applying the tag. * types.ObjectTagByObjectIdDict changed to be TagValuesByObjectIdDict, types.ObjectTagByTaxonomyIdDict changed to be TagValuesByTaxonomyIdDict

pomegranited · 2024-03-26T07:04:20Z

Hi @yusuf-musleh , this PR is ready for a full review now.

The error you found happens when the library content block doesn't have its source_library_id set, which users have to do in the Studio/course authoring MFE:

Traceback (most recent call last):
   File "/openedx/edx-platform/xmodule/library_content_block.py", line 185, in source_library_key
     return LibraryLocator.from_string(self.source_library_id)
   File "/openedx/venv/lib/python3.8/site-packages/opaque_keys/__init__.py", line 183, in from_string
     raise InvalidKeyError(cls, serialized)
 opaque_keys.InvalidKeyError: <class 'opaque_keys.edx.locator.LibraryLocator'>: None

The Course Authoring MFE doesn't let me edit content library blocks, so I'm not able to verify that providing a valid library key here fixes the issue.

But I'm pretty sure it doesn't have anything to do with tagging or the changes I've made here? But I can submit another PR to raise a better error here, if it's important to fix right away.

cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py

…blocks

yusuf-musleh

@pomegranited Looks good to me 👍 great work!

The error you found happens when the library content block doesn't have its source_library_id set, which users have to do in the Studio/course authoring MFE

Thanks for looking into it, you're right the error seems unrelated to the changes here, and I think it's fine leaving it for another ticket/PR.

I tested this: I following the testing instructions and confirmed everything is working well as expected
I read through the code
~~I checked for accessibility issues~~
~~Includes documentation~~
~~I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.~~

instead of constructing a CourseKey.

bradenmacdonald

Looks great - just a couple small questions

cms/djangoapps/contentstore/helpers.py

openedx/core/djangoapps/content_staging/models.py

openedx/core/djangoapps/content_tagging/api.py

pomegranited · 2024-03-29T03:07:54Z

Thank you for your review @bradenmacdonald ! I think I've addressed everything, but let me know if not?

openedx-webhooks · 2024-04-02T17:00:00Z

@pomegranited 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2024-04-02T18:10:01Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2024-04-02T18:26:46Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2024-04-02T18:26:46Z

2U Release Notice: This PR has been deployed to the edX production environment.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Mar 19, 2024

pomegranited force-pushed the jill/no-tags-in-olx branch from ff8b6b5 to 9ec8727 Compare March 19, 2024 07:39

pomegranited added 4 commits March 20, 2024 00:05

test: use api.add_tag_to_taxonomy instead of Tag.objects

1e99dd7

refactor: copy tags on XBLOCK_DUPLICATED signal

8b10303

instead of using TaggedBlockMixin.studio_post_duplicate Adds api.copy_tags method and tests to support this.

test: clear rules cache in between test runs

08f0dca

refactor: use get_all_object_tags in copy_object_tags

16192fc

to avoid duplicating logic. This required updating get_all_object_tags to accept ordinary ContentKey object_ids, not just Course or Library keys.

pomegranited force-pushed the jill/no-tags-in-olx branch from 793c7a7 to 261021c Compare March 20, 2024 00:26

pomegranited commented Mar 20, 2024

View reviewed changes

openedx/core/djangoapps/content_tagging/handlers.py Show resolved Hide resolved

pomegranited force-pushed the jill/no-tags-in-olx branch 4 times, most recently from 992187e to 3abe3c3 Compare March 20, 2024 06:11

bradenmacdonald mentioned this pull request Mar 20, 2024

refactor: inheritable authoring mixin callbacks for editing & duplication #33756

Merged

pomegranited force-pushed the jill/no-tags-in-olx branch from 42843b5 to 7702971 Compare March 22, 2024 07:24

pomegranited requested a review from yusuf-musleh March 22, 2024 07:46

yusuf-musleh reviewed Mar 25, 2024

View reviewed changes

cms/djangoapps/contentstore/helpers.py Show resolved Hide resolved

pomegranited added 2 commits March 26, 2024 13:17

refactor: rearrange content_tagging and content_libraries imports

b9d035c

to avoid circular imports when using these APIs.

pomegranited force-pushed the jill/no-tags-in-olx branch from 1a76c60 to 61b3c57 Compare March 26, 2024 06:58

pomegranited marked this pull request as ready for review March 26, 2024 07:00

pomegranited force-pushed the jill/no-tags-in-olx branch from 61b3c57 to c0b1412 Compare March 26, 2024 07:20

pomegranited commented Mar 26, 2024

View reviewed changes

cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py Show resolved Hide resolved

pomegranited added 4 commits March 27, 2024 11:24

Merge branch 'master' into jill/no-tags-in-olx

88d5a86

fix: tests were calling tag_object with UsageKeys, not strings.

c3262e7

feat: expose get_taxonomy_by_export_id from the oel_tagging library.

acb75da

test: test copy/paste tags for discussion, html, and content_library …

b3e65bb

…blocks

pomegranited requested a review from yusuf-musleh March 27, 2024 02:12

yusuf-musleh approved these changes Mar 27, 2024

View reviewed changes

pomegranited requested a review from bradenmacdonald March 27, 2024 03:51

pomegranited added 3 commits March 28, 2024 16:11

fix: use parent_key.context_key.make_usage_key

6c05c6e

instead of constructing a CourseKey.

fix: use django JSONField, not third party package

451b9cd

Merge branch 'master' into jill/no-tags-in-olx

6595da1

bradenmacdonald reviewed Mar 28, 2024

View reviewed changes

cms/djangoapps/contentstore/helpers.py Outdated Show resolved Hide resolved

openedx/core/djangoapps/content_staging/models.py Outdated Show resolved Hide resolved

openedx/core/djangoapps/content_tagging/api.py Show resolved Hide resolved

pomegranited requested a review from bradenmacdonald March 29, 2024 03:08

bradenmacdonald approved these changes Apr 1, 2024

View reviewed changes

bradenmacdonald merged commit 7ad2256 into openedx:master Apr 2, 2024
68 checks passed

bradenmacdonald deleted the jill/no-tags-in-olx branch April 2, 2024 16:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't use OLX when copying/duplicating blocks [FC-0049] #34386

Don't use OLX when copying/duplicating blocks [FC-0049] #34386

pomegranited commented Mar 19, 2024 •

edited

Loading

openedx-webhooks commented Mar 19, 2024 •

edited

Loading

yusuf-musleh left a comment

pomegranited commented Mar 26, 2024 •

edited

Loading

yusuf-musleh left a comment

bradenmacdonald left a comment

pomegranited commented Mar 29, 2024

openedx-webhooks commented Apr 2, 2024

edx-pipeline-bot commented Apr 2, 2024

edx-pipeline-bot commented Apr 2, 2024

edx-pipeline-bot commented Apr 2, 2024

Don't use OLX when copying/duplicating blocks [FC-0049] #34386

Don't use OLX when copying/duplicating blocks [FC-0049] #34386

Conversation

pomegranited commented Mar 19, 2024 • edited Loading

Description

Supporting information

Testing instructions

Deadline

openedx-webhooks commented Mar 19, 2024 • edited Loading

yusuf-musleh left a comment

Choose a reason for hiding this comment

pomegranited commented Mar 26, 2024 • edited Loading

yusuf-musleh left a comment

Choose a reason for hiding this comment

bradenmacdonald left a comment

Choose a reason for hiding this comment

pomegranited commented Mar 29, 2024

openedx-webhooks commented Apr 2, 2024

edx-pipeline-bot commented Apr 2, 2024

edx-pipeline-bot commented Apr 2, 2024

edx-pipeline-bot commented Apr 2, 2024

pomegranited commented Mar 19, 2024 •

edited

Loading

openedx-webhooks commented Mar 19, 2024 •

edited

Loading

pomegranited commented Mar 26, 2024 •

edited

Loading