Import v2 libraries content into openedx-learning data models #33577

ormsbee · 2023-10-24T16:07:33Z

Description

Initial code to move v2 libraries content into openedx-learning data models, via a new migrate_lib_to_learning_core management command in the content_libraries app. This will currently import XBlock definition OLX in both the published and draft branches from blockstore. It does not include associated assets at this time.

Blockers to Merging

Merge Various fixes/enhancements to support v2 libraries import openedx-learning#105
Figure out if this is really how we want to handle file storage configuration.

I plan to merge this without associated asset support, which means it won't be ready to fully switch over. The idea I had was that we could convert a library for testing purposes and gradually build out the read/write portions of the XBlock runtime that has to interact with this.

Testing instructions

Run the following command in the Studio/CMS image:

python ./manage.py cms migrate_lib_to_learning_core -f "lib:Axim:200"

Where you replace lib:Axim:200 with the library key for an actual v2 content library on your machine. After that, you can explore the content you imported via the Django admin at http://studio.local.overhang.io:8001/admin/oel_publishing/publishableentity/

Screenshots

…earning models

openedx/core/djangoapps/content_libraries/api.py

openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py

bradenmacdonald

I tried it out on a small v2 library, and it worked great! Code LGTM.

bradenmacdonald · 2023-11-03T17:31:32Z

openedx/core/djangoapps/content_libraries/models.py

