openedx / openedx-learning

GNU Affero General Public License v3.0
5 stars 11 forks source link

Asset handling updates (config, admin) #234

Closed ormsbee closed 1 month ago

ormsbee commented 1 month ago

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 https://github.com/overhangio/tutor/pull/1124 lands.

Screenshots:

admin-content-preview admin-component-asset

ormsbee commented 1 month ago

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

ormsbee commented 1 month ago

Now updated with tests.