From 80c6d0a1d1a7ed1d7e743fd228db13acb01329d3 Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Thu, 27 Aug 2026 14:02:11 -0400 Subject: [PATCH 01/14] Update dependency social-auth-app-django to v5.6.0 [SECURITY] (#3850) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --- uv.lock | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/uv.lock b/uv.lock index 2510d03616..47defa0494 100644 --- a/uv.lock +++ b/uv.lock @@ -4746,15 +4746,16 @@ wheels = [ [[package]] name = "social-auth-app-django" -version = "5.4.3" +version = "5.9.0" source = { registry = "https://pypi.org/simple" } dependencies = [ + { name = "asgiref" }, { name = "django" }, { name = "social-auth-core" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/2a/07/bb2465e4116d4761b028bd07b99087009caa81c1511c886d74c4ccece3a2/social_auth_app_django-5.4.3.tar.gz", hash = "sha256:d1f4286d5ca1e512c9b2f686e7ecb2a0128148f1a33d853b69dc07b58508362e", size = 24860, upload-time = "2025-02-13T13:07:34.557Z" } +sdist = { url = "https://files.pythonhosted.org/packages/62/f9/c761cf93cb0a102ddd59498ea560b398be6bd14b8d6a4c493cc011b4bb80/social_auth_app_django-5.9.0.tar.gz", hash = "sha256:5b79c53321f9528334d53ddb154452a3b7e598b7ff4c2c1302be9a76b1349e8d", size = 29751, upload-time = "2026-04-29T14:52:50.715Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/e0/cd/43a25dabdf7689109b01ac866848c984594769ec3cbc5ce4c261b4895237/social_auth_app_django-5.4.3-py3-none-any.whl", hash = "sha256:db70b972faeb10ee1ec83d0dc7dbd0558d5f5830417bba317b712b10ff58d031", size = 26241, upload-time = "2025-02-13T13:07:32.787Z" }, + { url = "https://files.pythonhosted.org/packages/f0/8c/5a06761549ea5e7e3c6e87d25ed2029ed5ba9ba39c493bc8a591548d6575/social_auth_app_django-5.9.0-py3-none-any.whl", hash = "sha256:80edf5c207d871f2225a0fc6bb414004c0342c30deb3a217eb38a63b66e749bb", size = 28568, upload-time = "2026-04-29T14:52:49.473Z" }, ] [[package]] From 78d2fd0d357726566e721dd5004312fe40b8c42a Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Thu, 27 Aug 2026 14:37:20 -0400 Subject: [PATCH 02/14] Update dependency Django to v5.2.17 [SECURITY] (#3849) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --- pyproject.toml | 2 +- uv.lock | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index b8be0da718..f9d5acc879 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -11,7 +11,7 @@ classifiers = [ "Programming Language :: Python :: 3.12", ] dependencies = [ - "Django==5.2.16", + "Django==5.2.17", "attrs>=25.0.0,<26", "base36>=0.1.1,<0.2", "beautifulsoup4>=4.8.2,<5", diff --git a/uv.lock b/uv.lock index 47defa0494..b8615c01fd 100644 --- a/uv.lock +++ b/uv.lock @@ -765,16 +765,16 @@ sdist = { url = "https://files.pythonhosted.org/packages/2b/8f/77a4b8ec50c821193 [[package]] name = "django" -version = "5.2.16" +version = "5.2.17" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "asgiref" }, { name = "sqlparse" }, { name = "tzdata", marker = "sys_platform == 'win32'" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/a9/26/889449d521ae508b26de715954faecd8bcf3f740affb81b2d146a83b42a5/django-5.2.16.tar.gz", hash = "sha256:59ea02020c3136fce14bef0bbece21a10a4febef5eed1c51c22ae468efa22200", size = 10890894, upload-time = "2026-07-07T13:52:17.005Z" } +sdist = { url = "https://files.pythonhosted.org/packages/d5/d8/43e9d000519adceb189620b6869ff88031e046df91c2e9da72f8f6918399/django-5.2.17.tar.gz", hash = "sha256:9d4d93be539a18ab80d058eb515900e10951e04c537c5a6b394fc49528d3251f", size = 10889740, upload-time = "2026-08-04T15:04:03.173Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/4e/13/1e5e3e4c15dcecb04281b3cb2a46a4670e1cef131068e202f6040df19224/django-5.2.16-py3-none-any.whl", hash = "sha256:04f354bf9d807a86ad1a8392fe3808d362358a8eafc322848e0e43e59b24371d", size = 8311943, upload-time = "2026-07-07T13:52:11.223Z" }, + { url = "https://files.pythonhosted.org/packages/df/f8/ce120525ca78f12b07daf65786679c5d0b54a75285a8958d3ae55e39da35/django-5.2.17-py3-none-any.whl", hash = "sha256:f04fb3b36ee119e1af4fa1d397d5fd6cf12700f49321e84d4f4c642c5b1973db", size = 8315563, upload-time = "2026-08-04T15:03:59.1Z" }, ] [[package]] @@ -2643,7 +2643,7 @@ requires-dist = [ { name = "deepmerge", specifier = ">=2.0,<3" }, { name = "dj-database-url", specifier = ">=3.0.0,<4" }, { name = "dj-static", specifier = ">=0.0.6,<0.0.7" }, - { name = "django", specifier = "==5.2.16" }, + { name = "django", specifier = "==5.2.17" }, { name = "django-anymail", extras = ["mailgun"], specifier = ">=13.0,<14" }, { name = "django-bitfield", specifier = ">=2.2.0,<3" }, { name = "django-cache-memoize", specifier = ">=0.2.0,<0.3" }, From e10bb8e72ee5e76c126ddb64804f9cad7c7ac78d Mon Sep 17 00:00:00 2001 From: Shankar Ambady Date: Thu, 27 Aug 2026 16:37:55 -0400 Subject: [PATCH 03/14] staleness penalty for vector search results (#3834) * adding staleness penalty for resource vector search * update tests --- main/settings.py | 11 ++++ vector_search/constants.py | 15 +++++ vector_search/utils.py | 88 ++++++++++++++++++++++---- vector_search/utils_test.py | 123 ++++++++++++++++++++++++++++++++++++ vector_search/views_test.py | 77 +++++++++++++++++----- 5 files changed, 288 insertions(+), 26 deletions(-) diff --git a/main/settings.py b/main/settings.py index dccd8113aa..44874ee02b 100644 --- a/main/settings.py +++ b/main/settings.py @@ -885,6 +885,17 @@ def get_all_config_keys(): name="VECTOR_SEARCH_INCOMPLETENESS_PENALTY_WEIGHT", default=0.05 ) +# Score subtracted from a resource that is VECTOR_SEARCH_STALENESS_HORIZON_YEARS +# or more old in vector search, ramped linearly by age. 0 disables the penalty. +VECTOR_SEARCH_STALENESS_PENALTY_WEIGHT = get_float( + name="VECTOR_SEARCH_STALENESS_PENALTY_WEIGHT", default=0.05 +) + +# Age at which a resource takes the full VECTOR_SEARCH_STALENESS_PENALTY_WEIGHT; +VECTOR_SEARCH_STALENESS_HORIZON_YEARS = get_float( + name="VECTOR_SEARCH_STALENESS_HORIZON_YEARS", default=20 +) + # serve learning resource search hits from the Qdrant payload instead of # re-hydrating them from the database. Set to False to fall back to database # hydration without a deploy. diff --git a/vector_search/constants.py b/vector_search/constants.py index a22904d4d1..bf551db319 100644 --- a/vector_search/constants.py +++ b/vector_search/constants.py @@ -48,6 +48,18 @@ # collection carries it; content file payloads do not. COMPLETENESS_PAYLOAD_KEY = "completeness" +# Payload key holding the date a resource is considered to have aged from -- the +# start date of its last run, or the last modified date for learning materials. +# Null (or absent) means nothing to penalize: resources with an upcoming run are +# never stale. Set by the search serializer, so only the resources collection +# carries it. +RESOURCE_AGE_DATE_PAYLOAD_KEY = "resource_age_date" + +# Qdrant decay expressions measure the distance between datetimes in seconds, so +# a staleness horizon in years is converted with this. 365 days, the same year +# length the OpenSearch decay's 365d scale uses. +SECONDS_PER_YEAR = 365 * 24 * 60 * 60 + QDRANT_RESOURCE_PARAM_MAP = { "readable_id": "readable_id", "resource_type": "resource_type", @@ -104,6 +116,9 @@ # Not filterable or facetable -- indexed because Qdrant rejects a scoring # formula that reads an unindexed payload key (see COMPLETENESS_PAYLOAD_KEY). COMPLETENESS_PAYLOAD_KEY: models.PayloadSchemaType.FLOAT, + # Scoring-only for the same reason: the staleness penalty decays over it, and + # a datetime index is what makes it readable from a formula. + RESOURCE_AGE_DATE_PAYLOAD_KEY: models.PayloadSchemaType.DATETIME, } diff --git a/vector_search/utils.py b/vector_search/utils.py index d0251419e8..f539f73357 100644 --- a/vector_search/utils.py +++ b/vector_search/utils.py @@ -2,6 +2,7 @@ import gc import logging import uuid +from datetime import UTC, datetime from functools import cache from textwrap import dedent @@ -62,9 +63,11 @@ QDRANT_OPTIMIZER_THRESHOLD_SMALL, QDRANT_RESOURCE_PARAM_MAP, QDRANT_TOPIC_INDEXES, + RESOURCE_AGE_DATE_PAYLOAD_KEY, RESOURCES_COLLECTION_NAME, RESOURCES_PAYLOAD_EXCLUDE, RESOURCES_RETRIEVE_PAYLOAD, + SECONDS_PER_YEAR, TOPICS_COLLECTION_NAME, VECTOR_SEARCH_SCORE_BOOST, ) @@ -1773,24 +1776,87 @@ def completeness_penalty_expression( ) +def staleness_penalty_expression( + collection_name: str, + now: datetime, +) -> models.NegExpression | None: + """ + Build the staleness penalty term: -weight * (1 - decay), where decay ramps + linearly from 1 at `now` down to 0 at VECTOR_SEARCH_STALENESS_HORIZON_YEARS + and stays there, so the penalty grows with age and saturates at the weight. + + Additive rather than the multiplicative decay OpenSearch applies -- see + VECTOR_SEARCH_STALENESS_PENALTY_WEIGHT. None when the penalty is disabled or + the collection has no resource age. + """ + if collection_name != RESOURCES_COLLECTION_NAME: + return None + weight = max(settings.VECTOR_SEARCH_STALENESS_PENALTY_WEIGHT or 0, 0) + horizon_years = settings.VECTOR_SEARCH_STALENESS_HORIZON_YEARS or 0 + if not weight or horizon_years <= 0: + return None + return models.NegExpression( + neg=models.MultExpression( + mult=[ + weight, + models.SumExpression( + sum=[ + 1, + models.NegExpression( + neg=models.LinDecayExpression( + lin_decay=models.DecayParamsExpression( + x=models.DatetimeKeyExpression( + datetime_key=RESOURCE_AGE_DATE_PAYLOAD_KEY + ), + target=models.DatetimeExpression( + datetime=now.isoformat() + ), + scale=horizon_years * SECONDS_PER_YEAR, + # decay reaches 0 -- a full penalty -- at the + # horizon rather than the default half of it + midpoint=0.0, + ) + ) + ), + ] + ), + ] + ) + ) + + def score_formula_query(collection_name: str) -> models.FormulaQuery | None: """ Build a collection's rescoring formula: the score, plus the - VECTOR_SEARCH_SCORE_BOOST boosts, minus the incompleteness penalty. None when - neither applies, so callers can skip rescoring entirely. + VECTOR_SEARCH_SCORE_BOOST boosts, minus the incompleteness and staleness + penalties. None when none of them apply, so callers can skip rescoring + entirely. """ + now = datetime.now(tz=UTC) boost_expressions = custom_score_formula(collection_name) - penalty = completeness_penalty_expression(collection_name) - if not boost_expressions and penalty is None: + penalties = [] + # Payload values to fall back on, so that a point missing one -- indexed + # before the key existed, or a resource type that never carries it -- is + # penalized for neither. + defaults = {} + + completeness_penalty = completeness_penalty_expression(collection_name) + if completeness_penalty is not None: + penalties.append(completeness_penalty) + defaults[COMPLETENESS_PAYLOAD_KEY] = 1.0 + + staleness_penalty = staleness_penalty_expression(collection_name, now) + if staleness_penalty is not None: + penalties.append(staleness_penalty) + # A null resource_age_date means an upcoming run, which is not stale, and + # scores as if it were published right now. + defaults[RESOURCE_AGE_DATE_PAYLOAD_KEY] = now.isoformat() + + if not boost_expressions and not penalties: return None - terms = ["$score", *boost_expressions] - if penalty is None: - return models.FormulaQuery(formula=models.SumExpression(sum=terms)) return models.FormulaQuery( - formula=models.SumExpression(sum=[*terms, penalty]), - # Points indexed before completeness was added to the payload, and any - # resource type that does not carry it, score as fully complete. - defaults={COMPLETENESS_PAYLOAD_KEY: 1.0}, + formula=models.SumExpression(sum=["$score", *boost_expressions, *penalties]), + defaults=defaults, ) diff --git a/vector_search/utils_test.py b/vector_search/utils_test.py index 0207b5b317..87a8f7c4e4 100644 --- a/vector_search/utils_test.py +++ b/vector_search/utils_test.py @@ -1,5 +1,6 @@ import asyncio import random +from datetime import UTC, datetime from decimal import Decimal from unittest.mock import MagicMock @@ -8,6 +9,7 @@ from django.conf import settings from django.contrib.auth.models import Group from django.urls import reverse +from freezegun import freeze_time from langchain_core.documents import Document from qdrant_client import models from qdrant_client.http.models.models import CountResult @@ -57,9 +59,11 @@ QDRANT_OPTIMIZER_THRESHOLD_MEDIUM, QDRANT_OPTIMIZER_THRESHOLD_SMALL, QDRANT_RESOURCE_PARAM_MAP, + RESOURCE_AGE_DATE_PAYLOAD_KEY, RESOURCES_COLLECTION_NAME, RESOURCES_PAYLOAD_EXCLUDE, RESOURCES_RETRIEVE_PAYLOAD, + SECONDS_PER_YEAR, ) from vector_search.encoders.utils import dense_encoder, sparse_encoder from vector_search.utils import ( @@ -88,6 +92,7 @@ score_formula_query, should_generate_content_embeddings, should_generate_resource_embeddings, + staleness_penalty_expression, update_content_file_payload, update_learning_resource_payload, update_qdrant_indexes, @@ -2963,6 +2968,7 @@ def test_completeness_penalty_expression_other_collections(settings): def test_score_formula_query_combines_boosts_and_penalty(mocker, settings): """Boosts add to the score and the penalty subtracts from it.""" settings.VECTOR_SEARCH_INCOMPLETENESS_PENALTY_WEIGHT = 0.05 + settings.VECTOR_SEARCH_STALENESS_PENALTY_WEIGHT = 0 mocker.patch( "vector_search.utils.VECTOR_SEARCH_SCORE_BOOST", {RESOURCES_COLLECTION_NAME: [{"boost": 0.15, "params": {"free": True}}]}, @@ -2980,6 +2986,7 @@ def test_score_formula_query_combines_boosts_and_penalty(mocker, settings): def test_score_formula_query_penalty_only(mocker, settings): """With no boosts configured the formula is the score minus the penalty.""" settings.VECTOR_SEARCH_INCOMPLETENESS_PENALTY_WEIGHT = 0.05 + settings.VECTOR_SEARCH_STALENESS_PENALTY_WEIGHT = 0 mocker.patch("vector_search.utils.VECTOR_SEARCH_SCORE_BOOST", {}) formula_query = score_formula_query(RESOURCES_COLLECTION_NAME) @@ -2992,6 +2999,7 @@ def test_score_formula_query_penalty_only(mocker, settings): def test_score_formula_query_boosts_only(mocker, settings): """With the penalty disabled the formula keeps the boosts and no defaults.""" settings.VECTOR_SEARCH_INCOMPLETENESS_PENALTY_WEIGHT = 0 + settings.VECTOR_SEARCH_STALENESS_PENALTY_WEIGHT = 0 mocker.patch( "vector_search.utils.VECTOR_SEARCH_SCORE_BOOST", {RESOURCES_COLLECTION_NAME: [{"boost": 0.15, "params": {"free": True}}]}, @@ -3005,6 +3013,121 @@ def test_score_formula_query_boosts_only(mocker, settings): assert isinstance(boost, models.MultExpression) +def test_staleness_penalty_expression(settings): + """The penalty decays linearly over resource_age_date, from now.""" + settings.VECTOR_SEARCH_STALENESS_PENALTY_WEIGHT = 0.05 + settings.VECTOR_SEARCH_STALENESS_HORIZON_YEARS = 20 + now = datetime(2026, 1, 1, tzinfo=UTC) + + expression = staleness_penalty_expression(RESOURCES_COLLECTION_NAME, now) + + assert isinstance(expression, models.NegExpression) + weight, staleness = expression.neg.mult + assert weight == 0.05 + # 1 - decay + assert staleness.sum[0] == 1 + decay = staleness.sum[1].neg.lin_decay + assert decay.x.datetime_key == RESOURCE_AGE_DATE_PAYLOAD_KEY + assert decay.target.datetime == now.isoformat() + assert decay.scale == 20 * SECONDS_PER_YEAR + # decay bottoms out at the horizon rather than halfway to it + assert decay.midpoint == 0.0 + + +@pytest.mark.parametrize("age_years", [0, 5, 20, 40]) +def test_staleness_penalty_ramps_linearly_to_the_horizon(settings, age_years): + """ + The emitted decay params subtract weight * age / horizon, saturating at the + weight once a resource is at least a horizon old. + """ + settings.VECTOR_SEARCH_STALENESS_PENALTY_WEIGHT = 0.05 + settings.VECTOR_SEARCH_STALENESS_HORIZON_YEARS = 20 + now = datetime(2026, 1, 1, tzinfo=UTC) + + expression = staleness_penalty_expression(RESOURCES_COLLECTION_NAME, now) + weight, staleness = expression.neg.mult + decay_params = staleness.sum[1].neg.lin_decay + + # Qdrant's linear decay, evaluated for a resource of this age + age_seconds = age_years * SECONDS_PER_YEAR + decay = max(0, 1 - (1 - decay_params.midpoint) * age_seconds / decay_params.scale) + penalty = weight * (1 - decay) + + assert penalty == pytest.approx(0.05 * min(age_years / 20, 1)) + + +@pytest.mark.parametrize("weight", [0, None, -1]) +def test_staleness_penalty_expression_disabled(settings, weight): + """A weight of 0, unset, or negative leaves scores alone.""" + settings.VECTOR_SEARCH_STALENESS_PENALTY_WEIGHT = weight + + assert ( + staleness_penalty_expression(RESOURCES_COLLECTION_NAME, datetime.now(tz=UTC)) + is None + ) + + +@pytest.mark.parametrize("horizon_years", [0, None, -1]) +def test_staleness_penalty_expression_without_horizon(settings, horizon_years): + """A horizon of 0, unset, or negative has no ramp to penalize along.""" + settings.VECTOR_SEARCH_STALENESS_PENALTY_WEIGHT = 0.05 + settings.VECTOR_SEARCH_STALENESS_HORIZON_YEARS = horizon_years + + assert ( + staleness_penalty_expression(RESOURCES_COLLECTION_NAME, datetime.now(tz=UTC)) + is None + ) + + +def test_staleness_penalty_expression_other_collections(settings): + """Only resource payloads carry an age date, so only they are penalized.""" + settings.VECTOR_SEARCH_STALENESS_PENALTY_WEIGHT = 0.05 + + assert ( + staleness_penalty_expression( + CONTENT_FILES_COLLECTION_NAME, datetime.now(tz=UTC) + ) + is None + ) + + +def test_score_formula_query_combines_both_penalties(mocker, settings): + """Incompleteness and staleness both subtract from the score.""" + settings.VECTOR_SEARCH_INCOMPLETENESS_PENALTY_WEIGHT = 0.05 + settings.VECTOR_SEARCH_STALENESS_PENALTY_WEIGHT = 0.05 + mocker.patch("vector_search.utils.VECTOR_SEARCH_SCORE_BOOST", {}) + now = datetime(2026, 1, 1, tzinfo=UTC) + + with freeze_time(now): + formula_query = score_formula_query(RESOURCES_COLLECTION_NAME) + + score, completeness_penalty, staleness = formula_query.formula.sum + assert score == "$score" + assert completeness_penalty == completeness_penalty_expression( + RESOURCES_COLLECTION_NAME + ) + assert staleness == staleness_penalty_expression(RESOURCES_COLLECTION_NAME, now) + # a resource with no age date is not stale, and scores as if published now + assert formula_query.defaults == { + COMPLETENESS_PAYLOAD_KEY: 1.0, + RESOURCE_AGE_DATE_PAYLOAD_KEY: now.isoformat(), + } + + +def test_score_formula_query_staleness_penalty_only(mocker, settings): + """With incompleteness disabled, only the age date needs a default.""" + settings.VECTOR_SEARCH_INCOMPLETENESS_PENALTY_WEIGHT = 0 + settings.VECTOR_SEARCH_STALENESS_PENALTY_WEIGHT = 0.05 + mocker.patch("vector_search.utils.VECTOR_SEARCH_SCORE_BOOST", {}) + + formula_query = score_formula_query(RESOURCES_COLLECTION_NAME) + + assert list(formula_query.defaults) == [RESOURCE_AGE_DATE_PAYLOAD_KEY] + score, staleness = formula_query.formula.sum + assert score == "$score" + assert isinstance(staleness.neg.mult[1].sum[1].neg, models.LinDecayExpression) + + def test_score_formula_query_nothing_to_apply(mocker, settings): """Nothing to boost and nothing to penalize means no rescoring stage.""" settings.VECTOR_SEARCH_INCOMPLETENESS_PENALTY_WEIGHT = 0.05 diff --git a/vector_search/views_test.py b/vector_search/views_test.py index d25a0ffb79..9aa099117a 100644 --- a/vector_search/views_test.py +++ b/vector_search/views_test.py @@ -18,9 +18,11 @@ from vector_search.constants import ( COMPLETENESS_PAYLOAD_KEY, CONTENT_FILES_RETRIEVE_PAYLOAD, + RESOURCE_AGE_DATE_PAYLOAD_KEY, RESOURCES_COLLECTION_NAME, RESOURCES_PAYLOAD_EXCLUDE, RESOURCES_RETRIEVE_PAYLOAD, + SECONDS_PER_YEAR, ) from vector_search.encoders.utils import dense_encoder, sparse_encoder from vector_search.utils import score_formula_query @@ -847,17 +849,28 @@ def test_vector_search_with_score_cutoff_enforces_min_score( ) -def _completeness_penalty(formula_query): - """Pull the completeness penalty term out of a resource score formula.""" +def _penalty(formula_query): + """Pull the trailing penalty term out of a resource score formula.""" return formula_query.formula.sum[-1] +def _formula_queries(call_kwargs, hybrid_search): + """Return the score formulas a query_points call rescores with.""" + if hybrid_search: + # One rescored prefetch per vector arm, fused afterwards + assert isinstance(call_kwargs["query"], models.FusionQuery) + return [prefetch.query for prefetch in call_kwargs["prefetch"]] + assert call_kwargs["prefetch"].using == dense_encoder().model_short_name() + return [call_kwargs["query"]] + + @pytest.mark.parametrize("hybrid_search", [True, False]) def test_vector_search_applies_completeness_penalty( mocker, client, settings, hybrid_search ): """Both search modes must rescore resources with the completeness penalty.""" settings.VECTOR_SEARCH_INCOMPLETENESS_PENALTY_WEIGHT = 0.05 + settings.VECTOR_SEARCH_STALENESS_PENALTY_WEIGHT = 0 mock_qdrant = mocker.patch( "qdrant_client.AsyncQdrantClient", return_value=mocker.AsyncMock() @@ -874,23 +887,55 @@ def test_vector_search_applies_completeness_penalty( ) call_kwargs = mock_qdrant.query_points.mock_calls[0].kwargs - expected_penalty = _completeness_penalty( - score_formula_query(RESOURCES_COLLECTION_NAME) + expected_penalty = _penalty(score_formula_query(RESOURCES_COLLECTION_NAME)) + formula_queries = _formula_queries(call_kwargs, hybrid_search) + + assert formula_queries + for formula_query in formula_queries: + assert isinstance(formula_query, models.FormulaQuery) + assert formula_query.defaults == {COMPLETENESS_PAYLOAD_KEY: 1.0} + assert _penalty(formula_query) == expected_penalty + + +@pytest.mark.parametrize("hybrid_search", [True, False]) +def test_vector_search_applies_staleness_penalty( + mocker, client, settings, hybrid_search +): + """Both search modes must rescore resources with the staleness penalty.""" + settings.VECTOR_SEARCH_INCOMPLETENESS_PENALTY_WEIGHT = 0 + settings.VECTOR_SEARCH_STALENESS_PENALTY_WEIGHT = 0.05 + + mock_qdrant = mocker.patch( + "qdrant_client.AsyncQdrantClient", return_value=mocker.AsyncMock() + )() + mock_result = mocker.MagicMock() + mock_result.points = [] + mock_qdrant.query_points = mocker.AsyncMock(return_value=mock_result) + mock_qdrant.scroll = mocker.AsyncMock(return_value=([], None)) + mocker.patch("vector_search.views.async_qdrant_client", return_value=mock_qdrant) + + client.get( + reverse("vector_search:v0:vector_learning_resources_search"), + data={"q": "test", "hybrid_search": hybrid_search}, ) - if hybrid_search: - # One rescored prefetch per vector arm, fused afterwards - assert isinstance(call_kwargs["query"], models.FusionQuery) - formula_queries = [prefetch.query for prefetch in call_kwargs["prefetch"]] - else: - formula_queries = [call_kwargs["query"]] - assert call_kwargs["prefetch"].using == dense_encoder().model_short_name() + formula_queries = _formula_queries( + mock_qdrant.query_points.mock_calls[0].kwargs, hybrid_search + ) assert formula_queries for formula_query in formula_queries: assert isinstance(formula_query, models.FormulaQuery) - assert formula_query.defaults == {COMPLETENESS_PAYLOAD_KEY: 1.0} - assert _completeness_penalty(formula_query) == expected_penalty + # resources with no age date -- those with an upcoming run -- are scored + # as if published at query time, so they take no penalty + assert list(formula_query.defaults) == [RESOURCE_AGE_DATE_PAYLOAD_KEY] + weight, staleness = _penalty(formula_query).neg.mult + assert weight == settings.VECTOR_SEARCH_STALENESS_PENALTY_WEIGHT + decay = staleness.sum[1].neg.lin_decay + assert decay.x.datetime_key == RESOURCE_AGE_DATE_PAYLOAD_KEY + assert decay.scale == ( + settings.VECTOR_SEARCH_STALENESS_HORIZON_YEARS * SECONDS_PER_YEAR + ) def test_dense_vector_search_without_formula_queries_vectors_directly( @@ -898,6 +943,7 @@ def test_dense_vector_search_without_formula_queries_vectors_directly( ): """With nothing to rescore, dense search skips the prefetch entirely.""" settings.VECTOR_SEARCH_INCOMPLETENESS_PENALTY_WEIGHT = 0 + settings.VECTOR_SEARCH_STALENESS_PENALTY_WEIGHT = 0 mocker.patch("vector_search.utils.VECTOR_SEARCH_SCORE_BOOST", {}) mock_qdrant = mocker.patch( @@ -921,11 +967,12 @@ def test_dense_vector_search_without_formula_queries_vectors_directly( @pytest.mark.django_db(transaction=True) -def test_content_file_search_has_no_completeness_penalty( +def test_content_file_search_has_no_resource_penalties( mocker, client, settings, content_file_viewer ): - """Content file payloads carry no completeness, so nothing is penalized.""" + """Content file payloads carry neither completeness nor an age date.""" settings.VECTOR_SEARCH_INCOMPLETENESS_PENALTY_WEIGHT = 0.05 + settings.VECTOR_SEARCH_STALENESS_PENALTY_WEIGHT = 0.05 mock_qdrant = mocker.patch( "qdrant_client.AsyncQdrantClient", return_value=mocker.AsyncMock() From aeb8a6b0c7c385ce8b32826125be91aee5468cea Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Fri, 28 Aug 2026 09:23:46 -0400 Subject: [PATCH 04/14] Show Stay Updated based only on the CMS page flag (#3841) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Stay Updated button was gated on three things: the NEXT_PUBLIC_STAY_UPDATED_HUBSPOT_FORM_ID env var, the CMS page's show_stay_updated flag, and an enrollment-mode check requiring every enrollment mode to be "verified". The mode check was doing work the CMS flag already covers, and on courses it iterated course.courseruns unfiltered — including runs with live: false / is_enrollable: false that appear nowhere else on the page (the session selector filters to is_enrollable). One retired audit run therefore suppressed the button for the whole course. Example: course-v1:UAI_SOURCE+UAI.MLTL.1 has show_stay_updated: true and one enrollable verified-only run, but a second run (id 2465, live: false, is_enrollable: false) carries an "audit" mode, so the button never rendered. Visibility is now just: can it be shown (env var, checked in ProductPageTemplate) and should it be shown (page.show_stay_updated). Tests for each page collapse to those branches; the shared PROGRAM_HIDE_STAY_UPDATED_CASES only served the removed mode permutations. The env-var tests now set show_stay_updated explicitly — the page factory randomizes it, so without the mode clause they would have been coin-flips. Co-authored-by: Claude Opus 5 (1M context) --- .../ProductPages/CoursePage.test.tsx | 70 ++++--------------- .../src/app-pages/ProductPages/CoursePage.tsx | 13 +--- .../ProductPages/ProgramAsCoursePage.test.tsx | 56 ++++++--------- .../ProductPages/ProgramAsCoursePage.tsx | 9 +-- .../ProductPages/ProgramPage.test.tsx | 53 ++++++-------- .../app-pages/ProductPages/ProgramPage.tsx | 9 +-- .../ProductPages/test-utils/stayUpdated.ts | 29 -------- 7 files changed, 58 insertions(+), 181 deletions(-) diff --git a/frontends/main/src/app-pages/ProductPages/CoursePage.test.tsx b/frontends/main/src/app-pages/ProductPages/CoursePage.test.tsx index cb47b28782..f675da8b11 100644 --- a/frontends/main/src/app-pages/ProductPages/CoursePage.test.tsx +++ b/frontends/main/src/app-pages/ProductPages/CoursePage.test.tsx @@ -677,16 +677,8 @@ describe("CoursePage", () => { describe("Stay Updated button", () => { useStayUpdatedEnv() - test("Shows button when all course runs have only the verified enrollment mode", async () => { - const verifiedMode = mitxFactories.courses.enrollmentMode({ - mode_slug: "verified", - }) - const course = makeCourse({ - courseruns: [ - mitxFactories.courses.courseRun({ enrollment_modes: [verifiedMode] }), - mitxFactories.courses.courseRun({ enrollment_modes: [verifiedMode] }), - ], - }) + test("Shows button when the page enables Stay Updated", async () => { + const course = makeCourse() const page = makePage({ course_details: course, show_stay_updated: true, @@ -699,46 +691,12 @@ describe("CoursePage", () => { ).toBeInTheDocument() }) - test.each([ - { - label: "one run has a non-verified mode", - buildRuns: () => [ - mitxFactories.courses.courseRun({ - enrollment_modes: [ - mitxFactories.courses.enrollmentMode({ mode_slug: "verified" }), - ], - }), - mitxFactories.courses.courseRun({ - enrollment_modes: [ - mitxFactories.courses.enrollmentMode({ mode_slug: "audit" }), - ], - }), - ], - }, - { - label: "a run has mixed verified and non-verified modes", - buildRuns: () => [ - mitxFactories.courses.courseRun({ - enrollment_modes: [ - mitxFactories.courses.enrollmentMode({ mode_slug: "verified" }), - mitxFactories.courses.enrollmentMode({ mode_slug: "audit" }), - ], - }), - ], - }, - { - label: "a run has no enrollment modes", - buildRuns: () => [ - mitxFactories.courses.courseRun({ enrollment_modes: [] }), - ], - }, - { - label: "the course has no runs", - buildRuns: () => [], - }, - ])("Hides button when $label", async ({ buildRuns }) => { - const course = makeCourse({ courseruns: buildRuns() }) - const page = makePage({ course_details: course }) + test("Hides button when the page disables Stay Updated", async () => { + const course = makeCourse() + const page = makePage({ + course_details: course, + show_stay_updated: false, + }) setupApis({ course, page }) renderWithProviders() @@ -750,15 +708,11 @@ describe("CoursePage", () => { test("Hides button when Stay Updated form ID is not configured", async () => { delete process.env.NEXT_PUBLIC_STAY_UPDATED_HUBSPOT_FORM_ID - const verifiedMode = mitxFactories.courses.enrollmentMode({ - mode_slug: "verified", - }) - const course = makeCourse({ - courseruns: [ - mitxFactories.courses.courseRun({ enrollment_modes: [verifiedMode] }), - ], + const course = makeCourse() + const page = makePage({ + course_details: course, + show_stay_updated: true, }) - const page = makePage({ course_details: course }) setupApis({ course, page }) renderWithProviders() diff --git a/frontends/main/src/app-pages/ProductPages/CoursePage.tsx b/frontends/main/src/app-pages/ProductPages/CoursePage.tsx index 99d695f094..f408f66de1 100644 --- a/frontends/main/src/app-pages/ProductPages/CoursePage.tsx +++ b/frontends/main/src/app-pages/ProductPages/CoursePage.tsx @@ -18,7 +18,6 @@ import ProductPageTemplate from "./ProductPageTemplate" import WhatYoullLearnSection from "./WhatYoullLearnSection" import HowYoullLearnSection from "./HowYoullLearnSection" import { DEFAULT_RESOURCE_IMG } from "ol-utilities" -import { isVerifiedEnrollmentMode } from "@/common/mitxonline" import CourseInfoBox from "./InfoBoxCourse" import CourseOutlineSection from "./CourseOutlineSection" import { @@ -128,17 +127,7 @@ const CoursePage: React.FC = ({ readableId }) => { enrollmentAction={ } - showStayUpdated={ - course.courseruns.length > 0 && - (page.show_stay_updated ?? false) && - course.courseruns.every( - (run) => - run.enrollment_modes.length > 0 && - run.enrollment_modes.every((mode) => - isVerifiedEnrollmentMode(mode.mode_slug), - ), - ) - } + showStayUpdated={page.show_stay_updated ?? false} resource={{ readable_id: course.readable_id, resource_type: "course", diff --git a/frontends/main/src/app-pages/ProductPages/ProgramAsCoursePage.test.tsx b/frontends/main/src/app-pages/ProductPages/ProgramAsCoursePage.test.tsx index a641dfb798..af57c2661d 100644 --- a/frontends/main/src/app-pages/ProductPages/ProgramAsCoursePage.test.tsx +++ b/frontends/main/src/app-pages/ProductPages/ProgramAsCoursePage.test.tsx @@ -26,10 +26,7 @@ import { import { assertHeadings } from "ol-test-utilities" import ProgramAsCoursePage from "./ProgramAsCoursePage" import { notFound } from "next/navigation" -import { - useStayUpdatedEnv, - PROGRAM_HIDE_STAY_UPDATED_CASES, -} from "./test-utils/stayUpdated" +import { useStayUpdatedEnv } from "./test-utils/stayUpdated" import invariant from "tiny-invariant" import { getIdsFromReqTree } from "@/common/mitxonline" @@ -342,12 +339,8 @@ describe("ProgramAsCoursePage", () => { describe("Stay Updated button", () => { useStayUpdatedEnv() - test("Shows button when program has only the verified enrollment mode", async () => { - const program = makeProgramAsCourse({ - enrollment_modes: [ - factories.courses.enrollmentMode({ mode_slug: "verified" }), - ], - }) + test("Shows button when the page enables Stay Updated", async () => { + const program = makeProgramAsCourse() const page = makePage({ program_details: program, show_stay_updated: true, @@ -362,33 +355,30 @@ describe("ProgramAsCoursePage", () => { ).toBeInTheDocument() }) - test.each(PROGRAM_HIDE_STAY_UPDATED_CASES)( - "Hides button when $label", - async ({ enrollment_modes: enrollmentModes }) => { - const program = makeProgramAsCourse({ - enrollment_modes: enrollmentModes, - }) - const page = makePage({ program_details: program }) - setupApis({ program, page }) - renderWithProviders( - , - ) - - await screen.findByRole("heading", { name: page.title }) - expect( - screen.queryByRole("button", { name: "Stay Updated" }), - ).not.toBeInTheDocument() - }, - ) + test("Hides button when the page disables Stay Updated", async () => { + const program = makeProgramAsCourse() + const page = makePage({ + program_details: program, + show_stay_updated: false, + }) + setupApis({ program, page }) + renderWithProviders( + , + ) + + await screen.findByRole("heading", { name: page.title }) + expect( + screen.queryByRole("button", { name: "Stay Updated" }), + ).not.toBeInTheDocument() + }) test("Hides button when Stay Updated form ID is not configured", async () => { delete process.env.NEXT_PUBLIC_STAY_UPDATED_HUBSPOT_FORM_ID - const program = makeProgramAsCourse({ - enrollment_modes: [ - factories.courses.enrollmentMode({ mode_slug: "verified" }), - ], + const program = makeProgramAsCourse() + const page = makePage({ + program_details: program, + show_stay_updated: true, }) - const page = makePage({ program_details: program }) setupApis({ program, page }) renderWithProviders( , diff --git a/frontends/main/src/app-pages/ProductPages/ProgramAsCoursePage.tsx b/frontends/main/src/app-pages/ProductPages/ProgramAsCoursePage.tsx index cbef8148c5..ca82abbd31 100644 --- a/frontends/main/src/app-pages/ProductPages/ProgramAsCoursePage.tsx +++ b/frontends/main/src/app-pages/ProductPages/ProgramAsCoursePage.tsx @@ -9,7 +9,6 @@ import { programsQueries } from "api/mitxonline-hooks/programs" import { notFound } from "next/navigation" import { HeadingIds, parseReqTree } from "./util" import type { RequirementItem } from "./util" -import { isVerifiedEnrollmentMode } from "@/common/mitxonline" import useReqTreeChildren from "./useReqTreeChildren" import InstructorsSection from "./InstructorsSection" import RawHTML from "./RawHTML" @@ -242,13 +241,7 @@ const ProgramAsCoursePage: React.FC = ({ enrollmentAction={ } - showStayUpdated={ - program.enrollment_modes.length > 0 && - (page.show_stay_updated ?? false) && - program.enrollment_modes.every((mode) => - isVerifiedEnrollmentMode(mode.mode_slug), - ) - } + showStayUpdated={page.show_stay_updated ?? false} resource={{ readable_id: program.readable_id, resource_type: "program", diff --git a/frontends/main/src/app-pages/ProductPages/ProgramPage.test.tsx b/frontends/main/src/app-pages/ProductPages/ProgramPage.test.tsx index 3b73a391aa..1d61c71622 100644 --- a/frontends/main/src/app-pages/ProductPages/ProgramPage.test.tsx +++ b/frontends/main/src/app-pages/ProductPages/ProgramPage.test.tsx @@ -28,10 +28,7 @@ import { reqTreeChildQueries } from "./useReqTreeChildren" import { TestIds } from "./ProductSummary" import { assertHeadings, allowConsoleErrors } from "ol-test-utilities" import { notFound } from "next/navigation" -import { - useStayUpdatedEnv, - PROGRAM_HIDE_STAY_UPDATED_CASES, -} from "./test-utils/stayUpdated" +import { useStayUpdatedEnv } from "./test-utils/stayUpdated" import { usePostHog } from "posthog-js/react" import invariant from "tiny-invariant" @@ -903,13 +900,8 @@ describe("ProgramPage", () => { describe("Stay Updated button", () => { useStayUpdatedEnv() - test("Shows button when program has only the verified enrollment mode", async () => { - const program = makeProgram({ - ...makeReqs(), - enrollment_modes: [ - factories.courses.enrollmentMode({ mode_slug: "verified" }), - ], - }) + test("Shows button when the page enables Stay Updated", async () => { + const program = makeProgram(makeReqs()) const page = makePage({ program_details: program, show_stay_updated: true, @@ -922,33 +914,28 @@ describe("ProgramPage", () => { ).toBeInTheDocument() }) - test.each(PROGRAM_HIDE_STAY_UPDATED_CASES)( - "Hides button when $label", - async ({ enrollment_modes: enrollmentModes }) => { - const program = makeProgram({ - ...makeReqs(), - enrollment_modes: enrollmentModes, - }) - const page = makePage({ program_details: program }) - setupApis({ program, page }) - renderWithProviders() + test("Hides button when the page disables Stay Updated", async () => { + const program = makeProgram(makeReqs()) + const page = makePage({ + program_details: program, + show_stay_updated: false, + }) + setupApis({ program, page }) + renderWithProviders() - await screen.findByRole("heading", { name: page.title }) - expect( - screen.queryByRole("button", { name: "Stay Updated" }), - ).not.toBeInTheDocument() - }, - ) + await screen.findByRole("heading", { name: page.title }) + expect( + screen.queryByRole("button", { name: "Stay Updated" }), + ).not.toBeInTheDocument() + }) test("Hides button when Stay Updated form ID is not configured", async () => { delete process.env.NEXT_PUBLIC_STAY_UPDATED_HUBSPOT_FORM_ID - const program = makeProgram({ - ...makeReqs(), - enrollment_modes: [ - factories.courses.enrollmentMode({ mode_slug: "verified" }), - ], + const program = makeProgram(makeReqs()) + const page = makePage({ + program_details: program, + show_stay_updated: true, }) - const page = makePage({ program_details: program }) setupApis({ program, page }) renderWithProviders() diff --git a/frontends/main/src/app-pages/ProductPages/ProgramPage.tsx b/frontends/main/src/app-pages/ProductPages/ProgramPage.tsx index 46b5c356c1..d30b564cef 100644 --- a/frontends/main/src/app-pages/ProductPages/ProgramPage.tsx +++ b/frontends/main/src/app-pages/ProductPages/ProgramPage.tsx @@ -10,7 +10,6 @@ import { programsQueries } from "api/mitxonline-hooks/programs" import { notFound } from "next/navigation" import { HeadingIds, parseReqTree, RequirementData } from "./util" import useReqTreeChildren from "./useReqTreeChildren" -import { isVerifiedEnrollmentMode } from "@/common/mitxonline" import InstructorsSection from "./InstructorsSection" import RawHTML from "./RawHTML" import UnstyledRawHTML from "@/components/UnstyledRawHTML/UnstyledRawHTML" @@ -268,13 +267,7 @@ const ProgramPage: React.FC = ({ readableId }) => { imageSrc={imageSrc} videoUrl={page.video_url} enrollmentAction={} - showStayUpdated={ - program.enrollment_modes.length > 0 && - (page.show_stay_updated ?? false) && - program.enrollment_modes.every((mode) => - isVerifiedEnrollmentMode(mode.mode_slug), - ) - } + showStayUpdated={page.show_stay_updated ?? false} resource={{ readable_id: program.readable_id, resource_type: "program", diff --git a/frontends/main/src/app-pages/ProductPages/test-utils/stayUpdated.ts b/frontends/main/src/app-pages/ProductPages/test-utils/stayUpdated.ts index e070b06152..f05c4059d9 100644 --- a/frontends/main/src/app-pages/ProductPages/test-utils/stayUpdated.ts +++ b/frontends/main/src/app-pages/ProductPages/test-utils/stayUpdated.ts @@ -1,5 +1,3 @@ -import { factories } from "api/mitxonline-test-utils" -import type { EnrollmentMode } from "@mitodl/mitxonline-api-axios/v2" import { faker } from "@faker-js/faker/locale/en" export const STAY_UPDATED_FORM_ID = faker.string.uuid() @@ -24,30 +22,3 @@ export const useStayUpdatedEnv = () => { } }) } - -/** - * Shared test.each cases for the "Stay Updated button is hidden" scenarios on - * program-level enrollment_modes (used by ProgramPage and ProgramAsCoursePage). - */ -export const PROGRAM_HIDE_STAY_UPDATED_CASES: { - label: string - enrollment_modes: EnrollmentMode[] -}[] = [ - { - label: "program has a non-verified enrollment mode", - enrollment_modes: [ - factories.courses.enrollmentMode({ mode_slug: "audit" }), - ], - }, - { - label: "program has mixed verified and non-verified modes", - enrollment_modes: [ - factories.courses.enrollmentMode({ mode_slug: "verified" }), - factories.courses.enrollmentMode({ mode_slug: "audit" }), - ], - }, - { - label: "program has no enrollment modes", - enrollment_modes: [], - }, -] From bf2da2eee9fee4069983c0d4797b0f2cfb232a78 Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Fri, 28 Aug 2026 10:48:05 -0400 Subject: [PATCH 05/14] Overridable Mutation Error Toast (#3837) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add overridable mutation error-toast infrastructure (Option 2) Global MutationCache.onError shows a top-center error toast for every browser mutation failure, so a failed mutation is never silent. Call sites override via typed `meta` (declared in api/mutation-meta): - `showErrorToast: false` to opt out (e.g. renders its own inline error) - `errorMessage` / `getErrorMessage(error, variables)` for custom copy `api` stays UI-free (declares typed `meta` only); the toast lives in `main`. Copy resolution is defensive: a throwing getErrorMessage or empty copy falls back to the generic message inside onError, so a bad override can never make the failure silent again. The store holds one toast; a newer error replaces it (never stacks), and it persists until dismissed via smoot Alert's own close control. The convention is documented in .github/instructions/frontend.instructions.md. Not yet shippable: sites that already render inline alerts will double-fire until opted out (follow-up migration). Co-Authored-By: Claude Opus 4.8 Co-Authored-By: Claude Fable 5 * Opt out surfaced inline-error sites from the global error toast The global MutationCache.onError toast (previous commit) fires on every browser mutation failure. Sites that already render their own co-located inline error would therefore double-fire (inline alert + toast). Give each shared api/ mutation hook an optional `meta` param (MutationHookOptions, exported from api/mutation-meta) forwarded to useMutation, and have every consumer that renders an inline error pass `meta: SILENCE_ERROR_TOAST`. Opt-out lives at the consumer, never baked into the shared hook, because several hooks (enrollment, baskets) are used in both a surfaced and a silent context. UpgradeBanner opts out only when its onUpgradeFailure callback is wired — without it the banner has no error surface of its own, so the toast must stay. Also mount in renderWithProviders and reset the toast store per test, so a missing opt-out at an inline-error site surfaces as a double-role="alert" test failure rather than only showing up in manual QA, and add end-to-end coverage (mutationErrorToast.test.tsx) that the default toast fires and SILENCE_ERROR_TOAST suppresses it — so removing the MutationCache wiring can no longer pass green. Adopting the genuinely-silent sites (Start button, destroy dialogs, listItemMove rollback) with tailored copy is a separate follow-up; those already toast via the global default. Co-Authored-By: Claude Opus 4.8 Co-Authored-By: Claude Fable 5 * Strengthen consumer error-path tests around the toast opt-outs Convert text-based error assertions to singular role queries (findByRole("alert")) in the CourseEnrollmentDialog and JustInTimeDialog tests, so a stray global toast — a missing opt-out, or a shared hook that stops forwarding `meta` — fails these tests as a second alert. Add failing-mutation coverage for all three DashboardDialogs (email settings, unenroll, unenroll program), whose opt-outs and inline error rendering were previously untested. Writing those tests surfaced that each dialog awaited mutateAsync inside formik's onSubmit: the rejection escaped onSubmit (formik logs an unhandled-error warning), and the post-await isError guards were unreachable on the error path. Switch to mutate(..., { onSuccess }) — same success/error behavior (inline alert via isError, busy state via isPending), no escaping rejection. Co-Authored-By: Claude Opus 4.8 Co-Authored-By: Claude Fable 5 * Fail any test that leaves the global error toast unacknowledged The double-alert test net only tripped where an error-path test happened to assert via a singular role="alert" query. Make it structural: an afterEach in setupJest fails any test that ends with the global error toast still showing, and its message teaches the fix — opt the component out with `meta: SILENCE_ERROR_TOAST` if it renders its own inline error, or acknowledge the toast with the new `expectErrorToast(message)` helper if the toast is the intended error surface. Every mutation failure a test drives now requires a deliberate decision about its error surface. The full main suite passes with zero acknowledgment churn — no existing test drives a toasting failure it doesn't already account for. Co-Authored-By: Claude Fable 5 * Add tailored enroll error copy as errorMessage PoC The dashboard enroll flow (useEnrollmentHandler) becomes the first adopter of meta.errorMessage: a failed enroll toasts course- or program-specific copy instead of the generic message, demonstrating the per-site override. Co-Authored-By: Claude Opus 4.8 * Fall back to errorMessage when getErrorMessage throws or returns blank resolveErrorMessage collapsed the dynamic and static tiers into one nullish-coalescing expression, so a getErrorMessage that threw or returned blank copy skipped a defined errorMessage and jumped straight to the generic fallback. Evaluate the tiers separately so each falls through to the next when it yields no message. Flagged in review: https://github.com/mitodl/mit-learn/pull/3837#discussion_r3856680236 Co-Authored-By: Claude Fable 5 --------- Co-authored-by: Claude Opus 4.8 --- .../frontend-tests.instructions.md | 12 ++ .github/instructions/frontend.instructions.md | 8 ++ frontends/api/package.json | 1 + frontends/api/src/hooks/hubspot/index.ts | 4 +- .../api/src/hooks/learningPaths/index.ts | 7 +- frontends/api/src/hooks/unsubscribe/index.ts | 4 +- frontends/api/src/hooks/userLists/index.ts | 7 +- .../api/src/hooks/website_content/index.ts | 10 +- .../api/src/mitxonline/hooks/baskets/index.ts | 7 +- .../src/mitxonline/hooks/enrollment/index.ts | 24 ++-- .../mitxonline/hooks/organizations/index.ts | 22 +++- .../api/src/mitxonline/hooks/user/index.ts | 4 +- frontends/api/src/mutations/mutationMeta.ts | 72 ++++++++++++ .../ContractAdminPage/AssignSeatsSection.tsx | 7 +- .../ContractAdminPage/RowActionMenu.tsx | 9 +- .../DashboardDialogs.test.tsx | 110 ++++++++++++++++++ .../CoursewareDisplay/DashboardDialogs.tsx | 57 +++++---- .../CoursewareDisplay/EnrolledCourseCard.tsx | 13 ++- .../UnenrolledCourseCard.test.tsx | 70 +++++++++++ .../hooks/useEnrollmentHandler.ts | 17 ++- .../EnrollmentCodePage/EnrollmentCodePage.tsx | 12 +- .../ProductPages/StayUpdatedModal.tsx | 5 +- .../ProductPages/useCourseEnrollment.ts | 7 +- .../ProductPages/useProgramEnrollment.ts | 9 +- .../UnsubscribePage/UnsubscribePage.tsx | 5 +- frontends/main/src/app/getQueryClient.ts | 65 ++++++++++- .../main/src/app/handleMutationError.test.ts | 80 +++++++++++++ .../main/src/app/mutationErrorToast.test.tsx | 66 +++++++++++ frontends/main/src/app/providers.tsx | 2 + .../common/mitxonline/useReplaceBasketItem.ts | 7 +- .../CourseEnrollmentDialog.test.tsx | 12 +- .../CourseEnrollmentDialog.tsx | 9 +- .../JustInTimeDialog.test.tsx | 8 +- .../EnrollmentDialogs/JustInTimeDialog.tsx | 7 +- .../ManageListDialogs/ManageListDialogs.tsx | 13 ++- .../contentTypes/article/ArticleEditor.tsx | 11 +- .../contentTypes/news/NewsEditor.tsx | 11 +- .../page-components/Toaster/Toaster.test.tsx | 52 +++++++++ .../src/page-components/Toaster/Toaster.tsx | 49 ++++++++ .../Toaster/toastStore.test.ts | 19 +++ .../src/page-components/Toaster/toastStore.ts | 45 +++++++ frontends/main/src/test-utils/index.tsx | 33 +++++- frontends/main/src/test-utils/setupJest.tsx | 26 +++++ frontends/ol-components/src/index.ts | 2 + 44 files changed, 923 insertions(+), 97 deletions(-) create mode 100644 frontends/api/src/mutations/mutationMeta.ts create mode 100644 frontends/main/src/app/handleMutationError.test.ts create mode 100644 frontends/main/src/app/mutationErrorToast.test.tsx create mode 100644 frontends/main/src/page-components/Toaster/Toaster.test.tsx create mode 100644 frontends/main/src/page-components/Toaster/Toaster.tsx create mode 100644 frontends/main/src/page-components/Toaster/toastStore.test.ts create mode 100644 frontends/main/src/page-components/Toaster/toastStore.ts diff --git a/.github/instructions/frontend-tests.instructions.md b/.github/instructions/frontend-tests.instructions.md index fad0232604..a2914568ab 100644 --- a/.github/instructions/frontend-tests.instructions.md +++ b/.github/instructions/frontend-tests.instructions.md @@ -183,6 +183,18 @@ expect(consoleError).toHaveBeenCalled() // In rare cases, TestingErrorBoundary can be used to test thrown errors. ``` +**Mutation failures and the global error toast:** + +Every mutation failure raises a global error toast unless the call site opts out (`meta: SILENCE_ERROR_TOAST`), and an `afterEach` fails any test that leaves a toast unacknowledged. + +```tsx +import { expectErrorToast } from "@/test-utils" +// After driving a mutation failure whose intended error surface is the toast: +await expectErrorToast("Something went wrong") +// If the component shows its own inline error instead, don't acknowledge — +// opt the component out of the toast with `meta: SILENCE_ERROR_TOAST`. +``` + ## Troubleshooting - **"No response specified"** → Mock the API call with `setMockResponse` diff --git a/.github/instructions/frontend.instructions.md b/.github/instructions/frontend.instructions.md index b289f49599..747eadea46 100644 --- a/.github/instructions/frontend.instructions.md +++ b/.github/instructions/frontend.instructions.md @@ -8,3 +8,11 @@ applyTo: "**/*.ts,**/*.tsx,**/package.json" - `api` contains generated API client code and react-query hooks - For reusable UI, use components from `@mitodl/smoot-design`, `ol-components` preferentially - Within `main`, use `@/` for root-relative imports + +## Mutation error handling + +Every mutation failure in the browser shows a global error toast by default (`MutationCache.onError` in `main/src/app/getQueryClient.ts`), so failures are never silent. Tune it per call site via React Query `meta` (typed in `api/mutation-meta`): + +- Component renders its own inline error for the failure → pass `meta: SILENCE_ERROR_TOAST` to the mutation hook, or the user sees a double alert. Required even if you catch the `mutateAsync` rejection yourself — catching does not suppress the toast. +- Custom toast copy → `meta: { errorMessage: "Could not save your changes." }`, or `getErrorMessage(error, variables)` for data-driven copy. +- Shared `api/` hooks accept `{ meta }` (`MutationHookOptions`) and forward it; the opt-out belongs at the _consumer_, never baked into a shared hook (many hooks are surfaced in one place and silent in another). diff --git a/frontends/api/package.json b/frontends/api/package.json index f8b3df4903..449edf96e6 100644 --- a/frontends/api/package.json +++ b/frontends/api/package.json @@ -16,6 +16,7 @@ "./test-utils/mockAxios": "./src/test-utils/mockAxios.ts", "./test-utils": "./src/test-utils/index.ts", "./mitxonline-hooks/*": "./src/mitxonline/hooks/*/index.ts", + "./mutation-meta": "./src/mutations/mutationMeta.ts", "./mitxonline-test-utils": "./src/mitxonline/test-utils/index.ts", "./analytics-hooks/*": "./src/analytics/hooks/*/index.ts", "./analytics-types": "./src/analytics/types.ts", diff --git a/frontends/api/src/hooks/hubspot/index.ts b/frontends/api/src/hooks/hubspot/index.ts index 85af0439a9..5073f57f03 100644 --- a/frontends/api/src/hooks/hubspot/index.ts +++ b/frontends/api/src/hooks/hubspot/index.ts @@ -10,6 +10,7 @@ import type { } from "../../generated/v1" import type { HubspotFormDetailResponse } from "./queries" import { hubspotKeys, hubspotQueries } from "./queries" +import type { MutationHookOptions } from "../../mutations/mutationMeta" const HUBSPOT_UTK_COOKIE = "hubspotutk" const HUBSPOT_UTK_MAX_AGE = 34190000 // ~13 months, matching HubSpot's tracking script @@ -84,8 +85,9 @@ const useHubspotFormDetail = ( }) } -const useHubspotFormSubmit = () => { +const useHubspotFormSubmit = ({ meta }: MutationHookOptions = {}) => { return useMutation({ + meta, mutationFn: ({ formId, fields, diff --git a/frontends/api/src/hooks/learningPaths/index.ts b/frontends/api/src/hooks/learningPaths/index.ts index ada723a6eb..98a96c0c86 100644 --- a/frontends/api/src/hooks/learningPaths/index.ts +++ b/frontends/api/src/hooks/learningPaths/index.ts @@ -15,6 +15,7 @@ import { learningPathsApi } from "../../clients" import { learningPathQueries, learningPathKeys } from "./queries" import { learningResourceKeys } from "../learningResources/queries" import { useUserHasPermission, Permission } from "api/hooks/user" +import type { MutationHookOptions } from "../../mutations/mutationMeta" const useLearningPathsList = ( params: ListRequest = {}, @@ -44,7 +45,7 @@ type LearningPathCreateRequest = Omit< CreateRequest["LearningPathResourceRequest"], "readable_id" | "resource_type" > -const useLearningPathCreate = () => { +const useLearningPathCreate = ({ meta }: MutationHookOptions = {}) => { const queryClient = useQueryClient() return useMutation({ mutationFn: (params: LearningPathCreateRequest) => @@ -54,10 +55,11 @@ const useLearningPathCreate = () => { onSettled: () => { queryClient.invalidateQueries({ queryKey: learningPathKeys.listRoot() }) }, + meta, }) } -const useLearningPathUpdate = () => { +const useLearningPathUpdate = ({ meta }: MutationHookOptions = {}) => { const queryClient = useQueryClient() return useMutation({ mutationFn: ( @@ -76,6 +78,7 @@ const useLearningPathUpdate = () => { queryKey: learningResourceKeys.featuredRoot(), }) }, + meta, }) } diff --git a/frontends/api/src/hooks/unsubscribe/index.ts b/frontends/api/src/hooks/unsubscribe/index.ts index 4b7dda9ff3..0f26f96730 100644 --- a/frontends/api/src/hooks/unsubscribe/index.ts +++ b/frontends/api/src/hooks/unsubscribe/index.ts @@ -1,10 +1,12 @@ import { useMutation } from "@tanstack/react-query" import { unsubscribeApi } from "../../clients" +import type { MutationHookOptions } from "../../mutations/mutationMeta" -const useUnsubscribe = () => +const useUnsubscribe = ({ meta }: MutationHookOptions = {}) => useMutation({ mutationFn: (token: string) => unsubscribeApi.unsubscribeCreate({ token }).then((res) => res.data), + meta, }) export { useUnsubscribe } diff --git a/frontends/api/src/hooks/userLists/index.ts b/frontends/api/src/hooks/userLists/index.ts index 83c29f24d5..315a8ed650 100644 --- a/frontends/api/src/hooks/userLists/index.ts +++ b/frontends/api/src/hooks/userLists/index.ts @@ -14,6 +14,7 @@ import type { } from "../../generated/v1" import { userlistKeys, userlistQueries } from "./queries" import { useUserIsAuthenticated } from "api/hooks/user" +import type { MutationHookOptions } from "../../mutations/mutationMeta" const useUserListList = ( params: ListRequest = {}, @@ -29,7 +30,7 @@ const useUserListsDetail = (id: number) => { return useQuery(userlistQueries.detail(id)) } -const useUserListCreate = () => { +const useUserListCreate = ({ meta }: MutationHookOptions = {}) => { const queryClient = useQueryClient() return useMutation({ mutationFn: (params: CreateRequest["UserListRequest"]) => @@ -39,9 +40,10 @@ const useUserListCreate = () => { onSettled: () => { queryClient.invalidateQueries({ queryKey: userlistKeys.listRoot() }) }, + meta, }) } -const useUserListUpdate = () => { +const useUserListUpdate = ({ meta }: MutationHookOptions = {}) => { const queryClient = useQueryClient() return useMutation({ mutationFn: (params: Pick & Partial) => @@ -53,6 +55,7 @@ const useUserListUpdate = () => { queryClient.invalidateQueries({ queryKey: userlistKeys.listRoot() }) queryClient.invalidateQueries({ queryKey: userlistKeys.detail(vars.id) }) }, + meta, }) } diff --git a/frontends/api/src/hooks/website_content/index.ts b/frontends/api/src/hooks/website_content/index.ts index 2ed18c7ee8..354d0cf197 100644 --- a/frontends/api/src/hooks/website_content/index.ts +++ b/frontends/api/src/hooks/website_content/index.ts @@ -8,6 +8,7 @@ import type { WebsiteContent, } from "../../generated/v1" import { websiteContentQueries, websiteContentKeys } from "./queries" +import type { MutationHookOptions } from "../../mutations/mutationMeta" const useWebsiteContentList = ( params: WebsiteContentListRequest = {}, @@ -36,7 +37,7 @@ const useWebsiteContentDetailRetrieve = (identifier: string | undefined) => { }) } -const useWebsiteContentCreate = () => { +const useWebsiteContentCreate = ({ meta }: MutationHookOptions = {}) => { const client = useQueryClient() return useMutation({ mutationFn: ( @@ -56,15 +57,17 @@ const useWebsiteContentCreate = () => { onSuccess: () => { client.invalidateQueries({ queryKey: websiteContentKeys.listRoot() }) }, + meta, }) } -export const useMediaUpload = () => { +export const useMediaUpload = ({ meta }: MutationHookOptions = {}) => { const nextProgressCb = useRef<((percent: number) => void) | undefined>( undefined, ) const mutation = useMutation({ + meta, mutationFn: async (data: { file: File }) => { const response = await mediaApi.mediaUpload( { image_file: data.file }, @@ -111,7 +114,7 @@ const useWebsiteContentDestroy = () => { }, }) } -const useWebsiteContentPartialUpdate = () => { +const useWebsiteContentPartialUpdate = ({ meta }: MutationHookOptions = {}) => { const client = useQueryClient() return useMutation({ mutationFn: ({ @@ -124,6 +127,7 @@ const useWebsiteContentPartialUpdate = () => { PatchedWebsiteContentRequest: data, }) .then((response) => response.data), + meta, onSuccess: (websiteContent: WebsiteContent) => { client.invalidateQueries({ queryKey: websiteContentKeys.detail(websiteContent.id), diff --git a/frontends/api/src/mitxonline/hooks/baskets/index.ts b/frontends/api/src/mitxonline/hooks/baskets/index.ts index a603caf661..78c9cd4ee0 100644 --- a/frontends/api/src/mitxonline/hooks/baskets/index.ts +++ b/frontends/api/src/mitxonline/hooks/baskets/index.ts @@ -2,12 +2,13 @@ import { basketQueries } from "./queries" import { useMutation, useQueryClient } from "@tanstack/react-query" import { basketsApi } from "../../clients" import type { BasketWithProduct } from "@mitodl/mitxonline-api-axios/v2" +import type { MutationHookOptions } from "../../../mutations/mutationMeta" /** * Hook to add a product to the user's basket. * Creates or updates the basket, adding the specified product. */ -const useAddToBasket = () => { +const useAddToBasket = ({ meta }: MutationHookOptions = {}) => { const queryClient = useQueryClient() return useMutation({ mutationFn: async (productId: number): Promise => { @@ -22,13 +23,14 @@ const useAddToBasket = () => { queryKey: basketQueries.basketState().queryKey, }) }, + meta, }) } /** * Hook to clear the user's basket. */ -const useClearBasket = () => { +const useClearBasket = ({ meta }: MutationHookOptions = {}) => { const queryClient = useQueryClient() return useMutation({ mutationFn: async (): Promise => { @@ -39,6 +41,7 @@ const useClearBasket = () => { queryKey: basketQueries.basketState().queryKey, }) }, + meta, }) } diff --git a/frontends/api/src/mitxonline/hooks/enrollment/index.ts b/frontends/api/src/mitxonline/hooks/enrollment/index.ts index 94315f6e87..8693a686ff 100644 --- a/frontends/api/src/mitxonline/hooks/enrollment/index.ts +++ b/frontends/api/src/mitxonline/hooks/enrollment/index.ts @@ -1,5 +1,6 @@ import { enrollmentQueries, enrollmentKeys } from "./queries" import { useMutation, useQueryClient } from "@tanstack/react-query" +import type { MutationHookOptions } from "../../../mutations/mutationMeta" import { b2bApi, courseRunEnrollmentsApi, @@ -14,7 +15,7 @@ import { VerifiedProgramEnrollmentsApiVerifiedProgramEnrollmentsCreateRequest, } from "@mitodl/mitxonline-api-axios/v2" -const useCreateB2bEnrollment = () => { +const useCreateB2bEnrollment = ({ meta }: MutationHookOptions = {}) => { const queryClient = useQueryClient() return useMutation({ mutationFn: (opts: B2bApiB2bEnrollCreateRequest) => @@ -24,10 +25,11 @@ const useCreateB2bEnrollment = () => { queryKey: enrollmentKeys.courseRunEnrollmentsList(), }) }, + meta, }) } -const useCreateEnrollment = () => { +const useCreateEnrollment = ({ meta }: MutationHookOptions = {}) => { const queryClient = useQueryClient() return useMutation({ mutationFn: (opts: CourseRunEnrollmentRequest) => { @@ -43,10 +45,11 @@ const useCreateEnrollment = () => { queryKey: enrollmentKeys.programEnrollmentsList(), }) }, + meta, }) } -const useUpdateEnrollment = () => { +const useUpdateEnrollment = ({ meta }: MutationHookOptions = {}) => { const queryClient = useQueryClient() return useMutation({ mutationFn: (opts: EnrollmentsApiEnrollmentsPartialUpdateRequest) => @@ -56,10 +59,11 @@ const useUpdateEnrollment = () => { queryKey: enrollmentKeys.courseRunEnrollmentsList(), }) }, + meta, }) } -const useDestroyEnrollment = () => { +const useDestroyEnrollment = ({ meta }: MutationHookOptions = {}) => { const queryClient = useQueryClient() return useMutation({ mutationFn: (enrollmentId: number) => @@ -75,10 +79,11 @@ const useDestroyEnrollment = () => { queryKey: enrollmentKeys.courseRunEnrollmentsList(), }) }, + meta, }) } -const useDestroyProgramEnrollment = () => { +const useDestroyProgramEnrollment = ({ meta }: MutationHookOptions = {}) => { const queryClient = useQueryClient() return useMutation({ mutationFn: (programId: number) => @@ -96,10 +101,11 @@ const useDestroyProgramEnrollment = () => { queryKey: enrollmentKeys.programEnrollmentsList(), }) }, + meta, }) } -const useCreateProgramEnrollment = () => { +const useCreateProgramEnrollment = ({ meta }: MutationHookOptions = {}) => { const queryClient = useQueryClient() return useMutation({ mutationFn: ( @@ -110,10 +116,13 @@ const useCreateProgramEnrollment = () => { queryKey: enrollmentKeys.programEnrollmentsList(), }) }, + meta, }) } -const useCreateVerifiedProgramEnrollment = () => { +const useCreateVerifiedProgramEnrollment = ({ + meta, +}: MutationHookOptions = {}) => { const queryClient = useQueryClient() return useMutation({ mutationFn: ( @@ -127,6 +136,7 @@ const useCreateVerifiedProgramEnrollment = () => { queryKey: enrollmentKeys.programEnrollmentsList(), }) }, + meta, }) } diff --git a/frontends/api/src/mitxonline/hooks/organizations/index.ts b/frontends/api/src/mitxonline/hooks/organizations/index.ts index 83d08aabef..82ebff845b 100644 --- a/frontends/api/src/mitxonline/hooks/organizations/index.ts +++ b/frontends/api/src/mitxonline/hooks/organizations/index.ts @@ -13,8 +13,12 @@ import { managerOrganizationQueries, managerOrganizationKeys, } from "./queries" +import type { MutationHookOptions } from "../../../mutations/mutationMeta" -const useB2BAttachMutation = (opts: B2bApiB2bAttachCreateRequest) => { +const useB2BAttachMutation = ( + opts: B2bApiB2bAttachCreateRequest, + { meta }: MutationHookOptions = {}, +) => { const queryClient = useQueryClient() return useMutation({ mutationFn: async () => { @@ -24,6 +28,7 @@ const useB2BAttachMutation = (opts: B2bApiB2bAttachCreateRequest) => { onSuccess: () => { queryClient.invalidateQueries({ queryKey: ["mitxonline"] }) }, + meta, }) } @@ -32,12 +37,13 @@ const useB2BAttachMutation = (opts: B2bApiB2bAttachCreateRequest) => { * auto-allocated by the backend (one per record); the response reports which * addresses were assigned and which failed. */ -const useBulkAssignSeats = () => { +const useBulkAssignSeats = ({ meta }: MutationHookOptions = {}) => { const queryClient = useQueryClient() return useMutation({ mutationFn: ( opts: B2bApiB2bManagerOrganizationsContractsCodesBulkAssignCreateRequest, ) => b2bApi.b2bManagerOrganizationsContractsCodesBulkAssignCreate(opts), + meta, onSettled: (_data, _err, vars) => { queryClient.invalidateQueries({ queryKey: managerOrganizationKeys.contractCodesForContract( @@ -58,12 +64,13 @@ const useBulkAssignSeats = () => { } /** Resend the claim email for an assigned-but-unredeemed enrollment code. */ -const useRemindCode = () => { +const useRemindCode = ({ meta }: MutationHookOptions = {}) => { const queryClient = useQueryClient() return useMutation({ mutationFn: ( opts: B2bApiB2bManagerOrganizationsContractsCodesRemindCreateRequest, ) => b2bApi.b2bManagerOrganizationsContractsCodesRemindCreate(opts), + meta, onSettled: (_data, _err, vars) => { queryClient.invalidateQueries({ queryKey: managerOrganizationKeys.contractCodesForContract( @@ -76,12 +83,13 @@ const useRemindCode = () => { } /** Revoke a code assignment, returning the code to the unassigned pool. */ -const useRevokeCode = () => { +const useRevokeCode = ({ meta }: MutationHookOptions = {}) => { const queryClient = useQueryClient() return useMutation({ mutationFn: ( opts: B2bApiB2bManagerOrganizationsContractsCodesRevokeDestroyRequest, ) => b2bApi.b2bManagerOrganizationsContractsCodesRevokeDestroy(opts), + meta, onSettled: (_data, _err, vars) => { queryClient.invalidateQueries({ queryKey: managerOrganizationKeys.contractCodesForContract( @@ -106,12 +114,13 @@ const useRevokeCode = () => { * The backend updates the existing assignment in place and re-sends the claim * email. Returns 409 if the code has already been redeemed. */ -const useReassignCode = () => { +const useReassignCode = ({ meta }: MutationHookOptions = {}) => { const queryClient = useQueryClient() return useMutation({ mutationFn: ( opts: B2bApiB2bManagerOrganizationsContractsCodesReassignUpdateRequest, ) => b2bApi.b2bManagerOrganizationsContractsCodesReassignUpdate(opts), + meta, onSettled: (_data, _err, vars) => { queryClient.invalidateQueries({ queryKey: managerOrganizationKeys.contractCodesForContract( @@ -124,11 +133,12 @@ const useReassignCode = () => { } /** Send a test enrollment code to the given email address. */ -const useSendTestEmail = () => +const useSendTestEmail = ({ meta }: MutationHookOptions = {}) => useMutation({ mutationFn: ( opts: B2bApiB2bManagerOrganizationsContractsCodesSendTestEmailCreateRequest, ) => b2bApi.b2bManagerOrganizationsContractsCodesSendTestEmailCreate(opts), + meta, }) export { diff --git a/frontends/api/src/mitxonline/hooks/user/index.ts b/frontends/api/src/mitxonline/hooks/user/index.ts index eabdf349a7..2f5625d935 100644 --- a/frontends/api/src/mitxonline/hooks/user/index.ts +++ b/frontends/api/src/mitxonline/hooks/user/index.ts @@ -5,6 +5,7 @@ import { useQueryClient, } from "@tanstack/react-query" import { countriesApi, usersApi } from "../../clients" +import type { MutationHookOptions } from "../../../mutations/mutationMeta" import type { User } from "@mitodl/mitxonline-api-axios/v2" import { UsersApiUsersMePartialUpdateRequest } from "@mitodl/mitxonline-api-axios/v2" @@ -39,7 +40,7 @@ const queries = { const useMitxOnlineUserMe = (opts: { enabled?: boolean } = {}) => useQuery({ ...queries.me(), ...opts }) -const useUpdateUserMutation = () => { +const useUpdateUserMutation = ({ meta }: MutationHookOptions = {}) => { const queryClient = useQueryClient() return useMutation({ mutationFn: (opts: UsersApiUsersMePartialUpdateRequest) => @@ -47,6 +48,7 @@ const useUpdateUserMutation = () => { onSuccess: () => { queryClient.invalidateQueries({ queryKey: userKeys.me() }) }, + meta, }) } diff --git a/frontends/api/src/mutations/mutationMeta.ts b/frontends/api/src/mutations/mutationMeta.ts new file mode 100644 index 0000000000..f1e0e0cc68 --- /dev/null +++ b/frontends/api/src/mutations/mutationMeta.ts @@ -0,0 +1,72 @@ +/** + * Typed React Query mutation `meta`, read by the global mutation-error handler + * (a `MutationCache.onError` wired up in the `main` app's query client). + * + * The default behavior is: any mutation error shows a top-center error toast. + * A call site overrides that through `meta` — declared here as plain data so + * the `api` package stays UI-free (the toast itself lives in `main`). + * + * Augmenting `Register.mutationMeta` makes `mutation.meta` typed everywhere it + * is set (here in `api`) and read (in `main`), instead of `Record`. + */ + +/** + * NOTE: this must stay a `type` alias. As an `interface` it no longer satisfies + * the `TMutationMeta extends Record` constraint in React + * Query's `Register` lookup (interfaces lack an implicit index signature), and + * `meta` silently degrades to `Record` everywhere — with zero + * compiler errors. + */ +export type MutationErrorMeta = { + /** + * Set `false` to suppress the default error toast — e.g. when the call site + * renders its own inline error colocated with the action. + * + * Catching a `mutateAsync` rejection does NOT suppress the toast — the + * cache-level `onError` runs before the error is rethrown to the caller — so + * a call site that handles the error itself still needs this opt-out. + */ + showErrorToast?: boolean + /** Static toast copy for this mutation. */ + errorMessage?: string + /** + * Data-driven toast copy; takes precedence over `errorMessage`. Declared in + * the typed `api` hook where `TVariables` is known; the global handler passes + * the raw (`unknown`) error and variables, so cast inside the implementation: + * + * ```ts + * meta: { + * getErrorMessage: (_error, variables) => + * `Could not remove "${(variables as DestroyRequest).title}".`, + * } + * ``` + */ + getErrorMessage?: (error: unknown, variables: unknown) => string +} + +/** + * Options a shared mutation hook forwards to `useMutation`, letting a *consumer* + * tune error handling without the hook baking in a policy. Chiefly used to pass + * `meta: { showErrorToast: false }` from a call site that renders its own inline + * error, so the same hook can stay silent there and toast by default elsewhere. + */ +export type MutationHookOptions = { + meta?: MutationErrorMeta +} + +/** + * Canonical `meta` for a call site that renders its own inline error and so + * opts out of the global error toast. Named (not inlined) so every opt-out site + * is greppable and reads as a deliberate choice rather than a magic boolean. + */ +export const SILENCE_ERROR_TOAST: MutationErrorMeta = Object.freeze({ + // Frozen: this one object is shared by identity across every opt-out site, + // so a stray mutation would poison all of them. + showErrorToast: false, +}) + +declare module "@tanstack/react-query" { + interface Register { + mutationMeta: MutationErrorMeta + } +} diff --git a/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsSection.tsx b/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsSection.tsx index c801c88d56..b32d7deff8 100644 --- a/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsSection.tsx +++ b/frontends/main/src/app-pages/ContractAdminPage/AssignSeatsSection.tsx @@ -15,6 +15,7 @@ import { useBulkAssignSeats, useSendTestEmail, } from "api/mitxonline-hooks/organizations" +import { SILENCE_ERROR_TOAST } from "api/mutation-meta" import { mitxUserQueries } from "api/mitxonline-hooks/user" import { useQuery } from "@tanstack/react-query" import type { BulkAssignError } from "@mitodl/mitxonline-api-axios/v2" @@ -232,8 +233,10 @@ const AssignSeatsSection: React.FC = ({ const [errorAnnouncement, setErrorAnnouncement] = useState("") const fileInputRef = useRef(null) - const bulkAssign = useBulkAssignSeats() - const sendTestEmail = useSendTestEmail() + // Both surface their outcome via inline Alerts (bulk-assign result Alert and + // the send-test-email alert), so suppress the global error toast. + const bulkAssign = useBulkAssignSeats({ meta: SILENCE_ERROR_TOAST }) + const sendTestEmail = useSendTestEmail({ meta: SILENCE_ERROR_TOAST }) const { data: user } = useQuery(mitxUserQueries.me()) const submitResult = useMemo( diff --git a/frontends/main/src/app-pages/ContractAdminPage/RowActionMenu.tsx b/frontends/main/src/app-pages/ContractAdminPage/RowActionMenu.tsx index e01eabc567..4c5c2924b1 100644 --- a/frontends/main/src/app-pages/ContractAdminPage/RowActionMenu.tsx +++ b/frontends/main/src/app-pages/ContractAdminPage/RowActionMenu.tsx @@ -21,6 +21,7 @@ import { useRevokeCode, } from "api/mitxonline-hooks/organizations" import type { ManagerEnrollmentCode } from "api/mitxonline-hooks/organizations" +import { SILENCE_ERROR_TOAST } from "api/mutation-meta" import type { AxiosError } from "axios" const ActionMenuItem = styled(MenuItem)(({ theme }) => ({ @@ -96,9 +97,11 @@ const RowActionMenu: React.FC = ({ const reassignDescId = useId() const open = Boolean(anchorEl) - const remind = useRemindCode() - const revoke = useRevokeCode() - const reassign = useReassignCode() + // These actions surface their outcome via the page-level result Alert (see + // onResult), so suppress the global error toast. + const remind = useRemindCode({ meta: SILENCE_ERROR_TOAST }) + const revoke = useRevokeCode({ meta: SILENCE_ERROR_TOAST }) + const reassign = useReassignCode({ meta: SILENCE_ERROR_TOAST }) const isRedeemed = code.redemption_status === "redeemed" const hasAssignedEmail = Boolean(code.assigned_to?.trim()) diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/DashboardDialogs.test.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/DashboardDialogs.test.tsx index 00dad8c511..37803725c8 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/DashboardDialogs.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/DashboardDialogs.test.tsx @@ -110,6 +110,83 @@ describe("DashboardDialogs", () => { ) }) + test("The email settings dialog shows an inline error when the update fails", async () => { + const { enrollments } = setupApis() + const enrollment = faker.helpers.arrayElement(enrollments) + + setMockResponse.patch( + mitxonline.urls.enrollment.courseEnrollment(enrollment.id), + {}, + { code: 500 }, + ) + renderWithProviders() + + const cards = await screen.findAllByTestId("enrollment-card-desktop") + const card = cards.find( + (c) => !!within(c).queryByText(enrollment.run.title), + ) + invariant(card) + + await user.click(await within(card).findByLabelText("More options")) + await user.click( + await screen.findByRole("menuitem", { name: "Email Settings" }), + ) + + const dialog = await screen.findByRole("dialog", { name: "Email Settings" }) + await user.click( + within(dialog).getByRole("checkbox", { name: "Receive course emails" }), + ) + await user.click( + within(dialog).getByRole("button", { name: "Save Settings" }), + ) + + // The dialog always renders a warning alert about unchecking the box, so the + // failure adds a second alert; pinning the count keeps a stray extra alert + // from passing as the inline error. + await waitFor(() => + expect(within(dialog).getAllByRole("alert")).toHaveLength(2), + ) + const [, errorAlert] = within(dialog).getAllByRole("alert") + expect(errorAlert).toHaveTextContent( + "There was a problem updating your email settings. Please try again later.", + ) + // Dialog stays open so the user can retry. + expect(dialog).toBeInTheDocument() + }) + + test("The unenroll dialog shows an inline error when the unenroll fails", async () => { + const { enrollments } = setupApis() + const enrollment = faker.helpers.arrayElement(enrollments) + + setMockResponse.delete( + mitxonline.urls.enrollment.courseEnrollment(enrollment.id), + {}, + { code: 500 }, + ) + renderWithProviders() + + const cards = await screen.findAllByTestId("enrollment-card-desktop") + const card = cards.find( + (c) => !!within(c).queryByText(enrollment.run.title), + ) + invariant(card) + + await user.click(await within(card).findByLabelText("More options")) + await user.click(await screen.findByRole("menuitem", { name: "Unenroll" })) + + const dialog = await screen.findByRole("dialog", { + name: `Unenroll from ${enrollment.run.title}`, + }) + await user.click(within(dialog).getByRole("button", { name: "Unenroll" })) + + expect(await within(dialog).findByRole("alert")).toHaveTextContent( + "There was a problem unenrolling you from this course. Please try again later.", + ) + // The card survives a failed unenroll. + expect(card).toBeInTheDocument() + expect(trackCourseUnenrolled).not.toHaveBeenCalled() + }) + test("Opening the unenroll dialog and confirming the unenroll fires the proper API call", async () => { const { enrollments } = setupApis() const enrollment = faker.helpers.arrayElement(enrollments) @@ -335,6 +412,39 @@ describe("UnenrollProgramDialog", () => { expect(trackCourseUnenrolled).not.toHaveBeenCalled() }) + test("Shows an inline error when the unenroll fails", async () => { + const { programEnrollment } = setupProgramCard("audit", null) + + setMockResponse.delete( + mitxonline.urls.programEnrollments.programEnrollment( + programEnrollment.program.id, + ), + {}, + { code: 500 }, + ) + + renderWithProviders( + , + ) + + const desktopCard = await screen.findByTestId("enrollment-card-desktop") + await user.click(within(desktopCard).getByLabelText("More options")) + await user.click(await screen.findByRole("menuitem", { name: "Unenroll" })) + + const dialog = await screen.findByRole("dialog", { + name: `Unenroll from ${programEnrollment.program.title}`, + }) + await user.click(within(dialog).getByRole("button", { name: "Unenroll" })) + + expect(await within(dialog).findByRole("alert")).toHaveTextContent( + "There was a problem unenrolling you from this program. Please try again later.", + ) + expect(trackProgramUnenrolled).not.toHaveBeenCalled() + }) + test("Cancelling the dialog does not fire the API call", async () => { const { programEnrollment } = setupProgramCard("audit", null) diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/DashboardDialogs.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/DashboardDialogs.tsx index b41781c19a..5262a4f3f3 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/DashboardDialogs.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/DashboardDialogs.tsx @@ -16,6 +16,7 @@ import { useDestroyProgramEnrollment, useUpdateEnrollment, } from "api/mitxonline-hooks/enrollment" +import { SILENCE_ERROR_TOAST } from "api/mutation-meta" import { CourseRunEnrollmentV3 } from "@mitodl/mitxonline-api-axios/v2" import { trackCourseUnenrolled, @@ -42,19 +43,21 @@ const EmailSettingsDialogInner: React.FC = ({ initialValues: { receive_emails: enrollment.edx_emails_subscription ?? true, }, - onSubmit: async () => { - await updateEnrollment.mutateAsync({ - id: enrollment.id, - PatchedUpdateCourseRunEnrollmentRequest: { - receive_emails: formik.values.receive_emails, + onSubmit: () => { + updateEnrollment.mutate( + { + id: enrollment.id, + PatchedUpdateCourseRunEnrollmentRequest: { + receive_emails: formik.values.receive_emails, + }, }, - }) - if (!updateEnrollment.isError) { - modal.hide() - } + { onSuccess: () => modal.hide() }, + ) }, }) - const updateEnrollment = useUpdateEnrollment() + // Renders its own inline error below (updateEnrollment.isError), so suppress + // the global error toast. + const updateEnrollment = useUpdateEnrollment({ meta: SILENCE_ERROR_TOAST }) return ( = ({ enrollment, }) => { const modal = NiceModal.useModal() - const destroyEnrollment = useDestroyEnrollment() + // Renders its own inline error below (destroyEnrollment.isError), so suppress + // the global error toast. + const destroyEnrollment = useDestroyEnrollment({ meta: SILENCE_ERROR_TOAST }) const formik = useFormik({ enableReinitialize: true, validateOnChange: false, validateOnBlur: false, initialValues: {}, - onSubmit: async () => { - await destroyEnrollment.mutateAsync(enrollment.id) - if (!destroyEnrollment.isError) { - trackCourseUnenrolled(title) - modal.hide() - } + onSubmit: () => { + destroyEnrollment.mutate(enrollment.id, { + onSuccess: () => { + trackCourseUnenrolled(title) + modal.hide() + }, + }) }, }) return ( @@ -189,16 +195,23 @@ const UnenrollProgramDialogInner: React.FC = ({ programId, }) => { const modal = NiceModal.useModal() - const destroyProgramEnrollment = useDestroyProgramEnrollment() + // Renders its own inline error below (destroyProgramEnrollment.isError), so + // suppress the global error toast. + const destroyProgramEnrollment = useDestroyProgramEnrollment({ + meta: SILENCE_ERROR_TOAST, + }) const formik = useFormik({ enableReinitialize: true, validateOnChange: false, validateOnBlur: false, initialValues: {}, - onSubmit: async () => { - await destroyProgramEnrollment.mutateAsync(programId) - trackProgramUnenrolled(title) - modal.hide() + onSubmit: () => { + destroyProgramEnrollment.mutate(programId, { + onSuccess: () => { + trackProgramUnenrolled(title) + modal.hide() + }, + }) }, }) return ( diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx index 0dbf8eaae9..87bcdf47d2 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx @@ -30,6 +30,7 @@ import { RiArrowUpCircleLine, RiAwardLine, RiMore2Line } from "@remixicon/react" import { useReplaceBasketItem } from "@/common/mitxonline/useReplaceBasketItem" import { useComplianceGate } from "@/common/mitxonline/useComplianceGate" import { useCreateVerifiedProgramEnrollment } from "api/mitxonline-hooks/enrollment" +import { SILENCE_ERROR_TOAST } from "api/mutation-meta" import { isInPast, calendarDaysUntil, NoSSR } from "ol-utilities" import { SiblingRunsPanel, SiblingRunsToggle } from "./SiblingRunsAccordion" import { EnrollmentStatusIcon } from "./EnrollmentStatus" @@ -98,8 +99,16 @@ const UpgradeBanner: React.FC< onUpgradeFailure, ...others }) => { - const replaceBasketItem = useReplaceBasketItem() - const createVerifiedProgramEnrollment = useCreateVerifiedProgramEnrollment() + // Upgrade failures are caught below and surfaced via onUpgradeFailure (an + // inline alert in the parent), so suppress the global error toast. A caught + // mutateAsync rejection does NOT suppress the cache-level onError, so this + // opt-out is what prevents a double alert. `onUpgradeFailure` is optional — + // a caller that omits it has no error surface of its own, so only silence + // the toast when the callback is actually wired. + const upgradeErrorMeta = onUpgradeFailure ? { meta: SILENCE_ERROR_TOAST } : {} + const replaceBasketItem = useReplaceBasketItem(upgradeErrorMeta) + const createVerifiedProgramEnrollment = + useCreateVerifiedProgramEnrollment(upgradeErrorMeta) const { ensureCompliance } = useComplianceGate() const programRequestBody = programReadableIds?.length diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.test.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.test.tsx index 45b099a984..5d799ce52b 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.test.tsx @@ -1,5 +1,6 @@ import React from "react" import { + expectErrorToast, renderWithProviders, screen, setMockResponse, @@ -822,6 +823,75 @@ describe.each([ }) }) +describe("UnenrolledCourseCard enrollment error toast", () => { + setupLocationMock() + + const getCard = () => screen.getByTestId("enrollment-card-desktop") + + test("Failed course enrollment surfaces a course-specific error toast", async () => { + setupUserApis() + const run = mitxonline.factories.courses.courseRun({ + b2b_contract: null, + is_enrollable: true, + enrollment_modes: [ + mitxonline.factories.courses.enrollmentMode({ + requires_payment: false, + }), + ], + }) + const course = mitxOnlineCourse({ courseruns: [run], next_run_id: run.id }) + + setMockResponse.get(mitxonline.urls.enrollment.enrollmentsListV3(), []) + setMockResponse.post( + mitxonline.urls.enrollment.enrollmentsListV1(), + {}, + { code: 500 }, + ) + + renderWithProviders() + + await user.click(within(getCard()).getByTestId("courseware-button")) + + await expectErrorToast( + "Something went wrong enrolling you in this course. Please try again.", + ) + }) + + test("Failed verified program enrollment surfaces a program-specific error toast", async () => { + setupUserApis() + const run = mitxonline.factories.courses.courseRun({ + b2b_contract: null, + is_enrollable: true, + courseware_url: faker.internet.url(), + }) + const course = mitxOnlineCourse({ courseruns: [run], next_run_id: run.id }) + + const programEnrollment = + mitxonline.factories.enrollment.programEnrollmentV3({ + enrollment_mode: "verified", + }) + + setMockResponse.post( + mitxonline.urls.verifiedProgramEnrollments.create(run.courseware_id), + {}, + { code: 500 }, + ) + + renderWithProviders( + , + ) + + await user.click(within(getCard()).getByTestId("courseware-button")) + + await expectErrorToast( + "Something went wrong enrolling you in this program. Please try again.", + ) + }) +}) + describe("UnenrolledCourseCard card type label", () => { setupLocationMock() diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/hooks/useEnrollmentHandler.ts b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/hooks/useEnrollmentHandler.ts index fca020debd..4728e13f36 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/hooks/useEnrollmentHandler.ts +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/hooks/useEnrollmentHandler.ts @@ -15,10 +15,21 @@ import { useComplianceGate } from "@/common/mitxonline/useComplianceGate" import CourseEnrollmentDialog from "@/page-components/EnrollmentDialogs/CourseEnrollmentDialog" import { trackCourseEnrolled } from "@/common/analytics/gtm" +const ENROLL_COURSE_ERROR = + "Something went wrong enrolling you in this course. Please try again." +const ENROLL_PROGRAM_ERROR = + "Something went wrong enrolling you in this program. Please try again." + export const useEnrollmentHandler = () => { - const createB2bEnrollment = useCreateB2bEnrollment() - const createEnrollment = useCreateEnrollment() - const createVerifiedProgramEnrollment = useCreateVerifiedProgramEnrollment() + const createB2bEnrollment = useCreateB2bEnrollment({ + meta: { errorMessage: ENROLL_COURSE_ERROR }, + }) + const createEnrollment = useCreateEnrollment({ + meta: { errorMessage: ENROLL_COURSE_ERROR }, + }) + const createVerifiedProgramEnrollment = useCreateVerifiedProgramEnrollment({ + meta: { errorMessage: ENROLL_PROGRAM_ERROR }, + }) const replaceBasketItem = useReplaceBasketItem() const { ensureCompliance } = useComplianceGate() diff --git a/frontends/main/src/app-pages/EnrollmentCodePage/EnrollmentCodePage.tsx b/frontends/main/src/app-pages/EnrollmentCodePage/EnrollmentCodePage.tsx index d46ffd48eb..2e0fd160bc 100644 --- a/frontends/main/src/app-pages/EnrollmentCodePage/EnrollmentCodePage.tsx +++ b/frontends/main/src/app-pages/EnrollmentCodePage/EnrollmentCodePage.tsx @@ -8,6 +8,7 @@ import { } from "@/common/mitxonline" import * as urls from "@/common/urls" import { useB2BAttachMutation } from "api/mitxonline-hooks/organizations" +import { SILENCE_ERROR_TOAST } from "api/mutation-meta" import { userQueries } from "api/hooks/user" import { useQuery } from "@tanstack/react-query" import { useRouter } from "next-nprogress-bar" @@ -24,9 +25,14 @@ const InterstitialMessage = styled(Typography)(({ theme }) => ({ const EnrollmentCodePage: React.FC = ({ code }) => { const router = useRouter() - const enrollment = useB2BAttachMutation({ - enrollment_code: code, - }) + // Failure redirects to an error page (onError below), so suppress the global + // error toast. + const enrollment = useB2BAttachMutation( + { + enrollment_code: code, + }, + { meta: SILENCE_ERROR_TOAST }, + ) const { isLoading: userLoading, data: user } = useQuery({ ...userQueries.me(), diff --git a/frontends/main/src/app-pages/ProductPages/StayUpdatedModal.tsx b/frontends/main/src/app-pages/ProductPages/StayUpdatedModal.tsx index 32381dedad..7f327a8d02 100644 --- a/frontends/main/src/app-pages/ProductPages/StayUpdatedModal.tsx +++ b/frontends/main/src/app-pages/ProductPages/StayUpdatedModal.tsx @@ -23,6 +23,7 @@ import { useHubspotFormSubmit, type HubspotSubmitField, } from "api/hooks/hubspot" +import { SILENCE_ERROR_TOAST } from "api/mutation-meta" import { trackSignUpForUpdates } from "@/common/analytics/gtm" const StayUpdatedDialogContainer = styled.div(({ theme }) => ({ @@ -94,7 +95,9 @@ const StayUpdatedDialogInner: React.FC = ({ const { data: hubspotForm, isLoading } = useHubspotFormDetail( stayUpdatedFormId ? { form_id: stayUpdatedFormId } : undefined, ) - const hubspotFormSubmit = useHubspotFormSubmit() + // The form renders its own inline submission error (errorText below), so + // suppress the global error toast. + const hubspotFormSubmit = useHubspotFormSubmit({ meta: SILENCE_ERROR_TOAST }) const [email, setEmail] = useState("") const closeDialog = async () => { diff --git a/frontends/main/src/app-pages/ProductPages/useCourseEnrollment.ts b/frontends/main/src/app-pages/ProductPages/useCourseEnrollment.ts index 5d656c881c..5ea91ba570 100644 --- a/frontends/main/src/app-pages/ProductPages/useCourseEnrollment.ts +++ b/frontends/main/src/app-pages/ProductPages/useCourseEnrollment.ts @@ -5,6 +5,7 @@ import type { } from "@mitodl/mitxonline-api-axios/v2" import { userQueries } from "api/hooks/user" import { useCreateEnrollment } from "api/mitxonline-hooks/enrollment" +import { SILENCE_ERROR_TOAST } from "api/mutation-meta" import { useReplaceBasketItem } from "@/common/mitxonline/useReplaceBasketItem" import { useComplianceGate } from "@/common/mitxonline/useComplianceGate" import { enrollmentAlertSuccessUrl } from "@/common/mitxonline" @@ -60,8 +61,10 @@ export const useCourseEnrollment = ( const { runIds: enrolledRunIds, isLoading: enrollmentsIsLoading } = useCourseEnrolledRunIds(course) - const replaceBasketItem = useReplaceBasketItem() - const createEnrollment = useCreateEnrollment() + // This area renders its own inline enrollment-failure alert (see + // EnrollOfferingBoxes `isError`), so suppress the global error toast. + const replaceBasketItem = useReplaceBasketItem({ meta: SILENCE_ERROR_TOAST }) + const createEnrollment = useCreateEnrollment({ meta: SILENCE_ERROR_TOAST }) const router = useRouter() const posthog = usePostHog() // Paid enrollments are gated inside useReplaceBasketItem; the free track diff --git a/frontends/main/src/app-pages/ProductPages/useProgramEnrollment.ts b/frontends/main/src/app-pages/ProductPages/useProgramEnrollment.ts index 079eaccb43..8cce18321c 100644 --- a/frontends/main/src/app-pages/ProductPages/useProgramEnrollment.ts +++ b/frontends/main/src/app-pages/ProductPages/useProgramEnrollment.ts @@ -2,6 +2,7 @@ import { useQuery } from "@tanstack/react-query" import type { V2ProgramDetail } from "@mitodl/mitxonline-api-axios/v2" import { userQueries } from "api/hooks/user" import { useCreateProgramEnrollment } from "api/mitxonline-hooks/enrollment" +import { SILENCE_ERROR_TOAST } from "api/mutation-meta" import { useReplaceBasketItem } from "@/common/mitxonline/useReplaceBasketItem" import { useComplianceGate } from "@/common/mitxonline/useComplianceGate" import { enrollmentAlertSuccessUrl } from "@/common/mitxonline" @@ -48,8 +49,12 @@ export const useProgramEnrollment = ( const { isEnrolled, isLoading: enrollmentsIsLoading } = useProgramIsEnrolled(program) - const replaceBasketItem = useReplaceBasketItem() - const createProgramEnrollment = useCreateProgramEnrollment() + // This area renders its own inline enrollment-failure alert (see + // EnrollOfferingBoxes `isError`), so suppress the global error toast. + const replaceBasketItem = useReplaceBasketItem({ meta: SILENCE_ERROR_TOAST }) + const createProgramEnrollment = useCreateProgramEnrollment({ + meta: SILENCE_ERROR_TOAST, + }) const router = useRouter() const posthog = usePostHog() // Paid enrollments are gated inside useReplaceBasketItem; the free track diff --git a/frontends/main/src/app-pages/UnsubscribePage/UnsubscribePage.tsx b/frontends/main/src/app-pages/UnsubscribePage/UnsubscribePage.tsx index 06a183ada1..1d2416a14b 100644 --- a/frontends/main/src/app-pages/UnsubscribePage/UnsubscribePage.tsx +++ b/frontends/main/src/app-pages/UnsubscribePage/UnsubscribePage.tsx @@ -4,6 +4,7 @@ import React from "react" import { Card, Container, Typography, styled } from "ol-components" import { Button, ButtonLoadingIcon } from "@mitodl/smoot-design" import { useUnsubscribe } from "api/hooks/unsubscribe" +import { SILENCE_ERROR_TOAST } from "api/mutation-meta" import { UnsubscribedPage } from "@/app-pages/UnsubscribedPage/UnsubscribedPage" const PageContainer = styled(Container)({ @@ -29,7 +30,9 @@ type UnsubscribePageProps = { } const UnsubscribePage: React.FC = ({ token }) => { - const unsubscribe = useUnsubscribe() + // Failure renders its own error page (isError below), so suppress the global + // error toast. + const unsubscribe = useUnsubscribe({ meta: SILENCE_ERROR_TOAST }) if (!token) { return diff --git a/frontends/main/src/app/getQueryClient.ts b/frontends/main/src/app/getQueryClient.ts index 15dcf3d9f0..19260b36fe 100644 --- a/frontends/main/src/app/getQueryClient.ts +++ b/frontends/main/src/app/getQueryClient.ts @@ -1,11 +1,21 @@ // Based on https://tanstack.com/query/v5/docs/framework/react/guides/advanced-ssr -import { QueryClient, isServer, focusManager } from "@tanstack/react-query" +import { + QueryClient, + MutationCache, + isServer, + focusManager, +} from "@tanstack/react-query" +import type { Mutation } from "@tanstack/react-query" import type { AxiosError } from "axios" import { cache } from "react" import { notFound } from "next/navigation" import { bootstrapApiClients } from "@/bootstrap/api" import { getCacheSMaxageSeconds } from "@/common/config" +import { showErrorToast } from "@/page-components/Toaster/toastStore" +// Type-only: augments `mutation.meta` (see api/src/mutations/mutationMeta.ts). +import "api/mutation-meta" +import type { MutationErrorMeta } from "api/mutation-meta" /** Max retries after first failure */ const MAX_RETRIES = 3 @@ -22,6 +32,58 @@ const MAX_RETRY_DELAY = 1000 const THROW_ERROR_CODES = [400, 401, 403] const NO_RETRY_CODES = [400, 401, 403, 404, 405, 409, 422] +/** Fallback error-toast copy when a mutation declares no `meta.errorMessage`. */ +export const GENERIC_ERROR_MESSAGE = "Something went wrong. Please try again." + +/** Blank/whitespace copy is treated as "no message" so a mistaken `""` never + * renders a blank toast. */ +const isMessage = (value: unknown): value is string => + typeof value === "string" && value.trim() !== "" + +/** + * Resolve the toast copy: `meta.getErrorMessage(error, variables)`, then + * `meta.errorMessage`, then the generic fallback. Each tier falls through to + * the next when it yields no message. + * + * Defensive by design: a call site's `getErrorMessage` could throw (e.g. + * reading `error.response.data` on a network error with no response), and an + * exception escaping `onError` would leave the failure completely silent — the + * exact thing this handler exists to prevent. + */ +const resolveErrorMessage = ( + meta: MutationErrorMeta | undefined, + error: unknown, + variables: unknown, +): string => { + try { + const custom = meta?.getErrorMessage?.(error, variables) + if (isMessage(custom)) return custom + } catch { + // Fall through to the static message. + } + const staticMessage = meta?.errorMessage + if (isMessage(staticMessage)) return staticMessage + return GENERIC_ERROR_MESSAGE +} + +/** + * Default error handling for *all* browser mutations: show a top-center error + * toast unless the mutation opts out via `meta.showErrorToast === false`. This + * guarantees a mutation failure is never silent. + * + * Exported for unit testing; wired into the browser client's `MutationCache`. + */ +export const handleMutationError = ( + error: unknown, + variables: unknown, + _context: unknown, + mutation: Mutation, +): void => { + const meta = mutation.meta + if (meta?.showErrorToast === false) return + showErrorToast(resolveErrorMessage(meta, error, variables)) +} + /** * Extended QueryClient with custom fetchQueryOr404 method. * Automatically calls notFound() on 404 errors when running on the server. @@ -146,6 +208,7 @@ const makeBrowserQueryClient = ( ): AugmentedQueryClient => { const { maxRetries } = config return new AugmentedQueryClient({ + mutationCache: new MutationCache({ onError: handleMutationError }), defaultOptions: { queries: { /** diff --git a/frontends/main/src/app/handleMutationError.test.ts b/frontends/main/src/app/handleMutationError.test.ts new file mode 100644 index 0000000000..63e218d3f0 --- /dev/null +++ b/frontends/main/src/app/handleMutationError.test.ts @@ -0,0 +1,80 @@ +import { handleMutationError, GENERIC_ERROR_MESSAGE } from "./getQueryClient" +import { showErrorToast } from "@/page-components/Toaster/toastStore" +import type { Mutation } from "@tanstack/react-query" +import type { MutationErrorMeta } from "api/mutation-meta" + +jest.mock("@/page-components/Toaster/toastStore", () => ({ + showErrorToast: jest.fn(), +})) +const mockShowErrorToast = showErrorToast as jest.Mock + +/** The handler only reads `mutation.meta`; fake the rest. */ +const fakeMutation = (meta: MutationErrorMeta | undefined) => + ({ meta }) as unknown as Mutation + +/** Invoke the handler with just the `meta` that the real code reads. */ +const fireError = ( + meta: MutationErrorMeta | undefined, + error: unknown = new Error("boom"), + variables: unknown = { id: 1 }, +) => handleMutationError(error, variables, undefined, fakeMutation(meta)) + +beforeEach(() => { + mockShowErrorToast.mockClear() +}) + +// The no-meta default and the showErrorToast opt-out are covered end-to-end in +// mutationErrorToast.test.tsx; this file pins the copy-resolution edge cases. + +test("uses meta.errorMessage when provided", () => { + fireError({ errorMessage: "Could not save your changes." }) + expect(mockShowErrorToast).toHaveBeenCalledWith( + "Could not save your changes.", + ) +}) + +test("meta.getErrorMessage wins over errorMessage and receives error + variables", () => { + const getErrorMessage = jest.fn().mockReturnValue("Derived message") + const error = new Error("nope") + const variables = { id: 7 } + + handleMutationError( + error, + variables, + undefined, + fakeMutation({ errorMessage: "static", getErrorMessage }), + ) + + expect(getErrorMessage).toHaveBeenCalledWith(error, variables) + expect(mockShowErrorToast).toHaveBeenCalledWith("Derived message") +}) + +test("falls back to errorMessage when getErrorMessage throws", () => { + fireError({ + getErrorMessage: () => { + throw new Error("bad derivation") + }, + errorMessage: "Static fallback.", + }) + // The throw must not escape onError (which would leave the failure silent). + expect(mockShowErrorToast).toHaveBeenCalledWith("Static fallback.") +}) + +test("falls back to errorMessage when getErrorMessage returns blank", () => { + fireError({ getErrorMessage: () => "", errorMessage: "Static fallback." }) + expect(mockShowErrorToast).toHaveBeenCalledWith("Static fallback.") +}) + +test("falls back to generic copy when getErrorMessage throws and there is no errorMessage", () => { + fireError({ + getErrorMessage: () => { + throw new Error("bad derivation") + }, + }) + expect(mockShowErrorToast).toHaveBeenCalledWith(GENERIC_ERROR_MESSAGE) +}) + +test("falls back to generic copy when the resolved message is empty", () => { + fireError({ errorMessage: " " }) + expect(mockShowErrorToast).toHaveBeenCalledWith(GENERIC_ERROR_MESSAGE) +}) diff --git a/frontends/main/src/app/mutationErrorToast.test.tsx b/frontends/main/src/app/mutationErrorToast.test.tsx new file mode 100644 index 0000000000..11aadf617d --- /dev/null +++ b/frontends/main/src/app/mutationErrorToast.test.tsx @@ -0,0 +1,66 @@ +import React from "react" +import { useMutation } from "@tanstack/react-query" +import { + renderWithProviders, + screen, + user, + expectErrorToast, +} from "@/test-utils" +import { GENERIC_ERROR_MESSAGE } from "@/app/getQueryClient" +import { SILENCE_ERROR_TOAST } from "api/mutation-meta" +import type { MutationErrorMeta } from "api/mutation-meta" + +/** + * End-to-end coverage of the wiring the whole feature rests on: + * `MutationCache.onError` (getQueryClient) -> toast store -> . The + * unit tests exercise those pieces in isolation; these prove they are actually + * connected, so deleting the `mutationCache` wiring can't pass silently. + * + * `renderWithProviders` builds the real browser query client and mounts the + * real (see test-utils/index.tsx). + */ + +const MutatingButton = ({ + meta, + withInlineError = false, +}: { + meta?: MutationErrorMeta + withInlineError?: boolean +}) => { + const mutation = useMutation({ + mutationFn: () => Promise.reject(new Error("network boom")), + meta, + }) + return ( +
+ + {withInlineError && mutation.isError ? ( +
Inline failure
+ ) : null} +
+ ) +} + +test("a mutation failure with no meta shows the global error toast", async () => { + renderWithProviders() + + await user.click(screen.getByRole("button", { name: "Submit" })) + + expect(await screen.findByRole("alert")).toHaveTextContent( + GENERIC_ERROR_MESSAGE, + ) + await expectErrorToast(GENERIC_ERROR_MESSAGE) +}) + +test("a mutation with SILENCE_ERROR_TOAST shows only its inline error, no toast", async () => { + renderWithProviders( + , + ) + + await user.click(screen.getByRole("button", { name: "Submit" })) + + const alert = await screen.findByRole("alert") + expect(alert).toHaveTextContent("Inline failure") + // Exactly one alert — the inline one — i.e. the global toast was suppressed. + expect(screen.getAllByRole("alert")).toHaveLength(1) +}) diff --git a/frontends/main/src/app/providers.tsx b/frontends/main/src/app/providers.tsx index 6f21d355e6..5b83c8e0aa 100644 --- a/frontends/main/src/app/providers.tsx +++ b/frontends/main/src/app/providers.tsx @@ -15,6 +15,7 @@ import { AppProgressBar as ProgressBar } from "next-nprogress-bar" import type { NProgressOptions } from "next-nprogress-bar" import { ReloadOnUserChange } from "@/page-components/ReloadOnUserChange/ReloadOnUserChange" import { PublicEnvInsertedHtml } from "./components/PublicEnvInsertedHtml" +import { Toaster } from "@/page-components/Toaster/Toaster" const PROGRESS_BAR_OPTS: NProgressOptions = { showSpinner: false } @@ -39,6 +40,7 @@ export default function Providers({ children }: { children: React.ReactNode }) { {children} + diff --git a/frontends/main/src/common/mitxonline/useReplaceBasketItem.ts b/frontends/main/src/common/mitxonline/useReplaceBasketItem.ts index 8de072f4c6..ee4265dcf5 100644 --- a/frontends/main/src/common/mitxonline/useReplaceBasketItem.ts +++ b/frontends/main/src/common/mitxonline/useReplaceBasketItem.ts @@ -1,4 +1,5 @@ import { useAddToBasket, useClearBasket } from "api/mitxonline-hooks/baskets" +import type { MutationHookOptions } from "api/mutation-meta" import { mitxonlineLegacyUrl } from "@/common/mitxonline" import { useComplianceGate } from "./useComplianceGate" @@ -13,9 +14,9 @@ const cartUrl = () => mitxonlineLegacyUrl("/cart/") * the gate, nothing is added and no redirect happens — callers see a resolved * promise with no navigation, so a cancel never reads as a failure. */ -const useReplaceBasketItem = () => { - const addToBasket = useAddToBasket() - const clearBasket = useClearBasket() +const useReplaceBasketItem = (opts: MutationHookOptions = {}) => { + const addToBasket = useAddToBasket(opts) + const clearBasket = useClearBasket(opts) const { ensureCompliance } = useComplianceGate() const redirect = () => window.location.assign(cartUrl()) diff --git a/frontends/main/src/page-components/EnrollmentDialogs/CourseEnrollmentDialog.test.tsx b/frontends/main/src/page-components/EnrollmentDialogs/CourseEnrollmentDialog.test.tsx index 4c268fba2a..4315f8205f 100644 --- a/frontends/main/src/page-components/EnrollmentDialogs/CourseEnrollmentDialog.test.tsx +++ b/frontends/main/src/page-components/EnrollmentDialogs/CourseEnrollmentDialog.test.tsx @@ -213,7 +213,7 @@ describe("CourseEnrollmentDialog", () => { }) await user.click(upgradeButton) - await screen.findByText( + expect(await screen.findByRole("alert")).toHaveTextContent( "There was a problem processing your enrollment. Please try again.", ) }) @@ -492,13 +492,9 @@ describe("CourseEnrollmentDialog", () => { await user.click(enrollButton) // Check for error alert - the mutation error should be displayed - await waitFor(() => { - expect( - screen.getByText( - /There was a problem enrolling you in this course. Please try again later./i, - ), - ).toBeInTheDocument() - }) + expect(await screen.findByRole("alert")).toHaveTextContent( + /There was a problem enrolling you in this course. Please try again later./i, + ) }) }) diff --git a/frontends/main/src/page-components/EnrollmentDialogs/CourseEnrollmentDialog.tsx b/frontends/main/src/page-components/EnrollmentDialogs/CourseEnrollmentDialog.tsx index 3475704275..8827904098 100644 --- a/frontends/main/src/page-components/EnrollmentDialogs/CourseEnrollmentDialog.tsx +++ b/frontends/main/src/page-components/EnrollmentDialogs/CourseEnrollmentDialog.tsx @@ -26,6 +26,7 @@ import { priceWithDiscount, } from "@/common/mitxonline" import { useCreateEnrollment } from "api/mitxonline-hooks/enrollment" +import { SILENCE_ERROR_TOAST } from "api/mutation-meta" import { useReplaceBasketItem } from "@/common/mitxonline/useReplaceBasketItem" import { useComplianceGate } from "@/common/mitxonline/useComplianceGate" import { useRouter } from "next-nprogress-bar" @@ -207,7 +208,9 @@ const CertificateUpsell: React.FC<{ const enabled = (enrollmentType === "both" || enrollmentType === "paid") && !!(product && courseRun && canPurchaseRun(courseRun)) - const replaceBasketItem = useReplaceBasketItem() + // Renders its own inline error below (replaceBasketItem.isError), so suppress + // the global error toast. + const replaceBasketItem = useReplaceBasketItem({ meta: SILENCE_ERROR_TOAST }) const userFlexiblePrice = useQuery({ ...productQueries.userFlexiblePriceDetail({ productId: product?.id ?? 0, @@ -329,7 +332,9 @@ const CourseEnrollmentDialogInner: React.FC = ({ const [chosenRun, setChosenRun] = React.useState(getDefaultOption) const run = course.courseruns.find((r) => `${r.id}` === chosenRun) const enrollmentType = getEnrollmentType(run?.enrollment_modes) - const createEnrollment = useCreateEnrollment() + // Renders its own inline error below (createEnrollment.isError), so suppress + // the global error toast. + const createEnrollment = useCreateEnrollment({ meta: SILENCE_ERROR_TOAST }) const router = useRouter() // The "Add to Cart" path inside CertificateUpsell is gated by // useReplaceBasketItem; this free-enrollment submit gates here. diff --git a/frontends/main/src/page-components/EnrollmentDialogs/JustInTimeDialog.test.tsx b/frontends/main/src/page-components/EnrollmentDialogs/JustInTimeDialog.test.tsx index 978c8d3d3e..0cb8541299 100644 --- a/frontends/main/src/page-components/EnrollmentDialogs/JustInTimeDialog.test.tsx +++ b/frontends/main/src/page-components/EnrollmentDialogs/JustInTimeDialog.test.tsx @@ -398,11 +398,9 @@ describe("JustInTimeDialog", () => { await user.click(within(dialog).getByRole("button", { name: "Submit" })) - expect( - await within(dialog).findByText( - "There was a problem saving your details. Please try again later.", - ), - ).toBeInTheDocument() + expect(await screen.findByRole("alert")).toHaveTextContent( + "There was a problem saving your details. Please try again later.", + ) // The entered values survive so the user can retry without retyping. expect(textbox(dialog, "First Name")).toHaveValue("Ada") }, 10000) diff --git a/frontends/main/src/page-components/EnrollmentDialogs/JustInTimeDialog.tsx b/frontends/main/src/page-components/EnrollmentDialogs/JustInTimeDialog.tsx index e3e1efefb9..c36ae2212c 100644 --- a/frontends/main/src/page-components/EnrollmentDialogs/JustInTimeDialog.tsx +++ b/frontends/main/src/page-components/EnrollmentDialogs/JustInTimeDialog.tsx @@ -18,6 +18,7 @@ import { mitxUserQueries, useUpdateUserMutation, } from "api/mitxonline-hooks/user" +import { SILENCE_ERROR_TOAST } from "api/mutation-meta" import { FIELD_SPECS, JIT_FIELDS, @@ -69,7 +70,11 @@ const JustInTimeDialogInner: React.FC = () => { const modal = NiceModal.useModal() const user = useQuery(mitxUserQueries.me()) const countries = useQuery(mitxUserQueries.countries()) - const updateUser = useUpdateUserMutation() + // Failures are surfaced by the inline error alert below (driven by + // `updateUser.isError`), so opt out of the global error toast to avoid a + // double alert. The caught `mutateAsync` rejection does not suppress the + // cache-level `onError`, so this opt-out is what prevents it. + const updateUser = useUpdateUserMutation({ meta: SILENCE_ERROR_TOAST }) const fieldsRef = React.useRef(null) const [subdivisionNotice, setSubdivisionNotice] = React.useState("") diff --git a/frontends/main/src/page-components/ManageListDialogs/ManageListDialogs.tsx b/frontends/main/src/page-components/ManageListDialogs/ManageListDialogs.tsx index 2d4bebd119..aabf7b1a30 100644 --- a/frontends/main/src/page-components/ManageListDialogs/ManageListDialogs.tsx +++ b/frontends/main/src/page-components/ManageListDialogs/ManageListDialogs.tsx @@ -23,6 +23,7 @@ import { useUserListUpdate, useUserListDestroy, } from "api/hooks/userLists" +import { SILENCE_ERROR_TOAST } from "api/mutation-meta" const learningPathFormSchema = Yup.object().shape({ published: Yup.boolean() @@ -81,8 +82,10 @@ const UpsertLearningPathDialog = NiceModal.create( const topicsQuery = useLearningResourceTopics(undefined, { enabled: modal.visible, }) - const createList = useLearningPathCreate() - const updateList = useLearningPathUpdate() + // The dialog renders its own inline error (mutation.isError below), so + // suppress the global error toast. + const createList = useLearningPathCreate({ meta: SILENCE_ERROR_TOAST }) + const updateList = useLearningPathUpdate({ meta: SILENCE_ERROR_TOAST }) const mutation = resource?.id ? updateList : createList const handleSubmit: FormikConfig< LearningPathResource | LearningPathFormValues @@ -201,8 +204,10 @@ interface UpsertUserListDialogProps { const UpsertUserListDialog = NiceModal.create( ({ userList, title, onDestroy }: UpsertUserListDialogProps) => { const modal = NiceModal.useModal() - const createList = useUserListCreate() - const updateList = useUserListUpdate() + // The dialog renders its own inline error (mutation.isError below), so + // suppress the global error toast. + const createList = useUserListCreate({ meta: SILENCE_ERROR_TOAST }) + const updateList = useUserListUpdate({ meta: SILENCE_ERROR_TOAST }) const mutation = userList?.id ? updateList : createList const handleSubmit: FormikConfig< UserList | UserListFormValues 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 c7ced70591..311f296c72 100644 --- a/frontends/main/src/page-components/TiptapEditor/contentTypes/article/ArticleEditor.tsx +++ b/frontends/main/src/page-components/TiptapEditor/contentTypes/article/ArticleEditor.tsx @@ -8,6 +8,7 @@ import { useWebsiteContentPartialUpdate, useMediaUpload, } from "api/hooks/website_content" +import { SILENCE_ERROR_TOAST } from "api/mutation-meta" import { WebsiteContentEditor } from "../../core/WebsiteContentEditor" import { createArticleExtensions, @@ -67,9 +68,13 @@ interface ArticleEditorProps { */ const ArticleEditor = ({ onSave, readOnly, article }: ArticleEditorProps) => { // Swap these hooks when a dedicated article API exists. - const createMutation = useWebsiteContentCreate() - const updateMutation = useWebsiteContentPartialUpdate() - const uploadImage = useMediaUpload() + // The editor renders its own inline error (WebsiteContentEditor `error`), so + // suppress the global error toast. + const createMutation = useWebsiteContentCreate({ meta: SILENCE_ERROR_TOAST }) + const updateMutation = useWebsiteContentPartialUpdate({ + meta: SILENCE_ERROR_TOAST, + }) + const uploadImage = useMediaUpload({ meta: SILENCE_ERROR_TOAST }) return ( { // News content type uses the websiteContent API. // A different content type would call different hooks here. - const createMutation = useWebsiteContentCreate() - const updateMutation = useWebsiteContentPartialUpdate() - const uploadImage = useMediaUpload() + // The editor renders its own inline error (WebsiteContentEditor `error`), so + // suppress the global error toast. + const createMutation = useWebsiteContentCreate({ meta: SILENCE_ERROR_TOAST }) + const updateMutation = useWebsiteContentPartialUpdate({ + meta: SILENCE_ERROR_TOAST, + }) + const uploadImage = useMediaUpload({ meta: SILENCE_ERROR_TOAST }) return ( { + renderWithTheme() + expect(screen.queryByRole("alert")).not.toBeInTheDocument() +}) + +test("renders the error message when showErrorToast is called", async () => { + renderWithTheme() + + act(() => { + showErrorToast("Enrollment failed") + }) + + expect(await screen.findByRole("alert")).toHaveTextContent( + "Enrollment failed", + ) + act(() => dismissErrorToast()) +}) + +test("a new error replaces the current toast rather than stacking", async () => { + renderWithTheme() + act(() => { + showErrorToast("First failure") + }) + await screen.findByRole("alert") + + act(() => { + showErrorToast("Second failure") + }) + + const alert = await screen.findByRole("alert") // findByRole throws if >1 + expect(alert).toHaveTextContent("Second failure") + act(() => dismissErrorToast()) +}) + +test("the dismiss button clears the toast", async () => { + renderWithTheme() + act(() => { + showErrorToast("Enrollment failed") + }) + await screen.findByRole("alert") + + await user.click(screen.getByRole("button", { name: "Dismiss" })) + + await waitFor(() => { + expect(screen.queryByRole("alert")).not.toBeInTheDocument() + }) +}) diff --git a/frontends/main/src/page-components/Toaster/Toaster.tsx b/frontends/main/src/page-components/Toaster/Toaster.tsx new file mode 100644 index 0000000000..7cfa0a9e58 --- /dev/null +++ b/frontends/main/src/page-components/Toaster/Toaster.tsx @@ -0,0 +1,49 @@ +"use client" + +import React, { useSyncExternalStore } from "react" +import { Snackbar, HEADER_HEIGHT } from "ol-components" +import { Alert } from "@mitodl/smoot-design" +import { + subscribeToToast, + getToastSnapshot, + dismissErrorToast, +} from "./toastStore" + +const getServerSnapshot = () => null + +/** + * App-level host for the global error toast. Mounted once (in `providers`). + * Subscribes to the module-level toast store that `MutationCache.onError` + * writes to, and renders the current error as a persistent top-center toast. + * + * Persistent (no `autoHideDuration`): an error may carry a "Contact Support" + * action, so it stays until dismissed via the `Alert`'s own close button. + * + * The `Alert` is wrapped in a `div` because `Snackbar` clones its child with a + * ref and the `Alert` does not forward one. + */ +export const Toaster: React.FC = () => { + const toast = useSyncExternalStore( + subscribeToToast, + getToastSnapshot, + getServerSnapshot, + ) + + return ( + +
+ {toast ? ( + + {toast.message} + + ) : undefined} +
+ + ) +} diff --git a/frontends/main/src/page-components/Toaster/toastStore.test.ts b/frontends/main/src/page-components/Toaster/toastStore.test.ts new file mode 100644 index 0000000000..7ce3d950c5 --- /dev/null +++ b/frontends/main/src/page-components/Toaster/toastStore.test.ts @@ -0,0 +1,19 @@ +import { + showErrorToast, + dismissErrorToast, + subscribeToToast, +} from "./toastStore" + +// The store's other behaviors (show, replace, dismiss) are covered at the +// render level in Toaster.test.tsx; only unsubscription has no render-level +// coverage, since never unmounts there. +test("an unsubscribed listener is not notified", () => { + const listener = jest.fn() + const unsubscribe = subscribeToToast(listener) + unsubscribe() + + showErrorToast("ignored") + + expect(listener).not.toHaveBeenCalled() + dismissErrorToast() +}) diff --git a/frontends/main/src/page-components/Toaster/toastStore.ts b/frontends/main/src/page-components/Toaster/toastStore.ts new file mode 100644 index 0000000000..9d7da63499 --- /dev/null +++ b/frontends/main/src/page-components/Toaster/toastStore.ts @@ -0,0 +1,45 @@ +/** + * A tiny module-level store for the app's error toast. + * + * The global `MutationCache.onError` (in `getQueryClient`) fires *outside* React, + * so it can't call a hook or context setter. It calls `showErrorToast` here; the + * `` component subscribes via `useSyncExternalStore` and renders it. + * + * Deliberately free of React/MUI imports so `getQueryClient` (which also runs + * during SSR) can import `showErrorToast` without pulling UI into that module. + */ + +export type ErrorToast = { message: string } + +let current: ErrorToast | null = null +const listeners = new Set<() => void>() + +const emit = () => { + listeners.forEach((listener) => listener()) +} + +/** Show (or replace) the single error toast. */ +export const showErrorToast = (message: string): void => { + // `current` is a module-level singleton; writing it on the server would leak + // across concurrent SSR requests. The `MutationCache.onError` that calls this + // is wired only on the browser client — this guard enforces that invariant. + if (typeof window === "undefined") return + current = { message } + emit() +} + +/** Dismiss the current toast, if any. */ +export const dismissErrorToast = (): void => { + if (!current) return + current = null + emit() +} + +export const subscribeToToast = (listener: () => void): (() => void) => { + listeners.add(listener) + return () => { + listeners.delete(listener) + } +} + +export const getToastSnapshot = (): ErrorToast | null => current diff --git a/frontends/main/src/test-utils/index.tsx b/frontends/main/src/test-utils/index.tsx index e9e3ad6908..f082c33adb 100644 --- a/frontends/main/src/test-utils/index.tsx +++ b/frontends/main/src/test-utils/index.tsx @@ -6,7 +6,12 @@ import { Provider as NiceModalProvider } from "@ebay/nice-modal-react" import { ComplianceGateProvider } from "@/common/mitxonline/useComplianceGate" import { makeBrowserQueryClient } from "@/app/getQueryClient" -import { render } from "@testing-library/react" +import { Toaster } from "@/page-components/Toaster/Toaster" +import { + getToastSnapshot, + dismissErrorToast, +} from "@/page-components/Toaster/toastStore" +import { act, render, waitFor } from "@testing-library/react" import { factories, setMockResponse } from "api/test-utils" import type { CurrentUser, User } from "api/hooks/user" import { userQueries } from "api/hooks/user" @@ -53,6 +58,10 @@ const TestProviders: React.FC<{ {children} + {/* Mirror the real app (see providers.tsx) so the global mutation-error + toast renders in tests. This makes a missing `showErrorToast: false` + opt-out at a site with its own inline error a visible double-alert. */} + ) @@ -174,6 +183,27 @@ const ignoreError = (errorMessage: string, timeoutMs?: number) => { return { clear } } +/** + * Assert that the global mutation-error toast fired with the given message, + * then dismiss it. + * + * Every test ends with a check that no unacknowledged error toast is left + * showing (see setupJest.tsx). A test that intentionally drives a mutation + * failure whose error surface IS the toast acknowledges it with this; a + * component that renders its own inline error should instead opt out via + * `meta: SILENCE_ERROR_TOAST`. + * + * Together the check and this helper ensure every mutation failure has exactly + * one deliberate error surface — no double alert (inline error plus toast), + * and no silent failure. + */ +const expectErrorToast = async (message: string | RegExp) => { + // The toast fires from `MutationCache.onError`, outside React — wait for it. + await waitFor(() => expect(getToastSnapshot()).not.toBeNull()) + expect(getToastSnapshot()?.message).toMatch(message) + act(() => dismissErrorToast()) +} + const getMetaContent = ({ property, name, @@ -300,6 +330,7 @@ export { expectProps, expectLastProps, expectWindowNavigation, + expectErrorToast, ignoreError, getMetas, assertPartialMetas, diff --git a/frontends/main/src/test-utils/setupJest.tsx b/frontends/main/src/test-utils/setupJest.tsx index d96b2dd3ad..6175eabd7f 100644 --- a/frontends/main/src/test-utils/setupJest.tsx +++ b/frontends/main/src/test-utils/setupJest.tsx @@ -8,6 +8,10 @@ import { assertMockAdapterInstalled, } from "api/test-utils/mockAxios" import preloadAll from "jest-next-dynamic-ts" +import { + dismissErrorToast, + getToastSnapshot, +} from "@/page-components/Toaster/toastStore" // Wrapped in `() => …` to defer the identifier lookup past TDZ — jest.mock // is hoisted above imports, so passing `mockAxiosFactory` directly would @@ -70,7 +74,29 @@ beforeEach(() => { // document.head.innerHTML = "" document.querySelector("title")?.remove() + // The error-toast store is module-level global state; a toast fired by one + // test would otherwise persist into the next. (The afterEach below usually + // clears it, but a toast can land asynchronously after that check runs.) + dismissErrorToast() + assertMockAdapterInstalled() }) +afterEach(() => { + // Every mutation failure raises the global error toast unless the call site + // opts out. A toast left showing at the end of a test means the test drove a + // failure without deciding which error surface the user should see. + const toast = getToastSnapshot() + if (toast) { + dismissErrorToast() + throw new Error( + [ + `A mutation failure fired the global error toast ("${toast.message}") and the test did not acknowledge it.`, + '- If the component renders its own inline error for this failure, opt out of the toast: pass `meta: SILENCE_ERROR_TOAST` (from "api/mutation-meta") to the mutation hook.', + '- If the toast is the intended error surface, acknowledge it in the test: `await expectErrorToast(...)` (from "@/test-utils").', + ].join("\n"), + ) + } +}) + window.scrollTo = jest.fn() diff --git a/frontends/ol-components/src/index.ts b/frontends/ol-components/src/index.ts index 45c4e17b9f..ea9e7d3825 100644 --- a/frontends/ol-components/src/index.ts +++ b/frontends/ol-components/src/index.ts @@ -22,6 +22,8 @@ export { default as AccordionSummary } from "@mui/material/AccordionSummary" export type { AccordionSummaryProps } from "@mui/material/AccordionSummary" export { default as AccordionDetails } from "@mui/material/AccordionDetails" export type { AccordionDetailsProps } from "@mui/material/AccordionDetails" +export { default as Snackbar } from "@mui/material/Snackbar" +export type { SnackbarProps } from "@mui/material/Snackbar" export { default as AppBar } from "@mui/material/AppBar" export type { AppBarProps } from "@mui/material/AppBar" From 7ab92e2b614adda05034b55e5160515907526ee5 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Fri, 28 Aug 2026 10:56:47 -0400 Subject: [PATCH 06/14] ci: add a ci-gate job so one required check can cover the whole suite (#3825) * ci: add a ci-gate job so one required check covers the whole suite GitHub's required status checks take an exact list of context names. There are no wildcards and no "all checks must pass" option, so requiring these six jobs from the ruleset in mitodl/ol-infrastructure means restating all six names there, and that list then goes stale in two ways. A renamed job keeps being required under its old name, which GitHub will never report again, so every PR waits forever on it. mitxonline hit exactly this when its python-tests job became a 4-way matrix and the checks turned into `python-tests (1)`..`(4)`. A newly added job is not required until somebody remembers to go and add it, so new CI silently cannot block a merge. `ci-gate` fails if any job it needs ends in anything but success or skipped. Requiring it instead keeps the list of what must pass in the same file as the jobs it names, so adding or renaming a job is one edit in one repo. openapi-diff is deliberately not covered: it lives in its own workflow, and it runs `oasdiff breaking --fail-on ERR`, so it goes red on intentional breaking API changes. It was red at merge on 8 of the last 40 merged PRs here, release PRs included, so gating merges on it would mean bypassing the ruleset routinely. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QygRcd6WF4BmUi1C5cez4d * ci: wire up err-ignore allowlist for openapi-diff breaking check needs can't reach openapi-diff (separate workflow, pull_request trigger), so it can't join ci-gate directly. Give it the allowlist mechanism oasdiff already supports so intentional breaking changes stop needing a routine ruleset bypass, then require it alongside ci-gate in ol-infrastructure. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_013wnKLHVJ2vP4G6mPN9AgFr * ci: move oasdiff-err-ignore.txt out of openapi/specs/ openapi_spec_check.sh diffs openapi/specs/ against freshly generated specs and fails on any extra file -- confirmed in the python-tests CI run for dcd62cf3. Move the ignore list to openapi/ instead. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_013wnKLHVJ2vP4G6mPN9AgFr --------- Co-authored-by: Claude Opus 5 --- .github/workflows/ci.yml | 59 ++++++++++++++++++++++++++++++ .github/workflows/openapi-diff.yml | 1 + openapi/oasdiff-err-ignore.txt | 12 ++++++ 3 files changed, 72 insertions(+) create mode 100644 openapi/oasdiff-err-ignore.txt diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6409bb3f9a..91adcee7b0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -272,3 +272,62 @@ jobs: run: | diff $GENERATOR_OUTPUT_DIR_CI $GENERATOR_OUTPUT_DIR_VC \ || { echo "OpenAPI spec is out of date. Please regenerate via ./scripts/generate_openapi.sh"; exit 1; } + + # ONE required status check standing for this whole workflow. + # + # GitHub's required status checks take an exact list of context names -- no + # wildcards, no "all checks must pass" option -- so requiring these six jobs + # directly means restating all six names in mitodl/ol-infrastructure, where the + # ruleset lives. That list then goes stale in two ways: + # + # A renamed job keeps being required under its old name, which GitHub will never + # report again, so every PR waits forever on it. mitxonline hit exactly this when + # `python-tests` became a 4-way matrix and the checks turned into + # `python-tests (1)`..`(4)`. + # + # A newly added job is not required until somebody remembers to go and add it, so + # new CI silently cannot block a merge. + # + # Requiring `ci-gate` instead keeps the list of what must pass in the same file as + # the jobs it names, so adding or renaming a job is one edit in one repo. + # + # `openapi-diff` is absent from `needs` because `needs` can only reference jobs in + # this same workflow file, and `openapi-diff` runs in its own workflow (triggered on + # `pull_request` rather than `push`). It now has an `--err-ignore` allowlist + # (openapi/oasdiff-err-ignore.txt) for intentional breaking changes, so it + # should be required directly, alongside `ci-gate`, in the ol-infrastructure ruleset + # -- its job name is stable and isn't subject to the renaming/matrix drift `ci-gate` + # exists to solve. + ci-gate: + needs: + - python-tests + - javascript-tests + - build-nextjs-container + - build-storybook + - openapi-generated-client-check-v0 + - openapi-generated-client-check-v1 + # Without this the gate is skipped when a dependency fails, and a skipped required + # check leaves the PR pending rather than failing it. + if: always() + runs-on: ubuntu-24.04 + permissions: {} + steps: + - name: Evaluate CI result + env: + NEEDS: ${{ toJSON(needs) }} + run: | + set -euo pipefail + echo "$NEEDS" | jq -r 'to_entries[] | "\(.key): \(.value.result)"' + # Read from `needs` rather than naming each job again: adding a job to the + # list above is then the only edit, which is the entire point of this job. + # `skipped` passes so that a job legitimately gated behind an `if:` does not + # wedge the branch; `failure` and `cancelled` do not. + failed=$(echo "$NEEDS" | jq -r ' + to_entries[] + | select(.value.result != "success" and .value.result != "skipped") + | .key') + if [ -n "$failed" ]; then + echo "::error::CI did not succeed: $(echo "$failed" | paste -sd, -)" + exit 1 + fi + echo "All CI jobs succeeded." diff --git a/.github/workflows/openapi-diff.yml b/.github/workflows/openapi-diff.yml index b0c571520d..b54ff415d3 100644 --- a/.github/workflows/openapi-diff.yml +++ b/.github/workflows/openapi-diff.yml @@ -67,6 +67,7 @@ jobs: --volume ${{ github.workspace }}:${{ github.workspace }}:ro \ -e GITHUB_WORKSPACE=${{ github.workspace }} \ tufin/oasdiff breaking \ + --err-ignore head/openapi/oasdiff-err-ignore.txt \ --fail-on ERR \ --format githubactions \ --composed --flatten-allof \ diff --git a/openapi/oasdiff-err-ignore.txt b/openapi/oasdiff-err-ignore.txt new file mode 100644 index 0000000000..cfbab3b778 --- /dev/null +++ b/openapi/oasdiff-err-ignore.txt @@ -0,0 +1,12 @@ +# Allowlist for intentional breaking API changes, read by `oasdiff breaking +# --err-ignore` in .github/workflows/openapi-diff.yml. +# +# Each line must contain the method + path of the changed endpoint (or the +# keyword `components` for a components-section change) plus enough of the +# breaking-change message to match it. Copy the line straight out of the +# failing `openapi-diff` job's output -- oasdiff only needs the method/path +# (or `components`) and message text to line up; order and casing don't +# matter, and anything else on the line (a date, a ticket link) is fine. +# +# Example: +# 2026-08-24 GET /api/v1/foo/ removed the success response with the status '200' From ff912432501ce5952d4d0bb1a32da82c6ee467bc Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Fri, 28 Aug 2026 11:00:21 -0400 Subject: [PATCH 07/14] feat(cohort-1): MicroMasters/MITx Online certificate warehouse-pull sync (#3808) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(cohort-1): MicroMasters/MITx Online certificate warehouse-pull sync Second PR in the replacement stack for closed mit-learn#3566, stacked on the StarRocks warehouse-pull machinery (#3807). Replaces Hightouch as the writer of external.programcertificate — currently broken since ~2025-06-01 — with a warehouse-pull Celery task reading integrations__learn__program_certificates (mitodl/ol-data-platform#2591). Resolves mitodl/hq#12954. - profiles/etl.py: transform_program_certificate + upsert_program_certificate. Deliberately never prunes, even on full_refresh — a certificate is a durable achievement record, not a catalog resource that should self-heal by deleting rows a pull didn't see (a transient upstream join gap must never read as "certificate revoked"). - profiles/tasks.py: SyncProgramCertificatesTask(BaseWarehouseETLTask), registered via app.register_task per the base class's documented requirement (the @app.task(base=...) decorator doesn't work for it). - main/settings_celery.py: one beat entry, gated on STARROCKS_HOST (daily). Doesn't participate in the WAREHOUSE_ETL_CUTOVER_SOURCES mechanism — Hightouch ran externally, not as an MIT Learn Celery task, so there's no legacy beat entry to retire. - main/settings.py: removed "programcertificate" from EXTERNAL_MODELS. That setting made main.routers.ExternalSchemaRouter reject every Django-side write to this model, correct back when only Hightouch wrote to it — this task is now the intended writer, so the guard no longer applies. Flagging explicitly: this removes a safety check (main/routers_test.py's test_external_tables_are_readonly, which asserted the exact behavior just removed, is deleted alongside it). Field set was checked directly against profiles.ProgramCertificate (not assumed from memory) — the model stores 11 more columns than first planned (user_edxorg_username, user_mitxonline_username, name/ demographic/address fields), all now covered by both the dbt model and this transform. * fix: require STARROCKS_USER too before scheduling the certificate sync The beat entry gate checked only STARROCKS_HOST, but _connect_starrocks() (learning_resources.lib.warehouse) also requires STARROCKS_USER — set HOST without USER and the task gets scheduled but fails every run with ImproperlyConfigured. Addresses sentry[bot] feedback on #3808. * Address review feedback: manage the model, isolate bad rows shanbady: with the warehouse-pull cutover Django owns writes to external.programcertificate, so it should own the schema too. Flipping managed generates a state-only migration — migration 0015 already created the table, so there is no DDL and no data risk. shanbady: one malformed certificate row no longer aborts the batch. Skipped rows self-heal because the daily beat schedule runs full_refresh over the whole view. An all-rows-failed batch still raises: that is a broken database rather than a bad row, and returning normally would advance an incremental run's watermark past a window nothing was written for. Copilot: the mapping test claimed to cover every field but left the three address fields null and unasserted. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JFhTucXavYwkjv6o4qXZ8r --------- Co-authored-by: Claude Opus 5 --- main/routers_test.py | 12 -- main/settings.py | 8 +- main/settings_celery.py | 21 ++++ main/settings_test.py | 49 ++++++++ profiles/etl.py | 57 +++++++++ profiles/etl_test.py | 114 ++++++++++++++++++ .../0038_alter_programcertificate_options.py | 16 +++ profiles/models.py | 7 +- profiles/tasks.py | 62 ++++++++++ profiles/tasks_test.py | 99 ++++++++++++++- 10 files changed, 430 insertions(+), 15 deletions(-) delete mode 100644 main/routers_test.py create mode 100644 profiles/etl.py create mode 100644 profiles/etl_test.py create mode 100644 profiles/migrations/0038_alter_programcertificate_options.py diff --git a/main/routers_test.py b/main/routers_test.py deleted file mode 100644 index 6cbf1b52db..0000000000 --- a/main/routers_test.py +++ /dev/null @@ -1,12 +0,0 @@ -import pytest - -from main.routers import ReadOnlyModelError -from profiles.factories import ProgramCertificateFactory - - -def test_external_tables_are_readonly(): - """ - Test that external tables cannot be written to - """ - with pytest.raises(ReadOnlyModelError): - ProgramCertificateFactory(user_email="test@test.com", program_title="test") diff --git a/main/settings.py b/main/settings.py index 44874ee02b..a16f5ac60b 100644 --- a/main/settings.py +++ b/main/settings.py @@ -294,7 +294,13 @@ DATABASE_ROUTERS = ["main.routers.ExternalSchemaRouter"] -EXTERNAL_MODELS = ["programcertificate"] +# "programcertificate" was removed from here deliberately (was +# EXTERNAL_MODELS = ["programcertificate"]): this app is now the writer of +# that table (profiles.tasks.SyncProgramCertificatesTask, replacing the +# Hightouch sync per mitodl/hq#12954) rather than a read-only consumer of +# rows Hightouch wrote. If another external-schema, write-once-elsewhere +# model needs the same protection in the future, add it here. +EXTERNAL_MODELS = [] # Internationalization # https://docs.djangoproject.com/en/1.8/topics/i18n/ diff --git a/main/settings_celery.py b/main/settings_celery.py index c7f6331677..146a480a58 100644 --- a/main/settings_celery.py +++ b/main/settings_celery.py @@ -221,6 +221,27 @@ } ) +# MicroMasters/MITx Online program certificate sync (StarRocks-backed — see +# learning_resources.lib.warehouse.BaseWarehouseETLTask), replacing the +# Hightouch sync into external.programcertificate (mitodl/hq#12954). +# Doesn't participate in the cutover-switch mechanism below: Hightouch runs +# externally, not as an MIT Learn Celery task, so there's no legacy beat +# entry here to retire once validated. +if ( + not CELERY_BEAT_DISABLED + and get_string("STARROCKS_HOST", None) + and get_string("STARROCKS_USER", None) +): + CELERY_BEAT_SCHEDULE.update( + { + "warehouse-sync-program-certificates-every-1-days": { + "task": "profiles.tasks.SyncProgramCertificatesTask", + "schedule": crontab(minute=0, hour=9), # 5:00am EDT / 4:00am EST + "kwargs": {"full_refresh": True}, + }, + } + ) + # Per-source cutover switch for warehouse-pull catalog ETL # (learning_resources.tasks.Sync*Task, StarRocks-backed — see # learning_resources.lib.warehouse.BaseWarehouseETLTask). Each stacked PR diff --git a/main/settings_test.py b/main/settings_test.py index 7bbe49047e..4614b3b33d 100644 --- a/main/settings_test.py +++ b/main/settings_test.py @@ -330,6 +330,55 @@ def test_warehouse_etl_cutover_sources_rejects_unknown_source(self): ): self.reload_settings(module="main.settings_celery") + def test_program_certificates_beat_entry_absent_without_starrocks_host(self): + """The certificate-sync beat entry isn't registered when StarRocks + isn't configured, so it can't fail on every tick in an environment + without warehouse connectivity. + """ + with mock.patch.dict("os.environ", REQUIRED_SETTINGS, clear=True): + settings_vars = self.reload_settings(module="main.settings_celery") + assert ( + "warehouse-sync-program-certificates-every-1-days" + not in settings_vars["CELERY_BEAT_SCHEDULE"] + ) + + def test_program_certificates_beat_entry_absent_with_only_starrocks_host(self): + """STARROCKS_HOST alone isn't enough — _connect_starrocks also + requires STARROCKS_USER, so gating on host alone would schedule a + task that fails every run with ImproperlyConfigured. + """ + with mock.patch.dict( + "os.environ", + {**REQUIRED_SETTINGS, "STARROCKS_HOST": "starrocks.example.com"}, + clear=True, + ): + settings_vars = self.reload_settings(module="main.settings_celery") + assert ( + "warehouse-sync-program-certificates-every-1-days" + not in settings_vars["CELERY_BEAT_SCHEDULE"] + ) + + def test_program_certificates_beat_entry_present_with_starrocks_configured(self): + """The certificate-sync beat entry is registered once StarRocks is + fully configured (host and user), pointing at + profiles.tasks.SyncProgramCertificatesTask. + """ + with mock.patch.dict( + "os.environ", + { + **REQUIRED_SETTINGS, + "STARROCKS_HOST": "starrocks.example.com", + "STARROCKS_USER": "testuser", + }, + clear=True, + ): + settings_vars = self.reload_settings(module="main.settings_celery") + entry = settings_vars["CELERY_BEAT_SCHEDULE"][ + "warehouse-sync-program-certificates-every-1-days" + ] + assert entry["task"] == "profiles.tasks.SyncProgramCertificatesTask" + assert entry["kwargs"] == {"full_refresh": True} + def _assert_s3_storage_config( self, storages_dict, diff --git a/profiles/etl.py b/profiles/etl.py new file mode 100644 index 0000000000..ad369b863e --- /dev/null +++ b/profiles/etl.py @@ -0,0 +1,57 @@ +"""Warehouse-pull ETL for profiles.ProgramCertificate. + +Replaces Hightouch as the writer of external.programcertificate +(mitodl/hq#12954). Unlike learning_resources/etl/catalog_sources.py's +transform_* functions, this doesn't feed learning_resources.etl.loaders — +ProgramCertificate is a distinct fact model, not a LearningResource, so it +gets its own transform + upsert here rather than being forced into the +catalog contract shape. +""" + +from profiles.models import ProgramCertificate + + +def transform_program_certificate(row: dict) -> dict: + """Map an integrations__learn__program_certificates row to + ProgramCertificate field kwargs (everything but record_hash, which the + caller uses as the upsert key). + """ + return { + "program_title": row.get("program_title") or "", + "user_full_name": row.get("user_full_name") or "", + "user_email": row.get("user_email") or "", + "user_edxorg_id": row.get("user_edxorg_id"), + "user_edxorg_username": row.get("user_edxorg_username"), + "user_mitxonline_username": row.get("user_mitxonline_username"), + "micromasters_program_id": row.get("micromasters_program_id"), + "mitxonline_program_id": row.get("mitxonline_program_id"), + "user_first_name": row.get("user_first_name"), + "user_last_name": row.get("user_last_name"), + "user_gender": row.get("user_gender"), + "user_year_of_birth": row.get("user_year_of_birth"), + "user_country": row.get("user_country"), + "user_address_state_or_territory": row.get("user_address_state_or_territory"), + "user_address_city": row.get("user_address_city"), + "user_address_postal_code": row.get("user_address_postal_code"), + "user_street_address": row.get("user_street_address"), + "program_completion_timestamp": row.get("program_completion_timestamp"), + } + + +def upsert_program_certificate(row: dict) -> ProgramCertificate: + """Upsert one ProgramCertificate row, keyed on record_hash. + + No pruning: a certificate is a durable record of something a learner + achieved, so unlike catalog resources (which self-heal via full-refresh + pruning per BaseWarehouseETLTask), a certificate missing from one pull + — a transient join gap upstream, for instance — must never be + interpreted as "no longer earned" and deleted. Certificates are + upserted and otherwise left alone; nothing in this pipeline deletes a + ProgramCertificate row. + """ + record_hash = row["record_hash"] + certificate, _ = ProgramCertificate.objects.update_or_create( + record_hash=record_hash, + defaults=transform_program_certificate(row), + ) + return certificate diff --git a/profiles/etl_test.py b/profiles/etl_test.py new file mode 100644 index 0000000000..cd75cabd41 --- /dev/null +++ b/profiles/etl_test.py @@ -0,0 +1,114 @@ +"""Tests for profiles.etl.""" + +from datetime import UTC, datetime + +import pytest + +from profiles.etl import transform_program_certificate, upsert_program_certificate +from profiles.factories import ProgramCertificateFactory +from profiles.models import ProgramCertificate + +pytestmark = pytest.mark.django_db + + +def _row(**overrides): + row = { + "record_hash": "abc123", + "program_title": "Data, Economics, and Development Policy", + "user_full_name": "Ada Lovelace", + "user_email": "ada@example.com", + "user_edxorg_id": 42, + "user_edxorg_username": "ada", + "user_mitxonline_username": "ada.lovelace", + "micromasters_program_id": 7, + "mitxonline_program_id": None, + "user_first_name": "Ada", + "user_last_name": "Lovelace", + "user_gender": "f", + "user_year_of_birth": "1815", + "user_country": "GB", + "user_address_state_or_territory": "Greater London", + "user_address_city": "London", + "user_address_postal_code": "NW1 2DB", + "user_street_address": "12 Marylebone Road", + "program_completion_timestamp": datetime(2026, 1, 1, tzinfo=UTC), + } + row.update(overrides) + return row + + +def test_transform_program_certificate_maps_all_fields(): + """transform_program_certificate maps every warehouse column to its + ProgramCertificate field, excluding record_hash (the caller's upsert key). + """ + fields = transform_program_certificate(_row()) + + assert fields["program_title"] == "Data, Economics, and Development Policy" + assert fields["user_full_name"] == "Ada Lovelace" + assert fields["user_email"] == "ada@example.com" + assert fields["user_edxorg_id"] == 42 + assert fields["user_edxorg_username"] == "ada" + assert fields["user_mitxonline_username"] == "ada.lovelace" + assert fields["micromasters_program_id"] == 7 + assert fields["mitxonline_program_id"] is None + assert fields["user_first_name"] == "Ada" + assert fields["user_last_name"] == "Lovelace" + assert fields["user_gender"] == "f" + assert fields["user_year_of_birth"] == "1815" + assert fields["user_country"] == "GB" + assert fields["user_address_state_or_territory"] == "Greater London" + assert fields["user_address_city"] == "London" + assert fields["user_address_postal_code"] == "NW1 2DB" + assert fields["user_street_address"] == "12 Marylebone Road" + assert fields["program_completion_timestamp"] == datetime(2026, 1, 1, tzinfo=UTC) + assert "record_hash" not in fields + + +def test_transform_program_certificate_defaults_missing_name_fields_to_empty_string(): + """program_title/user_full_name/user_email are non-nullable CharFields on + ProgramCertificate — a missing warehouse value must become '', not None. + """ + fields = transform_program_certificate( + _row(program_title=None, user_full_name=None, user_email=None) + ) + + assert fields["program_title"] == "" + assert fields["user_full_name"] == "" + assert fields["user_email"] == "" + + +def test_upsert_program_certificate_creates_new_row(): + """upsert_program_certificate creates a new ProgramCertificate when + record_hash hasn't been seen before. + """ + upsert_program_certificate(_row()) + + certificate = ProgramCertificate.objects.get(record_hash="abc123") + assert certificate.user_full_name == "Ada Lovelace" + assert certificate.micromasters_program_id == 7 + + +def test_upsert_program_certificate_updates_existing_row(): + """upsert_program_certificate updates in place when record_hash matches + an existing certificate, rather than creating a duplicate. + """ + ProgramCertificateFactory(record_hash="abc123", user_full_name="Old Name") + + upsert_program_certificate(_row(user_full_name="Ada Lovelace")) + + assert ProgramCertificate.objects.filter(record_hash="abc123").count() == 1 + certificate = ProgramCertificate.objects.get(record_hash="abc123") + assert certificate.user_full_name == "Ada Lovelace" + + +def test_upsert_program_certificate_never_deletes(): + """upsert_program_certificate only ever touches the row it's given — + a certificate is a durable achievement record, not something this + pipeline prunes on a full refresh (see profiles.etl module docstring). + """ + ProgramCertificateFactory(record_hash="untouched") + + upsert_program_certificate(_row(record_hash="abc123")) + + assert ProgramCertificate.objects.filter(record_hash="untouched").exists() + assert ProgramCertificate.objects.count() == 2 diff --git a/profiles/migrations/0038_alter_programcertificate_options.py b/profiles/migrations/0038_alter_programcertificate_options.py new file mode 100644 index 0000000000..93badc5d03 --- /dev/null +++ b/profiles/migrations/0038_alter_programcertificate_options.py @@ -0,0 +1,16 @@ +# Generated by Django 5.2.16 on 2026-08-25 20:33 + +from django.db import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ("profiles", "0037_profile_has_logged_in"), + ] + + operations = [ + migrations.AlterModelOptions( + name="programcertificate", + options={"managed": True}, + ), + ] diff --git a/profiles/models.py b/profiles/models.py index df991c57cd..fc3a6d33e0 100644 --- a/profiles/models.py +++ b/profiles/models.py @@ -280,7 +280,12 @@ class ProgramCertificate(models.Model): program_completion_timestamp = models.DateTimeField(null=True, blank=True) class Meta: - managed = False + # Managed as of the warehouse-pull cutover (mitodl/hq#12954): Django + # is now the writer of this table, not Hightouch, so schema changes + # belong in migrations rather than in an external tool's config. The + # table stays in the `external` schema — other consumers still read it + # by that name. + managed = True db_table = '"external"."programcertificate"' def __str__(self): diff --git a/profiles/tasks.py b/profiles/tasks.py index 0840fadf10..a7187b0fa0 100644 --- a/profiles/tasks.py +++ b/profiles/tasks.py @@ -5,13 +5,75 @@ from django.contrib.auth import get_user_model from django.core.exceptions import ObjectDoesNotExist +from learning_resources.lib.warehouse import BaseWarehouseETLTask, iter_rows from main.celery import app +from profiles.etl import upsert_program_certificate from profiles.utils import send_template_email log = logging.getLogger(__name__) User = get_user_model() +class SyncProgramCertificatesTask(BaseWarehouseETLTask): + """Warehouse-pull sync of profiles.ProgramCertificate, replacing the + Hightouch sync into external.programcertificate (mitodl/hq#12954). + """ + + name = "profiles.tasks.SyncProgramCertificatesTask" + view_name = ( + "ol_data_lake_production.ol_warehouse_production_integrations" + ".integrations__learn__program_certificates" + ) + + def fetch_and_upsert(self, conn, *, since=None) -> int: + """Upsert every row iter_rows yields; see profiles.etl for why this + never prunes, even on a full_refresh run. + + A row that fails to upsert is logged and skipped rather than aborting + the batch — one malformed certificate shouldn't cost every other + learner their sync. Skipped rows aren't lost: the daily beat schedule + runs full_refresh, which re-reads the whole view, so a row that fails + today is retried tomorrow. + + The one case that still raises is *every* row failing, which is not a + bad-row problem — it's the database or the model being broken. Letting + that return normally would advance an incremental run's watermark past + a window nothing was written for, permanently skipping it (see + BaseWarehouseETLTask.run). + """ + count = 0 + failed = 0 + for row in iter_rows(conn, self.view_name, since=since): + try: + upsert_program_certificate(row) + except Exception: + failed += 1 + log.exception( + "Failed to upsert program certificate record_hash=%s", + row.get("record_hash"), + ) + else: + count += 1 + + if failed: + if count == 0: + msg = ( + f"{self.__class__.__name__}: all {failed} rows failed to " + f"upsert; refusing to report success" + ) + raise RuntimeError(msg) + log.error( + "%s: skipped %d of %d rows that failed to upsert", + self.__class__.__name__, + failed, + count + failed, + ) + return count + + +SyncProgramCertificatesTask = app.register_task(SyncProgramCertificatesTask()) + + @app.task def send_welcome_email(user_id): """ diff --git a/profiles/tasks_test.py b/profiles/tasks_test.py index ff07462ce3..3b9c4c6fb3 100644 --- a/profiles/tasks_test.py +++ b/profiles/tasks_test.py @@ -3,7 +3,8 @@ import pytest from main.factories import UserFactory -from profiles.tasks import send_welcome_email +from profiles.models import ProgramCertificate +from profiles.tasks import SyncProgramCertificatesTask, send_welcome_email @pytest.mark.django_db @@ -114,3 +115,99 @@ def test_send_welcome_email_handles_missing_profile_relation(mocker): context={"display_name": "profile-missing"}, is_transactional=True, ) + + +@pytest.mark.django_db +def test_sync_program_certificates_task_upserts_iterated_rows(mocker): + """fetch_and_upsert calls upsert_program_certificate for every row + iter_rows yields, and returns the row count. + """ + rows = [ + {"record_hash": "a", "program_title": "Program A"}, + {"record_hash": "b", "program_title": "Program B"}, + ] + mocker.patch("profiles.tasks.iter_rows", return_value=iter(rows)) + mocked_upsert = mocker.patch("profiles.tasks.upsert_program_certificate") + + count = SyncProgramCertificatesTask.fetch_and_upsert(conn=mocker.Mock()) + + assert count == 2 + assert mocked_upsert.call_count == 2 + mocked_upsert.assert_any_call(rows[0]) + mocked_upsert.assert_any_call(rows[1]) + + +def test_sync_program_certificates_task_view_name_is_fully_qualified(): + """view_name is a fully-qualified catalog.database.table name, per + learning_resources.lib.warehouse.iter_rows's contract. + """ + assert SyncProgramCertificatesTask.view_name == ( + "ol_data_lake_production.ol_warehouse_production_integrations" + ".integrations__learn__program_certificates" + ) + + +@pytest.mark.django_db +def test_sync_program_certificates_task_does_not_prune(mocker): + """A full_refresh run must never delete rows this pull didn't see — + see profiles.etl.upsert_program_certificate's docstring. + """ + ProgramCertificate.objects.create(record_hash="untouched", user_email="") + mocker.patch( + "profiles.tasks.iter_rows", + return_value=iter([{"record_hash": "abc123"}]), + ) + + SyncProgramCertificatesTask.fetch_and_upsert(conn=mocker.Mock()) + + assert ProgramCertificate.objects.filter(record_hash="untouched").exists() + + +@pytest.mark.django_db +def test_sync_program_certificates_task_skips_rows_that_fail_to_upsert(mocker): + """One bad row is logged and skipped rather than aborting the batch, so + the remaining certificates still sync. + """ + rows = [ + {"record_hash": "a"}, + {"record_hash": "bad"}, + {"record_hash": "c"}, + ] + mocker.patch("profiles.tasks.iter_rows", return_value=iter(rows)) + mocker.patch( + "profiles.tasks.upsert_program_certificate", + side_effect=[None, ValueError("bad row"), None], + ) + + count = SyncProgramCertificatesTask.fetch_and_upsert(conn=mocker.Mock()) + + assert count == 2 + + +@pytest.mark.django_db +def test_sync_program_certificates_task_raises_when_every_row_fails(mocker): + """A batch where nothing succeeded is a broken database or model, not a + bad row — it must not report success, or an incremental run would advance + its watermark past a window it never wrote. + """ + mocker.patch( + "profiles.tasks.iter_rows", + return_value=iter([{"record_hash": "a"}, {"record_hash": "b"}]), + ) + mocker.patch( + "profiles.tasks.upsert_program_certificate", + side_effect=ValueError("everything is broken"), + ) + + with pytest.raises(RuntimeError, match="all 2 rows failed"): + SyncProgramCertificatesTask.fetch_and_upsert(conn=mocker.Mock()) + + +@pytest.mark.django_db +def test_sync_program_certificates_task_empty_batch_is_not_a_failure(mocker): + """An empty view yields no rows and no failures — that's a successful + no-op run, not the all-rows-failed case. + """ + mocker.patch("profiles.tasks.iter_rows", return_value=iter([])) + + assert SyncProgramCertificatesTask.fetch_and_upsert(conn=mocker.Mock()) == 0 From d1522be512ecce42fdbc3ad2bd7dea4304b25326 Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Fri, 28 Aug 2026 15:24:50 +0000 Subject: [PATCH 08/14] Update redis Docker tag to v8.10.0 (#2706) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --- .github/workflows/ci.yml | 2 +- docker-compose.services.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 91adcee7b0..c25f3df688 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -27,7 +27,7 @@ jobs: - 5432:5432 redis: - image: redis:8.2.2 + image: redis:8.10.0@sha256:344e3945a0b431c8ff1eecd58c5573538126bd756f02fc7e218ddf1fc2546366 ports: - 6379:6379 diff --git a/docker-compose.services.yml b/docker-compose.services.yml index e00646fb64..6ab1ac488e 100644 --- a/docker-compose.services.yml +++ b/docker-compose.services.yml @@ -31,7 +31,7 @@ services: redis: profiles: - backend - image: redis:8.2.2 + image: redis:8.10.0@sha256:344e3945a0b431c8ff1eecd58c5573538126bd756f02fc7e218ddf1fc2546366 healthcheck: test: ["CMD", "redis-cli", "ping", "|", "grep", "PONG"] interval: 3s From 56557f594c323a4a9be5d98105371d7cffb14423 Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Fri, 28 Aug 2026 11:56:27 -0400 Subject: [PATCH 09/14] Update dependency litellm to v1.96.2 (#3857) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --- pyproject.toml | 2 +- uv.lock | 63 ++++++++++++++++++++++++++++---------------------- 2 files changed, 37 insertions(+), 28 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index f9d5acc879..26bc0190a7 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -55,7 +55,7 @@ dependencies = [ "isodate>=0.7.2,<0.8", "jedi>=0.19.0,<0.20", "langchain>=1.3.9,<1.4", - "litellm==1.84.0", + "litellm==1.96.2", "llama-index>=0.14.0,<0.15", "llama-index-llms-openai>=0.7.10,<0.8", "lxml>=6.0.0,<7", diff --git a/uv.lock b/uv.lock index b8615c01fd..cd0ca2b8ea 100644 --- a/uv.lock +++ b/uv.lock @@ -35,7 +35,7 @@ wheels = [ [[package]] name = "aiohttp" -version = "3.13.3" +version = "3.14.3" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "aiohappyeyeballs" }, @@ -44,27 +44,29 @@ dependencies = [ { name = "frozenlist" }, { name = "multidict" }, { name = "propcache" }, + { name = "typing-extensions" }, { name = "yarl" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/50/42/32cf8e7704ceb4481406eb87161349abb46a57fee3f008ba9cb610968646/aiohttp-3.13.3.tar.gz", hash = "sha256:a949eee43d3782f2daae4f4a2819b2cb9b0c5d3b7f7a927067cc84dafdbb9f88", size = 7844556, upload-time = "2026-01-03T17:33:05.204Z" } -wheels = [ - { url = "https://files.pythonhosted.org/packages/a0/be/4fc11f202955a69e0db803a12a062b8379c970c7c84f4882b6da17337cc1/aiohttp-3.13.3-cp312-cp312-macosx_10_13_universal2.whl", hash = "sha256:b903a4dfee7d347e2d87697d0713be59e0b87925be030c9178c5faa58ea58d5c", size = 739732, upload-time = "2026-01-03T17:30:14.23Z" }, - { url = "https://files.pythonhosted.org/packages/97/2c/621d5b851f94fa0bb7430d6089b3aa970a9d9b75196bc93bb624b0db237a/aiohttp-3.13.3-cp312-cp312-macosx_10_13_x86_64.whl", hash = "sha256:a45530014d7a1e09f4a55f4f43097ba0fd155089372e105e4bff4ca76cb1b168", size = 494293, upload-time = "2026-01-03T17:30:15.96Z" }, - { url = "https://files.pythonhosted.org/packages/5d/43/4be01406b78e1be8320bb8316dc9c42dbab553d281c40364e0f862d5661c/aiohttp-3.13.3-cp312-cp312-macosx_11_0_arm64.whl", hash = "sha256:27234ef6d85c914f9efeb77ff616dbf4ad2380be0cda40b4db086ffc7ddd1b7d", size = 493533, upload-time = "2026-01-03T17:30:17.431Z" }, - { url = "https://files.pythonhosted.org/packages/8d/a8/5a35dc56a06a2c90d4742cbf35294396907027f80eea696637945a106f25/aiohttp-3.13.3-cp312-cp312-manylinux2014_aarch64.manylinux_2_17_aarch64.manylinux_2_28_aarch64.whl", hash = "sha256:d32764c6c9aafb7fb55366a224756387cd50bfa720f32b88e0e6fa45b27dcf29", size = 1737839, upload-time = "2026-01-03T17:30:19.422Z" }, - { url = "https://files.pythonhosted.org/packages/bf/62/4b9eeb331da56530bf2e198a297e5303e1c1ebdceeb00fe9b568a65c5a0c/aiohttp-3.13.3-cp312-cp312-manylinux2014_armv7l.manylinux_2_17_armv7l.manylinux_2_31_armv7l.whl", hash = "sha256:b1a6102b4d3ebc07dad44fbf07b45bb600300f15b552ddf1851b5390202ea2e3", size = 1703932, upload-time = "2026-01-03T17:30:21.756Z" }, - { url = "https://files.pythonhosted.org/packages/7c/f6/af16887b5d419e6a367095994c0b1332d154f647e7dc2bd50e61876e8e3d/aiohttp-3.13.3-cp312-cp312-manylinux2014_ppc64le.manylinux_2_17_ppc64le.manylinux_2_28_ppc64le.whl", hash = "sha256:c014c7ea7fb775dd015b2d3137378b7be0249a448a1612268b5a90c2d81de04d", size = 1771906, upload-time = "2026-01-03T17:30:23.932Z" }, - { url = "https://files.pythonhosted.org/packages/ce/83/397c634b1bcc24292fa1e0c7822800f9f6569e32934bdeef09dae7992dfb/aiohttp-3.13.3-cp312-cp312-manylinux2014_s390x.manylinux_2_17_s390x.manylinux_2_28_s390x.whl", hash = "sha256:2b8d8ddba8f95ba17582226f80e2de99c7a7948e66490ef8d947e272a93e9463", size = 1871020, upload-time = "2026-01-03T17:30:26Z" }, - { url = "https://files.pythonhosted.org/packages/86/f6/a62cbbf13f0ac80a70f71b1672feba90fdb21fd7abd8dbf25c0105fb6fa3/aiohttp-3.13.3-cp312-cp312-manylinux2014_x86_64.manylinux_2_17_x86_64.manylinux_2_28_x86_64.whl", hash = "sha256:9ae8dd55c8e6c4257eae3a20fd2c8f41edaea5992ed67156642493b8daf3cecc", size = 1755181, upload-time = "2026-01-03T17:30:27.554Z" }, - { url = "https://files.pythonhosted.org/packages/0a/87/20a35ad487efdd3fba93d5843efdfaa62d2f1479eaafa7453398a44faf13/aiohttp-3.13.3-cp312-cp312-manylinux_2_31_riscv64.manylinux_2_39_riscv64.whl", hash = "sha256:01ad2529d4b5035578f5081606a465f3b814c542882804e2e8cda61adf5c71bf", size = 1561794, upload-time = "2026-01-03T17:30:29.254Z" }, - { url = "https://files.pythonhosted.org/packages/de/95/8fd69a66682012f6716e1bc09ef8a1a2a91922c5725cb904689f112309c4/aiohttp-3.13.3-cp312-cp312-musllinux_1_2_aarch64.whl", hash = "sha256:bb4f7475e359992b580559e008c598091c45b5088f28614e855e42d39c2f1033", size = 1697900, upload-time = "2026-01-03T17:30:31.033Z" }, - { url = "https://files.pythonhosted.org/packages/e5/66/7b94b3b5ba70e955ff597672dad1691333080e37f50280178967aff68657/aiohttp-3.13.3-cp312-cp312-musllinux_1_2_armv7l.whl", hash = "sha256:c19b90316ad3b24c69cd78d5c9b4f3aa4497643685901185b65166293d36a00f", size = 1728239, upload-time = "2026-01-03T17:30:32.703Z" }, - { url = "https://files.pythonhosted.org/packages/47/71/6f72f77f9f7d74719692ab65a2a0252584bf8d5f301e2ecb4c0da734530a/aiohttp-3.13.3-cp312-cp312-musllinux_1_2_ppc64le.whl", hash = "sha256:96d604498a7c782cb15a51c406acaea70d8c027ee6b90c569baa6e7b93073679", size = 1740527, upload-time = "2026-01-03T17:30:34.695Z" }, - { url = "https://files.pythonhosted.org/packages/fa/b4/75ec16cbbd5c01bdaf4a05b19e103e78d7ce1ef7c80867eb0ace42ff4488/aiohttp-3.13.3-cp312-cp312-musllinux_1_2_riscv64.whl", hash = "sha256:084911a532763e9d3dd95adf78a78f4096cd5f58cdc18e6fdbc1b58417a45423", size = 1554489, upload-time = "2026-01-03T17:30:36.864Z" }, - { url = "https://files.pythonhosted.org/packages/52/8f/bc518c0eea29f8406dcf7ed1f96c9b48e3bc3995a96159b3fc11f9e08321/aiohttp-3.13.3-cp312-cp312-musllinux_1_2_s390x.whl", hash = "sha256:7a4a94eb787e606d0a09404b9c38c113d3b099d508021faa615d70a0131907ce", size = 1767852, upload-time = "2026-01-03T17:30:39.433Z" }, - { url = "https://files.pythonhosted.org/packages/9d/f2/a07a75173124f31f11ea6f863dc44e6f09afe2bca45dd4e64979490deab1/aiohttp-3.13.3-cp312-cp312-musllinux_1_2_x86_64.whl", hash = "sha256:87797e645d9d8e222e04160ee32aa06bc5c163e8499f24db719e7852ec23093a", size = 1722379, upload-time = "2026-01-03T17:30:41.081Z" }, - { url = "https://files.pythonhosted.org/packages/3c/4a/1a3fee7c21350cac78e5c5cef711bac1b94feca07399f3d406972e2d8fcd/aiohttp-3.13.3-cp312-cp312-win32.whl", hash = "sha256:b04be762396457bef43f3597c991e192ee7da460a4953d7e647ee4b1c28e7046", size = 428253, upload-time = "2026-01-03T17:30:42.644Z" }, - { url = "https://files.pythonhosted.org/packages/d9/b7/76175c7cb4eb73d91ad63c34e29fc4f77c9386bba4a65b53ba8e05ee3c39/aiohttp-3.13.3-cp312-cp312-win_amd64.whl", hash = "sha256:e3531d63d3bdfa7e3ac5e9b27b2dd7ec9df3206a98e0b3445fa906f233264c57", size = 455407, upload-time = "2026-01-03T17:30:44.195Z" }, +sdist = { url = "https://files.pythonhosted.org/packages/58/d9/22ce5786ac0c1653ae8b6c23bded02c1686d11f0dbb45b31ce128e0df985/aiohttp-3.14.3.tar.gz", hash = "sha256:9491196535a88924a60afd5b5f434b5b203b6cc616250878dbdb223a8f7844bc", size = 7971213, upload-time = "2026-07-23T01:57:27.037Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/18/d4/eb96299230e20acf2efae207cb8d69051f1f68e357e5ea5e479bf6fb097a/aiohttp-3.14.3-cp312-cp312-macosx_10_13_universal2.whl", hash = "sha256:39aded8c7f3b935b54aab1d8d73c70ec0ee2d3ec3b943e0e86611bc150ba47f5", size = 754690, upload-time = "2026-07-23T01:53:47.332Z" }, + { url = "https://files.pythonhosted.org/packages/88/11/e7a70a209eb9a067c0d3212b518a0134e3484f5178c7533878b6b514d469/aiohttp-3.14.3-cp312-cp312-macosx_10_13_x86_64.whl", hash = "sha256:5bcb6ff3fdab1258a192679ff1a05d44f59626430aa05cd1a9d2447423599228", size = 509484, upload-time = "2026-07-23T01:53:51.159Z" }, + { url = "https://files.pythonhosted.org/packages/30/07/4bbc222cc8dbe31d4c3e8a5baad2286e4d42026ac0c570027b89afce6344/aiohttp-3.14.3-cp312-cp312-macosx_11_0_arm64.whl", hash = "sha256:617105e2c3018ee38d0c8ce5ee3c84f621a6d8b9f723202aacaff28449ca91ee", size = 511949, upload-time = "2026-07-23T01:53:55.083Z" }, + { url = "https://files.pythonhosted.org/packages/54/b9/42e74c46b7b7c794b995bbc1f573fb48950c38b19d8600c62a6804ee2d67/aiohttp-3.14.3-cp312-cp312-manylinux2014_aarch64.manylinux_2_17_aarch64.manylinux_2_28_aarch64.whl", hash = "sha256:f631fe87a6f30df5fbe6d79640b25e4cffb38c31c7fb6f10871517b84b0f8c1a", size = 1765282, upload-time = "2026-07-23T01:53:59.662Z" }, + { url = "https://files.pythonhosted.org/packages/6b/ed/62bc4d74363ad346d518e0720363a949f63e2e23439a79eb5813d4d29bb3/aiohttp-3.14.3-cp312-cp312-manylinux2014_armv7l.manylinux_2_17_armv7l.manylinux_2_31_armv7l.whl", hash = "sha256:a94dbaae5ae27bd849c93570669bff91e0510f33a80805738e3de72a7be0447b", size = 1741511, upload-time = "2026-07-23T01:54:04.063Z" }, + { url = "https://files.pythonhosted.org/packages/d0/9f/181e8a8bc79e47d13c7fc4540bd7a3b729d9505609c61f392a8dd2fbfe55/aiohttp-3.14.3-cp312-cp312-manylinux2014_ppc64le.manylinux_2_17_ppc64le.manylinux_2_28_ppc64le.whl", hash = "sha256:8f2f1c4c032c7cedd7d8da6f54c97b70266c6570c3108d3fdffee7188bb70529", size = 1810680, upload-time = "2026-07-23T01:54:09.882Z" }, + { url = "https://files.pythonhosted.org/packages/5c/9a/dec94d6ad694552fe3424e3f1928d7a606a5d9d9433a04e7ecdd9d38ae7f/aiohttp-3.14.3-cp312-cp312-manylinux2014_s390x.manylinux_2_17_s390x.manylinux_2_28_s390x.whl", hash = "sha256:ea05e1f97ceea523942d9b2a7d7c0359d781d683d6b043f5943a602b14da4787", size = 1905646, upload-time = "2026-07-23T01:54:13.475Z" }, + { url = "https://files.pythonhosted.org/packages/52/b7/7cd31f29d6055bd711ae6e669367fba6f5ae9de463910a793e30556a8db7/aiohttp-3.14.3-cp312-cp312-manylinux2014_x86_64.manylinux_2_17_x86_64.manylinux_2_28_x86_64.whl", hash = "sha256:543906c127fb1d929b95076db19b83fa2d46751006ff1e23b093aa5ac4d8db42", size = 1792122, upload-time = "2026-07-23T01:54:15.752Z" }, + { url = "https://files.pythonhosted.org/packages/66/73/10b1ef93afa61f4963c746257b70ced619cf31a4798671de5fdb2608501d/aiohttp-3.14.3-cp312-cp312-manylinux_2_31_riscv64.manylinux_2_39_riscv64.whl", hash = "sha256:0a5ff2dfbb9ce645fa5b8ef3e02c6c0b9cc3f6030ff863d0c51fffc50cb5541b", size = 1591127, upload-time = "2026-07-23T01:54:19.489Z" }, + { url = "https://files.pythonhosted.org/packages/49/ed/3b203fa6de1b338c14acdc06bf6ca9b043b7944f005966958c2ced932cde/aiohttp-3.14.3-cp312-cp312-musllinux_1_2_aarch64.whl", hash = "sha256:041badb8f84396357c4d3ad26de6afd7a32b112f43d3c63045c0c8278cfd2043", size = 1725210, upload-time = "2026-07-23T01:54:24.129Z" }, + { url = "https://files.pythonhosted.org/packages/28/b7/1c2aab8c706436dcc28598452488ac9cd7c409da815237c28c27d58993e6/aiohttp-3.14.3-cp312-cp312-musllinux_1_2_armv7l.whl", hash = "sha256:530125ee1163c4219af35dc3aa1206e541e7b31b6efc1a3f93b70a136f65d427", size = 1764848, upload-time = "2026-07-23T01:54:27.973Z" }, + { url = "https://files.pythonhosted.org/packages/54/50/94c28f08b131c4bf10984ea2c7a536c9920608bb2d6e7f95642c30cc87b7/aiohttp-3.14.3-cp312-cp312-musllinux_1_2_ppc64le.whl", hash = "sha256:c8653fd547c93a61aadc612007790f5555cdd18946fa48cf45e26d8ea4ea473d", size = 1777102, upload-time = "2026-07-23T01:54:31.775Z" }, + { url = "https://files.pythonhosted.org/packages/13/d4/e7d09ba7d345fb2d74440fd2fa033c5e079fac05552927705986f41a364f/aiohttp-3.14.3-cp312-cp312-musllinux_1_2_riscv64.whl", hash = "sha256:89176250f686cb9853c0fb7ead90e639e915b84a6f43eedc2a4e7ec21f1037f0", size = 1580205, upload-time = "2026-07-23T01:54:34.518Z" }, + { url = "https://files.pythonhosted.org/packages/a3/84/072a91d68e1e1eb587985b54baab94221277f877e8ef274fc213a0ceae28/aiohttp-3.14.3-cp312-cp312-musllinux_1_2_s390x.whl", hash = "sha256:3a26434dafe408229ff3403458ca58de24fb51936504decac49ce6755f77e59d", size = 1797219, upload-time = "2026-07-23T01:54:36.995Z" }, + { url = "https://files.pythonhosted.org/packages/e0/eb/aad34e897e668424d6e995da5dff8a4a09af93363d3392488772957a63aa/aiohttp-3.14.3-cp312-cp312-musllinux_1_2_x86_64.whl", hash = "sha256:d1558173930a5a8d3069cee5c92fc91c87c4dbcb099debbb3622053717145a19", size = 1768629, upload-time = "2026-07-23T01:54:40.103Z" }, + { url = "https://files.pythonhosted.org/packages/b6/2b/6bb88ddba0fecd9122aa3ebcad25996cf6c083a4a7040dbb3a4f97972af6/aiohttp-3.14.3-cp312-cp312-win32.whl", hash = "sha256:16100ad3ab8d649fdfbee87602d9d2dcdca9df0b9eda8a1b5fdc0d41f96da559", size = 451481, upload-time = "2026-07-23T01:54:42.547Z" }, + { url = "https://files.pythonhosted.org/packages/76/9b/f2f8f108da17ecef2cc3efc424e8b7ad3782b1a8360f7b8eae8ced84f6ea/aiohttp-3.14.3-cp312-cp312-win_amd64.whl", hash = "sha256:33a2d7c28d33797a2e99923dffa63f83d908a19b6bf26cfe80fa790aa5e1a75a", size = 476845, upload-time = "2026-07-23T01:54:44.853Z" }, + { url = "https://files.pythonhosted.org/packages/3e/44/28dac80a8941b604f4da10ce21097614ca1bf905ce93dca28d8d7de9c1e7/aiohttp-3.14.3-cp312-cp312-win_arm64.whl", hash = "sha256:362a3fd481769cac1a824514bcd86fda51c65e8fe6e051099e008fddde6db17c", size = 448050, upload-time = "2026-07-23T01:54:47.087Z" }, ] [[package]] @@ -2207,7 +2209,7 @@ wheels = [ [[package]] name = "litellm" -version = "1.84.0" +version = "1.96.2" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "aiohttp" }, @@ -2219,13 +2221,20 @@ dependencies = [ { name = "jsonschema" }, { name = "openai" }, { name = "pydantic" }, + { name = "pydantic-settings" }, { name = "python-dotenv" }, { name = "tiktoken" }, { name = "tokenizers" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/dd/e9/8941b7e72a187000561d932c0f2f2ed2b0fd080dfc33ba6e05961d45ca7d/litellm-1.84.0.tar.gz", hash = "sha256:b8ad0cbea11a5941b18d5af973017a340abd3d3ab41cb86e5401b970626d71a6", size = 15103206, upload-time = "2026-05-14T05:45:53.017Z" } +sdist = { url = "https://files.pythonhosted.org/packages/79/fe/d03be8a6be6914fabacef29457a2b3d02128c52c448cc6f3019b8e63488b/litellm-1.96.2.tar.gz", hash = "sha256:80d477ae092b05ce023b5084542cb3cb75999b52b1dd2e4d834fb348effc9399", size = 17682558, upload-time = "2026-08-11T21:12:24.117Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/01/a6/77fa1bbf5e42eb596b06318b3f7e6af5d0f44028046d1d598c6a595d028f/litellm-1.84.0-py3-none-any.whl", hash = "sha256:2a58d6041e6aa27d1a28dc8d8828ab500fef1a00ef74ca65e60899035010c2f2", size = 16735062, upload-time = "2026-05-14T05:45:49.927Z" }, + { url = "https://files.pythonhosted.org/packages/46/85/653d0095a163cc9a1f2f1aa629cf4493c57c3abe13bd806290fc5d57458b/litellm-1.96.2-cp310-abi3-macosx_10_12_x86_64.whl", hash = "sha256:0aa667c6fc58b20ff04fe5efaac5cf48b1e224bac32dbf22ced2e5ee695bc4ba", size = 24102871, upload-time = "2026-08-11T21:12:06.152Z" }, + { url = "https://files.pythonhosted.org/packages/29/a1/1322630271fbb291e97b54eb46baa8c4f512130a620a8428c7460b4ef414/litellm-1.96.2-cp310-abi3-macosx_11_0_arm64.whl", hash = "sha256:e5d96d16b37a043e482a134a71b0c1df4e700603fc14df22e4bd27dc9e38f5c4", size = 23760722, upload-time = "2026-08-11T21:12:09.257Z" }, + { url = "https://files.pythonhosted.org/packages/17/0e/e0e2734194072a438298e94dbc3c37879d335fdf50881ce8995daa6878d1/litellm-1.96.2-cp310-abi3-manylinux_2_28_aarch64.whl", hash = "sha256:1a42d68b903f6b605bd7744e9700b16a725f897c5edcc448e491cec14cd5a9db", size = 23898969, upload-time = "2026-08-11T21:12:11.68Z" }, + { url = "https://files.pythonhosted.org/packages/b5/84/993c40c116c699383741fd27da515c71b395609859148ea32720925eaeaf/litellm-1.96.2-cp310-abi3-manylinux_2_28_x86_64.whl", hash = "sha256:76e9c72cb6757bb7c35cd23e3e4fc028910795910ae92491076bd50f7b61f205", size = 24264588, upload-time = "2026-08-11T21:12:14.18Z" }, + { url = "https://files.pythonhosted.org/packages/d8/02/21dc260ae3fd72b23544abce23ad5f3d563e3d39a4f617bba6d98f4b9c31/litellm-1.96.2-cp310-abi3-musllinux_1_2_aarch64.whl", hash = "sha256:1f304b8854e385946469d2dcf8ef88e24357120671caeaddea11b085b0302308", size = 23973160, upload-time = "2026-08-11T21:12:16.616Z" }, + { url = "https://files.pythonhosted.org/packages/64/5d/e79f86b43f07aff7f796faf3c6d1025f6ab9da43f72b7c555a26811af191/litellm-1.96.2-cp310-abi3-musllinux_1_2_x86_64.whl", hash = "sha256:8c21a57a0f3507176492a4f65361a4302af1a0723c97e96961c7ed0e96934832", size = 24360615, upload-time = "2026-08-11T21:12:18.947Z" }, + { url = "https://files.pythonhosted.org/packages/3e/6a/c2e3058fd3766eebe5685c688d9e85adb9749e946580f46214a651661522/litellm-1.96.2-cp310-abi3-win_amd64.whl", hash = "sha256:0168e49cffecb0b45a0d044ecb7e5f7f6fc9e14d974b50f54edfa9d8e0db9c1d", size = 24154302, upload-time = "2026-08-11T21:12:21.221Z" }, ] [[package]] @@ -2679,7 +2688,7 @@ requires-dist = [ { name = "langchain-community", specifier = ">=0.4.2" }, { name = "langchain-litellm", specifier = ">=0.5.1" }, { name = "langchain-text-splitters", specifier = ">=1.1.2" }, - { name = "litellm", specifier = "==1.84.0" }, + { name = "litellm", specifier = "==1.96.2" }, { name = "llama-index", specifier = ">=0.14.0,<0.15" }, { name = "llama-index-llms-openai", specifier = ">=0.7.10,<0.8" }, { name = "lxml", specifier = ">=6.0.0,<7" }, @@ -3849,16 +3858,16 @@ wheels = [ [[package]] name = "pydantic-settings" -version = "2.13.1" +version = "2.15.0" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "pydantic" }, { name = "python-dotenv" }, { name = "typing-inspection" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/52/6d/fffca34caecc4a3f97bda81b2098da5e8ab7efc9a66e819074a11955d87e/pydantic_settings-2.13.1.tar.gz", hash = "sha256:b4c11847b15237fb0171e1462bf540e294affb9b86db4d9aa5c01730bdbe4025", size = 223826, upload-time = "2026-02-19T13:45:08.055Z" } +sdist = { url = "https://files.pythonhosted.org/packages/68/ca/31c57507b13119d7d3cfa1576dad2911a4861e3be07b579395f4e9d393f9/pydantic_settings-2.15.0.tar.gz", hash = "sha256:694b793e84f766ba76a90ebdefc01d0a9a045dab0382bee70393da93712ad117", size = 261253, upload-time = "2026-08-07T09:24:57.419Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/00/4b/ccc026168948fec4f7555b9164c724cf4125eac006e176541483d2c959be/pydantic_settings-2.13.1-py3-none-any.whl", hash = "sha256:d56fd801823dbeae7f0975e1f8c8e25c258eb75d278ea7abb5d9cebb01b56237", size = 58929, upload-time = "2026-02-19T13:45:06.034Z" }, + { url = "https://files.pythonhosted.org/packages/30/a4/2bffa9f8e804325a09867f0e9d30795c80ea9f8d62560bd1b6ad6220eb2f/pydantic_settings-2.15.0-py3-none-any.whl", hash = "sha256:0ba092c291c94baceb5eff768aa0d56400a457585bc0175925a5a5510303da42", size = 69413, upload-time = "2026-08-07T09:24:55.839Z" }, ] [[package]] From 959ee89d427755ecd8a2f3292902450401852a44 Mon Sep 17 00:00:00 2001 From: Ahtesham Quraish Date: Fri, 28 Aug 2026 21:22:38 +0500 Subject: [PATCH 10/14] Submit and link each resource under one URL, from learn_url (#3843) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Submit and link each resource under one URL, from learn_url Four places derived a resource's URL on Learn independently — the resources, video, podcast and products sitemaps — and the card href and drawer canonical derived it a fifth and sixth time. They did not agree, and the resources sitemap submitted a drawer URL for every resource on top of whatever dedicated page the per-type sitemaps submitted. A video was therefore advertised twice, and because the drawer canonicalized to itself, both URLs claimed to be the original and crawlers had no way to consolidate ranking signal onto the dedicated page. Read `learn_url` instead, which the backend computes as the resource's own page where it has one and its search drawer where it does not: - The card href points at it. `pushUrl` is untouched, so a click still opens the drawer in place and nothing changes for users; only what a crawler follows changes. - The drawer canonicalizes to it. For a video or podcast episode that is now the dedicated page rather than the drawer itself, which is what actually consolidates the signal — excluding a URL from the sitemap only stops us advertising it, and says nothing about one that is linked or shared. - The resources sitemap emits it for every published resource, so the video, podcast and products sitemaps are gone: they existed to emit dedicated-page URLs the frontend had to build itself. Collapsing the sitemaps also settles which parent scopes an episode's URL. The podcast sitemap mapped over every parent podcast, so an episode with two parents would have been submitted twice; one URL per resource cannot. The count query now asks for `limit: 1`. It reads only `count`, so it was fetching a thousand rows and discarding them. Co-Authored-By: Claude Opus 5 (1M context) * Fall back to the drawer URL when learn_url is missing learn_url is non-nullable and never blank, but a frontend deployed ahead of the backend sees the field absent, and both call sites degrade badly on a falsy value rather than merely pointing somewhere less ideal: - the drawer emitted no canonical tag at all, which is worse than the self-canonical it replaces - the card dropped its href, and BaseLearningResourceCard's `linkTarget` drops `pushUrl` along with a falsy href — so the title would be neither a link nor clickable, breaking the drawer for that card Fall back to the drawer URL in both, and assert it: reverting either fix fails the new cases. Co-Authored-By: Claude Opus 5 (1M context) * Describe what the sitemap and canonical do, not what changed Both comments narrated the edit, which only parses while holding the diff. State the durable facts instead: the sitemap needs no per-type branching and a resource with several parents appears once under its canonical parent; the drawer hands its ranking signal to the resource's dedicated page. Co-Authored-By: Claude Opus 5 (1M context) * Trim the drawer canonical comment to the invariant Co-Authored-By: Claude Opus 5 (1M context) * Narrow the falsified-learn_url cast to the one field `as never` on the whole object drops type checking for every other prop, so a later change to the resource shape would not surface here. Co-Authored-By: Claude Opus 5 (1M context) * Share the resource's page rather than a drawer over search A shared link now lands on the page that owns the content where one exists, so sharing stops seeding drawer URLs for crawlers to find. Slightly user-visible: the recipient sees the dedicated page instead of the search page with a drawer over it. The drawer reads the detail endpoint, which always carries learn_url, so the fallback here is only for a frontend deployed ahead of the backend. Co-Authored-By: Claude Opus 5 (1M context) * Read learn_url directly, without a fallback learn_url is non-nullable, so the drawer canonical, the card href and the share URL can read it as given. Drops the `||` branches, the comments explaining them, and the absent/blank cases that covered them. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Ahtesham Quraish Co-authored-by: Claude Opus 5 (1M context) --- .../(site)/articles/[slugOrId]/page.test.tsx | 7 +- .../app/(site)/news/[slugOrId]/page.test.tsx | 7 +- .../src/app/sitemaps/podcast/sitemap.test.ts | 130 ------------------ .../main/src/app/sitemaps/podcast/sitemap.ts | 82 ----------- .../src/app/sitemaps/products/sitemap.test.ts | 65 --------- .../main/src/app/sitemaps/products/sitemap.ts | 53 ------- .../app/sitemaps/resources/sitemap.test.ts | 93 +++++++++++-- .../src/app/sitemaps/resources/sitemap.ts | 29 ++-- .../app/sitemaps/sitemap-index.xml/route.ts | 8 +- .../src/app/sitemaps/video/sitemap.test.ts | 90 ------------ .../main/src/app/sitemaps/video/sitemap.ts | 88 ------------ frontends/main/src/common/metadata.test.ts | 28 +++- frontends/main/src/common/metadata.ts | 13 +- .../LearningResourceDrawer.test.tsx | 9 +- .../LearningResourceDrawer.tsx | 12 +- .../resourceDrawerPushUrl.ts | 5 +- .../ResourceCard/ResourceCard.test.tsx | 11 +- .../ResourceCard/ResourceCard.tsx | 13 +- frontends/main/src/proxy.test.ts | 2 +- 19 files changed, 167 insertions(+), 578 deletions(-) delete mode 100644 frontends/main/src/app/sitemaps/podcast/sitemap.test.ts delete mode 100644 frontends/main/src/app/sitemaps/podcast/sitemap.ts delete mode 100644 frontends/main/src/app/sitemaps/products/sitemap.test.ts delete mode 100644 frontends/main/src/app/sitemaps/products/sitemap.ts delete mode 100644 frontends/main/src/app/sitemaps/video/sitemap.test.ts delete mode 100644 frontends/main/src/app/sitemaps/video/sitemap.ts diff --git a/frontends/main/src/app/(site)/articles/[slugOrId]/page.test.tsx b/frontends/main/src/app/(site)/articles/[slugOrId]/page.test.tsx index 7acbd40d5c..518f4b4e2b 100644 --- a/frontends/main/src/app/(site)/articles/[slugOrId]/page.test.tsx +++ b/frontends/main/src/app/(site)/articles/[slugOrId]/page.test.tsx @@ -1,5 +1,4 @@ import { factories, setMockResponse, urls } from "api/test-utils" -import { resourceDrawerSearch } from "@/common/urls" import { generateMetadata } from "./page" jest.mock("@/app/getQueryClient", () => { @@ -46,7 +45,7 @@ test("resolving resource param: canonical points at the resource", async () => { searchParams: Promise.resolve({ resource: String(resource.id) }), }) - expect(meta.alternates?.canonical).toContain( - resourceDrawerSearch(resource.id, resource.title), - ) + // The drawer canonicalizes to the resource's location on Learn, whatever page + // it is opened over. + expect(meta.alternates?.canonical).toBe(resource.learn_url) }) diff --git a/frontends/main/src/app/(site)/news/[slugOrId]/page.test.tsx b/frontends/main/src/app/(site)/news/[slugOrId]/page.test.tsx index 99d4a60be6..aba8921935 100644 --- a/frontends/main/src/app/(site)/news/[slugOrId]/page.test.tsx +++ b/frontends/main/src/app/(site)/news/[slugOrId]/page.test.tsx @@ -1,6 +1,5 @@ import { factories, setMockResponse, urls } from "api/test-utils" import { nextNavigationMocks } from "ol-test-utilities/mocks/nextNavigation" -import { resourceDrawerSearch } from "@/common/urls" import { generateMetadata } from "./page" jest.mock("@/app/getQueryClient", () => { @@ -57,9 +56,9 @@ test("resolving resource param: canonical, title, description and image come fro alt: resource.image?.alt, }), ]) - expect(meta.alternates?.canonical).toContain( - resourceDrawerSearch(resource.id, resource.title), - ) + // The drawer canonicalizes to the resource's location on Learn, whatever page + // it is opened over. + expect(meta.alternates?.canonical).toBe(resource.learn_url) }) test("dead resource param: the article not-founds", async () => { diff --git a/frontends/main/src/app/sitemaps/podcast/sitemap.test.ts b/frontends/main/src/app/sitemaps/podcast/sitemap.test.ts deleted file mode 100644 index 9bc94fa28b..0000000000 --- a/frontends/main/src/app/sitemaps/podcast/sitemap.test.ts +++ /dev/null @@ -1,130 +0,0 @@ -import { faker } from "@faker-js/faker/locale/en" -import { generateSitemaps, default as sitemap } from "./sitemap" -import { setMockResponse, urls, factories } from "api/test-utils" -import { ResourceTypeEnum } from "api" -import { podcastPageView, podcastEpisodePageView } from "@/common/urls" - -const RESOURCE_TYPES = [ - ResourceTypeEnum.Podcast, - ResourceTypeEnum.PodcastEpisode, -] - -describe("Podcast Sitemaps", () => { - it("returns expected sitemap params", async () => { - const pages = faker.number.int({ min: 4, max: 6 }) - const summaries = factories.learningResources.resourceSummaries({ - count: pages * 1_000 - 350, - pageSize: 1, - }) - - setMockResponse.get( - urls.learningResources.summaryList({ - limit: 1, - resource_type: RESOURCE_TYPES, - }), - summaries, - ) - - const result = await generateSitemaps() - expect(result).toHaveLength(pages) - expect(result).toEqual( - new Array(pages).fill(null).map((_, index) => ({ - id: index, - location: `http://test.learn.odl.local:8062/sitemaps/podcast/sitemap/${index}.xml`, - })), - ) - }) - - it("generates expected URLs for podcast resources", async () => { - const page = faker.number.int({ min: 5, max: 10 }) - const results = Array.from({ length: 3 }, () => - factories.learningResources.resourceSummary({ - resource_type: ResourceTypeEnum.Podcast, - }), - ) - - setMockResponse.get( - urls.learningResources.summaryList({ - limit: 1_000, - offset: page * 1_000, - resource_type: RESOURCE_TYPES, - }), - { count: results.length, next: null, previous: null, results }, - ) - - const sitemapPage = await sitemap({ id: Promise.resolve(String(page)) }) - expect(sitemapPage).toEqual( - results.map((resource) => ({ - url: `http://test.learn.odl.local:8062${podcastPageView( - String(resource.id), - resource.title, - )}`, - lastModified: resource.last_modified ?? undefined, - })), - ) - }) - - it("generates expected URLs for podcast episode resources", async () => { - // Use a non-overlapping range from the podcast test (which uses { min: 5, max: 10 }) - // to avoid query cache collisions on the same list URL. - const page = faker.number.int({ min: 11, max: 20 }) - const podcastId1 = faker.number.int() - const podcastId2 = faker.number.int() - const episodeWithMultipleParents = - factories.learningResources.resourceSummary({ - resource_type: ResourceTypeEnum.PodcastEpisode, - canonical_parent_ids: [podcastId1, podcastId2], - }) - const episodeWithOneParent = factories.learningResources.resourceSummary({ - resource_type: ResourceTypeEnum.PodcastEpisode, - canonical_parent_ids: [podcastId1], - }) - const episodeWithoutParent = factories.learningResources.resourceSummary({ - resource_type: ResourceTypeEnum.PodcastEpisode, - }) - const results = [ - episodeWithMultipleParents, - episodeWithOneParent, - episodeWithoutParent, - ] - - setMockResponse.get( - urls.learningResources.summaryList({ - limit: 1_000, - offset: page * 1_000, - resource_type: RESOURCE_TYPES, - }), - { count: results.length, next: null, previous: null, results }, - ) - - const sitemapPage = await sitemap({ id: Promise.resolve(String(page)) }) - // episodeWithoutParent should be excluded; episodeWithMultipleParents emits one entry per parent - const base = "http://test.learn.odl.local:8062" - expect(sitemapPage).toEqual([ - { - url: `${base}${podcastEpisodePageView( - String(episodeWithMultipleParents.id), - String(podcastId1), - episodeWithMultipleParents.title, - )}`, - lastModified: episodeWithMultipleParents.last_modified ?? undefined, - }, - { - url: `${base}${podcastEpisodePageView( - String(episodeWithMultipleParents.id), - String(podcastId2), - episodeWithMultipleParents.title, - )}`, - lastModified: episodeWithMultipleParents.last_modified ?? undefined, - }, - { - url: `${base}${podcastEpisodePageView( - String(episodeWithOneParent.id), - String(podcastId1), - episodeWithOneParent.title, - )}`, - lastModified: episodeWithOneParent.last_modified ?? undefined, - }, - ]) - }) -}) diff --git a/frontends/main/src/app/sitemaps/podcast/sitemap.ts b/frontends/main/src/app/sitemaps/podcast/sitemap.ts deleted file mode 100644 index e35d954388..0000000000 --- a/frontends/main/src/app/sitemaps/podcast/sitemap.ts +++ /dev/null @@ -1,82 +0,0 @@ -import { requiredEnv } from "@/env" -import { getQueryClient } from "@/app/getQueryClient" -import { learningResourceQueries } from "api/hooks/learningResources" -import { ResourceTypeEnum } from "api" -import { podcastPageView, podcastEpisodePageView } from "@/common/urls" -import type { GenerateSitemapResult } from "../types" -import { - dangerouslyDetectProductionBuildPhase, - constructSitemap, -} from "../util" - -const PAGE_SIZE = 1_000 - -const RESOURCE_TYPES = [ - ResourceTypeEnum.Podcast, - ResourceTypeEnum.PodcastEpisode, -] - -/** - * As of NextJS 15.5.3, sitemaps are ALWAYS generated at build time, even with - * the force-dynamic below (this may be a NextJS bug?). However, the - * force-dynamic does force re-generation when requests are made in production. - */ -export const dynamic = "force-dynamic" - -export async function generateSitemaps(): Promise { - /** - * NextJS runs this at build time (despite force-dynamic above). - * Early exit here to avoid the useless build-time API calls. - */ - if (dangerouslyDetectProductionBuildPhase()) return [] - const BASE_URL = requiredEnv("NEXT_PUBLIC_ORIGIN") - - const queryClient = getQueryClient() - const { count } = await queryClient.fetchQuery( - learningResourceQueries.summaryList({ - limit: 1, - resource_type: RESOURCE_TYPES, - }), - ) - - const pages = Math.ceil(count / PAGE_SIZE) - - return new Array(pages).fill(null).map((_, index) => ({ - id: index, - location: `${BASE_URL}/sitemaps/podcast/sitemap/${index}.xml`, - })) -} - -export default constructSitemap(async (page) => { - const BASE_URL = requiredEnv("NEXT_PUBLIC_ORIGIN") - const queryClient = getQueryClient() - const data = await queryClient.fetchQuery( - learningResourceQueries.summaryList({ - limit: PAGE_SIZE, - offset: page * PAGE_SIZE, - resource_type: RESOURCE_TYPES, - }), - ) - - return data.results.flatMap((resource) => { - if (resource.resource_type === ResourceTypeEnum.Podcast) { - return [ - { - url: `${BASE_URL}${podcastPageView(String(resource.id), resource.title)}`, - lastModified: resource.last_modified ?? undefined, - }, - ] - } - if (resource.resource_type === ResourceTypeEnum.PodcastEpisode) { - return resource.canonical_parent_ids.map((parentPodcastId) => ({ - url: `${BASE_URL}${podcastEpisodePageView( - String(resource.id), - String(parentPodcastId), - resource.title, - )}`, - lastModified: resource.last_modified ?? undefined, - })) - } - return [] - }) -}) diff --git a/frontends/main/src/app/sitemaps/products/sitemap.test.ts b/frontends/main/src/app/sitemaps/products/sitemap.test.ts deleted file mode 100644 index 4ecdcacdde..0000000000 --- a/frontends/main/src/app/sitemaps/products/sitemap.test.ts +++ /dev/null @@ -1,65 +0,0 @@ -import { faker } from "@faker-js/faker/locale/en" -import { generateSitemaps, default as sitemap } from "./sitemap" -import { setMockResponse, urls, factories } from "api/test-utils" -import { PlatformEnum, ResourceTypeEnum } from "api" - -const { resourceSummaries } = factories.learningResources - -describe("Product Sitemaps", () => { - it("returns expected sitemap params", async () => { - const pages = faker.number.int({ min: 4, max: 6 }) - const summaries = resourceSummaries({ - count: pages * 1_000 - 350, - pageSize: 10, - }) - - setMockResponse.get( - urls.learningResources.summaryList({ - limit: 1_000, - platform: [PlatformEnum.Mitxonline], - resource_type: [ResourceTypeEnum.Course, ResourceTypeEnum.Program], - }), - summaries, - ) - - const result = await generateSitemaps() - expect(result).toHaveLength(pages) - expect(result).toEqual( - new Array(pages).fill(null).map((_, index) => ({ - id: index, - location: `http://test.learn.odl.local:8062/sitemaps/products/sitemap/${index}.xml`, - })), - ) - }) - - it("generates expected sitemap/", async () => { - const page = faker.number.int({ min: 5, max: 10 }) - const summaries = resourceSummaries({ - count: 15_000, - pageSize: 5, - }) - summaries.results[0].url = null - summaries.results[1].url = - "http://test.learn.odl.local:8062/programs/program-1" - - setMockResponse.get( - urls.learningResources.summaryList({ - limit: 1_000, - offset: page * 1_000, - platform: [PlatformEnum.Mitxonline], - resource_type: [ResourceTypeEnum.Course, ResourceTypeEnum.Program], - }), - summaries, - ) - - const sitemapPage = await sitemap({ id: Promise.resolve(String(page)) }) - expect(sitemapPage).toEqual( - summaries.results - .filter((resource) => Boolean(resource.url)) - .map((resource) => ({ - url: resource.url as string, - lastModified: resource.last_modified ?? undefined, - })), - ) - }) -}) diff --git a/frontends/main/src/app/sitemaps/products/sitemap.ts b/frontends/main/src/app/sitemaps/products/sitemap.ts deleted file mode 100644 index 26eebac545..0000000000 --- a/frontends/main/src/app/sitemaps/products/sitemap.ts +++ /dev/null @@ -1,53 +0,0 @@ -import { requiredEnv } from "@/env" -import { getQueryClient } from "@/app/getQueryClient" -import { learningResourceQueries } from "api/hooks/learningResources" -import { PlatformEnum, ResourceTypeEnum } from "api" -import type { GenerateSitemapResult } from "../types" -import { - dangerouslyDetectProductionBuildPhase, - constructSitemap, -} from "../util" - -const PAGE_SIZE = 1_000 - -export const dynamic = "force-dynamic" - -export async function generateSitemaps(): Promise { - if (dangerouslyDetectProductionBuildPhase()) return [] - const BASE_URL = requiredEnv("NEXT_PUBLIC_ORIGIN") - - const queryClient = getQueryClient() - const { count } = await queryClient.fetchQuery( - learningResourceQueries.summaryList({ - limit: PAGE_SIZE, - platform: [PlatformEnum.Mitxonline], - resource_type: [ResourceTypeEnum.Course, ResourceTypeEnum.Program], - }), - ) - - const pages = Math.ceil(count / PAGE_SIZE) - - return new Array(pages).fill(null).map((_, index) => ({ - id: index, - location: `${BASE_URL}/sitemaps/products/sitemap/${index}.xml`, - })) -} - -export default constructSitemap(async (page) => { - const queryClient = getQueryClient() - const data = await queryClient.fetchQuery( - learningResourceQueries.summaryList({ - limit: PAGE_SIZE, - offset: page * PAGE_SIZE, - platform: [PlatformEnum.Mitxonline], - resource_type: [ResourceTypeEnum.Course, ResourceTypeEnum.Program], - }), - ) - - return data.results - .filter((resource) => Boolean(resource.url)) - .map((resource) => ({ - url: resource.url as string, - lastModified: resource.last_modified ?? undefined, - })) -}) diff --git a/frontends/main/src/app/sitemaps/resources/sitemap.test.ts b/frontends/main/src/app/sitemaps/resources/sitemap.test.ts index 1759c4a93c..1fca51e028 100644 --- a/frontends/main/src/app/sitemaps/resources/sitemap.test.ts +++ b/frontends/main/src/app/sitemaps/resources/sitemap.test.ts @@ -1,13 +1,12 @@ import { faker } from "@faker-js/faker/locale/en" import { generateSitemaps, default as sitemap } from "./sitemap" import { setMockResponse, urls, factories } from "api/test-utils" -import { resourceDrawerSearch } from "@/common/urls" +import { ResourceTypeEnum } from "api" -const { resourceSummaries } = factories.learningResources +const { resourceSummaries, resourceSummary } = factories.learningResources describe("Resource Sitemaps", () => { it("returns expected sitemap params", async () => { - // Mock API response with fewer resources than TRY_FOR_PAGE_SIZE const pages = faker.number.int({ min: 4, max: 6 }) const summaries = resourceSummaries({ count: pages * 1_000 - 350, @@ -15,7 +14,7 @@ describe("Resource Sitemaps", () => { }) setMockResponse.get( - urls.learningResources.summaryList({ limit: 1_000 }), + urls.learningResources.summaryList({ limit: 1 }), summaries, ) @@ -30,7 +29,7 @@ describe("Resource Sitemaps", () => { ) }) - it("generates expected sitemap/", async () => { + it("submits each resource under its learn_url", async () => { const page = faker.number.int({ min: 5, max: 10 }) const summaries = resourceSummaries({ count: 15_000, @@ -46,17 +45,89 @@ describe("Resource Sitemaps", () => { ) const sitemapPage = await sitemap({ id: Promise.resolve(String(page)) }) + expect(sitemapPage).toEqual( summaries.results.map((resource) => ({ // "&" must be pre-escaped: NextJS inserts urls into tags verbatim - url: `http://test.learn.odl.local:8062${resourceDrawerSearch( - resource.id, - resource.title, - )}`.replaceAll("&", "&"), + url: resource.learn_url.replaceAll("&", "&"), lastModified: resource.last_modified ?? undefined, })), ) - // guard against the escape being vacuous (no multi-param urls generated) - expect(sitemapPage[0].url).toContain("&resource_title=") + }) + + /** + * The whole point of driving this from learn_url: a resource with a dedicated + * page is submitted under that page, not under a drawer URL that would compete + * with it. The backend decides, so the sitemap needs no per-type branching. + */ + it.each([ + { + page: 20, + resourceType: ResourceTypeEnum.Video, + learnUrl: + "http://test.learn.odl.local:8062/video/6395/lecture-11?playlist=6384", + }, + { + page: 21, + resourceType: ResourceTypeEnum.PodcastEpisode, + learnUrl: + "http://test.learn.odl.local:8062/podcast/14144/podcast_episode/14145/ep-4", + }, + { + page: 22, + resourceType: ResourceTypeEnum.Course, + learnUrl: + "http://test.learn.odl.local:8062/courses/course-v1:MITxT+14.100x", + }, + { + page: 23, + resourceType: ResourceTypeEnum.Program, + learnUrl: "http://test.learn.odl.local:8062/search?resource=99", + }, + ])( + "emits the learn_url for $resourceType", + async ({ page, resourceType, learnUrl }) => { + const resource = resourceSummary({ + resource_type: resourceType, + learn_url: learnUrl, + }) + + setMockResponse.get( + urls.learningResources.summaryList({ + limit: 1_000, + offset: page * 1_000, + }), + { count: 1, next: null, previous: null, results: [resource] }, + ) + + const sitemapPage = await sitemap({ id: Promise.resolve(String(page)) }) + + expect(sitemapPage).toEqual([ + { + url: learnUrl.replaceAll("&", "&"), + lastModified: resource.last_modified ?? undefined, + }, + ]) + }, + ) + + it("submits no resource more than once", async () => { + const results = [ + resourceSummary({ resource_type: ResourceTypeEnum.Video }), + resourceSummary({ resource_type: ResourceTypeEnum.PodcastEpisode }), + resourceSummary({ resource_type: ResourceTypeEnum.Course }), + ] + + setMockResponse.get( + urls.learningResources.summaryList({ limit: 1_000, offset: 30_000 }), + { count: results.length, next: null, previous: null, results }, + ) + + const sitemapPage = await sitemap({ id: Promise.resolve("30") }) + + expect(sitemapPage).toHaveLength(results.length) + expect(new Set(sitemapPage.map((entry) => entry.url)).size).toBe( + results.length, + ) }) }) diff --git a/frontends/main/src/app/sitemaps/resources/sitemap.ts b/frontends/main/src/app/sitemaps/resources/sitemap.ts index 2ea41a5f05..054ca00bbf 100644 --- a/frontends/main/src/app/sitemaps/resources/sitemap.ts +++ b/frontends/main/src/app/sitemaps/resources/sitemap.ts @@ -1,7 +1,6 @@ import { requiredEnv } from "@/env" import { getQueryClient } from "@/app/getQueryClient" import { learningResourceQueries } from "api/hooks/learningResources" -import { resourceDrawerSearch } from "@/common/urls" import type { GenerateSitemapResult } from "../types" import { dangerouslyDetectProductionBuildPhase, @@ -11,9 +10,13 @@ import { const PAGE_SIZE = 1_000 /** - * As of NextJS 15.5.3, sitemaps are ALWAYS generated at build time, even with - * the force-dynamic below (this may be a NextJS bug?). However, the - * force-dynamic does force re-generation when requests are made in production. + * Every published resource, submitted under exactly one URL. + * + * `learn_url` is the resource's location within Learn — its own page where it + * has one (videos, playlists, podcasts, episodes, MITx Online courses and + * programs), else the search drawer. Because the backend decides, this sitemap + * covers every resource type with no per-type branching, and an episode or + * video with multiple parents appears once, under its canonical parent. */ export const dynamic = "force-dynamic" @@ -26,9 +29,9 @@ export async function generateSitemaps(): Promise { const BASE_URL = requiredEnv("NEXT_PUBLIC_ORIGIN") const queryClient = getQueryClient() const { count } = await queryClient.fetchQuery( - learningResourceQueries.summaryList({ - limit: PAGE_SIZE, - }), + // Only the count is read; a full page of rows would be fetched and thrown + // away. + learningResourceQueries.summaryList({ limit: 1 }), ) const pages = Math.ceil(count / PAGE_SIZE) @@ -41,7 +44,6 @@ export async function generateSitemaps(): Promise { } export default constructSitemap(async (page) => { - const BASE_URL = requiredEnv("NEXT_PUBLIC_ORIGIN") const queryClient = getQueryClient() const data = await queryClient.fetchQuery( learningResourceQueries.summaryList({ @@ -51,7 +53,16 @@ export default constructSitemap(async (page) => { ) return data.results.map((resource) => ({ - url: `${BASE_URL}${resourceDrawerSearch(resource.id, resource.title)}`, + // Absolute already — the backend builds it from APP_BASE_URL. + url: resource.learn_url, + /** + * `last_modified` is an upstream change timestamp (openedx course + * `modified`, program `data_modified_timestamp`, the OCW course's S3 object + * mtime), not an ingest clock, so an ETL run over unchanged content does not + * move it. Coarser than a true content-change date, but it tracks the + * metadata these pages actually render. Omitting it would leave crawlers + * with no signal at all. + */ lastModified: resource.last_modified ?? undefined, })) }) diff --git a/frontends/main/src/app/sitemaps/sitemap-index.xml/route.ts b/frontends/main/src/app/sitemaps/sitemap-index.xml/route.ts index 28d84ff5e0..84641071bf 100644 --- a/frontends/main/src/app/sitemaps/sitemap-index.xml/route.ts +++ b/frontends/main/src/app/sitemaps/sitemap-index.xml/route.ts @@ -2,9 +2,6 @@ import { requiredEnv } from "@/env" import { NextResponse } from "next/server" import * as resourceSitemap from "../resources/sitemap" import * as channelsSitemap from "../channels/sitemap" -import * as productsSitemap from "../products/sitemap" -import * as videoSitemap from "../video/sitemap" -import * as podcastSitemap from "../podcast/sitemap" export async function GET() { const content = await buildSitemapIndex() @@ -20,12 +17,11 @@ async function buildSitemapIndex(): Promise { // Read at request time so this works in the standalone Docker image where // NEXT_PUBLIC_ORIGIN is injected by Kubernetes, not baked at build time. const BASE_URL = requiredEnv("NEXT_PUBLIC_ORIGIN") + // The resources sitemap covers every published resource, each under the one + // URL `learn_url` names, so there are no per-type sitemaps to combine. const sitemaps = await Promise.all([ resourceSitemap.generateSitemaps(), channelsSitemap.generateSitemaps(), - productsSitemap.generateSitemaps(), - videoSitemap.generateSitemaps(), - podcastSitemap.generateSitemaps(), ]) const sitemapUrls = [ `${BASE_URL}/sitemaps/static/sitemap.xml`, diff --git a/frontends/main/src/app/sitemaps/video/sitemap.test.ts b/frontends/main/src/app/sitemaps/video/sitemap.test.ts deleted file mode 100644 index 8bfc933049..0000000000 --- a/frontends/main/src/app/sitemaps/video/sitemap.test.ts +++ /dev/null @@ -1,90 +0,0 @@ -import { faker } from "@faker-js/faker/locale/en" -import { generateSitemaps, default as sitemap } from "./sitemap" -import { setMockResponse, urls, factories } from "api/test-utils" -import { ResourceTypeEnum } from "api" -import { videoDetailPageView, videoPlaylistPageView } from "@/common/urls" - -const RESOURCE_TYPES = [ResourceTypeEnum.Video, ResourceTypeEnum.VideoPlaylist] - -describe("Video Sitemaps", () => { - it("returns expected sitemap params", async () => { - const pages = faker.number.int({ min: 4, max: 6 }) - const summaries = factories.learningResources.resourceSummaries({ - count: pages * 1_000 - 350, - pageSize: 1, - }) - - setMockResponse.get( - urls.learningResources.summaryList({ - limit: 1, - resource_type: RESOURCE_TYPES, - }), - summaries, - ) - - const result = await generateSitemaps() - expect(result).toHaveLength(pages) - expect(result).toEqual( - new Array(pages).fill(null).map((_, index) => ({ - id: index, - location: `http://test.learn.odl.local:8062/sitemaps/video/sitemap/${index}.xml`, - })), - ) - }) - - it("generates expected URLs for video and video playlist resources", async () => { - const page = faker.number.int({ min: 5, max: 10 }) - const playlistId = faker.number.int() - const otherPlaylistId = faker.number.int() - // A video in several playlists is addressed by its first, matching the - // canonical tag and the bare-URL redirect on the video page. - const videoWithPlaylists = factories.learningResources.resourceSummary({ - resource_type: ResourceTypeEnum.Video, - canonical_parent_ids: [playlistId, otherPlaylistId], - }) - const videoWithoutPlaylist = factories.learningResources.resourceSummary({ - resource_type: ResourceTypeEnum.Video, - }) - const playlist = factories.learningResources.resourceSummary({ - resource_type: ResourceTypeEnum.VideoPlaylist, - }) - const results = [videoWithPlaylists, videoWithoutPlaylist, playlist] - - setMockResponse.get( - urls.learningResources.summaryList({ - limit: 1_000, - offset: page * 1_000, - resource_type: RESOURCE_TYPES, - }), - { count: results.length, next: null, previous: null, results }, - ) - - const sitemapPage = await sitemap({ id: Promise.resolve(String(page)) }) - const base = "http://test.learn.odl.local:8062" - expect(sitemapPage).toEqual([ - { - url: `${base}${videoDetailPageView( - videoWithPlaylists.id, - playlistId, - videoWithPlaylists.title, - )}`, - lastModified: videoWithPlaylists.last_modified ?? undefined, - }, - { - url: `${base}${videoDetailPageView( - videoWithoutPlaylist.id, - undefined, - videoWithoutPlaylist.title, - )}`, - lastModified: videoWithoutPlaylist.last_modified ?? undefined, - }, - { - url: `${base}${videoPlaylistPageView( - String(playlist.id), - playlist.title, - )}`, - lastModified: playlist.last_modified ?? undefined, - }, - ]) - }) -}) diff --git a/frontends/main/src/app/sitemaps/video/sitemap.ts b/frontends/main/src/app/sitemaps/video/sitemap.ts deleted file mode 100644 index 310db360f7..0000000000 --- a/frontends/main/src/app/sitemaps/video/sitemap.ts +++ /dev/null @@ -1,88 +0,0 @@ -import { requiredEnv } from "@/env" -import { getQueryClient } from "@/app/getQueryClient" -import { learningResourceQueries } from "api/hooks/learningResources" -import { ResourceTypeEnum } from "api" -import { videoDetailPageView, videoPlaylistPageView } from "@/common/urls" -import type { GenerateSitemapResult } from "../types" -import { - dangerouslyDetectProductionBuildPhase, - constructSitemap, -} from "../util" - -const PAGE_SIZE = 1_000 - -const RESOURCE_TYPES = [ResourceTypeEnum.Video, ResourceTypeEnum.VideoPlaylist] - -/** - * As of NextJS 15.5.3, sitemaps are ALWAYS generated at build time, even with - * the force-dynamic below (this may be a NextJS bug?). However, the - * force-dynamic does force re-generation when requests are made in production. - */ -export const dynamic = "force-dynamic" - -export async function generateSitemaps(): Promise { - /** - * NextJS runs this at build time (despite force-dynamic above). - * Early exit here to avoid the useless build-time API calls. - */ - if (dangerouslyDetectProductionBuildPhase()) return [] - const BASE_URL = requiredEnv("NEXT_PUBLIC_ORIGIN") - - const queryClient = getQueryClient() - const { count } = await queryClient.fetchQuery( - learningResourceQueries.summaryList({ - limit: 1, - resource_type: RESOURCE_TYPES, - }), - ) - - const pages = Math.ceil(count / PAGE_SIZE) - - return new Array(pages).fill(null).map((_, index) => ({ - id: index, - location: `${BASE_URL}/sitemaps/video/sitemap/${index}.xml`, - })) -} - -export default constructSitemap(async (page) => { - const BASE_URL = requiredEnv("NEXT_PUBLIC_ORIGIN") - const queryClient = getQueryClient() - const data = await queryClient.fetchQuery( - learningResourceQueries.summaryList({ - limit: PAGE_SIZE, - offset: page * PAGE_SIZE, - resource_type: RESOURCE_TYPES, - }), - ) - - return data.results.flatMap((resource) => { - if (resource.resource_type === ResourceTypeEnum.Video) { - // Emit the true canonical: a video with playlists redirects bare → - // playlists[0], so include it (couples to playlists[0] ordering, same as - // the canonical tag + page redirect — no new coupling). - const [firstPlaylist] = resource.canonical_parent_ids - return [ - { - url: `${BASE_URL}${videoDetailPageView( - resource.id, - firstPlaylist, - resource.title, - )}`, - lastModified: resource.last_modified ?? undefined, - }, - ] - } - if (resource.resource_type === ResourceTypeEnum.VideoPlaylist) { - return [ - { - url: `${BASE_URL}${videoPlaylistPageView( - String(resource.id), - resource.title, - )}`, - lastModified: resource.last_modified ?? undefined, - }, - ] - } - return [] - }) -}) diff --git a/frontends/main/src/common/metadata.test.ts b/frontends/main/src/common/metadata.test.ts index 7abefca8c6..ded6dac2cc 100644 --- a/frontends/main/src/common/metadata.test.ts +++ b/frontends/main/src/common/metadata.test.ts @@ -81,8 +81,27 @@ describe("standardizeMetadata", () => { }) describe("getMetadataAsync drawer canonical", () => { - test("emits a slugged separate-param canonical for a valid ?resource=", async () => { - const resource = factories.learningResources.course() + test("canonicalizes the drawer to the resource's learn_url", async () => { + // A resource with no page of its own: learn_url is this drawer URL, so the + // canonical is self-referential. + const resource = factories.learningResources.course({ + learn_url: "http://test.learn.odl.local:8062/search?resource=42", + }) + setMockResponse.get( + urls.learningResources.details({ id: resource.id }), + resource, + ) + const meta = await getMetadataAsync({ + searchParams: Promise.resolve({ resource: String(resource.id) }), + }) + expect(meta.alternates?.canonical).toBe(resource.learn_url) + }) + + test("canonicalizes the drawer to a dedicated page where one exists", async () => { + const resource = factories.learningResources.video({ + learn_url: + "http://test.learn.odl.local:8062/video/6395/lecture-11?playlist=6384", + }) setMockResponse.get( urls.learningResources.details({ id: resource.id }), resource, @@ -90,8 +109,9 @@ describe("getMetadataAsync drawer canonical", () => { const meta = await getMetadataAsync({ searchParams: Promise.resolve({ resource: String(resource.id) }), }) - expect(meta.alternates?.canonical).toContain(`resource=${resource.id}`) - expect(meta.alternates?.canonical).toMatch(/resource_title=[a-z0-9-]+/) + expect(meta.alternates?.canonical).toBe( + "http://test.learn.odl.local:8062/video/6395/lecture-11?playlist=6384", + ) }) test("no canonical override when ?resource= is not a valid id", async () => { diff --git a/frontends/main/src/common/metadata.ts b/frontends/main/src/common/metadata.ts index e29769d13a..f59f9d1054 100644 --- a/frontends/main/src/common/metadata.ts +++ b/frontends/main/src/common/metadata.ts @@ -1,8 +1,5 @@ import { env } from "@/env" -import { - canonicalResourceDrawerUrl, - RESOURCE_DRAWER_PARAMS, -} from "@/common/urls" +import { RESOURCE_DRAWER_PARAMS } from "@/common/urls" import { parseResourceId } from "@/common/slugs" import type { ServerSearchParam } from "@/common/searchParams" import { htmlToPlainText } from "@/common/htmlToPlainText" @@ -95,7 +92,13 @@ export const getMetadataAsync = async ({ image = data.image.url imageAlt = data.image.alt || "" } - alts.canonical = canonicalResourceDrawerUrl(learningResourceId, data?.title) + /** + * Canonicalize the drawer to the resource's location on Learn: its own page + * where it has one, else this drawer URL itself. The backend owns the + * choice, so the canonical here, the card hrefs, and the sitemap cannot + * disagree. + */ + alts.canonical = data.learn_url } return standardizeMetadata({ diff --git a/frontends/main/src/page-components/LearningResourceDrawer/LearningResourceDrawer.test.tsx b/frontends/main/src/page-components/LearningResourceDrawer/LearningResourceDrawer.test.tsx index b178bbea49..374b6c6e2e 100644 --- a/frontends/main/src/page-components/LearningResourceDrawer/LearningResourceDrawer.test.tsx +++ b/frontends/main/src/page-components/LearningResourceDrawer/LearningResourceDrawer.test.tsx @@ -11,10 +11,7 @@ import { import LearningResourceDrawer from "./LearningResourceDrawer" import { urls, factories, setMockResponse } from "api/test-utils" import { LearningResourceExpanded } from "../LearningResourceExpanded/LearningResourceExpanded" -import { - canonicalResourceDrawerUrl, - RESOURCE_DRAWER_PARAMS, -} from "@/common/urls" +import { RESOURCE_DRAWER_PARAMS } from "@/common/urls" import { LearningResource, ResourceTypeEnum } from "api" import { makeUserSettings } from "@/test-utils/factories" import type { User } from "api/hooks/user" @@ -112,8 +109,8 @@ describe("LearningResourceDrawer", () => { await waitFor(() => { expectProps(LearningResourceExpanded, { resource, - // Share links use the canonical slugged drawer URL - shareUrl: canonicalResourceDrawerUrl(resource.id, resource.title), + // Share links use the resource's location on Learn + shareUrl: resource.learn_url, }) }) await screen.findByText(resource.title) diff --git a/frontends/main/src/page-components/LearningResourceDrawer/LearningResourceDrawer.tsx b/frontends/main/src/page-components/LearningResourceDrawer/LearningResourceDrawer.tsx index 7a541c490b..23a1a22673 100644 --- a/frontends/main/src/page-components/LearningResourceDrawer/LearningResourceDrawer.tsx +++ b/frontends/main/src/page-components/LearningResourceDrawer/LearningResourceDrawer.tsx @@ -8,10 +8,7 @@ import type { } from "ol-components" import { useLearningResourcesDetail } from "api/hooks/learningResources" -import { - canonicalResourceDrawerUrl, - RESOURCE_DRAWER_PARAMS, -} from "@/common/urls" +import { RESOURCE_DRAWER_PARAMS } from "@/common/urls" import { parseResourceId } from "@/common/slugs" import { useCanonicalizeResourceParam } from "./useCanonicalizeResourceParam" import { useUserMe } from "api/hooks/user" @@ -228,7 +225,12 @@ const DrawerContent: React.FC<{ bottomCarousels={bottomCarousels} chatExpanded={chatExpanded} user={user} - shareUrl={canonicalResourceDrawerUrl(resourceId, resource.data?.title)} + /** + * Share the resource's location on Learn — its own page where it has + * one — so a shared link lands on the page that owns the content rather + * than on a drawer over the search page. + */ + shareUrl={resource.data?.learn_url} inLearningPath={inLearningPath} inUserList={inUserList} onAddToLearningPathClick={handleAddToLearningPathClick} diff --git a/frontends/main/src/page-components/LearningResourceDrawer/resourceDrawerPushUrl.ts b/frontends/main/src/page-components/LearningResourceDrawer/resourceDrawerPushUrl.ts index 9732c3d225..28fba2477d 100644 --- a/frontends/main/src/page-components/LearningResourceDrawer/resourceDrawerPushUrl.ts +++ b/frontends/main/src/page-components/LearningResourceDrawer/resourceDrawerPushUrl.ts @@ -4,9 +4,8 @@ import { setResourceParams } from "@/common/urls" /** * The URL a card click pushes: the *current* page's URL plus the drawer's * params, preserving every other param and the fragment. This is deliberately - * not the card's href — the href is the canonical `/search?resource=…` URL - * (see `resourceDrawerSearch`), which is what crawlers and Copy Link Address - * should get. + * not the card's href — the href is the resource's `learn_url`, its location on + * Learn, which is what crawlers and Copy Link Address should get. * * Deliberately not a hook: it reads `window.location` at click time instead of * subscribing to `useSearchParams()`, which is a dynamic API — a client diff --git a/frontends/main/src/page-components/ResourceCard/ResourceCard.test.tsx b/frontends/main/src/page-components/ResourceCard/ResourceCard.test.tsx index 8b4c2f3cfe..ea6f7bc299 100644 --- a/frontends/main/src/page-components/ResourceCard/ResourceCard.test.tsx +++ b/frontends/main/src/page-components/ResourceCard/ResourceCard.test.tsx @@ -17,7 +17,7 @@ import { } from "../Dialogs/AddToListDialog" import type { ResourceCardProps } from "./ResourceCard" import { urls, factories, setMockResponse } from "api/test-utils" -import { RESOURCE_DRAWER_PARAMS, resourceDrawerSearch } from "@/common/urls" +import { RESOURCE_DRAWER_PARAMS } from "@/common/urls" import { slugify } from "@/common/slugs" import invariant from "tiny-invariant" import { LearningResourceCard } from "ol-components" @@ -197,7 +197,7 @@ describe.each([ expect(dialog).toHaveTextContent("Sign Up") }) - test("Card links to the canonical resource URL regardless of host page", () => { + test("Card links to the resource's learn_url regardless of host page", () => { const { resource } = setup({ user: { is_learning_path_editor: true }, url: HOST_URL, @@ -208,10 +208,9 @@ describe.each([ name: new RegExp(resource.title), }) - expect(link).toHaveAttribute( - "href", - resourceDrawerSearch(resource.id, resource.title), - ) + // What crawlers follow: the resource's own page where it has one, else + // its drawer. The backend decides, so the card does no URL building. + expect(link).toHaveAttribute("href", resource.learn_url) }) test("Clicking the title pushes the host page's URL with the drawer params", async () => { diff --git a/frontends/main/src/page-components/ResourceCard/ResourceCard.tsx b/frontends/main/src/page-components/ResourceCard/ResourceCard.tsx index 5439d12feb..a8b36cf270 100644 --- a/frontends/main/src/page-components/ResourceCard/ResourceCard.tsx +++ b/frontends/main/src/page-components/ResourceCard/ResourceCard.tsx @@ -7,7 +7,6 @@ import { AddToUserListDialog, } from "../Dialogs/AddToListDialog" import { resourceDrawerPushUrl } from "../LearningResourceDrawer/resourceDrawerPushUrl" -import { resourceDrawerSearch } from "@/common/urls" import { useUserMe } from "api/hooks/user" import { LearningResource } from "api" import { SignupPopover } from "../SignupPopover/SignupPopover" @@ -121,11 +120,13 @@ const ResourceCard: React.FC = ({ resourceDrawerPushUrl(resource) : undefined} onAddToLearningPathClick={handleAddToLearningPathClick} onAddToUserListClick={handleAddToUserListClick} diff --git a/frontends/main/src/proxy.test.ts b/frontends/main/src/proxy.test.ts index 4ff257e042..ae102acacd 100644 --- a/frontends/main/src/proxy.test.ts +++ b/frontends/main/src/proxy.test.ts @@ -11,7 +11,7 @@ describe("isPageRoute", () => { "/courses/course-v1:MITxT+5.601x", "/programs/program-v1:MITxT+18.01x", // Sitemaps are dynamically generated and tagged for purge-on-deploy. - "/sitemaps/products.xml", + "/sitemaps/resources/sitemap/0.xml", "/sitemaps/sitemap-index.xml", ])("treats %s as a page route", (pathname) => { expect(isPageRoute(pathname)).toBe(true) From c7c0f81c39eccdb7387b64b93ba5d7e1cab4c11a Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Fri, 28 Aug 2026 15:07:35 -0400 Subject: [PATCH 11/14] Update dependency ruff to v0.16.3 (#3859) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --- pyproject.toml | 2 +- uv.lock | 44 ++++++++++++++++++++++---------------------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 26bc0190a7..f92a5e237f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -149,7 +149,7 @@ dev = [ "pytest-mock>=3.10.0,<4", "pytest-repeat>=0.9.4", "responses>=0.25.0,<0.26", - "ruff==0.15.16", + "ruff==0.16.3", "safety>=3.0.0,<4", "semantic-version>=2.10.0,<3", "freezegun>=1.4.0,<2", diff --git a/uv.lock b/uv.lock index cd0ca2b8ea..d6c2fd1571 100644 --- a/uv.lock +++ b/uv.lock @@ -2772,7 +2772,7 @@ dev = [ { name = "pytest-repeat", specifier = ">=0.9.4" }, { name = "pytest-xdist", extras = ["psutil"], specifier = ">=3.6.1,<4" }, { name = "responses", specifier = ">=0.25.0,<0.26" }, - { name = "ruff", specifier = "==0.15.16" }, + { name = "ruff", specifier = "==0.16.3" }, { name = "safety", specifier = ">=3.0.0,<4" }, { name = "semantic-version", specifier = ">=2.10.0,<3" }, { name = "traceback-with-variables", specifier = ">=2.1.1,<3" }, @@ -4507,27 +4507,27 @@ wheels = [ [[package]] name = "ruff" -version = "0.15.16" -source = { registry = "https://pypi.org/simple" } -sdist = { url = "https://files.pythonhosted.org/packages/a6/bd/5f7ec371001337d8fa61701c186ff8b613ecac1651848c5950f4c4d5f2e9/ruff-0.15.16.tar.gz", hash = "sha256:d05e78d38c78caf020b03789e25106c93017db5a0cb6e2819885018c61343b78", size = 4714267, upload-time = "2026-06-04T16:33:09.974Z" } -wheels = [ - { url = "https://files.pythonhosted.org/packages/0c/42/53ef1c3953f157956db9bf7861e3bc50b9b887ce93300aa48cdba8336fe6/ruff-0.15.16-py3-none-linux_armv6l.whl", hash = "sha256:6ac3c0b3969cc6cf6b158c4e2f8f682acb58e7d700d8a44b65ecdc72d66ab0b2", size = 10709025, upload-time = "2026-06-04T16:32:51.935Z" }, - { url = "https://files.pythonhosted.org/packages/93/9a/a79159346f19134a956607754e57d8d128f7a4c00f4ad2f7514d224c172c/ruff-0.15.16-py3-none-macosx_10_12_x86_64.whl", hash = "sha256:197c207ed75ffba54a0dec23db4aa939a27a3053073e085e0042433cbdc58e4a", size = 11063550, upload-time = "2026-06-04T16:32:42.24Z" }, - { url = "https://files.pythonhosted.org/packages/bc/72/3ce2ac000a5299ec238e01f51397b3b653c93b077d9b1bfe8715bb895f20/ruff-0.15.16-py3-none-macosx_11_0_arm64.whl", hash = "sha256:3a39fec45ab316cc23e7558f23fea4a70403ddb5648ea9a4a3854a16973d0071", size = 10421345, upload-time = "2026-06-04T16:32:37.251Z" }, - { url = "https://files.pythonhosted.org/packages/b0/c2/cc7fad3ec9169373f5b6a18f1917b91080feec40c3f9658334a1d28e2f03/ruff-0.15.16-py3-none-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:ba93191d79003116b95128c9d306e045200fdbd0bccb782b110f3cd1d4abc5cf", size = 10757217, upload-time = "2026-06-04T16:32:54.722Z" }, - { url = "https://files.pythonhosted.org/packages/69/d2/3474009eaa0a65b31fa7152a2fad5e2f050c640ceb1e6b02ee6922e94c82/ruff-0.15.16-py3-none-manylinux_2_17_armv7l.manylinux2014_armv7l.whl", hash = "sha256:c6ee4b90520630120ef032aa5cc10db483852dff950e78b1d717e2993a61ac8d", size = 10507035, upload-time = "2026-06-04T16:33:05.343Z" }, - { url = "https://files.pythonhosted.org/packages/ca/81/b7ae6ccbd11f0c8dc3d5d67fc4be9b57ff57ca86ba56152021378e1277f2/ruff-0.15.16-py3-none-manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:4e4215bc938bc3c8215c1472c1aa437e310fee20cd427335fec9d7e609563628", size = 11255291, upload-time = "2026-06-04T16:32:49.49Z" }, - { url = "https://files.pythonhosted.org/packages/d9/e1/46e526f1a7cc90857ce6ddf25fbb77eb6568651ac38d71b033af07076dd5/ruff-0.15.16-py3-none-manylinux_2_17_ppc64le.manylinux2014_ppc64le.whl", hash = "sha256:7c8d26be963b090f10e29abc8b3e74a2a321f6fa34e02424e30b5af89350ecbb", size = 12124922, upload-time = "2026-06-04T16:33:07.821Z" }, - { url = "https://files.pythonhosted.org/packages/1a/da/5c791b088b596b24d0deb967fa28ae02ad751a140c0b9ea81c5ab915d6c0/ruff-0.15.16-py3-none-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:f198cf4123602a2280ed46c307bcbafe41758d6fee5b456b6b6058ca1514b3b4", size = 11332186, upload-time = "2026-06-04T16:33:02.971Z" }, - { url = "https://files.pythonhosted.org/packages/72/11/5da87abe20047c8962361473923ebb2f62b595250126aadfad8c20649c1e/ruff-0.15.16-py3-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:bb27515fa6240fb586ae82b901a59e67d24acff86f2190b433dc542fe0435aeb", size = 11373541, upload-time = "2026-06-04T16:32:47.007Z" }, - { url = "https://files.pythonhosted.org/packages/fe/2a/8554754c23a854ae3fd6b507e36ad61ddb121e298c6d5d617dec94ed0f14/ruff-0.15.16-py3-none-manylinux_2_31_riscv64.whl", hash = "sha256:a267c46ba1593fc26b8eecbea050b39d40c0b6bb7781ee11c90a02cd10032951", size = 11353014, upload-time = "2026-06-04T16:32:34.795Z" }, - { url = "https://files.pythonhosted.org/packages/62/25/62ea41529ec89f742ea3fed9cb1059c72877ec7cf9b9e99ac9cf3294d1d9/ruff-0.15.16-py3-none-musllinux_1_2_aarch64.whl", hash = "sha256:528c68f39a91498a8d50e91ff5985df3d105782bab49cc378e73ac26bff083e8", size = 10737467, upload-time = "2026-06-04T16:32:26.348Z" }, - { url = "https://files.pythonhosted.org/packages/90/17/334d3ad9de4d40f9dd58fdd09e35ce64553bb501e2f19a839e2fb6be14fc/ruff-0.15.16-py3-none-musllinux_1_2_armv7l.whl", hash = "sha256:7ed55c58950df60589a9a7a5d2f8fa5f54ebd287163be805adfe6ee95a9de123", size = 10521910, upload-time = "2026-06-04T16:32:32.54Z" }, - { url = "https://files.pythonhosted.org/packages/4d/bd/3ac7c6ae77a885c1004b3dda2446ea401768d24f851c14b4ad4b24f6639c/ruff-0.15.16-py3-none-musllinux_1_2_i686.whl", hash = "sha256:d482feaf51512b50f9790ceb417a56a61dd1e9d9bf967662b9ed27c01b34f53a", size = 10979190, upload-time = "2026-06-04T16:32:57.492Z" }, - { url = "https://files.pythonhosted.org/packages/33/d7/609546e6a413c3f216fbf2a50c928f97c80939154f6a0503114094a86191/ruff-0.15.16-py3-none-musllinux_1_2_x86_64.whl", hash = "sha256:1e15bc8c94513dae2a40cc9ef07c94fdd4ecc9e29dabebeebe170f952322c9e3", size = 11477014, upload-time = "2026-06-04T16:32:44.687Z" }, - { url = "https://files.pythonhosted.org/packages/74/0d/f2cd247ad32633a5c36e97141a2c21b11c6279f7957bc2ff360b1e08fddd/ruff-0.15.16-py3-none-win32.whl", hash = "sha256:580378f7bd4aa25f72e74aa54948a9622f142b1e509521dd10902e886681cc1e", size = 10735541, upload-time = "2026-06-04T16:32:30.145Z" }, - { url = "https://files.pythonhosted.org/packages/8b/9e/02e845ef151b1dee585e55c4739f8e1734ae1d9f1221dff65761c162208b/ruff-0.15.16-py3-none-win_amd64.whl", hash = "sha256:408256017284eddf98fff77b29aa4fb30f586042d535b2d9befc6512f400aaec", size = 11843403, upload-time = "2026-06-04T16:32:39.76Z" }, - { url = "https://files.pythonhosted.org/packages/15/19/016553f86f207450aebebc2b2b5088d086b901cc8186c02ac4284db3bd88/ruff-0.15.16-py3-none-win_arm64.whl", hash = "sha256:8cd61783afb39638a7133ef0d2dfb1e91277593962f81b5a8423eb0b888a6121", size = 11134555, upload-time = "2026-06-04T16:33:00.136Z" }, +version = "0.16.3" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/61/b3/3213589383f8f1b3938781bd1278713f6d18621a14992b3e81fefb8a5ef9/ruff-0.16.3.tar.gz", hash = "sha256:e76d33a347661a84b5be6d043d0347fdc745dfdcf825a8f4fed64b5e26eebdf2", size = 4891904, upload-time = "2026-08-13T15:17:13.381Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/bf/96/493770daebd68c0a67f1549fdf519f53be51fc435186c0585bcc272fd76c/ruff-0.16.3-py3-none-linux_armv6l.whl", hash = "sha256:0c5710e247a58a4521e66e124ba9a74655b414f61ba3a2e9e3811e11098f48f7", size = 10902799, upload-time = "2026-08-13T15:16:27.382Z" }, + { url = "https://files.pythonhosted.org/packages/5e/e6/2becf3942fddc29a29b8df47691d456fb1085391a694f74d84513251418c/ruff-0.16.3-py3-none-macosx_10_12_x86_64.whl", hash = "sha256:fe155130631a2471fd2e14a7a664a4dfbd7194b8229c3d7b2a40b21178639081", size = 11135539, upload-time = "2026-08-13T15:16:30.87Z" }, + { url = "https://files.pythonhosted.org/packages/3e/1e/4b8b72f0d006dbf19326aa99f9ca0ee2ff374187c4d301cf529a51aa06fe/ruff-0.16.3-py3-none-macosx_11_0_arm64.whl", hash = "sha256:e2ed719e14aa64d895c2ee922594a90a43c861a93f0575a95ff8c47cdbd13eb9", size = 10475095, upload-time = "2026-08-13T15:16:33.259Z" }, + { url = "https://files.pythonhosted.org/packages/92/32/2201fa49ba1f6c101ee321e83f051ac7a4b8d07b0ef6b4d3f2772b302275/ruff-0.16.3-py3-none-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:9e0b1da805eb043654645d74d5de1e5ce2edc686e40790d2b86f56d71cc06a84", size = 10668771, upload-time = "2026-08-13T15:16:35.65Z" }, + { url = "https://files.pythonhosted.org/packages/c3/66/4afc5c8363bd04d45effce1b7c8713ca037d7a6740b7451a2403a6e3a972/ruff-0.16.3-py3-none-manylinux_2_17_armv7l.manylinux2014_armv7l.whl", hash = "sha256:a37bdea0bbe21780f590bf437d6412c8c4e1b6cd010f91a65c2c40c5e5f5f870", size = 10699568, upload-time = "2026-08-13T15:16:38.195Z" }, + { url = "https://files.pythonhosted.org/packages/53/fd/c67d246bf36bf1698551c56de39e95cd07f70e64433e0098e6267d77061b/ruff-0.16.3-py3-none-manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:09571e6d1288ed9be475207a3ac04ada404f1cd898104be0f6ab8d7df438575b", size = 11499365, upload-time = "2026-08-13T15:16:40.623Z" }, + { url = "https://files.pythonhosted.org/packages/67/0b/00ecbceb99a263af7b12f6f05ac3c92bc47b905e91adc3f207a836e3bc01/ruff-0.16.3-py3-none-manylinux_2_17_ppc64le.manylinux2014_ppc64le.whl", hash = "sha256:2c18c5a101eb540010638cc1ff3c84944d3adb3df62b8d98ca8f22ba484d3413", size = 12311728, upload-time = "2026-08-13T15:16:43.564Z" }, + { url = "https://files.pythonhosted.org/packages/54/b2/b7b3bb54f4d3f7db504e476ad4ab8de530dceebe2c061384b2757ee419e8/ruff-0.16.3-py3-none-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:8457c44f15033c85ddbb77b15d451df9e24e4bd03b628396dd3610cedc3b8f82", size = 11699896, upload-time = "2026-08-13T15:16:46.209Z" }, + { url = "https://files.pythonhosted.org/packages/c7/30/4c468429ac195addc5ee1b717b6ab1b66632786737ca3b2ed3443fb0c26a/ruff-0.16.3-py3-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:294b95c4ae0cda9388525c2047778aa758d6b8d4bb876fd4e9eaa3ebc92343eb", size = 11058736, upload-time = "2026-08-13T15:16:48.823Z" }, + { url = "https://files.pythonhosted.org/packages/43/67/7a113cdaddf24b64d7f75b1242a99d04c82fcef4f6921fdbb832beaffb5f/ruff-0.16.3-py3-none-manylinux_2_31_riscv64.whl", hash = "sha256:3d0c7c40c87c2a820509c31ba007968da6e1306468c067b2d82fbfdbcd0e8474", size = 11586911, upload-time = "2026-08-13T15:16:51.913Z" }, + { url = "https://files.pythonhosted.org/packages/f1/c1/2e66f24c0f3ead25a5e660111778685e505e5da353c82802bf49f0cbe7b9/ruff-0.16.3-py3-none-musllinux_1_2_aarch64.whl", hash = "sha256:9f738c0fdfa8eed0b2ce7fb27ee7258208a92a68d7949e62aa15164bc7b389da", size = 10954265, upload-time = "2026-08-13T15:16:54.763Z" }, + { url = "https://files.pythonhosted.org/packages/c2/ba/4cee23bf52cba9a058d3726de623624daf50ef9638868edd86f4126157f6/ruff-0.16.3-py3-none-musllinux_1_2_armv7l.whl", hash = "sha256:fb785f0be25abe69d320415cd4f833b59e17ba7613d9ba6a958023b6bceb0a50", size = 10709886, upload-time = "2026-08-13T15:16:57.339Z" }, + { url = "https://files.pythonhosted.org/packages/82/df/7da7194fa5d9dc0a285f7e6fa5a4722e7c63faac0b45b614ded9314363a1/ruff-0.16.3-py3-none-musllinux_1_2_i686.whl", hash = "sha256:c5536e3acfbf9563085aa2be7b13c629c3077e902afc5b941ac44024dbb9f506", size = 11210392, upload-time = "2026-08-13T15:17:00.171Z" }, + { url = "https://files.pythonhosted.org/packages/35/85/7795f6e817af050e7517bf3e7aa9b061cce70ef33d280aad902c956c1ecf/ruff-0.16.3-py3-none-musllinux_1_2_x86_64.whl", hash = "sha256:a2d85c02f9b8e165d85e6779184d38c4132de12603dab59c51c28e22584f9e4d", size = 11626910, upload-time = "2026-08-13T15:17:03.299Z" }, + { url = "https://files.pythonhosted.org/packages/78/9b/475b927cf27a5cbbda3c7bafb69ed6ff77e1d7923d5d85f17c2749d7ae32/ruff-0.16.3-py3-none-win32.whl", hash = "sha256:388cdf2166642bd9b13d52b5932d3170f34f8abed7e8d9a855f1d84b83645a0a", size = 10931415, upload-time = "2026-08-13T15:17:05.726Z" }, + { url = "https://files.pythonhosted.org/packages/b2/99/e2a2bfc4fbf0a1e8a916bc9ebe6fe6c58cc34c28e0ffc6ce281d572d1c2e/ruff-0.16.3-py3-none-win_amd64.whl", hash = "sha256:e80a7d69ca2a6d1c4d352ec91458cdca6e56c83cdbcabd93e4abe1e53591d948", size = 11445993, upload-time = "2026-08-13T15:17:08.353Z" }, + { url = "https://files.pythonhosted.org/packages/69/3e/4132e539aed78c148854d4997a2685b0ed4dc4e87110b59ce528564e184e/ruff-0.16.3-py3-none-win_arm64.whl", hash = "sha256:b8ca152da82c1acc1fa8d5874b15951935f0eef46f10e6954c83859011b6178a", size = 11399302, upload-time = "2026-08-13T15:17:10.908Z" }, ] [[package]] From 65ba190c3f93a0aea14283dbe62555ec42a2be02 Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Fri, 28 Aug 2026 15:28:30 -0400 Subject: [PATCH 12/14] Update dependency drf-spectacular to >=0.30,<0.31 (#3858) * Update dependency drf-spectacular to >=0.30,<0.31 * update spec --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Anastasia Beglova --- frontends/api/src/generated/v0/api.ts | 52 +++++++++++++----- frontends/api/src/generated/v1/api.ts | 66 ++++++++++++++++------ openapi/specs/v0.yaml | 65 ++++++++++++++-------- openapi/specs/v1.yaml | 79 ++++++++++++++++++--------- pyproject.toml | 2 +- uv.lock | 8 +-- 6 files changed, 187 insertions(+), 85 deletions(-) diff --git a/frontends/api/src/generated/v0/api.ts b/frontends/api/src/generated/v0/api.ts index 30e6ddce9a..5d4cbc5153 100644 --- a/frontends/api/src/generated/v0/api.ts +++ b/frontends/api/src/generated/v0/api.ts @@ -429,10 +429,10 @@ export interface ContentFeedback { unit_title?: string /** * - * @type {string} + * @type {ContentFeedbackUrl} * @memberof ContentFeedback */ - url?: string + url?: ContentFeedbackUrl /** * * @type {ContentFeedbackSentimentEnum} @@ -491,10 +491,10 @@ export interface ContentFeedbackRequest { unit_title?: string /** * - * @type {string} + * @type {ContentFeedbackUrl} * @memberof ContentFeedbackRequest */ - url?: string + url?: ContentFeedbackUrl /** * * @type {ContentFeedbackSentimentEnum} @@ -539,6 +539,12 @@ export const ContentFeedbackSentimentEnum = { export type ContentFeedbackSentimentEnum = (typeof ContentFeedbackSentimentEnum)[keyof typeof ContentFeedbackSentimentEnum] +/** + * @type ContentFeedbackUrl + * @export + */ +export type ContentFeedbackUrl = string + /** * Serializer class for course run ContentFiles * @export @@ -679,10 +685,10 @@ export interface ContentFile { checksum?: string /** * - * @type {string} + * @type {ContentFileImageSrc} * @memberof ContentFile */ - image_src?: string | null + image_src?: ContentFileImageSrc | null /** * * @type {string} @@ -798,6 +804,12 @@ export const ContentFileContentTypeEnum = { export type ContentFileContentTypeEnum = (typeof ContentFileContentTypeEnum)[keyof typeof ContentFileContentTypeEnum] +/** + * @type ContentFileImageSrc + * @export + */ +export type ContentFileImageSrc = string + /** * SearchResponseSerializer with OpenAPI annotations for Content Files search * @export @@ -2921,10 +2933,10 @@ export interface LearningResourceOfferorDetail { content_types?: Array /** * - * @type {string} + * @type {LearningResourceOfferorDetailMoreInformation} * @memberof LearningResourceOfferorDetail */ - more_information?: string + more_information?: LearningResourceOfferorDetailMoreInformation /** * * @type {string} @@ -2938,6 +2950,12 @@ export interface LearningResourceOfferorDetail { */ display_facet?: boolean } +/** + * @type LearningResourceOfferorDetailMoreInformation + * @export + */ +export type LearningResourceOfferorDetailMoreInformation = string + /** * Serializer for LearningResourcePlatform * @export @@ -3461,10 +3479,10 @@ export interface NestedContentFile { checksum?: string /** * - * @type {string} + * @type {ContentFileImageSrc} * @memberof NestedContentFile */ - image_src?: string | null + image_src?: ContentFileImageSrc | null /** * * @type {string} @@ -6216,16 +6234,16 @@ export interface Video { caption_urls: Array /** * - * @type {string} + * @type {VideoStreamingUrl} * @memberof Video */ - streaming_url: string | null + streaming_url: VideoStreamingUrl | null /** * - * @type {string} + * @type {VideoStreamingUrl} * @memberof Video */ - cover_image_url: string | null + cover_image_url: VideoStreamingUrl | null /** * * @type {string} @@ -6951,6 +6969,12 @@ export const VideoResourceResourceTypeEnum = { export type VideoResourceResourceTypeEnum = (typeof VideoResourceResourceTypeEnum)[keyof typeof VideoResourceResourceTypeEnum] +/** + * @type VideoStreamingUrl + * @export + */ +export type VideoStreamingUrl = string + /** * WidgetInstance serializer * @export diff --git a/frontends/api/src/generated/v1/api.ts b/frontends/api/src/generated/v1/api.ts index deffdac0c9..27eaa98359 100644 --- a/frontends/api/src/generated/v1/api.ts +++ b/frontends/api/src/generated/v1/api.ts @@ -374,10 +374,10 @@ export interface ContentFile { checksum?: string /** * - * @type {string} + * @type {ContentFileImageSrc} * @memberof ContentFile */ - image_src?: string | null + image_src?: ContentFileImageSrc | null /** * * @type {string} @@ -493,6 +493,12 @@ export const ContentFileContentTypeEnum = { export type ContentFileContentTypeEnum = (typeof ContentFileContentTypeEnum)[keyof typeof ContentFileContentTypeEnum] +/** + * @type ContentFileImageSrc + * @export + */ +export type ContentFileImageSrc = string + /** * SearchResponseSerializer with OpenAPI annotations for Content Files search * @export @@ -4311,10 +4317,10 @@ export interface LearningResourceOfferorDetail { content_types?: Array /** * - * @type {string} + * @type {LearningResourceOfferorDetailMoreInformation} * @memberof LearningResourceOfferorDetail */ - more_information?: string + more_information?: LearningResourceOfferorDetailMoreInformation /** * * @type {string} @@ -4328,6 +4334,12 @@ export interface LearningResourceOfferorDetail { */ display_facet?: boolean } +/** + * @type LearningResourceOfferorDetailMoreInformation + * @export + */ +export type LearningResourceOfferorDetailMoreInformation = string + /** * Serializer for LearningResourceOfferor with basic details * @export @@ -5384,10 +5396,10 @@ export interface NestedContentFile { checksum?: string /** * - * @type {string} + * @type {ContentFileImageSrc} * @memberof NestedContentFile */ - image_src?: string | null + image_src?: ContentFileImageSrc | null /** * * @type {string} @@ -6514,12 +6526,18 @@ export interface PatchedWebsiteContentRequest { is_published?: boolean /** * - * @type {string} + * @type {PatchedWebsiteContentRequestSlug} * @memberof PatchedWebsiteContentRequest */ - slug?: string + slug?: PatchedWebsiteContentRequestSlug } +/** + * @type PatchedWebsiteContentRequestSlug + * @export + */ +export type PatchedWebsiteContentRequestSlug = string + /** * Serializer for PercolateQuery objects * @export @@ -9380,16 +9398,16 @@ export interface Video { caption_urls: Array /** * - * @type {string} + * @type {VideoStreamingUrl} * @memberof Video */ - streaming_url: string | null + streaming_url: VideoStreamingUrl | null /** * - * @type {string} + * @type {VideoStreamingUrl} * @memberof Video */ - cover_image_url: string | null + cover_image_url: VideoStreamingUrl | null /** * * @type {string} @@ -10434,6 +10452,12 @@ export const VideoResourceResourceTypeEnum = { export type VideoResourceResourceTypeEnum = (typeof VideoResourceResourceTypeEnum)[keyof typeof VideoResourceResourceTypeEnum] +/** + * @type VideoStreamingUrl + * @export + */ +export type VideoStreamingUrl = string + /** * Serializer for webhook responses. * @export @@ -10527,16 +10551,16 @@ export interface WebsiteContent { is_published?: boolean /** * - * @type {string} + * @type {PatchedWebsiteContentRequestSlug} * @memberof WebsiteContent */ - slug?: string + slug?: PatchedWebsiteContentRequestSlug /** * - * @type {string} + * @type {WebsiteContentCoverImage} * @memberof WebsiteContent */ - cover_image: string + cover_image: WebsiteContentCoverImage } /** @@ -10564,6 +10588,12 @@ export const WebsiteContentContentTypeEnum = { export type WebsiteContentContentTypeEnum = (typeof WebsiteContentContentTypeEnum)[keyof typeof WebsiteContentContentTypeEnum] +/** + * @type WebsiteContentCoverImage + * @export + */ +export type WebsiteContentCoverImage = string + /** * Serializer for WebsiteContent model. * @export @@ -10602,10 +10632,10 @@ export interface WebsiteContentRequest { is_published?: boolean /** * - * @type {string} + * @type {PatchedWebsiteContentRequestSlug} * @memberof WebsiteContentRequest */ - slug?: string + slug?: PatchedWebsiteContentRequestSlug } /** diff --git a/openapi/specs/v0.yaml b/openapi/specs/v0.yaml index 65ac8be804..55abdfd4f0 100644 --- a/openapi/specs/v0.yaml +++ b/openapi/specs/v0.yaml @@ -1750,9 +1750,12 @@ components: type: string maxLength: 255 url: - type: string - format: uri - maxLength: 2083 + oneOf: + - type: string + format: uri + maxLength: 2083 + - type: string + maxLength: 0 sentiment: $ref: '#/components/schemas/ContentFeedbackSentimentEnum' comment: @@ -1790,9 +1793,12 @@ components: type: string maxLength: 255 url: - type: string - format: uri - maxLength: 2083 + oneOf: + - type: string + format: uri + maxLength: 2083 + - type: string + maxLength: 0 sentiment: $ref: '#/components/schemas/ContentFeedbackSentimentEnum' comment: @@ -1890,10 +1896,13 @@ components: checksum: type: string image_src: - type: string - format: uri nullable: true - maxLength: 200 + oneOf: + - type: string + format: uri + maxLength: 200 + - type: string + maxLength: 0 resource_id: type: string readOnly: true @@ -3557,9 +3566,12 @@ components: type: string maxLength: 128 more_information: - type: string - format: uri - maxLength: 200 + oneOf: + - type: string + format: uri + maxLength: 200 + - type: string + maxLength: 0 value_prop: type: string display_facet: @@ -3975,10 +3987,13 @@ components: checksum: type: string image_src: - type: string - format: uri nullable: true - maxLength: 200 + oneOf: + - type: string + format: uri + maxLength: 200 + - type: string + maxLength: 0 resource_id: type: string readOnly: true @@ -5080,18 +5095,18 @@ components: image_file: type: string format: uri - readOnly: true nullable: true + readOnly: true image_small_file: type: string format: uri - readOnly: true nullable: true + readOnly: true image_medium_file: type: string format: uri - readOnly: true nullable: true + readOnly: true profile_image_small: type: string description: Custom getter for small profile image @@ -6045,15 +6060,21 @@ components: $ref: '#/components/schemas/CaptionUrl' readOnly: true streaming_url: - type: string - format: uri readOnly: true nullable: true + oneOf: + - type: string + format: uri + - type: string + maxLength: 0 cover_image_url: - type: string - format: uri readOnly: true nullable: true + oneOf: + - type: string + format: uri + - type: string + maxLength: 0 duration: type: string maxLength: 11 diff --git a/openapi/specs/v1.yaml b/openapi/specs/v1.yaml index 64c75d7d07..d1d96c12ad 100644 --- a/openapi/specs/v1.yaml +++ b/openapi/specs/v1.yaml @@ -10350,10 +10350,13 @@ components: checksum: type: string image_src: - type: string - format: uri nullable: true - maxLength: 200 + oneOf: + - type: string + format: uri + maxLength: 200 + - type: string + maxLength: 0 resource_id: type: string readOnly: true @@ -13033,9 +13036,12 @@ components: type: string maxLength: 128 more_information: - type: string - format: uri - maxLength: 200 + oneOf: + - type: string + format: uri + maxLength: 200 + - type: string + maxLength: 0 value_prop: type: string display_facet: @@ -13839,10 +13845,13 @@ components: checksum: type: string image_src: - type: string - format: uri nullable: true - maxLength: 200 + oneOf: + - type: string + format: uri + maxLength: 200 + - type: string + maxLength: 0 resource_id: type: string readOnly: true @@ -14616,9 +14625,12 @@ components: is_published: type: boolean slug: - type: string - maxLength: 60 - pattern: ^[-a-zA-Z0-9_]+$ + oneOf: + - type: string + maxLength: 60 + pattern: ^[-a-zA-Z0-9_]+$ + - type: string + maxLength: 0 PercolateQuery: type: object description: Serializer for PercolateQuery objects @@ -16799,15 +16811,21 @@ components: $ref: '#/components/schemas/CaptionUrl' readOnly: true streaming_url: - type: string - format: uri readOnly: true nullable: true + oneOf: + - type: string + format: uri + - type: string + maxLength: 0 cover_image_url: - type: string - format: uri readOnly: true nullable: true + oneOf: + - type: string + format: uri + - type: string + maxLength: 0 duration: type: string maxLength: 11 @@ -17756,15 +17774,21 @@ components: is_published: type: boolean slug: - type: string - maxLength: 60 - pattern: ^[-a-zA-Z0-9_]+$ + oneOf: + - type: string + maxLength: 60 + pattern: ^[-a-zA-Z0-9_]+$ + - type: string + maxLength: 0 cover_image: - type: string - format: uri readOnly: true - default: '' - maxLength: 2083 + oneOf: + - type: string + format: uri + default: '' + maxLength: 2083 + - type: string + maxLength: 0 required: - cover_image - created_on @@ -17811,8 +17835,11 @@ components: is_published: type: boolean slug: - type: string - maxLength: 60 - pattern: ^[-a-zA-Z0-9_]+$ + oneOf: + - type: string + maxLength: 60 + pattern: ^[-a-zA-Z0-9_]+$ + - type: string + maxLength: 0 required: - title diff --git a/pyproject.toml b/pyproject.toml index f92a5e237f..bddbc9be2f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -44,7 +44,7 @@ dependencies = [ "djangorestframework>=3.14.0,<4", "drf-jwt>=1.19.2,<2", "drf-nested-routers>=0.95.0,<0.96", - "drf-spectacular>=0.29,<0.30", + "drf-spectacular>=0.30,<0.31", "feedparser>=6.0.10,<7", "google-api-python-client>=2.89.0,<3", "hubspot-api-client>=12.0.0,<13", diff --git a/uv.lock b/uv.lock index d6c2fd1571..2dbb0f6304 100644 --- a/uv.lock +++ b/uv.lock @@ -1092,7 +1092,7 @@ wheels = [ [[package]] name = "drf-spectacular" -version = "0.29.0" +version = "0.30.0" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "django" }, @@ -1102,9 +1102,9 @@ dependencies = [ { name = "pyyaml" }, { name = "uritemplate" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/5e/0e/a4f50d83e76cbe797eda88fc0083c8ca970cfa362b5586359ef06ec6f70a/drf_spectacular-0.29.0.tar.gz", hash = "sha256:0a069339ea390ce7f14a75e8b5af4a0860a46e833fd4af027411a3e94fc1a0cc", size = 241722, upload-time = "2025-11-02T03:40:26.348Z" } +sdist = { url = "https://files.pythonhosted.org/packages/50/43/41d25039a6a53545420ebc98eb9f877ec9fe30c7bd03fefabcaf9b953af7/drf_spectacular-0.30.0.tar.gz", hash = "sha256:53e79e7ba00e240441b63c32273754a5368e4c2ab44a19f2595277cc1cd559c9", size = 252311, upload-time = "2026-07-06T11:29:46.264Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/32/d9/502c56fc3ca960075d00956283f1c44e8cafe433dada03f9ed2821f3073b/drf_spectacular-0.29.0-py3-none-any.whl", hash = "sha256:d1ee7c9535d89848affb4427347f7c4a22c5d22530b8842ef133d7b72e19b41a", size = 105433, upload-time = "2025-11-02T03:40:24.823Z" }, + { url = "https://files.pythonhosted.org/packages/c3/56/74dd7b45bbde6d24494220b98d6961cb1200b63a1800332b430daa2c4551/drf_spectacular-0.30.0-py3-none-any.whl", hash = "sha256:006cf5921ebe20a9bd24f7c846261ebbf78780be5961b0d6e87afaa82afd62ff", size = 111150, upload-time = "2026-07-06T11:29:45.12Z" }, ] [[package]] @@ -2673,7 +2673,7 @@ requires-dist = [ { name = "djangorestframework", specifier = ">=3.14.0,<4" }, { name = "drf-jwt", specifier = ">=1.19.2,<2" }, { name = "drf-nested-routers", specifier = ">=0.95.0,<0.96" }, - { name = "drf-spectacular", specifier = ">=0.29,<0.30" }, + { name = "drf-spectacular", specifier = ">=0.30,<0.31" }, { name = "feedparser", specifier = ">=6.0.10,<7" }, { name = "gensim", specifier = ">=4.4.0" }, { name = "google-api-python-client", specifier = ">=2.89.0,<3" }, From 4a68caa214f2dce96394bf5f1d8c91ca67442425 Mon Sep 17 00:00:00 2001 From: Matt Bertrand Date: Fri, 28 Aug 2026 13:44:17 -0700 Subject: [PATCH 13/14] Remove remaining API endpoint N+1 queries (#3845) --- channels/models.py | 11 +++ channels/views_test.py | 50 ++++++++++-- drf_lint_baseline.json | 5 +- learning_resources/filters_test.py | 6 -- learning_resources/views_learningpath_test.py | 1 - news_events/filters_test.py | 1 - news_events/serializers.py | 5 +- news_events/views.py | 2 +- news_events/views_test.py | 2 - profiles/models.py | 11 +++ profiles/serializers.py | 34 ++++++--- profiles/views.py | 76 +++++++++++++++++-- profiles/views_test.py | 44 ++++++++++- pyproject.toml | 2 +- uv.lock | 10 +-- website_content/serializers.py | 5 +- website_content/views.py | 2 +- website_content/views_test.py | 22 ++++++ 18 files changed, 241 insertions(+), 48 deletions(-) diff --git a/channels/models.py b/channels/models.py index d44c159f9b..07078f851b 100644 --- a/channels/models.py +++ b/channels/models.py @@ -69,10 +69,21 @@ def with_detail_relations(self) -> "ChannelQuerySet": "sub_channels__channel", queryset=Channel.objects.annotate_channel_url(), ), + # LearningResourceOfferor.channel_url reads + # channel_unit_details.first(); ordering the prefetch by pk + # lets that resolve from the cache and pick the same row it + # would have queried for. + Prefetch( + "unit_detail__unit__channel_unit_details", + queryset=ChannelUnitDetail.objects.select_related( + "channel" + ).order_by("pk"), + ), ) .annotate_channel_url() .select_related( "featured_list", + "unit_detail__unit", "topic_detail", "department_detail", "unit_detail", diff --git a/channels/views_test.py b/channels/views_test.py index fd8ce1c8bb..4dd671c8d6 100644 --- a/channels/views_test.py +++ b/channels/views_test.py @@ -7,7 +7,12 @@ from django.urls import reverse from channels.constants import ChannelType -from channels.factories import ChannelFactory, ChannelListFactory, SubChannelFactory +from channels.factories import ( + ChannelFactory, + ChannelListFactory, + ChannelUnitDetailFactory, + SubChannelFactory, +) from channels.models import Channel from channels.serializers import ChannelSerializer from learning_resources.factories import LearningResourceFactory @@ -16,7 +21,6 @@ pytestmark = pytest.mark.django_db -@pytest.mark.skip_nplusone_check def test_list_channels(user_client): """Test that all published channels are returned.""" ChannelFactory.create_batch(2, published=False) @@ -33,6 +37,44 @@ def test_list_channels(user_client): assert response_channels[idx] == ChannelSerializer(instance=channel).data +@pytest.mark.parametrize("channel_count", [2, 6]) +def test_list_unit_channels_query_count( + client, django_assert_num_queries, channel_count +): + """Unit channels cost the same number of queries however many are listed. + + unit_detail.unit is nested by the serializer and its channel_url is a + cached_property, so without the prefetch each row costs two extra queries. + """ + channels = ChannelFactory.create_batch(channel_count, is_unit=True) + # An offeror shared with later, unpublished channels: channel_url has to + # resolve to the same row the serializer would pick on its own, so the + # prefetch behind it can't be an unordered subquery. + for _ in range(3): + extra = ChannelFactory.create( + is_unit=True, published=False, create_unit_detail=False + ) + ChannelUnitDetailFactory.create( + channel=extra, unit=channels[0].unit_detail.unit + ) + + url = reverse("channels:v0:channels_api-list") + with django_assert_num_queries(5): + results = client.get(url).json()["results"] + + assert len(results) == channel_count + assert all(item["unit_detail"]["unit"]["channel_url"] for item in results) + # The listing must agree with serializing each instance directly. + for channel in channels: + listed = next(item for item in results if item["id"] == channel.id) + assert ( + listed["unit_detail"]["unit"]["channel_url"] + == ChannelSerializer(instance=channel).data["unit_detail"]["unit"][ + "channel_url" + ] + ) + + def test_channel_detail_has_no_is_moderator(client): """Channel detail no longer exposes moderator-specific fields.""" channel = ChannelFactory.create() @@ -153,7 +195,6 @@ def test_no_excess_list_queries(client, user, django_assert_num_queries, channel assert channel["channel_url"] is not None -@pytest.mark.skip_nplusone_check def test_channel_counts_view(client): """Channel counts should return per-channel resource counts.""" url = reverse( @@ -194,7 +235,6 @@ def enabled_view_cache(settings, request): } -@pytest.mark.skip_nplusone_check @pytest.mark.usefixtures("enabled_view_cache") def test_channel_detail_cache_is_global(client): """Cached detail responses are shared globally across users.""" @@ -210,7 +250,6 @@ def test_channel_detail_cache_is_global(client): assert client.get(url).json()["title"] == "Original title" -@pytest.mark.skip_nplusone_check @pytest.mark.usefixtures("enabled_view_cache") def test_channel_by_type_name_cache_is_global(client): """Cached by-type-name responses are shared globally across users.""" @@ -229,7 +268,6 @@ def test_channel_by_type_name_cache_is_global(client): assert client.get(url).json()["title"] == "Original title" -@pytest.mark.skip_nplusone_check @pytest.mark.usefixtures("enabled_view_cache") @pytest.mark.parametrize("is_authenticated", [False, True]) def test_channel_counts_view_is_cached(client, is_authenticated): diff --git a/drf_lint_baseline.json b/drf_lint_baseline.json index 2657099b78..2222d8bddd 100644 --- a/drf_lint_baseline.json +++ b/drf_lint_baseline.json @@ -2,8 +2,5 @@ "channels/serializers.py:132:24:ORM002", "channels/serializers.py:134:24:ORM002", "channels/serializers.py:136:24:ORM002", - "profiles/serializers.py:136:31:ORM002", - "profiles/serializers.py:196:16:ORM002", - "profiles/serializers.py:446:15:ORM001", - "profiles/serializers.py:447:27:ORM001" + "profiles/serializers.py:205:16:ORM002" ] diff --git a/learning_resources/filters_test.py b/learning_resources/filters_test.py index b2c6982900..8e2af7788b 100644 --- a/learning_resources/filters_test.py +++ b/learning_resources/filters_test.py @@ -558,7 +558,6 @@ def test_learning_resource_filter_delivery(mock_courses, client): ) -@pytest.mark.skip_nplusone_check def test_content_file_filter_run_id(mock_content_files, client): """Test that the run_id filter works for contentfiles""" @@ -578,7 +577,6 @@ def test_content_file_filter_run_id(mock_content_files, client): ) -@pytest.mark.skip_nplusone_check def test_content_file_filter_resource_id(mock_content_files, client): """Test that the resource_id filter works for contentfiles""" @@ -601,7 +599,6 @@ def test_content_file_filter_resource_id(mock_content_files, client): ) -@pytest.mark.skip_nplusone_check def test_content_file_filter_edx_module_id(mock_content_files, client): """Test that the resource_id filter works for contentfiles""" assert mock_content_files[0].edx_module_id is None @@ -649,7 +646,6 @@ def test_content_file_filter_present_edx_module_id_not_logged( mock_log.assert_not_called() -@pytest.mark.skip_nplusone_check def test_content_file_filter_platform(mock_content_files, client): """Test that the platform filter works""" @@ -667,7 +663,6 @@ def test_content_file_filter_platform(mock_content_files, client): ) -@pytest.mark.skip_nplusone_check def test_content_file_filter_offered_by(mock_content_files, client): """Test that the offered_by filter works for contentfiles""" @@ -685,7 +680,6 @@ def test_content_file_filter_offered_by(mock_content_files, client): ) -@pytest.mark.skip_nplusone_check def test_learning_resource_filter_content_feature_type(client): """Test that the resource_content_tag filter works""" diff --git a/learning_resources/views_learningpath_test.py b/learning_resources/views_learningpath_test.py index 9835bb7d50..b9205cc5cb 100644 --- a/learning_resources/views_learningpath_test.py +++ b/learning_resources/views_learningpath_test.py @@ -313,7 +313,6 @@ def test_learning_path_items_endpoint_update_items_wrong_list(client, user): assert resp.status_code == 404 -@pytest.mark.skip_nplusone_check @pytest.mark.parametrize("num_items", [2, 3]) @pytest.mark.parametrize("is_editor", [True, False]) def test_learning_path_items_endpoint_delete_items(client, user, is_editor, num_items): diff --git a/news_events/filters_test.py b/news_events/filters_test.py index ec0d36dd26..041a4b1fb8 100644 --- a/news_events/filters_test.py +++ b/news_events/filters_test.py @@ -9,7 +9,6 @@ ITEM_API_URL = "/api/v0/news_events/" -@pytest.mark.skip_nplusone_check @pytest.mark.parametrize( "multifilter", ["feed_type={}&feed_type={}", "feed_type={},{}"] ) diff --git a/news_events/serializers.py b/news_events/serializers.py index bac6736545..572e3a68a3 100644 --- a/news_events/serializers.py +++ b/news_events/serializers.py @@ -1,5 +1,6 @@ """Serializers for news_events""" +from mitol.common.serializers import BaseSerializer from rest_framework import serializers from news_events import models @@ -16,9 +17,11 @@ class Meta: exclude = COMMON_IGNORED_FIELDS -class FeedSourceSerializer(serializers.ModelSerializer): +class FeedSourceSerializer(BaseSerializer): """FeedSource serializer""" + required_prefetches: list[str] = ["image"] + image = FeedImageSerializer() class Meta: diff --git a/news_events/views.py b/news_events/views.py index 18864709c0..b6645c3b19 100644 --- a/news_events/views.py +++ b/news_events/views.py @@ -72,7 +72,7 @@ class FeedSourceViewSet(viewsets.ReadOnlyModelViewSet): serializer_class = FeedSourceSerializer filter_backends = [MultipleOptionsFilterBackend] filterset_class = FeedSourceFilter - queryset = FeedSource.objects.all().order_by("id") + queryset = FeedSource.objects.select_related("image").order_by("id") @method_decorator( cache_page_for_all_users( diff --git a/news_events/views_test.py b/news_events/views_test.py index de3f39d6ec..7e7eeb5371 100644 --- a/news_events/views_test.py +++ b/news_events/views_test.py @@ -11,7 +11,6 @@ from news_events.factories import FeedEventDetailFactory -@pytest.mark.skip_nplusone_check def test_feed_source_viewset_list(client): """Est that the feed sources list viewset returns data in expected format""" sources = sorted(factories.FeedSourceFactory.create_batch(5), key=lambda x: x.id) @@ -26,7 +25,6 @@ def test_feed_source_viewset_list(client): ) -@pytest.mark.skip_nplusone_check @pytest.mark.parametrize("feed_type", FeedType.names()) def test_feed_source_viewset_list_filtered(client, feed_type): """Test that the sources list viewset returns data filtered by feed type""" diff --git a/profiles/models.py b/profiles/models.py index fc3a6d33e0..e679ed7d47 100644 --- a/profiles/models.py +++ b/profiles/models.py @@ -1,5 +1,6 @@ """Profile models""" +from functools import cached_property from uuid import uuid4 from django.conf import settings @@ -201,6 +202,16 @@ def save(self, *args, update_image=False, **kwargs): # pylint: disable=argument self.image_medium_file = None super().save(*args, **kwargs) # pylint:disable=super-with-arguments + @cached_property + def annotated_topic_interests(self) -> list: + """ + Topic interests with channel_url annotated for serialization. + + Fallback only: API views fill this via + Prefetch(..., to_attr="annotated_topic_interests"). + """ + return list(self.topic_interests.for_serialization()) + class UserWebsite(models.Model): """A model for storing information for websites that should appear in a user's profile""" # noqa: E501 diff --git a/profiles/serializers.py b/profiles/serializers.py index 5ef0186be2..e3614d438e 100644 --- a/profiles/serializers.py +++ b/profiles/serializers.py @@ -11,6 +11,7 @@ from django.urls import reverse from drf_spectacular.utils import extend_schema_field from keycloak.exceptions import KeycloakError +from mitol.common.serializers import BaseSerializer from rest_framework import serializers from rest_framework.exceptions import ValidationError @@ -46,6 +47,10 @@ class TopicInterestsField(serializers.Field): Serializer field for topic interests """ + def get_attribute(self, instance): + """Read the dual-path list instead of the raw related manager""" + return instance.annotated_topic_interests + def to_representation(self, value): """Serialize the topic_interests""" return LearningResourceTopicSerializer(value, many=True).data @@ -132,8 +137,9 @@ def get_preference_search_filters(self, obj) -> dict: filters["certification"] = ( obj.certificate_desired == Profile.CertificateDesired.YES.value ) - if obj.topic_interests and obj.topic_interests.count() > 0: - filters["topic"] = obj.topic_interests.values_list("name", flat=True) + topic_names = [topic.name for topic in obj.annotated_topic_interests] + if topic_names: + filters["topic"] = topic_names if obj.delivery: filters["delivery"] = obj.delivery return PreferencesSearchSerializer(instance=filters).data @@ -154,6 +160,9 @@ def update(self, instance, validated_data): if topic_interests is not None: instance.topic_interests.set(topic_interests) + # drop any prefetched/cached list so the response reserializes + # the new interests + instance.__dict__.pop("annotated_topic_interests", None) email_optin_changed = ( "email_optin" in validated_data @@ -422,11 +431,15 @@ class Meta(UserSerializer.Meta): fields = (*UserSerializer.Meta.fields, "is_sso_user") -class ProgramCertificateSerializer(serializers.ModelSerializer): +class ProgramCertificateSerializer(BaseSerializer): """ Serializer for Program Certificates """ + # user_letter isn't a model field; callers attach the user's ProgramLetter + # to each certificate instance. + required_prefetches: list[str] = ["user_letter"] + program_letter_generate_url = serializers.SerializerMethodField() program_letter_share_url = serializers.SerializerMethodField() @@ -441,13 +454,10 @@ def get_program_letter_generate_url(self, instance) -> str: return letter_url def get_program_letter_share_url(self, instance) -> str: + # Callers attach user_letter, creating the letter if needed, so this is + # always a real URL -- same contract as when the get_or_create lived here. + letter_url = instance.user_letter.get_absolute_url() request = self.context.get("request") - - user = User.objects.get(email=instance.user_email) - letter, _created = ProgramLetter.objects.get_or_create( - user=user, certificate=instance - ) - letter_url = letter.get_absolute_url() if request: return request.build_absolute_uri(letter_url) return letter_url @@ -486,6 +496,12 @@ class ProgramLetterSerializer(serializers.ModelSerializer): certificate = ProgramCertificateSerializer() + def to_representation(self, instance): + """Attach the letter the nested certificate serializer needs.""" + # The view 404s a letter whose certificate is missing, so this is safe. + instance.certificate.user_letter = instance + return super().to_representation(instance) + @extend_schema_field(ProgramLetterTemplateFieldSerializer()) def get_template_fields(self, instance) -> dict: """Get template fields from the micromasters cms api""" diff --git a/profiles/views.py b/profiles/views.py index ab443cf814..7b2250e895 100644 --- a/profiles/views.py +++ b/profiles/views.py @@ -3,7 +3,8 @@ from cairosvg import svg2png # pylint:disable=no-name-in-module from django.contrib.auth import get_user_model from django.contrib.auth.decorators import login_required -from django.http import HttpResponse, HttpResponseRedirect +from django.db.models import Prefetch, QuerySet +from django.http import Http404, HttpResponse, HttpResponseRedirect from django.shortcuts import get_object_or_404, redirect from django.utils.decorators import method_decorator from django.views import View @@ -14,6 +15,7 @@ from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response +from learning_resources.models import LearningResourceTopic from main.permissions import ( AnonymousAccessReadonlyPermission, IsStaffPermission, @@ -35,6 +37,18 @@ ) +def profiles_for_serialization() -> QuerySet[Profile]: + """Profiles with the relations ProfileSerializer reads.""" + return Profile.objects.prefetch_related( + "userwebsite_set", + Prefetch( + "topic_interests", + queryset=LearningResourceTopic.objects.for_serialization(), + to_attr="annotated_topic_interests", + ), + ) + + class UserViewSet(viewsets.ModelViewSet): """View for users""" @@ -69,9 +83,7 @@ class ProfileViewSet( permission_classes = (AnonymousAccessReadonlyPermission, HasEditPermission) serializer_class = ProfileSerializer - queryset = Profile.objects.prefetch_related("userwebsite_set").filter( - user__is_active=True - ) + queryset = profiles_for_serialization().filter(user__is_active=True) lookup_field = "user__username" def get_object(self): @@ -79,7 +91,12 @@ def get_object(self): if self.kwargs["user__username"] == "me": ensure_profile(self.request.user) - return self.request.user.profile + # Fetched through the same queryset as the by-username route so the + # prefetches apply; deliberately without its user__is_active filter, + # which this branch has never applied. + return get_object_or_404( + profiles_for_serialization(), user=self.request.user + ) else: return super().get_object() @@ -106,6 +123,24 @@ class ProgramLetterViewSet(mixins.RetrieveModelMixin, viewsets.GenericViewSet): serializer_class = ProgramLetterSerializer queryset = ProgramLetter.objects.all() + def get_object(self) -> ProgramLetter: + """ + Return the letter, 404ing if its certificate is gone. + + certificate is nullable and its column has no FK constraint, since + ProgramCertificate is unmanaged, so the id can outlive the row. Every + field of the response derives from the certificate, so there is nothing + to render without it. + """ + letter = super().get_object() + try: + certificate = letter.certificate + except ProgramCertificate.DoesNotExist as exc: + raise Http404 from exc + if certificate is None: + raise Http404 + return letter + class UserProgramCertificateViewSet(viewsets.ViewSet): """ @@ -122,11 +157,40 @@ class UserProgramCertificateViewSet(viewsets.ViewSet): def list(self, request): queryset = ProgramCertificate.objects.filter(user_email=request.user.email) + certificates = list(self.filter_queryset(queryset)) + letters = self.user_letters(request.user, certificates) + for cert in certificates: + cert.user_letter = letters[cert.pk] serializer = ProgramCertificateSerializer( - self.filter_queryset(queryset), many=True, context={"request": request} + certificates, many=True, context={"request": request} ) return Response(serializer.data) + @staticmethod + def user_letters(user, certificates) -> dict: + """ + Map certificate id to the user's ProgramLetter, creating any that don't + exist yet. + + Serializing a certificate has always created its letter on demand; doing + it here in bulk keeps that behaviour without a query per certificate. + """ + letters = { + letter.certificate_id: letter + for letter in ProgramLetter.objects.filter( + user=user, certificate__in=certificates + ) + } + missing = [cert for cert in certificates if cert.pk not in letters] + if missing: + letters.update( + (letter.certificate_id, letter) + for letter in ProgramLetter.objects.bulk_create( + ProgramLetter(user=user, certificate=cert) for cert in missing + ) + ) + return letters + def filter_queryset(self, queryset): for backend in list(self.filter_backends): queryset = backend().filter_queryset(self.request, queryset, view=self) diff --git a/profiles/views_test.py b/profiles/views_test.py index 0852ab9406..5bdef471ec 100644 --- a/profiles/views_test.py +++ b/profiles/views_test.py @@ -241,7 +241,6 @@ def test_patch_profile_by_user(client, logged_in_profile): assert logged_in_profile.location == location_json -@pytest.mark.skip_nplusone_check def test_patch_topic_interests(client, logged_in_profile): """Test that patching Profile.topic_interests works correctly""" topics = LearningResourceTopicFactory.create_batch(3) @@ -507,6 +506,33 @@ def test_program_letter_api_view(mocker, client, rf, user, is_anonymous, setting ) +@pytest.mark.parametrize("certificate_id", [None, "no-such-record-hash"]) +def test_program_letter_api_view_without_certificate( + client, settings, user, certificate_id +): + """ + A letter whose certificate is gone 404s instead of erroring. + + ProgramCertificate is unmanaged, so certificate_id has no FK constraint and + can outlive the row it points at. Every field of the response derives from + the certificate, so there is nothing to serve without one. Nothing is mocked + here on purpose: MICROMASTERS_CMS_API_URL is set because the template fetch + dereferences the certificate too, and the view has to stop short of it. + """ + settings.DATABASE_ROUTERS = [] + settings.EXTERNAL_MODELS = [] + settings.MICROMASTERS_CMS_API_URL = "https://micromasters.example.com/api/v0/" + letter = ProgramLetter.objects.create(user=user, certificate=None) + if certificate_id is not None: + ProgramLetter.objects.filter(pk=letter.pk).update(certificate_id=certificate_id) + + response = client.get( + reverse("profile:v1:program_letters_api-detail", args=[letter.id]) + ) + + assert response.status_code == 404 + + @pytest.mark.parametrize("is_anonymous", [True, False]) def test_program_letter_api_view_returns_404_for_invalid_id( mocker, client, user, is_anonymous @@ -524,7 +550,6 @@ def test_program_letter_api_view_returns_404_for_invalid_id( assert response.status_code == 404 -@pytest.mark.skip_nplusone_check @pytest.mark.parametrize("is_anonymous", [True, False]) def test_list_user_program_certificates(mocker, client, user, is_anonymous): """ @@ -538,16 +563,31 @@ def test_list_user_program_certificates(mocker, client, user, is_anonymous): 3, user_email=user.email, ) + if not is_anonymous: + # One cert already has a letter; the GET creates the other two and + # reuses this one rather than issuing a second. + letter = ProgramLetterFactory(user=user, certificate=certs[0]) url = reverse("profile:v0:user_program_certificates_api-list") resp = client.get(url) if not is_anonymous: request = get_request_object(url) assert resp.status_code == 200 + assert ProgramLetter.objects.count() == len(certs) + assert ProgramLetter.objects.get(certificate=certs[0]).id == letter.id + + letters = { + item.certificate_id: item + for item in ProgramLetter.objects.filter(certificate__in=certs) + } + for cert in certs: + cert.user_letter = letters[cert.pk] assert ( resp.json() == ProgramCertificateSerializer( certs, many=True, context={"request": request} ).data ) + share_urls = [cert["program_letter_share_url"] for cert in resp.json()] + assert all(share_urls), share_urls else: assert resp.status_code == 403 diff --git a/pyproject.toml b/pyproject.toml index bddbc9be2f..ec17b90cc5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -61,7 +61,7 @@ dependencies = [ "lxml>=6.0.0,<7", "markdown>=3.7,<4", "markdown2>=2.4.8,<3", - "mitol-django-common>=2026.4.2,<2027", + "mitol-django-common>=2026.6.16.4,<2027", "mitol-django-scim>=2026.4.2,<2027", "mitol-django-observability>=2026.8.19,<2027", "named-enum>=1.4.0,<2", diff --git a/uv.lock b/uv.lock index 2dbb0f6304..c4bca095c5 100644 --- a/uv.lock +++ b/uv.lock @@ -2694,7 +2694,7 @@ requires-dist = [ { name = "lxml", specifier = ">=6.0.0,<7" }, { name = "markdown", specifier = ">=3.7,<4" }, { name = "markdown2", specifier = ">=2.4.8,<3" }, - { name = "mitol-django-common", specifier = ">=2026.4.2,<2027" }, + { name = "mitol-django-common", specifier = ">=2026.6.16.4,<2027" }, { name = "mitol-django-keycloak", specifier = ">=2026.7.13,<2027" }, { name = "mitol-django-observability", specifier = ">=2026.8.19,<2027" }, { name = "mitol-django-scim", specifier = ">=2026.4.2,<2027" }, @@ -2780,22 +2780,20 @@ dev = [ [[package]] name = "mitol-django-common" -version = "2026.4.2" +version = "2026.6.16.4" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "django" }, { name = "django-redis" }, { name = "django-stubs" }, { name = "django-webpack-loader" }, - { name = "factory-boy" }, - { name = "pytest" }, { name = "pytz" }, { name = "requests" }, { name = "typing-extensions" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/b1/47/1b9be48b8f5fc3cd3a9889853c78f8d8919ad0b22f1c6f4f4b92706ae45b/mitol_django_common-2026.4.2.tar.gz", hash = "sha256:609ad60133cac9d18cd5d2d4bcbc7048508b137b57c4990439de9c5016e758dd", size = 22447, upload-time = "2026-04-02T15:46:55.658Z" } +sdist = { url = "https://files.pythonhosted.org/packages/5a/c8/bfddaffd14e2f0233e0be31ea83a82865579f9eb1ecc9514cf14edd5d574/mitol_django_common-2026.6.16.4.tar.gz", hash = "sha256:f06dee7575c4c1cfefef016459ed8c673482511b9b0f8387fc2e056844ca6a8c", size = 24357, upload-time = "2026-06-16T19:21:07.244Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/bc/f5/f781062585655f35300d73317aa46cfdf9ab8f17f2142e7463e8a0722325/mitol_django_common-2026.4.2-py3-none-any.whl", hash = "sha256:4569bb3118047d2f79658fb71686584d140eaca9152b5f46ca52930fa0cddc8d", size = 32382, upload-time = "2026-04-02T15:46:54.84Z" }, + { url = "https://files.pythonhosted.org/packages/00/d2/db0dd231888821180997dd1a0c35fcd3d7999567596d27604bcb939d0ee6/mitol_django_common-2026.6.16.4-py3-none-any.whl", hash = "sha256:3506d91de35e91bc17e65eee73fe68cfa0b73254edb66d9a59d6e9cee870580b", size = 35258, upload-time = "2026-06-16T19:21:06.429Z" }, ] [[package]] diff --git a/website_content/serializers.py b/website_content/serializers.py index 1a737e8a77..d89535826c 100644 --- a/website_content/serializers.py +++ b/website_content/serializers.py @@ -1,5 +1,6 @@ from django.contrib.auth import get_user_model from drf_spectacular.utils import extend_schema_field +from mitol.common.serializers import BaseSerializer from rest_framework import serializers from website_content import models @@ -25,11 +26,13 @@ class Meta: fields = ["first_name", "last_name"] -class WebsiteContentSerializer(serializers.ModelSerializer): +class WebsiteContentSerializer(BaseSerializer): """ Serializer for WebsiteContent model. """ + required_prefetches: list[str] = ["user"] + created_on = serializers.DateTimeField(read_only=True, required=False) updated_on = serializers.DateTimeField(read_only=True, required=False) publish_date = serializers.DateTimeField(read_only=True, required=False) diff --git a/website_content/views.py b/website_content/views.py index e70557fb91..82131afddd 100644 --- a/website_content/views.py +++ b/website_content/views.py @@ -69,7 +69,7 @@ def get_queryset(self): if not (is_admin_user(self.request) or is_website_content_editor(self.request)): qs = qs.filter(is_published=True) - return qs.order_by("-publish_date", "-id") + return qs.select_related("user").order_by("-publish_date", "-id") @method_decorator( cache_page_per_user( diff --git a/website_content/views_test.py b/website_content/views_test.py index 1dc1604acf..7cd44fb24d 100644 --- a/website_content/views_test.py +++ b/website_content/views_test.py @@ -270,3 +270,25 @@ def test_content_type_filter(client, user): assert all(r["content_type"] == "news" for r in results) assert len(results) == 1 + + +@pytest.mark.parametrize("limit", [2, 10]) +def test_list_query_count_is_constant(client, django_assert_num_queries, limit): + """Listing costs the same number of queries whatever the page size.""" + for content_user in [*UserFactory.create_batch(5), None]: + WebsiteContent.objects.create( + title="t", + content={}, + is_published=True, + user=content_user, + content_type="news", + ) + + url = reverse("website_content:v1:website_content-list") + with django_assert_num_queries(2): + results = client.get(url, {"limit": limit}).json()["results"] + + assert len(results) == min(limit, 6) + # The most recently published item has no user; a nullable FK still has to + # serialize, which a select_related() join preserves and an inner join wouldn't. + assert results[0]["user"] is None From 89cb531f0adf99bdac702c0c85a6b31ae95e2e98 Mon Sep 17 00:00:00 2001 From: Doof Date: Mon, 31 Aug 2026 12:38:33 +0000 Subject: [PATCH 14/14] Release 0.78.0 --- RELEASE.rst | 17 +++++++++++++++++ main/settings.py | 2 +- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/RELEASE.rst b/RELEASE.rst index be020a4ea8..42b69a903d 100644 --- a/RELEASE.rst +++ b/RELEASE.rst @@ -1,6 +1,23 @@ Release Notes ============= +Version 0.78.0 +-------------- + +- Remove remaining API endpoint N+1 queries (#3845) +- Update dependency drf-spectacular to >=0.30,<0.31 (#3858) +- Update dependency ruff to v0.16.3 (#3859) +- Submit and link each resource under one URL, from learn_url (#3843) +- Update dependency litellm to v1.96.2 (#3857) +- Update redis Docker tag to v8.10.0 (#2706) +- feat(cohort-1): MicroMasters/MITx Online certificate warehouse-pull sync (#3808) +- ci: add a ci-gate job so one required check can cover the whole suite (#3825) +- Overridable Mutation Error Toast (#3837) +- Show Stay Updated based only on the CMS page flag (#3841) +- staleness penalty for vector search results (#3834) +- Update dependency Django to v5.2.17 [SECURITY] (#3849) +- Update dependency social-auth-app-django to v5.6.0 [SECURITY] (#3850) + Version 0.77.15 (Released August 27, 2026) --------------- diff --git a/main/settings.py b/main/settings.py index a16f5ac60b..ec2768a44f 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.77.15" +VERSION = "0.78.0" log = logging.getLogger()