+    ContentLibrary first.
+    """
+    content_library = models.OneToOneField(
+        ContentLibrary,


What's the advantage/rationale for having this as a separate model/table, rather than just adding the learning_package foreign key to ContentLibrary directly?

I ask because this code was unclear until I looked up what contents was:

if learning_package_already_exists: lp = lib.contents.learning_package log.info(f"Deleting existing LearningPackage {lp.key} ({lp.uuid})") lib.contents.delete() lp.delete()

@ormsbee I have the same question as @bradenmacdonald --why not add the FK to ContentLibrary directly?

Also, relatedly, what's the desired arity of the ContentLibrary->LearningPackage relationship? If I'm reading this model right, it's N->(0,1), but I'd imagine that the desired arity would be either 1->(0,1) or 1->1.

The ContentLibrary->LearningPackage relationship was something that I went back and forth about when trying to square the idea of multiple v1 libraries being mappable into a single v2 library. But I think I agree with both of you that it should be 1:1 for now, so I'll swap this for a simple ForeignKey.

bradenmacdonald · 2023-11-03T17:38:34Z

openedx/core/djangoapps/content_libraries/management/commands/migrate_lib_to_learning_core.py

+                    # Otherwise, it's just been modified...
+                    else:
+                        component_pk = published_component_pks[file_path]
+                        version_num = 2


It's a little odd to me that we have to carefully manage the version numbers like this, rather than having create_component_version return the new version numbers after success.

Agreed.

@ormsbee I don't want to block this PR, so consider adding something like create_next_component_version to the API in order to smooth out the I-just-want-to-create-the-next-version use case, while leaving create_component_version as-is for advanced cases.

I will, thanks. One of the reasons I kept it explicit is because I wanted to have a pattern where the caller has to explicitly say what version they think it should be in order to catch race conditions with multiple writers and force one of them to figure it out rather than to silently overwrite. But it's just clunky when used in this scenario.

I hadn't thought about race conditions. If you end up deciding to keep it explicit because of those, that sounds good to me too.

ormsbee · 2023-11-04T22:42:18Z

openedx/core/djangoapps/content_libraries/management/commands/migrate_lib_to_learning_core.py

+                    components_api.add_content_to_component_version(
+                        component_version,
+                        raw_content_id=text_content.pk,
+                        key="definition.xml",


@bradenmacdonald: I just copied the blockstore convention of definition.xml here, but I was wondering if I should change it to source.xml because it doesn't really represent the definition scope, and I was wondering if it might cause more confusion than it's worth.

Or some other name? @kdmccormick: any thoughts?

@ormsbee Good point. I like source.xml or block.xml

Thinking on this a little more, source.xml might get confused with Python source code (e.g. if we later allow a src dir in there for more convenient editing, as a sibling directory for static). So with that in mind, I'll go with block.xml.

Thank you!

block.xml sounds great.

kdmccormick

Looking good! A few comments, but my blocking request is the OPENEDX_LEARNING docstring.

I'll test this out on my local tutor this afternoon.

kdmccormick · 2023-11-27T15:49:47Z

cms/envs/common.py

 ]

+OPENEDX_LEARNING = {


Could you slap a setting annotation on this and the LMS setting, even if it's just a big WIP warning with a link to openedx-learning?

kdmccormick · 2023-11-27T16:01:06Z

openedx/core/djangoapps/content_libraries/management/commands/migrate_lib_to_learning_core.py

+            for deleted_definition_file in deleted_definition_files:
+                log.info(f"Deleting {deleted_definition_file} from draft")
+                component_pk = published_component_pks[deleted_definition_file]
+                publishing_api.set_draft_version(component_pk, None)


(openedx-learning API feedback, not blocking for this PR)

I'm enjoying this PR because it's cool seeing the publishing and components API in action in edx-platform. For the most part, each call to the APIs has been very clear and self-explanatory. Kudos.

For this particular set_draft_version call, though, I had to dive into the definition to understand that passing in None means "remove from the draft". It feels like that function does one of two different things depending on whether the second arg is None. I wonder if, rather than having a single API function which takes an Optional, it'd be better to have one API function which demands and int, and another which removes the draft:

def set_draft_version(publishable_entity_id: int, publishable_entity_version_pk: int) -> None: ... def remove_from_draft(publishable_entity_id: int) -> None: ...

Good point. I suspect I was just getting too wrapped up in the data model to step back and think about it from the API consumer's point of view. I'll do this.

kdmccormick · 2023-11-27T16:17:39Z

cms/envs/common.py

+
+    # Learning Core Apps, used by v2 content libraries (content_libraries app)
+    "openedx_learning.core.components.apps.ComponentsConfig",
+    "openedx_learning.core.contents.apps.ContentsConfig",
+    "openedx_learning.core.publishing.apps.PublishingConfig",


Suggested change

# Learning Core Apps, used by v2 content libraries (content_libraries app)

"openedx_learning.core.components.apps.ComponentsConfig",

"openedx_learning.core.contents.apps.ContentsConfig",

"openedx_learning.core.publishing.apps.PublishingConfig",

# Learning Core Apps, used by v2 content libraries (content_libraries app)

"openedx_learning.core.components",

"openedx_learning.core.contents",

"openedx_learning.core.publishing",

(non-blocking nit)

Django lets you abbreviate these when your app contains an apps.py with a lone/default app config.

kdmccormick · 2023-11-27T16:18:17Z

lms/envs/common.py

+
+    # Learning Core Apps, used by v2 content libraries (content_libraries app)
+    "openedx_learning.core.components.apps.ComponentsConfig",
+    "openedx_learning.core.contents.apps.ContentsConfig",
+    "openedx_learning.core.publishing.apps.PublishingConfig",


Suggested change

# Learning Core Apps, used by v2 content libraries (content_libraries app)

"openedx_learning.core.components.apps.ComponentsConfig",

"openedx_learning.core.contents.apps.ContentsConfig",

"openedx_learning.core.publishing.apps.PublishingConfig",

# Learning Core Apps, used by v2 content libraries (content_libraries app)

"openedx_learning.core.components",

"openedx_learning.core.contents",

"openedx_learning.core.publishing",

(non-blocking nit) Same comment

openedx/core/djangoapps/content_libraries/management/commands/migrate_lib_to_learning_core.py

ormsbee added 2 commits October 17, 2023 13:09

feat: first iteration of migration script for blockstore to openedx-l…

05b4106

…earning models

fixup!: add migrations, clean up import code, add comments

0967df9

ormsbee commented Oct 24, 2023

View reviewed changes

openedx/core/djangoapps/content_libraries/api.py Outdated Show resolved Hide resolved

ormsbee commented Oct 24, 2023

View reviewed changes

openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py Outdated Show resolved Hide resolved

ormsbee added 2 commits October 24, 2023 15:24

fixup!: moving XBlock runtime related commits to a different PR

31f008a

fixup!: one more view I needed to undo changes on.

828c822

ormsbee mentioned this pull request Oct 24, 2023

Read support for v2 library content using openedx-learning models #33579

Closed

5 tasks

ormsbee marked this pull request as ready for review October 26, 2023 21:51

ormsbee requested review from bradenmacdonald and kdmccormick October 26, 2023 21:51

bradenmacdonald approved these changes Nov 3, 2023

View reviewed changes

ormsbee commented Nov 4, 2023

View reviewed changes

kdmccormick reviewed Nov 27, 2023

View reviewed changes

ormsbee closed this Jan 16, 2024

ormsbee mentioned this pull request Jan 16, 2024

Switch v2 libraries to Learning Core data models #34066

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import v2 libraries content into openedx-learning data models #33577

Import v2 libraries content into openedx-learning data models #33577

ormsbee commented Oct 24, 2023 •

edited

Loading

bradenmacdonald left a comment

bradenmacdonald Nov 3, 2023

kdmccormick Nov 27, 2023 •

edited

Loading

ormsbee Nov 28, 2023

bradenmacdonald Nov 3, 2023

kdmccormick Nov 27, 2023

ormsbee Nov 28, 2023

kdmccormick Nov 30, 2023

ormsbee Nov 4, 2023

bradenmacdonald Nov 6, 2023

ormsbee Nov 13, 2023

kdmccormick Nov 27, 2023

kdmccormick left a comment

kdmccormick Nov 27, 2023

kdmccormick Nov 27, 2023

ormsbee Nov 28, 2023

kdmccormick Nov 27, 2023

kdmccormick Nov 27, 2023

Import v2 libraries content into openedx-learning data models #33577

Import v2 libraries content into openedx-learning data models #33577

Conversation

ormsbee commented Oct 24, 2023 • edited Loading

Description

Blockers to Merging

Testing instructions

Screenshots

bradenmacdonald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdmccormick Nov 27, 2023 • 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

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 left a 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

Choose a reason for hiding this comment

ormsbee commented Oct 24, 2023 •

edited

Loading

kdmccormick Nov 27, 2023 •

edited

Loading