From 05fc8ad80c8d25877b9bcdbaf37e3d0ecf414d95 Mon Sep 17 00:00:00 2001 From: Gregory Salaun Date: Fri, 14 Aug 2026 11:57:35 +0200 Subject: [PATCH] fix(qsl): copy a design's pictures, not just its row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit copyDirContents wrote into a destination it never created, so the first os.Create returned ENOENT. The caller was written as "ignore os.IsNotExist" — meant for a design with no pictures at all — and that guard matched the failed write exactly: nothing was copied, no error surfaced, and validation two lines later reported "copied design is incomplete: hero photo file img_… not found" with no way to tell why. Three fixes, one cause: - copyDirContents creates its destination; - the caller stats the source folder instead of pattern-matching an error, so a genuine copy failure is reported and rolls back both the row and the folder; - DuplicateProfile had the same defect in its own form — an INSERT … SELECT that cloned the template rows and left their photos behind, giving the new profile designs with no thumbnail and nothing to print. It now copies row and folder together, per template, since each needs its id first. A test reproduces the original failure: it fails with the exact ENOENT that was being swallowed. --- app.go | 12 +++++--- app_qsl_designer.go | 68 +++++++++++++++++++++++++++++++++++++++++---- changelog.json | 8 ++++-- copydir_test.go | 61 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 138 insertions(+), 11 deletions(-) create mode 100644 copydir_test.go diff --git a/app.go b/app.go index e246511..b0099a2 100644 --- a/app.go +++ b/app.go @@ -1704,6 +1704,13 @@ func copyDirContents(src, dst string) error { if err != nil { return err } + // Create the destination BEFORE writing into it. Without this the first + // os.Create failed with ENOENT, which reads as "not exist" — indistinguishable + // from a source folder that simply has nothing to copy, and callers testing + // for that swallowed it. + if err := os.MkdirAll(dst, 0o755); err != nil { + return err + } for _, e := range entries { srcPath := filepath.Join(src, e.Name()) dstPath := filepath.Join(dst, e.Name()) @@ -14099,10 +14106,7 @@ func (a *App) DuplicateProfile(id int64, newName string) (profile.Profile, error applog.Printf("duplicate profile: copy operating: %v", err) } } - if _, err := a.db.ExecContext(a.ctx, - `INSERT INTO qsl_templates (name, profile_id, json, is_default, created_at, updated_at) - SELECT name, ?, json, is_default, created_at, updated_at - FROM qsl_templates WHERE profile_id = ?`, p.ID, id); err != nil { + if err := a.copyQSLTemplatesToProfile(id, p.ID); err != nil { applog.Printf("duplicate profile: copy qsl templates: %v", err) } return p, nil diff --git a/app_qsl_designer.go b/app_qsl_designer.go index 7c0cff2..5f9ba38 100644 --- a/app_qsl_designer.go +++ b/app_qsl_designer.go @@ -827,18 +827,32 @@ func (a *App) QSLCopyTemplateToActiveProfile(id int64) (int64, error) { } srcDir := qslcard.TemplateDir(a.qslDir(), id) dstDir := qslcard.TemplateDir(a.qslDir(), rec.ID) - if err := copyDirContents(srcDir, dstDir); err != nil && !os.IsNotExist(err) { - // Roll back rather than leave a design whose pictures are missing. + // A design with no pictures at all has no folder, and that is not a failure. + // Anything else IS: this used to be written as "ignore os.IsNotExist", which + // also swallowed every ENOENT raised while copying — so a failed copy looked + // like a design that had nothing to copy, and the operator got "hero photo + // not found" with no clue why. + if _, statErr := os.Stat(srcDir); statErr == nil { + if err := copyDirContents(srcDir, dstDir); err != nil { + // Roll back rather than leave a design whose pictures are missing. + _ = a.qslTemplates.Delete(a.ctx, rec.ID) + _ = qslcard.RemoveTemplateDir(a.qslDir(), rec.ID) + return 0, fmt.Errorf("copy template assets: %w", err) + } + } + // Every failure from here rolls back BOTH halves: deleting only the row would + // leave the copied pictures on disk under an id nothing refers to. + rollback := func() { _ = a.qslTemplates.Delete(a.ctx, rec.ID) - return 0, fmt.Errorf("copy template assets: %w", err) + _ = qslcard.RemoveTemplateDir(a.qslDir(), rec.ID) } t, err := qslcard.Parse([]byte(rec.JSON)) if err != nil { - _ = a.qslTemplates.Delete(a.ctx, rec.ID) + rollback() return 0, err } if err := qslcard.Validate(t, qslcard.PhotoExistsIn(dstDir)); err != nil { - _ = a.qslTemplates.Delete(a.ctx, rec.ID) + rollback() return 0, fmt.Errorf("copied design is incomplete: %w", err) } applog.Printf("qsl: copied template %q (id %d) into the active profile as %q (id %d)", @@ -846,6 +860,50 @@ func (a *App) QSLCopyTemplateToActiveProfile(id int64) (int64, error) { return rec.ID, nil } +// copyQSLTemplatesToProfile duplicates one profile's QSL designs into another, +// PICTURES INCLUDED. +// +// This was an INSERT … SELECT, which cloned the rows and nothing else. A design +// references its photos by name, relative to its own asset folder, so every +// duplicated design pointed at a folder that did not exist: no thumbnail in the +// designer, and nothing to print. The rows looked right, which is why it went +// unnoticed — the damage is entirely on disk. +// +// Row by row rather than in one statement, because each new design needs its id +// before it can own a folder. A design whose files cannot be copied is dropped +// rather than kept empty: an operator who sees the design listed will believe +// it works. +func (a *App) copyQSLTemplatesToProfile(fromProfile, toProfile int64) error { + if a.qslTemplates == nil { + return fmt.Errorf("db not initialized") + } + all, err := a.qslTemplates.List(a.ctx) + if err != nil { + return err + } + for _, src := range all { + if src.ProfileID == nil || *src.ProfileID != fromProfile { + continue + } + rec := qslcard.Record{Name: src.Name, JSON: src.JSON, IsDefault: src.IsDefault} + to := toProfile + rec.ProfileID = &to + if err := a.qslTemplates.Save(a.ctx, &rec); err != nil { + return err + } + srcDir := qslcard.TemplateDir(a.qslDir(), src.ID) + if _, statErr := os.Stat(srcDir); statErr != nil { + continue // a design with no pictures at all — nothing to carry over + } + if err := copyDirContents(srcDir, qslcard.TemplateDir(a.qslDir(), rec.ID)); err != nil { + _ = a.qslTemplates.Delete(a.ctx, rec.ID) + _ = qslcard.RemoveTemplateDir(a.qslDir(), rec.ID) + return fmt.Errorf("copy assets of %q: %w", src.Name, err) + } + } + return nil +} + func firstNonEmptyStr(a, b string) string { if strings.TrimSpace(a) != "" { return a diff --git a/changelog.json b/changelog.json index 33222ba..fc862a8 100644 --- a/changelog.json +++ b/changelog.json @@ -13,7 +13,9 @@ "DX cluster: the US county column now shows the county the station is logged with, so it agrees with the Info panel.", "QSL: the manager (QSL_VIA) and the routing method (QSL_SENT_VIA / QSL_RCVD_VIA) are now separate fields, as ADIF defines them.", "ADIF import: QSL_SENT_VIA no longer lands in the manager field, and QSL_RCVD_VIA is no longer discarded. Both are exported.", - "QSL: logs where a routing word sits in the manager field are counted at startup and corrected only if you accept." + "QSL: logs where a routing word sits in the manager field are counted at startup and corrected only if you accept.", + "QSL designer: copying a design from another profile failed with \"hero photo not found\" — the pictures were never copied.", + "QSL designer: duplicating a profile cloned its card designs without their pictures, leaving designs that could not be printed." ], "fr": [ "Amplificateurs : coche ceux qui partagent un combiner et ON, OFF et OPERATE agissent sur tous à la fois. Chacun garde ses mesures.", @@ -26,7 +28,9 @@ "Cluster DX : la colonne comté US affiche le comté du log quand la station y figure, donc identique au panneau Info.", "QSL : le manager (QSL_VIA) et le mode d’envoi (QSL_SENT_VIA / QSL_RCVD_VIA) sont désormais deux champs distincts, comme le veut l’ADIF.", "Import ADIF : QSL_SENT_VIA ne se retrouve plus dans le champ manager, et QSL_RCVD_VIA n’est plus perdu. Les deux sont exportés.", - "QSL : les logs où un mode d’envoi occupe le champ manager sont comptés au démarrage et corrigés seulement si vous acceptez." + "QSL : les logs où un mode d’envoi occupe le champ manager sont comptés au démarrage et corrigés seulement si vous acceptez.", + "Concepteur QSL : copier un design depuis un autre profil échouait sur « photo introuvable » — les images n’étaient jamais copiées.", + "Concepteur QSL : dupliquer un profil clonait ses designs sans leurs images, donnant des cartes impossibles à imprimer." ] }, { diff --git a/copydir_test.go b/copydir_test.go new file mode 100644 index 0000000..89b9b98 --- /dev/null +++ b/copydir_test.go @@ -0,0 +1,61 @@ +package main + +import ( + "os" + "path/filepath" + "testing" +) + +// TestCopyDirContentsCreatesDestination pins the bug that broke copying a QSL +// design between profiles. +// +// copyDirContents wrote straight into dst without creating it, so the first +// os.Create failed with ENOENT. The caller was written as "ignore +// os.IsNotExist", meaning "the source has no assets, that is fine" — and ENOENT +// from the failed write matched it exactly. Nothing was copied, no error was +// reported, and the operator saw "copied design is incomplete: hero photo file +// … not found" from the validation two lines later. +func TestCopyDirContentsCreatesDestination(t *testing.T) { + base := t.TempDir() + src := filepath.Join(base, "src") + if err := os.MkdirAll(filepath.Join(src, "sub"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(src, "img_25c06bda.jpg"), []byte("hero"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(src, "sub", "back.png"), []byte("back"), 0o644); err != nil { + t.Fatal(err) + } + + // dst does not exist — the case of every first copy. + dst := filepath.Join(base, "dst") + if err := copyDirContents(src, dst); err != nil { + t.Fatalf("copyDirContents: %v", err) + } + for name, want := range map[string]string{ + "img_25c06bda.jpg": "hero", + "sub/back.png": "back", + } { + got, err := os.ReadFile(filepath.Join(dst, filepath.FromSlash(name))) + if err != nil { + t.Errorf("%s: %v", name, err) + continue + } + if string(got) != want { + t.Errorf("%s = %q, want %q", name, got, want) + } + } +} + +// A missing SOURCE is still an error from copyDirContents itself — it is the +// caller that decides whether a design with no pictures is acceptable, and it +// can only decide that if this does not quietly succeed. +func TestCopyDirContentsMissingSource(t *testing.T) { + base := t.TempDir() + if err := copyDirContents(filepath.Join(base, "nope"), filepath.Join(base, "dst")); err == nil { + t.Fatal("copying a missing source returned no error") + } else if !os.IsNotExist(err) { + t.Fatalf("want a not-exist error, got %v", err) + } +}