From 7c564aca3ca0a3326f0e8fcd5ca871e9494927c1 Mon Sep 17 00:00:00 2001 From: Suriyakumarvijayanayagam Date: Tue, 29 Sep 2026 16:02:06 +0530 Subject: [PATCH] A merge lost a phone number on its first live run Found by walking the scenario against production rather than by a test. Two records, each with a phone; the survivor kept its own, and the source's simply stopped existing. Searching for it returned nothing. The first version's rule was "fill the survivor's blanks, never overwrite what it has", which is right about which value WINS and said nothing about the one that loses. One person can have two numbers, two spellings of a name, a work address and a personal one - and a merge that quietly deletes one is exactly the data loss this file already refuses elsewhere: "silently turning Alice back into Visitor 3 is data loss the operator cannot see happen." The profile is now reconciled field by field in Go rather than in one clever upsert, because the interesting case was never the winner. Blanks are still filled and the survivor still keeps its own values, but every losing value is returned in `discarded` AND appended to the survivor's notes - the response is read once and the record is read forever. Notes themselves are additive rather than a winner: two people writing about one customer wrote two different true things. mergeProfiles is pure, so the rule is asserted directly - four cases including the ordinary one, a typed record joining a camera record with no profile at all, which must add no noise. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KGcjxF1cNLcuwc3DAPcnfj --- server/internal/api/types.go | 5 + server/internal/store/api_customers.go | 113 ++++++++++++++++---- server/internal/store/api_customers_test.go | 71 ++++++++++++ 3 files changed, 170 insertions(+), 19 deletions(-) create mode 100644 server/internal/store/api_customers_test.go diff --git a/server/internal/api/types.go b/server/internal/api/types.go index d97be8a..235ccf6 100644 --- a/server/internal/api/types.go +++ b/server/internal/api/types.go @@ -194,6 +194,11 @@ type MergeResult struct { // these on cards and read them aloud, so a merge has to say which one // died rather than leaving somebody to discover it at a counter. RetiredRef string `json:"retired_ref"` + // Discarded lists profile values the survivor already had a different + // answer for - a second phone number, a different spelling of a name. + // They are appended to the survivor's notes as well: this response is + // read once and the record is read forever. + Discarded []string `json:"discarded,omitempty"` } // MergeRequest names the record to keep. diff --git a/server/internal/store/api_customers.go b/server/internal/store/api_customers.go index ecaba15..d2e70d1 100644 --- a/server/internal/store/api_customers.go +++ b/server/internal/store/api_customers.go @@ -12,7 +12,9 @@ package store import ( "context" + "errors" "fmt" + "strings" "time" "github.com/jackc/pgx/v5" @@ -148,25 +150,39 @@ func (s *Store) MergeVisitors(ctx context.Context, clientID, sourceID, targetID return out, pgx.ErrNoRows } - // Profile: visitor_profiles is UNIQUE on visitor_id, so the two cannot - // simply both move. Blanks on the survivor are filled from the source and - // nothing the survivor already holds is overwritten - which is exactly - // right for the case this exists for, a hand-typed name and phone being - // joined to the face that was recognised later. - if _, err := tx.Exec(ctx, ` - INSERT INTO visitor_profiles (visitor_id, client_id, full_name, phone, - email, gender, notes, collected_by, collected_at) - SELECT $2::uuid, client_id, full_name, phone, email, gender, notes, - collected_by, collected_at - FROM visitor_profiles WHERE visitor_id = $1::uuid - ON CONFLICT (visitor_id) DO UPDATE SET - full_name = CASE WHEN visitor_profiles.full_name = '' THEN EXCLUDED.full_name ELSE visitor_profiles.full_name END, - phone = CASE WHEN visitor_profiles.phone = '' THEN EXCLUDED.phone ELSE visitor_profiles.phone END, - email = CASE WHEN visitor_profiles.email = '' THEN EXCLUDED.email ELSE visitor_profiles.email END, - gender = CASE WHEN visitor_profiles.gender = '' THEN EXCLUDED.gender ELSE visitor_profiles.gender END, - notes = CASE WHEN visitor_profiles.notes = '' THEN EXCLUDED.notes ELSE visitor_profiles.notes END, - updated_at = now()`, sourceID, targetID); err != nil { - return out, fmt.Errorf("merge profile: %w", err) + // Profile: visitor_profiles is UNIQUE on visitor_id, so the two cannot both + // move and something has to win. Merged field by field in Go rather than in + // one clever upsert, because the interesting case is not which value wins - + // it is what happens to the one that loses. + // + // Blanks on the survivor are filled from the source. Where BOTH hold a + // value the survivor keeps its own and the loser is recorded in `discarded` + // and appended to notes. Dropping it silently was the first version's + // behaviour and it lost a phone number on the very first live run: one + // person can have two numbers, and a merge that quietly deletes one is + // precisely the data loss an operator cannot see happen. + var src, dst profileFields + if err := readProfile(ctx, tx, sourceID, &src); err != nil { + return out, fmt.Errorf("read source profile: %w", err) + } + if err := readProfile(ctx, tx, targetID, &dst); err != nil { + return out, fmt.Errorf("read target profile: %w", err) + } + if src.exists { + merged, discarded := mergeProfiles(src, dst) + out.Discarded = discarded + if _, err := tx.Exec(ctx, ` + INSERT INTO visitor_profiles (visitor_id, client_id, full_name, phone, + email, gender, notes) + VALUES ($1::uuid, $2::uuid, $3, $4, $5, $6, $7) + ON CONFLICT (visitor_id) DO UPDATE SET + full_name = EXCLUDED.full_name, phone = EXCLUDED.phone, + email = EXCLUDED.email, gender = EXCLUDED.gender, + notes = EXCLUDED.notes, updated_at = now()`, + targetID, clientID, merged.fullName, merged.phone, merged.email, + merged.gender, merged.notes); err != nil { + return out, fmt.Errorf("merge profile: %w", err) + } } for _, q := range []struct { @@ -236,3 +252,62 @@ func (s *Store) MergeVisitors(ctx context.Context, clientID, sourceID, targetID func isAutoLabel(label string, number int64) bool { return label == fmt.Sprintf("Visitor %d", number) } + +// profileFields is the part of a profile a merge has to reconcile. +type profileFields struct { + exists bool + fullName, phone, email, gender, notes string +} + +func readProfile(ctx context.Context, tx pgx.Tx, visitorID string, out *profileFields) error { + err := tx.QueryRow(ctx, ` + SELECT full_name, phone, email, gender, notes + FROM visitor_profiles WHERE visitor_id = $1::uuid`, visitorID). + Scan(&out.fullName, &out.phone, &out.email, &out.gender, &out.notes) + if errors.Is(err, pgx.ErrNoRows) { + return nil + } + out.exists = err == nil + return err +} + +// mergeProfiles keeps the survivor's own values, fills its blanks from the +// source, and returns everything that lost so nothing disappears silently. +func mergeProfiles(src, dst profileFields) (profileFields, []string) { + out := dst + var discarded []string + keep := func(field string, mine *string, theirs string) { + switch { + case theirs == "": + case *mine == "": + *mine = theirs + case *mine != theirs: + discarded = append(discarded, field+": "+theirs) + } + } + keep("name", &out.fullName, src.fullName) + keep("phone", &out.phone, src.phone) + keep("email", &out.email, src.email) + keep("gender", &out.gender, src.gender) + + // Notes are additive rather than a winner: two people writing about one + // customer wrote two different true things. + if src.notes != "" && src.notes != out.notes { + if out.notes == "" { + out.notes = src.notes + } else { + out.notes += "\n" + src.notes + } + } + // And the losers land in notes too, because the response is read once and + // the record is read forever. + if len(discarded) > 0 { + line := "merged, also known as - " + strings.Join(discarded, ", ") + if out.notes == "" { + out.notes = line + } else { + out.notes += "\n" + line + } + } + return out, discarded +} diff --git a/server/internal/store/api_customers_test.go b/server/internal/store/api_customers_test.go new file mode 100644 index 0000000..d5df98a --- /dev/null +++ b/server/internal/store/api_customers_test.go @@ -0,0 +1,71 @@ +package store + +import ( + "strings" + "testing" +) + +// mergeProfiles is pure, so the rule it encodes can be asserted without a +// database - and it is the rule that matters: what happens to the value that +// LOSES. The first version dropped it, and lost a phone number on the first +// live run. +func TestMergingProfilesKeepsWhatLoses(t *testing.T) { + src := profileFields{exists: true, fullName: "Asha M", phone: "111", email: "a@b.c"} + dst := profileFields{exists: true, fullName: "Asha Menon", phone: "222"} + + out, discarded := mergeProfiles(src, dst) + + if out.fullName != "Asha Menon" || out.phone != "222" { + t.Errorf("survivor's own values must win: got %q / %q", out.fullName, out.phone) + } + if out.email != "a@b.c" { + t.Errorf("a blank must be filled from the source, got %q", out.email) + } + if len(discarded) != 2 { + t.Fatalf("discarded %v, want the losing name and phone", discarded) + } + // In the record, not only in the response: the response is read once. + for _, want := range []string{"Asha M", "111"} { + if !strings.Contains(out.notes, want) { + t.Errorf("notes must retain %q: %q", want, out.notes) + } + } +} + +// Nothing to reconcile is the ordinary case - a hand-typed record joining a +// camera record that has no profile at all - and it must add no noise. +func TestMergingIntoAnEmptyProfileDiscardsNothing(t *testing.T) { + src := profileFields{exists: true, fullName: "Asha Menon", phone: "111"} + out, discarded := mergeProfiles(src, profileFields{}) + + if len(discarded) != 0 { + t.Errorf("discarded %v, want none", discarded) + } + if out.fullName != "Asha Menon" || out.phone != "111" { + t.Errorf("the typed details must reach the surviving record: %+v", out) + } + if out.notes != "" { + t.Errorf("no conflict should leave no note, got %q", out.notes) + } +} + +// Identical values are not a conflict. +func TestIdenticalProfileValuesAreNotDiscarded(t *testing.T) { + p := profileFields{exists: true, fullName: "Asha Menon", phone: "111"} + out, discarded := mergeProfiles(p, p) + if len(discarded) != 0 || out.notes != "" { + t.Errorf("identical profiles produced %v / notes %q", discarded, out.notes) + } +} + +// Two people writing about one customer wrote two different true things. +func TestNotesAreAdditiveRatherThanAWinner(t *testing.T) { + out, _ := mergeProfiles( + profileFields{exists: true, notes: "prefers window seat"}, + profileFields{exists: true, notes: "allergic to nuts"}) + for _, want := range []string{"window seat", "allergic to nuts"} { + if !strings.Contains(out.notes, want) { + t.Errorf("notes lost %q: %q", want, out.notes) + } + } +}