diff --git a/internal/app/contact/import.go b/internal/app/contact/import.go index 83b83401..45401d12 100644 --- a/internal/app/contact/import.go +++ b/internal/app/contact/import.go @@ -3,7 +3,6 @@ package contact import ( "context" "encoding/csv" - "errors" "fmt" "io" "path/filepath" @@ -62,6 +61,88 @@ func (s *contactService) ImportPreview(ctx context.Context, r io.Reader, filenam }, nil } +// importColumn is one validated mapping entry: exactly one destination +// for one column index. Building these up front means a bad mapping is a +// single actionable 400 instead of the same message repeated once per row. +type importColumn struct { + index int + target models.ContactImportColumnTarget + customKey string +} + +// resolveMapping validates the client's column mapping once, before any +// row is touched. It returns the columns that actually go somewhere. +func resolveMapping(mapping []models.ContactImportColumnMapping) ([]importColumn, *errx.Error) { + out := make([]importColumn, 0, len(mapping)) + hasEmail := false + for _, m := range mapping { + if m.Index < 0 { + return nil, errx.New(errx.BadRequest, + fmt.Sprintf("column index %d is out of range", m.Index)) + } + target := m.Target + key := strings.TrimSpace(m.CustomKey) + // "custom:" is the legacy spelling of {target:"custom", + // custom_key:""}; an explicit custom_key still wins. + if rest, ok := strings.CutPrefix(string(target), string(models.ContactImportTargetCustom)+":"); ok { + target = models.ContactImportTargetCustom + if key == "" { + key = strings.TrimSpace(rest) + } + } + + switch target { + case models.ContactImportTargetIgnore, "": + continue + case models.ContactImportTargetEmail: + hasEmail = true + case models.ContactImportTargetFirstName, + models.ContactImportTargetLastName, + models.ContactImportTargetCompany, + models.ContactImportTargetPhone, + models.ContactImportTargetSubscribed, + models.ContactImportTargetCategories: + case models.ContactImportTargetCustom: + default: + // An unrecognised target with a custom key is how older clients + // spelled a custom field; anything else is a client bug. + if key == "" { + return nil, errx.New(errx.BadRequest, + "unknown target "+strconv.Quote(string(m.Target))+" for column "+strconv.Itoa(m.Index+1)) + } + target = models.ContactImportTargetCustom + } + + if target == models.ContactImportTargetCustom { + key = utils.NormalizeJSONKey(key) + if key == "" { + return nil, errx.New(errx.BadRequest, + fmt.Sprintf("column %d is mapped to a custom field but has no name", m.Index+1)) + } + if !utils.IsValidJSONKey(key) { + return nil, errx.New(errx.BadRequest, + "invalid custom field name "+strconv.Quote(key)+": "+utils.JSONKeyRules) + } + } + + out = append(out, importColumn{index: m.Index, target: target, customKey: key}) + } + if !hasEmail { + return nil, errx.New(errx.BadRequest, "map one column to Email before importing") + } + return out, nil +} + +// ValidateImportMapping exposes resolveMapping's verdict without running an +// import, so a saved mapping can be rejected at save time. +func (s *contactService) ValidateImportMapping(mapping []models.ContactImportColumnMapping) *errx.Error { + if len(mapping) == 0 { + return errx.New(errx.BadRequest, "no column mapping provided") + } + _, xerr := resolveMapping(mapping) + return xerr +} + // ImportCommit re-parses the file and writes the upsert. We don't share // state with ImportPreview on purpose — keeping the path stateless // makes the commit safe to retry without an opaque "session id". @@ -102,10 +183,24 @@ func (s *contactService) ImportCommit( subscribedDefault = *opts.SubscribedDefault } - // Validate category IDs are well-formed UUIDs. Ownership scoping - // happens later inside the repo (the INSERT joins against - // categories.user_id) so we don't need to round-trip the DB here. - catIDs, xerr := parseLocalCategoryIDs(opts.CategoryIDs) + // The mapping is validated once, up front: a mistyped custom-field name + // is one 400 the user can act on, not the same row error 50,000 times. + columns, xerr := resolveMapping(opts.Mapping) + if xerr != nil { + return nil, xerr + } + + // Validate the category and campaign IDs up front. Ownership scoping + // happens later inside the repo (the INSERTs join against + // categories.user_id / campaigns.organization_id) so we don't need to + // round-trip the DB here, but they do have to be well-formed UUIDs: a + // blank one reaches Postgres as `'' = ANY($1::uuid[])` and fails the + // statement, which used to surface as every row failing to link. + globalCatIDs, xerr := parseIDList(opts.CategoryIDs) + if xerr != nil { + return nil, xerr + } + globalCampaignIDs, xerr := parseIDList(opts.CampaignIDs) if xerr != nil { return nil, xerr } @@ -134,19 +229,27 @@ func (s *contactService) ImportCommit( // Build the parsed contacts up front so we can pre-check // collisions in one DB round trip instead of N. type pendingRow struct { - line int - raw []string - contact models.AddContact - ok bool - errMsg string + line int + raw []string + contact models.AddContact + categories []string // category titles read from the file + ok bool + // dupInFile marks a row whose address an earlier row already claimed. + // Not an error: it counts as skipped and its data was merged. + dupInFile bool + errMsg string } parsed := make([]pendingRow, 0, len(data)) + // firstByEmail points at the first pending row that claimed an address, so + // a file that lists the same person twice produces one contact instead of + // two upserts of the same row counted as two imports. + firstByEmail := make(map[string]int, len(data)) for i, row := range data { line := i + dataStart + 1 // 1-based for "open in Excel and jump" p := pendingRow{line: line, raw: row} - contact, err := buildAddContact(row, opts.Mapping, subscribedDefault, opts.CampaignIDs, opts.CategoryIDs) + contact, cats, err := buildAddContact(row, columns, globalCampaignIDs, globalCatIDs) if err != "" { p.errMsg = err parsed = append(parsed, p) @@ -159,11 +262,49 @@ func (s *contactService) ImportCommit( continue } contact.Email = strings.ToLower(contact.Email) + + if prev, dup := firstByEmail[contact.Email]; dup { + // Same address twice in one file. "skip" keeps the first row; + // the other strategies merge the later row onto it so no data + // from the file is silently dropped. + if dedup != models.ContactImportDedupSkip { + mergeAddContact(&parsed[prev].contact, contact) + parsed[prev].categories = appendUnique(parsed[prev].categories, cats...) + } + p.contact = contact + p.dupInFile = true + parsed = append(parsed, p) + continue + } + firstByEmail[contact.Email] = len(parsed) + p.contact = contact + p.categories = cats p.ok = true parsed = append(parsed, p) } + // Resolve every category title the file mentions in one round trip, + // creating the ones the user doesn't have yet. + titleToID := map[string]uuid.UUID{} + var allTitles []string + for i := range parsed { + allTitles = append(allTitles, parsed[i].categories...) + } + if len(allTitles) > 0 { + titleToID, xerr = s.contactRepository.ResolveCategoryNames(ctx, uid, allTitles) + if xerr != nil { + return nil, xerr + } + for i := range parsed { + for _, title := range parsed[i].categories { + if id, ok := titleToID[strings.ToLower(strings.TrimSpace(title))]; ok { + parsed[i].contact.Categories = appendUnique(parsed[i].contact.Categories, id.String()) + } + } + } + } + // Pre-check existing emails in one shot so we can route rows to // the right path (skip / update / dup). emails := make([]string, 0, len(parsed)) @@ -182,6 +323,21 @@ func (s *contactService) ImportCommit( StartedAt: startedAt, Errors: make([]models.ContactImportRowError, 0), } + // warn records a row-level note without counting the row as failed, so + // Total always equals imported + updated + skipped + failed. + warn := func(line int, addr string, values []string, reason string) { + if len(res.Errors) >= models.MaxContactImportReportedErrors { + res.ErrorsTruncated = true + return + } + res.Errors = append(res.Errors, models.ContactImportRowError{ + Line: line, Email: addr, Values: values, Reason: reason, + }) + } + fail := func(line int, addr string, values []string, reason string) { + res.Failed++ + warn(line, addr, values, reason) + } // Bucket rows by target action. We send fresh inserts through // contactRepository.Add in batches and fall back to per-row @@ -190,22 +346,41 @@ func (s *contactService) ImportCommit( toInsert := make([]models.AddContact, 0, len(parsed)) toInsertLines := make([]int, 0, len(parsed)) toUpdate := make([]pendingRow, 0) + // Existing contacts the file listed but did not change. They still have + // to join the campaign / categories this import targets: "skip" means + // "don't touch their fields", not "leave them out of the list". + skippedLinks := make([]linkTarget, 0) for _, p := range parsed { if !p.ok { - res.Failed++ - res.Errors = append(res.Errors, models.ContactImportRowError{ - Line: p.line, Email: p.contact.Email, Values: p.raw, Reason: p.errMsg, - }) + if p.dupInFile { + res.Skipped++ + continue + } + fail(p.line, p.contact.Email, p.raw, p.errMsg) continue } - _, dup := existing[p.contact.Email] + ex, dup := existing[p.contact.Email] switch { case !dup: + // SubscribedDefault is what a NEW contact inherits. An update must + // never touch the flag, or re-importing a list would resubscribe + // everyone who had opted out. + if p.contact.Subscribed == nil { + sub := subscribedDefault + p.contact.Subscribed = &sub + } toInsert = append(toInsert, p.contact) toInsertLines = append(toInsertLines, p.line) case dedup == models.ContactImportDedupSkip: res.Skipped++ + skippedLinks = append(skippedLinks, linkTarget{ + line: p.line, + email: p.contact.Email, + contactID: ex.ID.String(), + campaigns: p.contact.Campaigns, + categories: p.contact.Categories, + }) case dedup == models.ContactImportDedupUpdate: toUpdate = append(toUpdate, p) case dedup == models.ContactImportDedupCreateDuplicate: @@ -218,6 +393,13 @@ func (s *contactService) ImportCommit( } } + // Ask about the plan ceiling once for the whole batch. Per chunk it would + // report the same plan problem 500 times, which is what the row-level + // error list is explicitly not for. + if xerr := s.checkContactLimit(ctx, userID, len(toInsert)); xerr != nil { + return nil, xerr + } + // Insert in chunks so a 50k row import doesn't blow up a single // pgx batch. 500 lines up with the Search page size. for start := 0; start < len(toInsert); start += 500 { @@ -231,12 +413,7 @@ func (s *contactService) ImportCommit( // Per-row reasons are easier to act on than a "batch // failed" — record each as failed with the same reason. for i, p := range chunk { - res.Failed++ - res.Errors = append(res.Errors, models.ContactImportRowError{ - Line: toInsertLines[start+i], - Email: p.Email, - Reason: xerr.Message, - }) + fail(toInsertLines[start+i], p.Email, nil, xerr.Message) } continue } @@ -249,10 +426,11 @@ func (s *contactService) ImportCommit( idStr := ex.ID.String() update := &models.UpdateContact{ - FirstName: optString(p.contact.FirstName, ex.FirstName), - LastName: optString(p.contact.LastName, ex.LastName), - Company: optString(p.contact.Company, ex.Company), - Phone: optString(p.contact.Phone, ex.Phone), + FirstName: optString(p.contact.FirstName), + LastName: optString(p.contact.LastName), + Company: optString(p.contact.Company), + Phone: optString(p.contact.Phone), + Subscribed: p.contact.Subscribed, } if len(p.contact.CustomFields) > 0 { merged := make(map[string]string, len(p.contact.CustomFields)) @@ -261,20 +439,11 @@ func (s *contactService) ImportCommit( } update.CustomFields = &merged } - if len(catIDs) > 0 { - ids := make([]string, len(catIDs)) - for i, id := range catIDs { - ids[i] = id.String() - } - update.AddCategories = ids + if len(p.contact.Categories) > 0 { + update.AddCategories = p.contact.Categories } if _, xerr := s.contactRepository.Update(ctx, userID, idStr, orgID, update); xerr != nil { - res.Failed++ - res.Errors = append(res.Errors, models.ContactImportRowError{ - Line: p.line, - Email: p.contact.Email, - Reason: xerr.Message, - }) + fail(p.line, p.contact.Email, nil, xerr.Message) continue } res.Updated++ @@ -285,26 +454,129 @@ func (s *contactService) ImportCommit( Contacts: []string{idStr}, AddCampaigns: p.contact.Campaigns, }); xerr != nil { - // Non-fatal — the contact was updated, the link - // failed. Surface as a row-level warning. - res.Errors = append(res.Errors, models.ContactImportRowError{ - Line: p.line, - Email: p.contact.Email, - Reason: "contact updated but campaign link failed: " + xerr.Message, - }) + // Non-fatal: the contact was updated, only the link failed. + // Surface it as a row note, not as a failed row. + warn(p.line, p.contact.Email, nil, "contact updated but campaign link failed: "+xerr.Message) + } + } + } + + // One BulkUpdate per distinct (campaigns, categories) set covers every + // skipped contact that shares it, so the common case (one campaign, one + // category list for the whole file) is a single statement. + for _, group := range groupLinks(skippedLinks) { + if len(group.campaigns) == 0 && len(group.categories) == 0 { + continue + } + if _, xerr := s.contactRepository.BulkUpdate(ctx, userID, orgID, &models.BulkEditContactsData{ + Contacts: group.contactIDs, + AddCampaigns: group.campaigns, + AddCategories: group.categories, + }); xerr != nil { + // The rows that were imported are fine; only these links failed. + // Move the affected rows from skipped to failed rather than + // discarding the whole result. + for _, m := range group.members { + res.Skipped-- + fail(m.line, m.email, nil, + "contact already existed but could not be added to the campaign: "+xerr.Message) } } } res.EndedAt = time.Now().UTC() - if res.Imported > 0 || res.Updated > 0 { + if res.Imported > 0 || res.Updated > 0 || len(skippedLinks) > 0 { s.publishContactsReload(ctx, userID, "contacts:import") // Covers the Google Sheets sync too: it commits through this path. - s.wakeCampaigns(ctx, orgID, opts.CampaignIDs) + s.wakeCampaigns(ctx, orgID, globalCampaignIDs) } return res, nil } +// linkTarget is an existing contact that must join the import's campaigns and +// categories even though its own fields were left alone. +type linkTarget struct { + line int + email string + contactID string + campaigns []string + categories []string +} + +type linkGroup struct { + campaigns []string + categories []string + contactIDs []string + members []linkTarget +} + +func groupLinks(targets []linkTarget) []linkGroup { + byKey := map[string]*linkGroup{} + order := make([]string, 0, 1) + for _, t := range targets { + key := strings.Join(t.campaigns, ",") + "|" + strings.Join(t.categories, ",") + g, ok := byKey[key] + if !ok { + g = &linkGroup{campaigns: t.campaigns, categories: t.categories} + byKey[key] = g + order = append(order, key) + } + g.contactIDs = append(g.contactIDs, t.contactID) + g.members = append(g.members, t) + } + out := make([]linkGroup, 0, len(order)) + for _, key := range order { + out = append(out, *byKey[key]) + } + return out +} + +// mergeAddContact folds a later row for the same address onto the first one. +// Non-empty incoming values win; blanks never erase what an earlier row set. +func mergeAddContact(dst *models.AddContact, src models.AddContact) { + if strings.TrimSpace(src.FirstName) != "" { + dst.FirstName = src.FirstName + } + if strings.TrimSpace(src.LastName) != "" { + dst.LastName = src.LastName + } + if strings.TrimSpace(src.Company) != "" { + dst.Company = src.Company + } + if strings.TrimSpace(src.Phone) != "" { + dst.Phone = src.Phone + } + if src.Subscribed != nil { + dst.Subscribed = src.Subscribed + } + if len(src.CustomFields) > 0 { + if dst.CustomFields == nil { + dst.CustomFields = map[string]string{} + } + for k, v := range src.CustomFields { + dst.CustomFields[k] = v + } + } + dst.Campaigns = appendUnique(dst.Campaigns, src.Campaigns...) + dst.Categories = appendUnique(dst.Categories, src.Categories...) +} + +func appendUnique(dst []string, add ...string) []string { + for _, v := range add { + found := false + for _, have := range dst { + if have == v { + found = true + break + } + } + if !found { + dst = append(dst, v) + } + } + return dst +} + // parseSpreadsheet returns rows as a 2-D slice and the detected format. // CSV is decoded with the stdlib (forgiving about trailing commas / // quoting), XLSX is decoded with excelize. Anything else 400s. @@ -434,34 +706,30 @@ func guessTarget(idx int, header string) models.ContactImportColumnMapping { return models.ContactImportColumnMapping{Index: idx, Target: models.ContactImportTargetIgnore} } -// buildAddContact applies the column mapping to a single row. Returns -// either a fully-populated AddContact or a reason string explaining why -// the row was rejected. We don't bail on the first bad field — we -// gather everything so the user sees one good error. +// buildAddContact applies the resolved column mapping to a single row. +// It returns the contact, the category titles the row named, and a reason +// string when the row itself is unusable. func buildAddContact( row []string, - mapping []models.ContactImportColumnMapping, - subscribedDefault bool, + columns []importColumn, defaultCampaignIDs []string, defaultCategoryIDs []string, -) (models.AddContact, string) { +) (models.AddContact, []string, string) { ac := models.AddContact{ CustomFields: map[string]string{}, Campaigns: append([]string{}, defaultCampaignIDs...), Categories: append([]string{}, defaultCategoryIDs...), } - subscribedSet := false - for _, m := range mapping { - if m.Index < 0 || m.Index >= len(row) { + var categories []string + for _, col := range columns { + if col.index >= len(row) { continue } - val := strings.TrimSpace(row[m.Index]) + val := strings.TrimSpace(row[col.index]) if val == "" { continue } - switch m.Target { - case models.ContactImportTargetIgnore: - continue + switch col.target { case models.ContactImportTargetEmail: ac.Email = val case models.ContactImportTargetFirstName: @@ -473,38 +741,25 @@ func buildAddContact( case models.ContactImportTargetPhone: ac.Phone = val case models.ContactImportTargetSubscribed: - subscribedSet = true b, perr := parseBoolish(val) if perr != "" { - return models.AddContact{}, perr + return models.AddContact{}, nil, perr } - _ = b // not used: we don't have a way to push it into AddContact yet + sub := b + ac.Subscribed = &sub case models.ContactImportTargetCategories: - // Comma-separated list of category names — caller could - // also pass IDs but names are friendlier for CSV - // round-trips. For now we ignore names from the file - // (we'd need a lookup); the bulk category assignment - // applied by `opts.CategoryIDs` covers the common case. - _ = val - default: - if strings.HasPrefix(string(m.Target), "custom:") { - key := strings.TrimPrefix(string(m.Target), "custom:") - if key == "" || !utils.IsValidJSONKey(key) { - return models.AddContact{}, "invalid custom field key: " + key + // Comma- or semicolon-separated category titles. Resolved to ids + // (creating what's missing) once for the whole file by the caller. + for _, name := range strings.FieldsFunc(val, func(r rune) bool { return r == ',' || r == ';' }) { + if name = strings.TrimSpace(name); name != "" { + categories = appendUnique(categories, name) } - ac.CustomFields[key] = val - } - if m.CustomKey != "" { - if !utils.IsValidJSONKey(m.CustomKey) { - return models.AddContact{}, "invalid custom field key: " + m.CustomKey - } - ac.CustomFields[m.CustomKey] = val } + case models.ContactImportTargetCustom: + ac.CustomFields[col.customKey] = val } } - _ = subscribedSet // AddContact doesn't carry subscribed; default applies at row creation - _ = subscribedDefault - return ac, "" + return ac, categories, "" } // parseBoolish accepts the strings real CSV exporters emit for boolean @@ -520,27 +775,27 @@ func parseBoolish(v string) (bool, string) { return false, "could not parse subscribed value: " + v } -// optString returns a pointer to `incoming` if non-empty, else `fallback`. +// optString returns a pointer to `incoming` when it has content, else nil. // Used in the update path to avoid blanking a populated field with an // empty CSV cell — the importer's job is to enrich, not erase. -func optString(incoming, fallback string) *string { +func optString(incoming string) *string { if strings.TrimSpace(incoming) == "" { return nil } v := incoming - _ = fallback return &v } -// parseLocalCategoryIDs is the import-package twin of pg_contact's -// parseUUIDList. Kept private and small so we don't depend on the -// repository package's internals. -func parseLocalCategoryIDs(raw []string) ([]uuid.UUID, *errx.Error) { +// parseIDList canonicalises a list of UUID strings from the request: blanks +// dropped, duplicates removed, malformed rejected. It is the import-package +// twin of pg_contact's parseUUIDList, kept private and small so we don't +// depend on the repository package's internals. +func parseIDList(raw []string) ([]string, *errx.Error) { if len(raw) == 0 { return nil, nil } seen := make(map[uuid.UUID]struct{}, len(raw)) - out := make([]uuid.UUID, 0, len(raw)) + out := make([]string, 0, len(raw)) for _, s := range raw { s = strings.TrimSpace(s) if s == "" { @@ -554,9 +809,7 @@ func parseLocalCategoryIDs(raw []string) ([]uuid.UUID, *errx.Error) { continue } seen[id] = struct{}{} - out = append(out, id) + out = append(out, id.String()) } return out, nil } - -var _ = errors.New diff --git a/internal/app/contact/import_mapping_test.go b/internal/app/contact/import_mapping_test.go new file mode 100644 index 00000000..b1e218c4 --- /dev/null +++ b/internal/app/contact/import_mapping_test.go @@ -0,0 +1,137 @@ +package contact + +import ( + "strings" + "testing" + + "github.com/warmbly/warmbly/internal/models" +) + +// Issue #207: a mistyped custom-field name used to be reported once per row +// (1,000 identical errors for a 1,000-row file). The mapping is now resolved +// once, before any row is read. + +func mapCol(idx int, target models.ContactImportColumnTarget) models.ContactImportColumnMapping { + return models.ContactImportColumnMapping{Index: idx, Target: target} +} + +func TestResolveMapping(t *testing.T) { + emailCol := mapCol(0, models.ContactImportTargetEmail) + + t.Run("keeps only the columns that go somewhere", func(t *testing.T) { + cols, xerr := resolveMapping([]models.ContactImportColumnMapping{ + emailCol, + mapCol(1, models.ContactImportTargetIgnore), + {Index: 2, Target: models.ContactImportTargetCustom, CustomKey: " Company Mobile "}, + }) + if xerr != nil { + t.Fatalf("unexpected error: %s", xerr.Message) + } + if len(cols) != 2 { + t.Fatalf("got %d columns, want 2 (%+v)", len(cols), cols) + } + if cols[1].customKey != "Company Mobile" { + t.Fatalf("key not normalized: %q", cols[1].customKey) + } + }) + + t.Run("accepts the legacy custom: target", func(t *testing.T) { + cols, xerr := resolveMapping([]models.ContactImportColumnMapping{ + emailCol, {Index: 1, Target: "custom:plan_tier"}, + }) + if xerr != nil { + t.Fatalf("unexpected error: %s", xerr.Message) + } + if cols[1].target != models.ContactImportTargetCustom || cols[1].customKey != "plan_tier" { + t.Fatalf("legacy target not resolved: %+v", cols[1]) + } + }) + + t.Run("an explicit custom_key wins over the target suffix", func(t *testing.T) { + cols, _ := resolveMapping([]models.ContactImportColumnMapping{ + emailCol, {Index: 1, Target: "custom:old", CustomKey: "new"}, + }) + if cols[1].customKey != "new" { + t.Fatalf("custom_key ignored: %q", cols[1].customKey) + } + }) + + for name, tc := range map[string]struct { + mapping []models.ContactImportColumnMapping + wants string + }{ + "unusable name": { + []models.ContactImportColumnMapping{emailCol, {Index: 5, Target: models.ContactImportTargetCustom, CustomKey: "Company/Mobile"}}, + "Company/Mobile", + }, + "no name": { + []models.ContactImportColumnMapping{emailCol, {Index: 5, Target: models.ContactImportTargetCustom}}, + "column 6 is mapped to a custom field but has no name", + }, + "no email": { + []models.ContactImportColumnMapping{mapCol(0, models.ContactImportTargetFirstName)}, + "Email", + }, + "unknown target": { + []models.ContactImportColumnMapping{emailCol, mapCol(1, "middle_name")}, + "unknown target", + }, + "negative index": { + []models.ContactImportColumnMapping{{Index: -1, Target: models.ContactImportTargetEmail}}, + "out of range", + }, + } { + t.Run(name, func(t *testing.T) { + _, xerr := resolveMapping(tc.mapping) + if xerr == nil { + t.Fatalf("expected an error mentioning %q", tc.wants) + } + if !strings.Contains(xerr.Message, tc.wants) { + t.Fatalf("message %q does not mention %q", xerr.Message, tc.wants) + } + }) + } +} + +func TestBuildAddContactReadsEveryTarget(t *testing.T) { + cols, xerr := resolveMapping([]models.ContactImportColumnMapping{ + mapCol(0, models.ContactImportTargetEmail), + mapCol(1, models.ContactImportTargetFirstName), + mapCol(2, models.ContactImportTargetSubscribed), + mapCol(3, models.ContactImportTargetCategories), + {Index: 4, Target: models.ContactImportTargetCustom, CustomKey: "Company Mobile"}, + }) + if xerr != nil { + t.Fatalf("resolve: %s", xerr.Message) + } + + row := []string{"dana@acme.com", "Dana", "unsubscribed", "Agency; Enterprise , Agency", "+15550000"} + ac, cats, reason := buildAddContact(row, cols, nil, nil) + if reason != "" { + t.Fatalf("row rejected: %s", reason) + } + if ac.Email != "dana@acme.com" || ac.FirstName != "Dana" { + t.Fatalf("identity fields not read: %+v", ac) + } + if ac.Subscribed == nil || *ac.Subscribed { + t.Fatalf("subscribed column not honoured: %v", ac.Subscribed) + } + if ac.CustomFields["Company Mobile"] != "+15550000" { + t.Fatalf("custom field not stored: %+v", ac.CustomFields) + } + if len(cats) != 2 || cats[0] != "Agency" || cats[1] != "Enterprise" { + t.Fatalf("category titles: %+v", cats) + } + + // No subscribed column means "leave it to the caller's default", which is + // what keeps an update from resubscribing someone who opted out. + bare, _, _ := buildAddContact([]string{"x@y.com"}, cols[:1], nil, nil) + if bare.Subscribed != nil { + t.Fatalf("subscribed was decided without a column: %v", *bare.Subscribed) + } + + // A value the importer cannot read fails that row, not the import. + if _, _, reason = buildAddContact([]string{"a@b.com", "A", "maybe", "", ""}, cols, nil, nil); reason == "" { + t.Fatal("an unreadable subscribed value should fail the row") + } +} diff --git a/internal/app/contact/service.go b/internal/app/contact/service.go index 7d5a4180..9584732f 100644 --- a/internal/app/contact/service.go +++ b/internal/app/contact/service.go @@ -33,6 +33,12 @@ type ContactService interface { // the columns + first N rows + suggested mapping — no DB writes. ImportPreview(ctx context.Context, file io.Reader, filename string) (*models.ContactImportPreview, *errx.Error) + // ValidateImportMapping reports whether a column mapping is usable: + // exactly the checks ImportCommit runs before it touches a row. Callers + // that persist a mapping for later (the Google Sheets sync sources) use + // it so a bad mapping is caught when it is saved, not on the next sync. + ValidateImportMapping(mapping []models.ContactImportColumnMapping) *errx.Error + // ImportCommit re-parses the uploaded file with the chosen mapping // and performs the upsert / skip / dedup work. Returns per-row // result counts plus a list of rows that failed (with reasons). diff --git a/internal/app/leadsync/service.go b/internal/app/leadsync/service.go index d5b78a22..87ada052 100644 --- a/internal/app/leadsync/service.go +++ b/internal/app/leadsync/service.go @@ -145,7 +145,7 @@ func (s *service) Create(ctx context.Context, orgID, userID uuid.UUID, in *model if xerr := validateDedup(in.Dedup); xerr != nil { return nil, xerr } - if xerr := validateMappingHasEmail(in.ColumnMapping); xerr != nil { + if xerr := s.contacts.ValidateImportMapping(in.ColumnMapping); xerr != nil { return nil, xerr } @@ -221,7 +221,7 @@ func (s *service) Update(ctx context.Context, orgID, id uuid.UUID, in *models.Up if len(*in.ColumnMapping) == 0 { return nil, errx.New(errx.BadRequest, "column_mapping cannot be empty") } - if xerr := validateMappingHasEmail(*in.ColumnMapping); xerr != nil { + if xerr := s.contacts.ValidateImportMapping(*in.ColumnMapping); xerr != nil { return nil, xerr } src.ColumnMapping = *in.ColumnMapping @@ -371,17 +371,6 @@ func validateDedup(d models.ContactImportDedupStrategy) *errx.Error { return errx.New(errx.BadRequest, "unknown dedup strategy: "+string(d)) } -// validateMappingHasEmail enforces that at least one column maps to the email -// target — without it ImportCommit would reject every row as "missing email". -func validateMappingHasEmail(mapping []models.ContactImportColumnMapping) *errx.Error { - for _, m := range mapping { - if m.Target == models.ContactImportTargetEmail { - return nil - } - } - return errx.New(errx.BadRequest, "column_mapping must map one column to 'email'") -} - // normalizeHeaders trims the header row and synthesises a name for any blank // cell so every column is mappable, mirroring the CSV importer's behaviour. func normalizeHeaders(first []string) []string {