From 166f82d0bfdc598099c088275d68dc42499694f9 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 2 Jul 2026 09:10:46 +0200 Subject: feat: tx add --from-id/--to-id to disambiguate same-name accounts (v0.3.7) ISSUES.md #2: two accounts can share a name (e.g. expense id 52 and revenue id 129 both "Nexi"), making --from/--to ambiguous and unresolvable. Add --from-id/--to-id to target an account by numeric id. Per side, exactly one of the name flag or the id flag; sides independent. The id path fetches the account (resolver.account_by_id), validating existence before writing. Name-only callers unchanged; JSON/exit unchanged, so PATCH. Co-Authored-By: Claude Opus 4.8 --- SKILL.md | 7 ++- completions/firefly.bash | 2 +- firefly_cli/__init__.py | 2 +- firefly_cli/commands/transaction.py | 21 ++++++-- firefly_cli/resolver.py | 6 +++ pyproject.toml | 2 +- tests/unit/test_commands_transaction.py | 93 ++++++++++++++++++++++++++++----- tests/unit/test_resolver.py | 10 ++++ 8 files changed, 123 insertions(+), 20 deletions(-) diff --git a/SKILL.md b/SKILL.md index 53ebd6f..33e5ffc 100644 --- a/SKILL.md +++ b/SKILL.md @@ -37,6 +37,11 @@ If `firefly` is not on PATH, run from the repo with `python -m firefly_cli ...` account is a HARD error that lists the candidates and exits 1. When that happens, read the candidates, pick the right one, and retry. NEVER guess an account, a wrong account moves real money. +- **Two accounts can share a name** (e.g. an expense and a revenue both named + "Nexi"); `--from`/`--to` then error as ambiguous. Target one by id with + `tx add --from-id ` / `--to-id ` (from the candidate list). Per side, + supply exactly one of the name flag or the id flag, never both; the two sides + are independent (`--from NAME --to-id 129` is fine). The id is validated. - **Categories and tags auto-create.** `--category NAME` and `--tags a,b` are passed straight to Firefly, which creates the category/tag if it does not exist. No resolution, no error on a new name. Reuse an existing name (see @@ -55,7 +60,7 @@ firefly account get firefly account balance [--at YYYY-MM-DD] firefly account create --type asset|expense|revenue [--opening-balance N] [--currency CODE] -firefly tx add --from --to +firefly tx add (--from | --from-id ) (--to | --to-id ) [--desc TEXT] [--date YYYY-MM-DD] [--category NAME] [--tags a,b] [--type T] [--dry-run] [--skip-dupes] firefly tx edit diff --git a/completions/firefly.bash b/completions/firefly.bash index 6901d14..e9dbe0b 100644 --- a/completions/firefly.bash +++ b/completions/firefly.bash @@ -40,7 +40,7 @@ _firefly() { "account balance") leaf_opts="--at";; "account create") leaf_opts="--currency --if-not-exists --opening-balance --type";; "account list") leaf_opts="--type";; - "tx add") leaf_opts="--category --date --desc --dry-run --from --skip-dupes --tags --to --type";; + "tx add") leaf_opts="--category --date --desc --dry-run --from --from-id --skip-dupes --tags --to --to-id --type";; "tx delete") leaf_opts="--yes";; "tx edit") leaf_opts="--amount --category --date --desc --from --tags --to --type";; "tx list") leaf_opts="--account --all --flat --limit --since --until";; diff --git a/firefly_cli/__init__.py b/firefly_cli/__init__.py index 27e214f..1ed8352 100644 --- a/firefly_cli/__init__.py +++ b/firefly_cli/__init__.py @@ -2,4 +2,4 @@ # Copyright (C) 2026 Danilo M. # Licensed under the GNU General Public License v2.0 only. -__version__ = "0.3.6" +__version__ = "0.3.7" diff --git a/firefly_cli/commands/transaction.py b/firefly_cli/commands/transaction.py index 714d2d4..3ce20f3 100644 --- a/firefly_cli/commands/transaction.py +++ b/firefly_cli/commands/transaction.py @@ -22,8 +22,12 @@ def _infer_type(src_type, dst_type): def _add_args(p): p.add_argument("amount") - p.add_argument("--from", dest="source", required=True, help="source account") - p.add_argument("--to", dest="dest", required=True, help="destination account") + p.add_argument("--from", dest="source", default=None, help="source account (name)") + p.add_argument("--to", dest="dest", default=None, help="destination account (name)") + p.add_argument("--from-id", dest="source_id", default=None, + help="source account by numeric id (disambiguates same-name accounts)") + p.add_argument("--to-id", dest="dest_id", default=None, + help="destination account by numeric id (disambiguates same-name accounts)") p.add_argument("--desc", default=None) p.add_argument("--date", default=None, help="YYYY-MM-DD (default today)") p.add_argument("--category", default=None) @@ -36,9 +40,18 @@ def _add_args(p): help="skip if a tx with same amount+date+source+destination exists") @registry.command("tx add", help="record a transaction; source/destination resolve to accounts, category/tags auto-create", args=_add_args) +def _resolve_side(ctx, name, acc_id, side): + # Exactly one of name/id per side (mutually exclusive). id path (ISSUES.md + # #2) fetches by id, validating existence; name path resolves as before. + if bool(name) == bool(acc_id): + raise FireflyError( + f"--{side}/--{side}-id: supply exactly one " + f"(got name={name!r}, id={acc_id!r})") + return ctx.resolver.account_by_id(acc_id) if acc_id else ctx.resolver.account(name) + def cmd_add(args, ctx): - src = ctx.resolver.account(args.source) - dst = ctx.resolver.account(args.dest) + src = _resolve_side(ctx, args.source, args.source_id, "from") + dst = _resolve_side(ctx, args.dest, args.dest_id, "to") ttype = args.type or _infer_type(src.get("type"), dst.get("type")) from datetime import date as _date split = { diff --git a/firefly_cli/resolver.py b/firefly_cli/resolver.py index 7673af6..2a73e79 100644 --- a/firefly_cli/resolver.py +++ b/firefly_cli/resolver.py @@ -28,6 +28,12 @@ class Resolver: def account(self, name): return self._match("account", self._list("/api/v1/accounts"), name) + def account_by_id(self, acc_id): + # Escape hatch for same-name accounts (ISSUES.md #2): GET the account + # directly; a bad id 404s and client.request surfaces a FireflyError. + item = self.client.request("GET", f"/api/v1/accounts/{acc_id}")["data"] + return {"id": item["id"], **item.get("attributes", {})} + def tag(self, name): return self._match("tag", self._list("/api/v1/tags"), name) diff --git a/pyproject.toml b/pyproject.toml index 86e8e62..a356bd8 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "firefly-iii-agent" -version = "0.3.6" +version = "0.3.7" description = "CLI tool for agent interaction with Firefly III" readme = "README.md" requires-python = ">=3.11" diff --git a/tests/unit/test_commands_transaction.py b/tests/unit/test_commands_transaction.py index 28882d3..5f455cb 100644 --- a/tests/unit/test_commands_transaction.py +++ b/tests/unit/test_commands_transaction.py @@ -19,7 +19,7 @@ class TestTxAdd(unittest.TestCase): "attributes": {}}} args = MagicMock(amount="42.50", source="Checking", dest="Groceries", desc="food", date="2026-06-30", category=None, - tags=None, type=None, dry_run=False, skip_dupes=False) + tags=None, type=None, dry_run=False, skip_dupes=False, source_id=None, dest_id=None) rc = tx.cmd_add(args, ctx) self.assertEqual(rc, 0) method, path = client.request.call_args[0][:2] @@ -40,7 +40,7 @@ class TestTxAdd(unittest.TestCase): client.request.return_value = {"data": {"id": "1", "attributes": {}}} args = MagicMock(amount="1000", source="Salary", dest="Checking", desc="pay", date=None, category=None, tags=None, - type=None, dry_run=False, skip_dupes=False) + type=None, dry_run=False, skip_dupes=False, source_id=None, dest_id=None) tx.cmd_add(args, ctx) self.assertEqual(client.request.call_args[1]["body"]["transactions"][0]["type"], "deposit") @@ -51,7 +51,7 @@ class TestTxAdd(unittest.TestCase): client.request.return_value = {"data": {"id": "1", "attributes": {}}} args = MagicMock(amount="5", source="A", dest="B", desc=None, date=None, category=None, tags="food,fun", type="transfer", - dry_run=False, skip_dupes=False) + dry_run=False, skip_dupes=False, source_id=None, dest_id=None) tx.cmd_add(args, ctx) split = client.request.call_args[1]["body"]["transactions"][0] self.assertEqual(split["type"], "transfer") @@ -64,7 +64,7 @@ class TestTxAdd(unittest.TestCase): client.request.return_value = {"data": {"id": "1", "attributes": {}}} args = MagicMock(amount="5", source="A", dest="B", desc=None, date=None, category="Brand New Cat", tags=None, type="withdrawal", - dry_run=False, skip_dupes=False) + dry_run=False, skip_dupes=False, source_id=None, dest_id=None) tx.cmd_add(args, ctx) split = client.request.call_args[1]["body"]["transactions"][0] self.assertEqual(split["category_name"], "Brand New Cat") @@ -73,7 +73,7 @@ class TestTxAdd(unittest.TestCase): ctx, client, resolver = make_ctx() resolver.account.side_effect = lambda n: {"id": "1", "type": "asset", "name": n} args = MagicMock(amount="5", source="A", dest="B", desc="x", date="2026-06-01", - category=None, tags=None, type="withdrawal", dry_run=True, skip_dupes=False) + category=None, tags=None, type="withdrawal", dry_run=True, skip_dupes=False, source_id=None, dest_id=None) rc = tx.cmd_add(args, ctx) self.assertEqual(rc, 0) client.request.assert_not_called() # accounts resolved, nothing written @@ -84,7 +84,7 @@ class TestTxAdd(unittest.TestCase): ctx, client, resolver = make_ctx() resolver.account.side_effect = ResolutionError('No account named "B"') args = MagicMock(amount="5", source="A", dest="B", desc=None, date=None, - category=None, tags=None, type="withdrawal", dry_run=True, skip_dupes=False) + category=None, tags=None, type="withdrawal", dry_run=True, skip_dupes=False, source_id=None, dest_id=None) with self.assertRaises(ResolutionError): tx.cmd_add(args, ctx) client.request.assert_not_called() @@ -101,7 +101,7 @@ class TestTxAdd(unittest.TestCase): {"id": "441", "attributes": {}}]} args = MagicMock(amount="9.99", source="A", dest="B", desc="x", date="2026-06-10", category=None, tags=None, - type=None, dry_run=False, skip_dupes=True) + type=None, dry_run=False, skip_dupes=True, source_id=None, dest_id=None) rc = tx.cmd_add(args, ctx) self.assertEqual(rc, 0) # exactly one call, the GET search; no POST @@ -127,7 +127,7 @@ class TestTxAdd(unittest.TestCase): ] args = MagicMock(amount="9.99", source="A", dest="B", desc="x", date="2026-06-10", category=None, tags=None, - type=None, dry_run=False, skip_dupes=True) + type=None, dry_run=False, skip_dupes=True, source_id=None, dest_id=None) rc = tx.cmd_add(args, ctx) self.assertEqual(rc, 0) self.assertEqual(client.request.call_count, 2) @@ -145,7 +145,7 @@ class TestTxAdd(unittest.TestCase): client.request.return_value = {"data": {"id": "1", "attributes": {}}} args = MagicMock(amount="100", source="BBVA", dest="Medio", desc=None, date=None, category=None, tags=None, type=None, - dry_run=False, skip_dupes=False) + dry_run=False, skip_dupes=False, source_id=None, dest_id=None) buf = io.StringIO() with redirect_stderr(buf): tx.cmd_add(args, ctx) @@ -158,7 +158,7 @@ class TestTxAdd(unittest.TestCase): resolver.account.side_effect = lambda n: {"id": "1", "type": "asset", "name": n} args = MagicMock(amount="5", source="A", dest="B", desc=None, date=None, category=None, tags=None, type="transfer", - dry_run=True, skip_dupes=False) + dry_run=True, skip_dupes=False, source_id=None, dest_id=None) buf = io.StringIO() with redirect_stderr(buf): tx.cmd_add(args, ctx) @@ -176,7 +176,7 @@ class TestTxAdd(unittest.TestCase): client.request.return_value = {"data": {"id": "1", "attributes": {}}} args = MagicMock(amount="5", source="Checking", dest="Groceries", desc=None, date=None, category=None, tags=None, type=None, - dry_run=False, skip_dupes=False) + dry_run=False, skip_dupes=False, source_id=None, dest_id=None) buf = io.StringIO() with redirect_stderr(buf): tx.cmd_add(args, ctx) @@ -187,11 +187,80 @@ class TestTxAdd(unittest.TestCase): resolver.account.side_effect = lambda n: {"id": "1", "type": "asset", "name": n} args = MagicMock(amount="5", source="A", dest="B", desc=None, date="2026-06-01", category=None, tags=None, type="withdrawal", - dry_run=True, skip_dupes=True) + dry_run=True, skip_dupes=True, source_id=None, dest_id=None) rc = tx.cmd_add(args, ctx) self.assertEqual(rc, 0) client.request.assert_not_called() # dry-run wins: no search, no write + def test_from_id_resolves_by_id_not_name(self): + # --to-id targets an ambiguous account by numeric id (ISSUES.md #2). + ctx, client, resolver = make_ctx() + resolver.account.side_effect = lambda n: { + "Checking": {"id": "1", "name": "Checking", "type": "asset"}}[n] + resolver.account_by_id.side_effect = lambda i: { + "129": {"id": "129", "name": "Nexi", "type": "revenue"}}[i] + client.request.return_value = {"data": {"id": "1", "attributes": {}}} + args = MagicMock(amount="5", source=None, source_id="129", dest="Checking", + dest_id=None, desc=None, date=None, category=None, + tags=None, type=None, dry_run=False, skip_dupes=False) + rc = tx.cmd_add(args, ctx) + self.assertEqual(rc, 0) + split = client.request.call_args[1]["body"]["transactions"][0] + self.assertEqual(split["source_id"], "129") + self.assertEqual(split["destination_id"], "1") + self.assertEqual(split["type"], "deposit") # revenue -> asset + resolver.account.assert_called_once_with("Checking") + + def test_missing_both_name_and_id_errors(self): + from firefly_cli.errors import FireflyError + ctx, client, resolver = make_ctx() + args = MagicMock(amount="5", source=None, source_id=None, dest="B", + dest_id=None, desc=None, date=None, category=None, + tags=None, type=None, dry_run=False, skip_dupes=False) + with self.assertRaises(FireflyError): + tx.cmd_add(args, ctx) + client.request.assert_not_called() + + def test_both_name_and_id_on_same_side_errors(self): + from firefly_cli.errors import FireflyError + ctx, client, resolver = make_ctx() + args = MagicMock(amount="5", source="A", source_id="7", dest="B", + dest_id=None, desc=None, date=None, category=None, + tags=None, type=None, dry_run=False, skip_dupes=False) + with self.assertRaises(FireflyError): + tx.cmd_add(args, ctx) + client.request.assert_not_called() + + def test_mixed_name_source_id_dest(self): + ctx, client, resolver = make_ctx() + resolver.account.side_effect = lambda n: { + "Checking": {"id": "1", "name": "Checking", "type": "asset"}}[n] + resolver.account_by_id.side_effect = lambda i: { + "52": {"id": "52", "name": "Nexi", "type": "expense"}}[i] + client.request.return_value = {"data": {"id": "1", "attributes": {}}} + args = MagicMock(amount="5", source="Checking", source_id=None, dest=None, + dest_id="52", desc=None, date=None, category=None, + tags=None, type=None, dry_run=False, skip_dupes=False) + rc = tx.cmd_add(args, ctx) + self.assertEqual(rc, 0) + split = client.request.call_args[1]["body"]["transactions"][0] + self.assertEqual(split["source_id"], "1") + self.assertEqual(split["destination_id"], "52") + self.assertEqual(split["type"], "withdrawal") # asset -> expense + + def test_bad_id_surfaces_error(self): + from firefly_cli.errors import FireflyError + ctx, client, resolver = make_ctx() + resolver.account_by_id.side_effect = FireflyError("404 not found") + resolver.account.side_effect = lambda n: {"id": "1", "type": "asset", "name": n} + args = MagicMock(amount="5", source="A", source_id=None, dest=None, + dest_id="9999", desc=None, date=None, category=None, + tags=None, type=None, dry_run=False, skip_dupes=False) + with self.assertRaises(FireflyError): + tx.cmd_add(args, ctx) + client.request.assert_not_called() + + class TestTxEdit(unittest.TestCase): def test_edit_sends_only_provided_fields(self): ctx, client, resolver = make_ctx() diff --git a/tests/unit/test_resolver.py b/tests/unit/test_resolver.py index 2e00e83..82ac30b 100644 --- a/tests/unit/test_resolver.py +++ b/tests/unit/test_resolver.py @@ -41,3 +41,13 @@ class TestResolver(unittest.TestCase): r.account("Cash") self.assertIn("3", str(ctx.exception)) self.assertIn("9", str(ctx.exception)) + + def test_account_by_id_fetches_single(self): + c = MagicMock() + c.request.return_value = {"data": { + "id": "129", "type": "accounts", + "attributes": {"name": "Nexi", "type": "revenue"}}} + r = Resolver(c) + acc = r.account_by_id("129") + self.assertEqual(acc, {"id": "129", "name": "Nexi", "type": "revenue"}) + c.request.assert_called_once_with("GET", "/api/v1/accounts/129") -- cgit v1.2.3