diff --git a/frontend/src/pages/articleView.tsx b/frontend/src/pages/articleView.tsx index cd541ea..71e36ed 100644 --- a/frontend/src/pages/articleView.tsx +++ b/frontend/src/pages/articleView.tsx @@ -99,12 +99,6 @@ type ArticleViewProps = { // means send no status filter at all. type PublishedFilter = "all" | "published" | "draft" | "scheduled" -// A flag filter is three-state: "all" sends no parameter at all, the other two -// send breaking/featured explicitly. "Not breaking" has to be a real choice -// rather than the absence of one -- an editor clearing a false alarm wants the -// stories that are NOT flagged. -type FlagFilter = "all" | "on" | "off" - type ArticleViewUIState = { searchQuery?: string activeTab?: "all" | "trash" @@ -112,14 +106,15 @@ type ArticleViewUIState = { sectionFilterSlug?: string subsectionFilterSlug?: string publishedFilter?: PublishedFilter - breakingFilter?: FlagFilter - featuredFilter?: FlagFilter + // Each narrows to the flagged articles when on and does not filter at all + // when off, so neither ever asks for the unflagged half. The endpoint still + // accepts breaking=false/featured=false; nothing here sends it. + breakingOnly?: boolean + featuredOnly?: boolean dateSortDirection?: "asc" | "desc" pageSize?: number } -const flagFilterParam = (filter: FlagFilter) => (filter === "on" ? "true" : "false") - type ArticleResultsCacheEntry = { items: ArticleItem[] totalArticleCount: number @@ -141,6 +136,48 @@ const writeSessionJSON = (key: string, value: unknown) => { window.sessionStorage.setItem(key, JSON.stringify(value)) } +// A switch rather than a checkbox because these read as "on/off", not as items +// ticked off a list. Hand-rolled: there is no switch in components/ui and no +// @radix-ui/react-switch in the tree, and pulling one in for two toggles would +// be more code than this. +// +// role="switch" with aria-checked is what makes it announce as a switch; the +// track and knob are presentational, so they are spans inside the button rather +// than focusable elements of their own. +function FilterSwitch({ + checked, + label, + onChange, +}: { + checked: boolean + label: string + onChange: (next: boolean) => void +}) { + return ( + + ) +} + function ArticleView({ pageTitle = "Articles", fixedType, excludeType }: ArticleViewProps) { const navigate = useNavigate() const apiFetch = useApiFetch() @@ -166,8 +203,8 @@ function ArticleView({ pageTitle = "Articles", fixedType, excludeType }: Article const [subsections, setSubsections] = useState([]) const [subsectionFilterSlug, setSubsectionFilterSlug] = useState(() => loadUIState().subsectionFilterSlug ?? "") const [publishedFilter, setPublishedFilter] = useState(() => loadUIState().publishedFilter ?? "all") - const [breakingFilter, setBreakingFilter] = useState(() => loadUIState().breakingFilter ?? "all") - const [featuredFilter, setFeaturedFilter] = useState(() => loadUIState().featuredFilter ?? "all") + const [breakingOnly, setBreakingOnly] = useState(() => loadUIState().breakingOnly ?? false) + const [featuredOnly, setFeaturedOnly] = useState(() => loadUIState().featuredOnly ?? false) const [dateSortDirection, setDateSortDirection] = useState<"asc" | "desc">(() => loadUIState().dateSortDirection ?? "desc") const [isLoading, setIsLoading] = useState(true) const [error, setError] = useState(null) @@ -183,17 +220,17 @@ function ArticleView({ pageTitle = "Articles", fixedType, excludeType }: Article sectionFilterSlug, subsectionFilterSlug, publishedFilter, - breakingFilter, - featuredFilter, + breakingOnly, + featuredOnly, dateSortDirection, pageSize, } satisfies ArticleViewUIState) }, [ activeTab, authorQuery, - breakingFilter, + breakingOnly, dateSortDirection, - featuredFilter, + featuredOnly, pageSize, publishedFilter, searchQuery, @@ -408,11 +445,11 @@ function ArticleView({ pageTitle = "Articles", fixedType, excludeType }: Article if (subsectionFilterSlug) { params.set("subsection_slug", subsectionFilterSlug) } - if (breakingFilter !== "all") { - params.set("breaking", flagFilterParam(breakingFilter)) + if (breakingOnly) { + params.set("breaking", "true") } - if (featuredFilter !== "all") { - params.set("featured", flagFilterParam(featuredFilter)) + if (featuredOnly) { + params.set("featured", "true") } if (fixedType) { params.set("type", fixedType) @@ -494,7 +531,7 @@ function ArticleView({ pageTitle = "Articles", fixedType, excludeType }: Article return () => { cancelled = true } - }, [activeTab, apiFetch, authorQuery, breakingFilter, dateSortDirection, excludeType, featuredFilter, fixedType, page, pageSize, publishedFilter, resultsCacheKey, searchQuery, sectionFilterSlug, selectedAuthorSlug, subsectionFilterSlug]) + }, [activeTab, apiFetch, authorQuery, breakingOnly, dateSortDirection, excludeType, featuredOnly, fixedType, page, pageSize, publishedFilter, resultsCacheKey, searchQuery, sectionFilterSlug, selectedAuthorSlug, subsectionFilterSlug]) const onChangeTab = (tab: "all" | "trash") => { setActiveTab(tab) @@ -508,7 +545,7 @@ function ArticleView({ pageTitle = "Articles", fixedType, excludeType }: Article useEffect(() => { setPage(0) - }, [authorQuery, publishedFilter, breakingFilter, featuredFilter, dateSortDirection, searchQuery, sectionFilterSlug, subsectionFilterSlug, pageSize]) + }, [authorQuery, publishedFilter, breakingOnly, featuredOnly, dateSortDirection, searchQuery, sectionFilterSlug, subsectionFilterSlug, pageSize]) const effectiveTotalCount = Math.max(totalArticleCount, (page * pageSize) + articles.length) const totalPages = Math.max(1, Math.ceil(effectiveTotalCount / pageSize)) @@ -779,23 +816,14 @@ function ArticleView({ pageTitle = "Articles", fixedType, excludeType }: Article {/* Breaking and Featured are the two flags an editor sets from the article form, and the two they need to audit: which stories are - still flagged as breaking, and what is currently pinned. Both are - three-state, so "not flagged" is reachable rather than implied. */} + still flagged as breaking, and what is currently pinned. Each one + narrows to its flag or does nothing, so it is a switch rather than a + row of chips -- there is no third thing to pick. */}
- Breaking -
- - - -
-
- -
- Featured -
- - - + Flags +
+ +
diff --git a/server/docs/docs.go b/server/docs/docs.go index 0d85cf0..b0d92c2 100644 --- a/server/docs/docs.go +++ b/server/docs/docs.go @@ -144,13 +144,13 @@ const docTemplate = `{ }, { "type": "boolean", - "description": "When true, return only articles flagged as breaking news; when false, only those not flagged.", + "description": "When true, return only articles flagged as breaking news. Any other value does not filter.", "name": "breaking", "in": "query" }, { "type": "boolean", - "description": "When true, return only featured (pinned) articles; when false, only those not featured.", + "description": "When true, return only featured (pinned) articles. Any other value does not filter.", "name": "featured", "in": "query" }, diff --git a/server/docs/swagger.json b/server/docs/swagger.json index f835a1c..cfa2147 100644 --- a/server/docs/swagger.json +++ b/server/docs/swagger.json @@ -141,13 +141,13 @@ }, { "type": "boolean", - "description": "When true, return only articles flagged as breaking news; when false, only those not flagged.", + "description": "When true, return only articles flagged as breaking news. Any other value does not filter.", "name": "breaking", "in": "query" }, { "type": "boolean", - "description": "When true, return only featured (pinned) articles; when false, only those not featured.", + "description": "When true, return only featured (pinned) articles. Any other value does not filter.", "name": "featured", "in": "query" }, diff --git a/server/docs/swagger.yaml b/server/docs/swagger.yaml index 42fb8a2..52310bc 100644 --- a/server/docs/swagger.yaml +++ b/server/docs/swagger.yaml @@ -1362,13 +1362,13 @@ paths: in: query name: slug type: string - - description: When true, return only articles flagged as breaking news; when - false, only those not flagged. + - description: When true, return only articles flagged as breaking news. Any + other value does not filter. in: query name: breaking type: boolean - - description: When true, return only featured (pinned) articles; when false, - only those not featured. + - description: When true, return only featured (pinned) articles. Any other + value does not filter. in: query name: featured type: boolean diff --git a/server/internal/handlers/article_flag_filters_integration_test.go b/server/internal/handlers/article_flag_filters_integration_test.go index ec85a2e..6a1b999 100644 --- a/server/internal/handlers/article_flag_filters_integration_test.go +++ b/server/internal/handlers/article_flag_filters_integration_test.go @@ -12,14 +12,14 @@ import ( db "server/internal/database" ) -// The Articles screen offers Breaking and Featured as filters, so both have to +// The Articles screen offers Breaking and Featured as switches, so both have to // narrow the listing on their own and in combination with the section filter // the editor may already have set. // -// The awkward part is the "off" half. Both columns are nullable and the -// WordPress archive leaves them NULL rather than 0, so `breaking_news` = 0 -// matches none of it; a filter written that way would answer "not breaking" -// with only the handful of rows the CMS itself has written. +// The seed keeps a row with both flags NULL, which is the shape the WordPress +// archive is in. It must stay out of every "on" result: `= 1` is false for +// NULL, and a truthiness test written some other way could let nine thousand +// imported rows into a list of breaking stories. func seedFlagFilterArticles(t *testing.T, conn *sql.DB) { t.Helper() ctx := context.Background() @@ -89,21 +89,20 @@ func TestArticleFlagFiltersHTTP(t *testing.T) { query: "?breaking=true", want: []string{"breaking-campus-story", "breaking-sports-story"}, }, - { - // The NULL row has to be here, or "not breaking" hides the archive. - name: "not breaking", - query: "?breaking=false", - want: []string{"featured-campus-story", "imported-campus-story", "ordinary-campus-story"}, - }, { name: "featured only", query: "?featured=true", want: []string{"breaking-sports-story", "featured-campus-story"}, }, { - name: "not featured", - query: "?featured=false", - want: []string{"breaking-campus-story", "imported-campus-story", "ordinary-campus-story"}, + // A switch cannot ask for the unflagged half, so false is inert. The + // failure this pins is it being read as "not breaking" instead. + name: "false does not filter", + query: "?breaking=false", + want: []string{ + "breaking-campus-story", "breaking-sports-story", "featured-campus-story", + "imported-campus-story", "ordinary-campus-story", + }, }, { // Both at once: the article that is breaking AND pinned. diff --git a/server/internal/handlers/handlers.go b/server/internal/handlers/handlers.go index 9de4367..322ca43 100644 --- a/server/internal/handlers/handlers.go +++ b/server/internal/handlers/handlers.go @@ -1260,8 +1260,8 @@ func GetAuthorArticles(conn *sql.DB) http.HandlerFunc { // @Param archived query bool false "When true, return only soft-deleted articles. Ignored for unauthenticated callers." // @Param title query string false "Filter by title (partial match)" // @Param slug query string false "Filter by slug (partial match)" -// @Param breaking query bool false "When true, return only articles flagged as breaking news; when false, only those not flagged." -// @Param featured query bool false "When true, return only featured (pinned) articles; when false, only those not featured." +// @Param breaking query bool false "When true, return only articles flagged as breaking news. Any other value does not filter." +// @Param featured query bool false "When true, return only featured (pinned) articles. Any other value does not filter." // @Param sort_by query string false "Sort field" Enums(title,slug,creation_date,published_date,status,comment_status) // @Param sort_direction query string false "Sort direction" Enums(asc,desc) // @Success 200 {object} models.ArticlesResponse @@ -1708,49 +1708,36 @@ func articleQueryFilters(r *http.Request, params ArticleParams) ([]string, []any args = append(args, "%"+slug+"%") } - // Both columns are nullable and default to 0, and rows imported from - // WordPress leave them NULL rather than 0. COALESCE on the "off" side is - // what keeps ?breaking=false from hiding the whole archive, since - // `breaking_news` = 0 is never true for NULL. - if breaking, ok := parseBoolFilter(q, "breaking"); ok { - if breaking { - conditions = append(conditions, "`breaking_news` = 1") - } else { - conditions = append(conditions, "COALESCE(`breaking_news`, 0) = 0") - } + // Both flags narrow to the articles that carry them. `priority` is the + // featured/pinned column; is_featured is the name it goes by over the wire, + // and GetFeaturedArticles reads the same one. + if flagFilterOn(q, "breaking") { + conditions = append(conditions, "`breaking_news` = 1") } - - // `priority` is the featured/pinned flag; is_featured is the name it goes by - // over the wire. GetFeaturedArticles reads the same column. - if featured, ok := parseBoolFilter(q, "featured"); ok { - if featured { - conditions = append(conditions, "`priority` = 1") - } else { - conditions = append(conditions, "COALESCE(`priority`, 0) = 0") - } + if flagFilterOn(q, "featured") { + conditions = append(conditions, "`priority` = 1") } return conditions, args } -// parseBoolFilter reads an optional boolean query param. The second result says -// whether the caller asked at all, so an absent param and an explicit false stay -// distinguishable: absent means "do not filter", false means "only the ones that -// are off". A value that parses as neither is treated as absent rather than -// guessed at, so a typo widens the listing instead of silently narrowing it to -// the wrong half. -func parseBoolFilter(q url.Values, key string) (bool, bool) { +// flagFilterOn reports whether a flag filter is switched on. Absent, false and +// unparseable all mean "do not filter", which is the whole of the contract: the +// parameter is a switch, and a switch cannot ask for the unflagged half. +// +// `= 1` rather than a truthiness test because both columns are nullable, and +// NULL is what the WordPress archive holds -- it must read as unflagged, not as +// unknown. +func flagFilterOn(q url.Values, key string) bool { if _, provided := q[key]; !provided { - return false, false + return false } switch strings.ToLower(strings.TrimSpace(q.Get(key))) { + // Bare ?breaking is the HTML-form spelling of true. case "", "1", "true", "yes": - // Bare ?breaking is the HTML-form spelling of true. - return true, true - case "0", "false", "no": - return false, true + return true default: - return false, false + return false } }