refactor: incorporate suggestions from Kyle's review

olx_importer/management/commands/load_components.py

@@ -122,9 +122,9 @@ def create_content(self, static_local_path, now, component_version):
             mime_type=mime_type,
             created=now,
         )
-        components_api.add_content_to_component_version(
+        components_api.create_component_version_content(
             component_version,
-            content_id=content.id,
+            content.id,
             key=key,
             learner_downloadable=True,
         )
@@ -173,9 +173,9 @@ def import_block_type(self, block_type_name, now): # , publish_log_entry):
                 created=now,
             )
             # Add the OLX source text to the ComponentVersion
-            components_api.add_content_to_component_version(
+            components_api.create_component_version_content(
                 component_version,
-                content_id=text_content.pk,
+                text_content.pk,
                 key="block.xml",
                 learner_downloadable=False
             )

openedx_learning/__init__.py

@@ -1,4 +1,4 @@
 """
 Open edX Learning ("Learning Core").
 """
-__version__ = "0.5.2"
+__version__ = "0.6.0"

openedx_learning/contrib/media_server/views.py

@@ -8,7 +8,7 @@
 from django.core.exceptions import ObjectDoesNotExist, PermissionDenied
 from django.http import FileResponse, Http404
 
-from openedx_learning.core.components.api import get_component_version_content
+from openedx_learning.core.components.api import look_up_component_version_content
 
 
 def component_asset(
@@ -28,7 +28,7 @@ def component_asset(
     * Serving from a different domain than the rest of the service
     """
     try:
-        cvc = get_component_version_content(
+        cvc = look_up_component_version_content(
             learning_package_key, component_key, version_num, asset_path
         )
     except ObjectDoesNotExist:

openedx_learning/core/components/api.py

@@ -285,17 +285,19 @@ def get_components(
         qset = qset.filter(component_type__name__in=type_names)
     if draft_title is not None:
         qset = qset.filter(
-            publishable_entity__draft__version__title__icontains=draft_title
+            Q(publishable_entity__draft__version__title__icontains=draft_title) |
+            Q(local_key__icontains=draft_title)
         )
     if published_title is not None:
         qset = qset.filter(
-            publishable_entity__published__version__title__icontains=published_title
+            Q(publishable_entity__published__version__title__icontains=published_title) |
+            Q(local_key__icontains=published_title)
         )
 
     return qset
 
 
-def get_component_version_content(
+def look_up_component_version_content(
     learning_package_key: str,
     component_key: str,
     version_num: int,
@@ -323,10 +325,10 @@ def get_component_version_content(
                                   ).get(queries)
 
 
-def add_content_to_component_version(
+def create_component_version_content(
     component_version_id: int,
-    /,
     content_id: int,
+    /,
     key: str,
     learner_downloadable=False,
 ) -> ComponentVersionContent:

openedx_learning/core/contents/api.py

@@ -64,8 +64,8 @@ def get_content(content_id: int, /) -> Content:
 
 def get_or_create_text_content(
     learning_package_id: int,
-    /,
     media_type_id: int,
+    /,
     text: str,
     created: datetime,
     create_file: bool = False,
@@ -86,10 +86,6 @@ def get_or_create_text_content(
     If you want to create a large text file, or want to create a text file that
     doesn't need to be stored in the database, call ``create_file_content``
     instead of this function.
-
-    The `hash_digest` is included as an optional argument because a very common
-    pattern is going to be "I have this data, let's see if a Content already
-    exists for it."
     """
     text_as_bytes = text.encode('utf-8')
     hash_digest = create_hash_digest(text_as_bytes)
@@ -122,18 +118,19 @@ def get_or_create_text_content(
 
 def get_or_create_file_content(
     learning_package_id: int,
-    /,
     media_type_id: int,
+    /,
     data: bytes,
     created: datetime,
 ) -> Content:
     """
     Get or create a Content with data stored in a file storage backend.
 
     Use this function to store non-text data, large data, or data where low
-    latency access is not necessary. Also use this function to store any Content
-    that you want to be downloadable by browsers in the LMS since the static
-    asset serving system will only work with file-backed Content.
+    latency access is not necessary. Also use this function (or
+    ``get_or_create_text_content`` with ``create_file=True``) to store any
+    Content that you want to be downloadable by browsers in the LMS, since the
+    static asset serving system will only work with file-backed Content.
     """
     hash_digest = create_hash_digest(data)
     with atomic():

openedx_learning/core/contents/models.py

@@ -18,7 +18,7 @@
 from ..publishing.models import LearningPackage
 
 
-def get_storage_class() -> Storage:
+def get_storage() -> Storage:
     """
     Return the Storage instance for our Content file persistence.
 
@@ -227,7 +227,7 @@ class Content(models.Model):
     # This hash value may be calculated using create_hash_digest from the
     # openedx.lib.fields module. When storing text, we hash the UTF-8
     # encoding of that text value, regardless of whether we also write it to a
-    # file or not.
+    # file or not. When storing just a file, we hash the bytes in the file.
     hash_digest = hash_field()
 
     # Do we have file data stored for this Content in our file storage backend?
@@ -263,17 +263,28 @@ class Content(models.Model):
     created = manual_date_time_field()
 
     @cached_property
-    def mime_type(self):
+    def mime_type(self) -> str:
+        """
+        The IANA media type (a.k.a. MIME type) of the Content, in string form.
+
+        MIME types reference:
+          https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types
+        """
         return str(self.media_type)
 
     def file_path(self):
+        """
+        Path at which this content is stored (or would be stored).
+
+        This path is relative to configured storage root.
+        """
         return f"{self.learning_package.uuid}/{self.hash_digest}"
 
-    def write_file(self, file: File):
+    def write_file(self, file: File) -> None:
         """
         Write file contents to the file storage backend.
         """
-        storage = get_storage_class()
+        storage = get_storage()
         file_path = self.file_path()
 
         # There are two reasons why a file might already exist even if the the
@@ -290,11 +301,11 @@ def write_file(self, file: File):
         if not storage.exists(file_path):
             storage.save(file_path, file)
 
-    def file_url(self):
+    def file_url(self) -> str:
         """
         This will sometimes be a time-limited signed URL.
         """
-        return get_storage_class().url(self.file_path())
+        return get_storage().url(self.file_path())
 
     def clean(self):
         """

openedx_learning/core/publishing/models.py

@@ -30,10 +30,11 @@ class LearningPackage(models.Model):  # type: ignore[django-manager-missing]
 
     Each PublishableEntity belongs to exactly one LearningPackage.
     """
+    # Explictly declare a 4-byte ID instead of using the app-default 8-byte ID.
     # We do not expect to have more than 2 billion LearningPackages on a given
-    # site, but many, many things have foreign keys to this model and uniqueness
-    # indexes on those foreign keys + their own fields, so going from the
-    # app-default of 8-byte IDs to 4-byte IDs will add up over time.
+    # site. Furthermore, many, many things have foreign keys to this model and
+    # uniqueness indexes on those foreign keys + their own fields, so the 4
+    # bytes saved will add up over time.
     id = models.AutoField(primary_key=True)
 
     uuid = immutable_uuid_field()
@@ -366,6 +367,21 @@ class PublishLog(models.Model):
     Open question: Empty publishes are allowed at this time, and might be useful
     for "fake" publishes that are necessary to invoke other post-publish
     actions. It's not clear at this point how useful this will actually be.
+
+    The absence of a ``version_num`` field in this model is intentional, because
+    having one would potentially cause write contention/locking issues when
+    there are many people working on different entities in a very large library.
+    We already see some contention issues occuring in ModuleStore for courses,
+    and we want to support Libraries that are far larger.
+
+    If you need a LearningPackage-wide indicator for version and the only thing
+    you care about is "has *something* changed?", you can make a foreign key to
+    the most recent PublishLog, or use the most recent PublishLog's primary key.
+    This should be monotonically increasing, though there will be large gaps in
+    values, e.g. (5, 190, 1291, etc.). Be warned that this value will not port
+    across sites. If you need site-portability, the UUIDs for this model are a
+    safer bet, though there's a lot about import/export that we haven't fully
+    mapped out yet.
     """
 
     uuid = immutable_uuid_field()

tests/openedx_learning/core/components/test_api.py

@@ -100,15 +100,15 @@ def setUpTestData(cls) -> None:
         cls.published_problem, _version = components_api.create_component_and_version(
             cls.learning_package.id,
             component_type=v2_problem_type,
-            local_key="published_problem",
+            local_key="pp_lk",
             title="Published Problem",
             created=cls.now,
             created_by=None,
         )
         cls.published_html, _version = components_api.create_component_and_version(
             cls.learning_package.id,
             component_type=cls.html_type,
-            local_key="published_html",
+            local_key="ph_lk",
             title="Published HTML",
             created=cls.now,
             created_by=None,
@@ -122,15 +122,15 @@ def setUpTestData(cls) -> None:
         cls.unpublished_problem, _version = components_api.create_component_and_version(
             cls.learning_package.id,
             component_type=v2_problem_type,
-            local_key="unpublished_problem",
+            local_key="upp_lk",
             title="Unpublished Problem",
             created=cls.now,
             created_by=None,
         )
         cls.unpublished_html, _version = components_api.create_component_and_version(
             cls.learning_package.id,
             component_type=cls.html_type,
-            local_key="unpublished_html",
+            local_key="uph_lk",
             title="Unpublished HTML",
             created=cls.now,
             created_by=None,
@@ -141,7 +141,7 @@ def setUpTestData(cls) -> None:
         cls.deleted_video, _version = components_api.create_component_and_version(
             cls.learning_package.id,
             component_type=cls.video_type,
-            local_key="deleted_video",
+            local_key="dv_lk",
             title="Deleted Video",
             created=cls.now,
             created_by=None,
@@ -369,13 +369,13 @@ def test_add(self):
         )
         new_content = contents_api.get_or_create_text_content(
             self.learning_package.pk,
-            media_type_id=self.text_media_type.id,
+            self.text_media_type.id,
             text="This is some data",
             created=self.now,
         )
-        components_api.add_content_to_component_version(
+        components_api.create_component_version_content(
             new_version.pk,
-            content_id=new_content.pk,
+            new_content.pk,
             key="hello.txt",
             learner_downloadable=False,
         )
@@ -391,19 +391,19 @@ def test_add(self):
     def test_multiple_versions(self):
         hello_content = contents_api.get_or_create_text_content(
             self.learning_package.id,
-            media_type_id=self.text_media_type.id,
+            self.text_media_type.id,
             text="Hello World!",
             created=self.now,
         )
         goodbye_content = contents_api.get_or_create_text_content(
             self.learning_package.id,
-            media_type_id=self.text_media_type.id,
+            self.text_media_type.id,
             text="Goodbye World!",
             created=self.now,
         )
         blank_content = contents_api.get_or_create_text_content(
             self.learning_package.id,
-            media_type_id=self.text_media_type.id,
+            self.text_media_type.id,
             text="",
             created=self.now,
         )