Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,24 @@

## Unreleased

- [Feature] Support Tutor 20.x (Teak), in addition to the already-supported
21.x/22.x (Ulmo/Verawood). Teak's edx-platform and course-discovery still
use Django's old `DEFAULT_FILE_STORAGE` string setting instead of the
`STORAGES` dict, so every settings patch that touches `STORAGES` now
branches on `"STORAGES" in globals()` rather than the Django version
(Teak itself runs Django 4.2.20 too, so a version check would get this
backwards - this is the same presence-detection idiom edx-platform's own
`resolve_storage_backend()` uses). Profile images get the same full S3
treatment as Verawood/Ulmo in both dev and production: a thin wrapper
storage class (referenced via a self-referential `__name__`-based path)
strips the `base_url` key Teak's `lms/urls.py` still needs before
delegating to `S3Boto3Storage`. Verified live against real Tutor 20.0.5
(Teak), 21.0.9 (Ulmo) and 22.0.2 (Verawood) environments: LMS, CMS and
Discovery boot cleanly and read/write through all three backends on every
version, including `tutor local launch` on Teak for the production
(`DEBUG=False`) code path. `pyproject.toml`'s `tutor` dependency now reads
`>=20.0.0,<23.0.0`.

- [Bugfix] Profile images never reached object storage under `tutor dev`,
in all three backends - two compounding bugs, both masked by `tutor
local` never hitting the code paths that expose them. (1) Each backend
Expand Down
4 changes: 2 additions & 2 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@ classifiers = [
"Programming Language :: Python :: 3.12",
]
dependencies = [
"tutor>=21.0.0,<23.0.0",
"tutor>=20.0.0,<23.0.0",
]
# these fields will be set by hatch_build.py
dynamic = ["version"]

