diff --git a/learning_resources/etl/edx_shared.py b/learning_resources/etl/edx_shared.py
index 71fff4dbf3..0b77dc265e 100644
--- a/learning_resources/etl/edx_shared.py
+++ b/learning_resources/etl/edx_shared.py
@@ -15,10 +15,10 @@
from learning_resources.etl.loaders import load_content_files
from learning_resources.etl.utils import (
calc_checksum,
+ excluded_olx_paths,
get_bucket_by_name,
get_edx_module_id,
get_s3_prefix_for_source,
- staff_only_olx_paths,
transform_content_files,
)
from learning_resources.models import ContentFile, LearningResourceRun
@@ -369,27 +369,33 @@ def sync_edx_course_files(
)
-def unpublish_staff_only_content_files(
- etl_source: str, ids: list[int], keys: list[str]
-) -> int:
+def unpublish_excluded_content_files(
+ etl_source: str, ids: list[int], keys: list[str], *, dry_run: bool = False
+) -> list[dict]:
"""
- Unpublish (and deindex) content files under staff-only OLX subtrees for the
- runs matching the given archive keys, without re-extracting anything.
+ Unpublish (and deindex) content files the course does not use — staff-only
+ subtrees, asset manifests and unreferenced static files — for the runs
+ matching the given archive keys, without re-extracting anything.
Args:
etl_source(str): The edx ETL source
ids(list of int): list of course ids to process
keys(list[str]): list of S3 archive keys to search through
+ dry_run(bool): count the rows but leave them published and deindex nothing
Returns:
- int: number of content files unpublished
+ list of dict: a row per run whose archive excludes content files it has,
+ counting the excluded rows, the ones this call unpublished (or would
+ have, under dry_run) and the run's content files in total. Counts,
+ not paths, so the payload stays small enough to cross the celery
+ result backend for every run at once.
"""
from learning_resources_search import tasks as search_tasks
from vector_search import tasks as vector_tasks
bucket = get_bucket_by_name(settings.COURSE_ARCHIVE_BUCKET_NAME)
run_lookup = build_run_lookup(etl_source, ids)
- total = 0
+ rows = []
for key in keys:
matching_runs = run_lookup.get(extract_run_id_from_key(etl_source, key))
if not matching_runs:
@@ -408,23 +414,38 @@ def unpublish_staff_only_content_files(
if olx_path is None:
continue
try:
- hidden_paths = staff_only_olx_paths(olx_path)
+ excluded_paths = excluded_olx_paths(olx_path)
except ElementTree.ParseError:
log.exception("Malformed OLX in %s, skipping", key)
continue
- hidden_keys = {get_edx_module_id(str(path), run) for path in hidden_paths}
- if not hidden_keys:
+ excluded_keys = {
+ get_edx_module_id(str(path), run) for path in excluded_paths
+ }
+ if not excluded_keys:
continue
# scoped to this run: keys embed the run_id, but never rely on that alone
- hidden_files = ContentFile.objects.filter(run=run, key__in=hidden_keys)
- unpublished = hidden_files.filter(published=True).update(published=False)
- total += unpublished
- log.info(
- "Unpublished %d staff-only content files for %s", unpublished, run.run_id
- )
- # dispatched whenever hidden rows exist, not only when this call flipped
- # them, so a re-run after a failed deindex task cleans up the indexes
- if hidden_files.exists():
+ excluded_files = ContentFile.objects.filter(run=run, key__in=excluded_keys)
+ excluded = excluded_files.count()
+ if not excluded:
+ continue
+ if dry_run:
+ unpublished = excluded_files.filter(published=True).count()
+ else:
+ unpublished = excluded_files.filter(published=True).update(published=False)
+ log.info(
+ "Unpublished %d excluded content files for %s", unpublished, run.run_id
+ )
+ # dispatched whenever excluded rows exist, not only when this call
+ # flipped them, so a re-run after a failed deindex task cleans up
+ # the indexes
search_tasks.deindex_run_content_files.delay(run.id, unpublished_only=True)
vector_tasks.remove_unpublished_run_content_files.delay(run.id)
- return total
+ rows.append(
+ {
+ "run_id": run.run_id,
+ "excluded": excluded,
+ "unpublished": unpublished,
+ "total": ContentFile.objects.filter(run=run).count(),
+ }
+ )
+ return rows
diff --git a/learning_resources/etl/edx_shared_test.py b/learning_resources/etl/edx_shared_test.py
index 1d9cc34aa6..51d58c374f 100644
--- a/learning_resources/etl/edx_shared_test.py
+++ b/learning_resources/etl/edx_shared_test.py
@@ -19,7 +19,7 @@
normalize_run_id,
process_course_archive,
sync_edx_course_files,
- unpublish_staff_only_content_files,
+ unpublish_excluded_content_files,
)
from learning_resources.etl.utils import get_edx_module_id, get_s3_prefix_for_source
from learning_resources.factories import (
@@ -1670,6 +1670,8 @@ def _staff_only_archive(tmp_path) -> Path:
"vertical/v_staff.xml": '',
"html/h_staff.xml": '
',
"html/h_staff.html": "staff
",
+ # nothing links this, so it is from an earlier offering
+ "static/stale_syllabus.pdf": "stale",
}
for rel, text in files.items():
(olx / rel).parent.mkdir(parents=True, exist_ok=True)
@@ -1712,7 +1714,7 @@ def mock_deindex_tasks(mocker):
)
-def test_unpublish_staff_only_content_files(staff_only_run, mock_deindex_tasks):
+def test_unpublish_excluded_content_files(staff_only_run, mock_deindex_tasks):
"""Only the matching run's staff-only content files are unpublished and deindexed"""
run = staff_only_run.run
other_run = LearningResourceRunFactory.create(
@@ -1728,11 +1730,11 @@ def test_unpublish_staff_only_content_files(staff_only_run, mock_deindex_tasks):
for cf_key in run_keys.values():
ContentFileFactory.create(run=other_run, key=cf_key, published=True)
- unpublished = unpublish_staff_only_content_files(
+ rows = unpublish_excluded_content_files(
staff_only_run.source, [staff_only_run.course.id], [staff_only_run.key]
)
- assert unpublished == 1
+ assert [row["unpublished"] for row in rows] == [1]
assert not ContentFile.objects.filter(
run=run, key=run_keys["html/h_staff.xml"], published=True
).exists()
@@ -1744,7 +1746,7 @@ def test_unpublish_staff_only_content_files(staff_only_run, mock_deindex_tasks):
mock_deindex_tasks.qdrant.assert_called_once_with(run.id)
-def test_unpublish_staff_only_content_files_nothing_hidden(
+def test_unpublish_excluded_content_files_nothing_hidden(
staff_only_run, mock_deindex_tasks
):
"""No deindex tasks are queued when a run has no staff-only content files"""
@@ -1754,20 +1756,20 @@ def test_unpublish_staff_only_content_files_nothing_hidden(
)
assert (
- unpublish_staff_only_content_files(
+ unpublish_excluded_content_files(
staff_only_run.source, [staff_only_run.course.id], [staff_only_run.key]
)
- == 0
+ == []
)
mock_deindex_tasks.opensearch.assert_not_called()
-def test_unpublish_staff_only_content_files_malformed_archive(
+def test_unpublish_excluded_content_files_malformed_archive(
staff_only_run, mock_deindex_tasks, mocker
):
"""A malformed archive is skipped without touching its content files"""
mocker.patch(
- "learning_resources.etl.edx_shared.staff_only_olx_paths",
+ "learning_resources.etl.edx_shared.excluded_olx_paths",
side_effect=ElementTree.ParseError("bad"),
)
ContentFileFactory.create(
@@ -1777,16 +1779,16 @@ def test_unpublish_staff_only_content_files_malformed_archive(
)
assert (
- unpublish_staff_only_content_files(
+ unpublish_excluded_content_files(
staff_only_run.source, [staff_only_run.course.id], [staff_only_run.key]
)
- == 0
+ == []
)
assert ContentFile.objects.filter(run=staff_only_run.run, published=True).exists()
mock_deindex_tasks.opensearch.assert_not_called()
-def test_unpublish_staff_only_content_files_rerun_redeindexes(
+def test_unpublish_excluded_content_files_rerun_redeindexes(
staff_only_run, mock_deindex_tasks
):
"""A re-run with already-unpublished hidden files still queues the deindex tasks"""
@@ -1795,11 +1797,51 @@ def test_unpublish_staff_only_content_files_rerun_redeindexes(
run=run, key=get_edx_module_id("course/html/h_staff.xml", run), published=False
)
- assert (
- unpublish_staff_only_content_files(
- staff_only_run.source, [staff_only_run.course.id], [staff_only_run.key]
- )
- == 0
+ rows = unpublish_excluded_content_files(
+ staff_only_run.source, [staff_only_run.course.id], [staff_only_run.key]
)
+
+ # still counted as excluded, but this call had nothing left to flip
+ assert rows == [{"run_id": run.run_id, "excluded": 1, "unpublished": 0, "total": 1}]
mock_deindex_tasks.opensearch.assert_called_once_with(run.id, unpublished_only=True)
mock_deindex_tasks.qdrant.assert_called_once_with(run.id)
+
+
+def test_unpublish_excluded_content_files_drops_unreferenced_static(
+ staff_only_run, mock_deindex_tasks
+):
+ """A static file no block refers to is unpublished alongside staff-only files"""
+ run = staff_only_run.run
+ stale_key = get_edx_module_id("course/static/stale_syllabus.pdf", run)
+ ContentFileFactory.create(run=run, key=stale_key, published=True)
+
+ rows = unpublish_excluded_content_files(
+ staff_only_run.source, [staff_only_run.course.id], [staff_only_run.key]
+ )
+
+ assert [row["unpublished"] for row in rows] == [1]
+ assert not ContentFile.objects.filter(
+ run=run, key=stale_key, published=True
+ ).exists()
+
+
+def test_unpublish_excluded_content_files_dry_run(staff_only_run, mock_deindex_tasks):
+ """A dry run counts the rows but changes nothing and deindexes nothing"""
+ run = staff_only_run.run
+ ContentFileFactory.create(
+ run=run,
+ key=get_edx_module_id("course/static/stale_syllabus.pdf", run),
+ published=True,
+ )
+
+ rows = unpublish_excluded_content_files(
+ staff_only_run.source,
+ [staff_only_run.course.id],
+ [staff_only_run.key],
+ dry_run=True,
+ )
+
+ assert [row["unpublished"] for row in rows] == [1]
+ assert ContentFile.objects.filter(run=run, published=True).count() == 1
+ mock_deindex_tasks.opensearch.assert_not_called()
+ mock_deindex_tasks.qdrant.assert_not_called()
diff --git a/learning_resources/etl/utils.py b/learning_resources/etl/utils.py
index bd2a76eb41..4770a5482e 100644
--- a/learning_resources/etl/utils.py
+++ b/learning_resources/etl/utils.py
@@ -2,6 +2,7 @@
import base64
import glob
+import html
import json
import logging
import math
@@ -18,6 +19,7 @@
from io import BytesIO
from pathlib import Path
from tempfile import TemporaryDirectory
+from urllib.parse import unquote
import boto3
import pypdfium2 as pdfium
@@ -355,15 +357,12 @@ def staff_only_olx_paths(olx_path: str | Path) -> set[Path]:
course = _parse_olx_block(root, "", "course")
if course is None:
return set()
- hidden: set[Path] = set()
- seen: set[tuple[str, str]] = set()
+ hidden: dict[tuple[str, str], set[Path]] = {}
+ visible: set[tuple[str, str]] = set()
+ seen: set[tuple[str, str, bool]] = set()
stack = [(course, "course", course.get("url_name"), False)]
while stack:
pointer, tag, url_name, staff_only = stack.pop()
- if url_name:
- if (tag, url_name) in seen:
- continue
- seen.add((tag, url_name))
# pointer file wins when present; otherwise the element is the block itself
element = _parse_olx_block(root, tag, url_name) if url_name else None
if element is None:
@@ -372,19 +371,193 @@ def staff_only_olx_paths(olx_path: str | Path) -> set[Path]:
pointer.get("visible_to_staff_only"),
element.get("visible_to_staff_only"),
)
- if staff_only and url_name:
- hidden.update(_hidden_block_files(root, tag, url_name, element))
+ if url_name:
+ if (tag, url_name, staff_only) in seen:
+ continue
+ seen.add((tag, url_name, staff_only))
+ # a block can hang under two parents; seeing it anywhere a learner
+ # can reach makes it visible, whichever path the walk took first
+ if staff_only:
+ hidden[tag, url_name] = _hidden_block_files(
+ root, tag, url_name, element
+ )
+ else:
+ visible.add((tag, url_name))
stack.extend(
(child, child.tag, child.get("url_name"), staff_only) for child in element
)
- return hidden
+ return {
+ path
+ for block, files in hidden.items()
+ if block not in visible
+ for path in files
+ }
+
+
+REFERENCE_SCAN_EXTENSIONS = frozenset({".xml", ".html", ".htm", ".json", ".txt", ".md"})
+
+# Asset manifests list every file in the export and updates.items.json is mostly
+# an archive of deleted announcements. None of them describes current course
+# content, and treating them as references keeps every stale asset alive.
+NON_CONTENT_OLX_FILES = (
+ "policies/assets.json",
+ "assets/assets.xml",
+ "info/updates.items.json",
+)
+
+# A legacy transcript is named for its video's id rather than for anything the
+# course text contains, so the id is the only link back to the block using it.
+VIDEO_ID_ATTRIBUTES = ("sub", "youtube", "youtube_id_1_0")
+LEGACY_TRANSCRIPT_RE = re.compile(
+ r"^(?:[a-z]{2}(?:[-_][a-z]{2})?_)?subs_(.+)\.srt\.sjson$", re.IGNORECASE
+)
+
+
+def normalize_asset_ref(text: str) -> str:
+ """Collapse the spellings a filename takes on disk vs. in a reference"""
+ return re.sub(r"[\s_]", "", unquote(html.unescape(text)).lower())
+
+
+def _olx_reference_sources(root: Path, skip: set[Path]) -> list[Path]:
+ """Files whose text may legitimately refer to a static asset"""
+ sources = []
+ for path in root.rglob("*"):
+ if not path.is_file() or path.suffix.lower() not in REFERENCE_SCAN_EXTENSIONS:
+ continue
+ relative = path.relative_to(root)
+ if (
+ relative.parts[0] == "static"
+ or relative.as_posix() in NON_CONTENT_OLX_FILES
+ or path in skip
+ or any("draft" in part for part in relative.parts[:-1])
+ ):
+ continue
+ sources.append(path)
+ return sources
+
+
+def _live_course_updates(root: Path) -> str:
+ """
+ Text of the announcements the course team has not deleted. The whole file is
+ excluded as a reference source because edX keeps deleted announcements in
+ the export, but a live one still counts.
+ """
+ try:
+ items = json.loads(
+ (root / "info/updates.items.json").read_text(errors="ignore")
+ )
+ except (OSError, ValueError):
+ return ""
+ return "\n".join(
+ item.get("content") or ""
+ for item in items
+ if isinstance(item, dict) and item.get("status") != "deleted"
+ )
+
+
+def _olx_video_ids(sources: list[Path]) -> set[str] | None:
+ """
+ Video ids declared anywhere in the course, or None if any source could not
+ be parsed. None means "ids unknown", and callers keep every legacy
+ transcript rather than drop one whose video they failed to read.
+ """
+ ids = set()
+ for path in sources:
+ if path.suffix.lower() != ".xml":
+ continue
+ try:
+ element = ElementTree.parse(path).getroot()
+ except ElementTree.ParseError:
+ log.warning("Malformed XML in %s, keeping all legacy transcripts", path)
+ return None
+ # iter() finds ')
+ _write_olx(olx, "html/h_staff.html", 'key')
+ _write_olx(olx, "static/answers.pdf", "answers")
+ assert "static/answers.pdf" not in _olx_source_paths(olx)
+
+
+def test_documents_from_olx_drafts_do_not_keep_assets(tmp_path):
+ """Drafts are not ingested, so their links do not keep an asset alive"""
+ olx = _reference_olx(tmp_path, **{"draft_only.pdf": "pdf"})
+ _write_olx(olx, "html/h.html", "no links
")
+ _write_olx(olx, "drafts/html/d.html", 'd')
+ assert "static/draft_only.pdf" not in _olx_source_paths(olx)
+
+
+@pytest.mark.parametrize(
+ "video_xml",
+ [
+ # every spelling of the attribute is valid XML and must be honoured
+ '',
+ "",
+ '',
+ '',
+ '',
+ '',
+ ],
+)
+def test_documents_from_olx_keeps_legacy_transcripts(tmp_path, video_xml):
+ """subs_.srt.sjson is named for the video id, never for its own filename"""
+ olx = _reference_olx(
+ tmp_path,
+ **{"subs_AbC123.srt.sjson": "{}", "es_subs_AbC123.srt.sjson": "{}"},
+ )
+ _write_olx(olx, "html/h.html", "no links
")
+ _write_olx(olx, "vertical/v.xml", '')
+ _write_olx(olx, "video/vid.xml", video_xml)
+ paths = _olx_source_paths(olx)
+ assert "static/subs_AbC123.srt.sjson" in paths
+ assert "static/es_subs_AbC123.srt.sjson" in paths
+
+
+def test_documents_from_olx_drops_orphaned_legacy_transcripts(tmp_path):
+ """A legacy transcript whose video is gone belongs to an earlier offering"""
+ olx = _reference_olx(
+ tmp_path, **{"subs_Current.srt.sjson": "{}", "subs_Removed.srt.sjson": "{}"}
+ )
+ _write_olx(olx, "html/h.html", "no links
")
+ _write_olx(olx, "vertical/v.xml", '')
+ _write_olx(olx, "video/vid.xml", '')
+ paths = _olx_source_paths(olx)
+ assert "static/subs_Current.srt.sjson" in paths
+ assert "static/subs_Removed.srt.sjson" not in paths
+
+
+def test_documents_from_olx_keeps_inline_video_transcripts(tmp_path):
+ """A inline in a parent declares its transcripts just as a file does"""
+ olx = _reference_olx(tmp_path, **{"subs_Inline.srt.sjson": "{}"})
+ _write_olx(olx, "html/h.html", "no links
")
+ _write_olx(
+ olx,
+ "vertical/v.xml",
+ '',
+ )
+ assert "static/subs_Inline.srt.sjson" in _olx_source_paths(olx)
+
+
+def test_documents_from_olx_unparsable_xml_keeps_legacy_transcripts(tmp_path):
+ """Unknown video ids must not be read as "no video uses this transcript\""""
+ olx = _reference_olx(tmp_path, **{"subs_AbC123.srt.sjson": "{}"})
+ _write_olx(olx, "html/h.html", "no links
")
+ _write_olx(olx, "tabs/broken.xml", "
+
+ The excluded total counts archive paths, so it is an upper bound on what
+ unpublish_excluded_files would unpublish rather than a row count: several
+ paths can collapse onto one ContentFile key.
+ """
+
+ help = "Report the files an OLX course tree contains but does not use"
+
+ def add_arguments(self, parser):
+ parser.add_argument(
+ "olx_paths", nargs="+", help="Paths to extracted OLX course directories"
+ )
+
+ def handle(self, *args, **options): # noqa: ARG002
+ """Print per-archive exclusion counts split by location and file class"""
+ for olx_path in options["olx_paths"]:
+ root = Path(olx_path)
+ ingestable = {
+ path
+ for path in root.rglob("*")
+ if path.is_file()
+ and path.suffix.lower() in VALID_TEXT_FILE_TYPES
+ and not any(
+ "draft" in part for part in path.relative_to(root).parts[:-1]
+ )
+ }
+ excluded = excluded_olx_paths(root) & ingestable
+ static = {
+ path for path in excluded if path.relative_to(root).parts[0] == "static"
+ }
+ transcripts = {
+ path for path in static if path.suffix.lower() in TRANSCRIPT_EXTENSIONS
+ }
+ percent = 100 * len(excluded) / len(ingestable) if ingestable else 0
+ self.stdout.write(f"\n=== {root}")
+ self.stdout.write(f" ingestable files : {len(ingestable)}")
+ self.stdout.write(
+ f" excluded : {len(excluded)} ({percent:.0f}%)"
+ )
+ self.stdout.write(f" under static/ : {len(static)}")
+ self.stdout.write(f" transcripts : {len(transcripts)}")
+ self.stdout.write(f" documents : {len(static - transcripts)}")
+ self.stdout.write(
+ f" elsewhere : {len(excluded - static)}"
+ " (staff-only blocks, manifests, announcements)"
+ )
diff --git a/learning_resources/management/commands/unpublish_excluded_files.py b/learning_resources/management/commands/unpublish_excluded_files.py
new file mode 100644
index 0000000000..f261380a44
--- /dev/null
+++ b/learning_resources/management/commands/unpublish_excluded_files.py
@@ -0,0 +1,112 @@
+"""Unpublish edX content files that the course itself does not use"""
+
+import csv
+from operator import itemgetter
+from pathlib import Path
+
+from django.core.management import BaseCommand
+
+from learning_resources.etl.constants import ETLSource
+from learning_resources.tasks import unpublish_all_excluded_files
+from main import settings
+from main.utils import now_in_utc
+
+EDX_SOURCES = [
+ ETLSource.mitxonline.name,
+ ETLSource.mit_edx.name,
+ ETLSource.xpro.name,
+ ETLSource.oll.name,
+]
+
+
+REPORT_FIELDS = ("etl_source", "run_id", "excluded", "unpublished", "total")
+
+
+def _sum(rows, field):
+ return sum(row[field] for row in rows)
+
+
+class Command(BaseCommand):
+ """
+ Walk each course's current archive and unpublish the content files it
+ excludes — staff-only subtrees, asset manifests, and static files no block
+ refers to — then deindex them. Nothing is re-extracted or re-embedded.
+ """
+
+ help = "Unpublish unused edX content files from existing archives"
+
+ def add_arguments(self, parser):
+ parser.add_argument(
+ "--source",
+ dest="sources",
+ action="append",
+ choices=EDX_SOURCES,
+ help="ETL source to process (repeatable). Default: all edX sources",
+ )
+ parser.add_argument(
+ "-c",
+ "--chunk-size",
+ dest="chunk_size",
+ default=settings.LEARNING_COURSE_ITERATOR_CHUNK_SIZE,
+ type=int,
+ help="Chunk size for batch task",
+ )
+ parser.add_argument(
+ "--resource-ids",
+ dest="learning_resource_ids",
+ required=False,
+ help="If set, only process the learning resources with these ids",
+ )
+ parser.add_argument(
+ "--dry-run",
+ dest="dry_run",
+ action="store_true",
+ help="Report what would be unpublished without changing anything",
+ )
+ parser.add_argument(
+ "--report",
+ dest="report",
+ required=False,
+ help="Also write the per-run counts as CSV to this path",
+ )
+
+ def handle(self, *args, **options): # noqa: ARG002
+ """Run the unpublish tasks"""
+ resource_ids = (
+ options["learning_resource_ids"].split(",")
+ if options["learning_resource_ids"]
+ else None
+ )
+ start = now_in_utc()
+ report = []
+ for source in options["sources"] or EDX_SOURCES:
+ task = unpublish_all_excluded_files.delay(
+ etl_source=source,
+ chunk_size=options["chunk_size"],
+ learning_resource_ids=resource_ids,
+ dry_run=options["dry_run"],
+ )
+ self.stdout.write(f"Started task {task} for {source}, waiting...")
+ rows = [row for chunk in task.get() or [] for row in chunk or []]
+ for row in sorted(rows, key=itemgetter("run_id")):
+ self.stdout.write(
+ f"{source} run {row['run_id']}: {row['excluded']} out of "
+ f"{row['total']} content files excluded"
+ )
+ verb = "would unpublish" if options["dry_run"] else "unpublished"
+ self.stdout.write(
+ f"{source} summary: {_sum(rows, 'excluded')} out of "
+ f"{_sum(rows, 'total')} content files excluded across "
+ f"{len(rows)} run(s), {verb} {_sum(rows, 'unpublished')}"
+ )
+ report.extend({"etl_source": source, **row} for row in rows)
+
+ if options["report"]:
+ with Path(options["report"]).open("w", newline="") as report_file:
+ writer = csv.DictWriter(report_file, fieldnames=REPORT_FIELDS)
+ writer.writeheader()
+ writer.writerows(sorted(report, key=itemgetter("etl_source", "run_id")))
+ self.stdout.write(f"Wrote {len(report)} rows to {options['report']}")
+
+ total_seconds = (now_in_utc() - start).total_seconds()
+ self.stdout.write(f"Finished in {total_seconds} seconds")
diff --git a/learning_resources/management/commands/unpublish_staff_only_files.py b/learning_resources/management/commands/unpublish_staff_only_files.py
deleted file mode 100644
index a03e57d2c8..0000000000
--- a/learning_resources/management/commands/unpublish_staff_only_files.py
+++ /dev/null
@@ -1,69 +0,0 @@
-"""Unpublish edX content files that sit under staff-only OLX subtrees"""
-
-from django.core.management import BaseCommand
-
-from learning_resources.etl.constants import ETLSource
-from learning_resources.tasks import unpublish_all_staff_only_files
-from main import settings
-from main.utils import now_in_utc
-
-EDX_SOURCES = [
- ETLSource.mitxonline.name,
- ETLSource.mit_edx.name,
- ETLSource.xpro.name,
- ETLSource.oll.name,
-]
-
-
-class Command(BaseCommand):
- """
- Walk each course's current archive and unpublish content files under
- visible_to_staff_only subtrees, then deindex them. Nothing is re-extracted
- or re-embedded.
- """
-
- help = "Unpublish staff-only edX content files from existing archives"
-
- def add_arguments(self, parser):
- parser.add_argument(
- "--source",
- dest="sources",
- action="append",
- choices=EDX_SOURCES,
- help="ETL source to process (repeatable). Default: all edX sources",
- )
- parser.add_argument(
- "-c",
- "--chunk-size",
- dest="chunk_size",
- default=settings.LEARNING_COURSE_ITERATOR_CHUNK_SIZE,
- type=int,
- help="Chunk size for batch task",
- )
- parser.add_argument(
- "--resource-ids",
- dest="learning_resource_ids",
- required=False,
- help="If set, only process the learning resources with these ids",
- )
-
- def handle(self, *args, **options): # noqa: ARG002
- """Run the unpublish tasks"""
- resource_ids = (
- options["learning_resource_ids"].split(",")
- if options["learning_resource_ids"]
- else None
- )
- start = now_in_utc()
- for source in options["sources"] or EDX_SOURCES:
- task = unpublish_all_staff_only_files.delay(
- etl_source=source,
- chunk_size=options["chunk_size"],
- learning_resource_ids=resource_ids,
- )
- self.stdout.write(f"Started task {task} for {source}, waiting...")
- results = task.get()
- total = sum(count or 0 for count in results or [])
- self.stdout.write(f"{source}: unpublished {total} content files")
- total_seconds = (now_in_utc() - start).total_seconds()
- self.stdout.write(f"Finished in {total_seconds} seconds")
diff --git a/learning_resources/tasks.py b/learning_resources/tasks.py
index 7cc3a6df4f..73a0224ed0 100644
--- a/learning_resources/tasks.py
+++ b/learning_resources/tasks.py
@@ -29,7 +29,7 @@
get_most_recent_course_archives,
sync_edx_archive,
sync_edx_course_files,
- unpublish_staff_only_content_files,
+ unpublish_excluded_content_files,
)
from learning_resources.etl.loaders import (
load_learning_materials,
@@ -232,27 +232,37 @@ def _content_file_resource_ids(etl_source: str, learning_resource_ids):
@app.task(acks_late=True, reject_on_worker_lost=True)
-def unpublish_staff_only_files(ids: list[int], etl_source: str, keys: list[str]):
- """Unpublish staff-only content files for a chunk of courses"""
- return unpublish_staff_only_content_files(etl_source, ids, keys)
+def unpublish_excluded_files(
+ ids: list[int], etl_source: str, keys: list[str], *, dry_run: bool = False
+):
+ """Unpublish unused content files for a chunk of courses, a row per run"""
+ return unpublish_excluded_content_files(etl_source, ids, keys, dry_run=dry_run)
@app.task(bind=True)
-def unpublish_all_staff_only_files(
- self, *, etl_source, chunk_size=None, learning_resource_ids=None
+def unpublish_all_excluded_files(
+ self, *, etl_source, chunk_size=None, learning_resource_ids=None, dry_run=False
):
- """Fan out unpublish_staff_only_files over an edX source's current archives"""
+ """Fan out unpublish_excluded_files over an edX source's current archives"""
if chunk_size is None:
chunk_size = settings.LEARNING_COURSE_ITERATOR_CHUNK_SIZE
archive_keys = get_most_recent_course_archives(etl_source)
+ # a run with no content files has none to unpublish, and skipping it saves
+ # downloading and extracting its archive to find that out. Not applied to
+ # the ingestion fan-out, where a course with no content files yet is
+ # exactly the one that needs its archive read.
+ resource_ids = (
+ _content_file_resource_ids(etl_source, learning_resource_ids)
+ .filter(runs__content_files__isnull=False)
+ .distinct()
+ )
return self.replace(
celery.group(
[
- unpublish_staff_only_files.si(ids, etl_source, archive_keys)
- for ids in chunks(
- _content_file_resource_ids(etl_source, learning_resource_ids),
- chunk_size=chunk_size,
+ unpublish_excluded_files.si(
+ ids, etl_source, archive_keys, dry_run=dry_run
)
+ for ids in chunks(resource_ids, chunk_size=chunk_size)
]
)
)
diff --git a/learning_resources/tasks_test.py b/learning_resources/tasks_test.py
index 5ac6ca66ad..47b24e0bb3 100644
--- a/learning_resources/tasks_test.py
+++ b/learning_resources/tasks_test.py
@@ -1554,11 +1554,11 @@ def test_get_podcast_transcripts(mocker):
@mock_aws
-def test_unpublish_all_staff_only_files(
+def test_unpublish_all_excluded_files(
settings, mocker, mocked_celery, mock_course_archive_bucket
):
- """unpublish_all_staff_only_files fans out one task per chunk of course ids"""
- mock_task = mocker.patch("learning_resources.tasks.unpublish_staff_only_files.si")
+ """Only courses whose runs have content files are fanned out"""
+ mock_task = mocker.patch("learning_resources.tasks.unpublish_excluded_files.si")
mocker.patch("learning_resources.tasks.load_course_blocklist", return_value=[])
mocker.patch(
"learning_resources.tasks.get_most_recent_course_archives",
@@ -1569,22 +1569,33 @@ def test_unpublish_all_staff_only_files(
courses = factories.CourseFactory.create_batch(
3, etl_source=etl_source, platform=PlatformType.mitxonline.name
)
+ # the third course has no content files, so its archive is never downloaded
+ with_files = courses[:2]
+ for course in with_files:
+ factories.ContentFileFactory.create(
+ run=factories.LearningResourceRunFactory.create(
+ learning_resource=course.learning_resource
+ )
+ )
with pytest.raises(mocked_celery.replace_exception_class):
- tasks.unpublish_all_staff_only_files.delay(
+ tasks.unpublish_all_excluded_files.delay(
etl_source=etl_source, chunk_size=2, learning_resource_ids=None
)
- assert mock_task.call_count == 2
+ assert mock_task.call_count == 1
called_ids = sorted(
rid for call in mock_task.call_args_list for rid in call.args[0]
)
- assert called_ids == sorted(c.learning_resource_id for c in courses)
- mock_task.assert_any_call(ANY, etl_source, ["foo.tar.gz"])
+ assert called_ids == sorted(c.learning_resource_id for c in with_files)
+ mock_task.assert_any_call(ANY, etl_source, ["foo.tar.gz"], dry_run=False)
-def test_unpublish_staff_only_files_task(mocker):
- """unpublish_staff_only_files task delegates to edx_shared"""
+def test_unpublish_excluded_files_task(mocker):
+ """unpublish_excluded_files task delegates to edx_shared"""
mock_fn = mocker.patch(
- "learning_resources.tasks.unpublish_staff_only_content_files", return_value=3
+ "learning_resources.tasks.unpublish_excluded_content_files",
+ return_value=[{"run_id": "r", "excluded": 3, "unpublished": 3, "total": 9}],
)
- assert tasks.unpublish_staff_only_files([1, 2], "mitxonline", ["k"]) == 3
- mock_fn.assert_called_once_with("mitxonline", [1, 2], ["k"])
+ assert tasks.unpublish_excluded_files([1, 2], "mitxonline", ["k"]) == [
+ {"run_id": "r", "excluded": 3, "unpublished": 3, "total": 9}
+ ]
+ mock_fn.assert_called_once_with("mitxonline", [1, 2], ["k"], dry_run=False)
diff --git a/main/celery.py b/main/celery.py
index 6a83648815..8fedd258f1 100644
--- a/main/celery.py
+++ b/main/celery.py
@@ -30,8 +30,8 @@
"learning_resources.tasks.import_all_xpro_files": {"queue": "edx_content"},
"learning_resources.tasks.import_all_mit_edx_files": {"queue": "edx_content"},
"learning_resources.tasks.import_all_mitxonline_files": {"queue": "edx_content"},
- "learning_resources.tasks.unpublish_staff_only_files": {"queue": "edx_content"},
- "learning_resources.tasks.unpublish_all_staff_only_files": {"queue": "edx_content"},
+ "learning_resources.tasks.unpublish_excluded_files": {"queue": "edx_content"},
+ "learning_resources.tasks.unpublish_all_excluded_files": {"queue": "edx_content"},
"learning_resources_search.tasks.index_run_content_files": {"queue": "edx_content"},
"learning_resources_search.tasks.deindex_run_content_files": {
"queue": "edx_content"