Reader: order articles within a group newest-first #26
marvin8
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/issue-19-aggregate-article-ordering"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
get_articles_in_aggregatepreviously returned articles in insertion order (ORDER BY article_id), so the newest article in a group could sit anywhere in the reader view.The query now joins to
articlesand orders byCOALESCE(published_at, created_at) DESC, created_at DESC— newest publication first, articles without a publish date fall back to fetch time, ties break on fetch time. This matches the ordering precedent already used by the other article-list queries.Side effect (desirable): the reader index featured image now comes from the newest article in the group instead of the first-inserted one.
Design note recorded on the issue (corrects the findings comment:
articleshas noupdated_atcolumn). Covered by a new ordering test with deliberately out-of-order insertion and a missing publish date.Closes #19
WuMing
Found 3 issue(s). See inline comments below.
@ -212,0 +217,4 @@FROM article_aggregates AS aaJOIN articles AS a ON a.id = aa.article_idWHERE aa.aggregate_id = ?ORDER BY COALESCE(a.published_at, a.created_at) DESC, a.created_at DESCcode [LOW]
The ORDER BY clause has no final deterministic tie-breaker. If multiple articles have the same COALESCE(published_at, created_at) and created_at values (e.g., identical timestamps), the result order is undefined and may vary between executions. Add a stable tie-breaker such as aa.article_id DESC (or ASC) to ensure deterministic output.
@ -336,0 +366,4 @@article_ids.append(article.id)return aggregate, article_idsdef test_get_articles_in_aggregate_orders_newest_first(self, db_conn):code [LOW]
The new ordering test only covers one article with a missing publish date and distinct published_at values. It does not cover tie-breaking behavior when two or more articles share the same published_at (and rely on created_at), nor multiple NULL published_at articles. This leaves the ORDER BY's second sort key and any future tie-breaker unverified.
@ -336,0 +380,4 @@result = aggregates.get_articles_in_aggregate(db_conn, aggregate.id)assert result == [insertion_ids[2], insertion_ids[1], insertion_ids[0]]tests [MEDIUM]
The test data makes the expected order [insertion_ids[2], insertion_ids[1], insertion_ids[0]] identical to what a query ordering by created_at alone (or even by article_id DESC) would return: article 2 is fetched last (newest created_at, published_at NULL), article 1 is published 1 day ago, article 0 3 days ago, so published_at order, created_at order and reverse insertion order all coincide. As a result the published_at-primary part of
ORDER BY COALESCE(a.published_at, a.created_at) DESC, a.created_at DESCis never actually discriminated — the assertion would still pass if the query dropped published_at from the sort entirely. Add an article whose publish date is newer than the previously inserted ones but which is inserted first (or an article with an old published_at fetched most recently), so the published_at ordering conflicts with created_at/insertion ordering and only the correct query produces the expected list.src/yunjin/db/aggregates.py✅ Fixed —
aa.article_id DESCappended as the final sort key, with a dedicated test that forces identicalpublished_at/created_aton two articles and asserts the stable descending-id order.tests/test_db.py✅ Fixed — the dataset now includes two dateless articles (verifying the created_at fallback order between them), a same-
published_atpair (verifying the created_at tie-break), and an old-published/newly-fetched article; the tie-breaker test covers the exact-tie case.tests/test_db.py✅ Fixed — the rewritten dataset is constructed so the expected order differs from both alternatives: the last-inserted article has the oldest publish date, so only publication-based ordering places it last (plain created_at or reverse-insertion would place it first).