diff --git a/openedx_learning/apps/authoring/components/admin.py b/openedx_learning/apps/authoring/components/admin.py index f09483e9..2dc6f441 100644 --- a/openedx_learning/apps/authoring/components/admin.py +++ b/openedx_learning/apps/authoring/components/admin.py @@ -1,6 +1,8 @@ """ Django admin for components models """ +import base64 + from django.contrib import admin from django.template.defaultfilters import filesizeformat from django.urls import reverse @@ -67,19 +69,22 @@ def get_queryset(self, request): ) fields = [ - "format_key", + "key", "format_size", "learner_downloadable", "rendered_data", ] readonly_fields = [ "content", - "format_key", + "key", "format_size", "rendered_data", ] extra = 0 + def has_file(self, cvc_obj): + return cvc_obj.content.has_file + def rendered_data(self, cvc_obj): return content_preview(cvc_obj) @@ -87,15 +92,6 @@ def rendered_data(self, cvc_obj): def format_size(self, cvc_obj): return filesizeformat(cvc_obj.content.size) - @admin.display(description="Key") - def format_key(self, cvc_obj): - return format_html( - '{}', - link_for_cvc(cvc_obj), - # reverse("admin:components_content_change", args=(cvc_obj.content_id,)), - cvc_obj.key, - ) - @admin.register(ComponentVersion) class ComponentVersionAdmin(ReadOnlyModelAdmin): @@ -129,18 +125,6 @@ def get_queryset(self, request): ) -def link_for_cvc(cvc_obj: ComponentVersionContent) -> str: - """ - Get the download URL for the given ComponentVersionContent instance - """ - return "/media_server/component_asset/{}/{}/{}/{}".format( - cvc_obj.content.learning_package.key, - cvc_obj.component_version.component.key, - cvc_obj.component_version.version_num, - cvc_obj.key, - ) - - def format_text_for_admin_display(text: str) -> SafeText: """ Get the HTML to display the given plain text (preserving formatting) @@ -158,9 +142,17 @@ def content_preview(cvc_obj: ComponentVersionContent) -> SafeText: content_obj = cvc_obj.content if content_obj.media_type.type == "image": + # This base64 encoding looks really goofy and is bad for performance, + # but image previews in the admin are extremely useful, and this lets us + # have them without creating a separate view in Learning Core. (Keep in + # mind that these assets are private, so they cannot be accessed via the + # MEDIA_URL like most Django uploaded assets.) + data = content_obj.read_file().read() return format_html( - '', - content_obj.file_url(), + '
{}
', + content_obj.mime_type, + base64.encodebytes(data).decode('utf8'), + content_obj.storage_path(), ) return format_text_for_admin_display( diff --git a/openedx_learning/apps/authoring/contents/admin.py b/openedx_learning/apps/authoring/contents/admin.py index 036d171b..3da5135c 100644 --- a/openedx_learning/apps/authoring/contents/admin.py +++ b/openedx_learning/apps/authoring/contents/admin.py @@ -1,6 +1,8 @@ """ Django admin for contents models """ +import base64 + from django.contrib import admin from django.utils.html import format_html @@ -16,7 +18,6 @@ class ContentAdmin(ReadOnlyModelAdmin): """ list_display = [ "hash_digest", - "file_link", "learning_package", "media_type", "size", @@ -29,24 +30,34 @@ class ContentAdmin(ReadOnlyModelAdmin): "media_type", "size", "created", - "file_link", - "text_preview", "has_file", + "file_path", + "storage_path", + "text_preview", + "image_preview", ] list_filter = ("media_type", "learning_package") search_fields = ("hash_digest",) - def file_link(self, content: Content): - if not content.has_file: - return "" + def storage_path(self, content: Content): + return content.storage_path() if content.has_file else "" - return format_html( - 'Download', - content.file_url(), - ) + def file_path(self, content: Content): + return content.file_path() if content.has_file else "" def text_preview(self, content: Content): return format_html( '
\n{}\n
', content.text, ) + + def image_preview(self, content: Content): + if content.media_type.type != "image": + return "" + + data = content.read_file().read() + return format_html( + '', + content.mime_type, + base64.encodebytes(data).decode('utf8'), + ) diff --git a/openedx_learning/apps/authoring/contents/models.py b/openedx_learning/apps/authoring/contents/models.py index 7e4ff8cc..4c4b9e2e 100644 --- a/openedx_learning/apps/authoring/contents/models.py +++ b/openedx_learning/apps/authoring/contents/models.py @@ -7,11 +7,13 @@ from functools import cache, cached_property -from django.core.exceptions import ValidationError +from django.conf import settings +from django.core.exceptions import ImproperlyConfigured, ValidationError from django.core.files.base import File from django.core.files.storage import Storage, default_storage from django.core.validators import MaxValueValidator from django.db import models +from django.utils.module_loading import import_string from ....lib.fields import MultiCollationTextField, case_insensitive_char_field, hash_field, manual_date_time_field from ....lib.managers import WithRelationsManager @@ -28,12 +30,26 @@ def get_storage() -> Storage: """ Return the Storage instance for our Content file persistence. - For right now, we're still only storing inline text and not static assets in - production, so just return the default_storage. We're also going through a - transition between Django 3.2 -> 4.2, where storage configuration has moved. + This will first search for an OPENEDX_LEARNING config dictionary and return + a Storage subclass based on that configuration. - Make this work properly as part of adding support for static assets. + If there is no value for the OPENEDX_LEARNING setting, we return the default + MEDIA storage class. TODO: Should we make it just error instead? """ + config_dict = getattr(settings, 'OPENEDX_LEARNING', {}) + + if 'MEDIA' in config_dict: + storage_cls = import_string(config_dict['MEDIA']['BACKEND']) + options = config_dict['MEDIA'].get('OPTIONS', {}) + return storage_cls(**options) + + raise ImproperlyConfigured( + "Cannot access file storage: Missing the OPENEDX_LEARNING['MEDIA'] " + "setting, which should have a storage BACKEND and OPTIONS values for " + "a Storage subclass. These files should be stored in a location that " + "is NOT publicly accessible to browsers (so not in the MEDIA_ROOT)." + ) + return default_storage @@ -284,17 +300,34 @@ def mime_type(self) -> str: def file_path(self): """ - Path at which this content is stored (or would be stored). + Logical path at which this content is stored (or would be stored). - This path is relative to configured storage root. + This path is relative to OPENEDX_LEARNING['MEDIA'] configured storage + root. + """ + return f"content/{self.learning_package.uuid}/{self.hash_digest}" + + def storage_path(self): + """ + The full OS path for the underlying file for this Content. + + This will not be supported by all Storage class types. + """ + return get_storage().path(self.file_path()) + + def read_file(self) -> File: + """ + Get a File object that has been open for reading. """ - return f"{self.learning_package.uuid}/{self.hash_digest}" + return get_storage().open(self.file_path(), 'rb') def write_file(self, file: File) -> None: """ Write file contents to the file storage backend. - This function does nothing if the file already exists. + This function does nothing if the file already exists. Note that Content + is supposed to be immutable, so this should normally only be called once + for a given Content row. """ storage = get_storage() file_path = self.file_path() diff --git a/test_settings.py b/test_settings.py index db27f354..17becdf5 100644 --- a/test_settings.py +++ b/test_settings.py @@ -69,3 +69,11 @@ def root(*args): 'DEFAULT_PAGINATION_CLASS': 'edx_rest_framework_extensions.paginators.DefaultPagination', 'PAGE_SIZE': 10, } + +######################## LEARNING CORE SETTINGS ######################## + +OPENEDX_LEARNING = { + 'MEDIA': { + 'BACKEND': 'django.core.files.storage.InMemoryStorage' + } +}