From 698845e19a88d4531cf42363ef5fa1e8d4f7137a Mon Sep 17 00:00:00 2001 From: ssavutu Date: Thu, 3 Sep 2026 16:24:36 -0400 Subject: [PATCH] Let more than one story break at a time The newsroom hit both limits on the same morning. DragonFly went out, the Academy story broke an hour later, and there was nowhere to put it: the banner shows one story, and pinning the second would have taken the first one down. Erik asked for both, and Audrey's workaround was to wait a day. Banner: GetBreakingNewsState now returns every published flagged article, newest first, capped at three. The cap is about how long a reader waits for a headline to come back around on a scrolling banner, not about width. The response keeps `enabled`, `text` and `article_slug` describing the newest story and adds `items` alongside them. Scalene reads those three fields today, so it goes on rendering the newest story until it learns about `items` -- no window where the banner is blank because one repo deployed before the other. Pins: `priority` is no longer exclusive. The three ClearFeaturedExcept calls are gone, and with them the functions, so nothing takes a pin down on an editor's behalf. Pinned stories lead the homepage newest-first, up to three, and an editor who wants the older one on top unpins the newer. Recency decides the order; the toggle decides what is in the running. The homepage splices all of them rather than one, dropping duplicates so a pinned news story is promoted rather than printed twice, and re-trims to the block's limit. Settings lists every story on the banner with a link to each article, since an editor wondering why their headline is not up needs to see the ones that are. The two checkbox blurbs say what the caps are. Tests: the exclusivity test now asserts the opposite, and there are new ones for banner ordering and its cap, the manual banner as an item, pins ordering and their cap, and the homepage end-to-end with two pins. Run against a real MariaDB 11.8 with CMS_TEST_DSN, not just the unit path. The public site still renders one banner story and one lead card. This is the CMS half; Scalene needs the marquee and the second lead slot before any of it is visible. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01L4qBhBdQto1yNp1zP7VLYc --- frontend/src/pages/editArticleView.tsx | 7 +- frontend/src/pages/settingsPage.tsx | 59 +++++++--- server/docs/docs.go | 25 ++++- server/docs/swagger.json | 25 ++++- server/docs/swagger.yaml | 19 +++- .../breaking_news_integration_test.go | 65 ++++++++++- server/internal/database/featured_article.go | 70 +++++------- server/internal/database/settings.go | 73 ++++++++---- ...breaking_news_settings_integration_test.go | 45 ++++++++ .../featured_article_integration_test.go | 78 ++++++++++--- .../handlers/featured_article_test.go | 50 ++++++--- server/internal/handlers/handlers.go | 105 +++++++----------- server/internal/models/api_responses.go | 25 ++++- 13 files changed, 454 insertions(+), 192 deletions(-) diff --git a/frontend/src/pages/editArticleView.tsx b/frontend/src/pages/editArticleView.tsx index fcf9fa5..6194d83 100644 --- a/frontend/src/pages/editArticleView.tsx +++ b/frontend/src/pages/editArticleView.tsx @@ -1548,7 +1548,8 @@ function EditArticleView() { Breaking news - Raises the homepage banner with this headline once the article publishes. + Adds this headline to the scrolling homepage banner once the article publishes. Up to three run at + once, newest first. @@ -1563,8 +1564,8 @@ function EditArticleView() { Featured article - Runs as the big lead story on the homepage. Only one article can be - featured, so this replaces the current one. + Pins this story to the top of the homepage. Up to three can be pinned; the newest leads, and pinning + this one leaves the others up. diff --git a/frontend/src/pages/settingsPage.tsx b/frontend/src/pages/settingsPage.tsx index 11ae083..35e6535 100644 --- a/frontend/src/pages/settingsPage.tsx +++ b/frontend/src/pages/settingsPage.tsx @@ -39,12 +39,18 @@ const emptyCarouselSlide = (): CarouselSlide => ({ // Mirrors server/internal/database/settings.go; the API clamps anything larger. const MAX_BREAKING_WINDOW_HOURS = 24 * 7 +type BreakingNewsItem = { + text?: string + article_slug?: string +} + type BreakingNewsResponse = { enabled?: boolean text?: string source?: string article_slug?: string article_title?: string + items?: BreakingNewsItem[] manual?: { enabled?: boolean; text?: string } window_hours?: number } @@ -53,6 +59,7 @@ type BreakingNewsLive = { source: string text: string articleSlug?: string + items: { text: string; articleSlug?: string }[] } // The endpoint returns the resolved banner at the top level and the hand-typed @@ -69,6 +76,13 @@ function readBreakingNews(body: BreakingNewsResponse) { source: String(body.source ?? "none"), text: String(body.text ?? ""), articleSlug: body.article_slug, + // The banner scrolls every flagged story, so the screen has to show all + // of them: an editor looking for why their headline is not up needs to + // see the others that are. + items: (body.items ?? []).map((item) => ({ + text: String(item.text ?? ""), + articleSlug: item.article_slug, + })), } satisfies BreakingNewsLive, // 0 means no time limit, which is the default. window: String(body.window_hours ?? 0), @@ -122,7 +136,7 @@ export default function SettingsPage() { const [breakingMessage, setBreakingMessage] = useState(null) // What the public site is actually showing: an article can override the // fields below. - const [breakingLive, setBreakingLive] = useState({ source: "none", text: "" }) + const [breakingLive, setBreakingLive] = useState({ source: "none", text: "", items: [] }) const [carouselSlides, setCarouselSlides] = useState([]) const [carouselSaved, setCarouselSaved] = useState([]) const [carouselSaving, setCarouselSaving] = useState(false) @@ -694,23 +708,42 @@ export default function SettingsPage() { storageKey="breaking-news" dirty={breakingDirty} summary={ - breakingLive.source === "article" ? "Live from an article" : breakingLive.source === "manual" ? "Enabled" : "Off" + breakingLive.source === "article" + ? breakingLive.items.length > 1 + ? `Live from ${breakingLive.items.length} articles` + : "Live from an article" + : breakingLive.source === "manual" + ? "Enabled" + : "Off" } - description="Banner across the top of the public homepage. Editors can also raise one by ticking Breaking news on an article." + description="Banner across the top of the public homepage. Editors can also raise one by ticking Breaking news on an article; the banner scrolls through all of them." > {breakingLive.source === "article" && (
- An article is driving the banner: “{breakingLive.text}” + + {breakingLive.items.length > 1 ? "Articles on the banner:" : "An article is driving the banner:"} + +
    + {breakingLive.items.map((item, index) => ( +
  1. + “{item.text}” + {item.articleSlug && ( + <> + {" "} + + edit + + + )} +
    2. + ))} +
