A customer nobody has photographed, and the way back when they are seen
POST /api/customers and POST /api/visitors/{id}/merge. They ship together
because the first creates the need for the second: a customer typed in at
a counter has no face template, so when a camera sees that person later
the matcher has nothing to compare against and enrols them as somebody
new. That is the design working, not failing - and it means every
hand-created customer is a duplicate waiting to happen. Shipping the
create alone would manufacture duplicates into the state CLAUDE.md
already flags: "there is no merge endpoint server-side, so its
duplicates would be unrecoverable."
The number comes from clients.visitor_seq, taken exactly as RecordVisit
takes it. Two sources of visitor numbers that could disagree would be
worse than none: V-42 has to mean one person whichever way they arrived.
The label is the typed name, or "Visitor N" when they gave none - the
same string the engine writes, so a record created by hand is
indistinguishable from an enrolled one afterwards.
The merge is one transaction over FIVE tables, and the count is the
point. visits, purchases, visitor_embeddings, consents and
visitor_profiles all reference visitors ON DELETE CASCADE, so a table
this forgets to re-point is not an error - those rows are destroyed with
the source and nobody finds out until a customer's history is short.
visitor_profiles is UNIQUE on visitor_id, so the two cannot simply both
move and something has to win. Blanks on the survivor are filled from the
source and nothing it already holds is overwritten, which is exactly
right for the case this exists for: a hand-typed name and phone joining
the face that was recognised a week later.
Policies carried over from the edge gallery's merge, which had to settle
all of this once already: a human-assigned name outranks an auto
"Visitor N" whichever direction the operator merged; visit_count is
recomputed with COUNT(*) and never summed, because the stored counter may
be stale and the row count cannot be; first_seen_at takes the earlier of
the two, since it is one person and always was.
Two things that are this side's own:
- The source is deleted for real, not soft-deleted. A tombstone would
leave its number resolving to a record holding nothing, which reads as
"this customer exists and has never been here" - a worse answer than
"no such customer".
- The response names the RETIRED reference. Staff write V-42 on cards and
read it aloud; a merge that does not say which one stopped working
leaves somebody to discover it at a counter.
Manager and above, not staff. Apart from erasure this is the only
irreversible operation on a customer: two people welded together cannot
be separated, because nothing records which visit came from whom. It logs
at WARNING and writes an audit row for the same reason.
Also fixed while here: two s.Log.Printf calls - one of them mine, from
the password endpoint - that would panic on a nil logger. The package has
a nil-guarded s.logf and those were the only two not using it. The
password one sat in an error path no test reaches, which is exactly where
that bug waits.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGcjxF1cNLcuwc3DAPcnfj
This commit is contained in:
@@ -137,6 +137,8 @@ type Store interface {
|
||||
|
||||
// --- platform administration ---
|
||||
CreateClientWithOwner(ctx context.Context, in NewClientInput) (NewClientResult, error)
|
||||
CreateCustomer(ctx context.Context, clientID string, in Profile, createdBy string) (Customer, error)
|
||||
MergeVisitors(ctx context.Context, clientID, sourceID, targetID string) (MergeResult, error)
|
||||
Sales(ctx context.Context, q SaleQuery) ([]Sale, error)
|
||||
Sale(ctx context.Context, clientID, id string) (Sale, error)
|
||||
|
||||
@@ -356,6 +358,12 @@ func (s *Server) Routes() *http.ServeMux {
|
||||
mux.HandleFunc("GET /api/visitors", s.tenantOnly(s.handleVisitors))
|
||||
mux.HandleFunc("GET /api/visitors/{id}/history", s.tenantOnly(s.handleVisitorHistory))
|
||||
mux.HandleFunc("PUT /api/visitors/{id}/profile", s.tenantOnly(s.handleSaveProfile))
|
||||
|
||||
// A customer registered before any camera has seen them, and the repair
|
||||
// path that creates the need for: with no face template, recognition
|
||||
// cannot match them later and enrols them again.
|
||||
mux.HandleFunc("POST /api/customers", s.tenantOnly(s.handleCreateCustomer))
|
||||
mux.HandleFunc("POST /api/visitors/{id}/merge", s.tenantOnly(s.handleMergeCustomers))
|
||||
mux.HandleFunc("POST /api/purchases", s.tenantOnly(s.handlePurchase))
|
||||
|
||||
// Reading sales, not just aggregating them. /api/reports/conversion has
|
||||
@@ -612,6 +620,11 @@ func looksLikeUUID(s string) bool {
|
||||
// without importing the store package.
|
||||
var ErrNoSecrets = errors.New("this server has no encryption key, so camera passwords cannot be stored")
|
||||
|
||||
// ErrSameVisitor is a merge that names one customer twice. Declared here
|
||||
// rather than in the store for the reason ErrNoSecrets is: the store imports
|
||||
// this package, so a sentinel the other way round is an import cycle.
|
||||
var ErrSameVisitor = errors.New("a customer cannot be merged into themselves")
|
||||
|
||||
// ErrNoSnapshot means a camera has no stored picture. An ordinary state - a
|
||||
// camera added a minute ago has none - so it is reported as absence, never as
|
||||
// a failure.
|
||||
|
||||
148
server/internal/api/customers_test.go
Normal file
148
server/internal/api/customers_test.go
Normal file
@@ -0,0 +1,148 @@
|
||||
package api
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"net/http"
|
||||
"testing"
|
||||
)
|
||||
|
||||
func TestStaffCanRegisterACustomerNobodyHasPhotographed(t *testing.T) {
|
||||
s, fs := newServer(t)
|
||||
seedUser(fs)
|
||||
sess := login(t, s, "manager@acme.com", "correct horse battery")
|
||||
|
||||
rec := do(t, s, "POST", "/api/customers", sess.Token, map[string]string{
|
||||
"full_name": "Asha Menon", "phone": "9876543210",
|
||||
})
|
||||
if rec.Code != http.StatusCreated {
|
||||
t.Fatalf("got %d: %s", rec.Code, rec.Body.String())
|
||||
}
|
||||
var c Customer
|
||||
if err := json.Unmarshal(rec.Body.Bytes(), &c); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
// A reference a person can say, from the same counter the engine uses.
|
||||
if c.Ref == "" || c.Label != "Asha Menon" {
|
||||
t.Errorf("got ref=%q label=%q, want a V- reference and the typed name",
|
||||
c.Ref, c.Label)
|
||||
}
|
||||
}
|
||||
|
||||
// A record with no name and no phone is a number nobody can search for, and
|
||||
// the customer at the counter is the only source of either.
|
||||
func TestACustomerNeedsANameOrAPhone(t *testing.T) {
|
||||
s, fs := newServer(t)
|
||||
seedUser(fs)
|
||||
sess := login(t, s, "manager@acme.com", "correct horse battery")
|
||||
|
||||
rec := do(t, s, "POST", "/api/customers", sess.Token,
|
||||
map[string]string{"notes": "regular, likes the window seat"})
|
||||
if rec.Code != http.StatusBadRequest {
|
||||
t.Errorf("got %d, want 400: %s", rec.Code, rec.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------- merge
|
||||
|
||||
// Uuid-shaped on purpose: resolveVisitor takes a uuid or a V- reference and
|
||||
// correctly refuses anything else, so a made-up id would 404 before reaching
|
||||
// the handler under test.
|
||||
const (
|
||||
vTyped = "aaaaaaaa-1111-4111-8111-aaaaaaaaaaaa" // typed in at the counter
|
||||
vSeen = "bbbbbbbb-2222-4222-8222-bbbbbbbbbbbb" // enrolled by a camera
|
||||
)
|
||||
|
||||
func seedTwoCustomers(fs *fakeStore) {
|
||||
seedUser(fs)
|
||||
fs.visitors = []Customer{
|
||||
{ID: vTyped, Ref: "V-1", Label: "Asha Menon", FullName: "Asha Menon"},
|
||||
{ID: vSeen, Ref: "V-2", Label: "Visitor 2"},
|
||||
}
|
||||
}
|
||||
|
||||
func TestMergingFoldsOneCustomerIntoTheOtherAndSaysWhatMoved(t *testing.T) {
|
||||
s, fs := newServer(t)
|
||||
seedTwoCustomers(fs)
|
||||
sess := login(t, s, "manager@acme.com", "correct horse battery")
|
||||
|
||||
rec := do(t, s, "POST", "/api/visitors/"+vTyped+"/merge", sess.Token,
|
||||
map[string]string{"into": vSeen})
|
||||
if rec.Code != http.StatusOK {
|
||||
t.Fatalf("got %d: %s", rec.Code, rec.Body.String())
|
||||
}
|
||||
var out MergeResult
|
||||
if err := json.Unmarshal(rec.Body.Bytes(), &out); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
// The reference that STOPPED resolving has to be named. Staff write these
|
||||
// on cards; discovering it at a counter is the wrong place to find out.
|
||||
if out.RetiredRef != "V-1" || out.Ref != "V-2" {
|
||||
t.Errorf("kept %q retired %q, want V-2 kept and V-1 retired", out.Ref, out.RetiredRef)
|
||||
}
|
||||
}
|
||||
|
||||
// The only irreversible operation on a customer apart from erasure. Two people
|
||||
// welded together cannot be separated: nothing records which visit came from
|
||||
// whom.
|
||||
func TestStaffCannotMerge(t *testing.T) {
|
||||
s, fs := newServer(t)
|
||||
seedTwoCustomers(fs)
|
||||
fs.addUser("shopfloor@acme.com", "correct horse battery", UserRecord{
|
||||
ID: "u9", ClientID: "client-acme", Role: "staff", Active: true,
|
||||
})
|
||||
sess := login(t, s, "shopfloor@acme.com", "correct horse battery")
|
||||
|
||||
rec := do(t, s, "POST", "/api/visitors/"+vTyped+"/merge", sess.Token,
|
||||
map[string]string{"into": vSeen})
|
||||
if rec.Code != http.StatusForbidden {
|
||||
t.Errorf("got %d, want 403 for staff: %s", rec.Code, rec.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
func TestMergingACustomerIntoThemselvesIsRefused(t *testing.T) {
|
||||
s, fs := newServer(t)
|
||||
seedTwoCustomers(fs)
|
||||
sess := login(t, s, "manager@acme.com", "correct horse battery")
|
||||
|
||||
rec := do(t, s, "POST", "/api/visitors/"+vTyped+"/merge", sess.Token,
|
||||
map[string]string{"into": vTyped})
|
||||
if rec.Code != http.StatusBadRequest {
|
||||
t.Errorf("got %d, want 400: %s", rec.Code, rec.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
func TestMergingNeedsATarget(t *testing.T) {
|
||||
s, fs := newServer(t)
|
||||
seedTwoCustomers(fs)
|
||||
sess := login(t, s, "manager@acme.com", "correct horse battery")
|
||||
|
||||
for _, body := range []map[string]string{{}, {"into": " "}, {"into": "V-999"}} {
|
||||
rec := do(t, s, "POST", "/api/visitors/"+vTyped+"/merge", sess.Token, body)
|
||||
if rec.Code == http.StatusOK {
|
||||
t.Errorf("merge with %v succeeded, want a refusal", body)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Every merge leaves a trace: it is destructive and cannot be undone.
|
||||
func TestAMergeIsAudited(t *testing.T) {
|
||||
s, fs := newServer(t)
|
||||
seedTwoCustomers(fs)
|
||||
sess := login(t, s, "manager@acme.com", "correct horse battery")
|
||||
|
||||
do(t, s, "POST", "/api/visitors/"+vTyped+"/merge", sess.Token,
|
||||
map[string]string{"into": vSeen})
|
||||
|
||||
found := false
|
||||
for _, a := range fs.audits {
|
||||
if a.Action == "customer.merge" {
|
||||
found = true
|
||||
if a.Detail["retired_ref"] != "V-1" {
|
||||
t.Errorf("audit must name the retired reference: %+v", a.Detail)
|
||||
}
|
||||
}
|
||||
}
|
||||
if !found {
|
||||
t.Error("no audit row for a merge")
|
||||
}
|
||||
}
|
||||
@@ -58,6 +58,8 @@ type fakeStore struct {
|
||||
// cameraOwner method below.
|
||||
siteOwner map[string]string // site id -> client id
|
||||
salesRows []Sale
|
||||
visitorSeq int64
|
||||
lastMerge [2]string
|
||||
saleOwner map[string]string // sale id -> client id
|
||||
lastSaleQuery SaleQuery
|
||||
clientRows map[string]ClientDetail
|
||||
@@ -317,6 +319,57 @@ func (f *fakeStore) SiteHealth(_ context.Context, clientID string) ([]SiteHealth
|
||||
return out, nil
|
||||
}
|
||||
|
||||
func (f *fakeStore) CreateCustomer(_ context.Context, clientID string,
|
||||
in Profile, createdBy string) (Customer, error) {
|
||||
f.mu.Lock()
|
||||
defer f.mu.Unlock()
|
||||
f.visitorSeq++
|
||||
label := in.FullName
|
||||
if label == "" {
|
||||
label = fmt.Sprintf("Visitor %d", f.visitorSeq)
|
||||
}
|
||||
c := Customer{
|
||||
ID: fmt.Sprintf("new-%d", f.visitorSeq), Ref: VisitorRef(f.visitorSeq),
|
||||
Label: label, FullName: in.FullName, Phone: in.Phone, Email: in.Email,
|
||||
HasProfile: true,
|
||||
}
|
||||
f.visitors = append(f.visitors, c)
|
||||
f.lastProfile = in
|
||||
return c, nil
|
||||
}
|
||||
|
||||
func (f *fakeStore) MergeVisitors(_ context.Context, clientID, sourceID, targetID string) (
|
||||
MergeResult, error) {
|
||||
f.mu.Lock()
|
||||
defer f.mu.Unlock()
|
||||
f.lastMerge = [2]string{sourceID, targetID}
|
||||
if sourceID == targetID {
|
||||
return MergeResult{}, ErrSameVisitor
|
||||
}
|
||||
var src, dst *Customer
|
||||
for i := range f.visitors {
|
||||
switch f.visitors[i].ID {
|
||||
case sourceID:
|
||||
src = &f.visitors[i]
|
||||
case targetID:
|
||||
dst = &f.visitors[i]
|
||||
}
|
||||
}
|
||||
if src == nil || dst == nil {
|
||||
return MergeResult{}, pgx.ErrNoRows
|
||||
}
|
||||
out := MergeResult{VisitorID: dst.ID, Ref: dst.Ref, Label: dst.Label,
|
||||
RetiredRef: src.Ref}
|
||||
var kept []Customer
|
||||
for _, c := range f.visitors {
|
||||
if c.ID != sourceID {
|
||||
kept = append(kept, c)
|
||||
}
|
||||
}
|
||||
f.visitors = kept
|
||||
return out, nil
|
||||
}
|
||||
|
||||
func (f *fakeStore) SetUserPassword(_ context.Context, userID, hash string) error {
|
||||
f.mu.Lock()
|
||||
defer f.mu.Unlock()
|
||||
|
||||
134
server/internal/api/handlers_customers.go
Normal file
134
server/internal/api/handlers_customers.go
Normal file
@@ -0,0 +1,134 @@
|
||||
// Registering a customer nobody has photographed, and joining two records
|
||||
// that are one person.
|
||||
//
|
||||
// They ship together because the first creates the need for the second. A
|
||||
// customer typed in at a counter has no face template, so when a camera later
|
||||
// sees that person the matcher has nothing to compare against and enrols them
|
||||
// as somebody new. That is the design working, not failing - and it means
|
||||
// every hand-created customer is a duplicate waiting to happen. The merge is
|
||||
// the way back, and without it this pair of endpoints would manufacture
|
||||
// unrecoverable duplicates.
|
||||
package api
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"net/http"
|
||||
|
||||
"github.com/jackc/pgx/v5"
|
||||
)
|
||||
|
||||
// handleCreateCustomer is staff and above - the same bar as filling in a
|
||||
// profile, because that is what this is: a profile that arrives before the
|
||||
// face rather than after it.
|
||||
func (s *Server) handleCreateCustomer(w http.ResponseWriter, r *http.Request) {
|
||||
p := PrincipalFrom(r.Context())
|
||||
if !p.CanWriteProfiles() {
|
||||
writeErr(w, http.StatusForbidden, "forbidden",
|
||||
"Staff and above can add a customer.")
|
||||
return
|
||||
}
|
||||
|
||||
var in Profile
|
||||
if err := decode(w, r, &in); err != nil {
|
||||
badRequest(w, err.Error())
|
||||
return
|
||||
}
|
||||
in.FullName = clip(trim(in.FullName), 200)
|
||||
in.Phone = clip(trim(in.Phone), 40)
|
||||
in.Email = clip(trim(in.Email), 200)
|
||||
in.Gender = clip(trim(in.Gender), 40)
|
||||
in.Notes = clip(trim(in.Notes), 2000)
|
||||
|
||||
// Something has to identify them to a human. A record with no name and no
|
||||
// phone is a number nobody can search for, and the customer standing at
|
||||
// the counter is the only source of either.
|
||||
if in.FullName == "" && in.Phone == "" {
|
||||
badRequest(w, "give at least a name or a phone number")
|
||||
return
|
||||
}
|
||||
|
||||
out, err := s.Store.CreateCustomer(r.Context(), p.ClientID, in, p.UserID)
|
||||
if err != nil {
|
||||
s.serverError(w, "create customer", err)
|
||||
return
|
||||
}
|
||||
|
||||
s.Store.Audit(r.Context(), AuditEntry{
|
||||
ClientID: p.ClientID, ActorID: p.UserID, ActorKind: "user",
|
||||
Action: "customer.create", Entity: "visitor", EntityID: out.ID,
|
||||
Detail: map[string]any{"ref": out.Ref},
|
||||
})
|
||||
writeJSON(w, http.StatusCreated, out)
|
||||
}
|
||||
|
||||
// handleMergeCustomers folds one customer into another.
|
||||
//
|
||||
// Manager and above, not staff. This is the only irreversible operation on a
|
||||
// customer record apart from erasure: two people welded together cannot be
|
||||
// separated afterwards, because nothing records which visit came from whom.
|
||||
// The edge gallery draws the same line for the same reason.
|
||||
func (s *Server) handleMergeCustomers(w http.ResponseWriter, r *http.Request) {
|
||||
p := PrincipalFrom(r.Context())
|
||||
if !p.CanManageSites() {
|
||||
writeErr(w, http.StatusForbidden, "forbidden",
|
||||
"Merging two customers cannot be undone; a manager or owner must do it.")
|
||||
return
|
||||
}
|
||||
|
||||
source, ok := s.resolveVisitor(w, r, r.PathValue("id"))
|
||||
if !ok {
|
||||
return
|
||||
}
|
||||
var in MergeRequest
|
||||
if err := decode(w, r, &in); err != nil {
|
||||
badRequest(w, err.Error())
|
||||
return
|
||||
}
|
||||
if trim(in.Into) == "" {
|
||||
badRequest(w, `"into" must name the customer to keep`)
|
||||
return
|
||||
}
|
||||
// Resolved through the same path, so "into" accepts V-42 as well as a
|
||||
// uuid - the reference staff actually read off a screen.
|
||||
target, err := s.visitorIDFor(r.Context(), p.ClientID, trim(in.Into))
|
||||
if err != nil {
|
||||
s.serverError(w, "resolve customer", err)
|
||||
return
|
||||
}
|
||||
if target == "" {
|
||||
writeErr(w, http.StatusNotFound, "not_found", "No such customer to merge into.")
|
||||
return
|
||||
}
|
||||
|
||||
out, err := s.Store.MergeVisitors(r.Context(), p.ClientID, source, target)
|
||||
switch {
|
||||
case errors.Is(err, ErrSameVisitor):
|
||||
badRequest(w, "that is the same customer")
|
||||
return
|
||||
case errors.Is(err, pgx.ErrNoRows):
|
||||
// One of the two is gone, erased, or another tenant's. All three read
|
||||
// as absent; which one it is only helps somebody probing ids.
|
||||
writeErr(w, http.StatusNotFound, "not_found", "No such customer.")
|
||||
return
|
||||
case err != nil:
|
||||
s.serverError(w, "merge customers", err)
|
||||
return
|
||||
}
|
||||
|
||||
// Irreversible, so it leaves a trace at WARNING as well as in the audit
|
||||
// log - the same rule the edge gallery's merge follows.
|
||||
s.logf("WARNING merge: customer %s (%s) folded into %s (%s) by %s: "+
|
||||
"%d visits, %d purchases, %d templates moved",
|
||||
source, out.RetiredRef, out.VisitorID, out.Ref, p.Email,
|
||||
out.Visits, out.Purchases, out.Embeddings)
|
||||
s.Store.Audit(r.Context(), AuditEntry{
|
||||
ClientID: p.ClientID, ActorID: p.UserID, ActorKind: "user",
|
||||
Action: "customer.merge", Entity: "visitor", EntityID: out.VisitorID,
|
||||
Detail: map[string]any{
|
||||
"retired_ref": out.RetiredRef, "kept_ref": out.Ref,
|
||||
"visits": out.Visits, "purchases": out.Purchases,
|
||||
"embeddings": out.Embeddings,
|
||||
},
|
||||
})
|
||||
writeJSON(w, http.StatusOK, out)
|
||||
}
|
||||
@@ -77,7 +77,7 @@ func (s *Server) handleChangePassword(w http.ResponseWriter, r *http.Request) {
|
||||
// The password IS changed. Reporting a failure here would tell the
|
||||
// user to try again, and the retry would fail on the current password
|
||||
// they just replaced.
|
||||
s.Log.Printf("change password: revoke other sessions: %v", err)
|
||||
s.logf("change password: revoke other sessions: %v", err)
|
||||
}
|
||||
|
||||
s.Store.Audit(r.Context(), AuditEntry{
|
||||
|
||||
@@ -180,6 +180,27 @@ type ChangePassword struct {
|
||||
NewPassword string `json:"new_password"`
|
||||
}
|
||||
|
||||
// MergeResult says what moved, so an operator sees the size of a thing that
|
||||
// cannot be undone rather than a bare "ok".
|
||||
type MergeResult struct {
|
||||
VisitorID string `json:"visitor_id"`
|
||||
Ref string `json:"ref"`
|
||||
Label string `json:"label"`
|
||||
Visits int `json:"visits"`
|
||||
Purchases int `json:"purchases"`
|
||||
Embeddings int `json:"embeddings"`
|
||||
Consents int `json:"consents"`
|
||||
// RetiredRef is the reference that has STOPPED resolving. Staff write
|
||||
// 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"`
|
||||
}
|
||||
|
||||
// MergeRequest names the record to keep.
|
||||
type MergeRequest struct {
|
||||
Into string `json:"into"`
|
||||
}
|
||||
|
||||
type Customer struct {
|
||||
ID string `json:"id"`
|
||||
// Ref is the customer number - "V-42" - and is accepted anywhere this
|
||||
|
||||
Reference in New Issue
Block a user