diff --git a/internal/service/revisions.go b/internal/service/revisions.go index fc373c9..a0ae39b 100644 --- a/internal/service/revisions.go +++ b/internal/service/revisions.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "log/slog" + "sort" "sync" "gitea.stevedudenhoeffer.com/steve/pansy/internal/domain" @@ -334,9 +335,48 @@ func (s *Service) RevertChangeSet(ctx context.Context, actorID, changeSetID int6 if err != nil { return nil, nil, err } + // Fill in the per-op counts. commitScope returns the freshly inserted row, + // which carries no tally — and a caller receiving a change set with an empty + // Counts cannot tell "nothing was reverted" from "the tally wasn't loaded". + // That ambiguity is not theoretical: it made the chat panel report a + // successful undo as "nothing left to undo". + if cs != nil { + cs.Counts = countRevisions(sc.taken()) + } return cs, conflicts, nil } +// countRevisions tallies revisions by (entity type, op). +// +// This deliberately reproduces in Go what ListChangeSets does in SQL, because a +// change set that has just been written has no rows to GROUP BY yet — the +// alternative is a second round trip to count what we are already holding. +// The parity is a real cross-layer contract, and TestRevertResultCarriesItsCounts +// compares the two breakdowns row for row so it can't drift silently. +func countRevisions(revs []domain.Revision) []domain.ChangeCount { + type key struct{ entityType, op string } + seen := map[key]int{} + order := []key{} + for _, r := range revs { + k := key{r.EntityType, r.Op} + if _, ok := seen[k]; !ok { + order = append(order, k) + } + seen[k]++ + } + sort.Slice(order, func(i, j int) bool { + if order[i].entityType != order[j].entityType { + return order[i].entityType < order[j].entityType + } + return order[i].op < order[j].op + }) + counts := make([]domain.ChangeCount, 0, len(order)) + for _, k := range order { + counts = append(counts, domain.ChangeCount{EntityType: k.entityType, Op: k.op, N: seen[k]}) + } + return counts +} + // entityKey identifies a row across the entity types a revision can name. type entityKey struct { entityType string diff --git a/internal/service/revisions_test.go b/internal/service/revisions_test.go index 2963b47..fecd22f 100644 --- a/internal/service/revisions_test.go +++ b/internal/service/revisions_test.go @@ -704,3 +704,104 @@ func TestClearObjectOnlyClearsWhatItSnapshotted(t *testing.T) { t.Errorf("cleared %d of %d with %d revisions — all three should match", n, len(before), len(revs)) } } + +// TestRevertResultCarriesItsCounts — found by using the thing. +// +// commitScope returns the freshly inserted row, which carries no tally. A caller +// receiving a change set with empty Counts cannot tell "nothing was reverted" +// from "the tally wasn't loaded", and the chat panel read that ambiguity the +// wrong way: a successful undo reported "nothing left to undo", which is the +// worst possible thing to say about an action that just worked. +// +// It also pins the cross-layer contract that fix created. countRevisions tallies +// in Go what ListChangeSets tallies in SQL, and nothing but this test stops the +// two drifting — so it compares the FULL per-(entity, op) breakdown, not just +// the totals, which would agree even if the groupings had diverged. +func TestRevertResultCarriesItsCounts(t *testing.T) { + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g := seedGarden(t, s, owner) + bed := seedBed(t, s, owner, g.ID) + plant := seedOwnPlant(t, s, owner, 15) + ctx := context.Background() + + if _, err := s.FillNamedRegion(ctx, owner, bed.ID, "all", plant.ID, nil); err != nil { + t.Fatalf("fill: %v", err) + } + // A second, different kind of change, so the breakdown has more than one row + // to get wrong. + if _, err := s.UpdateObject(ctx, owner, bed.ID, ObjectPatch{Name: strPtr("North Bed")}, bed.Version); err != nil { + t.Fatalf("rename: %v", err) + } + sets := history(t, s, owner, g.ID) + rename, fill := sets[0], sets[1] + + undoRename, conflicts, err := s.RevertChangeSet(ctx, owner, rename.ID, domain.SourceUI) + if err != nil || len(conflicts) != 0 { + t.Fatalf("revert rename: err=%v conflicts=%+v", err, conflicts) + } + undoFill, conflicts, err := s.RevertChangeSet(ctx, owner, fill.ID, domain.SourceUI) + if err != nil || len(conflicts) != 0 { + t.Fatalf("revert fill: err=%v conflicts=%+v", err, conflicts) + } + + for _, undo := range []*domain.ChangeSet{undoRename, undoFill} { + if undo == nil { + t.Fatal("a revert that did work returned no change set") + } + if len(undo.Counts) == 0 { + t.Fatalf("change set %d came back with no tally — an empty tally reads as 'nothing happened'", undo.ID) + } + } + // Undoing a fill deletes every plop it created, so the tallies must match. + if got, want := countsTotal(undoFill.Counts), countsTotal(fill.Counts); got != want { + t.Errorf("undo of the fill reports %d changes, want %d", got, want) + } + + // The SQL tally and the Go tally must agree, row for row. + listed := history(t, s, owner, g.ID) + byID := map[int64][]domain.ChangeCount{} + for _, cs := range listed { + byID[cs.ID] = cs.Counts + } + for _, undo := range []*domain.ChangeSet{undoRename, undoFill} { + fromSQL, ok := byID[undo.ID] + if !ok { + t.Fatalf("revert %d is missing from the history list", undo.ID) + } + if !sameCounts(undo.Counts, fromSQL) { + t.Errorf("change set %d: revert returned %+v, the list read says %+v — countRevisions and the SQL grouping have drifted", + undo.ID, undo.Counts, fromSQL) + } + } +} + +// sameCounts compares two tallies regardless of order. +func sameCounts(a, b []domain.ChangeCount) bool { + if len(a) != len(b) { + return false + } + index := func(cs []domain.ChangeCount) map[string]int { + m := map[string]int{} + for _, c := range cs { + m[c.EntityType+"/"+c.Op] = c.N + } + return m + } + ia, ib := index(a), index(b) + for k, v := range ia { + if ib[k] != v { + return false + } + } + return true +} + +// countsTotal sums a tally — the server-side twin of the client's totalChanges. +func countsTotal(counts []domain.ChangeCount) int { + n := 0 + for _, c := range counts { + n += c.N + } + return n +} diff --git a/internal/store/revisions.go b/internal/store/revisions.go index 669ebab..d696710 100644 --- a/internal/store/revisions.go +++ b/internal/store/revisions.go @@ -119,7 +119,9 @@ func (d *DB) ListChangeSets(ctx context.Context, gardenID int64, limit, offset i return sets, nil } - // One grouped query for the whole page rather than N per-row counts. + // One grouped query for the whole page rather than N per-row counts. The + // service's countRevisions mirrors this grouping for change sets it has just + // written; TestRevertResultCarriesItsCounts holds the two in step. countRows, err := d.sql.QueryContext(ctx, `SELECT change_set_id, entity_type, op, COUNT(*) FROM revisions diff --git a/web/src/lib/history.test.ts b/web/src/lib/history.test.ts index 83176f7..b32f1f1 100644 --- a/web/src/lib/history.test.ts +++ b/web/src/lib/history.test.ts @@ -49,6 +49,14 @@ describe('describeConflict', () => { describe('describeUndo', () => { const target = changeSet({ counts: [{ entityType: 'object', op: 'update', n: 3 }] }) + // The bug this pair exists for: a real revert response carries a change set + // whose counts the server didn't populate. Reading that as "nothing happened" + // told the user a successful undo had done nothing. + it('trusts the change set, not the tally, for whether anything happened', () => { + const out = describeUndo({ id: 5 }, { changeSet: changeSet({ id: 6, counts: [] }), conflicts: [] }) + expect(out).toEqual({ tone: 'ok', message: 'Undone.' }) + }) + it('is a plain success when nothing conflicted', () => { const out = describeUndo(target, { changeSet: changeSet({ id: 2, counts: [{ entityType: 'object', op: 'update', n: 3 }] }), @@ -61,6 +69,7 @@ describe('describeUndo', () => { // already a no-op — reachable by undoing a creation whose object is gone. // Claiming "Undone." there reports work that didn't happen. it("doesn't claim to have undone a no-op", () => { + // A NULL change set — not an empty tally — is the server's no-op signal. const out = describeUndo(target, { changeSet: null, conflicts: [] }) expect(out.tone).toBe('ok') expect(out.message).toBe('Nothing left to undo — this was already reversed.') diff --git a/web/src/lib/history.ts b/web/src/lib/history.ts index c86d9f4..87df177 100644 --- a/web/src/lib/history.ts +++ b/web/src/lib/history.ts @@ -224,20 +224,23 @@ export interface UndoOutcome { */ export function describeUndo(target: UndoTarget, result: RevertResult): UndoOutcome { const skipped = result.conflicts.map(describeConflict).join('; ') + // A NULL change set is the server's signal that nothing needed doing. An empty + // `counts` is not the same thing and must not be read as one — that conflation + // made a successful undo report "nothing left to undo", which is the worst + // possible thing to tell someone about an action that just worked. + const didSomething = result.changeSet != null const applied = result.changeSet ? totalChanges(result.changeSet) : 0 if (result.conflicts.length === 0) { - // The server answers 200 with a null change set when every revision was - // already a no-op — reachable by undoing a creation whose object is gone. - // Claiming "Undone." there would be reporting work that didn't happen. - if (applied === 0) return { tone: 'ok', message: 'Nothing left to undo — this was already reversed.' } + // Reachable by undoing a creation whose object is already gone. + if (!didSomething) return { tone: 'ok', message: 'Nothing left to undo — this was already reversed.' } return { tone: 'ok', message: 'Undone.' } } - if (applied === 0) { + if (!didSomething) { return { tone: 'error', message: `Nothing was undone — ${skipped}.` } } // Only claim a denominator when we have one. "2 of 3" from a caller that // never knew the total would be a number invented to fill a sentence. const total = totalChanges(target) - const scale = total > 0 ? `${applied} of ${total} changes undone` : 'Partly undone' + const scale = total > 0 && applied > 0 ? `${applied} of ${total} changes undone` : 'Partly undone' return { tone: 'partial', message: `${scale} — ${skipped}.` } }