feat: removing get_storage_class from COURSE_METADATA_EXPORT_STORAGE #36761

dwong2708 · 2025-05-20T21:41:29Z

Description

The file cms/djangoapps/export_course_metadata/storage.py was updated to use storages, as get_storage_class will be removed in Django 5.1.

These changes include:

Ensuring the updated code is compatible with both Django 4.2 and Django 5 or above
Adding test cases to verify the compatibility mentioned above
Considering future compatibility with the STORAGES settings

Testing instructions

Run pytest cms cms/djangoapps/export_course_metadata/test_signals.py

Functionality verified locally

Exporting course:

Importing course:

openedx-webhooks · 2025-05-20T21:41:32Z

Thanks for the pull request, @dwong2708!

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.

cms/djangoapps/export_course_metadata/test_signals.py

common/djangoapps/util/storage.py

awais786 · 2025-05-21T04:43:48Z

cms/djangoapps/export_course_metadata/storage.py

@@ -17,4 +17,4 @@ def __init__(self):
        bucket = settings.COURSE_METADATA_EXPORT_BUCKET
        super().__init__(bucket_name=bucket, custom_domain=None, querystring_auth=True)

-course_metadata_export_storage = get_storage_class(settings.COURSE_METADATA_EXPORT_STORAGE)()
+course_metadata_export_storage = resolve_storage_backend("COURSE_METADATA_EXPORT_STORAGE")


nit: Consider renaming 'COURSE_METADATA_EXPORT_STORAGE' to lowercase (e.g., 'course_metadata_export_storage') for consistency with other keys in STORAGES, which are conventionally lowercase.

Got it, thanks for your feedback. I'm concerned about this because the current settings key, "COURSE_METADATA_EXPORT_STORAGE", is in uppercase. For that reason, I have a question: how will the transition to migrate these changes work? Will it be merged before the settings are migrated to STORAGES, or can we assume that the STORAGES settings will already exist?

Yes, your concern is valid, so let’s keep the key as-is for now. There isn’t a finalized discussion yet on how the migration to the new STORAGES setting will be handled. At this stage, our focus is on updating deprecated methods for Django 4.2 and ensuring the code remains compatible with future versions like Django 5.x.

Just to clarify, I was referring to the future STORAGES key, e.g.,
STORAGES = {"course_metadata_export_storage": "something"}
—not the existing settings.COURSE_METADATA_EXPORT_STORAGE. I mixed up the two earlier :) but for now no change require since we have no STORAGES plan yet.

Got it, thank you. Here are some ideas on how we could handle the migration:

Case 1:
Stage 1 (no breaking change): Migrate get_storage_class (this PR is an example) and merge it.
Stage 2 (breaking change): Migrate storage settings to the new convention, e.g., STORAGES = {}. and decide if we use lower or upper case in the key.
Stage 3: Ready to migrate to Django 5.

Case 2:
Stage 1 (no breaking change): Migrate get_storage_class (this PR is an example) and merge it.
Stage 2: Ready to migrate to Django 5 because the STORAGES settings is not required
Stage 3 (breaking change): Gradually migrate storage settings to the new convention, e.g., STORAGES = {}, in preparation for upgrading to Django 6.

Stage 1 would be merging all these PRs as the initial step.
Stage 2 involves reaching out to the owning teams to get their input on the proposed migration plan. because it will effect the community also.

Looping in @feanil for visibility.

awais786 · 2025-05-21T04:49:27Z

common/djangoapps/util/storage.py

+        # In Django 5.x, it uses STORAGES['default']
+        return default_storage
+
+    if django.VERSION >= (5, 0) and storage_key in storages_config:


Since the STORAGES setting is available starting in Django 4.2, we can simplify the logic by removing the django.VERSION >= (5, 0) check. Instead, we can directly check if storage_key exists in storages_config, allowing this logic to work consistently in both Django 4.2 and 5.x as long as STORAGES is defined.

