fix: remove unimplemented is_trade parameter from GET /api/v3/transactions (#27) #57

cal · 2026-03-06T05:02:52Z

cal commented

2026-03-06 05:02:52 +00:00

Summary

Removed is_trade: Optional[bool] = None from the get_transactions function signature
Removed the 501 stub block (if is_trade is not None: raise HTTPException(status_code=501, ...))

Reason

The Transaction model in db_engine.py has no is_trade field, so this parameter cannot be implemented without a schema migration. Removing it is the correct approach for an undocumented/unplanned feature stub.

Files Changed

app/routers_v3/transactions.py

Notes

ruff/black auto-reformatted the file on edit (single→double quotes, line wrapping, trailing commas). This is cosmetic noise expected from the project's linter setup and does not affect functionality.

Other observations

Pre-existing Pyright warnings about .id attribute on Peewee model classes (lines 69, 141, 145, 149) — unrelated to this change
x.dict() deprecation warning (Pydantic v1 → v2) at line 154 — pre-existing

Closes #27

## Summary - Removed `is_trade: Optional[bool] = None` from the `get_transactions` function signature - Removed the 501 stub block (`if is_trade is not None: raise HTTPException(status_code=501, ...)`) ## Reason The `Transaction` model in `db_engine.py` has no `is_trade` field, so this parameter cannot be implemented without a schema migration. Removing it is the correct approach for an undocumented/unplanned feature stub. ## Files Changed - `app/routers_v3/transactions.py` ## Notes ruff/black auto-reformatted the file on edit (single→double quotes, line wrapping, trailing commas). This is cosmetic noise expected from the project's linter setup and does not affect functionality. ## Other observations - Pre-existing Pyright warnings about `.id` attribute on Peewee model classes (lines 69, 141, 145, 149) — unrelated to this change - `x.dict()` deprecation warning (Pydantic v1 → v2) at line 154 — pre-existing Closes #27

cal added 1 commit 2026-03-06 05:02:53 +00:00

fix: remove unimplemented is_trade parameter from GET /api/v3/transactions (#27 )

Build Docker Image / build (pull_request) Successful in 2m9s

Details

1e8ab88690

The Transaction model has no is_trade field, so filtering on it cannot
be implemented without a schema migration. Removed the parameter from
the function signature and the 501 stub block.

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

cal referenced this pull request

2026-03-06 05:03:02 +00:00

Implement or remove the `is_trade` parameter from `GET /api/v3/transactions` #27

cal added the

ai-reviewing

label 2026-03-06 05:15:34 +00:00

cal reviewed 2026-03-06 05:16:33 +00:00

cal left a comment

AI Code Review

Files Reviewed

app/routers_v3/transactions.py (modified)

Findings

Correctness

The is_trade parameter and its 501 stub are cleanly removed from get_transactions. No other logic is affected.
Callers previously passing is_trade=true would have received a 501; they will now receive a 200 with data (FastAPI silently ignores unrecognized query params). This is strictly better UX for a parameter that was never functional.
The PR body correctly notes that Transaction has no is_trade field — removal is the right call without a schema migration.

Security

No new security issues introduced. Pre-existing {token} interpolation in logger.warning calls at patch_transactions, post_transactions, and delete_transactions remains, but this is tracked by PR #45 and is out of scope here.

Style & Conventions

ruff/black reformatting (single→double quotes, trailing commas, line wrapping) is cosmetic noise expected per the project setup. No functional changes.

Suggestions

None.

Verdict: APPROVED

Correct, minimal removal of a dead stub. No logic changes beyond the targeted deletion. Merge when ready.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/routers_v3/transactions.py` (modified) ### Findings #### Correctness - The `is_trade` parameter and its 501 stub are cleanly removed from `get_transactions`. No other logic is affected. - Callers previously passing `is_trade=true` would have received a 501; they will now receive a 200 with data (FastAPI silently ignores unrecognized query params). This is strictly better UX for a parameter that was never functional. - The PR body correctly notes that `Transaction` has no `is_trade` field — removal is the right call without a schema migration. #### Security - No new security issues introduced. Pre-existing `{token}` interpolation in `logger.warning` calls at `patch_transactions`, `post_transactions`, and `delete_transactions` remains, but this is tracked by PR #45 and is out of scope here. #### Style & Conventions - ruff/black reformatting (single→double quotes, trailing commas, line wrapping) is cosmetic noise expected per the project setup. No functional changes. #### Suggestions - None. ### Verdict: APPROVED Correct, minimal removal of a dead stub. No logic changes beyond the targeted deletion. Merge when ready. --- *Automated review by Claude PR Reviewer*

cal added

ai-reviewed

and removed

ai-reviewing

labels 2026-03-06 05:16:59 +00:00

cal changed target branch from main to next-release

2026-03-07 07:32:48 +00:00

Build Docker Image / build (pull_request) Successful in 2m9s

Details

This pull request has changes conflicting with the target branch.