diff --git a/frontends/api/src/hooks/learningPaths/index.test.ts b/frontends/api/src/hooks/learningPaths/index.test.ts index 2cb774c371..ddfe2ab584 100644 --- a/frontends/api/src/hooks/learningPaths/index.test.ts +++ b/frontends/api/src/hooks/learningPaths/index.test.ts @@ -15,6 +15,7 @@ import { useLearningPathCreate, useLearningPathDestroy, useLearningPathUpdate, + useLearningPathListItemMove, } from "./index" import { learningPathKeys } from "./queries" @@ -163,6 +164,9 @@ describe("LearningPath CRUD", () => { expect(queryClient.invalidateQueries).toHaveBeenCalledWith({ queryKey: ["learningPaths", "membershipList"], }) + expect(queryClient.invalidateQueries).toHaveBeenCalledWith({ + queryKey: learningResourceKeys.featuredRoot(), + }) }) test("useLearningPathUpdate calls correct API", async () => { @@ -190,5 +194,31 @@ describe("LearningPath CRUD", () => { expect(queryClient.invalidateQueries).toHaveBeenCalledWith({ queryKey: ["learningPaths", "detail", path.id], }) + expect(queryClient.invalidateQueries).toHaveBeenCalledWith({ + queryKey: learningResourceKeys.featuredRoot(), + }) + }) + + test("useLearningPathListItemMove invalidates featured and items queries", async () => { + const { path, relationship, pathUrls } = makeData() + setMockResponse.patch(pathUrls.relationshipDetails, relationship) + + const { wrapper, queryClient } = setupReactQueryTest() + jest.spyOn(queryClient, "invalidateQueries") + + const { result } = renderHook(useLearningPathListItemMove, { wrapper }) + result.current.mutate({ + parent: path.id, + id: relationship.id, + position: relationship.position, + }) + await waitFor(() => expect(result.current.isSuccess).toBe(true)) + + expect(queryClient.invalidateQueries).toHaveBeenCalledWith({ + queryKey: learningPathKeys.infiniteItemsRoot(path.id), + }) + expect(queryClient.invalidateQueries).toHaveBeenCalledWith({ + queryKey: learningResourceKeys.featuredRoot(), + }) }) }) diff --git a/frontends/api/src/hooks/learningPaths/index.ts b/frontends/api/src/hooks/learningPaths/index.ts index 75c5912c1d..ada723a6eb 100644 --- a/frontends/api/src/hooks/learningPaths/index.ts +++ b/frontends/api/src/hooks/learningPaths/index.ts @@ -13,6 +13,7 @@ import type { } from "../../generated/v1" import { learningPathsApi } from "../../clients" import { learningPathQueries, learningPathKeys } from "./queries" +import { learningResourceKeys } from "../learningResources/queries" import { useUserHasPermission, Permission } from "api/hooks/user" const useLearningPathsList = ( @@ -71,6 +72,9 @@ const useLearningPathUpdate = () => { queryClient.invalidateQueries({ queryKey: learningPathKeys.detail(vars.id), }) + queryClient.invalidateQueries({ + queryKey: learningResourceKeys.featuredRoot(), + }) }, }) } @@ -85,6 +89,9 @@ const useLearningPathDestroy = () => { queryClient.invalidateQueries({ queryKey: learningPathKeys.membershipList(), }) + queryClient.invalidateQueries({ + queryKey: learningResourceKeys.featuredRoot(), + }) }, }) } @@ -109,6 +116,9 @@ const useLearningPathListItemMove = () => { queryClient.invalidateQueries({ queryKey: learningPathKeys.infiniteItemsRoot(vars.parent), }) + queryClient.invalidateQueries({ + queryKey: learningResourceKeys.featuredRoot(), + }) }, }) } diff --git a/frontends/api/src/hooks/learningResources/index.test.ts b/frontends/api/src/hooks/learningResources/index.test.ts index 8b7ae81098..9191b39659 100644 --- a/frontends/api/src/hooks/learningResources/index.test.ts +++ b/frontends/api/src/hooks/learningResources/index.test.ts @@ -9,7 +9,10 @@ import { useInfiniteLearningResourceItems, useLearningResourcesList, useLearningResourceTopics, + useLearningResourceSetLearningPathRelationships, } from "./index" +import { learningResourceKeys } from "./queries" +import { learningPathKeys } from "../learningPaths/queries" import { setMockResponse, urls, makeRequest } from "../../test-utils" import * as factories from "../../test-utils/factories" import { UseQueryResult } from "@tanstack/react-query" @@ -142,3 +145,32 @@ describe("useLearningResourceTopics", () => { }, ) }) + +describe("useLearningResourceSetLearningPathRelationships", () => { + it("invalidates learning path and featured queries", async () => { + const resource = factory.resource() + const url = urls.learningResources.setLearningPathRelationships({ + id: resource.id, + }) + setMockResponse.patch(url, resource) + + const { wrapper, queryClient } = setupReactQueryTest() + jest.spyOn(queryClient, "invalidateQueries") + + const { result } = renderHook( + useLearningResourceSetLearningPathRelationships, + { + wrapper, + }, + ) + result.current.mutate({ id: resource.id }) + await waitFor(() => expect(result.current.isSuccess).toBe(true)) + + expect(queryClient.invalidateQueries).toHaveBeenCalledWith({ + queryKey: learningPathKeys.root, + }) + expect(queryClient.invalidateQueries).toHaveBeenCalledWith({ + queryKey: learningResourceKeys.featuredRoot(), + }) + }) +}) diff --git a/frontends/api/src/hooks/learningResources/index.ts b/frontends/api/src/hooks/learningResources/index.ts index 5144938a44..3c9d42d343 100644 --- a/frontends/api/src/hooks/learningResources/index.ts +++ b/frontends/api/src/hooks/learningResources/index.ts @@ -165,6 +165,9 @@ const useLearningResourceSetLearningPathRelationships = () => { * Additionally, the lists we've removed from the resource are not easily available. */ queryClient.invalidateQueries({ queryKey: learningPathKeys.root }) + queryClient.invalidateQueries({ + queryKey: learningResourceKeys.featuredRoot(), + }) }, }) } diff --git a/frontends/ol-components/src/components/LearningResourceCard/LearningResourceListCard.tsx b/frontends/ol-components/src/components/LearningResourceCard/LearningResourceListCard.tsx index 802c24d4e6..7132187540 100644 --- a/frontends/ol-components/src/components/LearningResourceCard/LearningResourceListCard.tsx +++ b/frontends/ol-components/src/components/LearningResourceCard/LearningResourceListCard.tsx @@ -91,7 +91,10 @@ export const Count = ({ resource }: { resource: LearningResource }) => { if (resource.resource_type !== ResourceTypeEnum.LearningPath) { return null } - const count = resource.learning_path.item_count + const count = resource.learning_path?.item_count + if (count === undefined) { + return null + } return (
{count} {pluralize("item", count)} diff --git a/learning_resources/views.py b/learning_resources/views.py index 589b5482c3..f659a87a15 100644 --- a/learning_resources/views.py +++ b/learning_resources/views.py @@ -19,6 +19,7 @@ inline_serializer, ) from grpc._channel import _InactiveRpcError +from requests.exceptions import RequestException from rest_framework import serializers, views, viewsets from rest_framework.decorators import action from rest_framework.filters import OrderingFilter @@ -109,7 +110,13 @@ AnonymousAccessReadonlyPermission, is_admin_user, ) -from main.utils import cache_page_for_all_users, cache_page_for_anonymous_users, chunks +from main.utils import ( + cache_page_for_all_users, + cache_page_for_anonymous_users, + call_fastly_purge_api, + chunks, + clear_views_cache, +) from vector_search.serializers import LearningResourcesSearchFiltersSerializer @@ -479,6 +486,50 @@ def get_queryset(self): ).filter(published=True) +def clear_featured_caches(channel_names): + """ + Clear the Redis featured-list cache, hard-purge channel pages from + Fastly, and soft-purge the homepage. Each Fastly purge is independently + best-effort so one failure doesn't leave the remaining pages stale. + """ + clear_views_cache(key_prefix="featured_resources") + purges = [(f"/c/unit/{name}", False) for name in channel_names] + [("/", True)] + for relative_url, soft in purges: + try: + call_fastly_purge_api(relative_url, timeout=5, soft=soft) + except RequestException: + log.exception("Featured cache Fastly purge failed for %s", relative_url) + + +def _clear_featured_caches_on_commit(path_resource_ids): + """ + Clear the featured caches after commit if any of the given paths is a + unit channel's featured list; best-effort, never raises. + + Runs synchronously in the request (not via Celery) so the purge is done + by the time the editor's save returns, regardless of worker backlog. + """ + channel_names = list( + Channel.objects.filter( + featured_list_id__in=path_resource_ids, + channel_type=ChannelType.unit.name, + ).values_list("name", flat=True) + ) + if not channel_names: + return + + def _clear(): + try: + clear_featured_caches(channel_names) + except Exception: + log.exception( + "Failed to clear featured caches for channels %s", + channel_names, + ) + + transaction.on_commit(_clear) + + @extend_schema_view( list=extend_schema( summary="List", description="Get a paginated list of learning paths" @@ -543,6 +594,18 @@ def update(self, request, *_args, **kwargs): ) return Response(serializer.data) + def perform_update(self, serializer): + super().perform_update(serializer) + _clear_featured_caches_on_commit([serializer.instance.id]) + + def perform_destroy(self, instance): + # Resolve channel names before the delete (Channel.featured_list is + # on_delete=SET_NULL); the atomic block defers the on_commit clear + # until after the delete commits. + with transaction.atomic(): + _clear_featured_caches_on_commit([instance.id]) + super().perform_destroy(instance) + @extend_schema_view( list=extend_schema( @@ -745,6 +808,9 @@ def learning_paths(self, request, *args, **kwargs): # noqa: ARG002 relation_type=LearningResourceRelationTypes.LEARNING_PATH_ITEMS.value, parent__resource_type=LearningResourceType.learning_path.name, ) + previous_parent_ids = list( + current_relationships.values_list("parent_id", flat=True) + ) # Remove the resource from lists it WAS in before but is not in now current_relationships.exclude(parent_id__in=learning_path_ids).delete() current_parent_lists = current_relationships.values_list("parent_id", flat=True) @@ -771,6 +837,7 @@ def learning_paths(self, request, *args, **kwargs): # noqa: ARG002 relation_type=LearningResourceRelationTypes.LEARNING_PATH_ITEMS.value, position=last_index + 1, ) + _clear_featured_caches_on_commit({*previous_parent_ids, *learning_path_ids}) current_relationships = LearningResourceRelationship.objects.prefetch_related( Prefetch( "child", @@ -845,6 +912,7 @@ def create(self, request, *args, **kwargs): # noqa: ARG002 serializer = self.get_serializer(data=request.data) serializer.is_valid(raise_exception=True) serializer.save(parent_id=self.kwargs.get("learning_resource_id")) + _clear_featured_caches_on_commit([self.kwargs.get("learning_resource_id")]) relationship = LearningResourceRelationship.objects.prefetch_related( Prefetch("child", queryset=LearningResource.objects.for_serialization()) @@ -857,7 +925,9 @@ def create(self, request, *args, **kwargs): # noqa: ARG002 return Response(response_serializer.data, status=201, headers=headers) def update(self, request, *args, **kwargs): - return super().update(request, *args, **kwargs) + response = super().update(request, *args, **kwargs) + _clear_featured_caches_on_commit([self.kwargs.get("learning_resource_id")]) + return response def perform_destroy(self, instance): """Delete the relationship and update the positions of the remaining items""" @@ -868,6 +938,7 @@ def perform_destroy(self, instance): position__gt=instance.position, ).update(position=F("position") - 1) instance.delete() + _clear_featured_caches_on_commit([instance.parent_id]) @extend_schema_view( diff --git a/learning_resources/views_learningpath_test.py b/learning_resources/views_learningpath_test.py index 0ff92afab3..9835bb7d50 100644 --- a/learning_resources/views_learningpath_test.py +++ b/learning_resources/views_learningpath_test.py @@ -5,8 +5,10 @@ import pytest from django.db.models import Max from django.urls import reverse +from requests.exceptions import RequestException -from learning_resources import factories, models +from channels.factories import ChannelFactory +from learning_resources import factories, models, views from learning_resources.constants import ( LearningResourceRelationTypes, ) @@ -565,3 +567,219 @@ def test_adding_to_learning_path_not_effect_existing_membership(client, staff_us new_additional_parent_count + 1 == new_additional_parent.learning_resource.resources.count() ) + + +@pytest.fixture +def mock_featured_clear(mocker): + """Mock the synchronous clear_featured_caches function""" + return mocker.patch("learning_resources.views.clear_featured_caches") + + +@pytest.fixture +def featured_path(client, user): + """Create a learning path featured by a unit channel and log in an editor""" + update_editor_group(user, True) # noqa: FBT003 + path = factories.LearningPathFactory.create(author=user) + channel = ChannelFactory.create(is_unit=True, featured_list=path.learning_resource) + client.force_login(user) + return path, channel + + +@pytest.mark.parametrize( + ("method", "data", "expected_status"), + [("patch", {"title": "New title"}, 200), ("delete", None, 204)], +) +def test_learning_path_write_clears_featured_caches( # noqa: PLR0913 + client, + featured_path, + mock_featured_clear, + django_capture_on_commit_callbacks, + method, + data, + expected_status, +): + """ + Updating or deleting a featured learning path clears the featured caches + on commit (delete resolves channel names before the row is gone) + """ + path, channel = featured_path + + with django_capture_on_commit_callbacks(execute=True): + resp = getattr(client, method)( + reverse("lr:v1:learningpaths_api-detail", args=[path.learning_resource.id]), + data=data, + format="json", + ) + + assert resp.status_code == expected_status + mock_featured_clear.assert_called_once_with([channel.name]) + + +def test_learning_path_update_not_featured_no_clear( + client, user, mock_featured_clear, django_capture_on_commit_callbacks +): + """PATCHing a learning path that is no channel's featured list clears nothing""" + update_editor_group(user, True) # noqa: FBT003 + path = factories.LearningPathFactory.create(author=user) + client.force_login(user) + + with django_capture_on_commit_callbacks(execute=True): + resp = client.patch( + reverse("lr:v1:learningpaths_api-detail", args=[path.learning_resource.id]), + data={"title": "New title"}, + format="json", + ) + + assert resp.status_code == 200 + mock_featured_clear.assert_not_called() + + +def test_featured_cache_clear_failure_does_not_break_save( + client, featured_path, mock_featured_clear, django_capture_on_commit_callbacks +): + """A failing cache clear (Redis/Fastly down) must not break the API response""" + mock_featured_clear.side_effect = Exception("broker down") + path, _ = featured_path + + with django_capture_on_commit_callbacks(execute=True): + resp = client.patch( + reverse("lr:v1:learningpaths_api-detail", args=[path.learning_resource.id]), + data={"title": "New title"}, + format="json", + ) + + assert resp.status_code == 200 + + +def test_learning_path_item_create_clears_featured_caches( + client, featured_path, mock_featured_clear, django_capture_on_commit_callbacks +): + """Adding an item to a featured path clears the featured caches""" + path, channel = featured_path + course = factories.CourseFactory.create() + + with django_capture_on_commit_callbacks(execute=True): + resp = client.post( + reverse( + "lr:v1:learningpathitems_api-list", args=[path.learning_resource.id] + ), + data={"child": course.learning_resource.id}, + format="json", + ) + + assert resp.status_code == 201 + mock_featured_clear.assert_called_once_with([channel.name]) + + +def test_learning_path_item_update_clears_featured_caches( + client, featured_path, mock_featured_clear, django_capture_on_commit_callbacks +): + """Reordering an item in a featured path clears the featured caches""" + path, channel = featured_path + path.learning_resource.children.all().delete() + items = sorted( + factories.LearningPathRelationshipFactory.create_batch( + 2, parent=path.learning_resource + ), + key=lambda item: item.position, + ) + + with django_capture_on_commit_callbacks(execute=True): + resp = client.patch( + reverse( + "lr:v1:learningpathitems_api-detail", + args=[path.learning_resource.id, items[0].id], + ), + data={"position": items[1].position}, + format="json", + ) + + assert resp.status_code == 200 + mock_featured_clear.assert_called_once_with([channel.name]) + + +def test_learning_path_item_delete_clears_featured_caches( + client, featured_path, mock_featured_clear, django_capture_on_commit_callbacks +): + """Removing an item from a featured path clears the featured caches""" + path, channel = featured_path + path.learning_resource.children.all().delete() + items = factories.LearningPathRelationshipFactory.create_batch( + 2, parent=path.learning_resource + ) + + with django_capture_on_commit_callbacks(execute=True): + resp = client.delete( + reverse( + "lr:v1:learningpathitems_api-detail", + args=[path.learning_resource.id, items[0].id], + ) + ) + + assert resp.status_code == 204 + mock_featured_clear.assert_called_once_with([channel.name]) + + +def test_set_learning_path_relationships_clears_featured_caches( + client, staff_user, mock_featured_clear, django_capture_on_commit_callbacks +): + """Bulk membership set clears caches for both added and removed featured paths""" + course = factories.CourseFactory.create() + added_path = factories.LearningPathFactory.create(author=staff_user) + removed_path = factories.LearningPathFactory.create(author=staff_user) + factories.LearningPathRelationshipFactory.create( + parent=removed_path.learning_resource, child=course.learning_resource + ) + added_channel = ChannelFactory.create( + is_unit=True, featured_list=added_path.learning_resource + ) + removed_channel = ChannelFactory.create( + is_unit=True, featured_list=removed_path.learning_resource + ) + url = reverse( + "lr:v1:learning_resource_relationships_api-learning-paths", + args=[course.learning_resource.id], + ) + client.force_login(staff_user) + + with django_capture_on_commit_callbacks(execute=True): + resp = client.patch(f"{url}?learning_path_id={added_path.learning_resource.id}") + + assert resp.status_code == 200 + mock_featured_clear.assert_called_once() + (names,) = mock_featured_clear.call_args.args + assert sorted(names) == sorted([added_channel.name, removed_channel.name]) + + +def test_clear_featured_caches(mocker): + """Clears the Redis prefix first, then purges channel pages hard and homepage soft""" + manager = mocker.Mock() + manager.attach_mock( + mocker.patch("learning_resources.views.clear_views_cache"), + "clear_views_cache", + ) + manager.attach_mock( + mocker.patch("learning_resources.views.call_fastly_purge_api"), "purge" + ) + + views.clear_featured_caches(["mitx", "ocw"]) + + assert manager.mock_calls == [ + mocker.call.clear_views_cache(key_prefix="featured_resources"), + mocker.call.purge("/c/unit/mitx", timeout=5, soft=False), + mocker.call.purge("/c/unit/ocw", timeout=5, soft=False), + mocker.call.purge("/", timeout=5, soft=True), + ] + + +def test_clear_featured_caches_continues_after_purge_failure(mocker): + """A failed channel purge must not skip the remaining Fastly purges""" + mocker.patch("learning_resources.views.clear_views_cache") + mock_purge = mocker.patch( + "learning_resources.views.call_fastly_purge_api", + side_effect=[RequestException("fastly down"), None, None], + ) + + views.clear_featured_caches(["mitx", "ocw"]) + + assert mock_purge.call_count == 3 diff --git a/main/utils.py b/main/utils.py index db209a800f..64301d5af4 100644 --- a/main/utils.py +++ b/main/utils.py @@ -212,7 +212,7 @@ def inner_function(request, *args, **kwargs): return inner_decorator -def call_fastly_purge_api(relative_url, timeout=30): +def call_fastly_purge_api(relative_url, timeout=30, *, soft=False): """ Call the Fastly purge API. @@ -223,6 +223,8 @@ def call_fastly_purge_api(relative_url, timeout=30): Args: - relative_url The relative URL to purge. - timeout Timeout in seconds for the request (default: 30) + - soft If True, send a soft purge (Fastly-Soft-Purge: 1) so + Fastly marks the object stale instead of evicting it Returns: - Dict of the response (resp.json) Raises: @@ -242,6 +244,9 @@ def call_fastly_purge_api(relative_url, timeout=30): headers = {} + if soft: + headers["Fastly-Soft-Purge"] = "1" + if settings.FASTLY_API_KEY: headers["fastly-key"] = settings.FASTLY_API_KEY diff --git a/main/utils_test.py b/main/utils_test.py index adeb91f757..27a9399364 100644 --- a/main/utils_test.py +++ b/main/utils_test.py @@ -26,6 +26,7 @@ _sorted_query_string, cache_page_for_all_users, cache_page_for_anonymous_users, + call_fastly_purge_api, chunks, clean_data, clear_views_cache, @@ -678,3 +679,16 @@ def test_clear_views_cache_uses_large_itersize(mock_caches): mock_cache.delete_pattern.assert_called_once_with("views.*", itersize=1000) assert result == 3 + + +@pytest.mark.parametrize("soft", [True, False]) +def test_call_fastly_purge_api_soft_header(mocker, settings, soft): + """soft=True sends the Fastly-Soft-Purge: 1 header; default sends none""" + settings.FASTLY_API_KEY = "fake-key" + mock_request = mocker.patch("main.utils.requests.request") + mock_request.return_value.json.return_value = {"status": "ok"} + + call_fastly_purge_api("/c/unit/mitx", soft=soft) + + headers = mock_request.call_args.kwargs["headers"] + assert headers.get("Fastly-Soft-Purge") == ("1" if soft else None)