diff --git a/CLAUDE.md b/CLAUDE.md index 9986afa..bc58a9c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -167,12 +167,31 @@ than a `#` comment so `LoadRules` can return it, the builder can write it and the rules screen can show it. It never takes part in matching — `rules.Engine` does not look at it — and it must stay that way. -**`config.DeleteRules` edits rules.toml textually, never by re-serialising the -parsed rules**, because comments and formatting are not recoverable from -`[]Rule`. A rule owns the comment lines directly above it; a comment separated -by a blank line is a heading for what follows and stays. Both writers go -through `writeFileAtomic`, and deletion re-parses the result before replacing -the file. +**`config.DeleteRules` and `config.ReplaceRule` edit rules.toml textually, never +by re-serialising the parsed rules**, because comments and formatting are not +recoverable from `[]Rule`. A rule owns the comment lines directly above it, so +deleting takes them with it; a comment separated by a blank line is a heading +for what follows and stays. Replacing keeps them — they say why the rule is +there, which editing its glob rarely changes — and rewrites only the rule's own +lines, leaving its *position* alone: position still breaks ties, so a rule that +moved could start beating an equally specific one it never used to. Every +writer goes through `writeFileAtomic`, and both editors re-parse the result +before replacing the file. + +**An edit must not lose what the builder does not show.** The form has four +fields and a `Rule` has five, so `saveRule` carries `Type` through from the rule +being edited and the form says it is doing so. Nothing may round-trip a rule +through those four fields alone — a pattern the user was never shown is not a +pattern they chose to remove. A new field on `Rule` needs the same treatment or +a field of its own. Covered by `TestRuleEditKeepsTypePattern`. + +**The rule builder's preview is what the rule is judged against, which is not +always "what is untagged".** For a new rule those are the same thing. For an +edit they are not: the rule's own transactions are tagged, so an untagged-only +preview would be empty for a rule that works. `reloadPreviewGroups` adds them +back, and `refreshRulePreview` keeps the ones the new glob stops catching on +screen marked `−` instead of dropping them silently, because giving one up is +the decision being made. ## Adding a bank parser diff --git a/README.md b/README.md index bf9caa1..c1cf222 100644 --- a/README.md +++ b/README.md @@ -189,6 +189,27 @@ because the more specific rule wins. Legs of a matched transfer are left out too: they are already accounted for by the definition that paired them, and the report leaves them out anyway. +#### Editing a rule + +`e` on the rules screen (`5`) opens the selected rule in this same form with the +fields filled in, and `enter` rewrites that rule where it sits instead of +appending a new one. It keeps its position, since position still breaks ties +between equally specific rules; `rules.toml` is edited textually, so the +comments and formatting around it survive, exactly as when deleting. + +An edit is judged against different rows to a new rule. What the rule already +claims is tagged, so none of it would be in an untagged preview and a rule that +works perfectly would preview as nothing; those rows are put back, and any the +new glob stops catching stays on screen marked `−`, under a "*n* no longer +claimed" line, rather than quietly disappearing the way an ordinary non-match +does. Narrowing `*LIDL*` to `*LIDL SOFIA*` therefore shows you the Varna branch +you are about to hand back to whatever rule catches it next. + +The form has no `type` field, so a rule that sets one carries it through +unchanged rather than losing it; it is shown under the glob as `+ type:… · +kept`. Saving lands back on the rules screen with the new counts, and `esc` +leaves the rule as it was. + ### Rules (`5`) Every rule in file order — the order they are written in, not the order they @@ -215,14 +236,16 @@ deleting. Note that shadowing has nothing to do with the numbering: rule 2 would be just as dead written above rule 1, because precedence is decided by how specific a rule is and not by where it sits. -`d` deletes the selected rule, `p` deletes every rule marked `✗` at once, and -both ask for a `y` first. `r` recounts against what is currently in the index, -which is what you want after an import has added rows; `rules.toml` itself is -read at startup and whenever you save a rule from the builder, so an edit made -in another window needs a restart. Deleting edits `rules.toml` textually, so your -comments, ordering and formatting survive; a comment sitting directly above a -deleted rule goes with it, while one separated by a blank line is treated as a -section heading and left alone. +`e` opens the selected rule in the builder to edit it, `d` deletes it, and `p` +deletes every rule marked `✗` at once; both deletions ask for a `y` first. `r` +recounts against what is currently in the index, which is what you want after an +import has added rows; `rules.toml` itself is read at startup and whenever you +save a rule from the builder, so an edit made in another window needs a restart. +Editing and deleting both work on `rules.toml` textually, so your comments, +ordering and formatting survive. A comment sitting directly above a deleted rule +goes with it, while one separated by a blank line is treated as a section +heading and left alone; an edited rule keeps its comments, since they say why it +is there and changing its glob rarely changes that. ### Transfer builder (`6`) diff --git a/internal/config/config.go b/internal/config/config.go index 4322016..5f0ad58 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -135,14 +135,73 @@ func checkTransfer(t Transfer) error { // The file is rewritten through a temporary file so a failure part-way cannot // leave the user with a truncated config. func AppendRule(root string, r Rule) error { + if err := checkRule(r); err != nil { + return err + } + return appendBlock(root, formatRule(r)) +} + +// checkRule rejects a rule that would match nothing or decide nothing. +func checkRule(r Rule) error { if r.Match == "" && r.Type == "" { return fmt.Errorf("a rule needs a match or type pattern") } if r.Tag == "" { return fmt.Errorf("a rule needs a tag") } + return nil +} - return appendBlock(root, formatRule(r)) +// ReplaceRule rewrites the rule at the given position (0-based, as loaded by +// LoadRules) with r, leaving it where it sits. Position no longer decides +// precedence, but it still breaks ties, so an edited rule that moved could +// start losing to — or start beating — an equally specific one it never used to. +// +// Like DeleteRules this edits the file textually rather than re-serialising the +// parsed rules, because comments and formatting are not recoverable from +// []Rule. Only the rule's own lines are replaced: the comments directly above +// it say why it is there, which editing its glob rarely changes, so they stay. +func ReplaceRule(root string, pos int, r Rule) error { + if err := checkRule(r); err != nil { + return err + } + + path := filepath.Join(root, RulesFile) + raw, err := os.ReadFile(path) + if err != nil { + return fmt.Errorf("read %s: %w", path, err) + } + lines := strings.Split(string(raw), "\n") + + starts := blockStarts(lines) + rules, transfers := blocksOf(starts, "rule"), blocksOf(starts, "transfer") + if pos < 0 || pos >= len(rules) { + return fmt.Errorf("rule %d is out of range; %s holds %d rules", pos+1, path, len(rules)) + } + + from, to := blockExtent(lines, starts, rules[pos]) + kept := make([]string, 0, len(lines)) + kept = append(kept, lines[:from]...) + kept = append(kept, strings.Split(strings.TrimRight(formatRule(r), "\n"), "\n")...) + kept = append(kept, lines[to:]...) + out := strings.Join(kept, "\n") + + // Never write something that will not load again, and never let editing one + // rule disturb a neighbour — of either kind, since both live in this file. + var check Rules + if _, err := toml.Decode(out, &check); err != nil { + return fmt.Errorf("editing %s would produce invalid TOML: %w", path, err) + } + if len(check.Rule) != len(rules) || len(check.Transfer) != len(transfers) { + return fmt.Errorf("editing %s would leave %d rules and %d transfers, expected %d and %d", + path, len(check.Rule), len(check.Transfer), len(rules), len(transfers)) + } + if check.Rule[pos] != r { + return fmt.Errorf("editing %s would leave rule %d as %+v, expected %+v", + path, pos+1, check.Rule[pos], r) + } + + return writeFileAtomic(root, path, out) } // AppendTransfer adds a transfer definition to the end of rules.toml. Order @@ -216,6 +275,45 @@ func blockStarts(lines []string) []blockStart { return out } +// blocksOf lists where the entries of one table sit among all the blocks, so a +// position in rules.toml as LoadRules numbers it can be turned into a position +// in the file. +func blocksOf(starts []blockStart, table string) []int { + var out []int + for k, s := range starts { + if s.table == table { + out = append(out, k) + } + } + return out +} + +// commentPrefix walks back from a block's header over the comment lines it +// owns. An entry owns the run of comments directly above it, with no blank line +// in between; anything further up is a heading for what follows. +func commentPrefix(lines []string, header int) int { + i := header + for i > 0 && strings.HasPrefix(strings.TrimSpace(lines[i-1]), "#") { + i-- + } + return i +} + +// blockExtent returns the k-th block's own lines as a half-open range: from its +// header to the last line it owns, leaving out the comments introducing the +// next block and the blank lines between the two, which belong to neither. +func blockExtent(lines []string, starts []blockStart, k int) (from, to int) { + from = starts[k].line + to = len(lines) + if k+1 < len(starts) { + to = commentPrefix(lines, starts[k+1].line) + } + for to > from+1 && strings.TrimSpace(lines[to-1]) == "" { + to-- + } + return from, to +} + // deleteBlocks removes entries of one table from rules.toml. // // The file is edited textually rather than re-serialised from the parsed @@ -241,12 +339,7 @@ func deleteBlocks(root, table string, positions []int) (int, error) { starts := blockStarts(lines) // mine[p] is where the p-th entry of this table sits among all the blocks. - var mine []int - for k, s := range starts { - if s.table == table { - mine = append(mine, k) - } - } + mine := blocksOf(starts, table) for _, p := range positions { if p < 0 || p >= len(mine) { return 0, fmt.Errorf("%s %d is out of range; %s holds %d %ss", @@ -254,33 +347,16 @@ func deleteBlocks(root, table string, positions []int) (int, error) { } } - // An entry owns the run of comment lines directly above it, with no blank - // line in between. Anything further up is a heading for what follows. - prefix := func(k int) int { - i := starts[k].line - for i > 0 && strings.HasPrefix(strings.TrimSpace(lines[i-1]), "#") { - i-- - } - return i - } - drop := map[int]bool{} for p := range doomed { k := mine[p] - // The block runs up to the next block's comment prefix, so a comment - // introducing the following one is not swept up with this one. - end := len(lines) - if k+1 < len(starts) { - end = prefix(k + 1) - } - for i := prefix(k); i < end; i++ { + // A rule's own comments document it and go with it; blockExtent leaves + // out the blank lines below it, which are the gap between blocks and + // part of neither, so the neighbours are not glued together. + _, end := blockExtent(lines, starts, k) + for i := commentPrefix(lines, starts[k].line); i < end; i++ { drop[i] = true } - // Blank lines are the gap between blocks, not part of either; leaving - // them avoids gluing the neighbours together. - for i := end - 1; i >= starts[k].line && strings.TrimSpace(lines[i]) == ""; i-- { - delete(drop, i) - } } kept := make([]string, 0, len(lines)) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index c604432..4ac8a39 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -673,3 +673,140 @@ func TestTransferToleranceIsBounded(t *testing.T) { t.Error("expected an out-of-range tolerance to be refused on load") } } + +// Editing rewrites the rule where it sits: the position is what still breaks +// ties between equally specific rules, so a rule that moved could start beating +// one it never used to. +func TestReplaceRuleKeepsPositionAndComments(t *testing.T) { + root := t.TempDir() + path := filepath.Join(root, RulesFile) + if err := os.WriteFile(path, []byte(rulesWithComments), 0o644); err != nil { + t.Fatal(err) + } + + edited := Rule{Match: "*LIDL SOFIA*", Tag: "groceries", Account: "checking", Note: "just the branch"} + if err := ReplaceRule(root, 0, edited); err != nil { + t.Fatal(err) + } + + loaded, err := LoadRules(root) + if err != nil { + t.Fatalf("the file no longer parses after editing: %v", err) + } + if len(loaded.Rule) != 3 { + t.Fatalf("got %d rules, want the same 3: %+v", len(loaded.Rule), loaded.Rule) + } + if loaded.Rule[0] != edited { + t.Errorf("rule 1 = %+v, want %+v", loaded.Rule[0], edited) + } + if loaded.Rule[1].Match != "*PAYROLL*" || loaded.Rule[2].Match != "*TO SAVINGS*" { + t.Errorf("the rest = %+v, want them where they were", loaded.Rule[1:]) + } + + // The comment above a rule says why it is there, which editing its glob + // does not change. + text, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + for _, want := range []string{"# Rules for my accounts.", "# Weekly shop.", "# Moving money to myself."} { + if !strings.Contains(string(text), want) { + t.Errorf("comment %q was lost:\n%s", want, text) + } + } + if strings.Contains(string(text), `"*LIDL*"`) { + t.Errorf("the old pattern is still there:\n%s", text) + } +} + +// A rule and the transfer below it share the file, so a replacement has to end +// where the next block of any kind begins. +func TestReplaceRuleLeavesTheOtherKindAlone(t *testing.T) { + root := t.TempDir() + path := filepath.Join(root, RulesFile) + original := `[[rule]] +match = "*LIDL*" +tag = "groceries" + +# Moving money to the broker. +[[transfer]] +from_account = "nlb" +from_desc = "*TO TRADEREPUBLIC*" +to_account = "traderepublic" +to_desc = "*FROM NLB*" + +[[rule]] +match = "*ZARA*" +tag = "clothes" +` + if err := os.WriteFile(path, []byte(original), 0o644); err != nil { + t.Fatal(err) + } + + if err := ReplaceRule(root, 0, Rule{Match: "*KAUFLAND*", Tag: "groceries"}); err != nil { + t.Fatal(err) + } + loaded, err := LoadRules(root) + if err != nil { + t.Fatal(err) + } + if len(loaded.Transfer) != 1 || loaded.Transfer[0].FromDesc != "*TO TRADEREPUBLIC*" { + t.Errorf("transfers = %+v, want the definition untouched", loaded.Transfer) + } + if len(loaded.Rule) != 2 || loaded.Rule[0].Match != "*KAUFLAND*" || loaded.Rule[1].Match != "*ZARA*" { + t.Errorf("rules = %+v, want only the first one rewritten", loaded.Rule) + } + raw, _ := os.ReadFile(path) + if !strings.Contains(string(raw), "# Moving money to the broker.") { + t.Errorf("the transfer's comment was lost:\n%s", raw) + } +} + +// Editing a rule that sets a type keeps it: the caller decides what the rule +// becomes, and a pattern left out of the replacement is a pattern removed. +func TestReplaceRuleWritesEveryPattern(t *testing.T) { + root := t.TempDir() + body := "[[rule]]\nmatch = \"*SPOTIFY*\"\ntype = \"CARD_PAYMENT\"\ntag = \"music\"\n" + if err := os.WriteFile(filepath.Join(root, RulesFile), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + + kept := Rule{Match: "*SPOTIFY*", Type: "CARD_PAYMENT", Tag: "subscriptions"} + if err := ReplaceRule(root, 0, kept); err != nil { + t.Fatal(err) + } + loaded, err := LoadRules(root) + if err != nil { + t.Fatal(err) + } + if loaded.Rule[0] != kept { + t.Errorf("rule = %+v, want %+v", loaded.Rule[0], kept) + } + + // A type-only rule is a rule; a rule with neither pattern is not. + if err := ReplaceRule(root, 0, Rule{Type: "CARD_PAYMENT", Tag: "cards"}); err != nil { + t.Errorf("a type-only rule was refused: %v", err) + } + if err := ReplaceRule(root, 0, Rule{Tag: "nothing"}); err == nil { + t.Error("expected a rule with no pattern to be rejected") + } + if err := ReplaceRule(root, 0, Rule{Match: "*X*"}); err == nil { + t.Error("expected a rule with no tag to be rejected") + } +} + +func TestReplaceRuleOutOfRange(t *testing.T) { + root := t.TempDir() + if err := AppendRule(root, Rule{Match: "*LIDL*", Tag: "groceries"}); err != nil { + t.Fatal(err) + } + for _, pos := range []int{-1, 1, 7} { + if err := ReplaceRule(root, pos, Rule{Match: "*X*", Tag: "x"}); err == nil { + t.Errorf("position %d: expected an error", pos) + } + } + loaded, _ := LoadRules(root) + if len(loaded.Rule) != 1 || loaded.Rule[0].Match != "*LIDL*" { + t.Errorf("rules = %+v, want the file untouched", loaded.Rule) + } +} diff --git a/internal/tui/tui.go b/internal/tui/tui.go index e8b19d5..d2ed2cf 100644 --- a/internal/tui/tui.go +++ b/internal/tui/tui.go @@ -84,27 +84,41 @@ type Model struct { txns []model.Transaction // rows currently shown in txnTable - // Rule builder: three inputs on the left, and a live preview on the right - // of which untagged descriptions the glob would catch. - ruleGlob textinput.Model - ruleAccount textinput.Model - ruleTag textinput.Model - ruleNote textinput.Model - ruleFocus int // which of the inputs has the cursor - ruleTable table.Model - ruleReturn view // the view to go back to on esc - untagged []descGroup - ruleMatches int // untagged descriptions the current glob matches + // Rule builder: four inputs on the left, and a live preview on the right of + // which descriptions the glob would catch. + ruleGlob textinput.Model + ruleAccount textinput.Model + ruleTag textinput.Model + ruleNote textinput.Model + ruleFocus int // which of the inputs has the cursor + ruleTable table.Model + ruleReturn view // the view to go back to on esc + previewGroups []descGroup + ruleMatches int // previewed descriptions the current glob matches // ruleCandidates is how many were in view before the glob filtered them, // which the count needs: the preview now shows only matches, so the rows on // screen can no longer say what they were chosen out of. ruleCandidates int + // ruleDropped counts the descriptions an edit would let go of. It is only + // ever non-zero while editing: a new rule has nothing to lose. + ruleDropped int + + // Editing an existing rule reuses the builder rather than a second form: + // the fields start filled in, and enter rewrites the rule where it sits + // instead of appending. ruleEditPos is its file position, which the edit + // must not change — that is what still breaks ties between equally specific + // rules. ruleEditOrig is the rule as it was, so what the form does not show + // (the type pattern) survives the round trip instead of being dropped. + ruleEditing bool + ruleEditPos int + ruleEditOrig config.Rule // Rule list: every rule with the number of transactions it actually // claims, so dead ones can be found and removed. - ruleListTable table.Model - ruleUsage []int - confirm confirmation + ruleListTable table.Model + ruleUsage []int + ruleListReturn view // the view to go back to on esc + confirm confirmation // Transfer builder: the two sides of a movement on the left, and on the // right the pairs it would form out of what is already imported, together @@ -129,8 +143,9 @@ type Model struct { previewFeeDigits int // Transfer list: every definition with what it currently pairs. - transferListTable table.Model - transferResult transfers.Result + transferListTable table.Model + transferResult transfers.Result + transferListReturn view // the view to go back to on esc filter store.Filter onlyUntagged bool @@ -418,25 +433,55 @@ const ( ruleFormWidth = ruleInputWidth + 12 ) -// descGroup is one distinct untagged description and how often it occurs. -// Grouping matters: a statement holds the same payee dozens of times, and a -// rule is written against the description, not against individual rows. +// descGroup is one distinct description the builder previews against, and how +// often it occurs. Grouping matters: a statement holds the same payee dozens of +// times, and a rule is written against the description, not against individual +// rows. type descGroup struct { Description string Accounts map[string]bool Count int + // Claimed marks a description the rule being edited currently tags. Nothing + // sets it for a new rule, where every candidate is untagged by definition. + Claimed bool } -// reloadUntagged rebuilds the alphabetical list of untagged descriptions that -// the rule builder previews against. -func (m *Model) reloadUntagged() error { +// reloadPreviewGroups rebuilds the alphabetical list of descriptions the rule +// builder previews against: everything still waiting for a rule, plus — when a +// rule is being edited — what that rule already claims. +// +// The addition is what makes editing legible. Those rows are tagged, so none of +// them is in the untagged list, and the preview for a rule that works perfectly +// would otherwise be empty. They are also exactly what an edit is judged +// against: narrowing a glob is a decision about which of them to let go. +func (m *Model) reloadPreviewGroups() error { txns, err := m.db.Transactions(store.Filter{Untagged: true}) if err != nil { return err } + claimedFrom := len(txns) // everything appended below is claimed by the edited rule + if m.ruleEditing { + seen := make(map[int64]bool, len(txns)) + for _, t := range txns { + seen[t.ID] = true + } + all, err := m.db.Transactions(store.Filter{}) + if err != nil { + return err + } + for _, t := range all { + // An index written before rules.toml last changed can hold a row + // that is untagged on disk and claimed by the engine in memory; + // counting it twice would overstate the group. + if seen[t.ID] || m.engine.MatchIndex(t.AccountSlug, t) != m.ruleEditPos { + continue + } + txns = append(txns, t) + } + } byDesc := map[string]*descGroup{} - for _, t := range txns { + for i, t := range txns { key := model.NormalizeDescription(t.Description) g, ok := byDesc[key] if !ok { @@ -445,21 +490,31 @@ func (m *Model) reloadUntagged() error { } g.Accounts[t.AccountSlug] = true g.Count++ + g.Claimed = g.Claimed || i >= claimedFrom } - m.untagged = make([]descGroup, 0, len(byDesc)) + m.previewGroups = make([]descGroup, 0, len(byDesc)) for _, g := range byDesc { - m.untagged = append(m.untagged, *g) + m.previewGroups = append(m.previewGroups, *g) } - sort.Slice(m.untagged, func(i, j int) bool { - a := model.NormalizeDescription(m.untagged[i].Description) - b := model.NormalizeDescription(m.untagged[j].Description) + sort.Slice(m.previewGroups, func(i, j int) bool { + a := model.NormalizeDescription(m.previewGroups[i].Description) + b := model.NormalizeDescription(m.previewGroups[j].Description) if a != b { return a < b } - return m.untagged[i].Description < m.untagged[j].Description + return m.previewGroups[i].Description < m.previewGroups[j].Description }) + // The column says what is in it, which an edit changes: the rows it claims + // are on screen precisely because they are not waiting for a rule. + cols := m.ruleTable.Columns() + cols[1].Title = "Untagged description" + if m.ruleEditing { + cols[1].Title = "Description" + } + m.ruleTable.SetColumns(cols) + m.refreshRulePreview() return nil } @@ -474,29 +529,40 @@ func (m *Model) reloadUntagged() error { // still waiting for a rule — which is the other question this screen answers. // Either way the count beside the form is measured against everything in view, // so a glob that has narrowed the list to three of forty still says so. +// +// A description the rule being edited currently tags is the one exception to +// the rows going: losing one is the thing worth seeing before saving, so it +// stays on screen marked “−” instead of vanishing silently with the rest. func (m *Model) refreshRulePreview() { var ( pattern = strings.TrimSpace(m.ruleGlob.Value()) account = strings.TrimSpace(m.ruleAccount.Value()) - rows = make([]table.Row, 0, len(m.untagged)) + rows = make([]table.Row, 0, len(m.previewGroups)) ) - m.ruleMatches, m.ruleCandidates = 0, 0 + m.ruleMatches, m.ruleCandidates, m.ruleDropped = 0, 0, 0 - for _, g := range m.untagged { + for _, g := range m.previewGroups { // An account filter narrows the preview the same way the saved rule // will narrow its matching. - if account != "" && !g.Accounts[account] { - continue + inAccount := account == "" || g.Accounts[account] + if inAccount { + m.ruleCandidates++ } - m.ruleCandidates++ + matched := inAccount && + (pattern == "" || glob.Match(pattern, model.NormalizeDescription(g.Description))) marker := " " - if pattern != "" { - if !glob.Match(pattern, model.NormalizeDescription(g.Description)) { - continue - } + switch { + case matched && pattern != "": marker = "▸" m.ruleMatches++ + case matched: + // No glob yet, so the row is context rather than an answer. + case g.Claimed: + marker = "−" + m.ruleDropped++ + default: + continue } rows = append(rows, table.Row{marker, g.Description, fmt.Sprintf("%d", g.Count)}) } @@ -512,8 +578,9 @@ func (m *Model) refreshRulePreview() { m.ruleTable.SetCursor(cursor) } -// saveRule appends the composed rule to rules.toml, reloads the engine and -// retags, so the effect is visible immediately. +// saveRule writes the composed rule to rules.toml — appending it, or rewriting +// the rule being edited in place — then reloads the engine and retags, so the +// effect is visible immediately. func (m *Model) saveRule() error { r := config.Rule{ Match: strings.TrimSpace(m.ruleGlob.Value()), @@ -521,7 +588,13 @@ func (m *Model) saveRule() error { Tag: strings.TrimSpace(m.ruleTag.Value()), Note: strings.TrimSpace(m.ruleNote.Value()), } - if r.Match == "" { + if m.ruleEditing { + // The form has no type field, so an edit carries the rule's type + // through untouched rather than quietly dropping a pattern it never + // showed the user. It is also why a type rule can have no glob at all. + r.Type = m.ruleEditOrig.Type + } + if r.Match == "" && r.Type == "" { return fmt.Errorf("enter a glob first, e.g. *LIDL*") } if r.Tag == "" { @@ -531,7 +604,11 @@ func (m *Model) saveRule() error { return fmt.Errorf("no account called %q; leave it blank to apply to every account", r.Account) } - if err := config.AppendRule(m.root, r); err != nil { + if m.ruleEditing { + if err := config.ReplaceRule(m.root, m.ruleEditPos, r); err != nil { + return err + } + } else if err := config.AppendRule(m.root, r); err != nil { return err } if err := m.reloadConfig(); err != nil { @@ -543,7 +620,18 @@ func (m *Model) saveRule() error { return err } - m.status = fmt.Sprintf("saved rule %s → %s, %d transactions retagged", r.Match, r.Tag, n) + if m.ruleEditing { + m.status = fmt.Sprintf("rule %d is now %s → %s, %d transactions retagged", + m.ruleEditPos+1, rulePattern(r), r.Tag, n) + // The account came from the rule rather than from the user, so it goes + // with the rest; an appended rule keeps it, since the next one written + // is usually for the same account. + m.ruleEditing = false + m.ruleEditOrig = config.Rule{} + m.ruleAccount.SetValue("") + } else { + m.status = fmt.Sprintf("saved rule %s → %s, %d transactions retagged", r.Match, r.Tag, n) + } m.ruleGlob.SetValue("") m.ruleTag.SetValue("") m.ruleNote.SetValue("") @@ -752,9 +840,16 @@ func (m *Model) deleteRules(positions []int) error { // openRuleList switches to the rule list, remembering where to return to. func (m *Model) openRuleList() { - if m.view != viewRuleList { - m.ruleReturn = m.view + if m.view != viewRuleList && m.view != viewRules { + m.ruleListReturn = m.view } + m.showRuleList() +} + +// showRuleList switches to the rule list without recording where esc goes. +// Coming back from the builder must not record it as the way out, or esc from +// the list would bounce back into the form the user just left. +func (m *Model) showRuleList() { m.view = viewRuleList m.confirm = confirmNone if err := m.reloadRuleList(); err != nil { @@ -791,7 +886,7 @@ func (m *Model) updateRuleList(msg tea.KeyMsg) (tea.Model, tea.Cmd) { case "q", "ctrl+c": return m, tea.Quit case "esc": - m.view = m.ruleReturn + m.view = m.ruleListReturn return m, nil case "1": m.view = viewAccounts @@ -810,6 +905,9 @@ func (m *Model) updateRuleList(msg tea.KeyMsg) (tea.Model, tea.Cmd) { m.openTransferList() return m, nil + case "e": + return m, m.openRuleEditor(m.ruleListTable.Cursor()) + case "d": i := m.ruleListTable.Cursor() rs := m.engine.Rules() @@ -842,21 +940,55 @@ func (m *Model) updateRuleList(msg tea.KeyMsg) (tea.Model, tea.Cmd) { return m, cmd } -// openRuleBuilder switches to the rule builder, remembering where to return to -// and seeding the account field from whatever account is being browsed. +// openRuleBuilder switches to the rule builder on a new rule, seeding the +// account field from whatever account is being browsed. func (m *Model) openRuleBuilder() tea.Cmd { + // Leaving an edit, the fields still hold that rule. A new rule starting out + // as a copy of the one just edited would be saved as a second, almost + // identical rule, so the form is emptied on the way out of edit mode. + if m.ruleEditing { + m.ruleEditing = false + m.ruleEditOrig = config.Rule{} + for _, in := range m.ruleInputs() { + in.SetValue("") + } + } + if m.ruleAccount.Value() == "" && m.filter.AccountSlug != "" { + m.ruleAccount.SetValue(m.filter.AccountSlug) + } + return m.showRuleBuilder() +} + +// openRuleEditor switches to the rule builder on the rule at a file position: +// the fields start filled in from it, and saving rewrites it where it sits +// rather than appending a near-duplicate. +func (m *Model) openRuleEditor(pos int) tea.Cmd { + rs := m.engine.Rules() + if pos < 0 || pos >= len(rs) { + return nil + } + r := rs[pos] + m.ruleEditing, m.ruleEditPos, m.ruleEditOrig = true, pos, r + m.ruleGlob.SetValue(r.Match) + m.ruleAccount.SetValue(r.Account) + m.ruleTag.SetValue(r.Tag) + m.ruleNote.SetValue(r.Note) + return m.showRuleBuilder() +} + +// showRuleBuilder switches to the builder and loads what the preview needs, +// remembering where to return to on esc. +func (m *Model) showRuleBuilder() tea.Cmd { if m.view != viewRules { m.ruleReturn = m.view } m.view = viewRules - if m.ruleAccount.Value() == "" && m.filter.AccountSlug != "" { - m.ruleAccount.SetValue(m.filter.AccountSlug) - } m.setRuleFocus(0) if err := m.reloadSuggestions(); err != nil { m.err = err } - if err := m.reloadUntagged(); err != nil { + // Last: what the preview lists depends on whether a rule is being edited. + if err := m.reloadPreviewGroups(); err != nil { m.err = err } return textinput.Blink @@ -1092,9 +1224,11 @@ func (m *Model) saveTransfer() error { } // openTransferList switches to the transfer list, remembering where to return. +// As on the rule list, the builder is not somewhere to return to: it is reached +// from here, and esc would bounce between the two. func (m *Model) openTransferList() { - if m.view != viewTransferList { - m.transferReturn = m.view + if m.view != viewTransferList && m.view != viewTransfers { + m.transferListReturn = m.view } m.view = viewTransferList m.confirm = confirmNone @@ -1231,7 +1365,7 @@ func (m *Model) updateTransferList(msg tea.KeyMsg) (tea.Model, tea.Cmd) { case "q", "ctrl+c": return m, tea.Quit case "esc": - m.view = m.transferReturn + m.view = m.transferListReturn return m, nil case "1": m.view = viewAccounts @@ -1495,11 +1629,18 @@ func (m *Model) updateRules(msg tea.KeyMsg) (tea.Model, tea.Cmd) { return m, nil case "enter": m.err = nil + editing := m.ruleEditing // saveRule clears it on the way through if err := m.saveRule(); err != nil { m.err = err return m, nil } - if err := m.reloadUntagged(); err != nil { + if editing { + // An edit is a round trip from the rules screen, so it ends there + // with the new counts rather than in an empty form. + m.showRuleList() + return m, nil + } + if err := m.reloadPreviewGroups(); err != nil { m.err = err } return m, nil @@ -1843,9 +1984,15 @@ func (m *Model) ruleFormView() string { // Give up the blank lines between fields first (21 lines) and the hints on // unfocused fields second (18), rather than letting the last field run off // the bottom. Below that the box borders are the floor. + // An edit carrying a type pattern renders one line more, and two once the + // fields are spaced out, so it asks for that much more room before either. + typed := 0 + if m.ruleEditing && m.ruleEditOrig.Type != "" { + typed = 1 + } room := m.height - 6 - spaced := m.height <= 0 || room >= 25 - hints := m.height <= 0 || room >= 21 + spaced := m.height <= 0 || room >= 25+2*typed + hints := m.height <= 0 || room >= 21+typed field := func(i int, label, help string) string { name := labelStyle.Render(" " + label) @@ -1867,6 +2014,14 @@ func (m *Model) ruleFormView() string { var b strings.Builder inputs := m.ruleInputs() b.WriteString(field(0, "glob", "vs. the description")) + // A type pattern has no field of its own, so an edit carrying one says so + // rather than leaving the rule looking broader than it is. + if m.ruleEditing && m.ruleEditOrig.Type != "" { + b.WriteString(hintStyle.Render("+ type:"+m.ruleEditOrig.Type+" · kept") + "\n") + if spaced { + b.WriteString("\n") + } + } b.WriteString(field(1, "account", completionHint(inputs[1], m.ruleFocus == 1, "blank = all accounts"))) b.WriteString(field(2, "tag", completionHint(inputs[2], m.ruleFocus == 2, "applied to matches"))) b.WriteString(field(3, "note", "why this rule exists")) @@ -1874,9 +2029,15 @@ func (m *Model) ruleFormView() string { // The count is the whole point of the preview: it says what the rule will // do before it is written to disk. summary := fmt.Sprintf("%d of %d descriptions match", m.ruleMatches, m.ruleCandidates) - if strings.TrimSpace(m.ruleGlob.Value()) == "" { + if strings.TrimSpace(m.ruleGlob.Value()) == "" && !m.ruleEditing { summary = fmt.Sprintf("%d untagged descriptions", m.ruleCandidates) } + // What an edit gives up is not visible in a count of what it keeps. It goes + // on its own line because the two together overflow the form's width, and + // lipgloss would wrap it mid-phrase. + if m.ruleDropped > 0 { + summary += fmt.Sprintf("\n%d no longer claimed", m.ruleDropped) + } b.WriteString(matchCountStyle.Render(summary)) return ruleFormStyle.Render(b.String()) @@ -1979,6 +2140,9 @@ func (m *Model) title() string { case viewAccounts: return "money · accounts" case viewRules: + if m.ruleEditing { + return fmt.Sprintf("money · editing rule %d · rewrites it in rules.toml", m.ruleEditPos+1) + } return "money · rule builder · writes to rules.toml" case viewRuleList: unused := len(m.unusedRules()) @@ -2021,12 +2185,17 @@ func (m *Model) help() string { case viewAccounts: return "enter open · 2 transactions · 3 report · 4 new rule · 5 rules · 6 new transfer · 7 transfers · i import · r retag · q quit" case viewRules: - return "tab complete/next field · ↑↓ field · ctrl+n/p other completions · pgup/pgdn scroll list · enter save rule · esc back · ctrl+c quit" + save := "enter save rule" + if m.ruleEditing { + save = "enter save changes" + } + return "tab complete/next field · ↑↓ field · ctrl+n/p other completions · pgup/pgdn scroll list · " + + save + " · esc back · ctrl+c quit" case viewRuleList: if m.confirm != confirmNone { return "y confirm · any other key cancels" } - return "d delete rule · p prune all unused · r refresh counts · 4 new rule · 7 transfers · 1 accounts · esc back · q quit" + return "e edit rule · d delete rule · p prune all unused · r refresh counts · 4 new rule · 7 transfers · 1 accounts · esc back · q quit" case viewTransfers: return "tab complete/next field · ↑↓ field · pgup/pgdn scroll pairs · enter save transfer · esc back · ctrl+c quit" case viewTransferList: diff --git a/internal/tui/tui_test.go b/internal/tui/tui_test.go index 4e8f079..a661e96 100644 --- a/internal/tui/tui_test.go +++ b/internal/tui/tui_test.go @@ -1914,3 +1914,213 @@ func TestRuleBuilderPreviewShowsOnlyMatches(t *testing.T) { t.Errorf("summary = %q, want 0 of 4", m.ruleFormView()) } } + +// loadRulesFile puts a rules.toml under the model's root and makes the model +// read it, as though it had been there all along. +func loadRulesFile(t *testing.T, m *Model, body string) { + t.Helper() + if err := os.WriteFile(filepath.Join(m.root, config.RulesFile), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + if err := m.reloadConfig(); err != nil { + t.Fatal(err) + } + if _, err := m.engine.Retag(m.db); err != nil { + t.Fatal(err) + } + if err := m.reload(); err != nil { + t.Fatal(err) + } +} + +const editableRules = `[[rule]] +match = "*ZZZ*" +tag = "misc" + +# The weekly shop. +[[rule]] +match = "*LIDL*" +tag = "groceries" +note = "both branches" +` + +// Editing rewrites the rule where it sits rather than appending a second, +// almost identical one, and lands back on the list it started from. +func TestRuleListEditsRuleInPlace(t *testing.T) { + m, db, root := newRuleModel(t) + loadRulesFile(t, m, editableRules) + + key(t, m, "5") + m.ruleListTable.SetCursor(1) + key(t, m, "e") + + if m.view != viewRules || !m.ruleEditing { + t.Fatalf("e opened view %v (editing %v), want the builder on the rule", m.view, m.ruleEditing) + } + if m.ruleGlob.Value() != "*LIDL*" || m.ruleTag.Value() != "groceries" || m.ruleNote.Value() != "both branches" { + t.Fatalf("form = %q / %q / %q, want the rule filled in", + m.ruleGlob.Value(), m.ruleTag.Value(), m.ruleNote.Value()) + } + + m.ruleTag.SetValue("food") + key(t, m, "enter") + + loaded, err := config.LoadRules(root) + if err != nil { + t.Fatal(err) + } + want := config.Rule{Match: "*LIDL*", Tag: "food", Note: "both branches"} + if len(loaded.Rule) != 2 { + t.Fatalf("rules = %+v, want the same 2: an edit must not append", loaded.Rule) + } + if loaded.Rule[0].Match != "*ZZZ*" { + t.Errorf("rule 1 = %+v, want it where it was", loaded.Rule[0]) + } + if loaded.Rule[1] != want { + t.Errorf("rule 2 = %+v, want %+v", loaded.Rule[1], want) + } + + // The file is edited textually, so the comment documenting the rule stays. + raw, err := os.ReadFile(filepath.Join(root, config.RulesFile)) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(raw), "# The weekly shop.") { + t.Errorf("the rule's comment was lost:\n%s", raw) + } + + // Saving retags immediately, as it does for a new rule. + txns, err := db.Transactions(store.Filter{}) + if err != nil { + t.Fatal(err) + } + tagged := 0 + for _, txn := range txns { + if txn.RuleTag == "food" { + tagged++ + } + } + if tagged != 3 { + t.Errorf("%d transactions tagged food, want 3", tagged) + } + + // An edit is a round trip: it ends on the list, with the new counts. + if m.view != viewRuleList { + t.Fatalf("view = %v after saving, want the rules list", m.view) + } + if rows := m.ruleListTable.Rows(); len(rows) != 2 || rows[1][4] != "food" { + t.Errorf("rows = %v, want the edited tag on the list", rows) + } + if !strings.Contains(m.status, "rule 2 is now") { + t.Errorf("status = %q, want it to name the rule that changed", m.status) + } +} + +// The preview lists what the rule already claims, not only what is untagged: +// for a rule that works those are the same rows, and an empty preview would say +// nothing about the edit. Narrowing the glob then shows what it gives up. +func TestRuleEditPreviewsWhatItDrops(t *testing.T) { + m, _, _ := newRuleModel(t) + loadRulesFile(t, m, editableRules) + + key(t, m, "5") + m.ruleListTable.SetCursor(1) + key(t, m, "e") + + all, matched := previewRows(m) + if len(all) != 2 || len(matched) != 2 { + t.Fatalf("preview = %v (matched %v), want the two descriptions the rule claims", all, matched) + } + + m.ruleGlob.SetValue("*SOFIA*") + m.refreshRulePreview() + + all, matched = previewRows(m) + if len(matched) != 1 || matched[0] != "LIDL SOFIA 4412" { + t.Errorf("matched = %v, want only the Sofia branch", matched) + } + // The row it stops claiming stays on screen: losing one is the thing worth + // seeing before saving, so it is marked rather than dropped silently. + if len(all) != 2 || m.ruleDropped != 1 { + t.Fatalf("preview = %v, dropped = %d; want the Varna row kept and counted", all, m.ruleDropped) + } + var dropped []string + for _, row := range m.ruleTable.Rows() { + if row[0] == "−" { + dropped = append(dropped, row[1]) + } + } + if len(dropped) != 1 || dropped[0] != "LIDL VARNA 9911" { + t.Errorf("dropped rows = %v, want the Varna branch", dropped) + } + if !strings.Contains(m.ruleFormView(), "1 no longer claimed") { + t.Errorf("summary = %q, want the dropped count", m.ruleFormView()) + } +} + +// The form has no type field, so an edit carries the pattern through instead of +// silently widening the rule to every transaction the glob matches. +func TestRuleEditKeepsTypePattern(t *testing.T) { + m, _, root := newRuleModel(t) + loadRulesFile(t, m, "[[rule]]\nmatch = \"*ZZZ*\"\ntype = \"CARD_PAYMENT\"\ntag = \"misc\"\n") + + key(t, m, "5") + key(t, m, "e") + + if !strings.Contains(m.ruleFormView(), "type:CARD_PAYMENT") { + t.Errorf("form = %q, want it to name the type it is carrying", m.ruleFormView()) + } + + m.ruleTag.SetValue("cards") + key(t, m, "enter") + + loaded, err := config.LoadRules(root) + if err != nil { + t.Fatal(err) + } + want := config.Rule{Match: "*ZZZ*", Type: "CARD_PAYMENT", Tag: "cards"} + if len(loaded.Rule) != 1 || loaded.Rule[0] != want { + t.Errorf("rules = %+v, want %+v", loaded.Rule, want) + } +} + +// Leaving an edit must not leave the form holding that rule, or the next thing +// saved would be a near-duplicate of the rule just edited. +func TestNewRuleAfterAnEditStartsEmpty(t *testing.T) { + m, _, _ := newRuleModel(t) + loadRulesFile(t, m, editableRules) + + key(t, m, "5") + m.ruleListTable.SetCursor(1) + key(t, m, "e") + key(t, m, "esc") + key(t, m, "4") + + if m.ruleEditing { + t.Error("4 opened the builder still in edit mode") + } + for _, in := range m.ruleInputs() { + if in.Value() != "" { + t.Errorf("form field = %q, want a blank form for a new rule", in.Value()) + } + } +} + +// The builder is reached from the list, so it is not somewhere the list can +// return to: esc must walk back out, not bounce between the two screens. +func TestEscFromRuleListDoesNotBounceIntoTheBuilder(t *testing.T) { + m, _, _ := newRuleModel(t) + loadRulesFile(t, m, editableRules) + + key(t, m, "2") + key(t, m, "5") + key(t, m, "e") + key(t, m, "esc") + if m.view != viewRuleList { + t.Fatalf("esc from the builder went to %v, want the list it was opened from", m.view) + } + key(t, m, "esc") + if m.view != viewTxns { + t.Errorf("esc from the list went to %v, want the transactions it was opened from", m.view) + } +}