feat: rework contactService.ImportCommit end to end so a 1,000-row 13-column upload lands: the column mapping is resolved once before any row is read (a name Warmbly cannot use, an unnamed custom column, an unknown target or a mapping with no email column is one actionable 400 instead of the same message per row, accepting both {target:"custom", custom_key} and the legacy "custom:<key>"), the subscribed column is actually applied instead of parsed and discarded and subscribed_default is confined to new contacts so an update never resubscribes someone who opted out, the categories column resolves titles to ids and creates the missing ones, a file that lists one address twice becomes one contact with the later row merged in, rows the dedup strategy skips still join the campaign and categories the import targets, campaign and category ids are canonicalised so a blank one cannot fail every row on an empty string cast to uuid[], the plan ceiling is checked once for the whole batch, and per-row notes are separated from failures so total always equals imported plus updated plus skipped plus failed; ValidateImportMapping exposes the same verdict so a saved Google Sheets sync source is rejected when it is written rather than on its next sync

This commit is contained in:
Matthew Meszaros
2026-08-27 03:45:32 -07:00
parent 242e721966
commit 1ba07028a4
4 changed files with 494 additions and 109 deletions
+349 -96
View File
@@ -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:<key>" is the legacy spelling of {target:"custom",
// custom_key:"<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
+137
View File
@@ -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:<key> 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")
}
}
+6
View File
@@ -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).
+2 -13
View File
@@ -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 {