Hygiene sweep: dead code, role-assignment race, cached user lookup, stale files #60

Open

padreug wants to merge 1 commit from chore/hygiene into fix/auth-and-input-validation

padreug commented

2026-07-12 14:01:56 +00:00

Owner

Sixth PR of the refactor series (stacked on #59 → #58 → #57 → #56 → #55; merge in order).

Sweeps the LOW-tier review items + two folded issues:

Dead code: validate_journal_entry deleted (validated the pre-Fava, pre-string-amount model; never called) with exports, unused crud imports, and its tests.
Role-assignment race (review LOW): migration m006 adds a UNIQUE index on user_roles(user_id, role_id) after deduping; assign_user_role inserts with ON CONFLICT DO NOTHING and returns the existing row — two concurrent logins can no longer double-assign the default role.
User lookup extraction (review #18): the 110-line _get_username_from_user_id (fresh LNbits Database per call in per-row hot paths) moves to user_lookup.py with one shared core-DB handle, a 60s TTL cache and a batch get_usernames API.
libra-#35: receivable-entry API responses now report CLEARED — matching the * flag format_receivable_entry actually writes. (Income/expense genuinely are pending; only receivable misreported.)
libra-#54: account roots derived from account_utils.ACCOUNT_TYPE_ROOTS (the last hardcoded tuple in fava_client); the no-op per-test rate-limit reset removed from conftest.
print() → logger in tasks.py; stale files deleted (migrations_old.py.bak, MIGRATION_SQUASH_SUMMARY.md, docs/PHASE*_COMPLETE.md, rendered .html); data/ gitignored (holds the runtime .lnbits_auth_key); docs/CODE-REVIEW-2026-06.md now tracked with statuses updated for this series; CLAUDE.md notes LNbits pins Pydantic v1 (keep .dict()).

Non-fix: format_payment_entry's is_payable docstring, flagged in the review follow-up as inverted, is actually consistent with the body — left alone.

Still open from the review's LOW tier: parse_legacy_account_name fragility (internal input only, documented) and the is_active/is_virtual filter inconsistency.

Tests: 177 passed, 3 skipped (5 tests removed with the dead validator).

🤖 Generated with Claude Code

Sixth PR of the refactor series (stacked on #59 → #58 → #57 → #56 → #55; merge in order). Sweeps the LOW-tier review items + two folded issues: - **Dead code**: `validate_journal_entry` deleted (validated the pre-Fava, pre-string-amount model; never called) with exports, unused crud imports, and its tests. - **Role-assignment race** (review LOW): migration `m006` adds a UNIQUE index on `user_roles(user_id, role_id)` after deduping; `assign_user_role` inserts with `ON CONFLICT DO NOTHING` and returns the existing row — two concurrent logins can no longer double-assign the default role. - **User lookup extraction** (review #18): the 110-line `_get_username_from_user_id` (fresh LNbits `Database` per call in per-row hot paths) moves to `user_lookup.py` with one shared core-DB handle, a 60s TTL cache and a batch `get_usernames` API. - **libra-#35**: receivable-entry API responses now report CLEARED — matching the `*` flag `format_receivable_entry` actually writes. (Income/expense genuinely are pending; only receivable misreported.) - **libra-#54**: account roots derived from `account_utils.ACCOUNT_TYPE_ROOTS` (the last hardcoded tuple in fava_client); the no-op per-test rate-limit reset removed from conftest. - `print()` → `logger` in tasks.py; stale files deleted (`migrations_old.py.bak`, `MIGRATION_SQUASH_SUMMARY.md`, `docs/PHASE*_COMPLETE.md`, rendered `.html`); `data/` gitignored (holds the runtime `.lnbits_auth_key`); `docs/CODE-REVIEW-2026-06.md` now tracked with statuses updated for this series; CLAUDE.md notes LNbits pins **Pydantic v1** (keep `.dict()`). Non-fix: `format_payment_entry`'s `is_payable` docstring, flagged in the review follow-up as inverted, is actually consistent with the body — left alone. Still open from the review's LOW tier: `parse_legacy_account_name` fragility (internal input only, documented) and the `is_active`/`is_virtual` filter inconsistency. **Tests:** 177 passed, 3 skipped (5 tests removed with the dead validator). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

padreug added 1 commit

2026-07-12 14:01:56 +00:00

chore: hygiene sweep — dead code, role race, user lookup, stale files ec6cac51f0

One pass over the LOW-tier review items plus two folded issues:

- Delete validate_journal_entry (dead since the Fava migration; it
  validated the pre-string-amount model) with its exports, unused
  crud imports, and tests. Beancount validates entries now.
- Migration m006: UNIQUE index on user_roles(user_id, role_id) after
  deduping; assign_user_role inserts with ON CONFLICT DO NOTHING and
  returns the existing assignment — closes the auto-assign
  check-then-act race on concurrent logins.
- Extract _get_username_from_user_id (110 lines in views_api, fresh
  LNbits Database per call inside per-row hot paths) into
  user_lookup.py with one shared core-DB handle, a 60s TTL cache and
  a batch get_usernames API (review #18).
- Receivable-entry responses report CLEARED, matching the flag the
  formatter actually writes; PENDING misled the UI (libra-#35).
- Replace the remaining print() calls in tasks.py with logger.
- get_all_accounts derives valid roots from
  account_utils.ACCOUNT_TYPE_ROOTS instead of a hardcoded tuple, and
  the no-op per-test rate-limit reset is gone (libra-#54).
- Delete migrations_old.py.bak, MIGRATION_SQUASH_SUMMARY.md,
  docs/PHASE*_COMPLETE.md and the rendered .html; .gitignore data/
  (it holds the runtime .lnbits_auth_key secret).
- Track docs/CODE-REVIEW-2026-06.md with finding statuses updated for
  the PR #55-#59 + chore/hygiene series.
- CLAUDE.md notes LNbits pins Pydantic v1: keep .dict(), don't
  "modernize" to .model_dump().

Note: format_payment_entry's is_payable docstring (flagged in review
follow-up) turned out to be consistent with the body — no change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

padreug referenced this pull request

2026-07-12 14:13:07 +00:00

Split views_api into a domain package; unshadow /accounts/hierarchy #61

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin chore/hygiene:chore/hygiene

git switch chore/hygiene

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.