Thanks. I have the same question regarding the removal of the django.VERSION >= (5, 0) check. How will the transition for these changes work? If I remove this check while we're still on Django 4 and the STORAGES settings have already been migrated, I believe it won't work because django.core.files.storage.storages doesn't exist for Django 4

@dwong2708 I'm a bit confused about this. My assumption is that django.core.files.storage.storages should work with Django 4.2. Could you please take a look at this code reference?

@awais786 You’re right! I looked into Django 4.0 and realized that Django 4.2 supports django.core.files.storage.storages. I’ve updated my logic—could you please take another look?

awais786 · 2025-05-21T05:05:07Z

@dwong2708 Thanks for this PR. Awesome work.

awais786

I am approving it from my side. But we need final review from @feanil.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label May 20, 2025

openedx-webhooks added this to Contributions May 20, 2025

github-project-automation bot moved this to Needs Triage in Contributions May 20, 2025

dwong2708 changed the title ~~Dwong/updgrade django 5 get storages~~ feat: removing get_storage_class from COURSE_METADATA_EXPORT_STORAGE May 20, 2025

dwong2708 force-pushed the dwong/updgrade-django-5-get-storages branch from b8da29c to e9daa7c Compare May 20, 2025 22:14

dwong2708 marked this pull request as ready for review May 20, 2025 22:37

wgu-taylor-payne reviewed May 21, 2025

View reviewed changes

dwong2708 force-pushed the dwong/updgrade-django-5-get-storages branch from e9daa7c to bb9ce7e Compare May 21, 2025 02:06

dwong2708 requested review from wgu-taylor-payne, awais786 and feanil May 21, 2025 02:06

awais786 reviewed May 21, 2025

View reviewed changes

dwong2708 requested a review from awais786 May 21, 2025 15:26

dwong2708 force-pushed the dwong/updgrade-django-5-get-storages branch 5 times, most recently from a54e56d to 2c40211 Compare May 22, 2025 15:27

awais786 approved these changes May 22, 2025

View reviewed changes

mphilbrick211 added the mao-onboarding Reviewing this will help onboard devs from an Axim mission-aligned organization (MAO). label May 22, 2025

mphilbrick211 moved this from Needs Triage to In Eng Review in Contributions May 22, 2025

dwong2708 added 3 commits May 26, 2025 09:54

feat: update file storage access to support Django 5.0 storages registry

a09135c

test: adding test cases for the export course metadata export storage

bad494c

fix: resolve feedback comments

ed97319

dwong2708 force-pushed the dwong/updgrade-django-5-get-storages branch from 2c40211 to ed97319 Compare May 26, 2025 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: removing get_storage_class from COURSE_METADATA_EXPORT_STORAGE #36761

feat: removing get_storage_class from COURSE_METADATA_EXPORT_STORAGE #36761

Uh oh!

dwong2708 commented May 20, 2025 •

edited

Loading

Uh oh!

openedx-webhooks commented May 20, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

awais786 May 21, 2025

Uh oh!

dwong2708 May 21, 2025

Uh oh!

awais786 May 21, 2025

Uh oh!

awais786 May 21, 2025

Uh oh!

dwong2708 May 21, 2025

Uh oh!

awais786 May 21, 2025

Uh oh!

awais786 May 21, 2025

Uh oh!

dwong2708 May 21, 2025

Uh oh!

awais786 May 21, 2025

Uh oh!

dwong2708 May 21, 2025

Uh oh!

awais786 commented May 21, 2025

Uh oh!

awais786 left a comment

Uh oh!

Uh oh!

feat: removing get_storage_class from COURSE_METADATA_EXPORT_STORAGE #36761

Are you sure you want to change the base?

feat: removing get_storage_class from COURSE_METADATA_EXPORT_STORAGE #36761

Uh oh!

Conversation

dwong2708 commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing instructions

Functionality verified locally

Uh oh!

openedx-webhooks commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awais786 commented May 21, 2025

Uh oh!

awais786 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dwong2708 commented May 20, 2025 •

edited

Loading

openedx-webhooks commented May 20, 2025 •

edited

Loading