Skip to content

Conversation

@RadCod3
Copy link
Owner

@RadCod3 RadCod3 commented Jan 13, 2026

Purpose

Use the same transaction type available in firefly models to avoid KeyErrors. Also adds a type field to the Account model.
Resolves #54

Use TransactionTypeProperty from firefly models to avoid KeyError's when encountered transactions types that were unavailble in the lampyrid model previously. Also adds a validation to the CreateBulkTransactionsRequest to only allow withdrawal, deposit and transfer
@coderabbitai
Copy link

coderabbitai bot commented Jan 13, 2026

📝 Walkthrough

Summary by CodeRabbit

  • New Features
    • Accounts now include type information, providing better clarity on account details.
    • Bulk transaction requests now validate that each transaction specifies a valid type—withdrawal, deposit, or transfer—ensuring data consistency and integrity.

✏️ Tip: You can customize this high-level summary in your review settings.

Walkthrough

Configuration enhancements to pyproject.toml include Ruff lint settings and datamodel-codegen options. The Account model gains a new type field, and Transaction type handling shifts from an enum-based approach to a property-based model with validation added to CreateBulkTransactionsRequest.

Changes

Cohort / File(s) Summary
Configuration Updates
pyproject.toml
Added Ruff lint configuration with import sorting (extend-select = ["I"]) and enhanced datamodel-codegen to output Pydantic v2 BaseModel types.
Model Refactoring
src/lampyrid/models/lampyrid_models.py
Removed TransactionType enum; replaced with TransactionTypeProperty for flexible type handling. Added type: ShortAccountTypeProperty field to Account. Updated from_transaction_single, from_transaction_read, and to_transaction_split_store to use direct property assignment. Introduced validate_transactions() validator on CreateBulkTransactionsRequest to enforce valid transaction types.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 An enum's shackles we cast away,
Properties dance in the light of day,
No more KeyErrors for reconciliation's call,
Flexibility wins—we welcome them all! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description clearly explains the purpose: replacing the local transaction type enum with Firefly models' type and adding a type field to Account, directly addressing issue #54.
Linked Issues check ✅ Passed The PR resolves issue #54 by replacing TransactionType enum with TransactionTypeProperty from Firefly models, which supports all transaction types including 'reconciliation', eliminating KeyErrors for reconciled transactions.
Out of Scope Changes check ✅ Passed All changes are directly in scope: pyproject.toml updates support the main code changes, Account model gains type field as intended, and Transaction type handling is refactored to use Firefly's types.
Title Check ✅ Passed Title check skipped as CodeRabbit has written the PR title.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c3aedb1 and c813439.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (2)
  • pyproject.toml
  • src/lampyrid/models/lampyrid_models.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Docker Build Test
🔇 Additional comments (6)
pyproject.toml (1)

35-36: LGTM!

Enabling isort rules via extend-select = ["I"] is a good practice for maintaining consistent import ordering across the codebase.

src/lampyrid/models/lampyrid_models.py (5)

2-17: LGTM!

The imports are correctly updated to include ShortAccountTypeProperty for the new Account type field, and TransactionTypeProperty is properly used instead of a local enum.


28-30: LGTM!

Adding the type field to the Account model with ShortAccountTypeProperty enriches the model and correctly maps from the Firefly API's account attributes.

Also applies to: 44-44


87-87: LGTM! Core fix for issue #54.

Using TransactionTypeProperty directly instead of a local enum correctly handles all transaction types from Firefly III (including reconciliation, opening_balance, etc.) without causing KeyError exceptions. The direct type assignment across from_transaction_single, from_transaction_read, and to_transaction_split_store maintains type consistency.

Also applies to: 112-112, 132-132, 144-144


544-556: LGTM! Good defensive validation.

The validator correctly restricts bulk transaction creation to user-creatable types (withdrawal, deposit, transfer), preventing attempts to create system-managed transaction types like reconciliation or opening_balance. The set-based membership check is efficient, and the error message is actionable.


6-17: TransactionTypeProperty enum is correctly defined.

The TransactionTypeProperty enum in firefly_models includes all expected transaction types: withdrawal, deposit, transfer, reconciliation, and opening_balance. The import is correct and complete.


Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai bot changed the title @coderabbitai Add Account type field and refactor Transaction handling Jan 13, 2026
@RadCod3 RadCod3 merged commit 3cd2a85 into main Jan 13, 2026
10 of 13 checks passed
@RadCod3 RadCod3 deleted the fix/54-keyerror-txtools branch January 13, 2026 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Transaction tools fail with KeyError: 'reconciliation' for reconciled transactions

2 participants