feat: Legacy Libraries Migration (Prototype) #35758

kdmccormick · 2024-11-01T19:08:18Z

What's ready for feedback

Firstly, the migration data models in openedx/core/djangoapps/content_libraries/models.py

Question: Is the content_libraries app the right place for these? Would they be better in contentstore, since they are CMS-focused?

Question: Alternatively, do we want to generalize these models so that they could be used as a basis for the course migration as well? Imagine, for example:

# split_django_modulestore/models.py
class SplitModulestoreIndexMigration(Model);
    source = ForeignKey(SplitModulestoreCourseIndex)  #  alegacy library for now, eventually courses too
    target = ForeignKey(LearningPackage)
    # Not sure how to capture collections in this version of the model.
    # Perhaps we'd want to supplement this model with the ContentLibraryMigration model.

The new migration Python API function, defined in openedx/core/djangoapps/content_libraries/migration_api.py.

If we move the models to contentstore or split_django_modulestore, we'd move this API function there as well.

Finally, the new upstream link computation for LegacyLibraryContentBlock children, which would allow them to seamlessly switch over to syncing from V2 Libraries once their source is migrated, without any backwards incompatibility, user intervention, or spooky background content changes. Defined incms/lib/xblock/upstream_sync.py and xmodule/library_content_block.py.

This LegacyLibraryContentBlock's source library has been migrated. You can see that the child is now considered to be "Sourced from a library" based on the icon. If I publish an edit to this problem in its new library, instead of seeing the legacy "Update Now" messaging from the LegacyLibraryContentBlock, I'd see the new "Update Available" button on the problem itself (I would show this but my authoring env is currently broken).

What's missing

Optimization. In the PR currently, rendering each LegacyLibraryContentBlock child requires looping through all of the source library's ContentLibraryBlockMigration objects in order to find a key match. This could easily be optimized with one or both of:
- some request-local caching
- a table where we persist upstream-downstream mappings (which I think we need anyway for Teak).
UI for migrating a legacy library, accessible to all library authors with the requisite access.
Shutting off write access to migrated legacy libraries, so that we don't end up with two divergent sources of truth for any library.
Shutting off the ability to reference migrated legacy libraries
Updating migrated LegacyLibraryContentBlocks' UI to match the ItemBankBlock's UI, since a migrated LLCB is essentially just an ItemBankBlock.

Hacky stuff in this PR, for demo purposes

This PR has an admin action on the split_django_modulestore index, and new migrate_legacy_library CMS management command. It could be good to clean one or both of these up and merge them into master so that developers can experiment with the migration. (This would not replace the need for a similar end-user-facing UI in Teak).

This PR also modifies the titles and URL targets of the Legacy Libraries home tab. Clicking on a migrated library will send you to the new library and/or collection within that library. For Teak, you could imagine a nicer-looking version of this, showing users which legacy libraries have been migrated where. Alternatively, we could just hide migrated legacy libraries from this list.

Sandbox env

I'm still getting Meili set up on this...

Link: https://studio.pr-35758-7daceb.sandboxes.opencraft.hosting/
UN/PW: openedx / openedx

Settings

TUTOR_GROVE_COMMON_SETTINGS : |
  SEARCH_ENGINE: "openedx.core.djangoapps.content.search.engine.MeilisearchEngine"

PLUGINS:
- mfe
- grove
- meilisearch
- s3
PLUGIN_INDEXES:
- https://overhang.io/tutor/main

Tutor requirements

git+https://github.com/overhangio/tutor.git@nightly
git+https://github.com/overhangio/tutor-mfe.git@nightly
git+https://gitlab.com/opencraft/dev/tutor-contrib-grove.git@4d4ce5c0e258c95c02abce17cc22138ce678fd22
git+https://github.com/hastexo/[email protected]
git+https://github.com/openedx/[email protected]#egg=tutor-contrib-harmony-plugin&subdirectory=tutor-contrib-harmony-plugin
git+https://github.com/open-craft/tutor-contrib-meilisearch.git@main

open-craft-grove · 2024-11-06T19:05:30Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

Ian2012 · 2024-11-07T12:30:11Z

