diff --git a/internal/service/revisions.go b/internal/service/revisions.go index fc373c9..271b7f7 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,44 @@ 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), matching what the +// history list read produces so both sides of the API describe a change set the +// same way. +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..8fd8fe9 100644 --- a/internal/service/revisions_test.go +++ b/internal/service/revisions_test.go @@ -704,3 +704,57 @@ 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. +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) + } + fill := history(t, s, owner, g.ID)[0] + planted := totalOf(fill.Counts) + if planted == 0 { + t.Fatal("the fill recorded no counts") + } + + undo, conflicts, err := s.RevertChangeSet(ctx, owner, fill.ID, domain.SourceUI) + if err != nil || len(conflicts) != 0 { + t.Fatalf("revert: err=%v conflicts=%+v", err, conflicts) + } + if undo == nil { + t.Fatal("a revert that did work returned no change set") + } + if got := totalOf(undo.Counts); got != planted { + t.Errorf("revert reported %d changes, want %d — an empty tally reads as 'nothing happened'", got, planted) + } + + // And the list read agrees with what the revert returned, so both halves of + // the API describe the same change set the same way. + listed := history(t, s, owner, g.ID)[0] + if listed.ID != undo.ID { + t.Fatalf("newest change set is %d, want the revert %d", listed.ID, undo.ID) + } + if totalOf(listed.Counts) != totalOf(undo.Counts) { + t.Errorf("list says %d changes, revert said %d", totalOf(listed.Counts), totalOf(undo.Counts)) + } +} + +func totalOf(counts []domain.ChangeCount) int { + n := 0 + for _, c := range counts { + n += c.N + } + return n +} 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}.` } }