fix: re-fetch existing transactions on each validation #72

cal · 2026-03-09T13:10:38Z

cal commented

2026-03-09 13:10:38 +00:00

Summary

TransactionBuilder.load_existing_transactions() cached results on first call and never refreshed, even across a multi-hour session
Transactions submitted by other users/sessions (or newly visible after API fixes) were invisible for the lifetime of the builder session
This caused incorrect MiL roster projections — MKE saw "8 players on MiL (limit 6)" because 2 scheduled FA releases weren't reflected
Fix: remove the _existing_transactions_loaded cache gate so the method always re-fetches from the API

Companion fix: cal/major-domo-database#60 (case-insensitive team_abbrev filter)

Test plan

39/39 transaction builder tests passing
MKE manager starts fresh /dropadd session, adds FA player to MiL — should project correct count (accounting for Myers/Ross releases)

🤖 Generated with Claude Code

## Summary - `TransactionBuilder.load_existing_transactions()` cached results on first call and never refreshed, even across a multi-hour session - Transactions submitted by other users/sessions (or newly visible after API fixes) were invisible for the lifetime of the builder session - This caused incorrect MiL roster projections — MKE saw "8 players on MiL (limit 6)" because 2 scheduled FA releases weren't reflected - Fix: remove the `_existing_transactions_loaded` cache gate so the method always re-fetches from the API ## Related - Companion fix: cal/major-domo-database#60 (case-insensitive `team_abbrev` filter) ## Test plan - [x] 39/39 transaction builder tests passing - [ ] MKE manager starts fresh `/dropadd` session, adds FA player to MiL — should project correct count (accounting for Myers/Ross releases) 🤖 Generated with [Claude Code](https://claude.com/claude-code)

cal added 1 commit 2026-03-09 13:10:39 +00:00

fix: re-fetch existing transactions on each validation 556a30c397

The TransactionBuilder cached pre-existing transactions on first load
and never refreshed them. This meant transactions submitted by other
sessions (or newly visible after API fixes) were invisible for the
lifetime of the builder session, causing incorrect roster projections.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Claude added the

ai-reviewing

label 2026-03-09 13:15:34 +00:00

Claude reviewed 2026-03-09 13:17:09 +00:00

Claude left a comment

AI Code Review

Files Reviewed

services/transaction_builder.py (modified)

Findings

Correctness

The fix is correct and well-targeted. Removing the _existing_transactions_loaded guard means every call to validate_transaction(next_week=...) re-fetches transactions from the API, capturing moves submitted by other users or sessions during the lifetime of the builder.

The _existing_transactions field correctly remains as Optional[List[Transaction]] = None in __init__ — it's now a per-call write target rather than a session cache. The guard in validate_transaction (if self._existing_transactions:) correctly handles the None case when next_week is not provided, so no regression there.

Error path is sound: the except block sets self._existing_transactions = [], which is falsy and skips the loop in validate_transaction. Same as before.

Security

No issues found.

Style & Conventions

Trivial: A blank line remains between the updated docstring and try: block (artifact of removing the two-line guard). Cosmetic only, not a blocker.
Doc debt: services/CLAUDE.md under "Transaction Builder Enhancements" still documents the old caching behavior: "Performance Optimization: Pre-existing transactions loaded once and cached". This is now inaccurate. Not blocking, but worth a follow-up edit.

Suggestions

Consider updating the services/CLAUDE.md note from "cached" to "re-fetched on each validation" to match the new behavior.

Verdict: APPROVED

Minimal, correct fix. The cache invalidation approach (removing the gate entirely rather than adding explicit invalidation logic) is the right call here — the builder session is long-lived but the underlying data changes frequently in a multi-user environment. 39/39 tests passing confirms no regressions.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `services/transaction_builder.py` (modified) ### Findings #### Correctness The fix is correct and well-targeted. Removing the `_existing_transactions_loaded` guard means every call to `validate_transaction(next_week=...)` re-fetches transactions from the API, capturing moves submitted by other users or sessions during the lifetime of the builder. The `_existing_transactions` field correctly remains as `Optional[List[Transaction]] = None` in `__init__` — it's now a per-call write target rather than a session cache. The guard in `validate_transaction` (`if self._existing_transactions:`) correctly handles the `None` case when `next_week` is not provided, so no regression there. Error path is sound: the `except` block sets `self._existing_transactions = []`, which is falsy and skips the loop in `validate_transaction`. Same as before. #### Security No issues found. #### Style & Conventions - **Trivial**: A blank line remains between the updated docstring and `try:` block (artifact of removing the two-line guard). Cosmetic only, not a blocker. - **Doc debt**: `services/CLAUDE.md` under "Transaction Builder Enhancements" still documents the old caching behavior: *"Performance Optimization: Pre-existing transactions loaded once and cached"*. This is now inaccurate. Not blocking, but worth a follow-up edit. #### Suggestions - Consider updating the `services/CLAUDE.md` note from "cached" to "re-fetched on each validation" to match the new behavior. ### Verdict: APPROVED Minimal, correct fix. The cache invalidation approach (removing the gate entirely rather than adding explicit invalidation logic) is the right call here — the builder session is long-lived but the underlying data changes frequently in a multi-user environment. 39/39 tests passing confirms no regressions. --- *Automated review by Claude PR Reviewer*

Claude added

ai-reviewed

and removed

ai-reviewing

labels 2026-03-09 13:17:39 +00:00

cal merged commit 541380acdf into next-release

2026-03-11 16:27:57 +00:00

cal deleted branch fix/refresh-existing-transactions-on-validation

2026-03-11 16:27:57 +00:00

cal referenced this issue from a commit

2026-03-11 16:27:58 +00:00

Merge pull request 'fix: re-fetch existing transactions on each validation' (#72) from fix/refresh-existing-transactions-on-validation into next-release