Enforce quality checks and type hints, improve quality and typing

.github/workflows/ci.yml

@@ -16,7 +16,7 @@ jobs:
       matrix:
         os: [ubuntu-latest]  # Add macos-latest later?
         python-version: ['3.8']
-        toxenv: ["py38-django32", "py38-django42", "package"]
+        toxenv: ["py38-django32", "py38-django42", "package", "quality"]
         # We're only testing against MySQL 8 right now because 5.7 is
         # incompatible with Djagno 4.2. We'd have to make the tox.ini file more
         # complicated than it's worth given the short expected shelf-life of

.gitignore

@@ -26,6 +26,7 @@ pip-log.txt
 # Unit test / coverage reports
 .cache/
 .pytest_cache/
+.mypy_cache/
 .coverage
 .coverage.*
 .tox

manage.py

@@ -18,7 +18,7 @@
         # issue is really that Django is missing to avoid masking other
         # exceptions on Python 2.
         try:
-            import django  # pylint: disable=unused-import, wrong-import-position
+            import django  # pylint: disable=unused-import
         except ImportError as import_error:
             raise ImportError(
                 "Couldn't import Django. Are you sure it's installed and "

mypy.ini

@@ -0,0 +1,15 @@
+[mypy]
+follow_imports = normal
+ignore_missing_imports = False
+allow_untyped_globals = False
+plugins =
+    mypy_django_plugin.main,
+    mypy_drf_plugin.main
+files =
+    openedx_learning,
+    openedx_tagging,
+    tests
+
+[mypy.plugins.django-stubs]
+# content_staging only works with CMS; others work with either, so we run mypy with CMS settings.
+django_settings_module = "projects.dev"

openedx_learning/__init__.py

@@ -1 +1,4 @@
+"""
+Open edX Learning ("Learning Core")
+"""
 __version__ = "0.1.5"

openedx_learning/contrib/media_server/__init__.py

@@ -0,0 +1,3 @@
+"""
+Media server (for dev or low-traffic instances)
+"""

openedx_learning/contrib/media_server/apps.py

@@ -1,3 +1,6 @@
+"""
+Django app metadata for the Media Server application.
+"""
 from django.apps import AppConfig
 from django.core.exceptions import ImproperlyConfigured
 from django.conf import settings

openedx_learning/contrib/media_server/urls.py

@@ -1,3 +1,6 @@
+"""
+URLs for the media server application
+"""
 from django.urls import path
 
 from .views import component_asset

openedx_learning/contrib/media_server/views.py

@@ -1,3 +1,8 @@
+"""
+Views for the media server application
+
+(serves media files in dev or low-traffic instances).
+"""
 from pathlib import Path
 
 from django.http import FileResponse
@@ -28,7 +33,7 @@ def component_asset(
             learning_package_key, component_key, version_num, asset_path
         )
     except ObjectDoesNotExist:
-        raise Http404("File not found")
+        raise Http404("File not found")  # pylint: disable=raise-missing-from
 
     if not cvc.learner_downloadable and not (
         request.user and request.user.is_superuser

openedx_learning/core/components/admin.py

@@ -1,34 +1,39 @@
+"""
+Django admin for components models
+"""
 from django.contrib import admin
 from django.template.defaultfilters import filesizeformat
 from django.urls import reverse
 from django.utils.html import format_html
+from django.utils.safestring import SafeText
 
+from openedx_learning.lib.admin_utils import ReadOnlyModelAdmin
 from .models import (
     Component,
     ComponentVersion,
     ComponentVersionRawContent,
 )
-from openedx_learning.lib.admin_utils import ReadOnlyModelAdmin
 
 
 class ComponentVersionInline(admin.TabularInline):
+    """Inline admin view of ComponentVersion from the Component Admin"""
     model = ComponentVersion
     fields = ["version_num", "created", "title", "format_uuid"]
     readonly_fields = ["version_num", "created", "title", "uuid", "format_uuid"]
     extra = 0
 
+    @admin.display(description="UUID")
     def format_uuid(self, cv_obj):
         return format_html(
             '<a href="{}">{}</a>',
             reverse("admin:oel_components_componentversion_change", args=(cv_obj.pk,)),
             cv_obj.uuid,
         )
 
-    format_uuid.short_description = "UUID"
-
 
 @admin.register(Component)
 class ComponentAdmin(ReadOnlyModelAdmin):
+    """Django admin configuration for Component"""
     list_display = ("key", "uuid", "namespace", "type", "created")
     readonly_fields = [
         "learning_package",
@@ -44,6 +49,7 @@ class ComponentAdmin(ReadOnlyModelAdmin):
 
 
 class RawContentInline(admin.TabularInline):
+    """Django admin configuration for RawContent"""
     model = ComponentVersion.raw_contents.through
 
     def get_queryset(self, request):
@@ -75,11 +81,11 @@ def get_queryset(self, request):
     def rendered_data(self, cvc_obj):
         return content_preview(cvc_obj)
 
+    @admin.display(description="Size")
     def format_size(self, cvc_obj):
         return filesizeformat(cvc_obj.raw_content.size)
 
-    format_size.short_description = "Size"
-
+    @admin.display(description="Key")
     def format_key(self, cvc_obj):
         return format_html(
             '<a href="{}">{}</a>',
@@ -88,11 +94,10 @@ def format_key(self, cvc_obj):
             cvc_obj.key,
         )
 
-    format_key.short_description = "Key"
-
 
 @admin.register(ComponentVersion)
 class ComponentVersionAdmin(ReadOnlyModelAdmin):
+    """Django admin configuration for ComponentVersion"""
     readonly_fields = [
         "component",
         "uuid",
@@ -120,29 +125,8 @@ def get_queryset(self, request):
         )
 
 
-def is_displayable_text(mime_type):
-    # Our usual text files, including things like text/markdown, text/html
-    media_type, media_subtype = mime_type.split("/")
-
-    if media_type == "text":
-        return True
-
-    # Our OLX goes here, but so do some other things like
-    if media_subtype.endswith("+xml"):
-        return True
-
-    # Other application/* types that we know we can display.
-    if media_subtype in ["json", "x-subrip"]:
-        return True
-
-    # Other formats that are really specific types of JSON
-    if media_subtype.endswith("+json"):
-        return True
-
-    return False
-
-
-def link_for_cvc(cvc_obj: ComponentVersionRawContent):
+def link_for_cvc(cvc_obj: ComponentVersionRawContent) -> str:
+    """Get the download URL for the given ComponentVersionRawContent instance"""
     return "/media_server/component_asset/{}/{}/{}/{}".format(
         cvc_obj.raw_content.learning_package.key,
         cvc_obj.component_version.component.key,
@@ -151,14 +135,20 @@ def link_for_cvc(cvc_obj: ComponentVersionRawContent):
     )
 
 
-def format_text_for_admin_display(text):
+def format_text_for_admin_display(text: str) -> SafeText:
+    """
+    Get the HTML to display the given plain text (preserving formatting)
+    """
     return format_html(
         '<pre style="white-space: pre-wrap;">\n{}\n</pre>',
         text,
     )
 
 
-def content_preview(cvc_obj: ComponentVersionRawContent):
+def content_preview(cvc_obj: ComponentVersionRawContent) -> SafeText:
+    """
+    Get the HTML to display a preview of the given ComponentVersionRawContent
+    """
     raw_content_obj = cvc_obj.raw_content
 
     if raw_content_obj.mime_type.startswith("image/"):

openedx_learning/core/components/api.py

@@ -10,6 +10,8 @@
 Please look at the models.py file for more information about the kinds of data
 are stored in this app.
 """
+from __future__ import annotations
+from datetime import datetime
 from pathlib import Path
 
 from django.db.models import Q
@@ -23,8 +25,14 @@
 
 
 def create_component(
-    learning_package_id, namespace, type, local_key, created, created_by
+    learning_package_id: int,
+    namespace: str,
+    type: str,  # pylint: disable=redefined-builtin
+    local_key: str,
+    created: datetime,
+    created_by: int | None,
 ):
+    """Create a new Component (an entity like a Problem or Video)"""
     key = f"{namespace}:{type}@{local_key}"
     with atomic():
         publishable_entity = create_publishable_entity(
@@ -41,6 +49,7 @@ def create_component(
 
 
 def create_component_version(component_pk, version_num, title, created, created_by):
+    """Create a new ComponentVersion"""
     with atomic():
         publishable_entity_version = create_publishable_entity_version(
             entity_id=component_pk,
@@ -57,8 +66,15 @@ def create_component_version(component_pk, version_num, title, created, created_
 
 
 def create_component_and_version(
-    learning_package_id, namespace, type, local_key, title, created, created_by
+    learning_package_id: int,
+    namespace: str,
+    type: str,  # pylint: disable=redefined-builtin
+    local_key: str,
+    title: str,
+    created: datetime,
+    created_by: int | None,
 ):
+    """Create a Component and associated ComponentVersion atomically"""
     with atomic():
         component = create_component(
             learning_package_id, namespace, type, local_key, created, created_by
@@ -103,8 +119,12 @@ def get_component_version_content(
 
 
 def add_content_to_component_version(
-    component_version, raw_content_id, key, learner_downloadable=False
-):
+    component_version,
+    raw_content_id,
+    key,
+    learner_downloadable=False,
+) -> ComponentVersionRawContent:
+    """Add a RawContent to the given ComponentVersion"""
     cvrc, _created = ComponentVersionRawContent.objects.get_or_create(
         component_version=component_version,
         raw_content_id=raw_content_id,

openedx_learning/core/components/apps.py

@@ -1,3 +1,6 @@
+"""
+Django metadata for the Components Django application.
+"""
 from django.apps import AppConfig
 
 
@@ -15,7 +18,7 @@ def ready(self):
         """
         Register Component and ComponentVersion.
         """
-        from ..publishing.api import register_content_models
-        from .models import Component, ComponentVersion
+        from ..publishing.api import register_content_models  # pylint: disable=import-outside-toplevel
+        from .models import Component, ComponentVersion  # pylint: disable=import-outside-toplevel
 
         register_content_models(Component, ComponentVersion)

openedx_learning/core/components/models.py

@@ -16,6 +16,7 @@
 by convention, but it's possible we might want to have special identifiers
 later.
 """
+from __future__ import annotations
 from django.db import models
 
 from openedx_learning.lib.fields import (
@@ -31,7 +32,7 @@
 from ..contents.models import RawContent
 
 
-class Component(PublishableEntityMixin):
+class Component(PublishableEntityMixin):  # type: ignore[django-manager-missing]
     """
     This represents any Component that has ever existed in a LearningPackage.
 
@@ -69,6 +70,10 @@ class Component(PublishableEntityMixin):
     Make a foreign key to the Component model when you need a stable reference
     that will exist for as long as the LearningPackage itself exists.
     """
+    # Tell mypy what type our objects manager has.
+    # It's actually PublishableEntityMixinManager, but that has the exact same
+    # interface as the base manager class.
+    objects: models.Manager[Component]
 
     # This foreign key is technically redundant because we're already locked to
     # a single LearningPackage through our publishable_entity relation. However, having
@@ -143,6 +148,10 @@ class ComponentVersion(PublishableEntityVersionMixin):
     This holds the content using a M:M relationship with RawContent via
     ComponentVersionRawContent.
     """
+    # Tell mypy what type our objects manager has.
+    # It's actually PublishableEntityVersionMixinManager, but that has the exact
+    # same interface as the base manager class.
+    objects: models.Manager[ComponentVersion]
 
     # This is technically redundant, since we can get this through
     # publishable_entity_version.publishable.component, but this is more

openedx_learning/core/contents/admin.py

@@ -1,13 +1,18 @@
+"""
+Django admin for contents models
+"""
 from django.contrib import admin
 from django.utils.html import format_html
 
-from .models import RawContent
-
 from openedx_learning.lib.admin_utils import ReadOnlyModelAdmin
+from .models import RawContent
 
 
 @admin.register(RawContent)
 class RawContentAdmin(ReadOnlyModelAdmin):
+    """
+    Django admin for RawContent model
+    """
     list_display = [
         "hash_digest",
         "file_link",