feat: bulk modulestore migration [FC-0097] #37381
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request, @ChrisChV! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
openedx-webhooks
added
the
open-source-contribution
ChrisChV
force-pushed
the
chris/FAL-4253-bulk-library-migration
branch
from
6e1903d to
c39836e
Compare
Merged
7 tasks
rpenido
approved these changes
Oct 1, 2025
There was a problem hiding this comment.
LGTM 👍
Thank you for your work, @ChrisChV!
- I tested this using the instructions from openedx/frontend-app-authoring#2493
- I read through the code
- I checked for accessibility issues
- Includes documentation
| collection_key=modified_key, | ||
| title=title, | ||
| ) | ||
| except libraries_api.LibraryCollectionAlreadyExists as e: |
There was a problem hiding this comment.
Note: We are always creating new collections here. Therefore, if you migrate the same library multiple times, while the components are not duplicated (due to the default skip strategy), the collections are.
I'm not sure if there is an easy and pretty solution for this, though.
There was a problem hiding this comment.
I think it is good enough for now! It worked after some tweaks (see the review).
Thanks @ChrisChV.
| @shared_task(base=_MigrationTask, bind=True) | ||
| # Note: The decorator @set_code_owner_attribute cannot be used here because the UserTaskMixin | ||
| # does stack inspection and can't handle additional decorators. | ||
| def migrate_from_modulestore( |
There was a problem hiding this comment.
Thanks for breaking this function into smaller ones! ❤️
rpenido
suggested changes
Oct 1, 2025
|
@kdmccormick did you also want to review this? |
|
Sorry for the delay! Please let me know if you need any additional information to reproduce it. |
@rpenido, I merged this branch with master. It works for me. Maybe you need to run |
|
You're right @ChrisChV! It was missing the migration. Sorry for the noise! @bradenmacdonald I manually tested it and it is working as expected. |
|
OK, this is good to go then. |
|
@rpenido Thanks! Could you make another kick review of b38cbdf. There are new small requirements: openedx/frontend-app-authoring#2201 (comment) |
rpenido
approved these changes
Oct 13, 2025
There was a problem hiding this comment.
LGTM 👍
Thank you for your work @ChrisChV!
- I tested this using the instructions from the PR
- I read through the code
- I checked for accessibility issues
- Includes documentation
This is working as expected!
I left you a comment about the datetime format.
| from django.utils.translation import gettext_lazy as _ | ||
|
|
||
| from common.djangoapps.split_modulestore_django.models import SplitModulestoreCourseIndex | ||
| from common.djangoapps.util.date_utils import strftime_localized, DEFAULT_LONG_DATE_FORMAT |
There was a problem hiding this comment.
DEFAULT_LONG_DATE_FORMAT doesn't include time. Should we use DEFAULT_DATE_TIME_FORMAT instead?
Suggested change
| from common.djangoapps.util.date_utils import strftime_localized, DEFAULT_LONG_DATE_FORMAT | |
| from common.djangoapps.util.date_utils import strftime_localized, DEFAULT_DATE_TIME_FORMAT |
| key = slugify(title) | ||
| collection = None | ||
| attempt = 0 | ||
| created_at = strftime_localized(datetime.now(timezone.utc), DEFAULT_LONG_DATE_FORMAT) |
There was a problem hiding this comment.
Suggested change
| created_at = strftime_localized(datetime.now(timezone.utc), DEFAULT_LONG_DATE_FORMAT) | |
| created_at = strftime_localized(datetime.now(timezone.utc), DEFAULT_DATE_TIME_FORMAT) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Supporting information
Testing instructions
Deadline
ASAP, before the Ulmo cut.
Other information
N/A