From 66e3bc482fa686e8d1c11ba97e096b15991370af Mon Sep 17 00:00:00 2001 From: David Gil Date: Sat, 23 May 2026 23:21:36 +0200 Subject: [PATCH] refactor: consolidate paging/print helpers + split buildTagPayload + close test gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to fix/api-error-envelope-and-safety. Now that every print* helper goes through the same respond() path, the per-resource families (investment_helpers, financial_cmds, family_exports, users, imports paging) are byte-identical except for the prefix in the function name. Collapse them. ## Helper consolidation - Delete printInvestmentGet/Post/Patch/Delete/DryRun (investment_helpers.go:55-112). All callers in investments_cmds.go + trades_cmd.go + holdings_cmd.go migrate to printGet / dispatchWrite. The only thing kept from that file is addRepeatedQuery (renamed from addRepeatedInvestmentQuery — the helper is generic). - Delete printFinancialGet/Post/Patch/DryRun (financial_cmds.go:260-310). Valuations create/update migrate to dispatchWrite; everything else goes through printGet. - Delete printFamilyExportGet/DryRun (family_exports_cmd.go:90-111). list/show migrate to printGet; create migrates to dispatchWrite. Preserved the no-body dry-run shape upstream relies on. - Delete printUsersGet/Delete/DryRun (users_cmd.go:57-86). reset / reset status / delete-me migrate to dispatchWrite + printGet. - Delete addImportPagingFlags/Query, importPathWithQuery (imports_cmd.go:266-291) — same shape as the reference_cmds.go trio, no behavioural delta. - Delete addInvestmentPagingFlags/Query, investmentPathWithQuery (investment_helpers.go:12-39). - Delete addFinancialPagingFlags/Query, financialPathWithQuery (financial_cmds.go:231-258). ## Pattern fix — buildTagPayload flag argument Split buildTagPayload(o, requireName bool) at reference_cmds.go:233 into separate buildTagCreatePayload (rejects empty Name) and buildTagUpdatePayload (rejects "no fields provided to update"). Matches the chats / valuations / trades / categories pattern of distinct create/update builders. Two 10-line funcs instead of one 12-line func with a magic boolean. ## Test helpers hoisted - New testhelpers_test.go with findSub + captureStdout. Both were hand-rolled in transfers_cmds_test.go and dispatch_write_test.go respectively but used across the whole package. Centralizing them avoids the "where does findSub live?" confusion called out in the refactor review. ## Test gaps closed - New orphan_commands_test.go with a registration smoke table for whoami, sync, refresh, login, status, export[transactions], transactions delete, insights[subscriptions/fees/leaks], plan[budget/runway/forecast], propose[rules]. Previously each of those commands had zero test coverage — deleting the AddCommand line in root.go would have silently still compiled. Now it fails fast. - reference_cmds_test.go: replaced the single TestBuildTagPayload with four focused tests covering create-required-name, create-color-optional, update-needs-at-least-one-field, update-partial-ok. Also tightened TestReferenceCommandsRegistered to compare resolved cmd.Name() to the expected leaf (cobra's loose Find pattern that bit us before). ## Coverage cmd/sure-cli/root: 47.6% → 52.9% (+5.3pp). go test ./... -race -cover green across the suite. ## Diff scope 14 files changed, +233/-391 = net **−158 LOC**. Pure mechanical collapse — every migrated site already had behavioural test coverage via the previous PR's respond() tests or the existing command-shape tests, and no functional change is intended. Refs the post-merge code review. --- cmd/sure-cli/root/dispatch_write_test.go | 29 ------- cmd/sure-cli/root/family_exports_cmd.go | 54 +++---------- cmd/sure-cli/root/financial_cmds.go | 92 ++++------------------- cmd/sure-cli/root/holdings_cmd.go | 10 +-- cmd/sure-cli/root/imports_cmd.go | 42 ++--------- cmd/sure-cli/root/investment_helpers.go | 76 +------------------ cmd/sure-cli/root/investments_cmds.go | 45 ++++------- cmd/sure-cli/root/orphan_commands_test.go | 46 ++++++++++++ cmd/sure-cli/root/reference_cmds.go | 18 +++-- cmd/sure-cli/root/reference_cmds_test.go | 72 ++++++++++++++---- cmd/sure-cli/root/testhelpers_test.go | 49 ++++++++++++ cmd/sure-cli/root/trades_cmd.go | 31 ++------ cmd/sure-cli/root/transfers_cmds_test.go | 11 --- cmd/sure-cli/root/users_cmd.go | 49 +----------- 14 files changed, 233 insertions(+), 391 deletions(-) create mode 100644 cmd/sure-cli/root/orphan_commands_test.go create mode 100644 cmd/sure-cli/root/testhelpers_test.go diff --git a/cmd/sure-cli/root/dispatch_write_test.go b/cmd/sure-cli/root/dispatch_write_test.go index 96149b8..0290d4a 100644 --- a/cmd/sure-cli/root/dispatch_write_test.go +++ b/cmd/sure-cli/root/dispatch_write_test.go @@ -1,39 +1,10 @@ package root import ( - "bytes" "encoding/json" - "io" - "os" "testing" ) -// captureStdout swaps os.Stdout for a pipe, runs fn, and returns whatever was -// written. It restores the original Stdout afterwards. -func captureStdout(t *testing.T, fn func()) string { - t.Helper() - orig := os.Stdout - r, w, err := os.Pipe() - if err != nil { - t.Fatalf("pipe: %v", err) - } - os.Stdout = w - - done := make(chan struct{}) - var buf bytes.Buffer - go func() { - _, _ = io.Copy(&buf, r) - close(done) - }() - - fn() - - _ = w.Close() - <-done - os.Stdout = orig - return buf.String() -} - func TestDispatchWrite_DryRun_POST(t *testing.T) { out := captureStdout(t, func() { // Reset to json format so the envelope is parseable. diff --git a/cmd/sure-cli/root/family_exports_cmd.go b/cmd/sure-cli/root/family_exports_cmd.go index 24a5bbd..23dee78 100644 --- a/cmd/sure-cli/root/family_exports_cmd.go +++ b/cmd/sure-cli/root/family_exports_cmd.go @@ -5,6 +5,7 @@ import ( "net/url" "github.com/spf13/cobra" + "github.com/we-promise/sure-cli/internal/api" "github.com/we-promise/sure-cli/internal/output" ) @@ -18,21 +19,11 @@ func newFamilyExportsCmd() *cobra.Command { Short: "List family exports", Run: func(cmd *cobra.Command, args []string) { q := url.Values{} - if page > 0 { - q.Set("page", fmt.Sprintf("%d", page)) - } - if perPage > 0 { - q.Set("per_page", fmt.Sprintf("%d", perPage)) - } - path := "/api/v1/family_exports" - if encoded := q.Encode(); encoded != "" { - path = path + "?" + encoded - } - printFamilyExportGet(path) + addPagingQuery(q, page, perPage) + printGet(pathWithQuery("/api/v1/family_exports", q)) }, } - list.Flags().IntVar(&page, "page", 1, "page number") - list.Flags().IntVar(&perPage, "per-page", 25, "items per page (maps to per_page)") + addPagingFlags(list, &page, &perPage) cmd.AddCommand(list) cmd.AddCommand(&cobra.Command{ @@ -40,7 +31,7 @@ func newFamilyExportsCmd() *cobra.Command { Short: "Show family export", Args: cobra.ExactArgs(1), Run: func(cmd *cobra.Command, args []string) { - printFamilyExportGet(fmt.Sprintf("/api/v1/family_exports/%s", url.PathEscape(args[0]))) + printGet(fmt.Sprintf("/api/v1/family_exports/%s", url.PathEscape(args[0]))) }, }) @@ -49,15 +40,15 @@ func newFamilyExportsCmd() *cobra.Command { Use: "create", Short: "Queue a family export (default dry-run; use --apply to execute)", Run: func(cmd *cobra.Command, args []string) { - path := "/api/v1/family_exports" + // Upstream Api::V1::FamilyExportsController#create ignores the body + // (it queues a job per current_resource_owner.family); send {} on + // apply and pass nil for dry-run to keep the previous envelope + // shape (no body key under request). if !apply { - printFamilyExportDryRun("POST", path, nil) + dispatchWrite(apply, "POST", "/api/v1/family_exports", nil) return } - client := api.New() - var res any - r, err := client.Post(path, map[string]any{}, &res) - respond(r, err, res) + dispatchWrite(true, "POST", "/api/v1/family_exports", map[string]any{}) }, } create.Flags().BoolVar(&apply, "apply", false, "execute the create (otherwise dry-run)") @@ -86,26 +77,3 @@ func newFamilyExportsCmd() *cobra.Command { return cmd } - -func printFamilyExportGet(path string) { - client := api.New() - var res any - r, err := client.Get(path, &res) - respond(r, err, res) -} - -func printFamilyExportDryRun(method, path string, body any) { - request := map[string]any{ - "method": method, - "path": path, - } - if body != nil { - request["body"] = body - } - if err := output.Print(format, output.Envelope{Data: map[string]any{ - "dry_run": true, - "request": request, - }}); err != nil { - output.Fail("output_failed", err.Error(), nil) - } -} diff --git a/cmd/sure-cli/root/financial_cmds.go b/cmd/sure-cli/root/financial_cmds.go index d6b3055..cb002f8 100644 --- a/cmd/sure-cli/root/financial_cmds.go +++ b/cmd/sure-cli/root/financial_cmds.go @@ -6,7 +6,7 @@ import ( "time" "github.com/spf13/cobra" - "github.com/we-promise/sure-cli/internal/api" + "github.com/we-promise/sure-cli/internal/output" ) @@ -16,7 +16,7 @@ func newBalanceSheetCmd() *cobra.Command { Use: "show", Short: "Show balance sheet", Run: func(cmd *cobra.Command, args []string) { - printFinancialGet("/api/v1/balance_sheet") + printGet("/api/v1/balance_sheet") }, }) return cmd @@ -32,7 +32,7 @@ func newBalancesCmd() *cobra.Command { Short: "List balance history records", Run: func(cmd *cobra.Command, args []string) { q := url.Values{} - addFinancialPagingQuery(q, page, perPage) + addPagingQuery(q, page, perPage) if accountID != "" { q.Set("account_id", accountID) } @@ -45,10 +45,10 @@ func newBalancesCmd() *cobra.Command { if endDate != "" { q.Set("end_date", endDate) } - printFinancialGet(financialPathWithQuery("/api/v1/balances", q)) + printGet(pathWithQuery("/api/v1/balances", q)) }, } - addFinancialPagingFlags(list, &page, &perPage) + addPagingFlags(list, &page, &perPage) list.Flags().StringVar(&accountID, "account-id", "", "account id") list.Flags().StringVar(¤cy, "currency", "", "currency") list.Flags().StringVar(&startDate, "start-date", "", "start date (YYYY-MM-DD)") @@ -60,7 +60,7 @@ func newBalancesCmd() *cobra.Command { Short: "Show balance history record", Args: cobra.ExactArgs(1), Run: func(cmd *cobra.Command, args []string) { - printFinancialGet(fmt.Sprintf("/api/v1/balances/%s", url.PathEscape(args[0]))) + printGet(fmt.Sprintf("/api/v1/balances/%s", url.PathEscape(args[0]))) }, }) return cmd @@ -72,7 +72,7 @@ func newFamilySettingsCmd() *cobra.Command { Use: "show", Short: "Show family settings", Run: func(cmd *cobra.Command, args []string) { - printFinancialGet("/api/v1/family_settings") + printGet("/api/v1/family_settings") }, }) return cmd @@ -104,7 +104,7 @@ func newValuationsCmd() *cobra.Command { Short: "List valuations", Run: func(cmd *cobra.Command, args []string) { q := url.Values{} - addFinancialPagingQuery(q, page, perPage) + addPagingQuery(q, page, perPage) if accountID != "" { q.Set("account_id", accountID) } @@ -114,10 +114,10 @@ func newValuationsCmd() *cobra.Command { if endDate != "" { q.Set("end_date", endDate) } - printFinancialGet(financialPathWithQuery("/api/v1/valuations", q)) + printGet(pathWithQuery("/api/v1/valuations", q)) }, } - addFinancialPagingFlags(list, &page, &perPage) + addPagingFlags(list, &page, &perPage) list.Flags().StringVar(&accountID, "account-id", "", "account id") list.Flags().StringVar(&startDate, "start-date", "", "start date (YYYY-MM-DD)") list.Flags().StringVar(&endDate, "end-date", "", "end date (YYYY-MM-DD)") @@ -128,7 +128,7 @@ func newValuationsCmd() *cobra.Command { Short: "Show valuation", Args: cobra.ExactArgs(1), Run: func(cmd *cobra.Command, args []string) { - printFinancialGet(fmt.Sprintf("/api/v1/valuations/%s", url.PathEscape(args[0]))) + printGet(fmt.Sprintf("/api/v1/valuations/%s", url.PathEscape(args[0]))) }, }) @@ -146,13 +146,9 @@ func newValuationsCreateCmd() *cobra.Command { payload, err := buildValuationCreatePayload(o) if err != nil { output.Fail("validation_failed", err.Error(), nil) - } - path := "/api/v1/valuations" - if !o.Apply { - printFinancialDryRun("POST", path, payload) return } - printFinancialPost(path, payload) + dispatchWrite(o.Apply, "POST", "/api/v1/valuations", payload) }, } cmd.Flags().StringVar(&o.AccountID, "account-id", "", "account id (required)") @@ -174,13 +170,9 @@ func newValuationsUpdateCmd() *cobra.Command { payload, err := buildValuationUpdatePayload(o) if err != nil { output.Fail("validation_failed", err.Error(), nil) - } - path := fmt.Sprintf("/api/v1/valuations/%s", url.PathEscape(args[0])) - if !o.Apply { - printFinancialDryRun("PATCH", path, payload) return } - printFinancialPatch(path, payload) + dispatchWrite(o.Apply, "PATCH", fmt.Sprintf("/api/v1/valuations/%s", url.PathEscape(args[0])), payload) }, } cmd.Flags().StringVar(&o.Amount, "amount", "", "valuation amount") @@ -235,61 +227,3 @@ func buildValuationUpdatePayload(o valuationUpdateOpts) (map[string]any, error) } return map[string]any{"valuation": valuation}, nil } - -func addFinancialPagingFlags(cmd *cobra.Command, page, perPage *int) { - cmd.Flags().IntVar(page, "page", 1, "page number") - cmd.Flags().IntVar(perPage, "per-page", 25, "items per page (maps to per_page)") -} - -func addFinancialPagingQuery(q url.Values, page, perPage int) { - if page > 0 { - q.Set("page", fmt.Sprintf("%d", page)) - } - if perPage > 0 { - q.Set("per_page", fmt.Sprintf("%d", perPage)) - } -} - -func financialPathWithQuery(path string, q url.Values) string { - if encoded := q.Encode(); encoded != "" { - return path + "?" + encoded - } - return path -} - -func printFinancialGet(path string) { - client := api.New() - var res any - r, err := client.Get(path, &res) - respond(r, err, res) -} - -func printFinancialPost(path string, body any) { - client := api.New() - var res any - r, err := client.Post(path, body, &res) - respond(r, err, res) -} - -func printFinancialPatch(path string, body any) { - client := api.New() - var res any - r, err := client.Patch(path, body, &res) - respond(r, err, res) -} - -func printFinancialDryRun(method, path string, body any) { - request := map[string]any{ - "method": method, - "path": path, - } - if body != nil { - request["body"] = body - } - if err := output.Print(format, output.Envelope{Data: map[string]any{ - "dry_run": true, - "request": request, - }}); err != nil { - output.Fail("output_failed", err.Error(), nil) - } -} diff --git a/cmd/sure-cli/root/holdings_cmd.go b/cmd/sure-cli/root/holdings_cmd.go index 8f4eef3..092bed9 100644 --- a/cmd/sure-cli/root/holdings_cmd.go +++ b/cmd/sure-cli/root/holdings_cmd.go @@ -15,7 +15,7 @@ func newHoldingsCmd() *cobra.Command { Short: "Show investment holding", Args: cobra.ExactArgs(1), Run: func(cmd *cobra.Command, args []string) { - printInvestmentGet(fmt.Sprintf("/api/v1/holdings/%s", url.PathEscape(args[0]))) + printGet(fmt.Sprintf("/api/v1/holdings/%s", url.PathEscape(args[0]))) }, }) return cmd @@ -30,11 +30,11 @@ func newHoldingsListCmd() *cobra.Command { Short: "List investment holdings", Run: func(cmd *cobra.Command, args []string) { q := url.Values{} - addInvestmentPagingQuery(q, page, perPage) + addPagingQuery(q, page, perPage) if accountID != "" { q.Set("account_id", accountID) } - addRepeatedInvestmentQuery(q, "account_ids", accountIDs) + addRepeatedQuery(q, "account_ids", accountIDs) if date != "" { q.Set("date", date) } @@ -47,10 +47,10 @@ func newHoldingsListCmd() *cobra.Command { if securityID != "" { q.Set("security_id", securityID) } - printInvestmentGet(investmentPathWithQuery("/api/v1/holdings", q)) + printGet(pathWithQuery("/api/v1/holdings", q)) }, } - addInvestmentPagingFlags(cmd, &page, &perPage) + addPagingFlags(cmd, &page, &perPage) cmd.Flags().StringVar(&accountID, "account-id", "", "account id") cmd.Flags().StringSliceVar(&accountIDs, "account-ids", nil, "account ids (repeat or comma-separated)") cmd.Flags().StringVar(&date, "date", "", "exact holding date (YYYY-MM-DD)") diff --git a/cmd/sure-cli/root/imports_cmd.go b/cmd/sure-cli/root/imports_cmd.go index 8def7f0..4db142f 100644 --- a/cmd/sure-cli/root/imports_cmd.go +++ b/cmd/sure-cli/root/imports_cmd.go @@ -61,14 +61,14 @@ func newImportsCmd() *cobra.Command { if importType != "" { q.Set("type", importType) } - addImportPagingQuery(q, page, perPage) - printImportGet(importPathWithQuery("/api/v1/imports", q)) + addPagingQuery(q, page, perPage) + printGet(pathWithQuery("/api/v1/imports", q)) }, } list.Flags().StringVar(&status, "status", "", "filter by status") list.Flags().StringVar(&importType, "type", "", "filter by import type") - addImportPagingFlags(list, &page, &perPage) + addPagingFlags(list, &page, &perPage) cmd.AddCommand(list) cmd.AddCommand(&cobra.Command{ @@ -76,7 +76,7 @@ func newImportsCmd() *cobra.Command { Short: "Show import", Args: cobra.ExactArgs(1), Run: func(cmd *cobra.Command, args []string) { - printImportGet(fmt.Sprintf("/api/v1/imports/%s", url.PathEscape(args[0]))) + printGet(fmt.Sprintf("/api/v1/imports/%s", url.PathEscape(args[0]))) }, }) @@ -96,11 +96,11 @@ func newImportsRowsCmd() *cobra.Command { Args: cobra.ExactArgs(1), Run: func(cmd *cobra.Command, args []string) { q := url.Values{} - addImportPagingQuery(q, page, perPage) - printImportGet(importPathWithQuery(fmt.Sprintf("/api/v1/imports/%s/rows", url.PathEscape(args[0])), q)) + addPagingQuery(q, page, perPage) + printGet(pathWithQuery(fmt.Sprintf("/api/v1/imports/%s/rows", url.PathEscape(args[0])), q)) }, } - addImportPagingFlags(cmd, &page, &perPage) + addPagingFlags(cmd, &page, &perPage) return cmd } @@ -262,31 +262,3 @@ func addImportField(fields map[string]string, name, value string) { fields[name] = value } } - -func addImportPagingFlags(cmd *cobra.Command, page, perPage *int) { - cmd.Flags().IntVar(page, "page", 1, "page number") - cmd.Flags().IntVar(perPage, "per-page", 25, "items per page (maps to per_page)") -} - -func addImportPagingQuery(q url.Values, page, perPage int) { - if page > 0 { - q.Set("page", fmt.Sprintf("%d", page)) - } - if perPage > 0 { - q.Set("per_page", fmt.Sprintf("%d", perPage)) - } -} - -func importPathWithQuery(path string, q url.Values) string { - if encoded := q.Encode(); encoded != "" { - return path + "?" + encoded - } - return path -} - -func printImportGet(path string) { - client := api.New() - var res any - r, err := client.Get(path, &res) - respond(r, err, res) -} diff --git a/cmd/sure-cli/root/investment_helpers.go b/cmd/sure-cli/root/investment_helpers.go index bbbadeb..3c42fd4 100644 --- a/cmd/sure-cli/root/investment_helpers.go +++ b/cmd/sure-cli/root/investment_helpers.go @@ -1,84 +1,16 @@ package root import ( - "fmt" "net/url" - - "github.com/spf13/cobra" - "github.com/we-promise/sure-cli/internal/api" - "github.com/we-promise/sure-cli/internal/output" ) -func addInvestmentPagingFlags(cmd *cobra.Command, page, perPage *int) { - cmd.Flags().IntVar(page, "page", 1, "page number") - cmd.Flags().IntVar(perPage, "per-page", 25, "items per page (maps to per_page)") -} - -func addInvestmentPagingQuery(q url.Values, page, perPage int) { - if page > 0 { - q.Set("page", fmt.Sprintf("%d", page)) - } - if perPage > 0 { - q.Set("per_page", fmt.Sprintf("%d", perPage)) - } -} - -func addRepeatedInvestmentQuery(q url.Values, key string, values []string) { +// addRepeatedQuery appends q[key][]=v for each non-empty v. Used by trades +// and holdings commands that accept --account-ids etc. Lives here for +// historical reasons; safe to call from any command file. +func addRepeatedQuery(q url.Values, key string, values []string) { for _, v := range values { if v != "" { q.Add(key+"[]", v) } } } - -func investmentPathWithQuery(path string, q url.Values) string { - if encoded := q.Encode(); encoded != "" { - return path + "?" + encoded - } - return path -} - -func printInvestmentGet(path string) { - client := api.New() - var res any - r, err := client.Get(path, &res) - respond(r, err, res) -} - -func printInvestmentPost(path string, body any) { - client := api.New() - var res any - r, err := client.Post(path, body, &res) - respond(r, err, res) -} - -func printInvestmentPatch(path string, body any) { - client := api.New() - var res any - r, err := client.Patch(path, body, &res) - respond(r, err, res) -} - -func printInvestmentDelete(path string) { - client := api.New() - var res any - r, err := client.Delete(path, &res) - respond(r, err, res) -} - -func printInvestmentDryRun(method, path string, body any) { - request := map[string]any{ - "method": method, - "path": path, - } - if body != nil { - request["body"] = body - } - if err := output.Print(format, output.Envelope{Data: map[string]any{ - "dry_run": true, - "request": request, - }}); err != nil { - output.Fail("output_failed", err.Error(), nil) - return - } -} diff --git a/cmd/sure-cli/root/investments_cmds.go b/cmd/sure-cli/root/investments_cmds.go index 819709b..598aa9d 100644 --- a/cmd/sure-cli/root/investments_cmds.go +++ b/cmd/sure-cli/root/investments_cmds.go @@ -18,7 +18,7 @@ func newSecuritiesCmd() *cobra.Command { Short: "List securities", Run: func(cmd *cobra.Command, args []string) { q := url.Values{} - addInvestmentPagingQuery(q, page, perPage) + addPagingQuery(q, page, perPage) if ticker != "" { q.Set("ticker", ticker) } @@ -31,10 +31,10 @@ func newSecuritiesCmd() *cobra.Command { if offline != "" { q.Set("offline", offline) } - printInvestmentGet(investmentPathWithQuery("/api/v1/securities", q)) + printGet(pathWithQuery("/api/v1/securities", q)) }, } - addInvestmentPagingFlags(list, &page, &perPage) + addPagingFlags(list, &page, &perPage) list.Flags().StringVar(&ticker, "ticker", "", "ticker filter") list.Flags().StringVar(&exchangeOperatingMIC, "exchange-operating-mic", "", "exchange operating MIC filter") list.Flags().StringVar(&kind, "kind", "", "security kind filter") @@ -46,7 +46,7 @@ func newSecuritiesCmd() *cobra.Command { Short: "Show security", Args: cobra.ExactArgs(1), Run: func(cmd *cobra.Command, args []string) { - printInvestmentGet(fmt.Sprintf("/api/v1/securities/%s", url.PathEscape(args[0]))) + printGet(fmt.Sprintf("/api/v1/securities/%s", url.PathEscape(args[0]))) }, }) return cmd @@ -62,7 +62,7 @@ func newSecurityPricesCmd() *cobra.Command { Short: "List security price history", Run: func(cmd *cobra.Command, args []string) { q := url.Values{} - addInvestmentPagingQuery(q, page, perPage) + addPagingQuery(q, page, perPage) if securityID != "" { q.Set("security_id", securityID) } @@ -78,10 +78,10 @@ func newSecurityPricesCmd() *cobra.Command { if provisional != "" { q.Set("provisional", provisional) } - printInvestmentGet(investmentPathWithQuery("/api/v1/security_prices", q)) + printGet(pathWithQuery("/api/v1/security_prices", q)) }, } - addInvestmentPagingFlags(list, &page, &perPage) + addPagingFlags(list, &page, &perPage) list.Flags().StringVar(&securityID, "security-id", "", "security id") list.Flags().StringVar(¤cy, "currency", "", "currency") list.Flags().StringVar(&startDate, "start-date", "", "start date (YYYY-MM-DD)") @@ -94,7 +94,7 @@ func newSecurityPricesCmd() *cobra.Command { Short: "Show security price", Args: cobra.ExactArgs(1), Run: func(cmd *cobra.Command, args []string) { - printInvestmentGet(fmt.Sprintf("/api/v1/security_prices/%s", url.PathEscape(args[0]))) + printGet(fmt.Sprintf("/api/v1/security_prices/%s", url.PathEscape(args[0]))) }, }) return cmd @@ -135,17 +135,17 @@ func newRecurringTransactionsCmd() *cobra.Command { Short: "List recurring transactions", Run: func(cmd *cobra.Command, args []string) { q := url.Values{} - addInvestmentPagingQuery(q, page, perPage) + addPagingQuery(q, page, perPage) if status != "" { q.Set("status", status) } if accountID != "" { q.Set("account_id", accountID) } - printInvestmentGet(investmentPathWithQuery("/api/v1/recurring_transactions", q)) + printGet(pathWithQuery("/api/v1/recurring_transactions", q)) }, } - addInvestmentPagingFlags(list, &page, &perPage) + addPagingFlags(list, &page, &perPage) list.Flags().StringVar(&status, "status", "", "status filter") list.Flags().StringVar(&accountID, "account-id", "", "account id filter") cmd.AddCommand(list) @@ -155,7 +155,7 @@ func newRecurringTransactionsCmd() *cobra.Command { Short: "Show recurring transaction", Args: cobra.ExactArgs(1), Run: func(cmd *cobra.Command, args []string) { - printInvestmentGet(fmt.Sprintf("/api/v1/recurring_transactions/%s", url.PathEscape(args[0]))) + printGet(fmt.Sprintf("/api/v1/recurring_transactions/%s", url.PathEscape(args[0]))) }, }) cmd.AddCommand(newRecurringTransactionsCreateCmd()) @@ -175,12 +175,7 @@ func newRecurringTransactionsCreateCmd() *cobra.Command { output.Fail("validation_failed", err.Error(), nil) return } - path := "/api/v1/recurring_transactions" - if !o.Apply { - printInvestmentDryRun("POST", path, payload) - return - } - printInvestmentPost(path, payload) + dispatchWrite(o.Apply, "POST", "/api/v1/recurring_transactions", payload) }, } cmd.Flags().StringVar(&o.Name, "name", "", "name") @@ -213,12 +208,7 @@ func newRecurringTransactionsUpdateCmd() *cobra.Command { output.Fail("validation_failed", err.Error(), nil) return } - path := fmt.Sprintf("/api/v1/recurring_transactions/%s", url.PathEscape(args[0])) - if !o.Apply { - printInvestmentDryRun("PATCH", path, payload) - return - } - printInvestmentPatch(path, payload) + dispatchWrite(o.Apply, "PATCH", fmt.Sprintf("/api/v1/recurring_transactions/%s", url.PathEscape(args[0])), payload) }, } cmd.Flags().StringVar(&o.Status, "status", "", "status") @@ -235,12 +225,7 @@ func newRecurringTransactionsDeleteCmd() *cobra.Command { Short: "Delete recurring transaction (default dry-run; use --apply to execute)", Args: cobra.ExactArgs(1), Run: func(cmd *cobra.Command, args []string) { - path := fmt.Sprintf("/api/v1/recurring_transactions/%s", url.PathEscape(args[0])) - if !apply { - printInvestmentDryRun("DELETE", path, nil) - return - } - printInvestmentDelete(path) + dispatchWrite(apply, "DELETE", fmt.Sprintf("/api/v1/recurring_transactions/%s", url.PathEscape(args[0])), nil) }, } cmd.Flags().BoolVar(&apply, "apply", false, "execute the delete (otherwise dry-run)") diff --git a/cmd/sure-cli/root/orphan_commands_test.go b/cmd/sure-cli/root/orphan_commands_test.go new file mode 100644 index 0000000..9db6b43 --- /dev/null +++ b/cmd/sure-cli/root/orphan_commands_test.go @@ -0,0 +1,46 @@ +package root + +import "testing" + +// TestOrphanCommandsRegistered locks in registration for top-level commands +// that previously had no test file at all. Deleting the AddCommand line for +// any of these in root.go would silently still compile; this table makes +// such regressions fail fast. +// +// Tracks the test-review finding: whoami / sync / refresh / transactions +// delete / insights_* / plan_* were orphans. +func TestOrphanCommandsRegistered(t *testing.T) { + root := New() + cases := []struct { + path []string + want string + }{ + {[]string{"whoami"}, "whoami"}, + {[]string{"sync"}, "sync"}, + {[]string{"refresh"}, "refresh"}, + {[]string{"login"}, "login"}, + {[]string{"status"}, "status"}, + {[]string{"export"}, "export"}, + {[]string{"export", "transactions"}, "transactions"}, + {[]string{"transactions", "delete"}, "delete"}, + {[]string{"insights"}, "insights"}, + {[]string{"insights", "subscriptions"}, "subscriptions"}, + {[]string{"insights", "fees"}, "fees"}, + {[]string{"insights", "leaks"}, "leaks"}, + {[]string{"plan"}, "plan"}, + {[]string{"plan", "budget"}, "budget"}, + {[]string{"plan", "runway"}, "runway"}, + {[]string{"plan", "forecast"}, "forecast"}, + {[]string{"propose"}, "propose"}, + {[]string{"propose", "rules"}, "rules"}, + } + for _, c := range cases { + got, _, err := root.Find(c.path) + if err != nil { + t.Fatalf("path %v not registered: %v", c.path, err) + } + if got.Name() != c.want { + t.Fatalf("path %v resolved to %q, want %q", c.path, got.Name(), c.want) + } + } +} diff --git a/cmd/sure-cli/root/reference_cmds.go b/cmd/sure-cli/root/reference_cmds.go index 7c16e46..1363586 100644 --- a/cmd/sure-cli/root/reference_cmds.go +++ b/cmd/sure-cli/root/reference_cmds.go @@ -183,7 +183,7 @@ func newTagsCreateCmd() *cobra.Command { Use: "create", Short: "Create tag (default dry-run; use --apply to execute)", Run: func(cmd *cobra.Command, args []string) { - payload, err := buildTagPayload(o, true) + payload, err := buildTagCreatePayload(o) if err != nil { failValidation(err) } @@ -203,7 +203,7 @@ func newTagsUpdateCmd() *cobra.Command { Short: "Update tag (default dry-run; use --apply to execute)", Args: cobra.ExactArgs(1), Run: func(cmd *cobra.Command, args []string) { - payload, err := buildTagPayload(o, false) + payload, err := buildTagUpdatePayload(o) if err != nil { failValidation(err) } @@ -230,10 +230,18 @@ func newTagsDeleteCmd() *cobra.Command { return cmd } -func buildTagPayload(o tagWriteOpts, requireName bool) (map[string]any, error) { - if requireName && o.Name == "" { +func buildTagCreatePayload(o tagWriteOpts) (map[string]any, error) { + if o.Name == "" { return nil, fmt.Errorf("name is required") } + tag := map[string]any{"name": o.Name} + if o.Color != "" { + tag["color"] = o.Color + } + return map[string]any{"tag": tag}, nil +} + +func buildTagUpdatePayload(o tagWriteOpts) (map[string]any, error) { tag := map[string]any{} if o.Name != "" { tag["name"] = o.Name @@ -242,7 +250,7 @@ func buildTagPayload(o tagWriteOpts, requireName bool) (map[string]any, error) { tag["color"] = o.Color } if len(tag) == 0 { - return nil, fmt.Errorf("no fields provided") + return nil, fmt.Errorf("no fields provided to update") } return map[string]any{"tag": tag}, nil } diff --git a/cmd/sure-cli/root/reference_cmds_test.go b/cmd/sure-cli/root/reference_cmds_test.go index 73d6370..10c1713 100644 --- a/cmd/sure-cli/root/reference_cmds_test.go +++ b/cmd/sure-cli/root/reference_cmds_test.go @@ -2,11 +2,11 @@ package root import "testing" -func TestBuildTagPayload_CreateRequiresName(t *testing.T) { - if _, err := buildTagPayload(tagWriteOpts{}, true); err == nil { +func TestBuildTagCreatePayload_RequiresName(t *testing.T) { + if _, err := buildTagCreatePayload(tagWriteOpts{}); err == nil { t.Fatal("expected missing name error") } - payload, err := buildTagPayload(tagWriteOpts{Name: "Travel", Color: "#3b82f6"}, true) + payload, err := buildTagCreatePayload(tagWriteOpts{Name: "Travel", Color: "#3b82f6"}) if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -19,17 +19,63 @@ func TestBuildTagPayload_CreateRequiresName(t *testing.T) { } } +func TestBuildTagCreatePayload_ColorOptional(t *testing.T) { + payload, err := buildTagCreatePayload(tagWriteOpts{Name: "Travel"}) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + tag := payload["tag"].(map[string]any) + if _, has := tag["color"]; has { + t.Fatalf("color should be omitted when empty, got %#v", tag) + } +} + +func TestBuildTagUpdatePayload_NeedsAtLeastOneField(t *testing.T) { + if _, err := buildTagUpdatePayload(tagWriteOpts{}); err == nil { + t.Fatal("expected error when no fields provided") + } +} + +func TestBuildTagUpdatePayload_PartialOK(t *testing.T) { + payload, err := buildTagUpdatePayload(tagWriteOpts{Color: "#ff0000"}) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + tag := payload["tag"].(map[string]any) + if tag["color"] != "#ff0000" { + t.Fatalf("color = %v", tag["color"]) + } + if _, has := tag["name"]; has { + t.Fatalf("name should be omitted, got %#v", tag) + } +} + func TestReferenceCommandsRegistered(t *testing.T) { - cmd := New() - for _, args := range [][]string{ - {"categories", "list"}, - {"merchants", "show"}, - {"tags", "create"}, - {"rules", "list"}, - {"rule-runs", "show"}, - } { - if _, _, err := cmd.Find(args); err != nil { - t.Fatalf("expected command %v: %v", args, err) + root := New() + // cobra's Find returns the nearest matching ancestor when a leaf is + // missing, so compare resolved Name to the expected leaf. + cases := []struct { + args []string + want string + }{ + {[]string{"categories", "list"}, "list"}, + {[]string{"categories", "show"}, "show"}, + {[]string{"categories", "create"}, "create"}, + {[]string{"merchants", "list"}, "list"}, + {[]string{"merchants", "show"}, "show"}, + {[]string{"tags", "create"}, "create"}, + {[]string{"tags", "update"}, "update"}, + {[]string{"tags", "delete"}, "delete"}, + {[]string{"rules", "list"}, "list"}, + {[]string{"rule-runs", "show"}, "show"}, + } + for _, c := range cases { + got, _, err := root.Find(c.args) + if err != nil { + t.Fatalf("path %v not registered: %v", c.args, err) + } + if got.Name() != c.want { + t.Fatalf("path %v resolved to %q, want %q", c.args, got.Name(), c.want) } } } diff --git a/cmd/sure-cli/root/testhelpers_test.go b/cmd/sure-cli/root/testhelpers_test.go new file mode 100644 index 0000000..6a77d20 --- /dev/null +++ b/cmd/sure-cli/root/testhelpers_test.go @@ -0,0 +1,49 @@ +package root + +import ( + "bytes" + "io" + "os" + "testing" + + "github.com/spf13/cobra" +) + +// findSub looks up cmd's subcommand by name and fails the test if not found. +// Lives here (not next to any single subcommand's tests) because it is used +// across the entire root package's test suite. +func findSub(t *testing.T, cmd *cobra.Command, name string) *cobra.Command { + t.Helper() + sub, _, err := cmd.Find([]string{name}) + if err != nil { + t.Fatalf("find %q: %v", name, err) + } + return sub +} + +// captureStdout swaps os.Stdout for a pipe, runs fn, and returns whatever was +// written. The writer goroutine signals via channel for race-free happens-before +// (verified clean under `go test -race`). +func captureStdout(t *testing.T, fn func()) string { + t.Helper() + orig := os.Stdout + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + os.Stdout = w + + done := make(chan struct{}) + var buf bytes.Buffer + go func() { + _, _ = io.Copy(&buf, r) + close(done) + }() + + fn() + + _ = w.Close() + <-done + os.Stdout = orig + return buf.String() +} diff --git a/cmd/sure-cli/root/trades_cmd.go b/cmd/sure-cli/root/trades_cmd.go index 23ba357..b03f854 100644 --- a/cmd/sure-cli/root/trades_cmd.go +++ b/cmd/sure-cli/root/trades_cmd.go @@ -40,11 +40,11 @@ func newTradesCmd() *cobra.Command { if accountID != "" { q.Set("account_id", accountID) } - addRepeatedInvestmentQuery(q, "account_ids", accountIDs) - addInvestmentPagingQuery(q, page, perPage) + addRepeatedQuery(q, "account_ids", accountIDs) + addPagingQuery(q, page, perPage) u := url.URL{Path: "/api/v1/trades", RawQuery: q.Encode()} - printInvestmentGet(u.String()) + printGet(u.String()) }, } @@ -55,7 +55,7 @@ func newTradesCmd() *cobra.Command { list.Flags().StringVar(&account, "account", "", "account id (alias for --account-id)") list.Flags().StringVar(&accountID, "account-id", "", "account id") list.Flags().StringSliceVar(&accountIDs, "account-ids", nil, "account ids (repeat or comma-separated)") - addInvestmentPagingFlags(list, &page, &perPage) + addPagingFlags(list, &page, &perPage) cmd.AddCommand(list) cmd.AddCommand(&cobra.Command{ @@ -64,7 +64,7 @@ func newTradesCmd() *cobra.Command { Args: cobra.ExactArgs(1), Run: func(cmd *cobra.Command, args []string) { path := fmt.Sprintf("/api/v1/trades/%s", url.PathEscape(args[0])) - printInvestmentGet(path) + printGet(path) }, }) @@ -116,12 +116,7 @@ func newTradesCreateCmd() *cobra.Command { output.Fail("validation_failed", err.Error(), nil) return } - path := "/api/v1/trades" - if !o.Apply { - printInvestmentDryRun("POST", path, payload) - return - } - printInvestmentPost(path, payload) + dispatchWrite(o.Apply, "POST", "/api/v1/trades", payload) }, } cmd.Flags().StringVar(&o.AccountID, "account-id", "", "account id (required)") @@ -151,12 +146,7 @@ func newTradesUpdateCmd() *cobra.Command { output.Fail("validation_failed", err.Error(), nil) return } - path := fmt.Sprintf("/api/v1/trades/%s", url.PathEscape(args[0])) - if !o.Apply { - printInvestmentDryRun("PATCH", path, payload) - return - } - printInvestmentPatch(path, payload) + dispatchWrite(o.Apply, "PATCH", fmt.Sprintf("/api/v1/trades/%s", url.PathEscape(args[0])), payload) }, } cmd.Flags().StringVar(&o.Name, "name", "", "name") @@ -181,12 +171,7 @@ func newTradesDeleteCmd() *cobra.Command { Short: "Delete trade (default dry-run; use --apply to execute)", Args: cobra.ExactArgs(1), Run: func(cmd *cobra.Command, args []string) { - path := fmt.Sprintf("/api/v1/trades/%s", url.PathEscape(args[0])) - if !apply { - printInvestmentDryRun("DELETE", path, nil) - return - } - printInvestmentDelete(path) + dispatchWrite(apply, "DELETE", fmt.Sprintf("/api/v1/trades/%s", url.PathEscape(args[0])), nil) }, } cmd.Flags().BoolVar(&apply, "apply", false, "execute the delete (otherwise dry-run)") diff --git a/cmd/sure-cli/root/transfers_cmds_test.go b/cmd/sure-cli/root/transfers_cmds_test.go index c007b27..43ae8fa 100644 --- a/cmd/sure-cli/root/transfers_cmds_test.go +++ b/cmd/sure-cli/root/transfers_cmds_test.go @@ -2,19 +2,8 @@ package root import ( "testing" - - "github.com/spf13/cobra" ) -func findSub(t *testing.T, cmd *cobra.Command, name string) *cobra.Command { - t.Helper() - sub, _, err := cmd.Find([]string{name}) - if err != nil { - t.Fatalf("find %q: %v", name, err) - } - return sub -} - func TestTransfersCommandShape(t *testing.T) { cmd := newTransfersCmd() if cmd.Use != "transfers" { diff --git a/cmd/sure-cli/root/users_cmd.go b/cmd/sure-cli/root/users_cmd.go index 698d822..45f969e 100644 --- a/cmd/sure-cli/root/users_cmd.go +++ b/cmd/sure-cli/root/users_cmd.go @@ -2,8 +2,6 @@ package root import ( "github.com/spf13/cobra" - "github.com/we-promise/sure-cli/internal/api" - "github.com/we-promise/sure-cli/internal/output" ) func newUsersCmd() *cobra.Command { @@ -15,12 +13,7 @@ func newUsersCmd() *cobra.Command { Short: "Queue account reset (default dry-run; use --apply to execute)", Args: cobra.NoArgs, Run: func(cmd *cobra.Command, args []string) { - path := "/api/v1/users/reset" - if !applyReset { - printUsersDryRun("DELETE", path, nil) - return - } - printUsersDelete(path) + dispatchWrite(applyReset, "DELETE", "/api/v1/users/reset", nil) }, } reset.Flags().BoolVar(&applyReset, "apply", false, "execute the reset (otherwise dry-run)") @@ -29,7 +22,7 @@ func newUsersCmd() *cobra.Command { Short: "Show reset status", Args: cobra.NoArgs, Run: func(cmd *cobra.Command, args []string) { - printUsersGet("/api/v1/users/reset/status") + printGet("/api/v1/users/reset/status") }, }) cmd.AddCommand(reset) @@ -40,12 +33,7 @@ func newUsersCmd() *cobra.Command { Short: "Delete current user account (default dry-run; use --apply to execute)", Args: cobra.NoArgs, Run: func(cmd *cobra.Command, args []string) { - path := "/api/v1/users/me" - if !applyDelete { - printUsersDryRun("DELETE", path, nil) - return - } - printUsersDelete(path) + dispatchWrite(applyDelete, "DELETE", "/api/v1/users/me", nil) }, } deleteMe.Flags().BoolVar(&applyDelete, "apply", false, "execute the account deletion (otherwise dry-run)") @@ -53,34 +41,3 @@ func newUsersCmd() *cobra.Command { return cmd } - -func printUsersGet(path string) { - client := api.New() - var res any - r, err := client.Get(path, &res) - respond(r, err, res) -} - -func printUsersDelete(path string) { - client := api.New() - var res any - r, err := client.Delete(path, &res) - respond(r, err, res) -} - -func printUsersDryRun(method, path string, body any) { - request := map[string]any{ - "method": method, - "path": path, - } - if body != nil { - request["body"] = body - } - if err := output.Print(format, output.Envelope{Data: map[string]any{ - "dry_run": true, - "request": request, - }}); err != nil { - output.Fail("output_failed", err.Error(), nil) - return - } -}