feat: library export support being added#737
Conversation
src/ol_openedx_git_auto_export/ol_openedx_git_auto_export/utils.py
Outdated
Show resolved
Hide resolved
src/ol_openedx_git_auto_export/ol_openedx_git_auto_export/tasks.py
Outdated
Show resolved
Hide resolved
| ] # type: ignore # noqa: PGH003 | ||
|
|
||
| operations = [ | ||
| migrations.CreateModel( |
There was a problem hiding this comment.
This should be rename model migration and then a rename field migration. We won't need the data migration in that case.
...git_auto_export/ol_openedx_git_auto_export/migrations/0003_migrate_course_to_content_repo.py
Outdated
Show resolved
Hide resolved
| # Library-specific settings (disabled by default) | ||
| settings.FEATURES[ENABLE_GIT_AUTO_LIBRARY_EXPORT] = env_tokens.get( | ||
| "FEATURES", {} | ||
| ).get(ENABLE_GIT_AUTO_LIBRARY_EXPORT, False) | ||
| settings.FEATURES[ENABLE_AUTO_GITHUB_LIBRARY_REPO_CREATION] = env_tokens.get( | ||
| "FEATURES", {} | ||
| ).get(ENABLE_AUTO_GITHUB_LIBRARY_REPO_CREATION, False) |
There was a problem hiding this comment.
As we are adding new settings, can you please look into openedx/openedx-platform#37345 and see if we should also flatten these feature flags into django settings.
There was a problem hiding this comment.
I think you missed this comment.
There was a problem hiding this comment.
We can flatten the FEATURES setting in the follow-up PR as this will require some configuration changes in ol-infrastructure repo as well
| ("course", "Course"), | ||
| ("library", "Library"), | ||
| ) |
There was a problem hiding this comment.
Can we create constants for these strings.
There was a problem hiding this comment.
These are custom filter options, not being used anywhere else
There was a problem hiding this comment.
Okay, I meant these strings Course, and Library. These strings are used at multiple places in this file. Maybe we can create constants CONTENT_TYPE_COURSE="course" and similarly for library.
There was a problem hiding this comment.
I will create ContentType Enum to represent both Course and Library
| PluginSignals.DISPATCH_UID: "ol_openedx_git_auto_export.signals.listen_for_course_created", # noqa: E501 | ||
| }, | ||
| # Library Signals | ||
| # NOTE: Library v1 (library-v1:) only has LIBRARY_UPDATED, no creation signal # noqa: E501 |
There was a problem hiding this comment.
Maybe we can disable E501 for whole file.
| content_library = kwargs.get("content_library") | ||
| if content_library: | ||
| library_key = content_library.library_key | ||
| log.info("Library v2 created signal received for library: %s", library_key) |
There was a problem hiding this comment.
Is it possible that we do not get content_library in kwargs? We are not doing the same for listen_for_library_updated
There was a problem hiding this comment.
Library v2 signals are a bit different and we get ContentLibraryData in the kwargs.
This is a preventive measure which I have noticed in openedx-platform
|
|
||
|
|
||
| # Library Signal Receivers | ||
| def listen_for_library_updated(sender, library_key, **kwargs): # noqa: ARG001 |
There was a problem hiding this comment.
| def listen_for_library_updated(sender, library_key, **kwargs): # noqa: ARG001 | |
| def listen_for_library_v1_updated(sender, library_key, **kwargs): # noqa: ARG001 |
| return True | ||
|
|
||
|
|
||
| def export_to_git(content_key, repo, user=None, rdir=None): |
There was a problem hiding this comment.
Do we need this wrapper? This makes the code more confusing. Instead we can directly call the platform_export_to_git from the tasks.
There was a problem hiding this comment.
This is a general comment.
- Can you please go through the names of all the utils?
- Also, at some places, we call task from the signals, and in some cases, we call a chain of utils that calls a tasks and then task calls the utils again. I would like to make it consistent. Either trigger task from a signal after validations or call a util in all cases.
| from ol_openedx_git_auto_export.tasks import async_export_to_git # noqa: PLC0415 | ||
|
|
||
| # Check library-specific flag | ||
| library_export_enabled = settings.FEATURES.get( |
There was a problem hiding this comment.
Like is_auto_repo_creation_enabled, we can also create for util for git auto export.
| @@ -0,0 +1,25 @@ | |||
| # Generated migration to rename course_key field to content_key and change type | |||
There was a problem hiding this comment.
We can merge this migration with 0002.
| # Library-specific settings (disabled by default) | ||
| settings.FEATURES[ENABLE_GIT_AUTO_LIBRARY_EXPORT] = env_tokens.get( | ||
| "FEATURES", {} | ||
| ).get(ENABLE_GIT_AUTO_LIBRARY_EXPORT, False) | ||
| settings.FEATURES[ENABLE_AUTO_GITHUB_LIBRARY_REPO_CREATION] = env_tokens.get( | ||
| "FEATURES", {} | ||
| ).get(ENABLE_AUTO_GITHUB_LIBRARY_REPO_CREATION, False) |
There was a problem hiding this comment.
I think you missed this comment.
| # Library-specific settings (disabled by default) | ||
| settings.FEATURES[ENABLE_GIT_AUTO_LIBRARY_EXPORT] = env_tokens.get( | ||
| "FEATURES", {} | ||
| ).get(ENABLE_GIT_AUTO_LIBRARY_EXPORT, False) | ||
| settings.FEATURES[ENABLE_AUTO_GITHUB_LIBRARY_REPO_CREATION] = env_tokens.get( | ||
| "FEATURES", {} | ||
| ).get(ENABLE_AUTO_GITHUB_LIBRARY_REPO_CREATION, False) |
There was a problem hiding this comment.
Same as common.py, I think we should flatten these settings to Django settings.
| ("course", "Course"), | ||
| ("library", "Library"), | ||
| ) |
There was a problem hiding this comment.
Okay, I meant these strings Course, and Library. These strings are used at multiple places in this file. Maybe we can create constants CONTENT_TYPE_COURSE="course" and similarly for library.
| content_key = LearningContextKey.from_string(content_key_string) | ||
| is_v1_library = isinstance(content_key, LibraryLocator) | ||
| is_v2_library = isinstance(content_key, LibraryLocatorV2) | ||
| except Exception: | ||
| LOGGER.exception("Failed to parse content key: %s", content_key_string) | ||
| return | ||
|
|
||
| # Get the content module (course or library) | ||
| if is_v2_library: | ||
| # V2 libraries use content_libraries API | ||
| content_module = get_library(content_key) | ||
| content_type = "library" | ||
| elif is_v1_library: | ||
| # V1 libraries use modulestore | ||
| content_module = modulestore().get_library(content_key) | ||
| content_type = "library" | ||
| else: | ||
| content_module = modulestore().get_course(content_key) | ||
| content_type = "course" | ||
|
|
There was a problem hiding this comment.
I think we can create a util that takes content_key and returns content_module, content_type, and url_path. It can be used in both the tasks async_export_to_git and async_create_github_repo. This will remove duplication.
| if ssh_url and export_course: | ||
| export_course_to_git(course_key) | ||
| if ssh_url and export_content: | ||
| async_export_to_git(content_key_str) |
There was a problem hiding this comment.
Bug: The synchronous call to async_export_to_git within the async_create_github_repo task skips the necessary step of creating the git export directory, causing the export to fail.
Severity: HIGH
Suggested Fix
Ensure the git export directory is created before attempting to export content. This can be done by calling get_or_create_git_export_repo_dir within the async_create_github_repo task before the async_export_to_git function is called when export_content is true. Alternatively, refactor the logic to use the existing utility functions that already handle directory creation.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/ol_openedx_git_auto_export/ol_openedx_git_auto_export/tasks.py#L196
Potential issue: In the `async_create_github_repo` task, the call to
`async_export_to_git` is made synchronously when `export_content` is true. This code
path is triggered upon the creation of a course rerun or a version 2 library. However,
the flow does not include a call to `get_or_create_git_export_repo_dir` to ensure the
target directory for the git export exists. Other export paths, such as course
publishing, do create this directory. As a result, the `export_to_git` function, which
is called by `async_export_to_git`, will fail because it attempts to operate within a
non-existent directory.
What are the relevant tickets?
https://github.com/mitodl/hq/issues/10083
Related
edx-platformPR:openedx/openedx-platform#38026
Description (What does it do?)
This PR adds support to auto export libraries (v1 and v2) content to Github repositories
How can this be tested?
Screenshots