cms/lib/xblock/upstream_sync.py

+        # If so, then we know that this block was derived from block in a legacy (v1) content library.
+        # Try to get that block's migrated (v2) content library equivalent and use it as our upstream.
+        elif downstream.parent and downstream.parent.block_type == "library_content":
+            from xmodule.library_content_block import LegacyLibraryContentBlock


nit: do we need this import here?

For the type annotation on the next line. Importing at the top of the file would be a cyclical import.

Alternatively, I could just # type: ignore the next line, but I'd rather avoid that.

EDIT: I think I could do this at the top of the file:

if t.TYPE_CHECKING: from xmodule.library_content_block import LegacyLibraryContentBlock

I'll try that when this PR is ready for actual review.

Ian2012 · 2024-11-07T13:31:42Z

I'm in favor of having an explicit table for a downstream - upstream mapping.

Question: Alternatively, do we want to generalize these models so that they could be used as a basis for the course migration as well? Imagine, for example:

As long as the relation is only a mapping between a source - target, I don't see any issues with that.

open-craft-grove · 2024-11-07T16:18:26Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

bradenmacdonald

@kdmccormick Exciting! Thanks for the prototype here.

Question: Is the content_libraries app the right place for these? Would they be better in contentstore, since they are CMS-focused?

Where possible, I do like to keep legacy/transition code separate from new/permanent code. So I'd lean toward openedx/core/djangoapps/content_libraries/migration/* as the home. But split_modulestore_django or contentstore are also fine with me.

These models can be deleted as soon as v1 libraries are fully removed, right?

Alternatively, do we want to generalize these models so that they could be used as a basis for the course migration as well?

We're so far from being able to migrate courses that I don't think we know enough to say what the data model requirements will be. For example, I think we'll have a 1:1 mapping from courses to learning package, but that's not the case for libraries. So I think it's premature to try and share the code. I'm not opposed to putting the library migration tracking model in split_modulestore_django though.

This LegacyLibraryContentBlock's source library has been migrated. You can see that the child is now considered to be "Sourced from a library" based on the icon. If I publish an edit to this problem in its new library, instead of seeing the legacy "Update Now" messaging from the LegacyLibraryContentBlock, I'd see the new "Update Available" button on the problem itself (I would show this but my authoring env is currently broken).

Help me understand here. So, we're letting users migrate libraries one at a time. As they do, if they happen to navigate into a course where a LegacyLibraryContentBlock references that, then our "upstream detection" code will treat them the same way as children of Problem Banks (that is, they are downstream copies from an upstream v2 library). But we haven't actually modified the LegacyLibraryContentBlock nor its children yet at this stage. So what happens if I edit the settings of the LegacyLibraryContentBlock , say increasing the number of children to display. Or what if I want to choose a different (v2) library/collection or anything else. Will I have to delete the LegacyLibraryContentBlock and manually replace it with a Problem Bank?

I guess what I'm asking is if we should instead scan for relevant LegacyLibraryContentBlocks and convert them to Problem Banks as an explicit migration process instead of having to handle these various hybrid half-migrated situations as they arise.

Timing

Oh, and please don't merge this PR anytime soon, while we're still in bugfix mode for Sumac :)

bradenmacdonald · 2024-11-07T21:42:13Z

xmodule/library_content_block.py

+
+        # In order identify the new v2 library block, we need to know the v1 library block that this child came from.
+        # Unfortunately, there's no straightforward mapping from these children back to their v1 library source blocks.
+        # (ModuleStore does have a get_original_usage function that inspects edit_info, but we can't count on


Suggested change

# (ModuleStore does have a get_original_usage function that inspects edit_info, but we can't count on

# (ModuleStore does have a get_block_original_usage function that inspects edit_info, but we can't count on

bradenmacdonald · 2024-11-07T21:43:24Z

xmodule/library_content_block.py

+        # (ModuleStore does have a get_original_usage function that inspects edit_info, but we can't count on
+        #  that always working, particularly if this block's course was imported from another instance.)
+        # However, we can work around this by just looping through every block in the legacy library, and testing to see
+        # if it's our source block.


