ldraney/pal-e-api
Archived
1
0
Fork 0
Code Issues 15 Pull requests 1 Projects Releases Packages Wiki Activity Actions

7e-1a: QA nits cleanup -- redundant assignment + test helper hygiene #114

Merged
forgejo_admin merged 1 commit from 111-7e-1a-qa-nits-cleanup into main 2026-03-08 07:14:12 +00:00
Conversation 1 Commits 1 Files changed 2 +37 -17
forgejo_admin commented 2026-03-08 07:11:42 +00:00
Contributor
Copy link

Summary

  • Remove redundant html_content=body.html_content assignment in create_note() since parse_and_store_blocks() overwrites it immediately
  • Document the separate-session test helper pattern and make _get_compiled_page() detach-safe

Changes

  • src/pal_e_docs/routes/notes.py: Changed html_content=body.html_content to html_content="" in the Note constructor with explanatory comment
  • tests/test_note_block_sync.py: Added block comment documenting why helpers use separate TestingSessionLocal() sessions; added docstrings to all three helpers; converted _get_compiled_page() to return a plain dict instead of a detached SQLAlchemy instance; updated all callers to dict access; replaced inline session usage in test_compiled_page_matches_note_html with API call + helper

Test Plan

  • Tests pass locally (509 passed)
  • ruff check and ruff format --check pass
  • No behavioral changes -- purely readability/hygiene

Review Checklist

  • Passed automated review-fix loop
  • No secrets committed
  • No unnecessary file changes
  • Commit messages are descriptive

Related

  • Closes forgejo_admin/pal-e-docs #111
  • plan-2026-02-26-tf-modularize-postgres -- Phase 7e-1a
## Summary - Remove redundant `html_content=body.html_content` assignment in `create_note()` since `parse_and_store_blocks()` overwrites it immediately - Document the separate-session test helper pattern and make `_get_compiled_page()` detach-safe ## Changes - `src/pal_e_docs/routes/notes.py`: Changed `html_content=body.html_content` to `html_content=""` in the Note constructor with explanatory comment - `tests/test_note_block_sync.py`: Added block comment documenting why helpers use separate `TestingSessionLocal()` sessions; added docstrings to all three helpers; converted `_get_compiled_page()` to return a plain dict instead of a detached SQLAlchemy instance; updated all callers to dict access; replaced inline session usage in `test_compiled_page_matches_note_html` with API call + helper ## Test Plan - [x] Tests pass locally (509 passed) - [x] `ruff check` and `ruff format --check` pass - [x] No behavioral changes -- purely readability/hygiene ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes forgejo_admin/pal-e-docs #111 - `plan-2026-02-26-tf-modularize-postgres` -- Phase 7e-1a
7e-1a: QA nits cleanup -- redundant assignment + test helper hygiene
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
7171343c7c 
- Change create_note() html_content initializer from body.html_content to
  empty string, since parse_and_store_blocks() overwrites it immediately
- Document the separate-session pattern in test helpers explaining why
  TestingSessionLocal() is used instead of the client's DB session
- Convert _get_compiled_page() to return a plain dict snapshot instead of
  a detached SQLAlchemy instance, preventing potential detached-object errors
- Replace last inline TestingSessionLocal usage in
  test_compiled_page_matches_note_html with API call + helper

Closes #111

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
forgejo_admin commented 2026-03-08 07:13:03 +00:00
Author
Contributor
Copy link

PR #114 Review

BLOCKERS

None.

NITS

  1. _get_compiled_page return type annotation uses dict | None (Python 3.10+ union syntax) -- This is fine as long as the project's minimum Python version supports it. Consistent with the rest of the codebase so no action needed, just noting it.

  2. _count_blocks and _get_blocks were not given return type annotations -- The PR added a return type to _get_compiled_page (-> dict | None) and docstrings to all three helpers, but the other two helpers still lack return type annotations on their signatures. Minor inconsistency, non-blocking.

CODE REVIEW

notes.py change (line 273): Correct. parse_and_store_blocks() at blocks/sync.py:68 overwrites note.html_content with compiled output via recompile_note(). Setting it to "" instead of body.html_content eliminates a redundant assignment and makes the data flow clearer. The guard at line 286 (if body.html_content:) means the empty string is never persisted for notes with actual content -- they always go through parse_and_store_blocks.

