Conversation
Implement comprehensive financial analysis capabilities, including: - Add insight API methods to FireflyClient for expense, income, and transfer data. - Implement InsightService to coordinate business logic and model transformations. - Create new MCP tools for detailed expense, income, and transfer analysis. - Provide a get_financial_summary tool for a high-level overview of net position. - Define structured Pydantic models for all insight requests and results.
- Implement comprehensive integration tests for expense, income, and transfer insights. - Add automated seed transaction generation and cleanup logic to conftest.py. - Introduce insight request factory fixtures for standardized test data. - Register the insights pytest marker in pyproject.toml.
📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds Insights: Firefly client insight methods, new insight request/result models, an InsightService, MCP tools/server for insights, test fixtures and integration tests with seeded transactions, and a pytest marker for insight tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as MCP Client
participant Tool as Insight Tool
participant Service as InsightService
participant FClient as FireflyClient
participant API as Firefly API
Client->>Tool: get_expense_insight(req)
Tool->>Service: get_expense_insight(req)
rect rgba(100, 150, 200, 0.5)
Note over Service,FClient: Service composes params and calls client endpoints
Service->>FClient: get_expense_total(start,end,accounts)
FClient->>API: GET /v1/insight/expense/total?...
API-->>FClient: InsightTotal
Service->>FClient: get_expense_by_expense_account(...)
FClient->>API: GET /v1/insight/expense/expense?...
API-->>FClient: InsightGroup
end
rect rgba(150, 100, 200, 0.5)
Note over Service: Transform API models -> internal entries/results
Service->>Service: _entries_from_insight_total(...)
Service->>Service: _entries_from_insight_group(...)
Service->>Service: _get_total_and_currency(...)
end
Service-->>Tool: ExpenseInsightResult
Tool-->>Client: ExpenseInsightResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
bot
changed the title
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #69 +/- ##
==========================================
+ Coverage 97.53% 97.62% +0.09%
==========================================
Files 17 19 +2
Lines 2841 3080 +239
==========================================
+ Hits 2771 3007 +236
- Misses 70 73 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/integration/test_insights.py`:
- Around line 55-60: The snapshot expectations in
tests/integration/test_insights.py disagree with the seeded withdrawal amounts
in tests/conftest.py (seeded withdrawals sum to 145), causing failures; either
update the snapshot expected totals and derived net positions in test_insights
(search for expected 'total_expenses' and related summary/net position values)
to match the actual seeded sum (145 and corresponding currency_code) or change
the seed withdrawals in tests/conftest.py to the values used in the current
snapshots and then regenerate all snapshots to keep them consistent; ensure
every affected snapshot entry (expense totals, summary totals, and net position
assertions) is updated consistently across the file.
- Line 96: The snapshot assertions in tests/integration/test_insights.py include
hard-coded ID literals (e.g., the dict with 'id': '6', 'name': 'Test Expense 2')
which makes tests brittle; update those expected snapshots to either use a
flexible matcher like IsStr(min_length=1) for 'id' fields or substitute IDs from
the test fixtures (where available) instead of literal strings; apply the same
change to the other hard-coded ID occurrences called out in the review (lines
referenced: occurrences around the shown dict and the other listed locations) so
all expected data uses matchers or fixture-supplied IDs rather than fixed
numeric strings.
🧹 Nitpick comments (6)
src/lampyrid/models/lampyrid_models.py (1)
783-815: Consider adding date range validation.The request models don't validate that
start_date <= end_date. While the API might handle this gracefully, adding a model validator would provide clearer error messages to users.📝 Optional: Add date validation
class GetExpenseInsightRequest(BaseModel): """Request for expense insight analysis.""" model_config = ConfigDict(extra='forbid') start_date: date = Field(..., description='Start date for the analysis period (YYYY-MM-DD)') end_date: date = Field(..., description='End date for the analysis period (YYYY-MM-DD)') # ... other fields ... + + `@model_validator`(mode='after') + def validate_date_range(self): + """Ensure start_date is not after end_date.""" + if self.start_date > self.end_date: + raise ValueError('start_date must be on or before end_date') + return selfSimilar validation could be added to the other insight request models.
src/lampyrid/services/insights.py (2)
89-97: Minor: Redundant conditional check.On line 96, the check
if entries else 'USD'is redundant because the function returns early on line 93-94 whenentriesis empty.🧹 Simplify the conditional
def _get_total_and_currency( self, entries: list[InsightEntry] | list[TransferInsightEntry] ) -> tuple[float, str]: """Calculate total amount and get primary currency from entries.""" if not entries: return 0.0, 'USD' total = sum(e.amount for e in entries) - currency = entries[0].currency_code if entries else 'USD' + currency = entries[0].currency_code return total, currency
261-278: Multi-currency aggregation may produce misleading results.When the user has transactions in multiple currencies, this implementation sums all amounts regardless of currency and uses the first available currency code. This could produce confusing results (e.g., summing USD and EUR amounts).
Consider either:
- Documenting this limitation in the API response/docstrings
- Returning separate totals per currency
- Filtering to a single currency if the API supports it
This is acceptable for an initial implementation but worth noting for future enhancement.
tests/fixtures/insights.py (1)
23-30: Extract a shared month-range helper to reduce duplication.The default date-range logic is repeated in all four factories; centralizing it will keep behavior consistent and lower maintenance overhead.
♻️ Proposed refactor
+def _resolve_month_range(start: date | None, end: date | None) -> tuple[date, date]: + """Resolve missing start/end dates to the current month.""" + if start is None: + start = date.today().replace(day=1) + if end is None: + next_month = start.replace(day=28) + timedelta(days=4) + end = next_month.replace(day=1) - timedelta(days=1) + return start, end + def make_get_expense_insight_request( start: date | None = None, end: date | None = None, @@ ) -> GetExpenseInsightRequest: """Create a GetExpenseInsightRequest for testing.""" - if start is None: - # Default to current month - start = date.today().replace(day=1) - if end is None: - # Default to end of current month - next_month = start.replace(day=28) + timedelta(days=4) - end = next_month.replace(day=1) - timedelta(days=1) + start, end = _resolve_month_range(start, end) @@ def make_get_income_insight_request( @@ ) -> GetIncomeInsightRequest: """Create a GetIncomeInsightRequest for testing.""" - if start is None: - # Default to current month - start = date.today().replace(day=1) - if end is None: - # Default to end of current month - next_month = start.replace(day=28) + timedelta(days=4) - end = next_month.replace(day=1) - timedelta(days=1) + start, end = _resolve_month_range(start, end) @@ def make_get_transfer_insight_request( @@ ) -> GetTransferInsightRequest: """Create a GetTransferInsightRequest for testing.""" - if start is None: - # Default to current month - start = date.today().replace(day=1) - if end is None: - # Default to end of current month - next_month = start.replace(day=28) + timedelta(days=4) - end = next_month.replace(day=1) - timedelta(days=1) + start, end = _resolve_month_range(start, end) @@ def make_get_financial_summary_request( @@ ) -> GetFinancialSummaryRequest: """Create a GetFinancialSummaryRequest for testing.""" - if start is None: - # Default to current month - start = date.today().replace(day=1) - if end is None: - # Default to end of current month - next_month = start.replace(day=28) + timedelta(days=4) - end = next_month.replace(day=1) - timedelta(days=1) + start, end = _resolve_month_range(start, end)Also applies to: 48-55, 71-78, 93-100
tests/integration/test_insights.py (1)
16-23: Align request date range with the seed month to avoid boundary flakes.Seeds are created using
date.today()at session start, while tests computedate.today()at run time. If a session crosses a month boundary, tests can query a different month than the seeded data. Consider a shared fixture for the seed period or freezing time for these tests.tests/conftest.py (1)
202-310: Consider gating seed transaction creation for parallel execution.While pytest-xdist is not currently configured, the session-scoped
_seed_transaction_idsinitialization could cause duplicate seed data if parallel execution is added. Each xdist worker would independently create transactions in the shared Firefly III backend sinceif not _seed_transaction_ids:is worker-local. The existing cleanup fixture helps but won't prevent duplication across workers.To future-proof, consider using
pytest-xdistworker detection (viaworker_idfixture) to create seed data only on worker 0, or tag transactions with a run-specific ID for filtering.
RadCod3
changed the title
RadCod3
force-pushed
the
feat/68-add-insight-tools
branch
from
56589b8 to
09d9547
Compare
2 participants
Resolves #68