mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-26 06:19:49 +00:00
fix(store): log why a workspace import renumbered its items (TASK-2923) (#1994)
When an archive's item numbers are not positive and workspace-unique, ImportWorkspace falls back to sequential numbering, which moves every reference number ([[TASK-2]] can then name a different item). That fallback was the one import coercion that logged nothing. It now logs one warning with the missing, invalid and duplicate counts and the first offending row. The scan runs to the end so the counts are complete; the numbering decision is unchanged.
This commit is contained in:
@@ -853,17 +853,42 @@ func (s *Store) importWorkspace(data *models.WorkspaceExport, newName string, ow
|
||||
// Legacy archives used per-collection numbering (or had no numbers), so
|
||||
// retain sequential allocation when the imported set is not workspace-unique.
|
||||
// Orphans are skipped below and must not force valid items to be renumbered.
|
||||
//
|
||||
// The fallback is logged (TASK-2923): it silently moves every reference
|
||||
// number, so an operator whose [[TASK-2]] now points elsewhere needs to
|
||||
// find why. The scan runs to the end so the line can count each reason,
|
||||
// as the coercions below do for theirs.
|
||||
preserveItemNumbers := true
|
||||
seenItemNumbers := make(map[int]bool, len(data.Items))
|
||||
var missingNumbers, invalidNumbers, duplicateNumbers int
|
||||
var firstRenumberRow string
|
||||
for _, it := range data.Items {
|
||||
if collMap[it.CollectionID] == "" {
|
||||
continue
|
||||
}
|
||||
if it.ItemNumber <= 0 || seenItemNumbers[it.ItemNumber] {
|
||||
preserveItemNumbers = false
|
||||
break
|
||||
switch {
|
||||
case it.ItemNumber == 0:
|
||||
missingNumbers++
|
||||
case it.ItemNumber < 0:
|
||||
invalidNumbers++
|
||||
case seenItemNumbers[it.ItemNumber]:
|
||||
duplicateNumbers++
|
||||
default:
|
||||
seenItemNumbers[it.ItemNumber] = true
|
||||
continue
|
||||
}
|
||||
seenItemNumbers[it.ItemNumber] = true
|
||||
if preserveItemNumbers {
|
||||
firstRenumberRow = it.ID
|
||||
}
|
||||
preserveItemNumbers = false
|
||||
}
|
||||
if !preserveItemNumbers {
|
||||
slog.Warn("import_workspace renumbered items sequentially: the archive's item numbers are not positive and workspace-unique, so references to them (e.g. [[TASK-2]]) may now point at different items",
|
||||
"workspace_id", ws.ID,
|
||||
"missing", missingNumbers,
|
||||
"invalid", invalidNumbers,
|
||||
"duplicate", duplicateNumbers,
|
||||
"first_row_id", firstRenumberRow)
|
||||
}
|
||||
|
||||
// items_per_workspace, resolved here and compared after the insert loop
|
||||
|
||||
@@ -0,0 +1,95 @@
|
||||
package store
|
||||
|
||||
import (
|
||||
"log/slog"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/PerpetualSoftware/pad/internal/models"
|
||||
)
|
||||
|
||||
// TASK-2923: when an archive's item numbers are not positive and
|
||||
// workspace-unique, ImportWorkspace falls back to sequential numbering and
|
||||
// every reference number moves. That fallback now logs one warning naming
|
||||
// why (missing, invalid, duplicate counts), the way the import's coercions
|
||||
// already log theirs. A clean archive keeps its numbers and logs nothing.
|
||||
func TestTASK2923_RenumberFallbackIsLogged(t *testing.T) {
|
||||
s := testStore(t)
|
||||
user, err := s.CreateUser(models.UserCreate{Email: "renumber@example.com", Name: "R", Password: "correct-horse-battery-staple"})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "Renumber Source", Slug: "renumber-src", OwnerID: user.ID})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
coll, err := s.CreateCollection(ws.ID, models.CollectionCreate{Name: "Tasks", Slug: "tasks", Prefix: "TASK"})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
for _, title := range []string{"one", "two", "three", "four"} {
|
||||
if _, err := s.CreateItem(ws.ID, coll.ID, models.ItemCreate{Title: title}); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
}
|
||||
|
||||
importWith := func(name string, mutate func(items []models.ItemExport)) []slog.Record {
|
||||
t.Helper()
|
||||
exp, err := s.ExportWorkspace(ws.Slug)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if len(exp.Items) != 4 {
|
||||
t.Fatalf("export has %d items, want 4", len(exp.Items))
|
||||
}
|
||||
mutate(exp.Items)
|
||||
var captured []slog.Record
|
||||
prev := slog.Default()
|
||||
slog.SetDefault(slog.New(&recordCapturingHandler{records: &captured}))
|
||||
_, err = s.ImportWorkspace(exp, name, user.ID, "")
|
||||
slog.SetDefault(prev)
|
||||
if err != nil {
|
||||
t.Fatalf("ImportWorkspace(%s): %v", name, err)
|
||||
}
|
||||
var out []slog.Record
|
||||
for _, r := range captured {
|
||||
if strings.HasPrefix(r.Message, "import_workspace renumbered items sequentially") {
|
||||
out = append(out, r)
|
||||
}
|
||||
}
|
||||
return out
|
||||
}
|
||||
attrs := func(r slog.Record) map[string]int64 {
|
||||
m := map[string]int64{}
|
||||
r.Attrs(func(a slog.Attr) bool {
|
||||
if a.Value.Kind() == slog.KindInt64 {
|
||||
m[a.Key] = a.Value.Int64()
|
||||
}
|
||||
return true
|
||||
})
|
||||
return m
|
||||
}
|
||||
|
||||
// Control: a clean archive keeps its numbers, so nothing is logged.
|
||||
if got := importWith("renumber-clean", func([]models.ItemExport) {}); len(got) != 0 {
|
||||
t.Fatalf("a clean archive logged the renumber warning %d time(s)", len(got))
|
||||
}
|
||||
|
||||
// One missing, one invalid and one duplicate number: ONE warning that
|
||||
// counts each, not one per row and not stopping at the first.
|
||||
got := importWith("renumber-bad", func(items []models.ItemExport) {
|
||||
items[0].ItemNumber = 0
|
||||
items[1].ItemNumber = -3
|
||||
items[3].ItemNumber = items[2].ItemNumber
|
||||
})
|
||||
if len(got) != 1 {
|
||||
t.Fatalf("renumber warnings = %d, want exactly 1", len(got))
|
||||
}
|
||||
a := attrs(got[0])
|
||||
if a["missing"] != 1 || a["invalid"] != 1 || a["duplicate"] != 1 {
|
||||
t.Errorf("counts = missing %d, invalid %d, duplicate %d; want 1, 1, 1", a["missing"], a["invalid"], a["duplicate"])
|
||||
}
|
||||
if got[0].Level != slog.LevelWarn {
|
||||
t.Errorf("level = %v, want WARN", got[0].Level)
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user