test_note_block_sync.py changes:

  • Block comment explaining the separate-session pattern is accurate and helpful.
  • Converting _get_compiled_page() to return a plain dict eliminates a real risk of detached-instance errors after session.close(). The current code on main accesses compiled.content_hash and compiled.toc_json after the session is closed in the finally block -- this works by accident (lazy loading already resolved) but is fragile. The dict approach is correct.
  • All callers properly updated from attribute access (compiled.content_hash) to dict access (compiled["content_hash"]).
  • test_compiled_page_matches_note_html refactored to use API call + helper instead of inline session -- consistent with the documented pattern and eliminates the last inline TestingSessionLocal() usage in test methods.

SOP COMPLIANCE

  • Branch named after issue (111-7e-1a-qa-nits-cleanup references issue #111)
  • PR body has: Summary, Changes, Test Plan, Related
  • Related references plan slug (plan-2026-02-26-tf-modularize-postgres)
  • Related references issue (Closes forgejo_admin/pal-e-docs #111)
  • No secrets committed
  • No unnecessary file changes (2 files, both in scope)
  • Commit messages are descriptive

VERDICT: APPROVED

## PR #114 Review ### BLOCKERS None. ### NITS 1. **`_get_compiled_page` return type annotation uses `dict | None` (Python 3.10+ union syntax)** -- This is fine as long as the project's minimum Python version supports it. Consistent with the rest of the codebase so no action needed, just noting it. 2. **`_count_blocks` and `_get_blocks` were not given return type annotations** -- The PR added a return type to `_get_compiled_page` (`-> dict | None`) and docstrings to all three helpers, but the other two helpers still lack return type annotations on their signatures. Minor inconsistency, non-blocking. ### CODE REVIEW **`notes.py` change (line 273):** Correct. `parse_and_store_blocks()` at `blocks/sync.py:68` overwrites `note.html_content` with compiled output via `recompile_note()`. Setting it to `""` instead of `body.html_content` eliminates a redundant assignment and makes the data flow clearer. The guard at line 286 (`if body.html_content:`) means the empty string is never persisted for notes with actual content -- they always go through `parse_and_store_blocks`. **`test_note_block_sync.py` changes:** - Block comment explaining the separate-session pattern is accurate and helpful. - Converting `_get_compiled_page()` to return a plain dict eliminates a real risk of detached-instance errors after `session.close()`. The current code on main accesses `compiled.content_hash` and `compiled.toc_json` after the session is closed in the `finally` block -- this works by accident (lazy loading already resolved) but is fragile. The dict approach is correct. - All callers properly updated from attribute access (`compiled.content_hash`) to dict access (`compiled["content_hash"]`). - `test_compiled_page_matches_note_html` refactored to use API call + helper instead of inline session -- consistent with the documented pattern and eliminates the last inline `TestingSessionLocal()` usage in test methods. ### SOP COMPLIANCE - [x] Branch named after issue (`111-7e-1a-qa-nits-cleanup` references issue #111) - [x] PR body has: Summary, Changes, Test Plan, Related - [x] Related references plan slug (`plan-2026-02-26-tf-modularize-postgres`) - [x] Related references issue (`Closes forgejo_admin/pal-e-docs #111`) - [x] No secrets committed - [x] No unnecessary file changes (2 files, both in scope) - [x] Commit messages are descriptive ### VERDICT: APPROVED
forgejo_admin deleted branch 111-7e-1a-qa-nits-cleanup 2026-03-08 07:14:12 +00:00
Commenting is not possible because the repository is archived.
Reviewers
No reviewers
Labels
Clear labels   
domain:backend

   
domain:devops

   
domain:frontend

   
status:approved

QA passed, awaiting merge approval

  
status:in-progress

Dev agent is actively working

  
status:needs-fix

QA found issues, back to dev

  
status:qa

PR submitted, awaiting QA review

  
type:bug

Bug fix

  
type:devops

Infrastructure/CI/config work

  
type:feature

New functionality

No labels
domain:backend
domain:devops
domain:frontend
status:approved
status:in-progress
status:needs-fix
status:qa
type:bug
type:devops
type:feature
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
ldraney/pal-e-api!114
No description provided.