feat: [AXM-2307] add management command to backfill ContentDate model…#37989
Conversation
|
Thanks for the pull request, @kyrylo-kh! 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. DetailsWhere 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/core/djangoapps/course_date_signals/management/commands/seed_content_dates.py
Outdated
Show resolved
Hide resolved
openedx/core/djangoapps/course_date_signals/management/commands/seed_content_dates.py
Outdated
Show resolved
Hide resolved
openedx/core/djangoapps/course_date_signals/management/commands/seed_content_dates.py
Outdated
Show resolved
Hide resolved
openedx/core/djangoapps/course_date_signals/management/commands/seed_content_dates.py
Outdated
Show resolved
Hide resolved
openedx/core/djangoapps/course_date_signals/management/commands/seed_content_dates.py
Outdated
Show resolved
Hide resolved
openedx/core/djangoapps/course_date_signals/management/commands/seed_content_dates.py
Outdated
Show resolved
Hide resolved
openedx/core/djangoapps/course_date_signals/management/commands/seed_content_dates.py
Outdated
Show resolved
Hide resolved
… with existing assignments
40a7206 to
57a09ad
Compare
db632e6 to
71a4fe7
Compare
e0d
left a comment
There was a problem hiding this comment.
A general comment: there are no tests for this new command.
| continue | ||
|
|
||
| try: | ||
| update_or_create_assignments_due_dates(course_key, [assignment]) |
There was a problem hiding this comment.
Why aren't we using the batch capability of the imported function rather than running this in a loop with single item lists?
There was a problem hiding this comment.
Re-implemented in batch way
| f"Created ContentDate for {assignment.title} " | ||
| f"in course {course_key}" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Why this broad catch and continue? I think this is problematic because Django, I believe, will mark the atomic transaction that you created above as invalid and all of the updates will ultimately fail. If the idea is to make this save as much as possible, you will have to change the transaction boundaries.
There was a problem hiding this comment.
Removed the outer atomic() around the course; each batch is its own atomic(); failures are handled outside that block.
| return processed, 0, 0, 0 | ||
|
|
||
| existing_due_locations: set = set( | ||
| when_models.ContentDate.objects.filter( |
There was a problem hiding this comment.
We shouldn't be reaching in to when's models directly. If there isn't an API that currently supports this, we should create it.
There was a problem hiding this comment.
Switched to get_existing_due_locations() on edx_when.api (added there).
| log.warning(f"Course not found in modulestore: {course_key}") | ||
| return (0, 0, 0, 0) | ||
|
|
||
| staff_user = User.objects.filter(is_staff=True).first() |
There was a problem hiding this comment.
This pattern is unacceptable in my opinion. We need a more robust approach that doesn't pick any old staff user for the convenience of the management command.
There was a problem hiding this comment.
Replaced with required --username and validation (is_staff, is_active).
| """ | ||
| Process a single course and return (processed, created, updated, skipped) counts. | ||
| """ | ||
| store = modulestore() |
There was a problem hiding this comment.
I'm not sure of the current performance of this, but passing it as an argument seems like the right thing to do.
There was a problem hiding this comment.
Re-implemented and now it's called once and the store is passed into _process_course
|
Tests are failing because they depends on openedx/edx-when#346. This should be merged after merging edx-when PR |
… with existing assignments
Description
This PR adds a new Django management command
seed_content_datesthat extracts assignment due dates from the modulestore and populates theContentDatetable in theedx-whenservice.Key Features
python manage.py lms seed_content_datesContentDatefrom course assignments in the modulestore.Options
--course-id: Seed a specific course.--org: Seed all courses for a specific organization.--dry-run: Simulate processing without writing to the database.--force-update: Overwrite existingContentDaterecords instead of skipping.--batch-size: Process assignments in batches (default: 100).Example Usage