Configurable ignore rules for unwanted content #31
marvin8
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/issue-20-ignore-rules"
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?
Adds display-time ignore rules: one keyword per line, stored in the settings table. A reader group is hidden when any rule matches (case-insensitive substring) a member article title or one of the groups tags. Aggregation is untouched, so rules are reversible at any time.
services/ignore_rules.py:normalize_rules+matches_any_rule(pure, unit-tested)/settings/ignore-rulesgnext-unread navigation walks past them tooDesign decisions (filter point, rule model) per the issue discussion.
Closes #20
WuMing
Found 8 issue(s). See inline comments below.
@ -95,0 +104,4 @@Cleaned ignore rules (empty when none are configured)."""raw_rules = settings_db.get_setting(db, user_id=1, key="ignore_rules") or "" # user_id=1 until #25code [LOW]
_load_ignore_rules hardcodes user_id=1 while the reader routes already accept/use a user_id. This silently loads the wrong user's rules when multi-user support lands (#25). Pass user_id into _load_ignore_rules and use it here and at the call site in the aggregate route.
security [MEDIUM]
A01: Ignore rules are loaded for hard-coded user_id=1 rather than the currently authenticated user. This causes all users to share user 1's ignore rules and applies one user's settings to everyone. Use the authenticated user ID and validate ownership when reading settings.
@ -95,0 +127,4 @@return Falsetexts: list[str] = []for article_id in agg_db.get_articles_in_aggregate(db, aggregate_id):code [LOW]
The index filtering logic calls _aggregate_is_ignored for every aggregate, and this method then performs per-article and per-tag DB queries. With many aggregates this is an N+1 pattern and can make the reader index slow. Consider loading article titles/tags for all aggregates in bulk or caching per request.
@ -195,3 +276,3 @@summary_info = summarizer.extract_summary(db, article_ids)next_unread_id = agg_db.get_next_aggregate_with_unread(db, user_id, aggregate_id)next_unread_id = _find_next_unread_id(db, user_id, aggregate_id, _load_ignore_rules(db))code [MEDIUM]
The aggregate detail route renders the summary and article content without checking _aggregate_is_ignored. A user can open /aggregate/ directly and bypass the ignore rules. Add an ignore check before rendering or redirect to the reader index if ignored groups must be hidden everywhere.
@ -141,0 +150,4 @@"""db = get_db()user_id = 1 # Default user for now (Phase 6 will add multi-user, see issue #25)security [MEDIUM]
A01: The /settings/ignore-rules POST handler hard-codes user_id=1 instead of using the authenticated user and performs no authorization check. Any request can overwrite user 1's ignore rules, an insecure direct object reference. Use the current authenticated user ID from the session and enforce ownership.
@ -583,0 +588,4 @@response = client.get("/settings/tags")assert response.status_code == 200assert b'name="ignore_rules"' in response.datacode [HIGH]
This test expects an ignore_rules textarea on /settings/tags, but the diff contains no settings.html changes adding that field. Without the template update the feature is not exposed and this test will fail. Ensure settings.html is modified in the PR.
@ -583,0 +695,4 @@response = client.get(f"/aggregate/{visible_agg.id}")assert f'/aggregate/{ignored_agg.id}"'.encode() not in response.datacode [LOW]
This test only asserts the ignored aggregate URL is absent from the visible aggregate page; it does not force ignored_agg to be the next unread candidate (timestamps are both datetime.now(), ordering is ambiguous). The skip logic may not be exercised. Use distinct published_at values or pre-seed read state/order so ignored_agg is definitely the next unread group.
tests [MEDIUM]
This test only asserts the ignored aggregate's URL is absent, which also passes if next-unread navigation renders no link at all (e.g.
_find_next_unread_idalways returns None). Add a positive assertion that the visible aggregate's /aggregate/ link (or a next-unread URL) is present, so the test actually verifies that navigation skipped the ignored group rather than yielding nothing.✅ Fixed — next-unread skip test strengthened (and it caught a real bug)
tests/test_web.py·tests/test_web.pyThe test now uses three groups with ids ordered so the naive unread query deterministically returns the ignored group first, and asserts both that the visible group is offered and the ignored one never is. Writing it exposed a genuine bug: after skipping a candidate, the walk re-queried with only the skipped id excluded, so navigation could point back at the group being read. The unread query now has a list variant (
get_aggregates_with_unread) and the walk picks the first non-ignored candidate from it. Fixed in commitb8043b5.🔴 Not actioned — hard-coded
user_id = 1/ missing auth + CSRFsrc/yunjin/web/routes/reader.py·src/yunjin/web/routes/reader.py·src/yunjin/web/routes/settings.pyDeliberate, project-level, and identical across every route in this codebase (each site carries a
# user_id=1 until #25comment). The auth/session/CSRF layer is tracked in issue #25 — the same findings were declined on PR #24 and consolidated there. Re-rolling it per-route would not change the outcome.🔴 Not actioned — direct URL access to an ignored group is intentional
src/yunjin/web/routes/reader.pyIgnore rules are display-time curation, not access control:
gnavigation never lands on an ignored group, and opening its URL directly is explicit intent to read it. This is the documented design (see the issue's plan-of-attack comment: "direct URLs to ignored groups still work").🔴 Not actioned — N+1 in rule matching matches the route's existing shape
src/yunjin/web/routes/reader.pyThe same route already loads articles, media, tags, and read state per aggregate — the reader is N+1 by design at personal scale. Rule checks only add queries when rules are configured. Bulk loading is a route-wide refactor better done as perf work than smuggled into this feature.
🔴 Not actioned — settings.html is in this diff
tests/test_web.pyFactually incorrect: commit
826371aadds the Ignore Rules card tosettings.html(+24 lines), and this PR's CI run (including the full pytest suite, which contains the test in question) is green.WuMing
Found 4 issue(s). See inline comments below.
@ -309,3 +334,1 @@(user_id, exclude_aggregate_id),)row = cursor.fetchone()aggregate_ids = get_aggregates_with_unread(conn, user_id, exclude_aggregate_id)code [MEDIUM]
get_next_aggregate_with_unread now calls get_aggregates_with_unread, which fetches every unread aggregate ID, then takes the first element. This removes the previous LIMIT 1 and can materialize a huge list unnecessarily. Use a LIMIT 1 query or add a limit parameter to the helper.
@ -0,0 +50,4 @@"""folded_rules = [rule.casefold() for rule in rules]return any(folded_rule in text.casefold() for folded_rule in folded_rules for text in texts)code [LOW]
matches_any_rule does not filter out empty rules. An empty rule ("") is an empty substring and therefore matches every text, so matches_any_rule([""], ["anything"]) returns True. Since this is a public pure helper, guard against empty rules (or document/handle them explicitly) to prevent accidental hide-everything behavior.
@ -95,0 +104,4 @@Cleaned ignore rules (empty when none are configured)."""raw_rules = settings_db.get_setting(db, user_id=1, key="ignore_rules") or "" # user_id=1 until #25security [MEDIUM]
A01: Ignore rules are loaded with hard-coded user_id=1 rather than the requesting user, so the reader applies user 1's rules regardless of identity. This breaks per-user access control / user isolation when authentication is present. Derive user_id from the authenticated session/request context.
@ -95,0 +128,4 @@texts: list[str] = []for article_id in agg_db.get_articles_in_aggregate(db, aggregate_id):article = articles_db.get_article(db, article_id)code [MEDIUM]
_aggregate_is_ignored runs get_article and get_tags_for_article for every article in every aggregate while rendering the index and for each next-unread candidate. With many aggregates/articles this causes a large N+1 query load and can make the reader slow. Batch-fetch article titles/tags per aggregate (or cache per request) instead.
✅ Fixed — empty rules can no longer match everything
src/yunjin/services/ignore_rules.pyValid catch.
matches_any_rulenow skips empty rules (documented in its docstring), with a regression test. Fixed in commit23e5770.✅ Resolved — the LIMIT-less single-result helper is gone
src/yunjin/db/aggregates.pyAfter the previous fix the single-result helper had no production callers left (the walk needs the full ordered list), so instead of duplicating the ordering SQL to restore
LIMIT 1, the helper is removed outright and its test folded into the list variant's tests. Fixed in commitaad5458.Duplicates of wave-1 findings (already answered)
src/yunjin/web/routes/reader.py·src/yunjin/web/routes/reader.pyIdentical in substance to the N+1 and hard-coded
user_id = 1findings answered in the consolidated review reply above — both declined there with rationale (route-wide N+1 is the existing design; auth/CSRF is tracked project-level in issue #25). No further action.