- It overrides the banner below. Untick “Breaking news” on{" "} - {breakingLive.articleSlug ? ( - - that article - - ) : ( - "that article" - )}{" "} - to take it down. + These override the banner below, newest first. Untick “Breaking news” on an article to take it + off.
)} diff --git a/server/docs/docs.go b/server/docs/docs.go index be5b0cc..5925717 100644 --- a/server/docs/docs.go +++ b/server/docs/docs.go @@ -4981,6 +4981,17 @@ const docTemplate = `{ } } }, + "models.BreakingNewsItem": { + "type": "object", + "properties": { + "article_slug": { + "type": "string" + }, + "text": { + "type": "string" + } + } + }, "models.BreakingNewsSettings": { "type": "object", "properties": { @@ -4990,6 +5001,12 @@ const docTemplate = `{ "enabled": { "type": "boolean" }, + "items": { + "type": "array", + "items": { + "$ref": "#/definitions/models.BreakingNewsItem" + } + }, "text": { "type": "string" } @@ -5021,6 +5038,12 @@ const docTemplate = `{ "enabled": { "type": "boolean" }, + "items": { + "type": "array", + "items": { + "$ref": "#/definitions/models.BreakingNewsItem" + } + }, "manual": { "$ref": "#/definitions/models.BreakingNewsSettings" }, @@ -5031,7 +5054,7 @@ const docTemplate = `{ "type": "string" }, "window_hours": { - "description": "WindowHours is 0 when a flagged article holds the banner indefinitely,\nwhich is the default; an admin sets a limit in Settings to opt in.", + "description": "WindowHours is 0 (the default) when a flagged article holds the banner\nindefinitely.", "type": "integer" } } diff --git a/server/docs/swagger.json b/server/docs/swagger.json index 4c26911..cb88e26 100644 --- a/server/docs/swagger.json +++ b/server/docs/swagger.json @@ -4978,6 +4978,17 @@ } } }, + "models.BreakingNewsItem": { + "type": "object", + "properties": { + "article_slug": { + "type": "string" + }, + "text": { + "type": "string" + } + } + }, "models.BreakingNewsSettings": { "type": "object", "properties": { @@ -4987,6 +4998,12 @@ "enabled": { "type": "boolean" }, + "items": { + "type": "array", + "items": { + "$ref": "#/definitions/models.BreakingNewsItem" + } + }, "text": { "type": "string" } @@ -5018,6 +5035,12 @@ "enabled": { "type": "boolean" }, + "items": { + "type": "array", + "items": { + "$ref": "#/definitions/models.BreakingNewsItem" + } + }, "manual": { "$ref": "#/definitions/models.BreakingNewsSettings" }, @@ -5028,7 +5051,7 @@ "type": "string" }, "window_hours": { - "description": "WindowHours is 0 when a flagged article holds the banner indefinitely,\nwhich is the default; an admin sets a limit in Settings to opt in.", + "description": "WindowHours is 0 (the default) when a flagged article holds the banner\nindefinitely.", "type": "integer" } } diff --git a/server/docs/swagger.yaml b/server/docs/swagger.yaml index cadfcb0..3dedc7e 100644 --- a/server/docs/swagger.yaml +++ b/server/docs/swagger.yaml @@ -475,12 +475,23 @@ definitions: pagination: $ref: '#/definitions/models.Pagination' type: object + models.BreakingNewsItem: + properties: + article_slug: + type: string + text: + type: string + type: object models.BreakingNewsSettings: properties: article_slug: type: string enabled: type: boolean + items: + items: + $ref: '#/definitions/models.BreakingNewsItem' + type: array text: type: string type: object @@ -501,6 +512,10 @@ definitions: type: string enabled: type: boolean + items: + items: + $ref: '#/definitions/models.BreakingNewsItem' + type: array manual: $ref: '#/definitions/models.BreakingNewsSettings' source: @@ -509,8 +524,8 @@ definitions: type: string window_hours: description: |- - WindowHours is 0 when a flagged article holds the banner indefinitely, - which is the default; an admin sets a limit in Settings to opt in. + WindowHours is 0 (the default) when a flagged article holds the banner + indefinitely. type: integer type: object models.CategorySummary: diff --git a/server/internal/database/breaking_news_integration_test.go b/server/internal/database/breaking_news_integration_test.go index ad40acb..e6defed 100644 --- a/server/internal/database/breaking_news_integration_test.go +++ b/server/internal/database/breaking_news_integration_test.go @@ -3,6 +3,7 @@ package database import ( "context" "database/sql" + "fmt" "os" "testing" @@ -187,7 +188,10 @@ func TestBreakingNewsState_ScheduledArticleWaitsForItsPubDate(t *testing.T) { } } -func TestBreakingNewsState_NewestFlaggedArticleWins(t *testing.T) { +// The banner carries every flagged story, newest first, and Text still names +// the newest so a reader that only knows the old single-story fields shows the +// same headline it always would have. +func TestBreakingNewsState_CarriesEveryFlaggedArticleNewestFirst(t *testing.T) { conn := breakingNewsTestDB(t) ctx := context.Background() @@ -198,8 +202,63 @@ func TestBreakingNewsState_NewestFlaggedArticleWins(t *testing.T) { if err != nil { t.Fatalf("get state: %v", err) } - if state.Text != "Newer breaking story" { - t.Errorf("text = %q, want the newer story", state.Text) + if state.Text != "Newer breaking story" || state.ArticleSlug != "newer" { + t.Errorf("single-story fields = %q/%q, want the newer story", state.Text, state.ArticleSlug) + } + if len(state.Items) != 2 { + t.Fatalf("items = %+v, want both stories", state.Items) + } + if state.Items[0].Text != "Newer breaking story" || state.Items[0].ArticleSlug != "newer" { + t.Errorf("items[0] = %+v, want the newer story", state.Items[0]) + } + if state.Items[1].Text != "Older breaking story" || state.Items[1].ArticleSlug != "older" { + t.Errorf("items[1] = %+v, want the older story", state.Items[1]) + } +} + +// The banner scrolls, so a reader waits through everything ahead of the story +// they came for. The cap is what bounds that wait. +func TestBreakingNewsState_CapsTheNumberOfStories(t *testing.T) { + conn := breakingNewsTestDB(t) + ctx := context.Background() + + for i, slug := range []string{"first", "second", "third", "fourth"} { + insertArticle(t, conn, slug, "Story "+slug, true, + fmt.Sprintf("UTC_TIMESTAMP() - INTERVAL %d HOUR", 10-i), "NULL") + } + + state, err := GetBreakingNewsState(ctx, conn) + if err != nil { + t.Fatalf("get state: %v", err) + } + if len(state.Items) != maxBreakingNewsItems { + t.Fatalf("items = %d, want the cap of %d", len(state.Items), maxBreakingNewsItems) + } + // Newest first, so the one that falls off the end is the oldest. + if state.Items[0].ArticleSlug != "fourth" || state.Items[2].ArticleSlug != "second" { + t.Errorf("items = %+v, want the three newest stories", state.Items) + } +} + +// The manual banner is one story like any other, so a reader can treat Items as +// the whole banner rather than special-casing the hand-typed case. +func TestBreakingNewsState_ManualBannerIsAnItemToo(t *testing.T) { + conn := breakingNewsTestDB(t) + ctx := context.Background() + + if err := SetBreakingNews(ctx, conn, models.BreakingNewsSettings{Enabled: true, Text: "Campus closed"}, 0); err != nil { + t.Fatalf("set manual banner: %v", err) + } + + state, err := GetBreakingNewsState(ctx, conn) + if err != nil { + t.Fatalf("get state: %v", err) + } + if len(state.Items) != 1 || state.Items[0].Text != "Campus closed" { + t.Fatalf("items = %+v, want just the manual banner", state.Items) + } + if state.Items[0].ArticleSlug != "" { + t.Errorf("manual item carried a slug: %q", state.Items[0].ArticleSlug) } } diff --git a/server/internal/database/featured_article.go b/server/internal/database/featured_article.go index 77c4457..556e0be 100644 --- a/server/internal/database/featured_article.go +++ b/server/internal/database/featured_article.go @@ -7,55 +7,39 @@ import ( "server/internal/models" ) -// The featured article is the one an editor has pinned to the big lead card in -// the middle of the homepage. It is stored in the legacy `priority` column and -// is exclusive: exactly zero or one article carries it at a time, enforced on -// write by ClearFeaturedExcept. +// Featured articles are the ones an editor has pinned to the top of the +// homepage, stored in the legacy `priority` column. +// +// The flag used to be exclusive, cleared on every write so that one article +// carried it. It is not any more: a second breaking story should be able to go +// up without taking the first one down, which is what pinning by hand and then +// pinning again meant before. +// +// Nothing clears the flag on an editor's behalf, so a pin comes down when +// somebody unticks it. Order is by recency, so the newest pinned story leads +// and an editor who wants the other one first unpins the newer. const featuredArticleConditions = "WHERE `priority` = 1 AND `pub_date` IS NOT NULL AND `pub_date` <= UTC_TIMESTAMP() AND `archived_at` IS NULL" -// GetFeaturedArticle returns the featured article, or nil when nothing is -// featured. Unpublished, scheduled and archived rows are skipped so an article -// cannot reach the homepage through the flag alone: an editor who features a -// draft and forgets to publish it gets the normal newest-first lead, not a -// headline the public should not see yet. -// -// The ORDER BY is a defensive tiebreak: exclusivity is enforced on write, but a -// direct DB edit or an ETL reseed could leave two rows flagged, and the homepage -// must still resolve to one article rather than picking arbitrarily. -func GetFeaturedArticle(ctx context.Context, conn *sql.DB) (*models.Article, error) { - query := searchSelectColumns + featuredArticleConditions + " ORDER BY `pub_date` DESC, `id` DESC LIMIT 1" - rows, err := conn.QueryContext(ctx, query) - if err != nil { - return nil, err +// MaxFeaturedArticles caps how many pinned stories lead the homepage. Past +// this the news block is all pins and the homepage stops being a rundown. +const MaxFeaturedArticles = 3 + +// GetFeaturedArticles returns the pinned articles, newest first, at most limit +// of them. Unpublished, scheduled and archived rows are skipped so an article +// cannot reach the homepage through the flag alone: an editor who pins a draft +// and forgets to publish it gets the normal newest-first lead, not a headline +// the public should not see yet. +func GetFeaturedArticles(ctx context.Context, conn *sql.DB, limit int) ([]models.Article, error) { + if limit <= 0 { + return nil, nil } - defer rows.Close() - - articles, err := CollectArticles(rows) + query := searchSelectColumns + featuredArticleConditions + " ORDER BY `pub_date` DESC, `id` DESC LIMIT ?" + rows, err := conn.QueryContext(ctx, query, limit) if err != nil { return nil, err } - if len(articles) == 0 { - return nil, nil - } - return &articles[0], nil -} - -// ClearFeaturedExcept unfeatures every article other than the given slug. Call -// it after the write that features an article, never before: if the ordering -// were reversed and the main update then failed, the site would be left with no -// featured article at all instead of the one it had. -func ClearFeaturedExcept(ctx context.Context, conn *sql.DB, slug string) error { - _, err := conn.ExecContext(ctx, - "UPDATE `articles` SET `priority` = 0 WHERE `priority` = 1 AND `slug` <> ?", slug) - return err -} + defer rows.Close() -// ClearFeaturedExceptID is ClearFeaturedExcept for the create path, where the -// slug may have been generated from the title rather than supplied by the -// caller and the insert's id is the only handle on the new row. -func ClearFeaturedExceptID(ctx context.Context, conn *sql.DB, id int64) error { - _, err := conn.ExecContext(ctx, - "UPDATE `articles` SET `priority` = 0 WHERE `priority` = 1 AND `id` <> ?", id) - return err + return CollectArticles(rows) } diff --git a/server/internal/database/settings.go b/server/internal/database/settings.go index 8ad98b0..917cc03 100644 --- a/server/internal/database/settings.go +++ b/server/internal/database/settings.go @@ -35,6 +35,11 @@ const ( maxBreakingNewsWindowHours = 24 * 7 ) +// How many flagged articles the banner carries at once. The banner scrolls +// them, so the cap is about how long a reader waits for a given headline to +// come back around, not about width. +const maxBreakingNewsItems = 3 + // NormalizeBreakingNewsWindow clamps a window into the supported range. Zero // or negative is "no limit". func NormalizeBreakingNewsWindow(hours int) int { @@ -100,9 +105,15 @@ func GetBreakingNews(ctx context.Context, conn *sql.DB) (models.BreakingNewsSett return state.BreakingNewsSettings, nil } -// GetBreakingNewsState resolves the banner from both of its sources. A -// published article flagged breaking wins over the manual banner; the newest -// such article wins over the others. +// GetBreakingNewsState resolves the banner from both of its sources. Published +// articles flagged breaking win over the manual banner, newest first, up to +// maxBreakingNewsItems. +// +// Items carries all of them and is what the banner scrolls. Enabled/Text/ +// ArticleSlug describe the first, which is the newest: the public site read +// those three before the banner could carry more than one story, so they stay +// the single-story view of the same state rather than becoming a second copy +// of it. func GetBreakingNewsState(ctx context.Context, conn *sql.DB) (models.BreakingNewsState, error) { manual, err := getManualBreakingNews(ctx, conn) if err != nil { @@ -120,22 +131,26 @@ func GetBreakingNewsState(ctx context.Context, conn *sql.DB) (models.BreakingNew Manual: manual, WindowHours: window, } - if !manual.Enabled { + if manual.Enabled && manual.Text != "" { + state.Items = []models.BreakingNewsItem{{Text: manual.Text}} + state.Manual.Items = state.Items + } else { state.Source = models.BreakingNewsSourceNone } - slug, title, err := latestBreakingArticle(ctx, conn, window) + articles, err := breakingArticles(ctx, conn, window, maxBreakingNewsItems) if err != nil { return models.BreakingNewsState{}, err } - if title != "" { + if len(articles) > 0 { state.BreakingNewsSettings = models.BreakingNewsSettings{ Enabled: true, - Text: title, - ArticleSlug: slug, + Text: articles[0].Text, + ArticleSlug: articles[0].ArticleSlug, + Items: articles, } state.Source = models.BreakingNewsSourceArticle - state.ArticleTitle = title + state.ArticleTitle = articles[0].Text } return state, nil @@ -171,14 +186,14 @@ func getManualBreakingNews(ctx context.Context, conn *sql.DB) (models.BreakingNe }, nil } -// latestBreakingArticle returns the newest published article flagged breaking, -// or empty strings when there is none. The published predicate is the one -// every other public read uses, so a scheduled article starts driving the -// banner exactly when it becomes readable. +// breakingArticles returns the published articles flagged breaking, newest +// first, at most limit of them. The published predicate is the one every other +// public read uses, so a scheduled article starts driving the banner exactly +// when it becomes readable. // // windowHours of 0 is no limit, so the age clause is omitted rather than // passed: `INTERVAL 0 HOUR` would exclude everything. -func latestBreakingArticle(ctx context.Context, conn *sql.DB, windowHours int) (string, string, error) { +func breakingArticles(ctx context.Context, conn *sql.DB, windowHours, limit int) ([]models.BreakingNewsItem, error) { query := ` SELECT slug, title FROM articles @@ -192,17 +207,31 @@ func latestBreakingArticle(ctx context.Context, conn *sql.DB, windowHours int) ( query += " AND pub_date > UTC_TIMESTAMP() - INTERVAL ? HOUR\n" args = append(args, windowHours) } - query += "\tORDER BY pub_date DESC, id DESC\n\tLIMIT 1" + query += "\tORDER BY pub_date DESC, id DESC\n\tLIMIT ?" + args = append(args, limit) - var slug, title sql.NullString - err := conn.QueryRowContext(ctx, query, args...).Scan(&slug, &title) - if err == sql.ErrNoRows { - return "", "", nil - } + rows, err := conn.QueryContext(ctx, query, args...) if err != nil { - return "", "", err + return nil, err + } + defer rows.Close() + + var items []models.BreakingNewsItem + for rows.Next() { + var slug, title sql.NullString + if err := rows.Scan(&slug, &title); err != nil { + return nil, err + } + // A flagged article with no headline has nothing to put on the banner, + // and the banner is all headline. + if text := strings.TrimSpace(title.String); text != "" { + items = append(items, models.BreakingNewsItem{ + Text: text, + ArticleSlug: strings.TrimSpace(slug.String), + }) + } } - return strings.TrimSpace(slug.String), strings.TrimSpace(title.String), nil + return items, rows.Err() } // SetBreakingNews persists the manual breaking-news banner and its window. diff --git a/server/internal/handlers/breaking_news_settings_integration_test.go b/server/internal/handlers/breaking_news_settings_integration_test.go index 4a55d20..5954779 100644 --- a/server/internal/handlers/breaking_news_settings_integration_test.go +++ b/server/internal/handlers/breaking_news_settings_integration_test.go @@ -74,6 +74,16 @@ func breakingNewsSettingsTestDB(t *testing.T) *sql.DB { return conn } +func seedBreakingArticle(t *testing.T, conn *sql.DB, slug, title, pubExpr string) { + t.Helper() + if _, err := conn.ExecContext(context.Background(), + "INSERT INTO articles (slug, title, breaking_news, pub_date) VALUES (?, ?, 1, "+pubExpr+")", + slug, title, + ); err != nil { + t.Fatalf("seed %s: %v", slug, err) + } +} + func patchBreakingNews(t *testing.T, conn *sql.DB, body string) *httptest.ResponseRecorder { t.Helper() rec := httptest.NewRecorder() @@ -93,6 +103,41 @@ func decodeBreakingNews(t *testing.T, rec *httptest.ResponseRecorder) map[string return payload } +// The settings screen lists what is on the banner, so the response has to carry +// every flagged story, not just the newest. +func TestBreakingNewsSettingsHTTP_ListsEveryArticleOnTheBanner(t *testing.T) { + conn := breakingNewsSettingsTestDB(t) + + seedBreakingArticle(t, conn, "academy", "Academy story", "UTC_TIMESTAMP() - INTERVAL 3 HOUR") + seedBreakingArticle(t, conn, "dragonfly", "DragonFly story", "UTC_TIMESTAMP() - INTERVAL 1 HOUR") + + rec := httptest.NewRecorder() + GetBreakingNews(conn).ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/v1/settings/breaking-news", nil)) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body = %s", rec.Code, rec.Body.String()) + } + + var body struct { + Text string `json:"text"` + Items []struct { + Text string `json:"text"` + ArticleSlug string `json:"article_slug"` + } `json:"items"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("decode: %v", err) + } + if body.Text != "DragonFly story" { + t.Errorf("text = %q, want the newest story", body.Text) + } + if len(body.Items) != 2 { + t.Fatalf("items = %+v, want both stories", body.Items) + } + if body.Items[0].ArticleSlug != "dragonfly" || body.Items[1].ArticleSlug != "academy" { + t.Errorf("items = %+v, want dragonfly then academy", body.Items) + } +} + // window_hours is optional: a plain enable/disable must not reset it. func TestBreakingNewsSettingsHTTP_OmittedWindowIsPreserved(t *testing.T) { conn := breakingNewsSettingsTestDB(t) diff --git a/server/internal/handlers/featured_article_integration_test.go b/server/internal/handlers/featured_article_integration_test.go index 59dabc0..56c8e7f 100644 --- a/server/internal/handlers/featured_article_integration_test.go +++ b/server/internal/handlers/featured_article_integration_test.go @@ -42,10 +42,10 @@ func featuredSlugs(t *testing.T, conn *sql.DB) []string { return slugs } -// The homepage has one lead card. If featuring a second article left the first -// one flagged, the tiebreak in GetFeaturedArticle, not the editor, would be -// deciding which story runs. -func TestFeaturedArticleHTTP_PatchUnfeaturesThePreviousPick(t *testing.T) { +// Pinning used to be exclusive, so a second breaking story took the first one +// down. The newsroom case that changed it: a story goes out, something else +// breaks an hour later, and both belong at the top. +func TestFeaturedArticleHTTP_PinningASecondStoryKeepsTheFirst(t *testing.T) { conn := articlePatchTestDB(t) published := time.Now().UTC().Add(-24 * time.Hour).Format("2006-01-02 15:04:05") seedFeaturedTestArticle(t, conn, "old-lead", true, published, nil) @@ -61,8 +61,8 @@ func TestFeaturedArticleHTTP_PatchUnfeaturesThePreviousPick(t *testing.T) { t.Fatalf("patch status = %d, want 204; body = %s", rec.Code, rec.Body.String()) } slugs := featuredSlugs(t, conn) - if len(slugs) != 1 || slugs[0] != "new-lead" { - t.Fatalf("featured slugs = %v, want [new-lead]", slugs) + if len(slugs) != 2 || slugs[0] != "new-lead" || slugs[1] != "old-lead" { + t.Fatalf("featured slugs = %v, want both new-lead and old-lead", slugs) } } @@ -85,12 +85,12 @@ func TestFeaturedArticleHTTP_PatchCanClearTheFeaturedFlag(t *testing.T) { if slugs := featuredSlugs(t, conn); len(slugs) != 0 { t.Fatalf("featured slugs = %v, want none", slugs) } - featured, err := db.GetFeaturedArticle(context.Background(), conn) + featured, err := db.GetFeaturedArticles(context.Background(), conn, db.MaxFeaturedArticles) if err != nil { - t.Fatalf("get featured article: %v", err) + t.Fatalf("get featured articles: %v", err) } - if featured != nil { - t.Fatalf("featured article = %q, want none", featured.Slug) + if len(featured) != 0 { + t.Fatalf("featured articles = %v, want none", featured) } } @@ -106,21 +106,46 @@ func TestFeaturedArticle_UnpublishedRowsNeverLead(t *testing.T) { seedFeaturedTestArticle(t, conn, "featured-scheduled", true, future, nil) seedFeaturedTestArticle(t, conn, "featured-archived", true, past, past) - featured, err := db.GetFeaturedArticle(context.Background(), conn) + featured, err := db.GetFeaturedArticles(context.Background(), conn, db.MaxFeaturedArticles) if err != nil { - t.Fatalf("get featured article: %v", err) + t.Fatalf("get featured articles: %v", err) } - if featured != nil { - t.Fatalf("featured article = %q, want none", featured.Slug) + if len(featured) != 0 { + t.Fatalf("featured articles = %v, want none", featured) } seedFeaturedTestArticle(t, conn, "featured-live", true, past, nil) - featured, err = db.GetFeaturedArticle(context.Background(), conn) + featured, err = db.GetFeaturedArticles(context.Background(), conn, db.MaxFeaturedArticles) if err != nil { - t.Fatalf("get featured article: %v", err) + t.Fatalf("get featured articles: %v", err) } - if featured == nil || featured.Slug != "featured-live" { - t.Fatalf("featured article = %v, want featured-live", featured) + if len(featured) != 1 || featured[0].Slug != "featured-live" { + t.Fatalf("featured articles = %v, want just featured-live", featured) + } +} + +// Pins lead newest-first, and the cap is what stops the news block from being +// nothing but pins. +func TestFeaturedArticles_NewestFirstAndCapped(t *testing.T) { + conn := articlePatchTestDB(t) + base := time.Now().UTC().Add(-96 * time.Hour) + for i, slug := range []string{"pin-oldest", "pin-middle", "pin-newer", "pin-newest"} { + at := base.Add(time.Duration(i) * time.Hour).Format("2006-01-02 15:04:05") + seedFeaturedTestArticle(t, conn, slug, true, at, nil) + } + + featured, err := db.GetFeaturedArticles(context.Background(), conn, db.MaxFeaturedArticles) + if err != nil { + t.Fatalf("get featured articles: %v", err) + } + if len(featured) != db.MaxFeaturedArticles { + t.Fatalf("got %d pinned articles, want the cap of %d", len(featured), db.MaxFeaturedArticles) + } + want := []string{"pin-newest", "pin-newer", "pin-middle"} + for i, slug := range want { + if featured[i].Slug != slug { + t.Fatalf("pinned articles = %v, want %v", featured, want) + } } } @@ -196,4 +221,21 @@ func TestFeaturedArticleHTTP_HomepageLeadsWithTheFeaturedArticle(t *testing.T) { if len(slugs) == 0 || slugs[0] != "old-sports-story" { t.Fatalf("featured news block = %v, want it to lead with old-sports-story", slugs) } + + // Erik's case: a second story breaks an hour later and is pinned too. It + // takes the lead because it is newer, and the first one keeps the slot + // behind it rather than dropping back into the rundown. + rec = httptest.NewRecorder() + body = `{"title":"Newest news","excerpt":"","content":"Body","comment_status":"open",` + + `"photo_url":"","breaking_news":false,"is_featured":true,"categories":["News"],` + + `"authors":[],"focus_keyword":"","meta_description":"","seo_title":""}` + PatchArticle(conn).ServeHTTP(rec, patchArticleRequest("newest-news", body)) + if rec.Code != http.StatusNoContent { + t.Fatalf("patch status = %d, want 204; body = %s", rec.Code, rec.Body.String()) + } + + slugs = homepageNewsSlugs() + if len(slugs) < 2 || slugs[0] != "newest-news" || slugs[1] != "old-sports-story" { + t.Fatalf("news block = %v, want both pins leading with newest-news", slugs) + } } diff --git a/server/internal/handlers/featured_article_test.go b/server/internal/handlers/featured_article_test.go index 9e6b081..64bae14 100644 --- a/server/internal/handlers/featured_article_test.go +++ b/server/internal/handlers/featured_article_test.go @@ -34,58 +34,82 @@ func equalIDs(a, b []int64) bool { return true } -func TestSpliceFeaturedLead(t *testing.T) { +func TestSpliceFeaturedLeads(t *testing.T) { tests := []struct { name string news []int64 - featured int64 + featured []int64 limit int want []int64 }{ { - // A featured sports story is not in the news block at all, so the + // A pinned sports story is not in the news block at all, so the // block grows by one and the oldest card falls off the end. name: "article from another section takes the lead slot", news: []int64{5, 4, 3}, - featured: 99, + featured: []int64{99}, limit: 3, want: []int64{99, 5, 4}, }, { - // The layout's big centre card is news[0]; featuring a story - // already in the block must move it, not duplicate it. + // The layout's big centre card is news[0]; pinning a story already + // in the block must move it, not duplicate it. name: "news article is promoted rather than duplicated", news: []int64{5, 4, 3}, - featured: 3, + featured: []int64{3}, limit: 3, want: []int64{3, 5, 4}, }, { name: "already leading stays put", news: []int64{5, 4, 3}, - featured: 5, + featured: []int64{5}, limit: 3, want: []int64{5, 4, 3}, }, + { + // Two pins hold the first two slots in the order given, which is + // newest pin first. This is the case the second breaking story + // exists for. + name: "two pins lead in order", + news: []int64{5, 4, 3}, + featured: []int64{99, 3}, + limit: 3, + want: []int64{99, 3, 5}, + }, + { + name: "a pin already in the block does not cost a slot twice", + news: []int64{5, 4, 3}, + featured: []int64{4, 5}, + limit: 3, + want: []int64{4, 5, 3}, + }, { name: "short block is not padded or trimmed", news: []int64{5}, - featured: 99, + featured: []int64{99}, limit: 13, want: []int64{99, 5}, }, { - name: "empty news block still leads with the featured article", + name: "empty news block still leads with the pins", news: nil, - featured: 99, + featured: []int64{99, 98}, limit: 13, - want: []int64{99}, + want: []int64{99, 98}, + }, + { + name: "nothing pinned leaves the block alone", + news: []int64{5, 4, 3}, + featured: nil, + limit: 3, + want: []int64{5, 4, 3}, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - got := spliceFeaturedLead(newsBlock(tc.news...), models.ArticleListItem{ID: tc.featured}, tc.limit) + got := spliceFeaturedLeads(newsBlock(tc.news...), newsBlock(tc.featured...), tc.limit) if !equalIDs(newsIDs(got), tc.want) { t.Errorf("news order = %v, want %v", newsIDs(got), tc.want) } diff --git a/server/internal/handlers/handlers.go b/server/internal/handlers/handlers.go index 7721ca3..097eb81 100644 --- a/server/internal/handlers/handlers.go +++ b/server/internal/handlers/handlers.go @@ -2465,13 +2465,6 @@ func PostArticles(conn *sql.DB) http.HandlerFunc { writeError(w, http.StatusInternalServerError, err.Error()) return } - // See PatchArticle: featuring is exclusive. - if body.IsFeatured { - if err := db.ClearFeaturedExceptID(r.Context(), conn, articleID); err != nil { - writeError(w, http.StatusInternalServerError, err.Error()) - return - } - } if err := db.ReplaceArticleAuthors(r.Context(), conn, articleID, body.Authors); err != nil { writeError(w, http.StatusInternalServerError, err.Error()) return @@ -2568,20 +2561,6 @@ func PutArticle(conn *sql.DB) http.HandlerFunc { writeError(w, http.StatusNotFound, "article not found") return } - // See PatchArticle: featuring is exclusive, so the previous pick is - // cleared once this one is safely written. - if body.IsFeatured { - var err error - if target.hasID { - err = db.ClearFeaturedExceptID(r.Context(), conn, target.id) - } else { - err = db.ClearFeaturedExcept(r.Context(), conn, body.Slug) - } - if err != nil { - writeError(w, http.StatusInternalServerError, err.Error()) - return - } - } if target.hasID { err = db.ReplaceArticleAuthors(r.Context(), conn, target.id, authorIDsFromOverviews(body.Authors)) } else { @@ -2700,7 +2679,6 @@ func PatchArticle(conn *sql.DB) http.HandlerFunc { var publishedDateValue any var scheduledDateValue any renamedSlug := "" - featuring := false publishedDateSet := false var statusValue models.ArticleStatus statusSet := false @@ -2848,7 +2826,6 @@ func PatchArticle(conn *sql.DB) http.HandlerFunc { } setCols = append(setCols, column) setArgs = append(setArgs, b) - featuring = b case "slug": s, ok := v.(string) if !ok { @@ -2902,21 +2879,6 @@ func PatchArticle(conn *sql.DB) http.HandlerFunc { targetSlug = strings.TrimSpace(newSlug) } } - // Exactly one article is featured at a time: the homepage has one lead - // card, and leaving the old pick flagged would make the tiebreak, not - // the editor, decide which story runs. - if featuring { - var err error - if target.hasID { - err = db.ClearFeaturedExceptID(r.Context(), conn, target.id) - } else { - err = db.ClearFeaturedExcept(r.Context(), conn, targetSlug) - } - if err != nil { - writeError(w, http.StatusInternalServerError, err.Error()) - return - } - } if authorIDs != nil { if target.hasID { err = db.ReplaceArticleAuthors(r.Context(), conn, target.id, *authorIDs) @@ -3071,42 +3033,51 @@ func RestoreArticle(conn *sql.DB) http.HandlerFunc { } } -// leadWithFeaturedArticle moves the featured article to the front of the -// homepage news block. It is a no-op when nothing is featured, so the default -// homepage stays newest-first. +// leadWithFeaturedArticles moves the pinned articles to the front of the +// homepage news block, newest first. It is a no-op when nothing is pinned, so +// the default homepage stays newest-first. // -// The featured article keeps its place in its own section block: a featured -// sports story leads the homepage and still appears in the sports rundown, -// which is what a lead story does in print. Only the news block dedupes, so a -// featured news story is promoted rather than duplicated. -func leadWithFeaturedArticle(r *http.Request, conn *sql.DB, homepage *models.HomepageResponse, excerptWords, newsLimit int, newsMatchSlugs []string) error { - featured, err := db.GetFeaturedArticle(r.Context(), conn) - if err != nil || featured == nil { +// A pinned article keeps its place in its own section block: a pinned sports +// story leads the homepage and still appears in the sports rundown, which is +// what a lead story does in print. Only the news block dedupes, so a pinned +// news story is promoted rather than duplicated. +func leadWithFeaturedArticles(r *http.Request, conn *sql.DB, homepage *models.HomepageResponse, excerptWords, newsLimit int, newsMatchSlugs []string) error { + featured, err := db.GetFeaturedArticles(r.Context(), conn, db.MaxFeaturedArticles) + if err != nil || len(featured) == 0 { return err } - articles := []models.Article{*featured} - if err := db.PopulateArticleAuthors(r.Context(), conn, articles); err != nil { + if err := db.PopulateArticleAuthors(r.Context(), conn, featured); err != nil { return err } - items := articleListItems(articles, excerptWords, newsMatchSlugs...) + items := articleListItems(featured, excerptWords, newsMatchSlugs...) if len(items) == 0 { return nil } - homepage.News = spliceFeaturedLead(homepage.News, items[0], newsLimit) + homepage.News = spliceFeaturedLeads(homepage.News, items, newsLimit) return nil } -// spliceFeaturedLead puts the featured article at the head of the news block, -// dropping the copy already in the list so a featured news story is promoted -// rather than printed twice. The list is re-trimmed to limit because splicing in -// a story from another section would otherwise push the block one card past the -// layout it was sized for. -func spliceFeaturedLead(news []models.ArticleListItem, featured models.ArticleListItem, limit int) []models.ArticleListItem { - out := make([]models.ArticleListItem, 0, len(news)+1) - out = append(out, featured) +// spliceFeaturedLeads puts the pinned articles at the head of the news block in +// the order given, dropping the copies already in the list so a pinned news +// story is promoted rather than printed twice. The list is re-trimmed to limit +// because splicing in stories from other sections would otherwise push the +// block past the layout it was sized for. +func spliceFeaturedLeads(news []models.ArticleListItem, featured []models.ArticleListItem, limit int) []models.ArticleListItem { + out := make([]models.ArticleListItem, 0, len(news)+len(featured)) + pinned := make(map[int64]bool, len(featured)) + for _, item := range featured { + // A pin is only ever meant to move a story up. Two pins on one id + // cannot happen through the API, but a duplicate here would print the + // same card twice, which is worse than dropping one. + if pinned[item.ID] { + continue + } + pinned[item.ID] = true + out = append(out, item) + } for _, item := range news { - if item.ID != featured.ID { + if !pinned[item.ID] { out = append(out, item) } } @@ -3238,13 +3209,13 @@ func GetHomepage(conn *sql.DB) http.HandlerFunc { } // The homepage lead is the first entry of the news block (Scalene's - // "3-6-3" layout renders news[0] as the big centre card), so featuring - // an article means moving it to the front of that list. It is spliced in - // rather than sorted into the news query because the featured article - // may be filed under any section: a featured sports story still takes - // the lead card, which a news-scoped ORDER BY could never do. + // "3-6-3" layout renders news[0] as the big centre card), so pinning an + // article means moving it to the front of that list. Pins are spliced in + // rather than sorted into the news query because a pinned article may be + // filed under any section: a pinned sports story still takes the lead + // card, which a news-scoped ORDER BY could never do. if offset == 0 { - if err := leadWithFeaturedArticle(r, conn, §ionArticles, excerptWords, newsLimit, newsMatchSlugs); err != nil { + if err := leadWithFeaturedArticles(r, conn, §ionArticles, excerptWords, newsLimit, newsMatchSlugs); err != nil { writeError(w, http.StatusInternalServerError, err.Error()) return } diff --git a/server/internal/models/api_responses.go b/server/internal/models/api_responses.go index 3e45c5a..aa7894f 100644 --- a/server/internal/models/api_responses.go +++ b/server/internal/models/api_responses.go @@ -301,16 +301,29 @@ type SiteSettingsPatchRequest struct { SiteTitle string `json:"site_title"` } -// BreakingNewsSettings controls the breaking-news banner shown on the public -// homepage: whether it is visible, the text it displays, and the article it -// links to. ArticleSlug is the slug alone, not a path, since the public site -// owns its URL shape; it is empty for a hand-typed banner. -type BreakingNewsSettings struct { - Enabled bool `json:"enabled"` +// BreakingNewsItem is one story on the breaking-news banner. ArticleSlug is +// the slug alone, not a path, since the public site owns its URL shape; it is +// empty for a hand-typed banner, which links nowhere. +type BreakingNewsItem struct { Text string `json:"text"` ArticleSlug string `json:"article_slug,omitempty"` } +// BreakingNewsSettings controls the breaking-news banner shown on the public +// homepage: whether it is visible and the stories it scrolls. +// +// Items is the banner. Text and ArticleSlug describe Items[0], the newest +// story, and exist because the public site read them before the banner could +// carry more than one: they are the single-story view of the same state, not a +// second copy of it, so a reader that never learns about Items keeps working +// and shows the newest breaking story. +type BreakingNewsSettings struct { + Enabled bool `json:"enabled"` + Text string `json:"text"` + ArticleSlug string `json:"article_slug,omitempty"` + Items []BreakingNewsItem `json:"items,omitempty"` +} + // Sources a banner can come from; an article wins over the manual banner. const ( BreakingNewsSourceNone = "none"