[project.optional-dependencies]
dev = [
"tutor[dev]>=21.0.0,<23.0.0",
"tutor[dev]>=20.0.0,<23.0.0",
"ruff",
"pytest",
"pyyaml",
Expand Down
241 changes: 240 additions & 1 deletion tests/test_patches.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@

import importlib
import re
import sys
import types
import typing as t

import pytest
Expand Down Expand Up @@ -144,7 +146,22 @@ def test_profile_image_s3_override_applies_in_both_dev_and_production(
"override never rendered - profile images would stay on local "
"disk under this settings file"
)
block = rendered[start : rendered.index("\n}\n", start) + 3]
# Brace-depth scan, not a literal "\n}\n" search: the dict literal is
# now nested inside an `if "STORAGES" in globals():` branch (Teak
# compat), so its closing brace is indented and would otherwise be
# skipped in favor of some unrelated later top-level "}" - silently
# swallowing the Teak else-branch's unrelated code into "block".
open_brace = rendered.index("{", start)
depth = 0
end = open_brace
for end, ch in enumerate(rendered[open_brace:], start=open_brace):
if ch == "{":
depth += 1
elif ch == "}":
depth -= 1
if depth == 0:
break
block = rendered[start : end + 1]
assert f'"bucket_name": "{bucket_name}"' in block, (
f"{backend}/lms/{settings_file}: STORAGES['profile_image'] did "
"not pick up the configured bucket name"
Expand All @@ -156,6 +173,228 @@ def test_profile_image_s3_override_applies_in_both_dev_and_production(
)


# --- Teak (Tutor 20.x) backward compatibility: exec, not just compile ------
# test_python_patches_are_syntactically_valid only compile()s - it would
# pass on code that raises NameError/ImproperlyConfigured the instant it
# actually runs, exactly the bug these patches fix (confirmed live against a
# real Tutor 20.0.5 + Teak edx-platform: `NameError: name 'STORAGES' is not
# defined`). These exec() the real rendered patches against two fabricated
# namespaces standing in for each era's pre-existing settings-module state.
# This is the durable regression guard: CI always resolves the newest Tutor
# satisfying pyproject.toml's range, so it will never actually install Tutor
# 20.x itself even after widening that range.


class _FakeS3Boto3Storage:
"""Stand-in for storages.backends.s3boto3.S3Boto3Storage: django-storages
is a dependency of the edx-platform venv these patches actually execute
inside, not of this plugin's own test environment."""

def __init__(self, **kwargs: t.Any) -> None:
self.kwargs = kwargs


class _FakeBotoConfig:
"""Stand-in for botocore.client.Config: botocore/boto3 are dependencies
of the edx-platform venv these patches actually execute inside, not of
this plugin's own test environment (confirmed: whether `import botocore`
succeeds here depends on what happens to be installed transitively in
whichever venv runs these tests, so it must be stubbed rather than relied
on)."""

def __init__(self, **kwargs: t.Any) -> None:
self.kwargs = kwargs


def _install_fake_s3_runtime_modules(monkeypatch: pytest.MonkeyPatch) -> None:
s3boto3_mod = types.ModuleType("storages.backends.s3boto3")
s3boto3_mod.S3Boto3Storage = _FakeS3Boto3Storage # type: ignore[attr-defined]
monkeypatch.setitem(sys.modules, "storages", types.ModuleType("storages"))
monkeypatch.setitem(
sys.modules, "storages.backends", types.ModuleType("storages.backends")
)
monkeypatch.setitem(sys.modules, "storages.backends.s3boto3", s3boto3_mod)

client_mod = types.ModuleType("botocore.client")
client_mod.Config = _FakeBotoConfig # type: ignore[attr-defined]
monkeypatch.setitem(sys.modules, "botocore", types.ModuleType("botocore"))
monkeypatch.setitem(sys.modules, "botocore.client", client_mod)


def _base_namespace() -> dict[str, t.Any]:
return {
"__name__": "lms.envs.tutor.development",
"VIDEO_IMAGE_SETTINGS": {"STORAGE_KWARGS": {"location": "/video-images/"}},
"VIDEO_TRANSCRIPTS_SETTINGS": {
"STORAGE_KWARGS": {"location": "/video-transcripts/"}
},
"GRADES_DOWNLOAD": {"STORAGE_KWARGS": {"location": "/grades/"}},
}


def _teak_namespace() -> dict[str, t.Any]:
"""No STORAGES, no OPENEDX_LEARNING - confirmed absent from Teak's real
edx-platform source even though it runs Django 4.2.20. PROFILE_IMAGE_
BACKEND is in the old shape get_profile_image_storage() reads directly,
base_url included (lms/urls.py's dev-mode static() route needs it)."""
ns = _base_namespace()
ns["PROFILE_IMAGE_BACKEND"] = {
"class": "storages.backends.s3boto3.S3Boto3Storage",
"options": {
"location": "profile-images/",
"base_url": "/media/profile-images/",
},
}
return ns


def _verawood_namespace() -> dict[str, t.Any]:
"""STORAGES/OPENEDX_LEARNING pre-populated, matching Verawood/Ulmo's
real edx-platform settings-module state."""
ns = _base_namespace()
ns["STORAGES"] = {
"default": {"BACKEND": "django.core.files.storage.FileSystemStorage"}
}
ns["OPENEDX_LEARNING"] = {"MEDIA": {}}
ns["PROFILE_IMAGE_BACKEND"] = {
"class": "openedx.core.storage.OverwriteStorage",
"options": {"location": "profile-images/"},
}
return ns


@pytest.mark.parametrize("backend", BACKENDS)
def test_shared_settings_exec_cleanly_on_teak(
backend: str,
rendered_by_backend: dict[str, dict[str, str]],
monkeypatch: pytest.MonkeyPatch,
) -> None:
_install_fake_s3_runtime_modules(monkeypatch)
ns = _teak_namespace()
exec(rendered_by_backend[backend]["openedx-common-settings"], ns) # must not raise

assert "STORAGES" not in ns
assert ns["DEFAULT_FILE_STORAGE"] == "storages.backends.s3boto3.S3Boto3Storage"
assert "OPENEDX_LEARNING" not in ns, (
"OPENEDX_LEARNING doesn't exist in Teak's edx-platform at all - "
"the guard must not create it"
)


@pytest.mark.parametrize("backend", BACKENDS)
def test_shared_settings_exec_cleanly_on_verawood(
backend: str,
rendered_by_backend: dict[str, dict[str, str]],
monkeypatch: pytest.MonkeyPatch,
) -> None:
_install_fake_s3_runtime_modules(monkeypatch)
ns = _verawood_namespace()
exec(rendered_by_backend[backend]["openedx-common-settings"], ns) # must not raise

assert "DEFAULT_FILE_STORAGE" not in ns
assert (
ns["STORAGES"]["default"]["BACKEND"]
== "storages.backends.s3boto3.S3Boto3Storage"
)
assert (
ns["OPENEDX_LEARNING"]["MEDIA"]["BACKEND"]
== "storages.backends.s3boto3.S3Boto3Storage"
)


@pytest.mark.parametrize("backend", BACKENDS)
def test_lms_profile_image_exec_cleanly_on_teak(
backend: str,
rendered_by_backend: dict[str, dict[str, str]],
monkeypatch: pytest.MonkeyPatch,
) -> None:
_install_fake_s3_runtime_modules(monkeypatch)
ns = _teak_namespace()
exec(rendered_by_backend[backend]["openedx-common-settings"], ns)
exec(
rendered_by_backend[backend]["openedx-lms-common-settings"], ns
) # must not raise

assert "STORAGES" not in ns
backend_cls = ns["PROFILE_IMAGE_BACKEND"]["class"]
assert (
backend_cls == "lms.envs.tutor.development._ObjstorageProfileImageS3Storage"
), (
"the wrapper class must be referenced via a self-referential "
"__name__-based dotted path, not a hardcoded module"
)
options = ns["PROFILE_IMAGE_BACKEND"]["options"]
assert "base_url" in options, (
"lms/urls.py's dev-mode static() route reads "
"PROFILE_IMAGE_BACKEND['options']['base_url'] directly - it must stay"
)

# The wrapper itself must actually strip base_url before delegating,
# confirmed live against the real Teak image - S3Storage otherwise
# rejects it as an unrecognized constructor kwarg.
wrapper_cls = ns["_ObjstorageProfileImageS3Storage"]
instance = wrapper_cls(**options)
assert "base_url" not in instance.kwargs
assert instance.kwargs["location"] == options["location"]


@pytest.mark.parametrize("backend", BACKENDS)
def test_lms_profile_image_exec_cleanly_on_verawood(
backend: str,
rendered_by_backend: dict[str, dict[str, str]],
monkeypatch: pytest.MonkeyPatch,
) -> None:
_install_fake_s3_runtime_modules(monkeypatch)
ns = _verawood_namespace()
exec(rendered_by_backend[backend]["openedx-common-settings"], ns)
exec(
rendered_by_backend[backend]["openedx-lms-common-settings"], ns
) # must not raise

assert ns["STORAGES"]["profile_image"]["OPTIONS"]["location"] == "profile-images/"
assert "_ObjstorageProfileImageS3Storage" not in ns
assert (
ns["PROFILE_IMAGE_BACKEND"]["class"] == "openedx.core.storage.OverwriteStorage"
)


@pytest.mark.parametrize("backend", BACKENDS)
def test_discovery_settings_exec_cleanly_on_teak(
backend: str,
rendered_by_backend: dict[str, dict[str, str]],
monkeypatch: pytest.MonkeyPatch,
) -> None:
_install_fake_s3_runtime_modules(monkeypatch)
ns: dict[str, t.Any] = {}
# must not raise
exec(rendered_by_backend[backend]["discovery-common-settings"], ns)

assert "STORAGES" not in ns
assert ns["DEFAULT_FILE_STORAGE"] == "storages.backends.s3boto3.S3Boto3Storage"


@pytest.mark.parametrize("backend", BACKENDS)
def test_discovery_settings_exec_cleanly_on_verawood(
backend: str,
rendered_by_backend: dict[str, dict[str, str]],
monkeypatch: pytest.MonkeyPatch,
) -> None:
_install_fake_s3_runtime_modules(monkeypatch)
ns: dict[str, t.Any] = {
"STORAGES": {
"default": {"BACKEND": "django.core.files.storage.FileSystemStorage"}
}
}
# must not raise
exec(rendered_by_backend[backend]["discovery-common-settings"], ns)

assert "DEFAULT_FILE_STORAGE" not in ns
assert (
ns["STORAGES"]["default"]["BACKEND"]
== "storages.backends.s3boto3.S3Boto3Storage"
)


def test_caddyfile_is_empty_for_external_s3(
rendered_by_backend: dict[str, dict[str, str]],
) -> None:
Expand Down
4 changes: 4 additions & 0 deletions tests/test_plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -233,3 +233,7 @@ def test_tutor_dependency_does_not_exclude_current_tutor_releases() -> None:
"excludes tutor 22.0.2, the version every other current Tutor plugin "
"requires - this would silently downgrade Tutor on install"
)
assert tutor_requirement.specifier.contains(Version("20.0.5"), prereleases=True), (
f"tutor-objectstorage's own 'tutor' dependency ({tutor_requirement.specifier}) "
"excludes tutor 20.0.5 (Teak) - this plugin is meant to support Teak too"
)
6 changes: 5 additions & 1 deletion tutorobjectstorage/patches/azure/discovery-common-settings
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
{% if OBJSTORAGE_BACKEND == "azure" and OBJSTORAGE_AZURE_ACKNOWLEDGE_RISK %}
# rclone S3 gateway
STORAGES["default"]["BACKEND"] = "storages.backends.s3boto3.S3Boto3Storage"
# Teak's course-discovery still uses DEFAULT_FILE_STORAGE (STORAGES arrives ~ulmo.1); presence-detection, not a django.VERSION check, for the same reason as the LMS/CMS patch.
if "STORAGES" in globals():
STORAGES["default"]["BACKEND"] = "storages.backends.s3boto3.S3Boto3Storage"
else:
DEFAULT_FILE_STORAGE = "storages.backends.s3boto3.S3Boto3Storage"
AWS_ACCESS_KEY_ID = "{{ OPENEDX_AWS_ACCESS_KEY }}"
AWS_SECRET_ACCESS_KEY = "{{ OPENEDX_AWS_SECRET_ACCESS_KEY }}"
AWS_S3_SIGNATURE_VERSION = "s3v4"
Expand Down
29 changes: 23 additions & 6 deletions tutorobjectstorage/patches/azure/openedx-lms-common-settings
Original file line number Diff line number Diff line change
@@ -1,11 +1,28 @@
{# STORAGES["profile_image"], not PROFILE_IMAGE_BACKEND: get_profile_image_storage() prefers it, and S3Storage rejects PROFILE_IMAGE_BACKEND's base_url kwarg. #}
{# Verawood/Ulmo: STORAGES["profile_image"], not PROFILE_IMAGE_BACKEND - get_profile_image_storage() prefers it, and S3Storage rejects PROFILE_IMAGE_BACKEND's base_url kwarg.
Teak: get_profile_image_storage() only ever reads PROFILE_IMAGE_BACKEND directly, spreading its whole "options" dict as constructor kwargs - so base_url has to stay in that dict for lms/urls.py's dev-mode static() route, while S3Storage itself rejects that same kwarg. A thin wrapper class that discards it before delegating resolves both needs without touching edx-platform. Confirmed live against the real Teak image. #}
{% if OBJSTORAGE_BACKEND == "azure" and OBJSTORAGE_AZURE_ACKNOWLEDGE_RISK %}
STORAGES["profile_image"] = {
"BACKEND": STORAGES["default"]["BACKEND"],
"OPTIONS": {
if "STORAGES" in globals():
STORAGES["profile_image"] = {
"BACKEND": STORAGES["default"]["BACKEND"],
"OPTIONS": {
"bucket_name": "{{ OBJSTORAGE_BUCKET_NAME }}",
"querystring_auth": False,
"location": PROFILE_IMAGE_BACKEND["options"]["location"].lstrip("/"),
},
}
else:
from storages.backends.s3boto3 import S3Boto3Storage as _objstorage_s3_base

class _ObjstorageProfileImageS3Storage(_objstorage_s3_base):
def __init__(self, **kwargs):
kwargs.pop("base_url", None)
super().__init__(**kwargs)

PROFILE_IMAGE_BACKEND["class"] = __name__ + "._ObjstorageProfileImageS3Storage"
PROFILE_IMAGE_BACKEND["options"] = {
"bucket_name": "{{ OBJSTORAGE_BUCKET_NAME }}",
"querystring_auth": False,
"location": PROFILE_IMAGE_BACKEND["options"]["location"].lstrip("/"),
},
}
"base_url": PROFILE_IMAGE_BACKEND["options"]["base_url"],
}
{% endif %}
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
{% if OBJSTORAGE_BACKEND == "external_s3" %}
# External S3-compatible object storage
STORAGES["default"]["BACKEND"] = "storages.backends.s3boto3.S3Boto3Storage"
# Teak's course-discovery still uses DEFAULT_FILE_STORAGE (STORAGES arrives ~ulmo.1); presence-detection, not a django.VERSION check, for the same reason as the LMS/CMS patch.
if "STORAGES" in globals():
STORAGES["default"]["BACKEND"] = "storages.backends.s3boto3.S3Boto3Storage"
else:
DEFAULT_FILE_STORAGE = "storages.backends.s3boto3.S3Boto3Storage"
AWS_ACCESS_KEY_ID = "{{ OPENEDX_AWS_ACCESS_KEY }}"
AWS_SECRET_ACCESS_KEY = "{{ OPENEDX_AWS_SECRET_ACCESS_KEY }}"
AWS_S3_SIGNATURE_VERSION = "s3v4"
Expand Down
Loading
Loading