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"