From a1593da9edf6f744d2168ece07e9d630955a1efd Mon Sep 17 00:00:00 2001 From: Anastasia Beglova Date: Thu, 24 Sep 2026 15:47:38 -0400 Subject: [PATCH 1/9] add back schedule and fix admin criteria text field (#3989) --- learning_resources/admin.py | 40 ++++++++- learning_resources/admin_test.py | 87 ++++++++++++++++++- .../commands/generate_credential_metadata.py | 3 +- main/settings_celery.py | 8 ++ main/settings_test.py | 20 +++++ 5 files changed, 153 insertions(+), 5 deletions(-) diff --git a/learning_resources/admin.py b/learning_resources/admin.py index 6204a9c421..7f08e78c83 100644 --- a/learning_resources/admin.py +++ b/learning_resources/admin.py @@ -2,10 +2,33 @@ from django.contrib import admin from django.contrib.admin import TabularInline +from django.contrib.admin.widgets import AdminTextareaWidget +from django.contrib.postgres.fields import ArrayField +from django.contrib.postgres.forms import SimpleArrayField from learning_resources import models +class LineSeparatedArrayField(SimpleArrayField): + """ + Edit an ArrayField as one item per line instead of one comma-separated line. + """ + + def __init__(self, base_field, **kwargs): + kwargs.setdefault("delimiter", "\n") + super().__init__(base_field, **kwargs) + + def to_python(self, value): + """Split on lines, dropping the blank ones and any carriage returns""" + if isinstance(value, str): + # A textarea submits CRLF, and a trailing newline or a gap + # between entries would otherwise become an empty item. + value = self.delimiter.join( + line.strip() for line in value.splitlines() if line.strip() + ) + return super().to_python(value) + + class LearningResourceInstructorAdmin(admin.ModelAdmin): """Instructor Admin""" @@ -342,10 +365,25 @@ class CredentialMetadataAdmin(admin.ModelAdmin): """CredentialMetadata Admin""" model = models.CredentialMetadata - list_display = ("learning_resource", "description", "created_on", "updated_on") + list_display = ( + "learning_resource", + "description", + "criteria", + "created_on", + "updated_on", + ) search_fields = ("learning_resource__readable_id", "learning_resource__title") readonly_fields = ("created_on", "updated_on") raw_id_fields = ("learning_resource",) + # `criteria` is the only ArrayField here. Without this it renders as a + # one-line TextInput beside `description`'s textarea, too small to read + # the values it holds. + formfield_overrides = { + ArrayField: { + "form_class": LineSeparatedArrayField, + "widget": AdminTextareaWidget, + } + } admin.site.register(models.LearningResourceTopic, LearningResourceTopicAdmin) diff --git a/learning_resources/admin_test.py b/learning_resources/admin_test.py index d2adafbe06..5e6d9f6be1 100644 --- a/learning_resources/admin_test.py +++ b/learning_resources/admin_test.py @@ -2,15 +2,17 @@ import pytest from django.contrib.admin.sites import site +from django.contrib.admin.widgets import AdminTextareaWidget from django.test import RequestFactory from django.urls import reverse -from learning_resources.admin import TutorProblemFileAdmin +from learning_resources.admin import CredentialMetadataAdmin, TutorProblemFileAdmin from learning_resources.factories import ( + CredentialMetadataFactory, LearningResourceRunFactory, TutorProblemFileFactory, ) -from learning_resources.models import TutorProblemFile +from learning_resources.models import CredentialMetadata, TutorProblemFile @pytest.mark.django_db @@ -27,3 +29,84 @@ def test_tutor_problem_file_changelist_query_count( model_admin = TutorProblemFileAdmin(TutorProblemFile, site) with django_assert_num_queries(3): model_admin.changelist_view(request).render() + + +@pytest.fixture +def criteria_field(): + """Return the `criteria` form field as the admin change form builds it""" + model_admin = CredentialMetadataAdmin(CredentialMetadata, site) + return model_admin.get_form(None)().fields["criteria"] + + +@pytest.mark.django_db +def test_credential_metadata_criteria_is_a_textarea(criteria_field): + """ + `criteria` gets a textarea, like `description` beside it. + + An ArrayField otherwise renders through SimpleArrayField, a CharField + subclass, so it lands in a one-line TextInput too small to read the + criteria it holds. + """ + assert isinstance(criteria_field.widget, AdminTextareaWidget) + + +@pytest.mark.django_db +def test_credential_metadata_criteria_is_one_per_line(criteria_field): + """The stored list is shown a criterion per line, and read back the same""" + stored = ["Applied conservation laws", "Modelled fluid flow"] + + shown = criteria_field.prepare_value(stored) + + assert shown == "Applied conservation laws\nModelled fluid flow" + assert criteria_field.clean(shown) == stored + + +@pytest.mark.django_db +def test_credential_metadata_criteria_keeps_a_comma(criteria_field): + """ + A criterion containing a comma survives a round trip. + + Comma-separated, `SimpleArrayField` would cut this one in two on save, + with no way to escape it -- and criteria are prose, so commas are + ordinary. + """ + stored = ["Applied conservation laws, including mass and momentum"] + + assert criteria_field.clean(criteria_field.prepare_value(stored)) == stored + + +@pytest.mark.django_db +@pytest.mark.parametrize( + ("submitted", "expected"), + [ + # Browsers submit CRLF from a textarea. + ("One\r\nTwo", ["One", "Two"]), + # A trailing newline or a gap between entries is not an empty + # criterion. + ("One\n\n\nTwo\n", ["One", "Two"]), + (" \n", []), + ("", []), + ], +) +def test_credential_metadata_criteria_cleans_whitespace( + criteria_field, submitted, expected +): + """Line endings and blank lines do not become criteria""" + assert criteria_field.clean(submitted) == expected + + +@pytest.mark.django_db +def test_credential_metadata_change_view_renders(admin_user): + """The change form itself still renders with the overridden field""" + stored = CredentialMetadataFactory.create(criteria=["Did a thing"]) + request = RequestFactory().get( + reverse("admin:learning_resources_credentialmetadata_change", args=(stored.id,)) + ) + request.user = admin_user + + response = CredentialMetadataAdmin(CredentialMetadata, site).change_view( + request, str(stored.id) + ) + + assert response.status_code == 200 + assert b"Did a thing" in response.render().content diff --git a/learning_resources/management/commands/generate_credential_metadata.py b/learning_resources/management/commands/generate_credential_metadata.py index 76020706ec..9a26222d29 100644 --- a/learning_resources/management/commands/generate_credential_metadata.py +++ b/learning_resources/management/commands/generate_credential_metadata.py @@ -52,6 +52,5 @@ def handle(self, *args, **options): # noqa: ARG002 self.stdout.write( "Generation runs in the background, roughly a minute per resource." - " Follow the celery logs for progress and completion:" + " Follow the celery logs for progress." ) - self.stdout.write(" docker compose logs -f celery") diff --git a/main/settings_celery.py b/main/settings_celery.py index a5c1e93eba..b613e8c035 100644 --- a/main/settings_celery.py +++ b/main/settings_celery.py @@ -226,6 +226,14 @@ minute=0, hour=4 ), # 04:00 UTC (midnight ET during DST, 11pm ET during standard time) }, + "generate-credential-metadata-every-1-days": { + "task": "learning_resources.tasks.generate_all_credential_metadata", + "schedule": crontab(minute=0, hour=11), # 7:00am EDT / 6:00am EST + # Gaps only. An overwriting sweep regenerates the whole MITx + # Online catalogue at full LLM cost every day; the non-overwriting + # one queues nothing once the catalogue is filled. + "kwargs": {"overwrite": False}, + }, } ) diff --git a/main/settings_test.py b/main/settings_test.py index ae48751e45..596b6f8f85 100644 --- a/main/settings_test.py +++ b/main/settings_test.py @@ -418,6 +418,26 @@ def test_program_certificates_beat_entry_present_with_starrocks_configured(self) assert entry["task"] == "profiles.tasks.SyncProgramCertificatesTask" assert entry["kwargs"] == {"full_refresh": True} + def test_credential_metadata_beat_entry(self): + """ + The credential metadata sweep is scheduled, and fills gaps only. + + An overwriting sweep regenerates the whole MITx Online catalogue at + full LLM cost every day, so `overwrite` being False here is the thing + worth pinning. No `resource_types`, so the sweep covers every type + credential metadata is generated for. + """ + with mock.patch.dict("os.environ", REQUIRED_SETTINGS, clear=True): + settings_vars = self.reload_settings(module="main.settings_celery") + entry = settings_vars["CELERY_BEAT_SCHEDULE"][ + "generate-credential-metadata-every-1-days" + ] + assert ( + entry["task"] + == "learning_resources.tasks.generate_all_credential_metadata" + ) + assert entry["kwargs"] == {"overwrite": False} + def _assert_s3_storage_config( self, storages_dict, From faae2d216f486f611608e22fa232beaba58ecc06 Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Thu, 24 Sep 2026 16:21:37 -0400 Subject: [PATCH 2/9] Re-ingest content files of republished runs, fix inconsistent contentfile (de)indexing (#3976) --- learning_resources/etl/canvas.py | 45 +-- learning_resources/etl/canvas_test.py | 106 ++++-- learning_resources/etl/edx_shared.py | 19 +- learning_resources/etl/edx_shared_test.py | 106 +++++- learning_resources/etl/loaders.py | 10 +- learning_resources/etl/loaders_test.py | 19 ++ learning_resources/utils.py | 7 +- learning_resources/utils_test.py | 43 +++ learning_resources_search/indexing_api.py | 61 +++- .../indexing_api_test.py | 106 +++++- learning_resources_search/plugins.py | 42 ++- learning_resources_search/plugins_test.py | 98 +++++- learning_resources_search/tasks.py | 315 +++++++----------- learning_resources_search/tasks_test.py | 241 +++++++++++--- learning_resources_search/utils.py | 32 ++ learning_resources_search/utils_test.py | 78 ++++- vector_search/tasks.py | 32 +- vector_search/tasks_test.py | 28 ++ vector_search/utils.py | 13 + vector_search/utils_test.py | 42 +++ 20 files changed, 1070 insertions(+), 373 deletions(-) diff --git a/learning_resources/etl/canvas.py b/learning_resources/etl/canvas.py index 5bc667371b..1cf752d064 100644 --- a/learning_resources/etl/canvas.py +++ b/learning_resources/etl/canvas.py @@ -32,13 +32,7 @@ LearningResourcePlatform, LearningResourceRun, ) -from learning_resources.utils import ( - bulk_resources_unpublished_actions, - resource_unpublished_actions, -) -from learning_resources_search.constants import ( - CONTENT_FILE_TYPE, -) +from learning_resources.utils import resource_unpublished_actions from main.utils import checksum_for_content log = logging.getLogger(__name__) @@ -184,7 +178,13 @@ def run_for_canvas_archive(course_archive_path, course_folder, checksum, overwri ) run = resource.runs.first() resource_readable_id = run.learning_resource.readable_id - if run.checksum == checksum and not overwrite: + # rows that are all unpublished were stripped by a bulk deindex, not by + # the export (which never unpublishes every row), so reload them + stale_run = ( + run.content_files.exists() + and not run.content_files.filter(published=True).exists() + ) + if run.checksum == checksum and not overwrite and not stale_run: log.debug("Checksums match for %s, skipping load", readable_id) return resource_readable_id, None return resource_readable_id, run @@ -201,8 +201,10 @@ def transform_canvas_content_files( """ Transform published content files from a Canvas course zipfile - Files whose extraction fails are skipped and their existing records - are retained (not deleted/unpublished). + Files whose extraction fails are skipped and their keys added to + failed_keys. Files no longer in the archive are unpublished by + load_content_files and purged from both indexes by its + content_files_loaded hook. """ basedir = course_zipfile.name.split(".")[0] zipfile_path = course_zipfile.absolute() @@ -247,24 +249,13 @@ def _generate_content(): yield content_data # use subgenerator for yielding content data - published_keys = [] - for content_data in _generate_content(): - full_path = Path(basedir) / Path(content_data["source_path"]) - published_keys.append(get_edx_module_id(str(full_path), run)) - yield content_data + yield from _generate_content() # files whose extraction failed are retained, not treated as unpublished - for source_path in failed_source_paths: - full_path = Path(basedir) / Path(source_path) - failed_key = get_edx_module_id(str(full_path), run) - published_keys.append(failed_key) - if failed_keys is not None: - failed_keys.append(failed_key) - unpublished_content = run.content_files.exclude(key__in=published_keys) - # remove unpublished contentfiles - bulk_resources_unpublished_actions( - list(unpublished_content.values_list("id", flat=True)), CONTENT_FILE_TYPE - ) - unpublished_content.delete() + if failed_keys is not None: + failed_keys.extend( + get_edx_module_id(str(Path(basedir) / Path(source_path)), run) + for source_path in failed_source_paths + ) def transform_canvas_problem_files( diff --git a/learning_resources/etl/canvas_test.py b/learning_resources/etl/canvas_test.py index d1a215cd95..f9092f9cd4 100644 --- a/learning_resources/etl/canvas_test.py +++ b/learning_resources/etl/canvas_test.py @@ -35,6 +35,7 @@ parse_web_content, ) from learning_resources.etl.constants import ETLSource +from learning_resources.etl.loaders import load_content_files from learning_resources.etl.utils import get_edx_module_id, process_olx_path from learning_resources.factories import ( ContentFileFactory, @@ -44,11 +45,11 @@ TutorProblemFileFactory, ) from learning_resources.models import ContentFile, LearningResource -from learning_resources_search.constants import CONTENT_FILE_TYPE from main.utils import now_in_utc pytestmark = pytest.mark.django_db + DEFAULT_SETTINGS_XML = b""" diff --git a/learning_resources/etl/edx_shared.py b/learning_resources/etl/edx_shared.py index 4f940ec255..0378036996 100644 --- a/learning_resources/etl/edx_shared.py +++ b/learning_resources/etl/edx_shared.py @@ -133,7 +133,15 @@ def process_course_archive( Returns: bool: False if skipped via matching archive_key, True otherwise """ - if run.archive_key == key and not overwrite: + # A saved checksum means this archive once produced published rows. If + # none are left (a bulk deindex unpublished them, cleanup may have deleted + # them since), the run is stale and must not skip the load. An empty + # archive records archive_key with no checksum, so it still skips. + stale_run = ( + bool(run.checksum) and not run.content_files.filter(published=True).exists() + ) + + if run.archive_key == key and not overwrite and not stale_run: log.debug("Archive key unchanged for %s, skipping download", key) return False with TemporaryDirectory() as export_tempdir: @@ -145,7 +153,7 @@ def process_course_archive( except tarfile.ReadError: log.exception("Error reading tar file %s, skipping", course_tarpath) return True - if run.checksum == checksum and not overwrite: + if run.checksum == checksum and not overwrite and not stale_run: # unchanged content under a new key: record it to skip future downloads run.archive_key = key run.save(update_fields=["archive_key"]) @@ -163,9 +171,12 @@ def process_course_archive( if failed_keys: # every file failed: retry next sync, don't mark as empty return True - # empty archive: stop re-downloading it + # empty archive: stop re-downloading it. Drop any checksum + # from an earlier ingest so a stale run doesn't keep + # forcing the download. run.archive_key = key - run.save(update_fields=["archive_key"]) + run.checksum = None + run.save(update_fields=["archive_key", "checksum"]) return True content_files_ids = load_content_files( run, chain([first], content_files_data), failed_keys=failed_keys diff --git a/learning_resources/etl/edx_shared_test.py b/learning_resources/etl/edx_shared_test.py index f60a5f378e..d7a49844e7 100644 --- a/learning_resources/etl/edx_shared_test.py +++ b/learning_resources/etl/edx_shared_test.py @@ -141,6 +141,7 @@ def test_sync_edx_course_files_matching_checksum(mocker, mock_course_archive_buc run.learning_resource.runs.exclude(id=run.id).first() run.checksum = "123" run.save() + ContentFileFactory.create(run=run) mocker.patch( "learning_resources.etl.edx_shared.calc_checksum", return_value=run.checksum ) @@ -1436,7 +1437,7 @@ def test_process_course_archive_does_not_set_checksum_on_exception(mocker): ) mocker.patch( "learning_resources.etl.edx_shared.transform_content_files", - return_value=iter([]), + return_value=iter([{"key": "content.txt"}]), ) mocker.patch( "learning_resources.etl.edx_shared.load_content_files", @@ -1455,6 +1456,7 @@ def test_process_course_archive_skips_download_when_key_matches(mocker): run = LearningResourceRunFactory.create( published=True, archive_key=key, checksum="oldchecksum" ) + ContentFileFactory.create(run=run) bucket = mocker.MagicMock() mock_load = mocker.patch("learning_resources.etl.edx_shared.load_content_files") @@ -1473,6 +1475,7 @@ def test_process_course_archive_stamps_key_on_checksum_match(mocker): run = LearningResourceRunFactory.create( published=True, archive_key=None, checksum="samechecksum" ) + ContentFileFactory.create(run=run) bucket = mocker.MagicMock() mocker.patch( "learning_resources.etl.edx_shared.calc_checksum", return_value="samechecksum" @@ -1888,3 +1891,104 @@ def test_unpublish_excluded_content_files_dry_run(staff_only_run, mock_deindex_t assert ContentFile.objects.filter(run=run, published=True).count() == 1 mock_deindex_tasks.opensearch.assert_not_called() mock_deindex_tasks.qdrant.assert_not_called() + + +@pytest.mark.parametrize( + "unpublished_rows", [False, True], ids=["deleted", "unpublished"] +) +def test_process_course_archive_reloads_when_receipt_is_stale(mocker, unpublished_rows): + """ + A matching archive_key and checksum must not skip a run whose rows are + gone or all unpublished + """ + key = "mitxonline/courses/course-v1:Test+Course+R1/archive.tar.gz" + run = LearningResourceRunFactory.create( + published=True, archive_key=key, checksum="abc123" + ) + if unpublished_rows: + ContentFileFactory.create_batch(2, run=run, published=False) + bucket = mocker.MagicMock() + mocker.patch( + "learning_resources.etl.edx_shared.calc_checksum", return_value="abc123" + ) + mocker.patch( + "learning_resources.etl.edx_shared.transform_content_files", + return_value=iter([{"key": "content.txt"}]), + ) + mock_load = mocker.patch( + "learning_resources.etl.edx_shared.load_content_files", return_value=[1] + ) + + assert process_course_archive(bucket, key, run) is True + + bucket.download_file.assert_called_once() + mock_load.assert_called_once() + + +@pytest.mark.parametrize( + ("checksum", "with_rows"), + [("abc123", True), (None, False)], + ids=["intact_rows", "empty_archive_receipt"], +) +def test_process_course_archive_skips_matching_archive_key(mocker, checksum, with_rows): + """A matching archive_key skips the download when rows exist or the archive was empty""" + key = "mitxonline/courses/course-v1:Test+Course+R1/archive.tar.gz" + run = LearningResourceRunFactory.create( + published=True, archive_key=key, checksum=checksum + ) + if with_rows: + ContentFileFactory.create(run=run) + bucket = mocker.MagicMock() + mock_load = mocker.patch("learning_resources.etl.edx_shared.load_content_files") + + assert process_course_archive(bucket, key, run) is False + + bucket.download_file.assert_not_called() + mock_load.assert_not_called() + + +def test_process_course_archive_skips_matching_checksum_with_rows(mocker): + """A matching checksum under a new key skips the load when rows exist""" + run = LearningResourceRunFactory.create( + published=True, archive_key="old/key.tar.gz", checksum="abc123" + ) + ContentFileFactory.create(run=run) + bucket = mocker.MagicMock() + mocker.patch( + "learning_resources.etl.edx_shared.calc_checksum", return_value="abc123" + ) + mock_load = mocker.patch("learning_resources.etl.edx_shared.load_content_files") + key = "mitxonline/courses/course-v1:Test+Course+R1/new.tar.gz" + + assert process_course_archive(bucket, key, run) is True + + bucket.download_file.assert_called_once() + mock_load.assert_not_called() + run.refresh_from_db() + assert run.archive_key == key + + +def test_process_course_archive_clears_stale_checksum_on_empty_archive(mocker): + """A stale run on a now-empty archive has its checksum cleared""" + key = "mitxonline/courses/course-v1:Test+Course+R1/archive.tar.gz" + run = LearningResourceRunFactory.create( + published=True, archive_key=key, checksum="abc123" + ) + bucket = mocker.MagicMock() + mocker.patch( + "learning_resources.etl.edx_shared.calc_checksum", return_value="abc123" + ) + mocker.patch( + "learning_resources.etl.edx_shared.transform_content_files", + return_value=iter([]), + ) + mock_load = mocker.patch("learning_resources.etl.edx_shared.load_content_files") + + assert process_course_archive(bucket, key, run) is True + run.refresh_from_db() + assert run.checksum is None + assert run.archive_key == key + + assert process_course_archive(bucket, key, run) is False + bucket.download_file.assert_called_once() + mock_load.assert_not_called() diff --git a/learning_resources/etl/loaders.py b/learning_resources/etl/loaders.py index c33b3fd4ac..d6fc28547f 100644 --- a/learning_resources/etl/loaders.py +++ b/learning_resources/etl/loaders.py @@ -444,7 +444,7 @@ def enqueue_content_tasks(): return learning_resource_run -def upsert_course_or_program( # noqa: C901 +def upsert_course_or_program( # noqa: C901, PLR0912 resource_data: dict, blocklist: list[str], resource_type: str, @@ -477,6 +477,9 @@ def upsert_course_or_program( # noqa: C901 if readable_id in blocklist or not runs: resource_data["published"] = False + if readable_id in blocklist: + # blocklisting overrides test_mode, which would keep the content indexed + resource_data["test_mode"] = False if not resource_data.get("resource_category"): if resource_type == LearningResourceType.course.name: @@ -600,7 +603,10 @@ def load_course( we set the course to "test_mode" in learn """ learning_resource.require_summaries = True - if learning_resource.published is False: + if ( + learning_resource.published is False + and learning_resource.readable_id not in blocklist + ): learning_resource.test_mode = True learning_resource.save() for course_run_data in runs_data: diff --git a/learning_resources/etl/loaders_test.py b/learning_resources/etl/loaders_test.py index b41502ab69..b5e85700ac 100644 --- a/learning_resources/etl/loaders_test.py +++ b/learning_resources/etl/loaders_test.py @@ -3693,6 +3693,25 @@ def test_course_with_unpublished_force_ingest_is_test_mode(): assert course.published is False +@pytest.mark.parametrize("force_ingest", [True, False]) +def test_load_course_blocklist_clears_test_mode(force_ingest): + """A blocklisted course leaves test_mode, even when force ingested""" + course = LearningResourceFactory.create( + is_course=True, published=False, test_mode=True + ) + course_data = { + "readable_id": course.readable_id, + "platform": course.platform.code, + "title": "test", + "url": "http://test.com", + "force_ingest": force_ingest, + "runs": [{"run_id": "test-run"}], + } + course = load_course(course_data, [course.readable_id]) + assert course.test_mode is False + assert course.published is False + + @pytest.mark.django_db def test_load_documents(mocker, climate_platform, mock_get_similar_topics_qdrant): documents_data = [ diff --git a/learning_resources/utils.py b/learning_resources/utils.py index 1795f9783b..6f631c50fb 100644 --- a/learning_resources/utils.py +++ b/learning_resources/utils.py @@ -377,7 +377,8 @@ def resource_unpublished_actions(resource: LearningResource): Unpublish a resource's direct content files (e.g. marketing pages) and trigger plugins when a LearningResource is removed/unpublished """ - resource.resource_content_files.filter(published=True).update(published=False) + if not resource.test_mode: + resource.resource_content_files.filter(published=True).update(published=False) pm = get_plugin_manager() hook = pm.hook hook.resource_unpublished(resource=resource) @@ -410,7 +411,9 @@ def bulk_resources_unpublished_actions(resource_ids: list[int], resource_type: s trigger plugins when LearningResources are removed/unpublished """ ContentFile.objects.filter( - learning_resource_id__in=resource_ids, published=True + learning_resource_id__in=resource_ids, + learning_resource__test_mode=False, + published=True, ).update(published=False) pm = get_plugin_manager() hook = pm.hook diff --git a/learning_resources/utils_test.py b/learning_resources/utils_test.py index 394212b9ed..5f57f1e3c8 100644 --- a/learning_resources/utils_test.py +++ b/learning_resources/utils_test.py @@ -346,6 +346,49 @@ def test_bulk_resources_unpublished_actions(mock_plugin_manager, fixture_resourc ) +def test_resource_unpublished_actions_keeps_test_mode_direct_files( + mock_plugin_manager, +): + """A test_mode resource's direct content files stay published when it is unpublished""" + resource = LearningResourceFactory.create(published=False, test_mode=True) + marketing_page = ContentFileFactory.create( + learning_resource=resource, published=True + ) + + utils.resource_unpublished_actions(resource) + + marketing_page.refresh_from_db() + assert marketing_page.published is True + mock_plugin_manager.hook.resource_unpublished.assert_called_once_with( + resource=resource + ) + + +def test_bulk_resources_unpublished_actions_keeps_test_mode_direct_files( + mock_plugin_manager, +): + """Only the non-test_mode resources' direct content files are unpublished in bulk""" + resource = LearningResourceFactory.create(is_course=True, published=False) + test_resource = LearningResourceFactory.create( + is_course=True, published=False, test_mode=True + ) + marketing_page = ContentFileFactory.create( + learning_resource=resource, published=True + ) + test_marketing_page = ContentFileFactory.create( + learning_resource=test_resource, published=True + ) + + utils.bulk_resources_unpublished_actions( + [resource.id, test_resource.id], resource.resource_type + ) + + marketing_page.refresh_from_db() + test_marketing_page.refresh_from_db() + assert marketing_page.published is False + assert test_marketing_page.published is True + + def test_resource_delete_actions(mock_plugin_manager, fixture_resource): """ resource_delete_actions function should trigger plugin hook's resource_deleted function diff --git a/learning_resources_search/indexing_api.py b/learning_resources_search/indexing_api.py index 042b3ee982..1717a7337d 100644 --- a/learning_resources_search/indexing_api.py +++ b/learning_resources_search/indexing_api.py @@ -11,7 +11,12 @@ from opensearchpy.exceptions import ConflictError, NotFoundError from opensearchpy.helpers import BulkIndexError, bulk -from learning_resources.models import ContentFile, LearningResourceRun +from learning_resources.etl.constants import QDRANT_RETAINED_SOURCES +from learning_resources.models import ( + ContentFile, + LearningResource, + LearningResourceRun, +) from learning_resources_search.connection import ( get_active_aliases, get_conn, @@ -44,6 +49,7 @@ serialize_content_file_for_bulk, serialize_content_file_for_bulk_deletion, ) +from learning_resources_search.utils import opensearch_runs from main.utils import chunks from vector_search.utils import dense_encoder, retrieve_points_matching_params @@ -393,10 +399,18 @@ def deindex_learning_resources(ids, base_index_name): ) if base_index_name in (COURSE_TYPE, PROGRAM_TYPE): - for run_id in LearningResourceRun.objects.filter( - learning_resource_id__in=ids - ).values_list("id", flat=True): - deindex_run_content_files(run_id, unpublished_only=False) + # test_mode resources keep their content files indexed; retained sources + # keep the rows published so they stay in Qdrant + runs = LearningResourceRun.objects.filter( + learning_resource_id__in=ids, learning_resource__test_mode=False + ).select_related("learning_resource") + for run in runs: + deindex_run_content_files( + run.id, + unpublished_only=False, + keep_published=run.learning_resource.etl_source + in QDRANT_RETAINED_SOURCES, + ) def deindex_percolators(ids): @@ -547,6 +561,43 @@ def deindex_run_content_files(run_id, unpublished_only, *, keep_published=False) ) +def deindex_non_opensearch_run_content_files( + learning_resource_id, resource_type=COURSE_TYPE +): + """ + Delete a resource's run content file documents whose run is no longer + selected for OpenSearch, e.g. an old best run. Asks OpenSearch for what it + holds, so a best run that changed with the date alone is caught too. + + Args: + learning_resource_id(int): Learning resource id of the content files + resource_type (string): The resource type of the parent learning resource + """ + resource = LearningResource.objects.get(id=learning_resource_id) + keep_run_ids = list(opensearch_runs(resource).values_list("id", flat=True)) + if not resource.runs.exclude(id__in=keep_run_ids).exists(): + return + query = { + "query": { + "bool": { + "filter": [ + {"term": {"resource_id": learning_resource_id}}, + {"exists": {"field": "run_id"}}, + ], + "must_not": [{"terms": {"run_id": keep_run_ids}}], + } + } + } + conn = get_conn() + for alias in get_active_aliases(conn, object_types=[resource_type]): + conn.delete_by_query( + index=alias, + body=query, + routing=learning_resource_id, + conflicts="proceed", + ) + + def deindex_document(doc_id, object_type, **kwargs): """ Make a request to ES to delete a document diff --git a/learning_resources_search/indexing_api_test.py b/learning_resources_search/indexing_api_test.py index 6ee7b69239..61cc33eb14 100644 --- a/learning_resources_search/indexing_api_test.py +++ b/learning_resources_search/indexing_api_test.py @@ -10,9 +10,11 @@ from anys import ANY_DICT, ANY_STR from opensearchpy.exceptions import ConflictError, NotFoundError +from learning_resources.etl.constants import ETLSource from learning_resources.factories import ( ContentFileFactory, CourseFactory, + LearningResourceFactory, LearningResourceRunFactory, ProgramFactory, ) @@ -36,6 +38,7 @@ deindex_content_files, deindex_document, deindex_learning_resources, + deindex_non_opensearch_run_content_files, deindex_percolators, deindex_run_content_files, delete_orphaned_indexes, @@ -511,25 +514,51 @@ def test_delete_orphaned_indexes(mocker, mocked_es, delete_reindexing_tags): assert mocked_es.conn.indices.delete.call_count == 1 -def test_bulk_content_file_deindex_on_course_deletion(mocker): - """ - OpenSearch should deindex content files on bulk course deletion - """ +def test_deindex_learning_resources_skips_test_mode_content_files(mocker): + """Bulk deindex leaves a test_mode course's content files alone""" mock_deindex_run_content_files = mocker.patch( "learning_resources_search.indexing_api.deindex_run_content_files", autospec=True, ) mocker.patch("learning_resources_search.indexing_api.deindex_items", autospec=True) + course = LearningResourceFactory.create( + is_course=True, create_runs=True, test_mode=True, published=False + ) + content_file = ContentFileFactory.create(run=course.runs.first(), published=True) - courses = CourseFactory.create_batch(2) - deindex_learning_resources( - [course.learning_resource_id for course in courses], COURSE_TYPE + deindex_learning_resources([course.id], COURSE_TYPE) + + mock_deindex_run_content_files.assert_not_called() + content_file.refresh_from_db() + assert content_file.published is True + + +@pytest.mark.parametrize( + ("etl_source", "stays_published"), + [(ETLSource.mitxonline.value, True), (ETLSource.ocw.value, False)], +) +def test_deindex_learning_resources_content_files_by_source( + mocker, etl_source, stays_published +): + """ + Bulk deindex removes content file docs from OpenSearch for every source but + only flips published for sources that are not retained in Qdrant. + """ + mock_deindex_items = mocker.patch( + "learning_resources_search.indexing_api.deindex_items", autospec=True ) - for course in courses: - for run in course.learning_resource.runs.all(): - mock_deindex_run_content_files.assert_any_call( - run.id, unpublished_only=False - ) + course = LearningResourceFactory.create( + is_course=True, create_runs=True, etl_source=etl_source + ) + run = course.runs.first() + content_files = ContentFileFactory.create_batch(2, run=run, published=True) + + deindex_learning_resources([course.id], COURSE_TYPE) + + for content_file in content_files: + content_file.refresh_from_db() + assert content_file.published is stays_published + assert mock_deindex_items.call_count == 2 def test_deindex_run_content_files(mocker): @@ -697,7 +726,7 @@ def test_bulk_content_file_deindex_on_program_deletion(mocker): for program in programs: for run in program.learning_resource.runs.all(): mock_deindex_run_content_files.assert_any_call( - run.id, unpublished_only=False + run.id, unpublished_only=False, keep_published=False ) @@ -1028,3 +1057,54 @@ def test_clear_featured_rank(mocked_es, mocker, clear_all_greater_than): "query": query, }, ) + + +def test_deindex_non_opensearch_run_content_files_single_run(mocked_es): + """No query is sent when the resource has no run outside the selected ones""" + course = LearningResourceFactory.create( + is_course=True, create_runs=False, published=True + ) + LearningResourceRunFactory.create(learning_resource=course, published=True) + + deindex_non_opensearch_run_content_files(course.id) + + mocked_es.conn.delete_by_query.assert_not_called() + + +def test_deindex_non_opensearch_run_content_files(mocker, mocked_es): + """Docs from every run but the OpenSearch-selected one are deleted by query""" + course = LearningResourceFactory.create( + is_course=True, create_runs=False, published=True + ) + best = LearningResourceRunFactory.create(learning_resource=course, published=True) + LearningResourceRunFactory.create( + learning_resource=course, + published=True, + start_date=best.start_date.replace(year=2000), + ) + assert course.best_run == best + mocker.patch( + "learning_resources_search.indexing_api.get_active_aliases", + autospec=True, + return_value=mocked_es.active_aliases, + ) + + deindex_non_opensearch_run_content_files(course.id) + + for alias in mocked_es.active_aliases: + mocked_es.conn.delete_by_query.assert_any_call( + index=alias, + body={ + "query": { + "bool": { + "filter": [ + {"term": {"resource_id": course.id}}, + {"exists": {"field": "run_id"}}, + ], + "must_not": [{"terms": {"run_id": [best.id]}}], + } + } + }, + routing=course.id, + conflicts="proceed", + ) diff --git a/learning_resources_search/plugins.py b/learning_resources_search/plugins.py index 0f3f5abfa6..13f3733e98 100644 --- a/learning_resources_search/plugins.py +++ b/learning_resources_search/plugins.py @@ -7,13 +7,14 @@ from django.conf import settings as django_settings from learning_resources.etl.constants import QDRANT_RETAINED_SOURCES -from learning_resources.models import ContentFile +from learning_resources.models import ContentFile, LearningResource from learning_resources_search import tasks from learning_resources_search.api import get_similar_topics_qdrant from learning_resources_search.constants import ( COURSE_TYPE, PERCOLATE_INDEX_TYPE, ) +from learning_resources_search.utils import opensearch_runs from main import settings from main.utils import chunks from vector_search import tasks as vector_tasks @@ -189,7 +190,14 @@ def bulk_resources_unpublished(self, resource_ids, resource_type): ) try_with_retry_as_task(chain(*unpublished_tasks)) - self._deindex_learning_resource_content_files(resource_ids, resource_type) + # test_mode resources keep their content files indexed, as in + # resource_unpublished + self._deindex_learning_resource_content_files( + LearningResource.objects.filter( + id__in=resource_ids, test_mode=False + ).values_list("id", flat=True), + resource_type, + ) @hookimpl def resource_before_delete(self, resource): @@ -223,23 +231,17 @@ def resource_run_unpublished(self, run): """ resource = run.learning_resource - if not run.content_files.exists(): + if not run.content_files.exists() or resource.test_mode: return - if resource.test_mode: - return - if resource.etl_source in QDRANT_RETAINED_SOURCES: - deindex_tasks = [ - tasks.deindex_run_content_files.si( - run.id, unpublished_only=False, keep_published=True - ), - ] - else: - deindex_tasks = [ - tasks.deindex_run_content_files.si(run.id, unpublished_only=False), - ] - if django_settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS: - deindex_tasks.append(vector_tasks.remove_run_content_files.si(run.id)) + keep_published = resource.etl_source in QDRANT_RETAINED_SOURCES + deindex_tasks = [ + tasks.deindex_run_content_files.si( + run.id, unpublished_only=False, keep_published=keep_published + ), + ] + if not keep_published and django_settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS: + deindex_tasks.append(vector_tasks.remove_run_content_files.si(run.id)) try_with_retry_as_task(chain(*deindex_tasks)) @hookimpl @@ -276,11 +278,7 @@ def content_files_loaded(self, run): resource = run.learning_resource if resource.published or resource.test_mode: - if ( - run.published - and not run.is_variant - and (resource.test_mode or resource.best_run == run) - ): + if opensearch_runs(resource).filter(id=run.id).exists(): index_tasks.append(tasks.index_run_content_files.si(run.id)) if django_settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS: diff --git a/learning_resources_search/plugins_test.py b/learning_resources_search/plugins_test.py index f18ed8359a..595278454e 100644 --- a/learning_resources_search/plugins_test.py +++ b/learning_resources_search/plugins_test.py @@ -116,7 +116,9 @@ def test_search_index_plugin_resource_unpublished( assert unpublish_run_mock.call_count == resource.runs.count() for run in resource.runs.all(): # Default "mock" source is non-retained -> removed from both indexes. - unpublish_run_mock.assert_any_call(run.id, unpublished_only=False) + unpublish_run_mock.assert_any_call( + run.id, unpublished_only=False, keep_published=False + ) else: unpublish_run_mock.assert_not_called() if test_mode: @@ -158,6 +160,51 @@ def test_search_index_plugin_bulk_resources_unpublished_direct_files( ) +@pytest.mark.django_db +def test_search_index_plugin_bulk_resources_unpublished_skips_test_mode_direct_files( + mocker, +): + """bulk_resources_unpublished leaves a test_mode resource's direct files indexed, like resource_unpublished""" + resource = LearningResourceFactory.create(is_course=True, published=False) + test_resource = LearningResourceFactory.create( + is_course=True, published=False, test_mode=True + ) + marketing_page = ContentFileFactory.create(learning_resource=resource) + ContentFileFactory.create(learning_resource=test_resource) + mocker.patch( + "learning_resources_search.plugins.tasks.bulk_deindex_learning_resources.si" + ) + deindex_direct_files_mock = mocker.patch( + "learning_resources_search.plugins.tasks.deindex_content_files.si" + ) + + SearchIndexPlugin().bulk_resources_unpublished( + [resource.id, test_resource.id], COURSE_TYPE + ) + + deindex_direct_files_mock.assert_called_once_with( + [marketing_page.id], resource.id, resource_type=COURSE_TYPE + ) + + +@pytest.mark.django_db +def test_search_index_plugin_resource_before_delete_test_mode_direct_files(mocker): + """Deleting a persisted test_mode resource still deindexes its direct content files""" + resource = LearningResourceFactory.create(is_course=True, test_mode=True) + marketing_page = ContentFileFactory.create(learning_resource=resource) + mocker.patch("learning_resources_search.plugins.tasks.deindex_document.si") + mocker.patch("learning_resources_search.plugins.tasks.deindex_run_content_files.si") + deindex_direct_files_mock = mocker.patch( + "learning_resources_search.plugins.tasks.deindex_content_files.si" + ) + + SearchIndexPlugin().resource_before_delete(resource) + + deindex_direct_files_mock.assert_called_once_with( + [marketing_page.id], resource.id, resource_type=COURSE_TYPE + ) + + @pytest.mark.django_db @pytest.mark.parametrize("resource_type", [COURSE_TYPE, PROGRAM_TYPE]) @pytest.mark.parametrize("test_mode", [True, False]) @@ -188,7 +235,7 @@ def test_search_index_plugin_resource_before_delete( ) for run in resource.runs.all(): mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_any_call( - run.id, unpublished_only=False + run.id, unpublished_only=False, keep_published=False ) else: mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_not_called() @@ -267,7 +314,7 @@ def test_resource_run_unpublished_non_retained_source_removes_both( SearchIndexPlugin().resource_run_unpublished(run) mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_called_once_with( - run.id, unpublished_only=False + run.id, unpublished_only=False, keep_published=False ) mock_search_index_helpers.mock_remove_run_contentfiles_immutable_signature.assert_called_once_with( run.id @@ -423,6 +470,29 @@ def test_content_files_loaded_unpublished_run_embeds_qdrant_only( mock_search_index_helpers.mock_remove_run_contentfiles_immutable_signature.assert_not_called() +@pytest.mark.django_db +def test_content_files_loaded_test_mode_canvas_purges_qdrant_only( + mock_search_index_helpers, settings +): + """A test_mode Canvas run drops its unpublished files from Qdrant and stays out of OpenSearch""" + settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True + run = LearningResourceRunFactory.create( + published=True, + learning_resource__etl_source=ETLSource.canvas.name, + learning_resource__published=False, + learning_resource__test_mode=True, + learning_resource__create_runs=False, + ) + ContentFileFactory.create(run=run, published=False) + + SearchIndexPlugin().content_files_loaded(run) + + mock_search_index_helpers.mock_remove_unpublished_run_contentfiles_immutable_signature.assert_called_once_with( + run.id + ) + mock_search_index_helpers.mock_upsert_contentfiles_immutable_signature.assert_not_called() + + @pytest.mark.django_db def test_content_files_loaded_non_best_published_run_skips_opensearch( mock_search_index_helpers, settings @@ -594,3 +664,25 @@ def test_content_files_loaded_always_purges_unpublished( purge = mock_search_index_helpers.mock_remove_unpublished_run_contentfiles_immutable_signature.return_value embed = mock_search_index_helpers.mock_embed_run_contentfiles_immutable_signature.return_value assert chained.index(purge) < chained.index(embed) + + +@pytest.mark.django_db +def test_content_files_loaded_best_run_with_only_unpublished_files_still_indexes( + mocker, settings +): + """ + The best run is re-indexed even when a reload left all its files unpublished, + so index_run_content_files clears their stale OpenSearch documents. + """ + settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = False + mocker.patch("learning_resources_search.plugins.try_with_retry_as_task") + index_mock = mocker.patch( + "learning_resources_search.plugins.tasks.index_run_content_files.si" + ) + course = LearningResourceFactory.create(is_course=True, create_runs=False) + run = LearningResourceRunFactory.create(learning_resource=course, published=True) + ContentFileFactory.create(run=run, published=False) + + SearchIndexPlugin().content_files_loaded(run) + + index_mock.assert_called_once_with(run.id) diff --git a/learning_resources_search/tasks.py b/learning_resources_search/tasks.py index 5fa43d5d73..3bf0ab87e4 100644 --- a/learning_resources_search/tasks.py +++ b/learning_resources_search/tasks.py @@ -56,6 +56,7 @@ serialize_learning_resource_for_update, serialize_percolate_query_for_update, ) +from learning_resources_search.utils import opensearch_content_files from main.celery import app from main.models import TaskBatch, TaskJob from main.tasks import maybe_finish_task_job @@ -595,6 +596,34 @@ def deindex_run_content_files(run_id, unpublished_only, keep_published=False): return error +@app.task( + autoretry_for=(RetryError,), + retry_backoff=True, + rate_limit=settings.CELERY_SEARCH_RATE_LIMIT, +) +def deindex_non_opensearch_run_content_files( + learning_resource_id, resource_type=COURSE_TYPE +): + """ + Deindex a resource's content files from runs no longer selected for OpenSearch + + Args: + learning_resource_id(int): Learning resource id of the content files + resource_type (string): The resource type of the parent learning resource + """ + try: + with wrap_retry_exception(*SEARCH_CONN_EXCEPTIONS): + api.deindex_non_opensearch_run_content_files( + learning_resource_id, resource_type=resource_type + ) + except (RetryError, Ignore): + raise + except: # noqa: E722 + error = "deindex_non_opensearch_run_content_files threw an error" + log.exception(error) + return error + + @contextmanager def wrap_retry_exception(*exception_classes): """ @@ -665,7 +694,10 @@ def add_batch(kind, batch_key, params): for chunk, resource_ids in enumerate( chunks( - Course.objects.filter(learning_resource__published=True) + Course.objects.filter( + Q(learning_resource__published=True) + | Q(learning_resource__test_mode=True) + ) .filter(learning_resource__etl_source__in=RESOURCE_FILE_ETL_SOURCES) .exclude(learning_resource__readable_id__in=blocklisted_ids) .order_by("learning_resource_id") @@ -720,9 +752,8 @@ def add_batch(kind, batch_key, params): if PROGRAM_TYPE in indexes: for chunk, resource_ids in enumerate( chunks( - LearningResource.objects.filter( - published=True, resource_type=PROGRAM_TYPE - ) + LearningResource.objects.filter(resource_type=PROGRAM_TYPE) + .filter(Q(published=True) | Q(test_mode=True)) .order_by("id") .values_list("id", flat=True), chunk_size=settings.OPENSEARCH_REINDEX_DISPATCH_CHUNK_SIZE, @@ -753,54 +784,31 @@ def _dispatch_content_file_batches(batch): """ resource_type = batch.params["resource_type"] children = [] - for resource_id in batch.params["learning_resource_ids"]: - for chunk, ids in enumerate( - chunks( - ContentFile.objects.filter( - run__learning_resource_id=resource_id, - published=True, - run__published=True, - ) - .order_by("id") - .values_list("id", flat=True), - chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, - ) - ): - children.append( - TaskBatch( - job=batch.job, - kind=ReindexBatchKind.content_files.value, - batch_key=f"content_files:{resource_id}:run:{chunk}", - params={ - "ids": ids, - "learning_resource_id": resource_id, - "resource_type": resource_type, - }, - ) - ) - for chunk, ids in enumerate( - chunks( - ContentFile.objects.filter( - learning_resource_id=resource_id, - published=True, + for resource in LearningResource.objects.filter( + id__in=batch.params["learning_resource_ids"] + ).order_by("id"): + indexable = opensearch_content_files(resource) + for label, direct in (("run", False), ("direct", True)): + for chunk, ids in enumerate( + chunks( + indexable.filter(run__isnull=direct) + .order_by("id") + .values_list("id", flat=True), + chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, ) - .order_by("id") - .values_list("id", flat=True), - chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, - ) - ): - children.append( - TaskBatch( - job=batch.job, - kind=ReindexBatchKind.content_files.value, - batch_key=f"content_files:{resource_id}:direct:{chunk}", - params={ - "ids": ids, - "learning_resource_id": resource_id, - "resource_type": resource_type, - }, + ): + children.append( + TaskBatch( + job=batch.job, + kind=ReindexBatchKind.content_files.value, + batch_key=f"content_files:{resource.id}:{label}:{chunk}", + params={ + "ids": ids, + "learning_resource_id": resource.id, + "resource_type": resource_type, + }, + ) ) - ) TaskBatch.objects.bulk_create(children, ignore_conflicts=True) child_ids = batch.job.batches.filter( batch_key__in=[child.batch_key for child in children], @@ -1084,6 +1092,48 @@ def start_update_index(self, indexes, etl_source): return self.replace(celery.chain(index_tasks, finish_update_index.s())) +def _update_content_files_tasks(learning_resource, resource_type): + """ + Get tasks that index a resource's OpenSearch content files and deindex + everything else: files of runs no longer selected, and unpublished files. + """ + unpublished = ContentFile.objects.filter( + Q(run__learning_resource_id=learning_resource.id) + | Q(learning_resource_id=learning_resource.id), + published=False, + ) + return ( + [ + index_content_files.si( + ids, + learning_resource.id, + index_types=IndexestoUpdate.current_index.value, + resource_type=resource_type, + ) + for ids in chunks( + opensearch_content_files(learning_resource) + .order_by("id") + .values_list("id", flat=True), + chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, + ) + ] + + [ + deindex_non_opensearch_run_content_files.si( + learning_resource.id, resource_type=resource_type + ) + ] + + [ + deindex_content_files.si( + ids, learning_resource.id, resource_type=resource_type + ) + for ids in chunks( + unpublished.order_by("id").values_list("id", flat=True), + chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, + ) + ] + ) + + def get_update_resource_files_tasks(blocklisted_ids, etl_source): """ Get list of tasks to update course files. @@ -1097,7 +1147,8 @@ def get_update_resource_files_tasks(blocklisted_ids, etl_source): if etl_source is None or etl_source in RESOURCE_FILE_ETL_SOURCES: course_update_query = ( - LearningResource.objects.filter(published=True, resource_type=COURSE_TYPE) + LearningResource.objects.filter(resource_type=COURSE_TYPE) + .filter(Q(published=True) | Q(test_mode=True)) .exclude(readable_id__in=blocklisted_ids) .order_by("id") ) @@ -1109,75 +1160,11 @@ def get_update_resource_files_tasks(blocklisted_ids, etl_source): etl_source__in=RESOURCE_FILE_ETL_SOURCES ) - index_tasks = [] - - for learning_resource in course_update_query.order_by("id"): - index_tasks = ( - index_tasks - + [ - index_content_files.si( - ids, - learning_resource.id, - index_types=IndexestoUpdate.current_index.value, - ) - for ids in chunks( - ContentFile.objects.filter( - run__learning_resource_id=learning_resource.id, - published=True, - run__published=True, - ) - .order_by("id") - .values_list("id", flat=True), - chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, - ) - ] - + [ - index_content_files.si( - ids, - learning_resource.id, - index_types=IndexestoUpdate.current_index.value, - ) - for ids in chunks( - ContentFile.objects.filter( - learning_resource_id=learning_resource.id, - published=True, - ) - .order_by("id") - .values_list("id", flat=True), - chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, - ) - ] - ) - - index_tasks = ( - index_tasks - + [ - deindex_content_files.si(ids, learning_resource.id) - for ids in chunks( - ContentFile.objects.filter( - run__learning_resource_id=learning_resource.id - ) - .filter(Q(published=False) | Q(run__published=False)) - .order_by("id") - .values_list("id", flat=True), - chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, - ) - ] - + [ - deindex_content_files.si(ids, learning_resource.id) - for ids in chunks( - ContentFile.objects.filter( - learning_resource_id=learning_resource.id, - published=False, - ) - .order_by("id") - .values_list("id", flat=True), - chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, - ) - ] - ) - - return index_tasks + return [ + task + for learning_resource in course_update_query + for task in _update_content_files_tasks(learning_resource, COURSE_TYPE) + ] else: return [] @@ -1192,9 +1179,11 @@ def get_update_program_files_tasks(etl_source): if etl_source is not None and etl_source not in RESOURCE_FILE_ETL_SOURCES: return [] - program_update_query = LearningResource.objects.filter( - published=True, resource_type=PROGRAM_TYPE - ).order_by("id") + program_update_query = ( + LearningResource.objects.filter(resource_type=PROGRAM_TYPE) + .filter(Q(published=True) | Q(test_mode=True)) + .order_by("id") + ) if etl_source: program_update_query = program_update_query.filter(etl_source=etl_source) @@ -1203,81 +1192,11 @@ def get_update_program_files_tasks(etl_source): etl_source__in=RESOURCE_FILE_ETL_SOURCES ) - index_tasks = [] - - for learning_resource in program_update_query: - index_tasks = ( - index_tasks - + [ - index_content_files.si( - ids, - learning_resource.id, - index_types=IndexestoUpdate.current_index.value, - resource_type=PROGRAM_TYPE, - ) - for ids in chunks( - ContentFile.objects.filter( - run__learning_resource_id=learning_resource.id, - published=True, - run__published=True, - ) - .order_by("id") - .values_list("id", flat=True), - chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, - ) - ] - + [ - index_content_files.si( - ids, - learning_resource.id, - index_types=IndexestoUpdate.current_index.value, - resource_type=PROGRAM_TYPE, - ) - for ids in chunks( - ContentFile.objects.filter( - learning_resource_id=learning_resource.id, - published=True, - ) - .order_by("id") - .values_list("id", flat=True), - chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, - ) - ] - ) - - index_tasks = ( - index_tasks - + [ - deindex_content_files.si( - ids, learning_resource.id, resource_type=PROGRAM_TYPE - ) - for ids in chunks( - ContentFile.objects.filter( - run__learning_resource_id=learning_resource.id - ) - .filter(Q(published=False) | Q(run__published=False)) - .order_by("id") - .values_list("id", flat=True), - chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, - ) - ] - + [ - deindex_content_files.si( - ids, learning_resource.id, resource_type=PROGRAM_TYPE - ) - for ids in chunks( - ContentFile.objects.filter( - learning_resource_id=learning_resource.id, - published=False, - ) - .order_by("id") - .values_list("id", flat=True), - chunk_size=settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE, - ) - ] - ) - - return index_tasks + return [ + task + for learning_resource in program_update_query + for task in _update_content_files_tasks(learning_resource, PROGRAM_TYPE) + ] def get_update_courses_tasks(blocklisted_ids, etl_source): diff --git a/learning_resources_search/tasks_test.py b/learning_resources_search/tasks_test.py index d38853451f..bde182d9e3 100644 --- a/learning_resources_search/tasks_test.py +++ b/learning_resources_search/tasks_test.py @@ -18,6 +18,7 @@ LearningResourceDepartmentFactory, LearningResourceFactory, LearningResourceOfferorFactory, + LearningResourceRunFactory, LearningResourceTopicFactory, ProgramFactory, ) @@ -54,6 +55,8 @@ deindex_document, deindex_run_content_files, finish_reindex_job, + get_update_program_files_tasks, + get_update_resource_files_tasks, index_learning_resources, index_run_content_files, run_reindex_batch, @@ -65,6 +68,7 @@ upsert_learning_resource, wrap_retry_exception, ) +from learning_resources_search.utils import opensearch_content_files from main.factories import TaskBatchFactory, TaskJobFactory, UserFactory from main.models import TaskBatch, TaskJob from main.test_utils import assert_not_raises @@ -788,7 +792,7 @@ def test_run_reindex_batch_dispatch_content_files(mocker, mocked_api): """ settings.OPENSEARCH_DOCUMENT_INDEXING_CHUNK_SIZE = 2 course = CourseFactory.create(etl_source=ETLSource.ocw.value) - run = course.learning_resource.runs.first() + run = course.learning_resource.best_run run_files = sorted( ContentFileFactory.create_batch(3, run=run), key=lambda file: file.id ) @@ -1097,9 +1101,7 @@ def test_start_update_index(mocker, mocked_celery, indexes, etl_source, settings ) for course in courses: - ContentFileFactory.create_batch( - 3, run=course.learning_resource.runs.first() - ) + ContentFileFactory.create_batch(3, run=course.learning_resource.best_run) # A resource-level (marketing page) content file attached directly to # the learning resource rather than a run. @@ -1136,7 +1138,7 @@ def test_start_update_index(mocker, mocked_celery, indexes, etl_source, settings program_with_files.learning_resource.etl_source = ETLSource.mitxonline.value program_with_files.learning_resource.save() program_run_file = ContentFileFactory.create( - run=program_with_files.learning_resource.runs.first() + run=program_with_files.learning_resource.best_run ) program_marketing_file = ContentFileFactory.create( learning_resource=program_with_files.learning_resource @@ -1236,14 +1238,8 @@ def test_start_update_index(mocker, mocked_celery, indexes, etl_source, settings # Program content files are indexed with resource_type=PROGRAM_TYPE, for # both run-level and resource-level (marketing page) content files. - index_content_mock.si.assert_any_call( - [program_run_file.id], - program_with_files.learning_resource_id, - index_types=IndexestoUpdate.current_index.value, - resource_type=PROGRAM_TYPE, - ) - index_content_mock.si.assert_any_call( - [program_marketing_file.id], + index_content_mock.si.assert_called_once_with( + [program_run_file.id, program_marketing_file.id], program_with_files.learning_resource_id, index_types=IndexestoUpdate.current_index.value, resource_type=PROGRAM_TYPE, @@ -1251,39 +1247,27 @@ def test_start_update_index(mocker, mocked_celery, indexes, etl_source, settings if CONTENT_FILE_TYPE in indexes: if etl_source in RESOURCE_FILE_ETL_SOURCES: - # 2 run-level chunks + 1 resource-level (marketing page) chunk - assert index_content_mock.si.call_count == 3 + # 3 run-level files + 1 resource-level (marketing page) file, in + # chunks of 2 + assert index_content_mock.si.call_count == 2 course = next( course for course in courses if course.learning_resource.etl_source == etl_source ) - - content_file_ids = ( - course.learning_resource.runs.first() - .content_files.order_by("id") - .values_list("id", flat=True) - ) - - index_content_mock.si.assert_any_call( - [content_file_ids[0], content_file_ids[1]], - course.learning_resource_id, - index_types=IndexestoUpdate.current_index.value, - ) - - index_content_mock.si.assert_any_call( - [content_file_ids[2]], - course.learning_resource_id, - index_types=IndexestoUpdate.current_index.value, - ) - - # resource-level (marketing page) content file attached directly to - # the learning resource - index_content_mock.si.assert_any_call( - [xpro_marketing_file.id], - course.learning_resource_id, - index_types=IndexestoUpdate.current_index.value, - ) + expected_ids = [ + *course.learning_resource.best_run.content_files.order_by( + "id" + ).values_list("id", flat=True), + xpro_marketing_file.id, + ] + for ids in (expected_ids[:2], expected_ids[2:]): + index_content_mock.si.assert_any_call( + ids, + course.learning_resource_id, + index_types=IndexestoUpdate.current_index.value, + resource_type=COURSE_TYPE, + ) elif etl_source: assert index_content_mock.si.call_count == 0 @@ -1968,3 +1952,178 @@ def test_cache_is_cleared_after_reindex(mocker): ) finish_reindex_job.delay(job.id) assert mocked_clear_views_cache.call_count == 1 + + +def _course_with_best_and_older_run(**kwargs): + """Create a published mitxonline course with a best run and an older published run, each with files""" + course = LearningResourceFactory.create( + is_course=True, + create_runs=False, + etl_source=ETLSource.mitxonline.value, + published=True, + **kwargs, + ) + best = LearningResourceRunFactory.create(learning_resource=course, published=True) + older = LearningResourceRunFactory.create( + learning_resource=course, + published=True, + start_date=best.start_date.replace(year=2000), + ) + ContentFileFactory.create_batch(2, run=best) + ContentFileFactory.create_batch(2, run=older) + ContentFileFactory.create(learning_resource=course) + assert course.best_run == best + return course, best, older + + +def test_get_update_resource_files_tasks_indexes_best_run_only(mocker): + """update_index indexes the best run's files and the direct files, not older runs'""" + course, _, older = _course_with_best_and_older_run() + index_content_mock = mocker.patch( + "learning_resources_search.tasks.index_content_files", autospec=True + ) + mocker.patch("learning_resources_search.tasks.deindex_content_files", autospec=True) + deindex_runs_mock = mocker.patch( + "learning_resources_search.tasks.deindex_non_opensearch_run_content_files", + autospec=True, + ) + + get_update_resource_files_tasks([], ETLSource.mitxonline.value) + + indexed = { + cf_id for call in index_content_mock.si.call_args_list for cf_id in call.args[0] + } + assert indexed == set(opensearch_content_files(course).values_list("id", flat=True)) + assert not indexed & set(older.content_files.values_list("id", flat=True)) + deindex_runs_mock.si.assert_called_once_with(course.id, resource_type=COURSE_TYPE) + + +def test_run_reindex_batch_dispatch_content_files_best_run_only(mocker, mocked_api): + """A full rebuild dispatches the best run's files and the direct files, not older runs'""" + course, _, older = _course_with_best_and_older_run() + mocker.patch.object(run_reindex_batch, "delay") + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, status=TaskJob.Status.RUNNING + ) + batch = TaskBatchFactory.create( + job=job, + kind=ReindexBatchKind.dispatch_content_files.value, + params={"learning_resource_ids": [course.id], "resource_type": COURSE_TYPE}, + ) + + run_reindex_batch(batch.id) + + dispatched = { + cf_id + for child in job.batches.filter(kind=ReindexBatchKind.content_files.value) + for cf_id in child.params["ids"] + } + assert dispatched == set( + opensearch_content_files(course).values_list("id", flat=True) + ) + assert not dispatched & set(older.content_files.values_list("id", flat=True)) + + +def test_get_update_program_files_tasks_indexes_best_run_only(mocker): + """update_index indexes a program's best run files and direct files, not older runs'""" + program = LearningResourceFactory.create( + is_program=True, + create_runs=False, + etl_source=ETLSource.mitxonline.value, + published=True, + ) + best = LearningResourceRunFactory.create(learning_resource=program, published=True) + older = LearningResourceRunFactory.create( + learning_resource=program, + published=True, + start_date=best.start_date.replace(year=2000), + ) + ContentFileFactory.create_batch(2, run=best) + ContentFileFactory.create_batch(2, run=older) + ContentFileFactory.create(learning_resource=program) + assert program.best_run == best + index_content_mock = mocker.patch( + "learning_resources_search.tasks.index_content_files", autospec=True + ) + mocker.patch("learning_resources_search.tasks.deindex_content_files", autospec=True) + deindex_runs_mock = mocker.patch( + "learning_resources_search.tasks.deindex_non_opensearch_run_content_files", + autospec=True, + ) + + get_update_program_files_tasks(ETLSource.mitxonline.value) + + indexed = { + cf_id for call in index_content_mock.si.call_args_list for cf_id in call.args[0] + } + assert indexed == set( + opensearch_content_files(program).values_list("id", flat=True) + ) + assert not indexed & set(older.content_files.values_list("id", flat=True)) + deindex_runs_mock.si.assert_called_once_with(program.id, resource_type=PROGRAM_TYPE) + + +def test_start_recreate_index_dispatches_test_mode_course_content_files( + mocker, mocked_api +): + """ + An unpublished test_mode course's content files are dispatched for indexing, + as the post-ingest hook indexes them, while its resource document is not. + """ + course = LearningResourceFactory.create( + is_course=True, + create_runs=True, + etl_source=ETLSource.mitxonline.value, + published=False, + test_mode=True, + ) + ContentFileFactory.create(run=course.runs.first()) + mocker.patch( + "learning_resources_search.tasks.load_course_blocklist", return_value=[] + ) + mocker.patch("learning_resources_search.tasks.run_reindex_batch", autospec=True) + mocked_api.get_existing_reindexing_indexes.return_value = [] + mocked_api.create_backing_index.return_value = "backing" + job = TaskJobFactory.create( + task_name=REINDEX_TASK_NAME, params={"indexes": [COURSE_TYPE]} + ) + + start_recreate_index.delay(job.id) + + dispatched_ids = { + resource_id + for batch in job.batches.filter( + kind=ReindexBatchKind.dispatch_content_files.value + ) + for resource_id in batch.params["learning_resource_ids"] + } + indexed_resource_ids = { + resource_id + for batch in job.batches.filter(kind=ReindexBatchKind.learning_resources.value) + for resource_id in batch.params["ids"] + } + assert course.id in dispatched_ids + assert course.id not in indexed_resource_ids + + +def test_get_update_resource_files_tasks_includes_test_mode_courses(mocker): + """update_index indexes an unpublished test_mode course's content files""" + course = LearningResourceFactory.create( + is_course=True, + create_runs=True, + etl_source=ETLSource.mitxonline.value, + published=False, + test_mode=True, + ) + content_file = ContentFileFactory.create(run=course.runs.first()) + index_content_mock = mocker.patch( + "learning_resources_search.tasks.index_content_files", autospec=True + ) + mocker.patch("learning_resources_search.tasks.deindex_content_files", autospec=True) + + get_update_resource_files_tasks([], ETLSource.mitxonline.value) + + indexed = { + cf_id for call in index_content_mock.si.call_args_list for cf_id in call.args[0] + } + assert content_file.id in indexed diff --git a/learning_resources_search/utils.py b/learning_resources_search/utils.py index b4a31053cd..3587ea37af 100644 --- a/learning_resources_search/utils.py +++ b/learning_resources_search/utils.py @@ -1,10 +1,13 @@ import logging import urllib +from django.db.models import Q from opensearch_dsl import Search from channels.models import Channel +from learning_resources.etl.constants import ETLSource from learning_resources.hooks import get_plugin_manager +from learning_resources.models import ContentFile from learning_resources_search.constants import LEARNING_RESOURCE from learning_resources_search.models import PercolateQuery @@ -152,3 +155,32 @@ def percolate_query_saved_actions(percolate_query): pm = get_plugin_manager() hook = pm.hook hook.percolate_query_upserted(percolate_query=percolate_query) + + +# OpenSearch carries a course's best published run only (any published +# non-variant run of a test_mode course, except Canvas, whose private course +# material is only searchable once published), while Qdrant carries every run +# (see vector_search.utils.qdrant_content_files). Both carry files attached +# directly to the resource. + + +def _opensearch_test_mode(resource): + """Whether test_mode alone puts `resource` in OpenSearch.""" + return resource.test_mode and resource.etl_source != ETLSource.canvas.name + + +def opensearch_runs(resource): + """Select the runs of `resource` whose content files belong in OpenSearch.""" + if _opensearch_test_mode(resource): + return resource.runs.filter(published=True, is_variant=False) + best_run = resource.published and resource.best_run + return resource.runs.filter(id=best_run.id) if best_run else resource.runs.none() + + +def opensearch_content_files(resource): + """Select the published content files of `resource` that belong in OpenSearch.""" + if not resource.published and not _opensearch_test_mode(resource): + return ContentFile.objects.none() + return ContentFile.objects.filter(published=True).filter( + Q(learning_resource_id=resource.id) | Q(run__in=opensearch_runs(resource)) + ) diff --git a/learning_resources_search/utils_test.py b/learning_resources_search/utils_test.py index 8a20d52452..c87f7d2a84 100644 --- a/learning_resources_search/utils_test.py +++ b/learning_resources_search/utils_test.py @@ -7,9 +7,18 @@ from django.urls import reverse from channels.factories import ChannelFactory +from learning_resources.etl.constants import ETLSource +from learning_resources.factories import ( + ContentFileFactory, + LearningResourceFactory, + LearningResourceRunFactory, +) from learning_resources_search.factories import PercolateQueryFactory from learning_resources_search.models import PercolateQuery -from learning_resources_search.utils import prune_channel_subscriptions +from learning_resources_search.utils import ( + opensearch_content_files, + prune_channel_subscriptions, +) from main.factories import UserFactory @@ -137,3 +146,70 @@ def test_prune_subscription_on_empty_channel_search_filter( == 2 ) assert user.percolate_queries.count() == 2 + + +def _course_with_runs(**kwargs): + """Create a course with a best run, an older published run, a variant and an unpublished run""" + course = LearningResourceFactory.create(is_course=True, create_runs=False, **kwargs) + best = LearningResourceRunFactory.create(learning_resource=course, published=True) + older = LearningResourceRunFactory.create( + learning_resource=course, + published=True, + start_date=best.start_date.replace(year=2000), + ) + variant = LearningResourceRunFactory.create( + learning_resource=course, published=True, is_variant=True + ) + unpublished = LearningResourceRunFactory.create( + learning_resource=course, published=False + ) + files = { + run: ContentFileFactory.create(run=run, published=True) + for run in (best, older, variant, unpublished) + } + files["direct"] = ContentFileFactory.create( + learning_resource=course, published=True + ) + files["withdrawn"] = ContentFileFactory.create(run=best, published=False) + assert course.best_run == best + return course, (best, older), files + + +@pytest.mark.django_db +def test_opensearch_content_files_best_run_and_direct_only(): + """OpenSearch gets the best run's published files plus the resource's direct files""" + course, (best, _), files = _course_with_runs() + + assert set(opensearch_content_files(course)) == {files[best], files["direct"]} + + +@pytest.mark.django_db +def test_opensearch_content_files_test_mode_any_published_non_variant_run(): + """A test_mode course indexes every published non-variant run""" + course, (best, older), files = _course_with_runs(published=False, test_mode=True) + + assert set(opensearch_content_files(course)) == { + files[best], + files[older], + files["direct"], + } + + +@pytest.mark.django_db +@pytest.mark.parametrize("published", [True, False]) +def test_opensearch_content_files_canvas_needs_published(published): + """test_mode alone keeps a Canvas course out of OpenSearch""" + course, (best, _), files = _course_with_runs( + etl_source=ETLSource.canvas.name, published=published, test_mode=True + ) + + expected = {files[best], files["direct"]} if published else set() + assert set(opensearch_content_files(course)) == expected + + +@pytest.mark.django_db +def test_opensearch_content_files_unpublished_course_has_none(): + """An unpublished, non-test_mode course indexes nothing""" + course, _, _ = _course_with_runs(published=False) + + assert not opensearch_content_files(course).exists() diff --git a/vector_search/tasks.py b/vector_search/tasks.py index c465c6283a..e90adf5ecd 100644 --- a/vector_search/tasks.py +++ b/vector_search/tasks.py @@ -48,6 +48,7 @@ embed_learning_resources, embed_topics, filter_existing_qdrant_points_by_ids, + qdrant_content_files, remove_qdrant_records, vector_point_id, vector_point_key, @@ -131,10 +132,7 @@ def _queue_program_content_file_embedding_tasks(index_tasks, program_ids, overwr return contentfile_ids = ( - ContentFile.objects.filter( - learning_resource_id__in=program_ids, - published=True, - ) + qdrant_content_files(LearningResource.objects.filter(id__in=program_ids)) .order_by("id") .values_list("id", flat=True) ) @@ -289,10 +287,8 @@ def start_embed_resources(self, indexes, skip_content_files, overwrite): # noqa # Embed published content files across all runs of the course # (Qdrant retains all runs, not just best_run). contentfiles = ( - ContentFile.objects.filter(published=True) - .filter( - Q(run__learning_resource=course) - | Q(learning_resource=course) + qdrant_content_files( + LearningResource.objects.filter(id=course.id) ) .order_by("id") .values_list("id", flat=True) @@ -393,10 +389,8 @@ def embed_learning_resources_by_id(self, ids, skip_content_files, overwrite): # Embed published content files across all runs of the course # (Qdrant retains all runs, not just best_run). content_ids = ( - ContentFile.objects.filter(published=True) - .filter( - Q(run__learning_resource=course) - | Q(learning_resource=course) + qdrant_content_files( + LearningResource.objects.filter(id=course.id) ) .order_by("id") .values_list("id", flat=True) @@ -465,13 +459,8 @@ def embed_new_content_files(self): log.info("Running content file embedding task") delta = datetime.timedelta(minutes=settings.QDRANT_EMBEDDINGS_TASK_LOOKBACK_WINDOW) since = now_in_utc() - delta - new_content_files = ( - ContentFile.objects.filter( - published=True, - created_on__gt=since, - ) - .exclude(run__published=False) - .exclude(learning_resource__published=False, learning_resource__test_mode=False) + new_content_files = qdrant_content_files(LearningResource.objects.all()).filter( + created_on__gt=since ) return _replace_with_finalized_chain( @@ -635,11 +624,8 @@ def embeddings_healthcheck(self): # streamed with iterator(): there are far more content files than resources, and # only one batch of ids needs to be in memory at a time to build the signatures content_file_ids = ( - ContentFile.objects.filter(published=True) + qdrant_content_files(resources) .exclude(Q(content="") | Q(content__isnull=True)) - .filter( - Q(run__learning_resource__in=resources) | Q(learning_resource__in=resources) - ) .order_by("id") .values_list("id", flat=True) .iterator(chunk_size=HEALTHCHECK_CONTENT_FILE_BATCH_SIZE) diff --git a/vector_search/tasks_test.py b/vector_search/tasks_test.py index ccf67a3a1d..9cfb918c1d 100644 --- a/vector_search/tasks_test.py +++ b/vector_search/tasks_test.py @@ -1785,3 +1785,31 @@ def test_finalize_embeddings_raises_and_clears_on_failures(embed_cache): def test_finalize_embeddings_succeeds_when_clean(embed_cache): assert finalize_embeddings("run-1") is None assert embed_cache.get("embed_errors:run-1") is None + + +def test_embed_new_content_files_includes_unpublished_runs(mocker, mocked_celery): + """ + New files on an unpublished run of a published course are embedded, matching + the post-ingest hook: Qdrant carries every run. + """ + mocker.patch("vector_search.tasks.load_course_blocklist", return_value=[]) + course = LearningResourceFactory.create( + is_course=True, create_runs=False, published=True + ) + run = LearningResourceRunFactory.create(learning_resource=course, published=False) + content_file = ContentFileFactory.create( + run=run, published=True, created_on=now_in_utc() - datetime.timedelta(minutes=5) + ) + generate_embeddings_mock = mocker.patch( + "vector_search.tasks.generate_embeddings", autospec=True + ) + + with pytest.raises(mocked_celery.replace_exception_class): + embed_new_content_files.delay() + + embedded_ids = { + cf_id + for call in generate_embeddings_mock.si.mock_calls + for cf_id in call.args[0] + } + assert content_file.id in embedded_ids diff --git a/vector_search/utils.py b/vector_search/utils.py index 4dc0798d6e..764b103507 100644 --- a/vector_search/utils.py +++ b/vector_search/utils.py @@ -1243,6 +1243,19 @@ def process_batch(docs_batch): ) +def qdrant_content_files(resources): + """ + Select the published content files of every run of, or attached directly + to, the published or test_mode resources in the `resources` queryset. + Unlike OpenSearch (see learning_resources_search.utils.opensearch_runs), + Qdrant carries every run, not just the best one. + """ + eligible = resources.filter(Q(published=True) | Q(test_mode=True)) + return ContentFile.objects.filter(published=True).filter( + Q(run__learning_resource__in=eligible) | Q(learning_resource__in=eligible) + ) + + def resources_payload_selector(): """ Return the `with_payload` value to use for the resources collection. diff --git a/vector_search/utils_test.py b/vector_search/utils_test.py index 618408de22..497985cea5 100644 --- a/vector_search/utils_test.py +++ b/vector_search/utils_test.py @@ -29,6 +29,7 @@ LearningResourceType, PlatformType, ) +from learning_resources.etl.constants import ETLSource from learning_resources.factories import ( ContentFileFactory, LearningResourceFactory, @@ -4375,3 +4376,44 @@ def test_async_content_file_chunks_for_resource_no_published_run(mocker): match=models.MatchAny(any=[resource.readable_id]), ) ] + + +def _course_with_content_files(**kwargs): + """Create a course with files on published, variant and unpublished runs, plus direct and withdrawn files""" + course = LearningResourceFactory.create(is_course=True, create_runs=False, **kwargs) + runs = [ + LearningResourceRunFactory.create(learning_resource=course, published=True), + LearningResourceRunFactory.create( + learning_resource=course, published=True, is_variant=True + ), + LearningResourceRunFactory.create(learning_resource=course, published=False), + ] + files = {ContentFileFactory.create(run=run, published=True) for run in runs} + files.add(ContentFileFactory.create(learning_resource=course, published=True)) + ContentFileFactory.create(run=runs[0], published=False) + return course, files + + +def test_qdrant_content_files_every_run(): + """Qdrant gets published files of every run, published or not, plus direct files""" + course, files = _course_with_content_files() + + selected = vs_utils.qdrant_content_files( + LearningResource.objects.filter(id=course.id) + ) + + assert set(selected) == files + + +def test_qdrant_content_files_unpublished_course_has_none(): + """Unpublished courses aren't embedded unless test_mode, which includes Canvas""" + course, _ = _course_with_content_files(published=False) + test_course, test_files = _course_with_content_files( + published=False, test_mode=True, etl_source=ETLSource.canvas.name + ) + + selected = vs_utils.qdrant_content_files( + LearningResource.objects.filter(id__in=[course.id, test_course.id]) + ) + + assert set(selected) == test_files From 3e4b437004fd457e5016838494448d9dabe89263 Mon Sep 17 00:00:00 2001 From: Ahtesham Quraish Date: Fri, 25 Sep 2026 12:17:32 +0500 Subject: [PATCH 3/9] feat(website-content): unpublish published items from the listing card (#3963) * feat(website-content): unpublish published items from the listing card Adds the three-dot menu from the design to published article and news listing cards, with Unpublish as its single action, and drops the Unpublish button from the content detail toolbar. The menu renders only for users who can edit content, so no callsite can leak it. Draft cards link to the editor, which is the only page they have. Unpublishing now takes effect within the request: the news feed entry and the article's LearningResource -- published flag and Qdrant points -- go before the response, so the listing's refetch and vector search stop serving what was just taken down. The view cache is cleared after them, so nothing can re-cache the stale listing, and vector search hydrates only published resources. Co-Authored-By: Claude Opus 5 (1M context) * fix(website-content): reconcile a sync that overtakes an unpublish Both sync tasks check `is_published` with a read and then write, and unpublishing now runs in the request, so it can land between the two with nothing queued behind it to notice: the sync would restore a published, indexed resource -- and the news feed entry -- for content that is no longer public. Whoever writes last reconciles, so each sync task re-reads the row after writing and undoes itself if the item has since been unpublished. Co-Authored-By: Claude Opus 5 (1M context) * fix(vector-search): check publication on the payload path too The published-row filter only covered database hydration, which the kill switch turns on; the default path answers from the Qdrant payloads, where a delete that is late or has failed outright kept serving an unpublished article. A payload cannot report this itself -- unpublishing deletes the point rather than rewriting it, so a stale payload still reads as published and no Qdrant-side filter can catch it. The payload path now costs one indexed lookup, asked negatively: only rows the database reports as unpublished are dropped, so a point with no row here -- a snapshot loaded from another system -- is still returned. Covered at the view level on the shipped default, not just in the hit builder. Co-Authored-By: Claude Opus 5 (1M context) * test(api): assert the patch hook invalidates the listings The page tests only assert that the PATCH was sent, so dropping the list invalidation still passed while a card unpublished from the listing stayed in the cache -- and on screen. The hook's own test now pins all three keys it invalidates, the detail retrieve one having been uncovered in the same way. Co-Authored-By: Claude Opus 5 (1M context) * feat(website-content): drop the topics section for news settings Only an article is projected into a LearningResource, so only there do topics put the content on a topic page. The news drawer keeps its SEO section and no longer fetches the topic list at all. The drawer stays presentational -- the caller decides with `showTopics` -- and reports no topics rather than an empty selection when the section is hidden, so saving news settings cannot empty a selection the editor was never shown, or re-run the publish plugins for nothing. Co-Authored-By: Claude Opus 5 (1M context) * feat(website-content): require topics to save an article An article's topics are what put it on a topic page, so saving one without them now opens the settings drawer instead -- for a draft save as well as a publish -- and the section says why it opened. News has no topics section at all, so nothing is required of it. The held-back press resumes once a topic is picked, carrying the new selection into the content write rather than PATCHing topics separately, and a publish still confirms as it would have. Closing the drawer abandons it, so a press the editor walked away from cannot fire the next time topics happen to be saved. Co-Authored-By: Claude Opus 5 (1M context) * test(website-content): pin that the unpublish hooks run in autocommit The hooks call out to Qdrant synchronously and swallow a DatabaseError to fall back to a queued task. Both are safe only outside a transaction: inside one, the call would hold it open across network I/O, and the swallowed error would poison it -- the fallback would never be queued and the next query would raise TransactionManagementError. Nothing asks for a transaction today (no ATOMIC_REQUESTS, no atomic in the write path), which two review passes have now assumed otherwise, so assert it rather than leave it to be re-derived. Verified to fail when ATOMIC_REQUESTS is switched on. Co-Authored-By: Claude Opus 5 (1M context) * revert(vector-search): drop the read-path publication filter Filtering hits against the database broke the counts beside them: on the no-query path `total` comes from Qdrant's own exact count() and the facets from facet(), neither of which knows about a Postgres filter, so totals and doc_counts over-reported and the last page came back short -- the very thing exact=True was set to prevent. Reconciling that properly means post-filtering the whole matched set in Python. The guard was partial regardless: content file hits were never checked, and unpublishing leaves content files in Qdrant by design. The removal path converges on its own -- inline delete, the retrying queued task behind it, and the sync task reconciling against the row -- so trust it. Also drops the points via remove_points_matching_params, keyed on the readable_id already in hand, rather than remove_embeddings, which re-serializes the resource only to derive the same filter. Co-Authored-By: Claude Opus 5 (1M context) * fix(website-content): re-attempt a failed reconciliation Both sync tasks undo themselves when the row turns out to be unpublished by the time they have written, and neither undo was retried on failure. In the news task the undo raises inside the task's own try, so the whole task retries -- and the retry took the "not published" path, which returned without touching the entry the previous attempt had created. That path now removes any entry instead of skipping, which is what makes the retry effective; deleting by guid is a no-op when there is nothing there, so the ordinary "queued, then unpublished" case is unchanged. The learning resource task does not retry at all, so a failed undo left the resource published and indexed with nothing behind it. It now hands the undo to the task that does retry, whose republish guard makes a late run safe. Co-Authored-By: Claude Opus 5 (1M context) * fix(website-content): do not fail an unpublish over the index work The inline removal caught only DatabaseError, but it runs the search and vector hooks inline, which fail in other ways -- an unreachable broker escapes try_with_retry_as_task, whose own fallback is an unguarded .delay(). That reached the client as a 500 after both rows had already committed as unpublished, and a retried request fires no hooks at all, since perform_update keys them off the published->unpublished transition. The deindex and the Qdrant removal were simply lost. Any failure now hands off to the retrying task, which redoes the removal in full. Queueing can fail too, for the same broker reason, so that is logged and the indexes are left to the next reindex rather than failing an unpublish whose own rows are already correct. Co-Authored-By: Claude Opus 5 (1M context) * fix(website-content): close the drawer's way past the topics rule Gating the save buttons left the drawer itself open: it is the one place a selection can be taken away, so removing every topic and saving straight from there PATCHed `topics: []` -- an article, published one included, left without the topics that put it on a topic page. Saving is now refused while a required selection is empty, which the section already explains, rather than silently ignoring the removal the editor can see on screen. Co-Authored-By: Claude Opus 5 (1M context) * fix(website-content): catch any failed undo, not just a database one The reconciliation caught `DatabaseError` alone, but the undo it guards runs the search and vector hooks inline, which fail in other ways -- an unreachable broker escapes `try_with_retry_as_task`, whose own fallback is an unguarded `.delay()`. This task does not retry, so that raised straight out, leaving the resource unpublished in the database and still in the index with nothing queued to remove it. Any failure now hands the undo to the retrying task, and queueing is itself guarded, since the broker is what may have failed. The same shape as the plugin's inline removal. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Ahtesham Quraish Co-authored-by: Claude Opus 5 (1M context) --- .../src/hooks/website_content/index.test.ts | 13 + .../api/src/hooks/website_content/index.ts | 8 + .../Articles/ArticleListingPage.test.tsx | 130 ++++++++++ .../app-pages/Articles/ArticleListingPage.tsx | 60 ++++- .../app-pages/News/NewsListingPage.test.tsx | 108 +++++++- .../src/app-pages/News/NewsListingPage.tsx | 67 +++++ frontends/main/src/common/website_content.ts | 17 ++ .../ArticleSettings/ArticleSettingsDrawer.tsx | 208 +++++++++------ .../article/ArticleEditor.happydom.test.tsx | 240 +++++++++++++----- .../news/NewsEditor.happydom.test.tsx | 45 ++++ .../core/WebsiteContentEditor.tsx | 135 ++++++---- .../WebsiteContentActionsMenu.test.tsx | 78 ++++++ .../WebsiteContentActionsMenu.tsx | 156 ++++++++++++ learning_resources/api.py | 30 +++ learning_resources/api_test.py | 59 +++++ learning_resources/plugins.py | 45 +++- learning_resources/plugins_test.py | 115 +++++++-- learning_resources/tasks.py | 34 +++ learning_resources/tasks_test.py | 115 +++++++++ news_events/plugins.py | 22 +- news_events/plugins_test.py | 54 +++- news_events/tasks.py | 41 ++- news_events/tasks_test.py | 62 +++++ website_content/views.py | 5 +- website_content/views_test.py | 147 ++++++++++- 25 files changed, 1753 insertions(+), 241 deletions(-) create mode 100644 frontends/main/src/app-pages/Articles/ArticleListingPage.test.tsx create mode 100644 frontends/main/src/page-components/WebsiteContentActionsMenu/WebsiteContentActionsMenu.test.tsx create mode 100644 frontends/main/src/page-components/WebsiteContentActionsMenu/WebsiteContentActionsMenu.tsx diff --git a/frontends/api/src/hooks/website_content/index.test.ts b/frontends/api/src/hooks/website_content/index.test.ts index a27397a719..dd79aeba58 100644 --- a/frontends/api/src/hooks/website_content/index.test.ts +++ b/frontends/api/src/hooks/website_content/index.test.ts @@ -102,6 +102,19 @@ describe("Website Content CRUD", () => { expect(queryClient.invalidateQueries).toHaveBeenCalledWith({ queryKey: websiteContentKeys.detail(article.id), }) + expect(queryClient.invalidateQueries).toHaveBeenCalledWith({ + queryKey: websiteContentKeys.websiteContentDetailRetrieve( + article.slug || String(article.id), + ), + }) + /** + * The listings render the fields a patch can change -- title, summary and + * whether the item is published at all. Without this, a card unpublished + * from the listing stays on screen until something else refetches. + */ + expect(queryClient.invalidateQueries).toHaveBeenCalledWith({ + queryKey: websiteContentKeys.listRoot(), + }) }) test("useWebsiteContentDestroy calls correct API", async () => { diff --git a/frontends/api/src/hooks/website_content/index.ts b/frontends/api/src/hooks/website_content/index.ts index 354d0cf197..ac3a3bdbb5 100644 --- a/frontends/api/src/hooks/website_content/index.ts +++ b/frontends/api/src/hooks/website_content/index.ts @@ -136,6 +136,14 @@ const useWebsiteContentPartialUpdate = ({ meta }: MutationHookOptions = {}) => { client.invalidateQueries({ queryKey: websiteContentKeys.websiteContentDetailRetrieve(identifier), }) + /** + * The listings render the fields a patch can change -- title, summary + * and whether the item is published at all -- so they go stale too. The + * destroy hook already invalidates them for the same reason. + */ + client.invalidateQueries({ + queryKey: websiteContentKeys.listRoot(), + }) }, }) } diff --git a/frontends/main/src/app-pages/Articles/ArticleListingPage.test.tsx b/frontends/main/src/app-pages/Articles/ArticleListingPage.test.tsx new file mode 100644 index 0000000000..cff02dfbd7 --- /dev/null +++ b/frontends/main/src/app-pages/Articles/ArticleListingPage.test.tsx @@ -0,0 +1,130 @@ +import React from "react" +import { screen, waitFor } from "@testing-library/react" +import userEvent from "@testing-library/user-event" +import { setMockResponse, factories, urls, makeRequest } from "api/test-utils" +import { renderWithProviders } from "@/test-utils" +import { ArticleListingPage } from "./ArticleListingPage" + +const setup = ({ + isArticleEditor = true, + isPublished = true, +}: { isArticleEditor?: boolean; isPublished?: boolean } = {}) => { + const user = factories.user.user({ + is_authenticated: true, + is_article_editor: isArticleEditor, + }) + setMockResponse.get(urls.userMe.get(), user) + + const article = factories.websiteContent.websiteContent({ + id: 701, + title: "Breaking the old model of education", + content_type: "article", + is_published: isPublished, + }) + setMockResponse.get( + urls.websiteContent.list({ limit: 10, offset: 0, content_type: "article" }), + { count: 1, next: null, previous: null, results: [article] }, + ) + + renderWithProviders(, { user }) + return { article } +} + +const menuName = (title: string) => `More options for ${title}` + +/** + * The page renders its mobile and desktop layouts together and hides one with + * CSS, so every card is in the DOM twice. These queries take the first match + * rather than asserting a single one. + */ +const findMenuButtons = (title: string) => + screen.findAllByRole("button", { name: menuName(title) }) +const queryMenuButtons = (title: string) => + screen.queryAllByRole("button", { name: menuName(title) }) + +describe("ArticleListingPage article actions", () => { + test("an editor can unpublish a published article from the card", async () => { + const { article } = setup() + setMockResponse.patch(urls.websiteContent.details(article.id), { + ...article, + is_published: false, + }) + + await userEvent.click((await findMenuButtons(article.title))[0]) + await userEvent.click( + await screen.findByRole("menuitem", { name: "Unpublish" }), + ) + + // The menu only asks; the dialog owns the confirmation. + await screen.findByRole("heading", { name: "Unpublish article" }) + await userEvent.click( + await screen.findByRole("button", { name: "Yes, Unpublish article" }), + ) + + await waitFor(() => { + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ + method: "patch", + url: urls.websiteContent.details(article.id), + body: { is_published: false }, + }), + ) + }) + }) + + test("cancelling the dialog unpublishes nothing", async () => { + const { article } = setup() + + await userEvent.click((await findMenuButtons(article.title))[0]) + await userEvent.click( + await screen.findByRole("menuitem", { name: "Unpublish" }), + ) + await userEvent.click(await screen.findByRole("button", { name: "Cancel" })) + + expect(makeRequest).not.toHaveBeenCalledWith( + expect.objectContaining({ method: "patch" }), + ) + }) + + test("the menu is hidden from users who cannot edit articles", async () => { + const { article } = setup({ isArticleEditor: false }) + + await screen.findAllByText(article.title) + + expect(queryMenuButtons(article.title)).toHaveLength(0) + }) + + test("a draft has no menu, since unpublishing is the only action", async () => { + const { article } = setup({ isPublished: false }) + + await screen.findAllByText(article.title) + + expect(queryMenuButtons(article.title)).toHaveLength(0) + }) +}) + +describe("ArticleListingPage links", () => { + const hrefs = (title: string) => + screen + .getAllByRole("link", { name: title }) + .map((a) => a.getAttribute("href")) + + test("a draft links to the editor, since it has no public page", async () => { + const { article } = setup({ isPublished: false }) + + await screen.findAllByText(article.title) + + /* Every link on the card, so the title and the image cannot diverge. */ + const unique = [...new Set(hrefs(article.title))] + expect(unique).toEqual([`/website_content/article/${article.id}/edit`]) + }) + + test("a published article links to its public page", async () => { + const { article } = setup({ isPublished: true }) + + await screen.findAllByText(article.title) + + const unique = [...new Set(hrefs(article.title))] + expect(unique).toEqual([`/articles/${article.slug ?? article.id}`]) + }) +}) diff --git a/frontends/main/src/app-pages/Articles/ArticleListingPage.tsx b/frontends/main/src/app-pages/Articles/ArticleListingPage.tsx index 1d47d54feb..a0aa1fad58 100644 --- a/frontends/main/src/app-pages/Articles/ArticleListingPage.tsx +++ b/frontends/main/src/app-pages/Articles/ArticleListingPage.tsx @@ -19,13 +19,19 @@ import { } from "ol-components" import Link from "next/link" import { RiArrowLeftLine, RiArrowRightLine } from "@remixicon/react" -import type { WebsiteContent } from "api/v1" +import { WebsiteContentContentTypeEnum, type WebsiteContent } from "api/v1" import { LocalDate } from "ol-utilities" import { useWebsiteContentList } from "api/hooks/website_content" import { extractArticleContent } from "@/common/websiteContentUtils" -import { articleView, websiteContentCreateView } from "@/common/urls" +import { CONTENT_TYPE_LABELS } from "@/common/website_content" +import { + articleView, + websiteContentCreateView, + websiteContentEditView, +} from "@/common/urls" import { Permission, useUserHasPermission } from "api/hooks/user" import { ButtonLink } from "@mitodl/smoot-design" +import { WebsiteContentActionsMenu } from "@/page-components/WebsiteContentActionsMenu/WebsiteContentActionsMenu" const PAGE_SIZE = 10 const MAX_PAGE = 50 @@ -57,6 +63,7 @@ const RegularStoryTitleWrapper = styled.div` const StoryCard = styled.div` display: flex; flex-direction: row; + position: relative; gap: 24px; background: white; border-radius: 8px; @@ -92,6 +99,25 @@ const StoryCard = styled.div` } ` +/** + * Holds the three-dot menu over the card's top-right corner, where the design + * puts it: level with the top of the image, inside the card's 16px padding. + * + * A sibling of the image's link rather than a child of it, so clicking the + * menu cannot navigate to the article. + */ +const StoryActions = styled.div` + position: absolute; + top: 16px; + right: 16px; + z-index: 1; + + ${theme.breakpoints.down("sm")} { + top: 16px; + right: 0; + } +` + const StoryImage = styled.div` width: 280px; min-width: 280px; @@ -372,14 +398,33 @@ const BreadcrumContainer = styled(Container)(({ theme }) => ({ const RegularStory: React.FC<{ item: WebsiteContent }> = ({ item }) => { const articleContent = extractArticleContent(item) const [imageError, setImageError] = React.useState(false) + /** + * An unpublished article has no public page to land on, so the card points + * at the editor instead. Only an editor ever sees one here: the listing + * endpoint filters unpublished items out for everyone else. + * + * By id rather than slug, which a draft may not have yet. + */ + const href = item.is_published + ? articleView(item.slug ?? String(item.id)) + : websiteContentEditView(WebsiteContentContentTypeEnum.Article, item.id) return ( + {/* A draft has nothing to unpublish; the menu hides itself from + users who cannot edit articles. */} + {item.is_published ? ( + + + + ) : null} - - {item.title} - + {item.title} {articleContent.paragraph && ( = ({ item }) => { {articleContent?.image?.src && !imageError && ( - + { expect(mainStoryInstances.length).toBeGreaterThan(0) }) }) + +/** + * The listing renders its mobile and desktop layouts together and hides one + * with CSS, so each card is in the DOM twice; these take the first match. + * + * Index 0 of the feed is the featured MainStory and the rest are + * RegularStories -- two different cards, so `index` picks which one a test + * puts the synced story in. + */ +describe("NewsListingPage story actions", () => { + const CONTENT_ID = 512 + + /* A sibling describe, so it does not inherit the suite's own beforeEach. */ + beforeEach(() => { + mockedUseFeatureFlagEnabled.mockReturnValue(true) + mockedUseFeatureFlagsLoaded.mockReturnValue(true) + }) + + afterEach(() => { + jest.clearAllMocks() + }) + + const setupWithSyncedStory = ({ + isContentEditor = true, + fromWebsiteContent = true, + index = 1, + } = {}) => { + setMockResponse.get(urls.userMe.get(), { + is_authenticated: isContentEditor, + is_article_editor: isContentEditor, + }) + const news = newsEvents.newsItems({ count: 3 }) + const story = news.results[index] as NewsFeedItem + if (fromWebsiteContent) { + /* The guid `WebsiteContentNewsPlugin` writes for synced content. */ + story.guid = `article-${CONTENT_ID}` + } + setMockResponse.get(expect.stringContaining(urls.newsEvents.list()), news) + renderWithProviders() + return story + } + + const menuButtons = (title: string) => + screen.queryAllByRole("button", { name: `More options for ${title}` }) + + /* Opens the story's menu and confirms the dialog it puts up. */ + const unpublishStory = async (title: string) => { + setMockResponse.patch(urls.websiteContent.details(CONTENT_ID), { + id: CONTENT_ID, + is_published: false, + }) + + await waitFor(() => expect(menuButtons(title).length).toBeGreaterThan(0)) + await user.click(menuButtons(title)[0]) + await user.click(await screen.findByRole("menuitem", { name: "Unpublish" })) + + await screen.findByRole("heading", { name: "Unpublish news" }) + await user.click( + await screen.findByRole("button", { name: "Yes, Unpublish news" }), + ) + } + + const expectUnpublished = () => + waitFor(() => { + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ + method: "patch", + url: urls.websiteContent.details(CONTENT_ID), + body: { is_published: false }, + }), + ) + }) + + test("an editor can unpublish a story synced from website content", async () => { + const story = setupWithSyncedStory() + + await unpublishStory(story.title) + + await expectUnpublished() + }) + + test("the featured story carries the same menu", async () => { + const story = setupWithSyncedStory({ index: 0 }) + + await unpublishStory(story.title) + + await expectUnpublished() + }) + + test("an externally ingested story has no menu", async () => { + const story = setupWithSyncedStory({ fromWebsiteContent: false }) + + await screen.findAllByText(story.title) + + /* No WebsiteContent behind it, so there is nothing to unpublish. */ + expect(menuButtons(story.title)).toHaveLength(0) + }) + + test("the menu is hidden from users who cannot edit content", async () => { + const story = setupWithSyncedStory({ isContentEditor: false }) + + await screen.findAllByText(story.title) + + expect(menuButtons(story.title)).toHaveLength(0) + }) +}) diff --git a/frontends/main/src/app-pages/News/NewsListingPage.tsx b/frontends/main/src/app-pages/News/NewsListingPage.tsx index d59a59c12f..5518f1d788 100644 --- a/frontends/main/src/app-pages/News/NewsListingPage.tsx +++ b/frontends/main/src/app-pages/News/NewsListingPage.tsx @@ -25,6 +25,11 @@ import { import type { NewsFeedItem } from "api/v0" import { LocalDate } from "ol-utilities" import { linkifyText } from "@/common/utils" +import { + CONTENT_TYPE_LABELS, + websiteContentIdFromFeedGuid, +} from "@/common/website_content" +import { WebsiteContentActionsMenu } from "@/page-components/WebsiteContentActionsMenu/WebsiteContentActionsMenu" import { NewsBanner } from "./NewsBanner" const PAGE_SIZE = 20 @@ -63,6 +68,7 @@ const FeaturedStorySection = styled.div` const MainStoryCard = styled.div` display: flex; + position: relative; border-bottom: 1px solid ${theme.custom.colors.lightGray2}; background: ${theme.custom.colors.darkGray2}; border-top: 4px solid #a31f34; @@ -202,6 +208,7 @@ const MainStoryDate = styled(Typography)` const StoryCard = styled.div` display: flex; flex-direction: row; + position: relative; gap: 24px; background: white; border-radius: 8px; @@ -237,6 +244,31 @@ const StoryCard = styled.div` } ` +/** + * Holds the three-dot menu over a card's top-right corner, where the design + * puts it: level with the top of the image, 16px inside the card. + * + * A sibling of the card's links rather than a child of one, so clicking the + * menu cannot navigate to the story. + */ +const MainStoryActions = styled.div` + position: absolute; + top: 16px; + right: 16px; + z-index: 1; +` + +/** + * The same slot on the regular card, which goes full-bleed on small screens: + * it drops its horizontal padding there, so the menu follows the content out + * to the card edge rather than staying 16px inside it. + */ +const RegularStoryActions = styled(MainStoryActions)` + ${theme.breakpoints.down("sm")} { + right: 0; + } +` + const StoryImage = styled.div` width: 280px; min-width: 280px; @@ -480,11 +512,44 @@ const NewsBannerStyled = styled(NewsBanner)<{ page: number }>( }), ) +/** + * A story's three-dot menu, or nothing where it does not apply. + * + * Both card layouts show the same menu under the same conditions, so the + * conditions live here and each card supplies its own positioned `slot`. + * Externally ingested stories have no WebsiteContent behind them, so there is + * nothing to unpublish. Presence in the feed already means the item is + * published -- unpublishing deletes the feed entry -- so unlike the article + * listing there is no published check to make. The menu itself hides from + * users who cannot edit content. + */ +const StoryActionsMenu: React.FC<{ + item: NewsFeedItem + slot: React.ComponentType<{ children: React.ReactNode }> +}> = ({ item, slot: Slot }) => { + const contentId = websiteContentIdFromFeedGuid(item.guid) + + if (contentId === null) { + return null + } + + return ( + + + + ) +} + const MainStory: React.FC<{ item: NewsFeedItem }> = ({ item }) => { const [imageError, setImageError] = React.useState(false) return ( + {item.image?.url && !imageError && ( @@ -523,6 +588,7 @@ const RegularStory: React.FC<{ item: NewsFeedItem }> = ({ item }) => { return ( + @@ -558,6 +624,7 @@ const RegularStory: React.FC<{ item: NewsFeedItem }> = ({ item }) => { const NewsListingPage: React.FC = () => { const searchParams = useAppSearchParams() const setSearchParams = useSetSearchParams() + /* News is edited behind the same permission as articles. */ const page = parseInt(searchParams.get("page") ?? "1", 10) const { data: news, isLoading } = useNewsEventsList({ diff --git a/frontends/main/src/common/website_content.ts b/frontends/main/src/common/website_content.ts index 0864a54547..4a85732c82 100644 --- a/frontends/main/src/common/website_content.ts +++ b/frontends/main/src/common/website_content.ts @@ -58,3 +58,20 @@ export const extractImageMetadata = ( alt: attrs.caption || attrs.alt || "", } } + +/** + * The WebsiteContent id behind a news feed item, or null if it has none. + * + * The news feed mixes externally ingested items with website content that + * `WebsiteContentNewsPlugin` syncs into it, and only the latter can be + * unpublished from here. The feed carries no content id, so the link is the + * guid the sync writes -- see `website_content_feed_guid` in + * `news_events/etl/articles_news.py`, which is the convention's source of + * truth. Anything that does not match that shape is not ours to act on. + */ +export const websiteContentIdFromFeedGuid = ( + guid: string | undefined, +): number | null => { + const match = /^article-(\d+)$/.exec(guid ?? "") + return match ? Number(match[1]) : null +} diff --git a/frontends/main/src/page-components/ArticleSettings/ArticleSettingsDrawer.tsx b/frontends/main/src/page-components/ArticleSettings/ArticleSettingsDrawer.tsx index 2d79f94015..de9154832a 100644 --- a/frontends/main/src/page-components/ArticleSettings/ArticleSettingsDrawer.tsx +++ b/frontends/main/src/page-components/ArticleSettings/ArticleSettingsDrawer.tsx @@ -174,8 +174,11 @@ export interface ArticleSettingsValues { * * The design still groups subtopics under their parent, which is derived * from each topic's own `parent` rather than stored alongside the id. + * + * Absent when `showTopics` is off: the drawer collected no selection, which + * is not the same as the editor having emptied one. */ - topics: number[] + topics?: number[] seoTitle: string seoDescription: string } @@ -194,6 +197,23 @@ export interface ArticleSettingsDrawerProps { * in the heading and the section copy. */ contentLabel?: string + /** + * Whether to offer the topics section. + * + * The caller decides, since what topics reach is its business: only an + * article is projected into a LearningResource, so only there do they put + * the content on a topic page. + */ + showTopics?: boolean + /** + * Whether the content cannot be saved without a topic. + * + * The section says so while none is picked, and saving is refused until one + * is: this drawer is the one place a selection can be taken away, so a + * caller that gates its own save buttons would otherwise still lose the + * topics through here. + */ + topicsRequired?: boolean /** Values to open with. Re-read each time the drawer opens. */ initialValues?: Partial /** @@ -210,6 +230,8 @@ const ArticleSettingsDrawer = ({ open, onClose, contentLabel = "Article", + showTopics = true, + topicsRequired = false, initialValues, onSave, }: ArticleSettingsDrawerProps) => { @@ -235,7 +257,7 @@ const ArticleSettingsDrawer = ({ * working from this same visible set. */ const { data: topicsData, isLoading: topicsLoading } = - useLearningResourceTopics({ limit: 1000 }, { enabled: open }) + useLearningResourceTopics({ limit: 1000 }, { enabled: open && showTopics }) const allTopics = useMemo(() => topicsData?.results ?? [], [topicsData]) @@ -260,7 +282,8 @@ const ArticleSettingsDrawer = ({ useEffect(() => { if (!open) return const values = { ...EMPTY_SETTINGS, ...initialValues } - setSelectedIds(values.topics) + /* `topics` is optional, so a caller may pass it explicitly undefined. */ + setSelectedIds(values.topics ?? []) setSeoTitle(values.seoTitle) setSeoDescription(values.seoDescription) setTopicId("") @@ -393,85 +416,93 @@ const ArticleSettingsDrawer = ({ - - - - Select Topics - - - Select one or more topics for your {contentLabel.toLowerCase()} - - - - { - setTopicId(event.target.value as string) - // The old subtopic belongs to the old parent. - setSubtopicId("") - }} - /> - - setSubtopicId(event.target.value as string) - } - /> - - - {groupedSelections.length > 0 ? ( - - {groupedSelections.map(([groupTopicId, group]) => { - const topicName = - topicsById.get(groupTopicId)?.name ?? - `Topic ${groupTopicId}` - return ( - - {topicName} - {group.map((id) => { - /* A topic added without a subtopic has no pill of its - own; its name alone represents it. */ - if (id === groupTopicId) return null - const subtopicName = - topicsById.get(id)?.name ?? `Subtopic ${id}` - return ( - - {subtopicName} - handleRemove(id)} - aria-label={`Remove ${subtopicName} from ${topicName}`} - > - - - - ) - })} - {group.every((id) => id === groupTopicId) ? ( - handleRemove(groupTopicId)} - aria-label={`Remove ${topicName}`} - > - - - ) : null} - - ) - })} - - ) : null} - + {showTopics ? ( + + + + Select Topics + + + {topicsRequired && selectedIds.length === 0 + ? `Select at least one topic to save your ${contentLabel.toLowerCase()}` + : `Select one or more topics for your ${contentLabel.toLowerCase()}`} + + + + { + setTopicId(event.target.value as string) + // The old subtopic belongs to the old parent. + setSubtopicId("") + }} + /> + + setSubtopicId(event.target.value as string) + } + /> + + + {groupedSelections.length > 0 ? ( + + {groupedSelections.map(([groupTopicId, group]) => { + const topicName = + topicsById.get(groupTopicId)?.name ?? + `Topic ${groupTopicId}` + return ( + + {topicName} + {group.map((id) => { + /* A topic added without a subtopic has no pill of its + own; its name alone represents it. */ + if (id === groupTopicId) return null + const subtopicName = + topicsById.get(id)?.name ?? `Subtopic ${id}` + return ( + + {subtopicName} + handleRemove(id)} + aria-label={`Remove ${subtopicName} from ${topicName}`} + > + + + + ) + })} + {group.every((id) => id === groupTopicId) ? ( + handleRemove(groupTopicId)} + aria-label={`Remove ${topicName}`} + > + + + ) : null} + + ) + })} + + ) : null} + + ) : null} @@ -509,8 +540,19 @@ const ArticleSettingsDrawer = ({ - ) : null} {statusSlot} ) @@ -643,6 +679,10 @@ const WebsiteContentEditor = ({ disabled={isPending || !touched || !title} onClick={() => { setIsPublishing(false) + if (topicsMissing) { + askForTopics(false) + return + } saveQuietly(false) }} size={buttonSize} @@ -664,23 +704,11 @@ const WebsiteContentEditor = ({ (!touched && contentItem?.is_published) } onClick={() => { - const publish = () => { - setIsPublishing(true) - return handleSave(true) - } - /** - * Confirm the transition to public, not every save. On - * an item that is already published this button pushes - * edits live, where "will make it publicly available" - * would be both wrong and a prompt on every save. - */ - if (contentItem?.is_published) { - // Nothing awaits this path, so do not leave the - // rejection unhandled; the alert below shows it. - publish().catch(() => undefined) - } else { - showPublishWebsiteContentDialog(contentLabel, publish) + if (topicsMissing) { + askForTopics(true) + return } + startPublish() }} size={buttonSize} endIcon={ @@ -703,8 +731,19 @@ const WebsiteContentEditor = ({ {isArticleEditor ? ( setSettingsOpen(false)} + onClose={() => { + setSettingsOpen(false) + // Dropped rather than kept: a press the editor walked away + // from must not fire the next time topics happen to be saved. + setPendingSave(null) + }} contentLabel={contentLabel} + /* Only an article becomes a LearningResource, so only there do + topics put the content on a topic page. */ + showTopics={ + contentType === WebsiteContentContentTypeEnum.Article + } + topicsRequired={topicsRequired} initialValues={{ topics }} onSave={handleSettingsSave} /> diff --git a/frontends/main/src/page-components/WebsiteContentActionsMenu/WebsiteContentActionsMenu.test.tsx b/frontends/main/src/page-components/WebsiteContentActionsMenu/WebsiteContentActionsMenu.test.tsx new file mode 100644 index 0000000000..4f0f75ed16 --- /dev/null +++ b/frontends/main/src/page-components/WebsiteContentActionsMenu/WebsiteContentActionsMenu.test.tsx @@ -0,0 +1,78 @@ +import React from "react" +import { screen, waitFor, within } from "@testing-library/react" +import userEvent from "@testing-library/user-event" +import { setMockResponse, factories, urls, makeRequest } from "api/test-utils" +import { renderWithProviders } from "@/test-utils" +import { WebsiteContentActionsMenu } from "./WebsiteContentActionsMenu" + +const TITLE = "Breaking the old model of education" +const CONTENT_ID = 701 + +const setup = ({ isArticleEditor }: { isArticleEditor: boolean }) => { + const user = factories.user.user({ + is_authenticated: isArticleEditor, + is_article_editor: isArticleEditor, + }) + setMockResponse.get(urls.userMe.get(), user) + + /* Seeds the user into the query cache, so the permission is known on the + first render and a missing menu cannot just mean a pending request. */ + renderWithProviders( + , + { user }, + ) +} + +const menuButton = () => + screen.queryByRole("button", { name: `More options for ${TITLE}` }) + +describe("WebsiteContentActionsMenu", () => { + test("a user who can edit content gets the menu", () => { + setup({ isArticleEditor: true }) + + expect(menuButton()).toBeVisible() + }) + + test("renders nothing for a user who cannot edit content", () => { + setup({ isArticleEditor: false }) + + /* Every action here edits content, so there is no read-only form of it. */ + expect(menuButton()).toBe(null) + }) + + /** + * The dialog closes only once `onConfirm` resolves, so the menu has to await + * the mutation. Fired and forgotten, the dialog would close on a failure and + * report a takedown that never happened -- and the editor would have nothing + * to retry from, since this menu silences the global error toast. + */ + test("a failed unpublish leaves the confirmation open", async () => { + setup({ isArticleEditor: true }) + setMockResponse.patch( + urls.websiteContent.details(CONTENT_ID), + { detail: "boom" }, + { code: 500 }, + ) + + await userEvent.click(menuButton()!) + await userEvent.click( + await screen.findByRole("menuitem", { name: "Unpublish" }), + ) + await userEvent.click( + await screen.findByRole("button", { name: "Yes, Unpublish article" }), + ) + + await waitFor(() => { + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ method: "patch" }), + ) + }) + + const dialog = screen.getByRole("dialog") + within(dialog).getByRole("alert") + }) +}) diff --git a/frontends/main/src/page-components/WebsiteContentActionsMenu/WebsiteContentActionsMenu.tsx b/frontends/main/src/page-components/WebsiteContentActionsMenu/WebsiteContentActionsMenu.tsx new file mode 100644 index 0000000000..1cefe78982 --- /dev/null +++ b/frontends/main/src/page-components/WebsiteContentActionsMenu/WebsiteContentActionsMenu.tsx @@ -0,0 +1,156 @@ +"use client" + +import React from "react" +import { SimpleMenu, styled, theme } from "ol-components" +import type { SimpleMenuItem, MenuOverrideProps } from "ol-components" +import { ActionButton } from "@mitodl/smoot-design" +import { RiEyeOffLine, RiMore2Fill } from "@remixicon/react" +import { useQueryClient } from "@tanstack/react-query" +import { useWebsiteContentPartialUpdate } from "api/hooks/website_content" +import { Permission, useUserHasPermission } from "api/hooks/user" +import { newsEventsKeys } from "api/hooks/newsEvents" +import { SILENCE_ERROR_TOAST } from "api/mutation-meta" +import { showUnpublishWebsiteContentDialog } from "@/page-components/WebsiteContentDialogs/PublishWebsiteContentDialog" + +/** + * The design's 32px white square, sitting over the top-right of the card. + * + * `size="small"` is already 32px; the rest is the design's white fill and 4px + * radius, which `variant="text"` leaves transparent because it is normally + * used on a plain background rather than on top of an image. + */ +const DotsButton = styled(ActionButton)(({ theme }) => ({ + backgroundColor: theme.custom.colors.white, + borderRadius: "4px", + color: theme.custom.colors.darkGray2, + ":hover:not(:disabled)": { + backgroundColor: theme.custom.colors.lightGray1, + }, + /* The design's icon is 18px, where ActionButton's own default is 1em. */ + svg: { + width: "18px", + height: "18px", + }, +})) + +const menuOverrideProps: MenuOverrideProps = { + /* Drops from the button's bottom-right corner, as the design shows it. */ + anchorOrigin: { vertical: "bottom", horizontal: "right" }, + transformOrigin: { vertical: "top", horizontal: "right" }, + slotProps: { + paper: { + sx: { + borderRadius: "4px", + /* Shadow/04dp */ + boxShadow: + "0px 2px 4px 0px rgba(37, 38, 43, 0.10), 0px 3px 8px 0px rgba(37, 38, 43, 0.12)", + /** + * The design insets each row by 24px and leaves 16px between rows. That + * spacing is put on the items rather than the popover so the hover + * target still spans the full width: MenuList's own 8px top and bottom + * padding plus 8px on each item reproduces the design's 16px, and two + * adjacent items give the 16px gap between them. + */ + ".MuiMenuItem-root": { + padding: "8px 24px", + gap: "8px", + ...theme.typography.subtitle3, + color: theme.custom.colors.silverGrayDark, + }, + /* ListItemIcon reserves 56px for alignment, which the 8px gap replaces. */ + ".MuiListItemIcon-root": { + minWidth: 0, + color: "inherit", + }, + ".MuiListItemIcon-root svg": { + width: "18px", + height: "18px", + }, + }, + }, + }, +} + +type WebsiteContentActionsMenuProps = { + /** id of the WebsiteContent item the actions apply to. */ + contentId: number + /** Display label for the content type, e.g. "Article". */ + contentLabel: string + /** Names the trigger, so a listing does not repeat one label per row. */ + title: string +} + +/** + * The three-dot menu on a content listing card. + * + * Every action here edits content, so the menu renders nothing at all for a + * user without that permission -- the check lives here rather than at each + * callsite, so a new listing cannot leak the menu by forgetting it. + * + * Unpublishing is the only action, so callers are still responsible for the + * conditions they alone know: on a draft the menu would open onto nothing. + */ +const WebsiteContentActionsMenu: React.FC = ({ + contentId, + contentLabel, + title, +}) => { + const canEditContent = useUserHasPermission(Permission.ArticleEditor) + const queryClient = useQueryClient() + const updateMutation = useWebsiteContentPartialUpdate({ + meta: SILENCE_ERROR_TOAST, + }) + + /* After the hooks above, which have to run on every render either way. */ + if (!canEditContent) { + return null + } + + const items: SimpleMenuItem[] = [ + { + key: "unpublish", + label: "Unpublish", + icon: , + onClick: () => + showUnpublishWebsiteContentDialog(contentLabel, async () => { + /** + * The dialog holds itself open until this settles and shows the + * failure inline if it rejects, which is also why the mutation + * silences the global error toast. Awaited rather than returned + * only to drop the resolved resource: `onConfirm` returns void. + */ + await updateMutation.mutateAsync({ + id: contentId, + is_published: false, + }) + /** + * Unpublishing a news item also tears down its news feed entry -- + * `WebsiteContentNewsPlugin` deletes the FeedItem -- so the feed the + * news listing reads is stale too. The content mutation itself only + * knows to invalidate website content, so that happens here. + */ + await queryClient.invalidateQueries({ + queryKey: newsEventsKeys.listRoot(), + }) + }), + }, + ] + + return ( + + + + } + /> + ) +} + +export { WebsiteContentActionsMenu } diff --git a/learning_resources/api.py b/learning_resources/api.py index 753c4ae5c9..eb31223253 100644 --- a/learning_resources/api.py +++ b/learning_resources/api.py @@ -1,5 +1,6 @@ """Learning resource APIs""" +import logging from urllib.parse import urljoin from django.conf import settings @@ -19,6 +20,8 @@ from main.utils import chunks from website_content.utils import extract_text_from_content +log = logging.getLogger(__name__) + VIEW_COUNT_BATCH_SIZE = 1000 @@ -139,3 +142,30 @@ def unpublish_website_content_learning_resource(content_id: int) -> None: resource.published = False resource.save() resource_unpublished_actions(resource) + + # That hook queues the Qdrant removal, but vector search answers from the + # payloads themselves -- see VECTOR_SEARCH_RESOURCES_FROM_PAYLOAD -- and a + # stale payload still reads as published, because unpublishing deletes the + # point rather than rewriting it. So drop the points here as well, keyed on + # the readable_id already in hand: `remove_embeddings` would re-serialize + # the resource only to derive the same filter. + # + # Best effort. The queued removal is the one carrying retries, so a Qdrant + # that cannot be reached here must not fail the unpublish, and deleting + # points that are already gone is a no-op when it runs. + if settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS: + # Gated as the search plugin gates its own: with the hooks off, + # nothing indexed the points in the first place. + from vector_search.constants import RESOURCES_COLLECTION_NAME + from vector_search.utils import remove_points_matching_params + + try: + remove_points_matching_params( + {"readable_id": website_content_readable_id(content_id)}, + collection_name=RESOURCES_COLLECTION_NAME, + ) + except Exception: + log.exception( + "Inline embedding removal failed for resource %s, leaving it queued", + resource.id, + ) diff --git a/learning_resources/api_test.py b/learning_resources/api_test.py index ca4dd3fe8b..36fca574cd 100644 --- a/learning_resources/api_test.py +++ b/learning_resources/api_test.py @@ -15,6 +15,7 @@ ) from learning_resources.factories import LearningResourceTopicFactory from learning_resources.models import LearningResource +from vector_search.constants import RESOURCES_COLLECTION_NAME from website_content.constants import WebsiteContentType from website_content.factories import WebsiteContentFactory @@ -33,6 +34,12 @@ def mock_unpublished(mocker): return mocker.patch("learning_resources.api.resource_unpublished_actions") +@pytest.fixture(autouse=True) +def mock_remove_points(mocker): + """Mock the inline Qdrant removal; the tests about it assert on this.""" + return mocker.patch("vector_search.utils.remove_points_matching_params") + + def _published_content(**kwargs): """Articles are the only type that gets mirrored, so default to one.""" kwargs.setdefault("content_type", WebsiteContentType.article.name) @@ -158,6 +165,58 @@ def test_unpublish_marks_the_resource_unpublished(mock_unpublished): mock_unpublished.assert_called_once_with(resource) +def test_unpublish_removes_the_embeddings_in_the_request(settings, mock_remove_points): + """ + The Qdrant points go before the response, not when a worker gets to them. + + `resource_unpublished_actions` queues that removal, but vector search + answers from the payloads themselves, and a payload still reads as + published because unpublishing deletes the point rather than rewriting it. + Queued alone, the unpublished article stays a search hit meanwhile. + """ + settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True + content = _published_content() + sync_website_content_to_learning_resource(content) + + unpublish_website_content_learning_resource(content.id) + + # Keyed on the readable_id rather than re-serializing the resource to + # derive the same filter, which is all `remove_embeddings` would add. + mock_remove_points.assert_called_once_with( + {"readable_id": website_content_readable_id(content.id)}, + collection_name=RESOURCES_COLLECTION_NAME, + ) + + +def test_unpublish_survives_an_unreachable_qdrant(settings, mock_remove_points): + """ + The queued removal is the one carrying retries, so a failure here is + logged and dropped: the editor's unpublish cannot fail over the index. + """ + settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True + mock_remove_points.side_effect = ConnectionError("qdrant is unreachable") + content = _published_content() + resource = sync_website_content_to_learning_resource(content) + + unpublish_website_content_learning_resource(content.id) + + resource.refresh_from_db() + assert resource.published is False + + +def test_unpublish_leaves_qdrant_alone_when_the_hooks_are_off( + settings, mock_remove_points +): + """With the indexing hooks off, nothing wrote the points to begin with.""" + settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = False + content = _published_content() + sync_website_content_to_learning_resource(content) + + unpublish_website_content_learning_resource(content.id) + + assert mock_remove_points.called is False + + def test_unpublish_without_a_resource_is_a_noop(mock_unpublished): """A draft never had a resource; unpublishing it must not raise.""" content = WebsiteContentFactory.create(is_published=False) diff --git a/learning_resources/plugins.py b/learning_resources/plugins.py index 9fbbb25573..7e6cfc3041 100644 --- a/learning_resources/plugins.py +++ b/learning_resources/plugins.py @@ -91,9 +91,7 @@ def website_content_unpublished(self, content): """ if not self._is_article(content): return - log.info( - "Scheduling learning resource removal for website content %s", content.id - ) + log.info("Removing learning resource for website content %s", content.id) content_id = content.id def trigger_async_unpublish(): @@ -101,6 +99,41 @@ def trigger_async_unpublish(): unpublish_website_content_learning_resource_task, ) - unpublish_website_content_learning_resource_task.delay(content_id) - - transaction.on_commit(trigger_async_unpublish) + try: + unpublish_website_content_learning_resource_task.delay(content_id) + except Exception: + # Queueing needs the broker, which is exactly what may have + # sent us here. Nothing further to try: the rows are already + # correct and the indexes are left to the next reindex. + log.exception( + "Could not queue the learning resource removal for content %s", + content_id, + ) + + # Inline, unlike the sync side: the resource row is what the APIs read, + # so leaving the flag to a worker keeps serving an article the editor + # has already unpublished. Taking it out of the search indexes stays + # queued inside `resource_unpublished_actions`, as for any resource. + from learning_resources.api import unpublish_website_content_learning_resource + + try: + unpublish_website_content_learning_resource(content_id) + except Exception: + # Any failure, not just a database one. This runs the search and + # vector hooks inline, which fail in other ways -- a broker that + # cannot be reached escapes `try_with_retry_as_task`, whose own + # fallback is an unguarded `.delay()`. + # + # Swallowed rather than raised because the request cannot usefully + # fail here: `perform_update` fires these hooks only on the + # published->unpublished transition, which has already committed, + # so a client that retries gets a 200 and no hooks at all. The + # rows are already correct; what is left is the index work, which + # the retrying task redoes in full -- including the Qdrant removal + # the raise skipped. on_commit, so a rolled back unpublish + # schedules nothing. + log.exception( + "Inline learning resource removal failed for content %s, queueing task", + content_id, + ) + transaction.on_commit(trigger_async_unpublish) diff --git a/learning_resources/plugins_test.py b/learning_resources/plugins_test.py index 45dcecae1a..46d6ac73f2 100644 --- a/learning_resources/plugins_test.py +++ b/learning_resources/plugins_test.py @@ -1,6 +1,7 @@ """Tests for learning_resources plugins""" import pytest +from django.db import DatabaseError from learning_resources.constants import FAVORITES_TITLE from learning_resources.factories import UserListFactory @@ -26,33 +27,22 @@ def test_favorites_plugin_user_created(existing_list): @pytest.mark.django_db -@pytest.mark.parametrize( - ("hook", "task_name"), - [ - ( - "website_content_published", - "sync_website_content_learning_resource", - ), - ( - "website_content_unpublished", - "unpublish_website_content_learning_resource_task", - ), - ], -) -def test_website_content_hooks_defer_to_a_task_on_commit(mocker, hook, task_name): +def test_website_content_published_hook_defers_to_a_task_on_commit(mocker): """ - Both hooks queue their task through on_commit. + Publishing queues its task through on_commit. Deferred so the task cannot read the content item before the write that - triggered it has landed, and so a slow index is not on the request. + triggered it has landed, and so the indexing is not on the request. """ from website_content.factories import WebsiteContentFactory content = WebsiteContentFactory.create(is_published=True, content_type="article") mock_on_commit = mocker.patch("learning_resources.plugins.transaction.on_commit") - mock_task = mocker.patch(f"learning_resources.tasks.{task_name}.delay") + mock_task = mocker.patch( + "learning_resources.tasks.sync_website_content_learning_resource.delay" + ) - getattr(WebsiteContentLearningResourcePlugin(), hook)(content) + WebsiteContentLearningResourcePlugin().website_content_published(content) assert mock_on_commit.call_count == 1 # Nothing is queued until the transaction actually commits. @@ -63,6 +53,91 @@ def test_website_content_hooks_defer_to_a_task_on_commit(mocker, hook, task_name mock_task.assert_called_once_with(content.id) +@pytest.mark.django_db +def test_website_content_unpublished_hook_runs_in_the_request(mocker): + """ + Unpublishing does not defer: the resource row is what the APIs read, so a + flag left to a worker keeps serving an article the editor has taken down. + """ + from website_content.factories import WebsiteContentFactory + + content = WebsiteContentFactory.create(is_published=False, content_type="article") + mock_on_commit = mocker.patch("learning_resources.plugins.transaction.on_commit") + mock_unpublish = mocker.patch( + "learning_resources.api.unpublish_website_content_learning_resource" + ) + + WebsiteContentLearningResourcePlugin().website_content_unpublished(content) + + mock_unpublish.assert_called_once_with(content.id) + assert mock_on_commit.called is False + + +@pytest.mark.django_db +@pytest.mark.parametrize( + "failure", + [ + # Racing the sync task for the same row. + DatabaseError("deadlock detected"), + # A broker that cannot be reached escapes `try_with_retry_as_task`, + # whose own fallback is an unguarded `.delay()`. + OSError("[Errno 111] Connection refused"), + # Anything else the search or vector hooks raise. + RuntimeError("boom"), + ], +) +def test_website_content_unpublished_hook_queues_task_on_failure(mocker, failure): + """ + Any failure hands off to the retrying task rather than raising. + + The editor's unpublish must not fail over the index, and a retried request + would not help: `perform_update` fires this hook only on the + published->unpublished transition, which has already committed. + """ + from website_content.factories import WebsiteContentFactory + + content = WebsiteContentFactory.create(is_published=False, content_type="article") + mock_on_commit = mocker.patch("learning_resources.plugins.transaction.on_commit") + mocker.patch( + "learning_resources.api.unpublish_website_content_learning_resource", + side_effect=failure, + ) + mock_task = mocker.patch( + "learning_resources.tasks.unpublish_website_content_learning_resource_task.delay" + ) + + WebsiteContentLearningResourcePlugin().website_content_unpublished(content) + + assert mock_on_commit.call_count == 1 + mock_on_commit.call_args[0][0]() + mock_task.assert_called_once_with(content.id) + + +@pytest.mark.django_db +def test_website_content_unpublished_hook_survives_an_unreachable_broker(mocker): + """ + Queueing needs the broker, which may be what failed in the first place. + + There is nothing further to try at that point, so it is logged and the + indexes are left to the next reindex -- the editor's unpublish still + stands, since the rows it owns are already correct. + """ + from website_content.factories import WebsiteContentFactory + + content = WebsiteContentFactory.create(is_published=False, content_type="article") + mocker.patch( + "learning_resources.api.unpublish_website_content_learning_resource", + side_effect=OSError("[Errno 111] Connection refused"), + ) + mocker.patch( + "learning_resources.tasks.unpublish_website_content_learning_resource_task.delay", + side_effect=OSError("[Errno 111] Connection refused"), + ) + + # Raising nothing is the assertion. + WebsiteContentLearningResourcePlugin().website_content_unpublished(content) + + @pytest.mark.django_db @pytest.mark.parametrize( "hook", ["website_content_published", "website_content_unpublished"] @@ -79,7 +154,11 @@ def test_website_content_hooks_skip_news(mocker, hook): content = WebsiteContentFactory.create(is_published=True, content_type="news") mock_on_commit = mocker.patch("learning_resources.plugins.transaction.on_commit") + mock_unpublish = mocker.patch( + "learning_resources.api.unpublish_website_content_learning_resource" + ) getattr(WebsiteContentLearningResourcePlugin(), hook)(content) assert mock_on_commit.called is False + assert mock_unpublish.called is False diff --git a/learning_resources/tasks.py b/learning_resources/tasks.py index 483fd674ef..cf9d7f581a 100644 --- a/learning_resources/tasks.py +++ b/learning_resources/tasks.py @@ -1089,6 +1089,40 @@ def sync_website_content_learning_resource(content_id: int) -> None: return sync_website_content_to_learning_resource(content) + # The check above is a read, and the row can change under it: unpublishing + # runs in the request, so it can land between that read and this write and + # then have nothing queued behind it to notice -- leaving a published, + # indexed resource for content that is no longer public. Whoever writes + # last reconciles, so re-read the row and undo if it has moved on. + if not WebsiteContent.objects.filter(id=content_id, is_published=True).exists(): + log.info( + "WebsiteContent %s was unpublished while syncing, undoing the sync", + content_id, + ) + try: + unpublish_website_content_learning_resource(content_id) + except Exception: + # Any failure, not just a database one: the undo runs the search + # and vector hooks inline, which fail in other ways -- a broker + # that cannot be reached escapes `try_with_retry_as_task`, whose + # own fallback is an unguarded `.delay()`. + # + # This task does not retry, so raising would leave the resource + # unpublished in the database and still in the index with nothing + # behind it. Hand the undo to the task that does retry; its own + # republish guard makes a late run safe. + log.exception( + "Undoing the sync failed for content %s, queueing the removal", + content_id, + ) + try: + unpublish_website_content_learning_resource_task.delay(content_id) + except Exception: + # Queueing needs the broker, which is exactly what may have + # sent us here. Nothing further to try: the row is already + # unpublished and the indexes are left to the next reindex. + log.exception("Could not queue the removal for content %s", content_id) + @app.task(acks_late=True, reject_on_worker_lost=True) def unpublish_website_content_learning_resource_task(content_id: int) -> None: diff --git a/learning_resources/tasks_test.py b/learning_resources/tasks_test.py index b4c7d3a6bf..0a0f2d0c3f 100644 --- a/learning_resources/tasks_test.py +++ b/learning_resources/tasks_test.py @@ -7,7 +7,9 @@ import pytest from decorator import contextmanager +from django.db import DatabaseError from django.utils import timezone +from kombu.exceptions import OperationalError as BrokerError from moto import mock_aws from safedelete.config import HARD_DELETE @@ -1644,6 +1646,119 @@ def test_sync_website_content_learning_resource_guards( assert mock_sync.called is expect_sync +def test_sync_website_content_undoes_itself_if_unpublished_meanwhile(mocker): + """ + A sync that overtakes an unpublish reconciles against the row. + + Unpublishing happens in the request, so it can land after this task has + read the item as published but before the task writes -- and there is + nothing queued behind it to notice. Left alone, the sync would restore a + published, indexed resource for content that is no longer public. + """ + from learning_resources.api import ( + sync_website_content_to_learning_resource, + website_content_readable_id, + ) + from website_content.factories import WebsiteContentFactory + from website_content.models import WebsiteContent + + # The search hand-off is covered in its own tests; this is about the row. + mocker.patch("learning_resources.api.resource_upserted_actions") + mocker.patch("learning_resources.api.resource_unpublished_actions") + content = WebsiteContentFactory.create(is_published=True, content_type="article") + + def unpublish_then_sync(item): + """Stand in for the editor's unpublish, after the published check.""" + WebsiteContent.objects.filter(id=item.id).update(is_published=False) + return sync_website_content_to_learning_resource(item) + + mocker.patch( + "learning_resources.tasks.sync_website_content_to_learning_resource", + side_effect=unpublish_then_sync, + ) + + tasks.sync_website_content_learning_resource.delay(content.id) + + resource = LearningResource.objects.get( + readable_id=website_content_readable_id(content.id) + ) + assert resource.published is False + + +def _unpublish_while_syncing(item): + """Stand in for the editor's unpublish, after the task's published check.""" + from website_content.models import WebsiteContent + + WebsiteContent.objects.filter(id=item.id).update(is_published=False) + + +@pytest.mark.parametrize( + "failure", + [ + # Losing a row lock race with the unpublish. + DatabaseError("deadlock detected"), + # An unreachable broker escapes `try_with_retry_as_task`, whose own + # fallback is an unguarded `.delay()`. + BrokerError("[Errno 111] Connection refused"), + # Anything else the search or vector hooks raise. + RuntimeError("boom"), + ], +) +def test_sync_website_content_queues_the_removal_if_undoing_fails(mocker, failure): + """ + Any failed undo is handed to the task that retries. + + This task does not retry, so raising would leave the resource unpublished + in the database and still in the index, with nothing behind it. + """ + from website_content.factories import WebsiteContentFactory + + content = WebsiteContentFactory.create(is_published=True, content_type="article") + + mocker.patch( + "learning_resources.tasks.sync_website_content_to_learning_resource", + side_effect=_unpublish_while_syncing, + ) + mocker.patch( + "learning_resources.tasks.unpublish_website_content_learning_resource", + side_effect=failure, + ) + mock_task = mocker.patch( + "learning_resources.tasks.unpublish_website_content_learning_resource_task.delay" + ) + + tasks.sync_website_content_learning_resource.delay(content.id) + + mock_task.assert_called_once_with(content.id) + + +def test_sync_website_content_survives_an_unreachable_broker(mocker): + """ + Queueing the undo needs the broker, which may be what failed in the first + place. There is nothing further to try, so it is logged and the indexes are + left to the next reindex rather than failing the sync that did work. + """ + from website_content.factories import WebsiteContentFactory + + content = WebsiteContentFactory.create(is_published=True, content_type="article") + + mocker.patch( + "learning_resources.tasks.sync_website_content_to_learning_resource", + side_effect=_unpublish_while_syncing, + ) + mocker.patch( + "learning_resources.tasks.unpublish_website_content_learning_resource", + side_effect=BrokerError("[Errno 111] Connection refused"), + ) + mocker.patch( + "learning_resources.tasks.unpublish_website_content_learning_resource_task.delay", + side_effect=BrokerError("[Errno 111] Connection refused"), + ) + + # Raising nothing is the assertion. + tasks.sync_website_content_learning_resource.delay(content.id) + + def test_unpublish_website_content_learning_resource_task(mocker): """The removal task works from the id, so a deleted item still leaves the index.""" mock_unpublish = mocker.patch( diff --git a/news_events/plugins.py b/news_events/plugins.py index eac88a703e..83f0ff446e 100644 --- a/news_events/plugins.py +++ b/news_events/plugins.py @@ -3,7 +3,7 @@ import logging from django.apps import apps -from django.db import transaction +from django.db import DatabaseError, transaction log = logging.getLogger(__name__) @@ -72,6 +72,8 @@ def website_content_unpublished(self, content): content.title, ) + from news_events.etl.articles_news import delete_website_content_news_from_news + content_id = content.id def trigger_async_delete(): @@ -83,5 +85,19 @@ def trigger_async_delete(): ) delete_website_content_from_news.delay(content_id) - # on_commit, so the feed is only torn down once the unpublish is durable. - transaction.on_commit(trigger_async_delete) + # Unlike the sync side, removal is a single indexed delete, so it runs + # inline: by the time the unpublish request answers, the news feed no + # longer serves the item. Queued, it left a window the editor could see + # through -- the listing refetches as soon as the request returns, and + # got the story back it had just unpublished. + try: + delete_website_content_news_from_news(content_id) + except DatabaseError: + # Losing a race with the sync task for the same row is transient, so + # hand off to the retrying task rather than failing the unpublish + # over it. on_commit, so a rolled back unpublish schedules nothing. + log.exception( + "Inline news feed removal failed for content %s, queueing task", + content_id, + ) + transaction.on_commit(trigger_async_delete) diff --git a/news_events/plugins_test.py b/news_events/plugins_test.py index b89f0d80a8..1755a90a56 100644 --- a/news_events/plugins_test.py +++ b/news_events/plugins_test.py @@ -3,6 +3,7 @@ from unittest.mock import patch import pytest +from django.db import DatabaseError from main.factories import UserFactory from news_events.plugins import WebsiteContentNewsPlugin @@ -112,8 +113,18 @@ def test_website_content_published_hook_captures_content_id(): mock_task.assert_called_once_with(content.id) -def test_website_content_unpublished_hook_calls_delete_task(): - """The unpublish hook schedules the feed removal task on commit""" +def test_website_content_unpublished_hook_removes_feed_item_inline(): + """ + The feed entry is gone by the time the hook returns. + + Nothing may be left for a worker to pick up: the listing refetches as soon + as the unpublish request answers, so anything deferred here is a window in + which the news feed still serves the unpublished story. + """ + from news_events.constants import FeedType + from news_events.etl.articles_news import website_content_feed_guid + from news_events.models import FeedItem, FeedSource + user = UserFactory.create() content = WebsiteContent.objects.create( title="Test Article", @@ -122,12 +133,51 @@ def test_website_content_unpublished_hook_calls_delete_task(): user=user, content_type="news", ) + source = FeedSource.objects.create( + title="MIT Learn Articles", url="/news", feed_type=FeedType.news.name + ) + guid = website_content_feed_guid(content.id) + FeedItem.objects.create( + guid=guid, source=source, title=content.title, url="/news/test-article" + ) plugin = WebsiteContentNewsPlugin() with patch("news_events.plugins.transaction.on_commit") as mock_on_commit: plugin.website_content_unpublished(content) + assert not FeedItem.objects.filter(guid=guid).exists() + # No deferred work at all: the removal already happened. + assert not mock_on_commit.called + + +def test_website_content_unpublished_hook_queues_task_on_db_error(): + """ + A transient database error hands off to the retrying task. + + Racing the sync task for the same row must not fail the editor's unpublish, + and the feed entry still has to go eventually. + """ + user = UserFactory.create() + content = WebsiteContent.objects.create( + title="Test Article", + content={}, + is_published=False, + user=user, + content_type="news", + ) + + plugin = WebsiteContentNewsPlugin() + + with ( + patch( + "news_events.etl.articles_news.delete_website_content_news_from_news", + side_effect=DatabaseError("deadlock detected"), + ), + patch("news_events.plugins.transaction.on_commit") as mock_on_commit, + ): + plugin.website_content_unpublished(content) + assert mock_on_commit.call_count == 1 callback = mock_on_commit.call_args[0][0] diff --git a/news_events/tasks.py b/news_events/tasks.py index b8fbc1deb3..d0469e525b 100644 --- a/news_events/tasks.py +++ b/news_events/tasks.py @@ -129,26 +129,51 @@ def sync_website_content_to_news(self, content_id: int): """ import logging - from news_events.etl.articles_news import sync_single_website_content_news_to_news + from news_events.etl.articles_news import ( + delete_website_content_news_from_news, + sync_single_website_content_news_to_news, + ) from website_content.models import WebsiteContent logger = logging.getLogger(__name__) try: - content = WebsiteContent.objects.get(id=content_id, is_published=True) + content = WebsiteContent.objects.filter( + id=content_id, is_published=True + ).first() + if content is None: + # Unpublished or gone since this was queued. Remove any entry + # rather than simply skipping: an earlier attempt of this task may + # have created one before the row changed -- including an attempt + # whose reconciliation below failed, which is what a retry lands + # here. Deleting by guid is a no-op when there is nothing to + # delete, so the ordinary "queued, then unpublished" case is free. + logger.warning( + "WebsiteContent %s not found or not published, removing any feed entry", + content_id, + ) + delete_website_content_news_from_news(content_id) + return sync_single_website_content_news_to_news(content) + # The published check above is a read, and the row can change under it: + # unpublishing runs in the request, so it can land between that read + # and this write and then have nothing queued behind it to notice -- + # leaving the story in the feed after it was taken down. Whoever writes + # last reconciles, so re-read the row and undo if it has moved on. + if not WebsiteContent.objects.filter(id=content_id, is_published=True).exists(): + logger.info( + "WebsiteContent %s was unpublished while syncing, undoing the sync", + content_id, + ) + delete_website_content_news_from_news(content_id) + return + logger.info( "Successfully synced content %s to news feed", content_id, ) - except WebsiteContent.DoesNotExist: - logger.warning( - "WebsiteContent %s not found or not published, skipping sync", - content_id, - ) - return except Exception: logger.exception( "Failed to sync content %s to news feed (retry %s/%s)", diff --git a/news_events/tasks_test.py b/news_events/tasks_test.py index d9224a7268..1c3c918563 100644 --- a/news_events/tasks_test.py +++ b/news_events/tasks_test.py @@ -153,6 +153,68 @@ def _news_content(user, *, is_published): @pytest.mark.django_db +def test_sync_website_content_to_news_removes_an_entry_it_must_not_keep(): + """ + Finding the item unpublished removes any feed entry, rather than skipping. + + That is what makes a retry effective: the reconciliation below runs inside + the task's own try, so a delete that fails there retries the whole task -- + and the retry arrives here, with the row already unpublished. Skipping + would strand the entry it had just created. + """ + from news_events.constants import FeedType + from news_events.etl.articles_news import website_content_feed_guid + from news_events.models import FeedItem, FeedSource + from website_content.factories import WebsiteContentFactory + + content = WebsiteContentFactory.create(is_published=False, content_type="news") + source = FeedSource.objects.create( + title="MIT Learn Articles", url="/news", feed_type=FeedType.news.name + ) + guid = website_content_feed_guid(content.id) + FeedItem.objects.create( + guid=guid, source=source, title=content.title, url="/news/stranded" + ) + + tasks.sync_website_content_to_news.delay(content.id) + + assert not FeedItem.objects.filter(guid=guid).exists() + + +@pytest.mark.django_db +def test_sync_website_content_to_news_undoes_itself_if_unpublished_meanwhile(mocker): + """ + A sync that overtakes an unpublish reconciles against the row. + + Unpublishing removes the feed entry in the request, so it can land after + this task has read the item as published but before the task writes -- and + there is nothing queued behind it to notice. Left alone, the sync would put + the story back in the feed after it was taken down. + """ + from news_events.etl import articles_news + from news_events.models import FeedItem + from website_content.factories import WebsiteContentFactory + from website_content.models import WebsiteContent + + content = WebsiteContentFactory.create(is_published=True, content_type="news") + real_sync = articles_news.sync_single_website_content_news_to_news + + def unpublish_then_sync(item): + """Stand in for the editor's unpublish, after the published check.""" + WebsiteContent.objects.filter(id=item.id).update(is_published=False) + return real_sync(item) + + mocker.patch( + "news_events.etl.articles_news.sync_single_website_content_news_to_news", + side_effect=unpublish_then_sync, + ) + + tasks.sync_website_content_to_news.delay(content.id) + + guid = articles_news.website_content_feed_guid(content.id) + assert not FeedItem.objects.filter(guid=guid).exists() + + def test_delete_website_content_from_news_removes_the_entry(mocker, user): """The ordinary case: the item is unpublished, so its entry goes""" content = _news_content(user, is_published=False) diff --git a/website_content/views.py b/website_content/views.py index a5ce2332c8..3a6aa65b8f 100644 --- a/website_content/views.py +++ b/website_content/views.py @@ -122,7 +122,6 @@ def perform_update(self, serializer): # an unpublish, which the saved instance alone cannot tell us. was_published = serializer.instance.is_published content = serializer.save() - transaction.on_commit(clear_views_cache) purge_content_on_save(content) content_published_actions(content=content) if was_published and not content.is_published: @@ -130,6 +129,10 @@ def perform_update(self, serializer): # now-private page and the listing still need clearing. purge_content_on_unpublish(content) content_unpublished_actions(content=content) + # Last here, unlike on create: the unpublish plugins take the news feed + # entry out synchronously, and clearing the cache before that ran would + # let any request in between re-cache the listing that still has it. + transaction.on_commit(clear_views_cache) serializer.instance = self._reloaded_for_response(content) def perform_destroy(self, instance): diff --git a/website_content/views_test.py b/website_content/views_test.py index 071998a79d..aeb92b6bdc 100644 --- a/website_content/views_test.py +++ b/website_content/views_test.py @@ -1,6 +1,7 @@ """Test for website_content views""" import pytest +from django.db import transaction from rest_framework.reverse import reverse from learning_resources.factories import LearningResourceTopicFactory @@ -31,6 +32,10 @@ def _mock_learning_resource_sync(mocker): mocker.patch( "learning_resources.tasks.sync_website_content_learning_resource.delay" ) + # The unpublish direction runs in the request rather than in the task, so + # the function is what has to be stubbed here; the task remains the + # fallback for a transient database error. + mocker.patch("learning_resources.api.unpublish_website_content_learning_resource") mocker.patch( "learning_resources.tasks.unpublish_website_content_learning_resource_task.delay" ) @@ -142,13 +147,13 @@ def mock_clear_views_cache(mocker): return mocker.patch("website_content.views.clear_views_cache") -def _make_content(user, *, is_published): +def _make_content(user, *, is_published, content_type="news"): return WebsiteContent.objects.create( title="t", content={}, is_published=is_published, user=user, - content_type="news", + content_type=content_type, ) @@ -356,6 +361,144 @@ def test_update_triggers_unpublish_actions_only_on_the_transition( assert mock_purge.called is expect_unpublish_actions +def test_unpublish_removes_the_news_feed_entry_inline(staff_client, user): + """ + The feed entry is gone by the time the unpublish request answers. + + The news listing refetches the moment it returns, so an entry left for a + worker to remove comes straight back to the editor who just unpublished it. + No worker runs here and no on_commit callback is executed: the removal has + to have happened during the request itself. + """ + from news_events.etl.articles_news import ( + sync_single_website_content_news_to_news, + website_content_feed_guid, + ) + from news_events.models import FeedItem + + content = _make_content(user, is_published=True) + sync_single_website_content_news_to_news(content) + guid = website_content_feed_guid(content.id) + assert FeedItem.objects.filter(guid=guid).exists() + url = reverse( + "website_content:v1:website_content-detail", kwargs={"pk": content.id} + ) + + resp = staff_client.patch(url, {"is_published": False}, format="json") + + assert resp.status_code == 200 + assert not FeedItem.objects.filter(guid=guid).exists() + + +def test_unpublish_survives_a_failing_search_hand_off( + staff_client, user, mocker, django_capture_on_commit_callbacks +): + """ + An unpublish is not failed by the index work behind it. + + The hooks run the search and vector plugins inline, which can fail in ways + a database error does not cover -- an unreachable broker, for one. Raising + would report a failure that did not happen: the rows are already committed + unpublished, and a retried request fires no hooks at all, because + `perform_update` keys them off the published->unpublished transition. + """ + mocker.patch( + "learning_resources.api.unpublish_website_content_learning_resource", + side_effect=OSError("[Errno 111] Connection refused"), + ) + mock_task = mocker.patch( + "learning_resources.tasks.unpublish_website_content_learning_resource_task.delay" + ) + content = _make_content(user, is_published=True, content_type="article") + url = reverse( + "website_content:v1:website_content-detail", kwargs={"pk": content.id} + ) + + with django_capture_on_commit_callbacks(execute=True): + resp = staff_client.patch(url, {"is_published": False}, format="json") + + assert resp.status_code == 200 + content.refresh_from_db() + assert content.is_published is False + # Left with the task that retries, rather than dropped. + mock_task.assert_called_once_with(content.id) + + +@pytest.mark.django_db(transaction=True) +def test_unpublish_hooks_run_outside_a_transaction(staff_client, user, mocker): + """ + The unpublish hooks must not run inside a transaction. + + They do two things that are only safe in autocommit: a synchronous call out + to Qdrant, which would otherwise hold a transaction open across network + I/O, and swallowing a `DatabaseError` to fall back to a queued task, which + inside an atomic block would poison the transaction instead -- the fallback + would never be queued, and the next query would raise + `TransactionManagementError`. + + Nothing here asks for a transaction today, so this asserts the property + rather than trusting it: enabling `ATOMIC_REQUESTS` (or wrapping the view) + breaks the assumption, and this is what says so. + """ + seen = {} + + def record(*, content): + connection = transaction.get_connection() + seen["in_atomic_block"] = connection.in_atomic_block + seen["autocommit"] = connection.get_autocommit() + + mocker.patch("website_content.views.content_unpublished_actions", record) + mocker.patch("website_content.views.clear_views_cache") + content = _make_content(user, is_published=True) + url = reverse( + "website_content:v1:website_content-detail", kwargs={"pk": content.id} + ) + + resp = staff_client.patch(url, {"is_published": False}, format="json") + + assert resp.status_code == 200 + assert seen == {"in_atomic_block": False, "autocommit": True} + + +@pytest.mark.django_db(transaction=True) +def test_unpublish_clears_the_view_cache_after_removing_the_feed_entry( + staff_client, user, mocker +): + """ + The cached news listing is dropped only once the entry it contains is gone. + + Cleared any earlier, a request landing in between re-caches the listing + that still holds the story, which then outlives the unpublish by the whole + cache duration. Needs a real commit: inside the usual test transaction + every on_commit callback is deferred to the end regardless of order. + """ + from news_events.etl.articles_news import ( + sync_single_website_content_news_to_news, + website_content_feed_guid, + ) + from news_events.models import FeedItem + + content = _make_content(user, is_published=True) + sync_single_website_content_news_to_news(content) + guid = website_content_feed_guid(content.id) + seen = {} + + mocker.patch( + "website_content.views.clear_views_cache", + side_effect=lambda: seen.update( + feed_entry=FeedItem.objects.filter(guid=guid).exists() + ), + ) + url = reverse( + "website_content:v1:website_content-detail", kwargs={"pk": content.id} + ) + + resp = staff_client.patch(url, {"is_published": False}, format="json") + + assert resp.status_code == 200 + assert seen == {"feed_entry": False} + + def test_create_with_topics(staff_client): """Topics sent on create are persisted and echoed back.""" topics = LearningResourceTopicFactory.create_batch(2) From 88b2386aa4b084dc9afe9fe71ab7e0402babca55 Mon Sep 17 00:00:00 2001 From: Alex H Date: Fri, 25 Sep 2026 10:06:59 -0400 Subject: [PATCH 4/9] Sanitize MITPE news and events summary/content like sibling sources (#3977) * Sanitize MITPE news summary/content like every other source Signed-off-by: Alex H * Sanitize MITPE events summary/content, same gap as mitpe_news Signed-off-by: Alex H * Address review feedback: dedupe sanitization, test onerror payload Signed-off-by: Alex H --------- Signed-off-by: Alex H --- news_events/etl/mitpe_events.py | 7 ++--- news_events/etl/mitpe_events_test.py | 38 +++++++++++++++++++++++++++- news_events/etl/mitpe_news.py | 6 +++-- news_events/etl/mitpe_news_test.py | 36 ++++++++++++++++++++++++-- 4 files changed, 79 insertions(+), 8 deletions(-) diff --git a/news_events/etl/mitpe_events.py b/news_events/etl/mitpe_events.py index fe2f9afa75..4a2e728dd2 100644 --- a/news_events/etl/mitpe_events.py +++ b/news_events/etl/mitpe_events.py @@ -6,7 +6,7 @@ from django.conf import settings -from main.utils import now_in_utc +from main.utils import clean_data, now_in_utc from news_events.constants import ALL_AUDIENCES, FeedType from news_events.etl.utils import fetch_data_by_page, parse_date_time_range @@ -75,12 +75,13 @@ def transform_item(item: dict) -> dict: if (not start_dt or start_dt < now) and (not end_dt or end_dt < now): return None + summary = clean_data(html.unescape(item["summary"])) return { "guid": item["id"], "title": html.unescape(item["title"]), "url": urljoin(settings.MITPE_BASE_URL, item["url"]), - "summary": html.unescape(item["summary"]), - "content": html.unescape(item["summary"]), + "summary": summary, + "content": summary, "image": transform_image(item), "detail": { "location": [], diff --git a/news_events/etl/mitpe_events_test.py b/news_events/etl/mitpe_events_test.py index 2206465f05..5edd689456 100644 --- a/news_events/etl/mitpe_events_test.py +++ b/news_events/etl/mitpe_events_test.py @@ -7,7 +7,7 @@ import pytest from freezegun import freeze_time -from news_events.etl.mitpe_events import extract, transform +from news_events.etl.mitpe_events import extract, transform, transform_item @pytest.fixture @@ -70,3 +70,39 @@ def test_transform(mitpe_events_json_data): assert items[3]["detail"]["event_end_datetime"] == datetime( 2023, 5, 12, 16, 0, 0, tzinfo=UTC ) + + +@freeze_time("2020-05-21") +def test_transform_item_sanitizes_entity_encoded_script(): + """Entity-encoded markup in the summary must not survive as live HTML""" + item = transform_item( + { + "id": "1", + "title": "Title", + "url": "events/1", + "summary": "Great event <script>alert(1)</script> today.", + "start_date": "2020-06-01", + "end_date": "2020-06-01", + "time_range": "9:00 AM - 5:00 PM", + } + ) + assert item["summary"] == "Great event today." + assert item["content"] == "Great event today." + + +@freeze_time("2020-05-21") +def test_transform_item_sanitizes_entity_encoded_img_onerror(): + """The ticket's exact repro payload must not survive as a live onerror handler""" + item = transform_item( + { + "id": "1", + "title": "Title", + "url": "events/1", + "summary": "Great event <img src=x onerror=alert(1)> today.", + "start_date": "2020-06-01", + "end_date": "2020-06-01", + "time_range": "9:00 AM - 5:00 PM", + } + ) + assert item["summary"] == "Great event today." + assert item["content"] == "Great event today." diff --git a/news_events/etl/mitpe_news.py b/news_events/etl/mitpe_news.py index ad50298eb7..b34c53fb27 100644 --- a/news_events/etl/mitpe_news.py +++ b/news_events/etl/mitpe_news.py @@ -6,6 +6,7 @@ from django.conf import settings +from main.utils import clean_data from news_events.constants import FeedType from news_events.etl.utils import fetch_data_by_page, parse_date @@ -84,12 +85,13 @@ def transform_item(item: list[dict]) -> dict: dict: transformed news item data """ + summary = clean_data(html.unescape(item["summary"])) return { "guid": item["id"], "title": html.unescape(item["title"]), "url": urljoin(settings.MITPE_BASE_URL, item["url"]), - "summary": html.unescape(item["summary"]), - "content": html.unescape(item["summary"]), + "summary": summary, + "content": summary, "image": transform_image(item), "detail": { "authors": parse_authors(item["author"]), diff --git a/news_events/etl/mitpe_news_test.py b/news_events/etl/mitpe_news_test.py index 2793daf327..4f1aa54047 100644 --- a/news_events/etl/mitpe_news_test.py +++ b/news_events/etl/mitpe_news_test.py @@ -6,7 +6,7 @@ import pytest -from news_events.etl.mitpe_news import extract, transform +from news_events.etl.mitpe_news import extract, transform, transform_item @pytest.fixture @@ -56,9 +56,41 @@ def test_transform(mitpe_news_json_data): "description": items[0]["title"], } assert items[0]["summary"].startswith( - "Discover how Erdin Beshimov, a lecturer at MIT & Senior" + "Discover how Erdin Beshimov, a lecturer at MIT & Senior" ) assert items[0]["summary"] == items[0]["content"] assert items[0]["detail"]["publish_date"] == datetime( 2020, 12, 4, 5, 0, 0, tzinfo=UTC ) + + +def test_transform_item_sanitizes_entity_encoded_script(): + """Entity-encoded markup in the summary must not survive as live HTML""" + item = transform_item( + { + "id": 1, + "title": "Title", + "url": "articles/1", + "summary": "<script>alert(1)</script>", + "author": "", + "date": "2020-12-04", + } + ) + assert item["summary"] == "" + assert item["content"] == "" + + +def test_transform_item_sanitizes_entity_encoded_img_onerror(): + """The ticket's exact repro payload must not survive as a live onerror handler""" + item = transform_item( + { + "id": 1, + "title": "Title", + "url": "articles/1", + "summary": "<img src=x onerror=alert(1)>", + "author": "", + "date": "2020-12-04", + } + ) + assert item["summary"] == "" + assert item["content"] == "" From 9ce3a00faa409cf6d5edca18d83f773e691b3400 Mon Sep 17 00:00:00 2001 From: Alex H Date: Fri, 25 Sep 2026 11:57:08 -0400 Subject: [PATCH 5/9] fix: scope unsubscribe lookup to the requesting user's own subscriptions (#3984) Signed-off-by: Alex H --- learning_resources_search/views.py | 3 +-- learning_resources_search/views_test.py | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/learning_resources_search/views.py b/learning_resources_search/views.py index 52417dbf02..8cbe6c7343 100644 --- a/learning_resources_search/views.py +++ b/learning_resources_search/views.py @@ -23,7 +23,6 @@ unsubscribe_user_from_percolate_query, ) from learning_resources_search.constants import CONTENT_FILE_TYPE, LEARNING_RESOURCE -from learning_resources_search.models import PercolateQuery from learning_resources_search.serializers import ( ContentFileSearchRequestSerializer, ContentFileSearchResponseSerializer, @@ -219,7 +218,7 @@ def unsubscribe(self, request, pk: int): PercolateQuerySerializer: The percolate query """ - percolate_query = get_object_or_404(PercolateQuery, id=pk) + percolate_query = get_object_or_404(self.get_queryset(), id=pk) unsubscribe_user_from_percolate_query(request.user, percolate_query) return Response( PercolateQuerySerializer(percolate_query).data["original_query"] diff --git a/learning_resources_search/views_test.py b/learning_resources_search/views_test.py index 68cc006f99..e88ead4ff4 100644 --- a/learning_resources_search/views_test.py +++ b/learning_resources_search/views_test.py @@ -20,6 +20,7 @@ LearningResourcesSearchRequestSerializer, LearningResourcesSearchResponseSerializer, ) +from main.factories import UserFactory from vector_search.constants import PROGRAM_SCORE_BOOST_NAME, default_score_boost FAKE_SEARCH_RESPONSE = { @@ -320,6 +321,30 @@ def test_user_unsubscribe_to_search_by_id(client, user): assert user.percolate_queries.count() == 0 +@pytest.mark.django_db +@factory.django.mute_signals(signals.post_delete, signals.post_save) +def test_user_cannot_unsubscribe_others_subscription(client, user): + """Unsubscribing from another user's subscription should 404, not disclose it""" + + sub_url = reverse("lr_search:v1:learning_resources_user_subscription-subscribe") + client.force_login(user) + params = {"q": "idor-test-marker-distinguishing-string"} + client.post(sub_url, json.dumps(params), content_type="application/json") + assert user.percolate_queries.count() == 1 + subscription_id = user.percolate_queries.first().id + + other_user = UserFactory.create() + client.force_login(other_user) + unsub_url = reverse( + "lr_search:v1:learning_resources_user_subscription-unsubscribe", + args=[subscription_id], + ) + resp = client.delete(unsub_url) + + assert resp.status_code == 404 + assert user.percolate_queries.count() == 1 + + @pytest.mark.django_db @factory.django.mute_signals(signals.post_delete, signals.post_save) def test_user_subscribed_to_search(client, user): From d2480c53f3f95fb15f8e05bc762f9415a6e16f0c Mon Sep 17 00:00:00 2001 From: Dan Subak Date: Fri, 25 Sep 2026 12:30:52 -0400 Subject: [PATCH 6/9] Posthog checkout_completed event (#3982) * WIP cut at adding a posthog purchase event * Add readable_id and contentType to posthog capture --- .../EnrollmentRedirectAlert.test.tsx | 58 ++++++++++++++++++- .../DashboardPage/EnrollmentRedirectAlert.tsx | 22 ++++++- frontends/main/src/common/constants.ts | 1 + 3 files changed, 77 insertions(+), 4 deletions(-) diff --git a/frontends/main/src/app-pages/DashboardPage/EnrollmentRedirectAlert.test.tsx b/frontends/main/src/app-pages/DashboardPage/EnrollmentRedirectAlert.test.tsx index 02ef896d3c..5f0a395ecb 100644 --- a/frontends/main/src/app-pages/DashboardPage/EnrollmentRedirectAlert.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/EnrollmentRedirectAlert.test.tsx @@ -10,16 +10,29 @@ import EnrollmentRedirectAlert from "./EnrollmentRedirectAlert" import { DASHBOARD_MY_LEARNING } from "@/common/urls" import * as mitxonline from "api/mitxonline-test-utils" import { trackCheckoutCompleted } from "@/common/analytics/gtm" +import { usePostHog } from "posthog-js/react" +import type { PostHog } from "posthog-js" +import { PostHogEvents } from "@/common/constants" jest.mock("@/common/analytics/gtm", () => ({ trackCheckoutCompleted: jest.fn(), })) +jest.mock("posthog-js/react") +const mockedPostHogCapture = jest.fn() +jest.mocked(usePostHog).mockReturnValue({ + capture: mockedPostHogCapture, +} as unknown as PostHog) const escapeRegExp = (s: string) => s.replace(/[.*+?^${}()|[\]\\]/g, "\\$&") describe("EnrollmentRedirectAlert", () => { beforeEach(() => { jest.clearAllMocks() + process.env.NEXT_PUBLIC_POSTHOG_API_KEY = "test-key" + }) + + afterEach(() => { + delete process.env.NEXT_PUBLIC_POSTHOG_API_KEY }) test("shows invalid-enrollment-code error alert and clears params", async () => { @@ -256,7 +269,11 @@ describe("EnrollmentRedirectAlert", () => { test("tracks checkout-completed once with order id, course name, and value from receipt", async () => { const receipt = mitxonline.factories.orders.order({ - lines: [mitxonline.factories.orders.transactionLine()], + lines: [ + mitxonline.factories.orders.transactionLine({ + content_type: "courserun", + }), + ], total_price_paid: "199.99", }) @@ -274,6 +291,17 @@ describe("EnrollmentRedirectAlert", () => { courseName: receipt.lines[0].content_title, value: 199.99, }) + expect(mockedPostHogCapture).toHaveBeenCalledTimes(1) + expect(mockedPostHogCapture).toHaveBeenCalledWith( + PostHogEvents.CheckoutCompleted, + { + orderId: 17, + courseName: receipt.lines[0].content_title, + value: 199.99, + readableId: receipt.lines[0].readable_id, + contentType: "courserun", + }, + ) }) test("tracks checkout-completed with a null value when the receipt fails to load", async () => { @@ -293,6 +321,33 @@ describe("EnrollmentRedirectAlert", () => { courseName: undefined, value: null, }) + expect(mockedPostHogCapture).toHaveBeenCalledWith( + PostHogEvents.CheckoutCompleted, + { + orderId: 18, + courseName: undefined, + value: null, + readableId: undefined, + contentType: undefined, + }, + ) + }) + + test("does not capture PostHog checkout_completed when PostHog is not configured", async () => { + delete process.env.NEXT_PUBLIC_POSTHOG_API_KEY + setMockResponse.get( + mitxonline.urls.orders.receipt(19), + mitxonline.factories.orders.order(), + ) + + renderWithProviders(, { + url: "/dashboard?order_status=fulfilled&order_id=19", + }) + + await screen.findByRole("alert") + + expect(trackCheckoutCompleted).toHaveBeenCalledTimes(1) + expect(mockedPostHogCapture).not.toHaveBeenCalled() }) test("does not track checkout-completed for non-paid alerts", async () => { @@ -303,6 +358,7 @@ describe("EnrollmentRedirectAlert", () => { await screen.findByRole("alert") expect(trackCheckoutCompleted).not.toHaveBeenCalled() + expect(mockedPostHogCapture).not.toHaveBeenCalled() }) test.each([ diff --git a/frontends/main/src/app-pages/DashboardPage/EnrollmentRedirectAlert.tsx b/frontends/main/src/app-pages/DashboardPage/EnrollmentRedirectAlert.tsx index d76b55db3c..ec193347b5 100644 --- a/frontends/main/src/app-pages/DashboardPage/EnrollmentRedirectAlert.tsx +++ b/frontends/main/src/app-pages/DashboardPage/EnrollmentRedirectAlert.tsx @@ -3,12 +3,14 @@ import { env } from "@/env" import React from "react" import { useQuery } from "@tanstack/react-query" +import { usePostHog } from "posthog-js/react" import { Alert } from "@mitodl/smoot-design" import { Link, Skeleton, styled } from "ol-components" import { orderQueries } from "api/mitxonline-hooks/orders" import { mitxUserQueries } from "api/mitxonline-hooks/user" import { DASHBOARD_MY_LEARNING } from "@/common/urls" import { trackCheckoutCompleted } from "@/common/analytics/gtm" +import { PostHogEvents } from "@/common/constants" import { ENROLLMENT_STATUS_PARAM, ENROLLMENT_ERROR_TYPE_PARAM, @@ -174,6 +176,7 @@ const parseAlertRequest = ( const EnrollmentRedirectAlert: React.FC = () => { const request = useConsumeSearchParamsOnce(parseAlertRequest) const supportEmail = env("NEXT_PUBLIC_MITOL_SUPPORT_EMAIL") || "" + const posthog = usePostHog() const mitxOnlineUserQuery = useQuery({ ...mitxUserQueries.me(), @@ -195,12 +198,25 @@ const EnrollmentRedirectAlert: React.FC = () => { ? Number(paidReceipt.data.total_price_paid) : NaN + const courseName = paidReceipt.data?.lines[0]?.content_title + const value = Number.isNaN(parsedValue) ? null : parsedValue + trackCheckoutCompleted({ orderId: request.orderId, - courseName: paidReceipt.data?.lines[0]?.content_title, - value: Number.isNaN(parsedValue) ? null : parsedValue, + courseName, + value, }) - }, [request, paidReceipt.isPending, paidReceipt.data]) + if (env("NEXT_PUBLIC_POSTHOG_API_KEY")) { + const line = paidReceipt.data?.lines[0] + posthog.capture(PostHogEvents.CheckoutCompleted, { + orderId: request.orderId, + courseName, + value, + readableId: line?.readable_id, + contentType: line?.content_type, + }) + } + }, [request, paidReceipt.isPending, paidReceipt.data, posthog]) if (request?.kind === "error") { const errorMessage = diff --git a/frontends/main/src/common/constants.ts b/frontends/main/src/common/constants.ts index 82ed51bc28..641a2f6dc2 100644 --- a/frontends/main/src/common/constants.ts +++ b/frontends/main/src/common/constants.ts @@ -36,6 +36,7 @@ export const PostHogEvents = { OrgLearningCtaClicked: "org_learning_cta_clicked", OrgLearningAudienceSelected: "org_learning_audience_selected", OrgLearningFormSubmitted: "org_learning_form_submitted", + CheckoutCompleted: "checkout_completed", } as const export const DigitalCredentialsFAQLink = From 52db1c4c93a5a21b721ade58d7ab00ab91b55932 Mon Sep 17 00:00:00 2001 From: Ahtesham Quraish Date: Mon, 28 Sep 2026 11:22:03 +0500 Subject: [PATCH 7/9] feat(website-content): save drafts automatically, drop the draft button (#3978) * feat(website-content): unpublish published items from the listing card Adds the three-dot menu from the design to published article and news listing cards, with Unpublish as its single action, and drops the Unpublish button from the content detail toolbar. The menu renders only for users who can edit content, so no callsite can leak it. Draft cards link to the editor, which is the only page they have. Unpublishing now takes effect within the request: the news feed entry and the article's LearningResource -- published flag and Qdrant points -- go before the response, so the listing's refetch and vector search stop serving what was just taken down. The view cache is cleared after them, so nothing can re-cache the stale listing, and vector search hydrates only published resources. Co-Authored-By: Claude Opus 5 (1M context) * fix(website-content): reconcile a sync that overtakes an unpublish Both sync tasks check `is_published` with a read and then write, and unpublishing now runs in the request, so it can land between the two with nothing queued behind it to notice: the sync would restore a published, indexed resource -- and the news feed entry -- for content that is no longer public. Whoever writes last reconciles, so each sync task re-reads the row after writing and undoes itself if the item has since been unpublished. Co-Authored-By: Claude Opus 5 (1M context) * fix(vector-search): check publication on the payload path too The published-row filter only covered database hydration, which the kill switch turns on; the default path answers from the Qdrant payloads, where a delete that is late or has failed outright kept serving an unpublished article. A payload cannot report this itself -- unpublishing deletes the point rather than rewriting it, so a stale payload still reads as published and no Qdrant-side filter can catch it. The payload path now costs one indexed lookup, asked negatively: only rows the database reports as unpublished are dropped, so a point with no row here -- a snapshot loaded from another system -- is still returned. Covered at the view level on the shipped default, not just in the hit builder. Co-Authored-By: Claude Opus 5 (1M context) * test(api): assert the patch hook invalidates the listings The page tests only assert that the PATCH was sent, so dropping the list invalidation still passed while a card unpublished from the listing stayed in the cache -- and on screen. The hook's own test now pins all three keys it invalidates, the detail retrieve one having been uncovered in the same way. Co-Authored-By: Claude Opus 5 (1M context) * feat(website-content): drop the topics section for news settings Only an article is projected into a LearningResource, so only there do topics put the content on a topic page. The news drawer keeps its SEO section and no longer fetches the topic list at all. The drawer stays presentational -- the caller decides with `showTopics` -- and reports no topics rather than an empty selection when the section is hidden, so saving news settings cannot empty a selection the editor was never shown, or re-run the publish plugins for nothing. Co-Authored-By: Claude Opus 5 (1M context) * feat(website-content): require topics to save an article An article's topics are what put it on a topic page, so saving one without them now opens the settings drawer instead -- for a draft save as well as a publish -- and the section says why it opened. News has no topics section at all, so nothing is required of it. The held-back press resumes once a topic is picked, carrying the new selection into the content write rather than PATCHing topics separately, and a publish still confirms as it would have. Closing the drawer abandons it, so a press the editor walked away from cannot fire the next time topics happen to be saved. Co-Authored-By: Claude Opus 5 (1M context) * test(website-content): pin that the unpublish hooks run in autocommit The hooks call out to Qdrant synchronously and swallow a DatabaseError to fall back to a queued task. Both are safe only outside a transaction: inside one, the call would hold it open across network I/O, and the swallowed error would poison it -- the fallback would never be queued and the next query would raise TransactionManagementError. Nothing asks for a transaction today (no ATOMIC_REQUESTS, no atomic in the write path), which two review passes have now assumed otherwise, so assert it rather than leave it to be re-derived. Verified to fail when ATOMIC_REQUESTS is switched on. Co-Authored-By: Claude Opus 5 (1M context) * revert(vector-search): drop the read-path publication filter Filtering hits against the database broke the counts beside them: on the no-query path `total` comes from Qdrant's own exact count() and the facets from facet(), neither of which knows about a Postgres filter, so totals and doc_counts over-reported and the last page came back short -- the very thing exact=True was set to prevent. Reconciling that properly means post-filtering the whole matched set in Python. The guard was partial regardless: content file hits were never checked, and unpublishing leaves content files in Qdrant by design. The removal path converges on its own -- inline delete, the retrying queued task behind it, and the sync task reconciling against the row -- so trust it. Also drops the points via remove_points_matching_params, keyed on the readable_id already in hand, rather than remove_embeddings, which re-serializes the resource only to derive the same filter. Co-Authored-By: Claude Opus 5 (1M context) * fix(website-content): re-attempt a failed reconciliation Both sync tasks undo themselves when the row turns out to be unpublished by the time they have written, and neither undo was retried on failure. In the news task the undo raises inside the task's own try, so the whole task retries -- and the retry took the "not published" path, which returned without touching the entry the previous attempt had created. That path now removes any entry instead of skipping, which is what makes the retry effective; deleting by guid is a no-op when there is nothing there, so the ordinary "queued, then unpublished" case is unchanged. The learning resource task does not retry at all, so a failed undo left the resource published and indexed with nothing behind it. It now hands the undo to the task that does retry, whose republish guard makes a late run safe. Co-Authored-By: Claude Opus 5 (1M context) * fix(website-content): do not fail an unpublish over the index work The inline removal caught only DatabaseError, but it runs the search and vector hooks inline, which fail in other ways -- an unreachable broker escapes try_with_retry_as_task, whose own fallback is an unguarded .delay(). That reached the client as a 500 after both rows had already committed as unpublished, and a retried request fires no hooks at all, since perform_update keys them off the published->unpublished transition. The deindex and the Qdrant removal were simply lost. Any failure now hands off to the retrying task, which redoes the removal in full. Queueing can fail too, for the same broker reason, so that is logged and the indexes are left to the next reindex rather than failing an unpublish whose own rows are already correct. Co-Authored-By: Claude Opus 5 (1M context) * feat(website-content): save drafts automatically, drop the draft button A draft writes itself a couple of seconds after typing stops, and the control bar says so -- "Saving..." while the write is in flight, then "Saved" until the next edit, as the design has it. The Save as Draft button is gone with it; a draft that has never been saved comes into existence the same way. Published content is untouched: every save there pushes edits live, so it stays an explicit press of Publish. Topics are consequently required to publish rather than to save at all. Autosave cannot stop to ask, and a drawer opening itself on a timer while someone types is not a prompt -- it is an interruption. Co-Authored-By: Claude Opus 5 (1M context) * feat(website-content): lay the control bar out as the design has it The status leads the row and the actions follow at the other end, with the settings control reduced to its icon and paired with Publish. Nothing labels the icon on screen any more, so it carries an aria-label and a title -- the existing "Settings" queries keep working through the former. Shared with the published view's bar, so the control looks the same wherever it appears. Co-Authored-By: Claude Opus 5 (1M context) * fix(website-content): align the action row with the article's column The row spanned the toolbar's full width, so the status sat against the far left and the actions against the far right, well outside the text they belong to. It now takes the article's own column -- the same 890px centred, 24px-padded column the banner and the body already use -- so the status lines up with the breadcrumb and title, and the actions with the far edge of the text. Co-Authored-By: Claude Opus 5 (1M context) * fix(website-content): register the save indicator before it speaks `role="status"` only announces reliably if the region was already in the page when its text changed; mounted in the same paint as its first message, that message is routinely dropped -- and it is the one that matters. The region is now mounted empty for the session and only its text changes. The spinner beside it is hidden from the region too: the text says the same thing, and its own "Loading" label would be read out as well. Also carries the edit-bar changes made alongside this -- a shorter status readout, a Publish button that no longer names the content type, and no Delete in the bar -- with the code they left dead removed and the tests brought in line. Deleting a draft remains on the drafts listing, which covers it. Co-Authored-By: Claude Opus 5 (1M context) * style(website-content): put the settings icon in a button box `bordered` rather than `text`, so it reads as a button alongside Publish and matches its height, with the icon still its only content. Co-Authored-By: Claude Opus 5 (1M context) * refactor(website-content): a flag, since only a publish is held back `pendingSave` was a tri-state remembering whether the held-back save was a publish, from when a draft save could be held back too. Since the draft button went, nothing set it to false, so the draft branch in `handleSettingsSave` was unreachable -- and `saveQuietly`, whose only caller it was, was dead with it. Now `awaitingTopicsForPublish`, a boolean that says what it is. Co-Authored-By: Claude Opus 5 (1M context) * fix(website-content): close the drawer's way past the topics rule Gating the save buttons left the drawer itself open: it is the one place a selection can be taken away, so removing every topic and saving straight from there PATCHed `topics: []` -- an article, published one included, left without the topics that put it on a topic page. Saving is now refused while a required selection is empty, which the section already explains, rather than silently ignoring the removal the editor can see on screen. Co-Authored-By: Claude Opus 5 (1M context) * fix(website-content): refuse an empty topic selection only once published The drawer's gate arrived with the stricter rule it was written for, where a draft could not be saved without topics either. Autosave changed that: a draft writes itself and cannot stop to ask, so it may sit without topics and is stopped at publishing instead. Left as it was, an editor could not remove a topic from a draft at all. So the two rules are now separate props. `topicsRequired` still says topics are wanted before publishing, which is what tells an editor why the drawer opened on them; `topicsMayNotBeEmptied` refuses the save, and the editor passes it only for content that is already public. The copy follows whichever rule is speaking rather than claiming a draft cannot be saved. Co-Authored-By: Claude Opus 5 (1M context) * fix(website-content): catch any failed undo, not just a database one The reconciliation caught `DatabaseError` alone, but the undo it guards runs the search and vector hooks inline, which fail in other ways -- an unreachable broker escapes `try_with_retry_as_task`, whose own fallback is an unguarded `.delay()`. This task does not retry, so that raised straight out, leaving the resource unpublished in the database and still in the index with nothing queued to remove it. Any failure now hands the undo to the retrying task, and queueing is itself guarded, since the broker is what may have failed. The same shape as the plugin's inline removal. Co-Authored-By: Claude Opus 5 (1M context) * fix(website-content): stop autosave re-navigating and re-creating Every autosave calls `onSave`, and the edit page answered it by pushing the route it is already on -- pointless work every couple of seconds, since the page reads its item through React Query, which the mutation already invalidates. The push is now guarded on the pathname, which also drops the same redundant push the draft button used to make. No progress bar was flickering, though: `next-nprogress-bar` compares the target with the current URL and suppresses the bar for a same-URL push. It still pushes, which is what this saves. Separately, an item created by autosave could be created twice. The caller moves the editor to the new item's URL only once the create has returned, so a second write before the route changed would insert another row; the editor now remembers what it created and updates that. Co-Authored-By: Claude Opus 5 (1M context) * fix(website-content): tell the caller only when the item has moved `onSave` exists to move the editor to where the saved item now lives, which is somewhere new on two transitions only: the save that created the item and the one that published it. Fired on every autosave, it asked the caller to navigate to the route it was already on, for as long as anyone kept typing. The edit page's pathname guard stayed as it is -- it also covers a URL that names the item by slug -- but the churn is now stopped at the source rather than at one caller of many. Co-Authored-By: Claude Opus 5 (1M context) * fix(website-content): serialize the writes, and fix the review's findings The reviewed items, in order: Drop the MUI `Container` wrapper. It is not the 890px column its comment claimed -- that is `TiptapEditor`'s own local `Container` -- but MUI's, which defaults to `maxWidth="lg"` and adds gutters of its own on top of the column `ActionRow` already implements. Serialize every write through one queue. A publish confirmed while a debounced draft save was still running raced it, and whichever landed last decided whether the item ended up public, whatever the dialog reported. A publish from this editor now also stops further draft writes: `contentItem` still says draft, so one queued behind it would take the item straight back down. The publish spinner stops on failure too, rather than staying on for the next autosave. Hold a created item until nothing is unsaved before handing it to the caller, which navigates and so unmounts the editor along with anything typed while the create was in flight. Stop the bar claiming "Saved" through the debounce after the next keystroke: it is read together with whether anything has changed since. Give the removal task a retry policy. The callers that hand off to it describe it as the one carrying the retries, and it had none. Not changed: the report of a failing save being retried every two seconds. An instrumented probe -- slow failing response, so the pending transition is observable -- fires the save exactly once and never re-arms, so there is nothing to guard. Co-Authored-By: Claude Opus 5 (1M context) * fix(website-content): a publish supersedes a pending handoff A created item is held until nothing is unsaved before being handed to the caller, which navigates. Confirming a publish while the create was still in flight left that handoff waiting on `isPending`, and it fired once the publish settled -- handing over the create response, which says `is_published: false`, so the caller sent the editor to the draft page for an item that was now public. The publish now clears it, and the effect refuses to hand anything over after a publish from this editor. Two of the tests here typed into a heading node captured before a re-render, so the keystrokes went nowhere and the save they waited for never came. That was the whole of the flakiness in these suites: with the node re-queried, seven consecutive runs are clean. Co-Authored-By: Claude Opus 5 (1M context) * fix(website-content): narrow the request body the test reads `RequestArgs.body` is `unknown` in the request harness, so reading `is_published` off it is an error. CI caught it and I had not: the repo-level typecheck stops at the first workspace that fails, and this checkout's `api` workspace fails on its own before `main` is reached -- its @mitodl/mitxonline-api-axios is older than the code expects. Co-Authored-By: Claude Opus 5 (1M context) * test(website-content): keep autosave out of the tests it is not about Autosave gave every typing test a two-second timer that fired mid interaction, re-rendering the toolbar and warning about updates outside `act` -- which the suites fail on. It surfaced as a different test each run, on CI as the edit page's drawer tests. The editors and the edit page now take the delay as a prop, defaulting to what production uses. The suites set it far out so nothing saves while they work, and the tests that are about autosave set it back. The create test no longer types while the create is in flight, which is where those gaps were widest; it types once the first save has settled, which exercises the same thing -- a second save updating what was created rather than inserting another row. Co-Authored-By: Claude Opus 5 (1M context) * fix(website-content): queue the settings write with the others Autosave went through the queue; the settings write did not. A content write already in flight carries the topics as they were when it started, so the two overlapped and the older list could be what the server stored last -- taking the selection the editor had just made back out, with nothing on screen to say so. It is queued now, like the publish. The test for it kept the indicator in its saving state long enough to surface a second defect: the indicator was a `Typography`, so a `

`, and the spinner it holds renders a div. It is its own span now, taking the typography from the theme, since `styled()` drops the polymorphic `component` prop that would have changed the tag. Also awaits the news suite's heading query. Under load the editor's content mounts after its container, and the synchronous query missed it about one run in three. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Ahtesham Quraish Co-authored-by: Claude Opus 5 (1M context) --- .../WebsiteContentEditPage.happydom.test.tsx | 70 ++- .../WebsiteContent/WebsiteContentEditPage.tsx | 29 +- .../ArticleSettings/ArticleSettingsDrawer.tsx | 52 +- .../article/ArticleEditor.happydom.test.tsx | 502 ++++++++++++++++-- .../contentTypes/article/ArticleEditor.tsx | 10 +- .../news/NewsEditor.happydom.test.tsx | 173 +++--- .../contentTypes/news/NewsEditor.tsx | 10 +- .../core/WebsiteContentEditor.tsx | 418 +++++++++++---- learning_resources/tasks.py | 11 +- learning_resources/tasks_test.py | 12 + 10 files changed, 1062 insertions(+), 225 deletions(-) diff --git a/frontends/main/src/app-pages/WebsiteContent/WebsiteContentEditPage.happydom.test.tsx b/frontends/main/src/app-pages/WebsiteContent/WebsiteContentEditPage.happydom.test.tsx index 80c4a8c0ff..ee2d814956 100644 --- a/frontends/main/src/app-pages/WebsiteContent/WebsiteContentEditPage.happydom.test.tsx +++ b/frontends/main/src/app-pages/WebsiteContent/WebsiteContentEditPage.happydom.test.tsx @@ -10,6 +10,7 @@ import userEvent from "@testing-library/user-event" import { setMockResponse, factories, urls, makeRequest } from "api/test-utils" import type { JSONContent } from "@tiptap/react" import { WebsiteContentEditPage } from "./WebsiteContentEditPage" +import { websiteContentEditView } from "@/common/urls" import { renderWithProviders } from "@/test-utils" /** @@ -25,6 +26,37 @@ import { renderWithProviders } from "@/test-utils" */ jest.setTimeout(20000) +/** + * The page navigates through `next-nprogress-bar`'s wrapper rather than + * `next/navigation` directly, so this is where a push can be observed -- + * spying on the memory router does not see it. The wrapper's own same-URL + * check only suppresses the progress bar; it still pushes. + * + * Referenced lazily so the factory, which jest hoists above these imports, + * does not read `routerMocks` before it exists. + */ +jest.mock("next-nprogress-bar", () => ({ + useRouter: () => ({ + push: (...args: unknown[]) => routerMocks.push(...args), + }), +})) + +const routerMocks = { push: jest.fn() } + +beforeEach(() => { + routerMocks.push.mockClear() +}) + +/** + * No draft saves itself while these tests work: this suite is about the + * drawer and the refetch, and a background write landing mid-interaction + * updates the toolbar outside `act`. + */ +const AUTOSAVE_OFF = 10 * 60 * 1000 + +/** What production uses; the navigation test waits this out deliberately. */ +const AUTOSAVE_DELAY_MS = 2000 + const SERVER_TEXT = "Paragraph as the server has it" const content: JSONContent = { @@ -56,7 +88,7 @@ const detailFetchCount = (id: number) => String(call[0]?.url).includes(`/website_content/detail/${id}/`), ).length -const setup = async (id: number) => { +const setup = async (id: number, autosaveDelayMs = AUTOSAVE_OFF) => { const user = factories.user.user({ is_authenticated: true, is_article_editor: true, @@ -76,8 +108,12 @@ const setup = async (id: number) => { setMockResponse.get(urls.topics.list({ limit: 1000 }), topics) renderWithProviders( - , - { user }, + , + { user, url: websiteContentEditView("article", id) }, ) await screen.findByTestId("editor") return { article, topic: topics.results[0] } @@ -126,6 +162,34 @@ const saveTopicInDrawer = async ( * select-all that would clear the node first -- so these match on a substring * of the resulting text rather than the whole of it. */ +describe("WebsiteContentEditPage navigation", () => { + test("a draft save does not re-navigate to the page it is already on", async () => { + const { article } = await setup(4242, AUTOSAVE_DELAY_MS) + setMockResponse.patch(urls.websiteContent.details(article.id), article) + + /** + * A draft writes itself every couple of seconds, and this page reads its + * item through React Query -- which the mutation already invalidates. So + * pushing the route we are on buys nothing and costs a soft navigation + * and a run of the progress bar each time. + */ + await userEvent.type( + await screen.findByRole("heading", { level: 1 }), + " edited", + ) + await waitFor( + () => { + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ method: "patch" }), + ) + }, + { timeout: 6000 }, + ) + + expect(routerMocks.push).not.toHaveBeenCalled() + }) +}) + describe("WebsiteContentEditPage settings drawer", () => { test("saving the drawer keeps unsaved body edits", async () => { const { article, topic } = await setup(601) diff --git a/frontends/main/src/app-pages/WebsiteContent/WebsiteContentEditPage.tsx b/frontends/main/src/app-pages/WebsiteContent/WebsiteContentEditPage.tsx index 4b622b68f7..a1a5e299c9 100644 --- a/frontends/main/src/app-pages/WebsiteContent/WebsiteContentEditPage.tsx +++ b/frontends/main/src/app-pages/WebsiteContent/WebsiteContentEditPage.tsx @@ -2,7 +2,7 @@ import React from "react" import { useRouter } from "next-nprogress-bar" -import { notFound } from "next/navigation" +import { notFound, usePathname } from "next/navigation" import { Permission } from "api/hooks/user" import { useWebsiteContentDetailRetrieve } from "api/hooks/website_content" import RestrictedRoute from "@/components/RestrictedRoute/RestrictedRoute" @@ -38,6 +38,7 @@ const EDITORS: Record< onSave?: (savedContent: WebsiteContent) => void readOnly?: boolean contentItem?: WebsiteContent + autosaveDelayMs?: number }> > = { article: ({ contentItem, ...props }) => ( @@ -51,13 +52,21 @@ const EDITORS: Record< interface WebsiteContentEditPageProps { type: string idOrSlug: string + /** + * Passed straight to the editor; only tests set it, to keep a background + * draft save from landing in the middle of their interactions. See + * `WebsiteContentEditor`. + */ + autosaveDelayMs?: number } const WebsiteContentEditPage = ({ type, idOrSlug, + autosaveDelayMs, }: WebsiteContentEditPageProps) => { const { data: article, isLoading } = useWebsiteContentDetailRetrieve(idOrSlug) + const pathname = usePathname() const router = useRouter() const Editor = EDITORS[type] @@ -88,12 +97,26 @@ const WebsiteContentEditPage = ({ { if (saved.is_published) { invariant(saved.slug, "Published content must have a slug") return router.push(viewUrl(saved.slug)) - } else { - router.push(websiteContentEditView(type, saved.id)) + } + /** + * Where a draft lives, which is usually where we already are -- + * the exception being a URL that names the item by slug, which + * this canonicalises to the id once. + * + * Guarded because a draft saves itself every couple of seconds: + * pushing the route we are on buys nothing (this page reads its + * item through React Query, which the mutation already + * invalidates) and costs a soft navigation and a run of the + * progress bar each time. + */ + const draftUrl = websiteContentEditView(type, saved.id) + if (draftUrl !== pathname) { + router.push(draftUrl) } }} /> diff --git a/frontends/main/src/page-components/ArticleSettings/ArticleSettingsDrawer.tsx b/frontends/main/src/page-components/ArticleSettings/ArticleSettingsDrawer.tsx index de9154832a..0f095519f1 100644 --- a/frontends/main/src/page-components/ArticleSettings/ArticleSettingsDrawer.tsx +++ b/frontends/main/src/page-components/ArticleSettings/ArticleSettingsDrawer.tsx @@ -163,6 +163,27 @@ const FooterCta = styled.div({ marginTop: "auto", }) +/** + * What the topics section says about itself, which depends on which rule is + * speaking: one that refuses to save without a topic, one that only wants them + * before publishing, or neither. + */ +const topicsMessage = ( + contentLabel: string, + empty: boolean, + required: boolean, + mayNotBeEmptied: boolean, +) => { + const noun = contentLabel.toLowerCase() + if (empty && mayNotBeEmptied) { + return `A published ${noun} needs at least one topic` + } + if (empty && required) { + return `Select at least one topic to publish your ${noun}` + } + return `Select one or more topics for your ${noun}` +} + /** Settings the drawer collects. Mirrors the fields in the design. */ export interface ArticleSettingsValues { /** @@ -206,14 +227,21 @@ export interface ArticleSettingsDrawerProps { */ showTopics?: boolean /** - * Whether the content cannot be saved without a topic. - * - * The section says so while none is picked, and saving is refused until one - * is: this drawer is the one place a selection can be taken away, so a - * caller that gates its own save buttons would otherwise still lose the - * topics through here. + * Whether the content needs a topic before it can go public. The section + * says so while none is picked, which is what tells an editor why the + * drawer opened on them when they pressed Publish. */ topicsRequired?: boolean + /** + * Whether an empty selection may not be saved at all. + * + * This drawer is the one place a selection can be taken away, so a caller + * that gates only its own save buttons would still lose the topics through + * here. Separate from `topicsRequired` because the two do not coincide: + * content that is not public yet can be left without topics -- it is + * stopped at publishing -- while content already public cannot. + */ + topicsMayNotBeEmptied?: boolean /** Values to open with. Re-read each time the drawer opens. */ initialValues?: Partial /** @@ -232,6 +260,7 @@ const ArticleSettingsDrawer = ({ contentLabel = "Article", showTopics = true, topicsRequired = false, + topicsMayNotBeEmptied = false, initialValues, onSave, }: ArticleSettingsDrawerProps) => { @@ -423,9 +452,12 @@ const ArticleSettingsDrawer = ({ Select Topics - {topicsRequired && selectedIds.length === 0 - ? `Select at least one topic to save your ${contentLabel.toLowerCase()}` - : `Select one or more topics for your ${contentLabel.toLowerCase()}`} + {topicsMessage( + contentLabel, + selectedIds.length === 0, + topicsRequired, + topicsMayNotBeEmptied, + )} @@ -543,7 +575,7 @@ const ArticleSettingsDrawer = ({ /* Refused rather than silently ignored: the editor has emptied the selection on screen and has to see why it will not save. */ disabled={ - topicsRequired && showTopics && selectedIds.length === 0 + topicsMayNotBeEmptied && showTopics && selectedIds.length === 0 } onClick={() => { onSave?.({ diff --git a/frontends/main/src/page-components/TiptapEditor/contentTypes/article/ArticleEditor.happydom.test.tsx b/frontends/main/src/page-components/TiptapEditor/contentTypes/article/ArticleEditor.happydom.test.tsx index 9657a41ede..ca7d379503 100644 --- a/frontends/main/src/page-components/TiptapEditor/contentTypes/article/ArticleEditor.happydom.test.tsx +++ b/frontends/main/src/page-components/TiptapEditor/contentTypes/article/ArticleEditor.happydom.test.tsx @@ -12,6 +12,17 @@ import type { JSONContent } from "@tiptap/react" import { ArticleEditor } from "./ArticleEditor" import { renderWithProviders } from "@/test-utils" +/** + * Far enough out that no draft saves itself while a test works. A background + * write landing mid-interaction re-renders the toolbar, and the update lands + * outside `act` -- which failed these suites intermittently, a different test + * each time. The tests that are about autosave set their own delay. + */ +const AUTOSAVE_OFF = 10 * 60 * 1000 + +/** What production uses; the autosave tests wait this out deliberately. */ +const AUTOSAVE_DELAY_MS = 2000 + const content: JSONContent = { type: "doc", content: [ @@ -35,10 +46,12 @@ const renderArticleEditor = ({ readOnly = false, isPublished = false, topics = [], + autosaveDelayMs = AUTOSAVE_OFF, }: { readOnly?: boolean isPublished?: boolean topics?: number[] + autosaveDelayMs?: number } = {}) => { const user = factories.user.user({ is_authenticated: true, @@ -50,9 +63,14 @@ const renderArticleEditor = ({ is_published: isPublished, topics, }) - renderWithProviders(, { - user, - }) + renderWithProviders( + , + { user }, + ) return { article } } @@ -76,11 +94,12 @@ describe("ArticleEditor article controls", () => { renderArticleEditor() await screen.findByRole("button", { name: "Settings" }) - await screen.findByRole("button", { name: "Save as Draft" }) - await screen.findByRole("button", { name: "Publish Article" }) - expect(await screen.findByText(/Article status:/)).toHaveTextContent( - "Article status: Draft", + await screen.findByRole("button", { name: "Publish" }) + expect(await screen.findByText(/Status:/)).toHaveTextContent( + "Status: Draft", ) + /* A draft writes itself now, so there is nothing to press. */ + expect(screen.queryByRole("button", { name: "Save as Draft" })).toBe(null) }) test("a published article offers Draft, Edit and Settings", async () => { @@ -89,8 +108,12 @@ describe("ArticleEditor article controls", () => { await screen.findByRole("link", { name: "Draft" }) await screen.findByRole("link", { name: "Edit" }) await screen.findByRole("button", { name: "Settings" }) - expect(await screen.findByText(/Article status:/)).toHaveTextContent( - "Article status: Published", + expect(await screen.findByText(/Status:/)).toHaveTextContent( + "Status: Published", + ) + /* Unpublishing lives on the listing card's menu, not here. */ + expect(screen.queryByRole("button", { name: "Unpublish Article" })).toBe( + null, ) /* Unpublishing lives on the listing card's menu, not here. */ expect(screen.queryByRole("button", { name: "Unpublish Article" })).toBe( @@ -272,7 +295,7 @@ describe("ArticleEditor publish confirmation", () => { }) await userEvent.click( - await screen.findByRole("button", { name: "Publish Article" }), + await screen.findByRole("button", { name: "Publish" }), ) // Nothing is saved until the dialog is confirmed. @@ -302,7 +325,7 @@ describe("ArticleEditor publish confirmation", () => { renderTopicalArticle() await userEvent.click( - await screen.findByRole("button", { name: "Publish Article" }), + await screen.findByRole("button", { name: "Publish" }), ) await screen.findByRole("heading", { name: "Publish article" }) await userEvent.click(screen.getByRole("button", { name: "Cancel" })) @@ -323,7 +346,7 @@ describe("ArticleEditor publish confirmation", () => { await userEvent.type(heading, " edited") await userEvent.click( - await screen.findByRole("button", { name: "Publish Article" }), + await screen.findByRole("button", { name: "Publish" }), ) expect( @@ -348,7 +371,7 @@ describe("ArticleEditor publish confirmation errors", () => { ) await userEvent.click( - await screen.findByRole("button", { name: "Publish Article" }), + await screen.findByRole("button", { name: "Publish" }), ) await screen.findByRole("heading", { name: "Publish article" }) await userEvent.click( @@ -384,7 +407,7 @@ describe("ArticleEditor publish confirmation errors", () => { }) await userEvent.click( - await screen.findByRole("button", { name: "Publish Article" }), + await screen.findByRole("button", { name: "Publish" }), ) await screen.findByRole("heading", { name: "Publish article" }) await userEvent.click( @@ -411,7 +434,7 @@ describe("ArticleEditor topics requirement", () => { renderArticleEditor() await userEvent.click( - await screen.findByRole("button", { name: "Publish Article" }), + await screen.findByRole("button", { name: "Publish" }), ) // The drawer, not the publish confirmation, and nothing saved. @@ -423,24 +446,34 @@ describe("ArticleEditor topics requirement", () => { expect.objectContaining({ method: "patch" }), ) /* The section says why it opened, rather than leaving the editor to guess. */ - await screen.findByText("Select at least one topic to save your article") + await screen.findByText("Select at least one topic to publish your article") }) - test("saving a draft with no topics asks for them instead", async () => { + test("a draft saves itself without them, rather than asking", async () => { mockTopics() - renderArticleEditor() + const { article } = renderArticleEditor({ + autosaveDelayMs: AUTOSAVE_DELAY_MS, + }) + setMockResponse.patch(urls.websiteContent.details(article.id), article) - // "Save as Draft" only enables once the document is touched. await userEvent.type(await screen.findByRole("heading", { level: 1 }), "!") - await userEvent.click( - await screen.findByRole("button", { name: "Save as Draft" }), - ) - await screen.findByRole("heading", { name: "Article Settings" }) - expect(makeRequest).not.toHaveBeenCalledWith( - expect.objectContaining({ method: "patch" }), + // Autosave cannot stop to ask, so the requirement is the publish's alone. + await waitFor( + () => { + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ + method: "patch", + body: expect.objectContaining({ is_published: false }), + }), + ) + }, + { timeout: 6000 }, ) - }) + expect(screen.queryByRole("heading", { name: "Article Settings" })).toBe( + null, + ) + }, 15000) test("the held-back publish resumes once a topic is picked", async () => { const topic = mockTopics() @@ -451,7 +484,7 @@ describe("ArticleEditor topics requirement", () => { }) await userEvent.click( - await screen.findByRole("button", { name: "Publish Article" }), + await screen.findByRole("button", { name: "Publish" }), ) await screen.findByRole("heading", { name: "Article Settings" }) @@ -481,7 +514,35 @@ describe("ArticleEditor topics requirement", () => { }) }) - test("the drawer will not save an article with its topics emptied", async () => { + test("a draft's topics can be cleared", async () => { + const topics = factories.learningResources.topics({ count: 1 }) + const [topic] = topics.results + setMockResponse.get(urls.topics.list({ limit: 1000 }), topics) + const { article } = renderArticleEditor({ topics: [topic.id] }) + setMockResponse.patch(urls.websiteContent.details(article.id), article) + + await userEvent.click( + await screen.findByRole("button", { name: "Settings" }), + ) + await userEvent.click( + await screen.findByRole("button", { name: `Remove ${topic.name}` }), + ) + + /** + * Refused only once the content is public. A draft may sit without topics + * -- publishing is where they are insisted on, and autosave cannot stop to + * ask -- so the editor is not trapped into keeping a topic they removed. + */ + await userEvent.click(screen.getByRole("button", { name: "Save Settings" })) + + await waitFor(() => { + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ method: "patch", body: { topics: [] } }), + ) + }) + }) + + test("the drawer will not save a published article with its topics emptied", async () => { const topics = factories.learningResources.topics({ count: 1 }) const [topic] = topics.results setMockResponse.get(urls.topics.list({ limit: 1000 }), topics) @@ -504,7 +565,7 @@ describe("ArticleEditor topics requirement", () => { * through here, and with them its place on a topic page. */ expect(screen.getByRole("button", { name: "Save Settings" })).toBeDisabled() - await screen.findByText("Select at least one topic to save your article") + await screen.findByText("A published article needs at least one topic") expect(makeRequest).not.toHaveBeenCalledWith( expect.objectContaining({ method: "patch" }), ) @@ -516,7 +577,7 @@ describe("ArticleEditor topics requirement", () => { setMockResponse.patch(urls.websiteContent.details(article.id), article) await userEvent.click( - await screen.findByRole("button", { name: "Publish Article" }), + await screen.findByRole("button", { name: "Publish" }), ) await screen.findByRole("heading", { name: "Article Settings" }) await userEvent.click(screen.getByRole("button", { name: "Cancel" })) @@ -549,12 +610,363 @@ describe("ArticleEditor topics requirement", () => { }) }) +describe("ArticleEditor autosave", () => { + test("a draft saves itself once typing stops", async () => { + const { article } = renderArticleEditor({ + autosaveDelayMs: AUTOSAVE_DELAY_MS, + }) + setMockResponse.patch(urls.websiteContent.details(article.id), article) + + await userEvent.type( + await screen.findByRole("heading", { level: 1 }), + " edited", + ) + + // Not on every keystroke: the write waits for the typing to stop. + expect(makeRequest).not.toHaveBeenCalledWith( + expect.objectContaining({ method: "patch" }), + ) + + await waitFor( + () => { + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ + method: "patch", + url: urls.websiteContent.details(article.id), + body: expect.objectContaining({ is_published: false }), + }), + ) + }, + { timeout: 6000 }, + ) + }, 15000) + + test("an article that has never been saved is created once, then updated", async () => { + const user = factories.user.user({ + is_authenticated: true, + is_article_editor: true, + }) + setMockResponse.get(urls.userMe.get(), user) + const created = factories.websiteContent.websiteContent({ + id: 909, + content, + is_published: false, + }) + setMockResponse.post(urls.websiteContent.list(), created) + setMockResponse.patch(urls.websiteContent.details(created.id), created) + + /* No `article`: the editor starts with nothing to update. */ + renderWithProviders(, { + user, + }) + + await userEvent.type( + await screen.findByRole("heading", { level: 1 }), + " first", + ) + await waitFor( + () => { + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ method: "post" }), + ) + }, + { timeout: 12000 }, + ) + // Settled first: typing while the create is still open puts a background + // write in every synchronous gap, which updates the toolbar outside `act`. + await waitFor(() => expect(screen.queryByText("Saving...")).toBe(null)) + + /** + * The editor has created the item but still holds no `contentItem` -- the + * caller has not navigated here -- so without remembering what it created + * this second save would insert another article. + */ + await userEvent.type(screen.getByRole("heading", { level: 1 }), " again") + await waitFor( + () => { + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ + method: "patch", + url: urls.websiteContent.details(created.id), + }), + ) + }, + { timeout: 12000 }, + ) + + const posts = makeRequest.mock.calls.filter( + (call) => call[0]?.method === "post", + ) + expect(posts).toHaveLength(1) + }, 30000) + + test("an autosave of an existing draft asks for no navigation", async () => { + const onSave = jest.fn() + const user = factories.user.user({ + is_authenticated: true, + is_article_editor: true, + }) + setMockResponse.get(urls.userMe.get(), user) + const article = factories.websiteContent.websiteContent({ + content, + is_published: false, + }) + setMockResponse.get(urls.websiteContent.details(article.id), article) + setMockResponse.patch(urls.websiteContent.details(article.id), article) + + renderWithProviders( + , + { user }, + ) + + await userEvent.type( + await screen.findByRole("heading", { level: 1 }), + " edited", + ) + await waitFor( + () => { + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ method: "patch" }), + ) + }, + { timeout: 6000 }, + ) + + /** + * `onSave` is how the caller learns to navigate, and a draft that already + * exists has not moved. Left to fire, every autosave would ask the page to + * push the route it is already on. + */ + expect(onSave).not.toHaveBeenCalled() + }, 15000) + + test("the indicator stops saying Saved as soon as typing resumes", async () => { + const { article } = renderArticleEditor({ + autosaveDelayMs: AUTOSAVE_DELAY_MS, + }) + setMockResponse.patch(urls.websiteContent.details(article.id), article) + const heading = await screen.findByRole("heading", { level: 1 }) + + await userEvent.type(heading, " edited") + await screen.findByText("Saved", {}, { timeout: 6000 }) + + /* The next keystroke is not saved, so the bar must stop claiming it is. + Re-queried: the node from before the save has been replaced. */ + await userEvent.type(screen.getByRole("heading", { level: 1 }), "!") + + expect(screen.queryByText("Saved")).toBe(null) + }, 15000) + + test("a settings save is not overtaken by an autosave in flight", async () => { + const topics = factories.learningResources.topics({ count: 1 }) + const [topic] = topics.results + setMockResponse.get(urls.topics.list({ limit: 1000 }), topics) + const { article } = renderArticleEditor({ + autosaveDelayMs: AUTOSAVE_DELAY_MS, + }) + + let contentWriteFinished = false + let settingsSawContentWriteFinished: boolean | null = null + /** + * The content write carries `topics` as they were when it started, so it + * is slow here on purpose: still open while the drawer saves new ones. + */ + setMockResponse.patch( + urls.websiteContent.details(article.id), + () => + new Promise((resolve) => + setTimeout(() => { + contentWriteFinished = true + resolve(article) + }, 3000), + ), + { requestBody: expect.objectContaining({ is_published: false }) }, + ) + /* The settings write, which sends topics and nothing else. */ + setMockResponse.patch( + urls.websiteContent.details(article.id), + () => { + settingsSawContentWriteFinished = contentWriteFinished + return article + }, + { + requestBody: expect.not.objectContaining({ + is_published: expect.anything(), + }), + }, + ) + + await userEvent.type( + await screen.findByRole("heading", { level: 1 }), + " edited", + ) + await waitFor( + () => { + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ + method: "patch", + body: expect.objectContaining({ is_published: false }), + }), + ) + }, + { timeout: 12000 }, + ) + + // Pick a topic while that write is still open. + await userEvent.click( + await screen.findByRole("button", { name: "Settings" }), + ) + await userEvent.click(await screen.findByLabelText("Topic")) + await userEvent.click( + await screen.findByRole("option", { name: topic.name }), + ) + await userEvent.click(screen.getByRole("button", { name: "Add" })) + await userEvent.click(screen.getByRole("button", { name: "Save Settings" })) + + await waitFor( + () => expect(settingsSawContentWriteFinished).not.toBe(null), + { timeout: 12000 }, + ) + + /** + * Sent only once the content write had finished. Overlapping, the content + * write's older topic list could be the one the server stored last, and + * the editor's new selection would vanish with nothing to say so. + */ + expect(settingsSawContentWriteFinished).toBe(true) + }, 30000) + + test("a publish waits for the draft save already in flight", async () => { + const { article } = renderArticleEditor({ + topics: [7], + autosaveDelayMs: AUTOSAVE_DELAY_MS, + }) + /** + * A slow draft write, so it is genuinely still running when the publish is + * confirmed -- which is the only way the two can interleave. + */ + setMockResponse.patch( + urls.websiteContent.details(article.id), + new Promise((resolve) => setTimeout(() => resolve(article), 800)), + { requestBody: expect.objectContaining({ is_published: false }) }, + ) + setMockResponse.patch( + urls.websiteContent.details(article.id), + { ...article, is_published: true }, + { requestBody: expect.objectContaining({ is_published: true }) }, + ) + + await userEvent.type( + await screen.findByRole("heading", { level: 1 }), + " edited", + ) + // Wait for the draft write to start, then publish while it is in flight. + await waitFor( + () => { + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ + method: "patch", + body: expect.objectContaining({ is_published: false }), + }), + ) + }, + { timeout: 6000 }, + ) + await userEvent.click( + await screen.findByRole("button", { name: "Publish" }), + ) + await userEvent.click( + await screen.findByRole("button", { name: "Yes, Publish article" }), + ) + + await waitFor( + () => { + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ + method: "patch", + body: expect.objectContaining({ is_published: true }), + }), + ) + }, + { timeout: 8000 }, + ) + + /** + * The publish is the last write. Unqueued it could be sent while the draft + * PATCH was still open, and whichever the server handled last would decide + * whether the item ended up public -- with the dialog reporting success + * either way. + */ + // Settled before teardown, so nothing updates after the test ends. + await waitFor(() => expect(screen.queryByRole("dialog")).toBe(null)) + + const bodies = makeRequest.mock.calls + .filter((call) => call[0]?.method === "patch") + // `body` is deliberately `unknown` in the request harness. + .map((call) => (call[0].body as { is_published?: boolean }).is_published) + expect(bodies).toEqual([false, true]) + }, 25000) + + test("a published article is never saved behind the author's back", async () => { + const { article } = renderArticleEditor({ + isPublished: true, + topics: [7], + autosaveDelayMs: AUTOSAVE_DELAY_MS, + }) + setMockResponse.patch(urls.websiteContent.details(article.id), article) + + await userEvent.type( + await screen.findByRole("heading", { level: 1 }), + " edited", + ) + await new Promise((resolve) => setTimeout(resolve, 3500)) + + /* Edits to something public go live only when Publish is pressed. */ + expect(makeRequest).not.toHaveBeenCalledWith( + expect.objectContaining({ method: "patch" }), + ) + }, 15000) + + test("the indicator is a live region before it has anything to say", async () => { + renderArticleEditor() + + /** + * Mounted empty, ahead of the first message. A live region inserted in the + * same paint as its content is routinely not announced at all, and the + * first message -- that the work is being saved -- is the one that matters. + */ + const region = await screen.findByRole("status") + expect(region).toBeEmptyDOMElement() + }) + + test("the control bar reports the save", async () => { + const { article } = renderArticleEditor({ + autosaveDelayMs: AUTOSAVE_DELAY_MS, + }) + setMockResponse.patch(urls.websiteContent.details(article.id), article) + + // Nothing is claimed before there is anything to save. + expect(screen.queryByText("Saved")).toBe(null) + + await userEvent.type( + await screen.findByRole("heading", { level: 1 }), + " edited", + ) + + await screen.findByText("Saved", {}, { timeout: 6000 }) + }, 15000) +}) + describe("ArticleEditor edit-mode control bar layout", () => { test("stacks the actions above the formatting controls", async () => { renderArticleEditor() const publish = await screen.findByRole("button", { - name: "Publish Article", + name: "Publish", }) const undo = screen.getByRole("button", { name: "Undo" }) const bar = screen.getByRole("toolbar") @@ -565,4 +977,32 @@ describe("ArticleEditor edit-mode control bar layout", () => { expect(actionRow).toContainElement(publish) expect(formattingRow).toContainElement(undo) }) + + test("puts the status at one end and the actions at the other", async () => { + renderArticleEditor() + + const publish = await screen.findByRole("button", { + name: "Publish", + }) + const settings = screen.getByRole("button", { name: "Settings" }) + const status = screen.getByText(/Status:/) + + // The status leads the row; the actions follow it. + expect( + status.compareDocumentPosition(publish) & + Node.DOCUMENT_POSITION_FOLLOWING, + ).toBeTruthy() + // Settings sits immediately after Publish, as the design pairs them. + expect(publish.nextElementSibling).toBe(settings) + }) + + test("the settings control is the icon alone", async () => { + renderArticleEditor() + + const settings = await screen.findByRole("button", { name: "Settings" }) + + /* No label of its own, so the name has to come from `aria-label`. */ + expect(settings).toHaveTextContent("") + expect(settings.querySelector("svg")).toBeInTheDocument() + }) }) diff --git a/frontends/main/src/page-components/TiptapEditor/contentTypes/article/ArticleEditor.tsx b/frontends/main/src/page-components/TiptapEditor/contentTypes/article/ArticleEditor.tsx index 311f296c72..1f4ecbae09 100644 --- a/frontends/main/src/page-components/TiptapEditor/contentTypes/article/ArticleEditor.tsx +++ b/frontends/main/src/page-components/TiptapEditor/contentTypes/article/ArticleEditor.tsx @@ -50,6 +50,8 @@ const extractArticleExtraFields = (content: { interface ArticleEditorProps { onSave?: (article: WebsiteContent) => void + /** See `WebsiteContentEditor`; only tests pass it. */ + autosaveDelayMs?: number readOnly?: boolean article?: WebsiteContent } @@ -66,7 +68,12 @@ interface ArticleEditorProps { * * WebsiteContentEditor does not need to change at all. */ -const ArticleEditor = ({ onSave, readOnly, article }: ArticleEditorProps) => { +const ArticleEditor = ({ + onSave, + autosaveDelayMs, + readOnly, + article, +}: ArticleEditorProps) => { // Swap these hooks when a dedicated article API exists. // The editor renders its own inline error (WebsiteContentEditor `error`), so // suppress the global error toast. @@ -85,6 +92,7 @@ const ArticleEditor = ({ onSave, readOnly, article }: ArticleEditorProps) => { saveMutations={{ create: createMutation, update: updateMutation }} uploadImage={uploadImage} onSave={onSave} + autosaveDelayMs={autosaveDelayMs} readOnly={readOnly} contentItem={article} bannerViewer={ArticleBannerViewer} diff --git a/frontends/main/src/page-components/TiptapEditor/contentTypes/news/NewsEditor.happydom.test.tsx b/frontends/main/src/page-components/TiptapEditor/contentTypes/news/NewsEditor.happydom.test.tsx index 089e90bc37..dae22ac0ff 100644 --- a/frontends/main/src/page-components/TiptapEditor/contentTypes/news/NewsEditor.happydom.test.tsx +++ b/frontends/main/src/page-components/TiptapEditor/contentTypes/news/NewsEditor.happydom.test.tsx @@ -30,6 +30,16 @@ jest.mock("posthog-js/react", () => ({ usePostHog: () => ({}), })) +/** + * Far enough out that no draft saves itself while a test works: a background + * write landing mid-interaction updates the toolbar outside `act`, which + * failed these suites intermittently. The autosave test sets its own. + */ +const AUTOSAVE_OFF = 10 * 60 * 1000 + +/** What production uses; the autosave test waits this out deliberately. */ +const AUTOSAVE_DELAY_MS = 2000 + const mockOnSave = jest.fn() describe("NewsEditor - Content Editing and Saving", () => { @@ -42,6 +52,7 @@ describe("NewsEditor - Content Editing and Saving", () => { content: JSONContent, articleId = 100, title = "Test Article", + autosaveDelayMs = AUTOSAVE_OFF, ) => { const user = factories.user.user({ is_authenticated: true, @@ -58,7 +69,11 @@ describe("NewsEditor - Content Editing and Saving", () => { setMockResponse.get(urls.websiteContent.details(articleId), newsItem) renderWithProviders( - , + , { user, }, @@ -139,7 +154,8 @@ describe("NewsEditor - Content Editing and Saving", () => { updatedArticle, ) - const heading = screen.getByRole("heading", { level: 1 }) + // Awaited: under load the editor's content mounts after its container. + const heading = await screen.findByRole("heading", { level: 1 }) await userEvent.click(heading) await userEvent.keyboard("{Control>}a{/Control}{Delete}") @@ -393,8 +409,8 @@ describe("NewsEditor - Content Editing and Saving", () => { }) }) - describe("Save as Draft functionality", () => { - test("can save news as draft", async () => { + describe("Autosaving a draft", () => { + test("a news draft saves itself once typing stops", async () => { const initialContent: JSONContent = { type: "doc", content: [ @@ -422,7 +438,12 @@ describe("NewsEditor - Content Editing and Saving", () => { ], } - const newsItem = await setupEditor(initialContent, 208, "Title") + const newsItem = await setupEditor( + initialContent, + 208, + "Title", + AUTOSAVE_DELAY_MS, + ) const paragraph = screen.getByText("Content") await userEvent.click(paragraph) @@ -441,23 +462,23 @@ describe("NewsEditor - Content Editing and Saving", () => { updatedNewsItem, ) - const saveDraftButton = await screen.findByRole("button", { - name: "Save as Draft", - }) - - expect(saveDraftButton).not.toBeDisabled() - - await userEvent.click(saveDraftButton) + // No button to press: a draft writes itself once typing stops. + expect(screen.queryByRole("button", { name: "Save as Draft" })).toBe(null) - expect(makeRequest).toHaveBeenCalledWith({ - method: "patch", - url: urls.websiteContent.details(newsItem.id), - body: expect.objectContaining({ - is_published: false, - author_name: "", - }), - }) - }) + await waitFor( + () => { + expect(makeRequest).toHaveBeenCalledWith({ + method: "patch", + url: urls.websiteContent.details(newsItem.id), + body: expect.objectContaining({ + is_published: false, + author_name: "", + }), + }) + }, + { timeout: 6000 }, + ) + }, 15000) }) describe("Error handling during save", () => { @@ -538,7 +559,10 @@ describe("NewsEditor - Content Editing and Saving", () => { }) setMockResponse.post(urls.websiteContent.list(), createdNewsItem) - renderWithProviders(, { user }) + renderWithProviders( + , + { user }, + ) await screen.findByTestId("editor") @@ -558,7 +582,7 @@ describe("NewsEditor - Content Editing and Saving", () => { }) const publishButton = await screen.findByRole("button", { - name: "Publish News", + name: "Publish", }) expect(publishButton).not.toBeDisabled() @@ -665,7 +689,12 @@ describe("NewsEditor - Document Rendering", () => { setMockResponse.get(urls.websiteContent.details(articleId), newsItem) renderWithProviders( - , + , { user }, ) @@ -680,7 +709,10 @@ describe("NewsEditor - Document Rendering", () => { }) setMockResponse.get(urls.userMe.get(), user) - renderWithProviders(, { user }) + renderWithProviders( + , + { user }, + ) await screen.findByTestId("editor") }) @@ -1583,7 +1615,14 @@ describe("NewsEditor - Byline publish date", () => { }) setMockResponse.get(urls.websiteContent.details(newsItem.id), newsItem) - renderWithProviders(, { user }) + renderWithProviders( + , + { user }, + ) await screen.findByTestId("editor") return newsItem } @@ -1623,7 +1662,7 @@ describe("NewsEditor - Delete draft", () => { jest.clearAllMocks() }) - test("editor can delete a draft from the edit toolbar", async () => { + test("the edit toolbar does not offer delete", async () => { const user = factories.user.user({ is_authenticated: true, is_article_editor: true, @@ -1632,63 +1671,30 @@ describe("NewsEditor - Delete draft", () => { const newsItem = factories.websiteContent.websiteContent({ id: 321, - title: "Draft to delete", + title: "Draft news", content_type: "news", is_published: false, }) setMockResponse.get(urls.websiteContent.details(newsItem.id), newsItem) - setMockResponse.delete(urls.websiteContent.details(newsItem.id), null) renderWithProviders( - , + , { user, }, ) - await screen.findByTestId("editor") - await userEvent.click(await screen.findByRole("button", { name: "Delete" })) - await userEvent.click( - await screen.findByRole("button", { name: "Yes, delete" }), - ) - - await waitFor(() => { - expect(makeRequest).toHaveBeenCalledWith( - expect.objectContaining({ - method: "delete", - url: urls.websiteContent.details(newsItem.id), - }), - ) - }) - }) - - test("delete button is not shown for published content", async () => { - const user = factories.user.user({ - is_authenticated: true, - is_article_editor: true, - }) - setMockResponse.get(urls.userMe.get(), user) - - const newsItem = factories.websiteContent.websiteContent({ - id: 322, - title: "Published item", - content_type: "news", - is_published: true, - }) - setMockResponse.get(urls.websiteContent.details(newsItem.id), newsItem) - - renderWithProviders( - , - { - user, - }, - ) - - await screen.findByTestId("editor") - expect( - screen.queryByRole("button", { name: "Delete" }), - ).not.toBeInTheDocument() + /** + * Deleting a draft lives on the drafts listing instead, where + * `WebsiteContentDraftListingPage delete` covers it. The edit bar carries + * the status, Publish and the settings icon, and nothing else. + */ + expect(screen.queryByRole("button", { name: "Delete" })).toBe(null) }) }) @@ -1702,7 +1708,7 @@ describe("NewsEditor - shared content controls", () => { * shared by every content type, so their copy must name the type being * edited. These lock the substitution in: news must never say "Article". */ - test("the news control bar names news in its status readout", async () => { + test("the news control bar carries the status readout", async () => { const user = factories.user.user({ is_authenticated: true, is_article_editor: true, @@ -1717,14 +1723,17 @@ describe("NewsEditor - shared content controls", () => { }) setMockResponse.get(urls.websiteContent.details(newsItem.id), newsItem) - renderWithProviders(, { user }) + renderWithProviders( + , + { user }, + ) await screen.findByTestId("editor") await screen.findByRole("button", { name: "Settings" }) - expect(await screen.findByText(/status:/)).toHaveTextContent( - "News status: Draft", + /* The readout no longer names the content type, for either type. */ + expect(await screen.findByText(/Status:/)).toHaveTextContent( + "Status: Draft", ) - expect(screen.queryByText(/Article status:/)).not.toBeInTheDocument() }) test("the settings drawer is titled for news", async () => { @@ -1746,7 +1755,10 @@ describe("NewsEditor - shared content controls", () => { }) setMockResponse.get(urls.websiteContent.details(newsItem.id), newsItem) - renderWithProviders(, { user }) + renderWithProviders( + , + { user }, + ) await screen.findByTestId("editor") await userEvent.click( @@ -1774,7 +1786,10 @@ describe("NewsEditor - shared content controls", () => { }) setMockResponse.get(urls.websiteContent.details(newsItem.id), newsItem) - renderWithProviders(, { user }) + renderWithProviders( + , + { user }, + ) await screen.findByTestId("editor") await userEvent.click( diff --git a/frontends/main/src/page-components/TiptapEditor/contentTypes/news/NewsEditor.tsx b/frontends/main/src/page-components/TiptapEditor/contentTypes/news/NewsEditor.tsx index d5c1389108..9ada65999f 100644 --- a/frontends/main/src/page-components/TiptapEditor/contentTypes/news/NewsEditor.tsx +++ b/frontends/main/src/page-components/TiptapEditor/contentTypes/news/NewsEditor.tsx @@ -24,6 +24,8 @@ const extractNewsExtraFields = (content: { interface NewsEditorProps { onSave?: (savedContent: WebsiteContent) => void + /** See `WebsiteContentEditor`; only tests pass it. */ + autosaveDelayMs?: number readOnly?: boolean newsItem?: WebsiteContent } @@ -33,7 +35,12 @@ interface NewsEditorProps { * Owns its own save mutations (websiteContent API) and passes them to * WebsiteContentEditor — keeping the generic shell decoupled from any specific API. */ -const NewsEditor = ({ onSave, readOnly, newsItem }: NewsEditorProps) => { +const NewsEditor = ({ + onSave, + autosaveDelayMs, + readOnly, + newsItem, +}: NewsEditorProps) => { // News content type uses the websiteContent API. // A different content type would call different hooks here. // The editor renders its own inline error (WebsiteContentEditor `error`), so @@ -53,6 +60,7 @@ const NewsEditor = ({ onSave, readOnly, newsItem }: NewsEditorProps) => { saveMutations={{ create: createMutation, update: updateMutation }} uploadImage={uploadImage} onSave={onSave} + autosaveDelayMs={autosaveDelayMs} readOnly={readOnly} contentItem={newsItem} /> diff --git a/frontends/main/src/page-components/TiptapEditor/core/WebsiteContentEditor.tsx b/frontends/main/src/page-components/TiptapEditor/core/WebsiteContentEditor.tsx index 89275dec18..0eb3d62ae7 100644 --- a/frontends/main/src/page-components/TiptapEditor/core/WebsiteContentEditor.tsx +++ b/frontends/main/src/page-components/TiptapEditor/core/WebsiteContentEditor.tsx @@ -12,18 +12,16 @@ import { HEADER_HEIGHT, HEADER_HEIGHT_MD, } from "ol-components" -import { Alert, Button, ButtonLink } from "@mitodl/smoot-design" +import { ActionButton, Alert, Button, ButtonLink } from "@mitodl/smoot-design" import { useUserHasPermission, Permission } from "api/hooks/user" import { useQueryClient, type QueryClient } from "@tanstack/react-query" import dynamic from "next/dynamic" -import { useRouter } from "next-nprogress-bar" import { - RiDeleteBinLine, + RiCheckLine, RiEditLine, RiEqualizerLine, RiSave3Line, } from "@remixicon/react" -import { showDeleteWebsiteContentDialog } from "@/page-components/WebsiteContentDialogs/DeleteWebsiteContentDialog" import { showPublishWebsiteContentDialog } from "@/page-components/WebsiteContentDialogs/PublishWebsiteContentDialog" import { ArticleSettingsDrawer, @@ -50,6 +48,17 @@ const LearningResourceDrawer = dynamic( const TOOLBAR_HEIGHT = 43 +/** + * How long typing has to stop before a draft saves itself. + * + * Long enough that ordinary typing does not queue a request per pause, short + * enough that little is at risk if the tab goes away. + */ +const AUTOSAVE_DELAY_MS = 2000 + +/** Matches the banner's and the body's column, which the action row follows. */ +const CONTENT_COLUMN_WIDTH = 890 + /* The pieces the stacked edit-mode bar is built from, per the design. */ const TOOLBAR_PADDING_Y = 12 const TOOLBAR_ROW_GAP = 24 @@ -99,12 +108,26 @@ const StackedToolbar = styled(StyledToolbar)({ }) /* Nowrap so the bar keeps its derived height; it scrolls instead. */ -const ActionRow = styled.div({ +const ActionRow = styled.div(({ theme }) => ({ display: "flex", alignItems: "center", gap: "16px", flexWrap: "nowrap", -}) + /** + * The article's own column, not the bar's full width: the status then lines + * up with the breadcrumb and title, and the actions with the far edge of the + * text. Both the banner (`InnerContainer`) and the body (`TiptapEditor`'s + * `Container`) are this same 890px centred column, padded by 24px, so these + * numbers follow them and have to keep following them. + */ + width: "100%", + maxWidth: `${CONTENT_COLUMN_WIDTH}px`, + margin: "0 auto", + padding: "0 24px", + [theme.breakpoints.down("sm")]: { + padding: "0 16px", + }, +})) /** * A real box for the formatting controls. `MainToolbarContent` wraps them in a @@ -172,6 +195,26 @@ const StatusValue = styled.span(({ theme }) => ({ color: theme.custom.colors.darkGray2, })) +/* Sits beside the status, as the design's "Saving..." does beside the title. */ +/** + * A span, not a `Typography`: it holds a spinner while saving, which renders a + * div -- and a `

`, Typography's default, may not contain one. Wrapping + * `Typography` cannot fix that, since `styled()` drops its polymorphic + * `component` prop, so the typography comes from the theme instead. + */ +const AutosaveText = styled.span(({ theme }) => ({ + display: "inline-flex", + alignItems: "center", + gap: "4px", + ...theme.typography.body3, + color: theme.custom.colors.silverGrayDark, + whiteSpace: "nowrap", + svg: { + width: "16px", + height: "16px", + }, +})) + export type UploadHandler = ( file: File, onProgress?: (e: { progress: number }) => void, @@ -284,6 +327,16 @@ export interface WebsiteContentEditorProps { */ uploadImage: MediaUpload onSave?: (contentItem: WebsiteContent) => void + /** + * How long typing has to stop before a draft saves itself. + * + * Only tests pass it. They set it far enough out that no save fires while + * they work -- a background write landing mid-interaction re-renders the + * toolbar and warns about updates outside `act`, which made every typing + * test in these suites intermittently fail -- and the ones that are about + * autosave set it back to something they can wait for. + */ + autosaveDelayMs?: number readOnly?: boolean contentItem?: WebsiteContent bannerViewer?: typeof BannerViewer @@ -298,6 +351,7 @@ const WebsiteContentEditor = ({ saveMutations, uploadImage, onSave, + autosaveDelayMs = AUTOSAVE_DELAY_MS, readOnly, contentItem, bannerViewer, @@ -305,10 +359,12 @@ const WebsiteContentEditor = ({ const [isPublishing, setIsPublishing] = useState(false) const [settingsOpen, setSettingsOpen] = useState(false) /** - * A save held back for want of topics, remembering whether it was a publish. - * `handleSettingsSave` resumes it once a topic has been picked. + * Whether a publish is waiting on topics. `handleSettingsSave` resumes it + * once one has been picked. Only a publish is ever held back: a draft saves + * itself, and autosave cannot stop to ask. */ - const [pendingSave, setPendingSave] = useState(null) + const [awaitingTopicsForPublish, setAwaitingTopicsForPublish] = + useState(false) const [uploadError, setUploadError] = useState(null) const [resetAttempted, setResetAttempted] = useState(false) const [content, setContent] = useState( @@ -317,6 +373,59 @@ const WebsiteContentEditor = ({ const [title, setTitle] = useState(contentItem?.title) const [topics, setTopics] = useState(contentItem?.topics ?? []) const [touched, setTouched] = useState(false) + /** + * The title and content as last written to the server, so autosave can tell + * an unsaved change from a re-render. Seeded with what was loaded: opening a + * draft and closing it must not write anything. + */ + /** + * The row this editor has created, when it started without one. + * + * Autosave repeats, and the caller moves the editor to the new item's URL + * only once that has happened -- so a second write before the route changes + * would create a second item. This is what makes it an update instead. + */ + const createdIdRef = useRef(null) + /** + * Every write goes through here, so two can never be in flight at once. + * + * A publish confirmed while a debounced draft save is still running would + * otherwise race it, and whichever landed last would decide whether the + * item ended up public -- however the dialog reported it. + */ + const saveChainRef = useRef>(Promise.resolve()) + /** + * Set once a publish from this editor has succeeded. A draft write queued + * behind it would send `is_published: false` and take the item straight back + * down, since `contentItem` still says draft until the caller reloads. + */ + const publishedHereRef = useRef(false) + /** + * A created item waiting to be handed to the caller, which navigates to its + * URL and so unmounts this editor. Held until nothing is unsaved, or + * anything typed while the create was in flight would go with it. + * + * A publish clears it: the response held here says `is_published: false`, + * so handing it over afterwards would send the editor to the draft page for + * an item that is now public. That was reachable -- a publish confirmed + * while the create was still in flight leaves this waiting on `isPending`, + * and it fired once the publish settled. + * + * Neither the holding nor that ordering is covered by a test. Both windows + * only exist while a create is in flight, and in happy-dom the editor's + * state updates and the mutation's pending flag do not interleave the way + * they do in a browser: the edit typed during a create has not reached + * state by the time the response lands, and forcing the other order needs a + * wait long enough that the suite trips over its own teardown. + */ + const handoffRef = useRef(null) + const savedRef = useRef({ + title: contentItem?.title, + content: contentItem?.content ?? initialDoc, + }) + const [autosaveState, setAutosaveState] = useState< + "idle" | "saving" | "saved" + >("idle") const { create: createMutation, update: updateMutation } = saveMutations const isPending = createMutation.isPending || updateMutation.isPending @@ -327,7 +436,6 @@ const WebsiteContentEditor = ({ uploadImageRef.current = uploadImage const queryClient = useQueryClient() - const router = useRouter() const isArticleEditor = useUserHasPermission(Permission.ArticleEditor) const uploadHandler = useCallback( @@ -376,9 +484,16 @@ const WebsiteContentEditor = ({ /** * Returns a promise that settles with the save, so a confirmation dialog can * await it: it must not close until the request has actually succeeded, and - * must stay open if it fails. Rejects on failure — see `saveQuietly` for the - * buttons that save without a dialog. + * must stay open if it fails. Rejects on failure, so every caller that is + * not a dialog has to handle the rejection itself. */ + /** Runs `write` after whatever is already queued, in order. */ + const queueSave = (write: () => Promise): Promise => { + const next = saveChainRef.current.catch(() => undefined).then(write) + saveChainRef.current = next + return next + } + const handleSave = async (publish: boolean, topicsOverride?: number[]) => { if (!title) return // Overridden when a held-back save resumes: `setTopics` has not landed yet @@ -386,9 +501,10 @@ const WebsiteContentEditor = ({ // leaving the drawer to PATCH it separately. const savedTopics = topicsOverride ?? topics const extraFields = extractExtraFields?.(content) ?? {} - const saved = contentItem + const existingId = contentItem?.id ?? createdIdRef.current + const saved = existingId ? await updateMutation.mutateAsync({ - id: contentItem.id, + id: existingId, title: title.trim(), content, is_published: publish, @@ -402,7 +518,28 @@ const WebsiteContentEditor = ({ topics: savedTopics, ...extraFields, }) - onSave?.(saved) + savedRef.current = { title, content } + createdIdRef.current = saved.id + /** + * `onSave` exists to move the editor to where the saved item now lives, + * and that is somewhere new on exactly two transitions: the save that + * created the item, and the one that published it. + * + * An autosave of a draft that already exists is already there, so calling + * it would ask the caller to navigate to the route it is on -- every + * couple of seconds, for as long as someone keeps typing. + */ + if (publish) { + // Supersedes a handoff still waiting to be made. The create response it + // holds says `is_published: false`, so a caller acting on it afterwards + // would send the editor to the draft page for an item now public. + handoffRef.current = null + onSave?.(saved) + } else if (!existingId) { + // Deferred to the effect below: handing over navigates, and anything + // typed while the create was in flight is not saved yet. + handoffRef.current = saved + } } /** @@ -412,7 +549,7 @@ const WebsiteContentEditor = ({ * ride along with the create. * * The failure is surfaced by the `saveError` alert below, so the rejection is - * swallowed rather than left unhandled -- as in `saveQuietly`. + * swallowed rather than left unhandled. */ const handleSettingsSave = ({ topics: nextTopics, @@ -423,42 +560,96 @@ const WebsiteContentEditor = ({ if (nextTopics === undefined) return setTopics(nextTopics) - // A save that was held back resumes here, carrying the new selection, so - // the content write persists the topics and no separate PATCH is needed. - if (pendingSave !== null && nextTopics.length > 0) { - const publish = pendingSave - setPendingSave(null) - if (publish) startPublish(nextTopics) - else saveQuietly(false, nextTopics) + // A publish that was held back resumes here, carrying the new selection, + // so the content write persists the topics and no separate PATCH is + // needed. + if (awaitingTopicsForPublish && nextTopics.length > 0) { + setAwaitingTopicsForPublish(false) + startPublish(nextTopics) return } if (!contentItem) return - updateMutation - .mutateAsync({ id: contentItem.id, topics: nextTopics }) - .catch(() => undefined) + // Queued like the others. A content write already in flight carries the + // topics as they were when it started, so sent alongside it this could be + // the older list that the server stores last -- taking the selection the + // editor just made back out, with nothing on screen to say so. + queueSave(() => + updateMutation.mutateAsync({ id: contentItem.id, topics: nextTopics }), + ).catch(() => undefined) } /** - * For the buttons that save with no dialog awaiting the result. The failure - * is already surfaced by the `saveError` alert below, so the rejection is - * swallowed here rather than left unhandled. + * A draft writes itself; published content does not. + * + * Once something is public every save pushes edits live, so it stays an + * explicit act -- the author presses Publish. A draft has no such audience, + * and losing unsaved work to a closed tab is the worse risk, so it is + * written for them. This covers content that has never been saved too: + * that is how it comes into existence now that there is no draft button. */ - const saveQuietly = (publish: boolean, topicsOverride?: number[]) => { - handleSave(publish, topicsOverride).catch(() => undefined) - } + const autosaves = !contentItem?.is_published + const hasUnsavedChanges = + title !== savedRef.current.title || content !== savedRef.current.content + + useEffect(() => { + // `isPending` in the deps is what re-arms this after an in-flight save: + // edits made while one was running are picked up when it settles. + // `touched` as well as the comparison: a new item's content starts out + // differing from the empty saved state, and opening the editor on it must + // not write anything until somebody types. + if (!autosaves || !touched || !hasUnsavedChanges || !title || isPending) { + return + } + // A publish from this editor supersedes draft writes: `contentItem` still + // says draft, so this would otherwise take down what was just published. + if (publishedHereRef.current) return + const timer = setTimeout(() => { + setAutosaveState("saving") + queueSave(() => handleSave(false)) + .then(() => setAutosaveState("saved")) + // The alert below reports the failure; the indicator drops back to + // saying nothing rather than claiming a save that did not happen. + .catch(() => setAutosaveState("idle")) + }, autosaveDelayMs) + return () => clearTimeout(timer) + // `handleSave` closes over the state it sends and is remade every render; + // the timer is rearmed on every edit regardless, which is the debounce. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [ + autosaves, + touched, + hasUnsavedChanges, + title, + content, + isPending, + autosaveDelayMs, + ]) + + useEffect(() => { + const created = handoffRef.current + if (!created || isPending || hasUnsavedChanges) return + // Nor after a publish from here, whatever set the handoff. + if (publishedHereRef.current) return + handoffRef.current = null + onSave?.(created) + // `onSave` is the caller's and stable in practice; re-running on a new + // identity would hand the same item over twice. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [isPending, hasUnsavedChanges]) /** - * An article's topics are what put it on a topic page, so it is not saved - * without them: the press opens the settings drawer instead and is resumed - * once a topic is picked. News has no topics section, so nothing to require. + * An article's topics are what put it on a topic page, so it is not + * published without them: the press opens the settings drawer instead and is + * resumed once a topic is picked. Drafts are exempt -- autosave cannot stop + * to ask -- and news has no topics section at all. */ const topicsRequired = contentType === WebsiteContentContentTypeEnum.Article const topicsMissing = topicsRequired && topics.length === 0 - /** Hold the press back and ask for topics. */ - const askForTopics = (publish: boolean) => { - setPendingSave(publish) + /** Hold the publish back and ask for topics. */ + const askForTopics = () => { + setAwaitingTopicsForPublish(true) setSettingsOpen(true) } @@ -471,7 +662,14 @@ const WebsiteContentEditor = ({ const startPublish = (topicsOverride?: number[]) => { const publish = () => { setIsPublishing(true) - return handleSave(true, topicsOverride) + // Queued, so a draft save already running settles first and this lands + // last -- what the dialog reports and what is stored then agree. + return queueSave(() => handleSave(true, topicsOverride)) + .then((result) => { + publishedHereRef.current = true + return result + }) + .finally(() => setIsPublishing(false)) } if (contentItem?.is_published) { // Nothing awaits this path, so do not leave the rejection unhandled; @@ -577,13 +775,57 @@ const WebsiteContentEditor = ({ const statusSlot = ( - {contentLabel} status:{" "} + Status:{" "} {contentItem?.is_published ? "Published" : "Draft"} ) + /** + * What autosave is doing, in the manner of the design: "Saving..." while a + * write is in flight, then "Saved" until the next edit. It says nothing + * until the first save, so a draft opened and left alone claims nothing. + * + * `role="status"` announces that without stealing focus -- but only if the + * region was already in the page when the text appeared. A live region + * mounted in the same paint as its first message is routinely dropped by + * screen readers, and that first message is the one that matters, so the + * region is mounted empty for the whole session and only its text changes. + */ + const autosaveMessages: Record = { + idle: null, + saving: ( + <> + {/* Decorative: the text beside it says the same thing, and the + spinner's own "Loading" label would be read out as well. */} + + + + Saving... + + ), + saved: ( + <> + + Saved + + ), + } + + /** + * "Saved" is only true until the next keystroke. The state itself cannot say + * that -- it is set once, when a write returns -- so it is read together + * with whether anything has changed since, rather than claiming a save that + * no longer covers what is on screen. + */ + const autosaveShown = + autosaveState === "saved" && hasUnsavedChanges ? "idle" : autosaveState + + const autosaveSlot = autosaves ? ( + {autosaveMessages[autosaveShown]} + ) : null + /** * "medium" reproduces the design's button box exactly: 40px tall, 14px medium * label, 20px icon, 8px gap, and 12px/16px horizontal padding (smoot-design @@ -591,15 +833,25 @@ const WebsiteContentEditor = ({ */ const buttonSize = "medium" + /** + * The icon alone in a button box, as the design has it -- `bordered` to + * match the other buttons in the bar, `ActionButton` because it carries no + * label. Named for assistive technology and for the pointer, since nothing + * on screen says what it opens. + * + * Shared with the published view's bar, so the control looks the same + * wherever it appears. + */ const settingsButton = ( - + + ) /** @@ -657,69 +909,37 @@ const WebsiteContentEditor = ({ {/* The design puts the actions above the formatting controls, both rows centred. */} - {contentItem && !contentItem.is_published ? ( - - ) : null} - {settingsButton} - {!contentItem?.is_published ? ( - - ) : null} - { - if (topicsMissing) { - askForTopics(true) - return - } - startPublish() - }} - size={buttonSize} - endIcon={ - isPending && isPublishing ? ( - - ) : null - } - > - Publish {contentLabel} - - {statusSlot} + Publish + + {settingsButton} + @@ -735,7 +955,7 @@ const WebsiteContentEditor = ({ setSettingsOpen(false) // Dropped rather than kept: a press the editor walked away // from must not fire the next time topics happen to be saved. - setPendingSave(null) + setAwaitingTopicsForPublish(false) }} contentLabel={contentLabel} /* Only an article becomes a LearningResource, so only there do @@ -744,6 +964,12 @@ const WebsiteContentEditor = ({ contentType === WebsiteContentContentTypeEnum.Article } topicsRequired={topicsRequired} + /* Only once it is public: a draft may be left without topics, + since publishing is where they are insisted on, and autosave + cannot stop to ask. */ + topicsMayNotBeEmptied={ + topicsRequired && !!contentItem?.is_published + } initialValues={{ topics }} onSave={handleSettingsSave} /> diff --git a/learning_resources/tasks.py b/learning_resources/tasks.py index cf9d7f581a..85b1febc2d 100644 --- a/learning_resources/tasks.py +++ b/learning_resources/tasks.py @@ -1124,7 +1124,16 @@ def sync_website_content_learning_resource(content_id: int) -> None: log.exception("Could not queue the removal for content %s", content_id) -@app.task(acks_late=True, reject_on_worker_lost=True) +@app.task( + acks_late=True, + reject_on_worker_lost=True, + # Retried, unlike the sync side: this task is where the inline removal + # hands off when it fails, and the callers that do so describe it as the + # one carrying the retries. Without a policy a transient database or + # search error failed it once and left the resource indexed. + autoretry_for=(Exception,), + retry_kwargs={"max_retries": 3, "countdown": 5}, +) def unpublish_website_content_learning_resource_task(content_id: int) -> None: """ Take an unpublished WebsiteContent item's LearningResource out of search. diff --git a/learning_resources/tasks_test.py b/learning_resources/tasks_test.py index 0a0f2d0c3f..9a26725123 100644 --- a/learning_resources/tasks_test.py +++ b/learning_resources/tasks_test.py @@ -1759,6 +1759,18 @@ def test_sync_website_content_survives_an_unreachable_broker(mocker): tasks.sync_website_content_learning_resource.delay(content.id) +def test_unpublish_website_content_task_retries(): + """ + The removal task retries, which is what the callers that hand off to it + depend on: the inline removal gives up its work to this one, so a + transient database or search error here must not end the attempt. + """ + task = tasks.unpublish_website_content_learning_resource_task + + assert task.autoretry_for == (Exception,) + assert task.retry_kwargs["max_retries"] == 3 + + def test_unpublish_website_content_learning_resource_task(mocker): """The removal task works from the id, so a deleted item still leaves the index.""" mock_unpublish = mocker.patch( From c64ee9456fb95b72979af64fd154ec9ae50aaf6b Mon Sep 17 00:00:00 2001 From: Zaman Afzal Date: Mon, 28 Sep 2026 18:51:12 +0500 Subject: [PATCH 8/9] Smooth FAQ accordion open/close animation (#3997) * fix(product-pages): smooth FAQ accordion open/close animation --- frontends/main/src/app-pages/ProductPages/FaqsSection.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/frontends/main/src/app-pages/ProductPages/FaqsSection.tsx b/frontends/main/src/app-pages/ProductPages/FaqsSection.tsx index 1c68f967bb..7db4cd8ca3 100644 --- a/frontends/main/src/app-pages/ProductPages/FaqsSection.tsx +++ b/frontends/main/src/app-pages/ProductPages/FaqsSection.tsx @@ -97,6 +97,7 @@ const FaqRow: React.FC<{ index: number; faq: FAQItem }> = ({ index, faq }) => { expanded={expanded} onChange={() => setExpanded(!expanded)} disableGutters + slotProps={{ transition: { timeout: 250 } }} > Date: Mon, 28 Sep 2026 14:16:55 +0000 Subject: [PATCH 9/9] Release 0.81.0 --- RELEASE.rst | 12 ++++++++++++ main/settings.py | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/RELEASE.rst b/RELEASE.rst index fa6311fd24..21d19197af 100644 --- a/RELEASE.rst +++ b/RELEASE.rst @@ -1,6 +1,18 @@ Release Notes ============= +Version 0.81.0 +-------------- + +- Smooth FAQ accordion open/close animation (#3997) +- feat(website-content): save drafts automatically, drop the draft button (#3978) +- Posthog checkout_completed event (#3982) +- fix: scope unsubscribe lookup to the requesting user's own subscriptions (#3984) +- Sanitize MITPE news and events summary/content like sibling sources (#3977) +- feat(website-content): unpublish published items from the listing card (#3963) +- Re-ingest content files of republished runs, fix inconsistent contentfile (de)indexing (#3976) +- add back schedule and fix admin criteria text field (#3989) + Version 0.80.18 --------------- diff --git a/main/settings.py b/main/settings.py index 0fb3b4ee48..39812dec86 100644 --- a/main/settings.py +++ b/main/settings.py @@ -36,7 +36,7 @@ from main.settings_pluggy import * # noqa: F403 from openapi.settings_spectacular import open_spectacular_settings -VERSION = "0.80.18" +VERSION = "0.81.0" log = logging.getLogger()