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) + } + } +}