Re-review complete. The prior blocker around HTMX error-path handling is now fixed (defensive coercion + safe fallback), and the additional regression test coverage is in place. I don’t see remaining blocking issues in this round. LGTM.
Re-review result: requesting changes.
Re-review result: requesting changes.
Follow-up with precise blocking findings (to avoid formatting loss):
- Approved replies are inserted as top-level comments in HTMX flow.
- In apps/comments/views.py lines 124-129, approved HTMX…
Requesting changes — I found several blocking correctness issues:
Great direction overall. This is a solid v2 target, and I think it solves real MVP pain.
Main risks / gotchas I’d address before implementation:
- Auto-approval logic and fallback
- I would…
Strict QA review complete: implementation matches Issue #41 scope and acceptance criteria. Verified search backend config, article-only query path, empty/whitespace/max-length guards, navigation + article index search UX updates, pagination query preservation, and deploy reindex hook. Full pytest suite passes in Docker (96.03% coverage) and update_index runs cleanly.
Correcting formatting from my previous comment (the CLI ate inline code formatting). Reposting cleanly:
Thanks for writing this up — the overall direction (Wagtail DB backend on Postgres, no…
Thanks for writing this up — the overall direction (Wagtail DB backend on Postgres, no extra search infra) looks right for this project size. I’d support this with a few important adjustments to…
Re-review complete. Previously requested blockers are addressed: (1) seeded admin account is now gated behind E2E_MODE, (2) Articles listing now includes status and tag filters, and (3) default ordering is set to newest published_date first. I also confirmed summary is included in ArticlePage search_fields. Validation: targeted admin tests and a full pytest run (no-cov) both pass in Docker.
Requesting changes due to blocking issues:\n\n1) Security regression in seed command: now creates a predictable superuser ( / ) unconditionally. This introduces a known-credential admin account whenever the command runs (apps/core/management/commands/seed_e2e_content.py:203-209). Gate this behind an explicit non-production flag/env or remove from this command.\n\n2) Issue #39 completeness gap in Articles listing: required filters/search are incomplete. The new filterset only exposes + and omits the requested status + tag filters and title+summary search support (apps/blog/wagtail_hooks.py:38-51, 53-69).\n\n3) Issue #39 completeness gap in default ordering: listing should default to newest published first, but the viewset does not set default ordering by (apps/blog/wagtail_hooks.py:53-69).\n\nValidation performed: full pytest run in container (........................................................................ [ 46%]
Re-reviewed after updating local main to latest origin/main. Scope is now correctly limited to Issue #37 (apps/comments/wagtail_hooks.py + apps/comments/tests/test_admin.py). The fix is correct: list_display uses an explicit Column for pending_in_article and includes a regression test for /cms/snippets/comments/comment/ returning 200. Validation passed: docker compose run --rm web pytest -q apps/comments/tests/test_admin.py --no-cov.
Requesting changes: this PR includes many unrelated files (blog/core/templates/workflow/config) beyond the Issue #37 comments-admin 500 fix, which makes scope/risk too high for safe review. Please rebase/cherry-pick only the comments fix and regression test into a clean branch targeting main. The comments-specific changes (Column for pending_in_article + admin index regression test) look directionally correct.
Re-review complete. Previously reported blockers are now addressed:
Re-review complete: still requesting changes.
