Skip to content

feat: create Import and related models #36515

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
Apr 17, 2025
Merged

feat: create Import and related models #36515

merged 9 commits into from
Apr 17, 2025

Conversation

NiedielnitsevIvan
Copy link
Contributor

@NiedielnitsevIvan NiedielnitsevIvan commented Apr 11, 2025 •
edited by kdmccormick
Loading

Description

A new application has been created, described in the following ADR
New models for saving events of legacy content imports to Content Libraries have been created, as well as related models for mapping original content and new content created during the import process.

Supporting information

More details about the new app can be found in the ADR, as well as the README file.

Testing instructions

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Apr 11, 2025
@openedx-webhooks
Copy link

Thanks for the pull request, @NiedielnitsevIvan!

This repository is currently maintained by @openedx/wg-maintenance-edx-platform.

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 approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.
🔘 Provide context

To 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:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads
🔘 Get a green build

If 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:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@github-project-automation github-project-automation bot moved this to Needs Triage in Contributions Apr 11, 2025
@kdmccormick kdmccormick self-requested a review April 11, 2025 16:19
@mphilbrick211 mphilbrick211 moved this from Needs Triage to Ready for Review in Contributions Apr 14, 2025
kdmccormick
kdmccormick requested changes Apr 15, 2025
View reviewed changes
@NiedielnitsevIvan NiedielnitsevIvan force-pushed the NiedielnitsevIvan/feat/import-from-modulestore-models branch 2 times, most recently from 1dea8a9 to c4178a7 Compare April 16, 2025 16:14
@NiedielnitsevIvan NiedielnitsevIvan mentioned this pull request Apr 17, 2025
Merged
kdmccormick
kdmccormick reviewed Apr 17, 2025
View reviewed changes
cms/djangoapps/import_from_modulestore/data.py Outdated
Comment on lines 13 to 23
# PENDING: The import has been created, but the OLX and related data are not yet in the library.
# It is not ready to be read.
PENDING = 'pending', _('Pending')
# READY: The content is staged and ready to be read.
READY = 'ready', _('Ready')
# IMPORTED: The content has been imported into the library.
IMPORTED = 'imported', _('Imported')
# CANCELED: The import was canceled before it was imported.
CANCELED = 'canceled', _('Canceled')
# ERROR: The content could not be imported.
ERROR = 'error', _('Error')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# PENDING: The import has been created, but the OLX and related data are not yet in the library.
# It is not ready to be read.
PENDING = 'pending', _('Pending')
# READY: The content is staged and ready to be read.
READY = 'ready', _('Ready')
# IMPORTED: The content has been imported into the library.
IMPORTED = 'imported', _('Imported')
# CANCELED: The import was canceled before it was imported.
CANCELED = 'canceled', _('Canceled')
# ERROR: The content could not be imported.
ERROR = 'error', _('Error')
NOT_STARTED = 'not_started', _('Waiting to stage content')
STAGING = 'staging', _('Staging content for import')
STAGING_FAILED = _('Failed to stage content')
STAGING_CANCELED = _('Canceled while staging content')
STAGED = 'staged', _('Content is staged and ready for import')
IMPORTING = 'importing', _('Importing staged content')
IMPORTING_FAILED = 'importing_failed', _('Failed to import staged content')
IMPORTING_CANCELED = 'canceled', _('Canceled while importing staged content')
IMPORTED = 'imported', _('Successfully imported content')

Since the import process happens in two major parts (staging, importing), I think we should reflect that in the statuses.

Also, we can make the human-readable names more informative, which removes the need for code comments, and makes these strings more useful for the end user.

How do these statuses look to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that 2 statuses STAGING_CANCELED and IMPORTING_CANCELED are redundant, since I can hardly imagine canceling an import that is already in the process of importing, so I left just 1 status for this, and all other changes to the ones you suggested.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me.

kdmccormick
kdmccormick reviewed Apr 17, 2025
View reviewed changes
Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your quick work. Above are my final comments. Once those are resolved, I'll merge this.

@NiedielnitsevIvan NiedielnitsevIvan force-pushed the NiedielnitsevIvan/feat/import-from-modulestore-models branch from b091bbc to d9c3959 Compare April 17, 2025 17:47
@NiedielnitsevIvan
Copy link
Contributor Author

Check Django Migrations is falling because openedx-learning has no commited migration.
I think we need to add the same linter there. Because it's the second time I've gotten this error in this PR.

@NiedielnitsevIvan
Copy link
Contributor Author

@kdmccormick I added the migration that was missing, I propose to merged it and update the version so that we can merge this PR.
openedx/openedx-learning#305

kdmccormick
kdmccormick approved these changes Apr 17, 2025
View reviewed changes
Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once tests pass

@NiedielnitsevIvan
Copy link
Contributor Author

Please restart failed checkers, they have to pass now, I don't have the permissions to do that

@kdmccormick kdmccormick enabled auto-merge (squash) April 17, 2025 19:02
@kdmccormick kdmccormick merged commit 505b4f4 into openedx:master Apr 17, 2025
50 checks passed
@github-project-automation github-project-automation bot moved this from Ready for Review to Done in Contributions Apr 17, 2025
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@navinkarkera
Copy link
Contributor

@NiedielnitsevIvan @kdmccormick Any specific reason for not creating the migration file for this new model file?

@kdmccormick
Copy link
Member

No reason @navinkarkera , I can fix forward shortly. I'm surprised the linter didn't catch it.

This was referenced Apr 21, 2025
Merged
Open
Open
@ormsbee ormsbee mentioned this pull request Apr 23, 2025
Merged
tonybusa pushed a commit to tonybusa/edx-platform that referenced this pull request Apr 23, 2025
@NiedielnitsevIvan @tonybusa
feat: Models for import_from_modulestore (openedx#36515)
1c36642 
A new application has been created, described in this ADR:
openedx#36545

have been created, as well as related models for mapping original content and
new content created during the import process. Python and Django APIs, as well
as a Django admin interface, will soon follow.
UsamaSadiq pushed a commit that referenced this pull request May 14, 2025
@NiedielnitsevIvan @UsamaSadiq
feat: Models for import_from_modulestore (#36515)
d812fda 
A new application has been created, described in this ADR:
#36545

have been created, as well as related models for mapping original content and
new content created during the import process. Python and Django APIs, as well
as a Django admin interface, will soon follow.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kdmccormick kdmccormick kdmccormick approved these changes

Assignees
No one assigned
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Contributions
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@NiedielnitsevIvan @openedx-webhooks @edx-pipeline-bot @navinkarkera @kdmccormick