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