Skip to content
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.

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

Asset handling updates (config, admin) #234

Merged
merged 2 commits into from
Oct 2, 2024

Conversation

ormsbee
Copy link
Contributor

@ormsbee ormsbee commented Sep 27, 2024

I think this should be the last of the Learning Core changes needed to get minimal asset handling into Content Libraries.

The meat of this is:

feat!: require OPENEDX_LEARNING setting for Content storage

WARNING: This commit will break any file-backed Content entries you
currently have. Though you will likely only have them if you've used the
very recently created add_assets_to_component command.

This commit switches us away from using the default_storage and requires
the project to set an OPENEDX_LEARNING settings dictionary with a MEDIA
key like:

OPENEDX_LEARNING = {
    'MEDIA': {
        "BACKEND": "django.core.files.storage.FileSystemStorage",
        "OPTIONS": {
            "location": "/openedx/media-private/openedx-learning",
        }
    }
}

If no such setting is present, Content operations that require file
access will raise a django.core.exceptions.ImproperlyConfigured error.

In addition to that, Content files will be stored in a "content"
subdirectory of the specified media location. This is to allow us more
flexibility as we start to use file storage for other things like the
import/export process.

We need to have a separate space for Learning Core media files because
these files should NOT be directly accessible via the browser (as the
MEDIA_ROOT is). This is because of access policies and the fact that the
filenames will not be meaningful by themselves and must be translated by
app logic. For details, please see:

* docs/decisions/0015-serving-static-assets.rst
* openedx_learning/apps/authoring/contents/models.py
* openedx_learning/apps/authoring/components/models.py

Hiding these files in a new location also required changes to the Django
admin, which are included here. This commit also adds a little extra to
the admin to make it easier to map Component assets to actual files on
disk.

This commit also adds the following helpers to the Content model:

* read_file(): return opened Django File object for the Content.
* os_path(): get the full OS path to the stored file, if available.
* path(): get the logical path for this asset relative to our configured
  OPENEDX_LEARNING['MEDIA'] storage root.

This commit also auto-strips file paths starting with '/'. Allowing
absolute paths tripped me up multiple times as I was setting up my test
data, and I don't think there's any advantage to allowing
inconsistencies (e.g. "/static/foo.png" and "static/foo.png"). We now
force the relative form ("static/foo.png").```

Also includes the following performance tweak:

perf: reduce db calls for ComponentVersion after lookup by UUID

When you fetch a single ComponentVersion, you are often going to want to
pull data related to its Component, and possibly the LearningPackage it
belongs to. This commit adds a select_related call to eliminate the
extra roundtrips for this information.

This will not work without the OPENEDX_LEARNING setting set, which will be done by default in tutor dev once overhangio/tutor#1124 lands.

Screenshots:

admin-content-preview
admin-component-asset

When you fetch a single ComponentVersion, you are often going to want to
pull data related to its Component, and possibly the LearningPackage it
belongs to. This commit adds a select_related call to eliminate the
extra roundtrips for this information.
@ormsbee ormsbee force-pushed the asset-integration branch 2 times, most recently from 2cc3ff2 to e055898 Compare September 27, 2024 03:44
@ormsbee
Copy link
Contributor Author

ormsbee commented Sep 27, 2024

@kdmccormick, @bradenmacdonald: I still need to add a couple of small tests, but this is the basic learning core piece of it.

@ormsbee
Copy link
Contributor Author

ormsbee commented Sep 27, 2024

Now updated with tests.

Copy link
Contributor

@bradenmacdonald bradenmacdonald left a comment

Choose a reason for hiding this comment

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

LGTM! A couple minor comments but nothing blocking.

I tested this as described here.

WARNING: This commit will break any file-backed Content entries you
currently have. Though you will likely only have them if you've used the
very recently created add_assets_to_component command.

This commit switches us away from using the default_storage and requires
the project to set an OPENEDX_LEARNING settings dictionary with a MEDIA
key like:

OPENEDX_LEARNING = {
    'MEDIA': {
        "BACKEND": "django.core.files.storage.FileSystemStorage",
        "OPTIONS": {
            "location": "/openedx/media-private/openedx-learning",
        }
    }
}

If no such setting is present, Content operations that require file
access will raise a django.core.exceptions.ImproperlyConfigured error.

In addition to that, Content files will be stored in a "content"
subdirectory of the specified media location. This is to allow us more
flexibility as we start to use file storage for other things like the
import/export process.

We need to have a separate space for Learning Core media files because
these files should NOT be directly accessible via the browser (as the
MEDIA_ROOT is). This is because of access policies and the fact that the
filenames will not be meaningful by themselves and must be translated by
app logic. For details, please see:

* docs/decisions/0015-serving-static-assets.rst
* openedx_learning/apps/authoring/contents/models.py
* openedx_learning/apps/authoring/components/models.py

Hiding these files in a new location also required changes to the Django
admin, which are included here. This commit also adds a little extra to
the admin to make it easier to map Component assets to actual files on
disk.

This commit also adds the following helpers to the Content model:

* read_file(): return opened Django File object for the Content.
* os_path(): get the full OS path to the stored file, if available.
* path(): get the logical path for this asset relative to our configured
  OPENEDX_LEARNING['MEDIA'] storage root.

This commit also auto-strips file paths starting with '/'. Allowing
absolute paths tripped me up multiple times as I was setting up my test
data, and I don't think there's any advantage to allowing
inconsistencies (e.g. "/static/foo.png" and "static/foo.png"). We now
force the relative form ("static/foo.png").
@ormsbee ormsbee force-pushed the asset-integration branch from 9b2679c to fd83506 Compare October 2, 2024 18:23
@ormsbee ormsbee merged commit 2c1f717 into openedx:main Oct 2, 2024
9 checks passed
@ormsbee ormsbee deleted the asset-integration branch October 2, 2024 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants