Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 10 additions & 2 deletions cms/djangoapps/contentstore/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -2340,12 +2340,20 @@ def _cancel_old_tasks(course_key: str, user: User, ignore_task_ids: list[str]):


@shared_task(base=LegacyLibraryContentToItemBank, bind=True)
def migrate_course_legacy_library_blocks_to_item_bank(self, user_id: int, course_key: str):
def migrate_course_legacy_library_blocks_to_item_bank(
self, user_id: int, course_key: str, persist_publish_state: bool = False,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what code would ever specify persist_publish_state = True?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The idea is to provide a management command to do this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ah, would the management command be an alternative to migrating the legacy library references from within the Studio UI?

could you describe the proposed CLI a bit more?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

When I explored the code around migration, I saw replace_v1_lib_refs_with_v2_in_courses. It looked like it should do what the migration API does when we call from the authoring MFE, but this command replaces the existing field references with V2, and that field is not even compatible with V2 Library locator. I don't know why it still exists there.

Then I found the API, and it had this publishing issue. My idea is to keep the current API as is (Keep the blocks in draft). Create a new command that does the same but also provides an optional argument to publish the blocks using this flag. So, for people who really know what they are doing, they can use this command.

Another option is to maybe give another button in the authoring MFE that hits the same API but also publishes the blocks using this flag.

Let me know what you think of the 2 options, and I can create a follow-up PRs or maybe if we go with the command then I can add it in this PR.

):
"""
Migrate legacy course library blocks to Item Bank.

Depending on the number of blocks and its children blocks this operation can take a significant
amount of time and this is why it is run as a celery task.

Arguments:
user_id: id of the user performing the migration.
course_key: the course whose legacy library content blocks should be migrated.
persist_publish_state: if True, blocks that were published before the migration are
re-published afterwards. Defaults to False, leaving migrated blocks as drafts.
"""
ensure_cms("Legacy library content references may only be executed in CMS")
set_code_owner_attribute_from_module(__name__)
Expand All @@ -2363,7 +2371,7 @@ def migrate_course_legacy_library_blocks_to_item_bank(self, user_id: int, course
with store.bulk_operations(key):
for block in blocks:
self.status.set_state(f'Migrating block: {block.usage_key}')
block.v2_update_children_upstream_version(user_id)
block.v2_update_children_upstream_version(user_id, persist_publish_state)
except Exception as exc: # pylint: disable=broad-except
LOGGER.exception(f'Error while migrating blocks: {exc}')
self.status.fail(str(exc))
8 changes: 7 additions & 1 deletion xmodule/library_content_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -329,14 +329,18 @@ def studio_post_paste(self, store, source_node) -> bool:
self.sync_from_library(upgrade_to_latest=False)
return True # Children have been handled

def v2_update_children_upstream_version(self, user_id=None):
def v2_update_children_upstream_version(self, user_id=None, persist_publish_state=False):
"""
Update the upstream and upstream version fields of all children to point to library v2 version of the legacy
library blocks. This essentially converts this legacy block to new ItemBankBlock.

If `persist_publish_state` is True, and this block was published prior to the migration, it is
re-published afterwards so that the upstream/upstream_version changes reach LMS.
"""
from cms.djangoapps.modulestore_migrator import api as migrator_api
store = modulestore()
with store.bulk_operations(self.course_id):
was_published = persist_publish_state and not store.has_changes(self)
children = self.get_children()
# These are the v1 library item upstream UsageKeys
child_old_upstream_keys = [
Expand All @@ -358,6 +362,8 @@ def v2_update_children_upstream_version(self, user_id=None):
self.is_migrated_to_v2 = True
self.save()
store.update_item(self, user_id)
if was_published:
store.publish(self.location, user_id)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ormsbee Is there prior art for publishing a single course component rather than a unit?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The only reason behind not publishing a unit was that a unit can contain draft and published blocks.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not that I'm aware of, though I think it should work. We do auto-publish some standalone HTMLBlock sub-types like for course-tab content.


def _validate_library_version(self, validation, lib_tools, version, library_key):
"""
Expand Down
66 changes: 66 additions & 0 deletions xmodule/tests/test_library_content.py
Original file line number Diff line number Diff line change
Expand Up @@ -826,3 +826,69 @@ def test_author_view(self):
assert '<li>html 2</li>' in rendered.content
assert '<li>html 3</li>' in rendered.content
assert '<li>html 4</li>' in rendered.content


@ddt.ddt
class TestLegacyLibraryContentBlockMigrationPublishing(LegacyLibraryContentTest):
"""
Unit tests for the `persist_publish_state` flag of
LegacyLibraryContentBlock.v2_update_children_upstream_version.
"""

def setUp(self):
from cms.djangoapps.modulestore_migrator import api
from cms.djangoapps.modulestore_migrator.data import CompositionLevel, RepeatHandlingStrategy
super().setUp()
user = UserFactory()
self._sync_lc_block_from_library()
self.organization = OrganizationFactory(short_name="myorg")
lib_api.create_library(
org=self.organization,
slug="mylib",
title="My Test V2 Library",
)
self.library_v2 = lib_api.ContentLibrary.objects.get(slug="mylib")
api.start_migration_to_library(
user=user,
source_key=self.library.location.library_key,
target_library_key=self.library_v2.library_key,
target_collection_slug=None,
composition_level=CompositionLevel.Component,
repeat_handling_strategy=RepeatHandlingStrategy.Skip,
preserve_url_slugs=True,
forward_source_to_target=True,
)

@ddt.data(
# Published before migration, flag True: re-published with the migration reflected.
(True, True),
# Published before migration, flag False (default): published branch is left untouched.
(True, False),
# Never published before migration, flag True: stays unpublished.
(False, True),
)
@ddt.unpack
def test_persist_publish_state(self, was_published_before, persist_publish_state):
"""
Tests the `persist_publish_state` flag of `v2_update_children_upstream_version` under
the various combinations of prior publish state and flag value.
"""
if was_published_before:
self.store.publish(self.course.location, self.user_id)

self.lc_block.v2_update_children_upstream_version(
self.user_id, persist_publish_state=persist_publish_state,
)

if was_published_before and persist_publish_state:
with self.store.branch_setting(ModuleStoreEnum.Branch.published_only):
published_block = self.store.get_item(self.lc_block.location)
assert published_block.is_migrated_to_v2 is True
assert published_block.get_children()[0].upstream == "lb:myorg:mylib:html:html_1"
elif was_published_before and not persist_publish_state:
with self.store.branch_setting(ModuleStoreEnum.Branch.published_only):
published_block = self.store.get_item(self.lc_block.location)
# The published version still reflects the pre-migration state.
assert published_block.is_migrated_to_v2 is False
else:
assert not self.store.has_published_version(self.store.get_item(self.lc_block.location))
Loading