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