Can't we at least start with get_block_original_usage and fall back to this loop if it doesn't work?

+1, but beware that at least one invocation of get_block_original_usage has cleanup code that implies it can pass back a versioned UsageKey:

edx-platform/xmodule/library_content_block.py

Lines 408 to 411 in d197077

orig_key, orig_version = self.runtime.modulestore.get_block_original_usage(usage_key)

return {

"usage_key": str(usage_key),

"original_usage_key": str(orig_key.replace(version=None, branch=None)) if orig_key else None,

Good call, and noted

ormsbee

This is really exciting stuff! 😁

ormsbee · 2024-11-09T18:59:25Z

openedx/core/djangoapps/content_libraries/models.py

+    """
+    source_key = LearningContextKeyField(unique=True, max_length=255)
+    target = models.ForeignKey(ContentLibrary, on_delete=models.CASCADE)
+    target_collection = models.ForeignKey(Collection, on_delete=models.SET_NULL, null=True)


This model encodes some very reasonable lifecycle behavior that is probably worth explicitly calling out in the comments, (e.g. "deleting the v2 library deletes any record of the migration", "collection is nullable because you can migrate and then delete the target collection, without actually deleting the components in that collection", etc.).

ormsbee · 2024-11-09T19:00:06Z

openedx/core/djangoapps/content_libraries/models.py

+
+class ContentLibraryBlockMigration(models.Model):
+    """
+    Record of a legacy (v1) content library block that has been migrated into a new (v) content library block.


Suggested change

Record of a legacy (v1) content library block that has been migrated into a new (v) content library block.

Record of a legacy (v1) content library block that has been migrated into a new (v2) content library block.

openedx/core/djangoapps/content_libraries/models.py

ormsbee · 2024-11-09T22:05:30Z

openedx/core/djangoapps/content_libraries/models.py

+    block_type = models.SlugField()
+    source_block_id = models.SlugField()


This is fine, but I'm curious why you didn't decide to use a UsageKeyField here instead?

Perhaps I was over-normalizing. I was trying make it impossible to encode three invalid cases:

The block type of the source doesn't make the block type of the target.

The LearningContexts of the source doesn't match library_migration.source_key

The LearningContexts of the target doesn't match library_migration.target

Sounds like it would be simpler to make the source a UsageKeyField and the target a Component, plus a mix of a db constraints and app-level validation to avoid those invalid cases.

Dave: Also, this is an edge case, but do we want to explicitly disallow having two separate v1 blocks map to the same v2 block?

Yes, I'll add that constraint.

ormsbee · 2024-11-10T22:34:17Z

xmodule/library_content_block.py

+        if not self.source_library_id:
+            raise NoUpstream()
+        try:
+            source_library_key = self.source_library_key


Nit: I think that properties that can throw exceptions like this are foot-guns waiting to happen. I realize that the source_library_key property method precedes this PR, so take this as a purely optional item.

ormsbee · 2024-11-11T00:08:22Z

xmodule/library_content_block.py

+        # (ModuleStore does have a get_original_usage function that inspects edit_info, but we can't count on
+        #  that always working, particularly if this block's course was imported from another instance.)
+        # However, we can work around this by just looping through every block in the legacy library, and testing to see
+        # if it's our source block.


+1, but beware that at least one invocation of get_block_original_usage has cleanup code that implies it can pass back a versioned UsageKey:

edx-platform/xmodule/library_content_block.py

Lines 408 to 411 in d197077

orig_key, orig_version = self.runtime.modulestore.get_block_original_usage(usage_key)

return {

"usage_key": str(usage_key),

"original_usage_key": str(orig_key.replace(version=None, branch=None)) if orig_key else None,

kdmccormick · 2025-01-08T20:56:00Z

Thanks all for your feedback. To avoid spamming you, I'm going to close this PR and re-open a new one (which I will factor your feedback into). I'd like to implement this on top of the upstream-downstream link models that @navinkarkera is taking the lead on currently, so the migration blocked by: openedx/modular-learning#242

kdmccormick force-pushed the kdmccormick/library-migration branch 2 times, most recently from 22c4880 to 6fb0955 Compare November 4, 2024 21:39

kdmccormick changed the title ~~feat: Backend for Migrating Legacy Libraries (wip)~~ feat: Legacy Libraries Migration (Prototype) Nov 4, 2024

kdmccormick force-pushed the kdmccormick/library-migration branch 2 times, most recently from 664021b to 4252351 Compare November 6, 2024 18:06

kdmccormick added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Nov 6, 2024

kdmccormick force-pushed the kdmccormick/library-migration branch from 4252351 to 985a912 Compare November 6, 2024 19:26

kdmccormick requested review from bradenmacdonald and ormsbee November 6, 2024 20:17

Ian2012 reviewed Nov 7, 2024

View reviewed changes

feat: Backend for Migrating Legacy Libraries (wip)

2d66c67

kdmccormick force-pushed the kdmccormick/library-migration branch from 985a912 to 2d66c67 Compare November 7, 2024 15:37

bradenmacdonald reviewed Nov 7, 2024

View reviewed changes

ormsbee reviewed Nov 11, 2024

View reviewed changes

kdmccormick closed this Jan 8, 2025

kdmccormick deleted the kdmccormick/library-migration branch January 8, 2025 20:56

kdmccormick restored the kdmccormick/library-migration branch January 8, 2025 20:56

kdmccormick mentioned this pull request Jan 8, 2025

feat: Legacy Libraries Migration (WIP) #36083

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Legacy Libraries Migration (Prototype) #35758

feat: Legacy Libraries Migration (Prototype) #35758

kdmccormick commented Nov 1, 2024 •

edited

Loading

open-craft-grove commented Nov 6, 2024

Ian2012 Nov 7, 2024

kdmccormick Nov 7, 2024 •

edited

Loading

Ian2012 commented Nov 7, 2024

open-craft-grove commented Nov 7, 2024

bradenmacdonald left a comment •

edited

Loading

bradenmacdonald Nov 7, 2024

bradenmacdonald Nov 7, 2024

ormsbee Nov 11, 2024

kdmccormick Jan 8, 2025

ormsbee left a comment

ormsbee Nov 9, 2024

kdmccormick Jan 8, 2025

ormsbee Nov 9, 2024 •

edited

Loading

ormsbee Nov 9, 2024

kdmccormick Jan 8, 2025

kdmccormick Jan 8, 2025

ormsbee Nov 10, 2024

ormsbee Nov 11, 2024

kdmccormick commented Jan 8, 2025 •

edited

Loading

	# (ModuleStore does have a get_original_usage function that inspects edit_info, but we can't count on
	# (ModuleStore does have a get_block_original_usage function that inspects edit_info, but we can't count on

	orig_key, orig_version = self.runtime.modulestore.get_block_original_usage(usage_key)
	return {
	"usage_key": str(usage_key),
	"original_usage_key": str(orig_key.replace(version=None, branch=None)) if orig_key else None,

	Record of a legacy (v1) content library block that has been migrated into a new (v) content library block.
	Record of a legacy (v1) content library block that has been migrated into a new (v2) content library block.

		block_type = models.SlugField()
		source_block_id = models.SlugField()

feat: Legacy Libraries Migration (Prototype) #35758

feat: Legacy Libraries Migration (Prototype) #35758

Conversation

kdmccormick commented Nov 1, 2024 • edited Loading

What's ready for feedback

What's missing

Hacky stuff in this PR, for demo purposes

Sandbox env

open-craft-grove commented Nov 6, 2024

Choose a reason for hiding this comment

kdmccormick Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Ian2012 commented Nov 7, 2024

open-craft-grove commented Nov 7, 2024

bradenmacdonald left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormsbee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormsbee Nov 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdmccormick commented Jan 8, 2025 • edited Loading

kdmccormick commented Nov 1, 2024 •

edited

Loading

kdmccormick Nov 7, 2024 •

edited

Loading

bradenmacdonald left a comment •

edited

Loading

ormsbee Nov 9, 2024 •

edited

Loading

kdmccormick commented Jan 8, 2025 •

edited

Loading