diff --git a/docs/content/docs/api/reference/contacts.mdx b/docs/content/docs/api/reference/contacts.mdx index f69031b0..1f006b17 100644 --- a/docs/content/docs/api/reference/contacts.mdx +++ b/docs/content/docs/api/reference/contacts.mdx @@ -367,7 +367,7 @@ Send `multipart/form-data` with a `file` field and an `options` field containing | `has_header` | boolean | Yes | Whether the first row is a header. | | `category_ids` | string[] | No | Categories to assign to imported contacts. | | `campaign_ids` | string[] | No | Campaigns to add imported contacts to. | -| `segment_ids` | string[] | No | Segments to pin imported contacts into, as a manual include override. Applies to imported, updated, and skipped-but-linked contacts alike. | +| `segment_ids` | string[] | No | Segments to pin imported contacts into, as a manual include override. Applies to imported, updated, and skipped-but-linked contacts alike. An id that is not a valid UUID, or that names no segment in the organization, is rejected with `400` before any row is written. | | `subscribed_default` | boolean | No | Subscription state for new contacts when no subscribed column is mapped. Defaults to true. | ```json @@ -398,6 +398,7 @@ Send `multipart/form-data` with a `file` field and an `options` field containing "errors": [ { "line": 57, "email": "not-an-email", "reason": "invalid email" } ], + "segments_pinned": true, "quality": { "malformed": 4, "disposable": 0, @@ -412,6 +413,8 @@ Send `multipart/form-data` with a `file` field and an `options` field containing Imports are capped at 50,000 rows. `errors` carries at most the first 1,000 entries; past that `errors_truncated` is `true` and the counters, not the list, are the real totals. Every row lands in exactly one of `imported`, `updated`, `skipped`, or `failed`, so those four always sum to `total`. +`errors` also carries notes about rows that were not failures, so an entry there does not always mean a lost row. A note about the import as a whole rather than one row carries `line: 0` and is listed first, so a file full of bad addresses cannot push it out of a truncated list. `segments_pinned` is present only when `segment_ids` was set: `true` when every membership write landed, `false` when one did not, with the reason among the notes. + A custom-field name may use letters, numbers, underscores, spaces, and dashes (`Company Mobile`, `first-name`, `plan_tier`). Anything else is a `400` on the whole request, raised before any row is written, along with a mapping that names no `email` column or a `custom` column with no `custom_key`. Per-row `errors` are reserved for problems with the data itself. ## Verification overview diff --git a/docs/content/docs/api/reference/integrations.mdx b/docs/content/docs/api/reference/integrations.mdx index 6bcd70fd..71377744 100644 --- a/docs/content/docs/api/reference/integrations.mdx +++ b/docs/content/docs/api/reference/integrations.mdx @@ -1061,13 +1061,14 @@ Auth: **Scope** `WRITE_CONTACTS` · **Org permission** `manage_contacts`. `GET /lead-sync/sources` -Lists this org's saved sync sources, optionally filtered to a campaign. +Lists this org's saved sync sources, optionally filtered to a campaign or a segment. Auth: **Scope** `WRITE_CONTACTS` · **Org permission** `manage_contacts`. | Parameter | In | Type | Description | |-----------|-----|------|-------------| | `campaign_id` | query | uuid | Optional. Restricts to sources targeting that campaign. Invalid ids return `400`. | +| `segment_id` | query | uuid | Optional. Restricts to sources that pin their rows into that segment. Invalid ids return `400`. Combines with `campaign_id`. | ### Response @@ -1090,6 +1091,7 @@ A `data` array (no pagination envelope on this list). ], "dedup": "update", "category_ids": [], + "segment_ids": [], "subscribed_default": true, "status": "idle", "last_synced_at": "2026-06-10T12:00:00Z", @@ -1121,6 +1123,7 @@ Auth: **Scope** `WRITE_CONTACTS` · **Org permission** `manage_contacts`. | `dedup` | string | No | Collision strategy: `skip`, `update`, or `create_duplicate`. | | `target_campaign_id` | uuid | No | Enrol new/updated leads into this campaign on each sync. | | `category_ids` | string[] | No | Categories to assign to synced leads. | +| `segment_ids` | string[] | No | Segments every synced row is pinned into as a manual include override, on every run. Each id must name a segment in the organization or the save is a `400`; a segment deleted later is dropped from the run instead of failing it. | | `subscribed_default` | boolean | No | Default subscription state for new contacts. | | `label` | string | No | Friendly name. | @@ -1138,6 +1141,7 @@ Auth: **Scope** `WRITE_CONTACTS` · **Org permission** `manage_contacts`. "dedup": "update", "target_campaign_id": null, "category_ids": [], + "segment_ids": [], "subscribed_default": true, "label": "Q2 inbound leads" } @@ -1162,6 +1166,7 @@ Auth: **Scope** `WRITE_CONTACTS` · **Org permission** `manage_contacts`. ], "dedup": "update", "category_ids": [], + "segment_ids": [], "subscribed_default": true, "status": "idle", "created_at": "2026-06-11T15:00:00Z", @@ -1210,6 +1215,7 @@ Auth: **Scope** `WRITE_CONTACTS` · **Org permission** `manage_contacts`. | `target_campaign_id` | uuid | No | New target campaign. | | `clear_campaign` | boolean | No | When `true`, unsets the target campaign. | | `category_ids` | string[] | No | Replacement category set. | +| `segment_ids` | string[] | No | Replacement segment target set. An unknown id is a `400`; an empty array clears the targets. | | `subscribed_default` | boolean | No | Default subscription state. | | `label` | string | No | New label. | diff --git a/docs/content/docs/guides/contacts-crm.mdx b/docs/content/docs/guides/contacts-crm.mdx index 8f1a53c1..3ebb9bdd 100644 --- a/docs/content/docs/guides/contacts-crm.mdx +++ b/docs/content/docs/guides/contacts-crm.mdx @@ -9,7 +9,7 @@ Contacts are the people you reach out to; the CRM tracks what happens after they Two routes share the same column-mapping screen: a file upload and an on-demand Google Sheets sync. -**From a file** (CSV, TSV, or XLSX, up to 50 MB and 50,000 rows), the wizard runs **Upload**, **Map**, **Options**, **Result**. Parsing happens server-side. Start it from Contacts, or from a campaign's Leads tab to attach the imports to that campaign automatically. The result step reports imported, updated, skipped, and failed counts, and failed rows download as an error CSV you can fix and re-import. +**From a file** (CSV, TSV, or XLSX, up to 50 MB and 50,000 rows), the wizard runs **Upload**, **Map**, **Options**, **Result**. Parsing happens server-side. Start it from Contacts, or from a campaign's Leads tab to attach the imports to that campaign automatically, or from a [segment](/guides/segments/)'s member list to pin every row into that segment. The wizard names the target it inherited in its header and on the Options step, and the result step confirms it. The result step reports imported, updated, skipped, and failed counts, and failed rows download as an error CSV you can fix and re-import. You must map at least one column to **Email**. @@ -31,9 +31,11 @@ Rows with a missing or invalid email are reported with a line number and reason Problems with the mapping itself, an unnamed custom field or a name Warmbly cannot use, are reported once before anything is written, so a single typo can never fail every row. -The **Options** step also decides where the contacts land: **Add to campaigns** enrols everyone in the file as leads of the campaigns you pick (hidden when the wizard was started from a campaign's Leads tab, since that campaign is already the target), and **Add to segments** pins them into the [segments](/guides/segments/) you pick as manual includes, so they stay members whatever the segment's conditions say. +The **Options** step also decides where the contacts land: **Add to campaigns** enrols everyone in the file as leads of the campaigns you pick (hidden when the wizard was started from a campaign's Leads tab, since that campaign is already the target), and **Add to segments** pins them into the [segments](/guides/segments/) you pick as manual includes, so they stay members whatever the segment's conditions say. Started from a segment, that segment is always applied and shown as a fixed row; the picker below it adds more. -**From Google Sheets**, a sheet is a reusable **sync source** rather than a one-time upload. Connect it once (Warmbly reads only the tab you choose and never writes back), paste the spreadsheet ID from the URL between `/d/` and `/edit`, pick the tab, map columns, then set duplicate handling, a label, and optionally a campaign to enroll into and categories to apply. **Nothing syncs automatically**: press **Sync now**, or save and sync immediately. +**From Google Sheets**, a sheet is a reusable **sync source** rather than a one-time upload. Connect it once (Warmbly reads only the tab you choose and never writes back), paste the spreadsheet ID from the URL between `/d/` and `/edit`, pick the tab, map columns, then set duplicate handling, a label, and optionally a campaign to enroll into, categories to apply and segments to pin into. **Nothing syncs automatically**: press **Sync now**, or save and sync immediately. + +Segment targets apply on every run, so a sheet you keep adding rows to keeps feeding the same audience. Opening **Sync sources** from a segment's member list lists only the sources feeding it and pre-targets a new one to it. A segment deleted later is dropped from the run rather than stopping it. Saved sources live in your Sync sources list to re-run, edit, or remove. Each sync dedupes on lowercased email using your chosen duplicate handling, so re-syncing a sheet with new rows is safe. diff --git a/docs/content/docs/guides/segments.mdx b/docs/content/docs/guides/segments.mdx index 1fe61192..bc9ee6bd 100644 --- a/docs/content/docs/guides/segments.mdx +++ b/docs/content/docs/guides/segments.mdx @@ -42,9 +42,11 @@ Conditions decide membership, and two overrides sit on top of them: The segment header shows how many contacts are pinned in or out, and a **Pinned contacts** panel below it lists them; **Back to automatic** clears an override so the conditions decide again. A contact's own drawer has a **Segments** section that shows every segment, whether the contact is in it, and the same pin in, pin out and back-to-automatic controls. -**New contact** on a segment page pins as well: a contact created from inside a segment joins that segment, so it appears in the list you created it from even when the conditions do not describe it yet. +**New contact** and **Import** on a segment page pin as well: a contact created or a file uploaded from inside a segment joins that segment, so it appears in the list you started from even when the conditions do not describe it yet. The import wizard names the segment in its header and confirms it on the result step, and rows it skipped as duplicates join too. -Sequences can pin too: the **Add to segment** and **Remove from segment** action steps apply the override to a contact as it moves through a campaign flow, so a positive reply can drop someone into a "warm" segment automatically. So can an import: the file import wizard's **Add to segments** picker on its Options step pins every contact in the file into the segments you choose, so a fresh list can land straight in the audience it was collected for. +Sequences can pin too: the **Add to segment** and **Remove from segment** action steps apply the override to a contact as it moves through a campaign flow, so a positive reply can drop someone into a "warm" segment automatically. So can an import: the file import wizard's **Add to segments** picker on its Options step pins every contact in the file into the segments you choose, so a fresh list can land straight in the audience it was collected for. A [Google Sheets sync source](/guides/contacts-crm/#importing) carries the same targets and re-applies them on every run, which is how a sheet you keep appending to keeps feeding one audience. + +A segment with no conditions at all is a plain list: it holds exactly the contacts you pin in, so creating one and importing into it is the shortest way to turn a file into an audience. ## Using a segment diff --git a/internal/api/handler/lead_sync.go b/internal/api/handler/lead_sync.go index c7a28627..b664e3e5 100644 --- a/internal/api/handler/lead_sync.go +++ b/internal/api/handler/lead_sync.go @@ -133,7 +133,7 @@ func (h *Handler) PreviewLeadSync(c *gin.Context) { } // ListLeadSyncSources lists this org's saved sources, optionally filtered to a -// campaign via ?campaign_id=. +// campaign via ?campaign_id= or to a segment via ?segment_id=. func (h *Handler) ListLeadSyncSources(c *gin.Context) { orgID, ok := requireOrgID(c) if !ok { @@ -148,7 +148,16 @@ func (h *Handler) ListLeadSyncSources(c *gin.Context) { } campaignID = &id } - sources, err := h.LeadSyncService.List(c.Request.Context(), orgID, campaignID) + var segmentID *uuid.UUID + if raw := strings.TrimSpace(c.Query("segment_id")); raw != "" { + id, err := uuid.Parse(raw) + if err != nil { + errx.JSON(c, errx.New(errx.BadRequest, "invalid segment_id")) + return + } + segmentID = &id + } + sources, err := h.LeadSyncService.List(c.Request.Context(), orgID, campaignID, segmentID) if err != nil { errx.JSON(c, errx.New(errx.Internal, "failed to list sync sources")) return diff --git a/internal/app/contact/import.go b/internal/app/contact/import.go index 436ee472..c25255df 100644 --- a/internal/app/contact/import.go +++ b/internal/app/contact/import.go @@ -224,7 +224,7 @@ func (s *contactService) ImportCommit( } // Segment targets are resolved up front: each must exist in the org. // Membership is written after the rows exist, as an include override. - segmentIDs, xerr := s.parseSegmentIDs(ctx, orgID, opts.SegmentIDs) + segmentIDs, xerr := s.resolveSegmentIDs(ctx, orgID, opts.SegmentIDs, opts.SkipMissingSegments) if xerr != nil { return nil, xerr } @@ -379,6 +379,18 @@ func (s *contactService) ImportCommit( res.Failed++ warn(line, addr, values, reason) } + // noteImport records a message about the whole import rather than one row. + // It goes to the front and is never dropped by the per-row cap: a file full + // of bad addresses must not push out the one note explaining why the rows + // that DID import are not in the segment they were imported into, nor bury + // it past the entries the dashboard renders. + noteImport := func(reason string) { + res.Errors = append([]models.ContactImportRowError{{Reason: reason}}, res.Errors...) + if len(res.Errors) > models.MaxContactImportReportedErrors { + res.Errors = res.Errors[:models.MaxContactImportReportedErrors] + res.ErrorsTruncated = true + } + } // Bucket rows by target action. We send fresh inserts through // contactRepository.Add in batches and fall back to per-row @@ -542,13 +554,26 @@ func (s *contactService) ImportCommit( } // Segment membership last, once every touched row exists. A failed write - // is a note, not a failed import: the contacts themselves are in. - if len(touched) > 0 { + // is a note, not a failed import: the contacts themselves are in, and the + // result says the pin did not land so the UI does not claim it did. + if len(segmentIDs) > 0 && len(touched) > 0 { + pinned, failedPins, firstReason := true, 0, "" for _, segID := range segmentIDs { if _, xerr := s.segmentLinker.SetMembers(ctx, orgID, segID, touched, models.SegmentMemberInclude); xerr != nil { - warn(0, "", nil, "imported contacts could not be added to a segment: "+xerr.Message) + pinned, failedPins = false, failedPins+1 + if firstReason == "" { + firstReason = xerr.Message + } } } + // One note for the whole pin, however many segments were targeted: + // the same failure repeated per segment is noise, not information. + if failedPins == 1 { + noteImport("imported contacts could not be added to a segment: " + firstReason) + } else if failedPins > 1 { + noteImport(fmt.Sprintf("imported contacts could not be added to %d segments: %s", failedPins, firstReason)) + } + res.SegmentsPinned = &pinned } res.EndedAt = time.Now().UTC() @@ -564,9 +589,22 @@ func (s *contactService) ImportCommit( return res, nil } +// ValidateSegmentTargets exposes resolveSegmentIDs' verdict without running an +// import, so a saved Google Sheets source is rejected at save time. +func (s *contactService) ValidateSegmentTargets(ctx context.Context, orgID uuid.UUID, ids []string) *errx.Error { + _, xerr := s.resolveSegmentIDs(ctx, orgID, ids, false) + return xerr +} + // parseSegmentIDs validates the import's target segments: well-formed ids // that exist in the org, deduplicated. func (s *contactService) parseSegmentIDs(ctx context.Context, orgID uuid.UUID, raw []string) ([]uuid.UUID, *errx.Error) { + return s.resolveSegmentIDs(ctx, orgID, raw, false) +} + +// resolveSegmentIDs is parseSegmentIDs with the recurring-source relaxation: +// skipMissing drops an id whose segment is gone instead of failing the run. +func (s *contactService) resolveSegmentIDs(ctx context.Context, orgID uuid.UUID, raw []string, skipMissing bool) ([]uuid.UUID, *errx.Error) { if len(raw) == 0 { return nil, nil } @@ -578,6 +616,9 @@ func (s *contactService) parseSegmentIDs(ctx context.Context, orgID uuid.UUID, r for _, r := range raw { id, err := uuid.Parse(strings.TrimSpace(r)) if err != nil { + if skipMissing { + continue + } return nil, errx.New(errx.BadRequest, "invalid segment id") } if seen[id] { @@ -586,6 +627,9 @@ func (s *contactService) parseSegmentIDs(ctx context.Context, orgID uuid.UUID, r seen[id] = true if _, xerr := s.segmentLinker.Get(ctx, orgID, id); xerr != nil { if xerr.Code == errx.NotFound { + if skipMissing { + continue + } return nil, errx.New(errx.BadRequest, "a selected segment does not exist") } return nil, xerr diff --git a/internal/app/contact/import_live_test.go b/internal/app/contact/import_live_test.go index ff6d2572..14e3f079 100644 --- a/internal/app/contact/import_live_test.go +++ b/internal/app/contact/import_live_test.go @@ -11,6 +11,7 @@ import ( "github.com/jackc/pgx/v5/pgxpool" "github.com/warmbly/warmbly/internal/app/orgrisk" + "github.com/warmbly/warmbly/internal/errx" "github.com/warmbly/warmbly/internal/infrastructure/db" "github.com/warmbly/warmbly/internal/models" "github.com/warmbly/warmbly/internal/repository" @@ -33,6 +34,7 @@ type importFixture struct { campaign uuid.UUID svc ContactService repo repository.ContactRepository + segments repository.SegmentRepository pool *pgxpool.Pool } @@ -79,6 +81,8 @@ func newImportFixture(t *testing.T) *importFixture { {`DELETE FROM campaign_leads WHERE campaign_id IN (SELECT id FROM campaigns WHERE organization_id = $1)`, f.org}, {`DELETE FROM campaigns WHERE organization_id = $1`, f.org}, {`DELETE FROM contact_categories WHERE contact_id IN (SELECT id FROM contacts WHERE organization_id = $1)`, f.org}, + {`DELETE FROM segment_members WHERE segment_id IN (SELECT id FROM segments WHERE organization_id = $1)`, f.org}, + {`DELETE FROM segments WHERE organization_id = $1`, f.org}, {`DELETE FROM contacts WHERE organization_id = $1`, f.org}, {`DELETE FROM categories WHERE user_id = $1`, f.user}, {`DELETE FROM organization_members WHERE organization_id = $1`, f.org}, @@ -92,8 +96,12 @@ func newImportFixture(t *testing.T) *importFixture { }) f.repo = repository.NewContactRepostory(handle) + f.segments = repository.NewSegmentRepository(handle) // nil sub/plan repos: the plan cap is skipped, which is what we want here. f.svc = NewService(f.repo, nil, nil) + // Segment targets on an import are written through the same repository the + // backend wires in; no syncer, so nothing enrols campaigns here. + f.svc.(SegmentAware).WireSegments(f.segments, nil) return f } @@ -721,3 +729,159 @@ func TestLiveImportQualityFoldsInALegacyFinding(t *testing.T) { t.Errorf("the finding survived enough good data to outweigh it: %v", risk.Signals) } } + +// Issue #381: a CSV imported with segment targets must land in those segments, +// including rows the dedup strategy skipped, and the segment's own contact +// list (the query the dashboard runs) must return them. +func TestLiveImportPinsRowsIntoSegments(t *testing.T) { + f := newImportFixture(t) + ctx := context.Background() + + // Conditions nothing in the file matches, so membership can only come + // from the import's pin. + seg, xerr := f.segments.Create(ctx, f.org, &f.user, &models.Segment{ + Name: "Import target", Color: "#0284c7", Match: models.SegmentMatchAll, + Conditions: []models.SegmentCondition{{Field: "company", Operator: "equals", Value: "nobody-here"}}, + }) + if xerr != nil { + t.Fatalf("create segment: %v", xerr) + } + + tag := uuid.New().String()[:6] + emails := []string{"seg1-" + tag + "@i381.test", "seg2-" + tag + "@i381.test"} + opts := &models.ContactImportCommit{ + Mapping: emailOnlyMapping(), + Dedup: models.ContactImportDedupSkip, + HasHeader: true, + SegmentIDs: []string{seg.ID.String()}, + } + res, msg := f.commit(t, simpleCSV(emails), opts) + if msg != "" { + t.Fatalf("commit: %s", msg) + } + if res.Imported != 2 { + t.Fatalf("imported = %d, want 2", res.Imported) + } + if res.SegmentsPinned == nil || !*res.SegmentsPinned { + t.Fatalf("result does not report the pin: %+v", res.Errors) + } + + members := func() []string { + t.Helper() + page, xerr := f.repo.Search(ctx, f.org.String(), nil, nil, + models.SearchContacts{SegmentIDs: []string{seg.ID.String()}}, 50) + if xerr != nil { + t.Fatalf("search: %v", xerr) + } + out := make([]string, 0, len(page.Data)) + for _, c := range page.Data { + out = append(out, c.Email) + } + return out + } + if got := members(); len(got) != 2 { + t.Fatalf("segment holds %v, want both imported rows", got) + } + + // The same file again: every row is skipped as a duplicate, and a third + // row is new. All three have to be in the segment afterwards, because + // "skip" means "leave their fields alone", not "leave them out". + emails = append(emails, "seg3-"+tag+"@i381.test") + res, msg = f.commit(t, simpleCSV(emails), opts) + if msg != "" { + t.Fatalf("second commit: %s", msg) + } + if res.Skipped != 2 || res.Imported != 1 { + t.Fatalf("second run: imported=%d skipped=%d, want 1/2", res.Imported, res.Skipped) + } + if got := members(); len(got) != 3 { + t.Fatalf("segment holds %v, want all three rows", got) + } + + // No targets at all leaves the flag absent, so a caller can tell "nothing + // to pin" apart from "the pin failed". + res, msg = f.commit(t, simpleCSV([]string{"untargeted-" + tag + "@i381.test"}), + &models.ContactImportCommit{Mapping: emailOnlyMapping(), Dedup: models.ContactImportDedupSkip, HasHeader: true}) + if msg != "" { + t.Fatalf("untargeted commit: %s", msg) + } + if res.SegmentsPinned != nil { + t.Fatalf("import with no segment targets reported segments_pinned=%v", *res.SegmentsPinned) + } + + // A segment id from another organization is refused up front rather than + // dropping the pin silently. + gone := uuid.NewString() + opts.SegmentIDs = []string{gone} + if _, msg := f.commit(t, simpleCSV([]string{"ghost-" + tag + "@i381.test"}), opts); msg == "" { + t.Fatalf("import with an unknown segment succeeded") + } + + // A recurring source (the Google Sheets sync) keeps importing when one of + // its saved targets is gone, and says the pin did not fully land. + opts.SegmentIDs = []string{seg.ID.String(), gone} + opts.SkipMissingSegments = true + res, msg = f.commit(t, simpleCSV([]string{"lenient-" + tag + "@i381.test"}), opts) + if msg != "" { + t.Fatalf("lenient commit: %s", msg) + } + if res.Imported != 1 || res.SegmentsPinned == nil || !*res.SegmentsPinned { + t.Fatalf("lenient run: imported=%d pinned=%v", res.Imported, res.SegmentsPinned) + } + if got := members(); len(got) != 4 { + t.Fatalf("segment holds %v after the lenient run, want four rows", got) + } +} + +// failingLinker validates segment targets through the real repository but +// refuses every membership write, which is the only way to reach the pin +// failure path from a test. +type failingLinker struct{ repository.SegmentRepository } + +func (failingLinker) SetMembers(context.Context, uuid.UUID, uuid.UUID, []uuid.UUID, models.SegmentMemberMode) (int, *errx.Error) { + return 0, errx.New(errx.Internal, "segment store unavailable") +} + +// A pin that does not land is reported as segments_pinned=false with one note +// saying why, and that note survives a file that produced more row errors than +// the payload cap: it explains the rows that DID import, so it must not be the +// entry that gets dropped. +func TestLiveImportReportsAPinThatDidNotLand(t *testing.T) { + f := newImportFixture(t) + ctx := context.Background() + + seg, xerr := f.segments.Create(ctx, f.org, &f.user, &models.Segment{ + Name: "Unreachable " + uuid.New().String()[:6], Color: "#0284c7", Match: models.SegmentMatchAll, + }) + if xerr != nil { + t.Fatalf("create segment: %v", xerr) + } + f.svc.(SegmentAware).WireSegments(failingLinker{f.segments}, nil) + + tag := uuid.New().String()[:6] + rows := []string{"good-" + tag + "@i381.test"} + for i := 0; i < models.MaxContactImportReportedErrors+5; i++ { + rows = append(rows, fmt.Sprintf("not-an-email-%d", i)) + } + res, msg := f.commit(t, simpleCSV(rows), &models.ContactImportCommit{ + Mapping: emailOnlyMapping(), Dedup: models.ContactImportDedupSkip, HasHeader: true, + SegmentIDs: []string{seg.ID.String()}, + }) + if msg != "" { + t.Fatalf("commit: %s", msg) + } + if res.Imported != 1 { + t.Fatalf("imported = %d, want the one valid row", res.Imported) + } + if res.SegmentsPinned == nil || *res.SegmentsPinned { + t.Fatalf("a refused membership write reported segments_pinned=%v", res.SegmentsPinned) + } + if !res.ErrorsTruncated || len(res.Errors) != models.MaxContactImportReportedErrors { + t.Fatalf("errors = %d truncated=%v, want the cap", len(res.Errors), res.ErrorsTruncated) + } + // First, so the dashboard renders it however many row errors came with it. + first := res.Errors[0] + if first.Line != 0 || !strings.Contains(first.Reason, "could not be added to a segment") { + t.Fatalf("first note = %+v, want the segment-pin reason", first) + } +} diff --git a/internal/app/contact/service.go b/internal/app/contact/service.go index e634db57..7bf62c4e 100644 --- a/internal/app/contact/service.go +++ b/internal/app/contact/service.go @@ -40,6 +40,10 @@ type ContactService interface { // 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 + // ValidateSegmentTargets reports whether every id names a segment in the + // organization, so a saved source is refused when it is written rather + // than on its next run. + ValidateSegmentTargets(ctx context.Context, orgID uuid.UUID, ids []string) *errx.Error // ImportCommit re-parses the uploaded file with the chosen mapping // and performs the upsert / skip / dedup work. Returns per-row diff --git a/internal/app/leadsync/service.go b/internal/app/leadsync/service.go index 410f846a..23303128 100644 --- a/internal/app/leadsync/service.go +++ b/internal/app/leadsync/service.go @@ -47,7 +47,7 @@ type Service interface { Preview(ctx context.Context, orgID, connID uuid.UUID, sheetID, tabTitle string) (*models.ContactImportPreview, *errx.Error) // Source CRUD. - List(ctx context.Context, orgID uuid.UUID, campaignID *uuid.UUID) ([]models.LeadSyncSource, error) + List(ctx context.Context, orgID uuid.UUID, campaignID, segmentID *uuid.UUID) ([]models.LeadSyncSource, error) Get(ctx context.Context, orgID, id uuid.UUID) (*models.LeadSyncSource, *errx.Error) Create(ctx context.Context, orgID, userID uuid.UUID, in *models.CreateLeadSyncSource) (*models.LeadSyncSource, *errx.Error) Update(ctx context.Context, orgID, id uuid.UUID, in *models.UpdateLeadSyncSource) (*models.LeadSyncSource, *errx.Error) @@ -114,8 +114,8 @@ func (s *service) Preview(ctx context.Context, orgID, connID uuid.UUID, sheetID, }, nil } -func (s *service) List(ctx context.Context, orgID uuid.UUID, campaignID *uuid.UUID) ([]models.LeadSyncSource, error) { - return s.repo.List(ctx, orgID, campaignID) +func (s *service) List(ctx context.Context, orgID uuid.UUID, campaignID, segmentID *uuid.UUID) ([]models.LeadSyncSource, error) { + return s.repo.List(ctx, orgID, campaignID, segmentID) } func (s *service) Get(ctx context.Context, orgID, id uuid.UUID) (*models.LeadSyncSource, *errx.Error) { @@ -166,6 +166,13 @@ func (s *service) Create(ctx context.Context, orgID, userID uuid.UUID, in *model if cats == nil { cats = []string{} } + segs := in.SegmentIDs + if segs == nil { + segs = []string{} + } + if xerr := s.contacts.ValidateSegmentTargets(ctx, orgID, segs); xerr != nil { + return nil, xerr + } src := &models.LeadSyncSource{ OrganizationID: orgID, @@ -181,6 +188,7 @@ func (s *service) Create(ctx context.Context, orgID, userID uuid.UUID, in *model Dedup: in.Dedup, TargetCampaignID: in.TargetCampaignID, CategoryIDs: cats, + SegmentIDs: segs, SubscribedDefault: subscribed, Label: strings.TrimSpace(in.Label), Status: models.LeadSyncStatusIdle, @@ -240,6 +248,12 @@ func (s *service) Update(ctx context.Context, orgID, id uuid.UUID, in *models.Up if in.CategoryIDs != nil { src.CategoryIDs = *in.CategoryIDs } + if in.SegmentIDs != nil { + if xerr := s.contacts.ValidateSegmentTargets(ctx, orgID, *in.SegmentIDs); xerr != nil { + return nil, xerr + } + src.SegmentIDs = *in.SegmentIDs + } if in.SubscribedDefault != nil { src.SubscribedDefault = *in.SubscribedDefault } @@ -297,14 +311,18 @@ func (s *service) SyncNow(ctx context.Context, triggeringUserID, orgID, sourceID } subscribed := src.SubscribedDefault opts := &models.ContactImportCommit{ - Mapping: src.ColumnMapping, - Dedup: src.Dedup, - HasHeader: src.HasHeader, - CategoryIDs: src.CategoryIDs, - CampaignIDs: campaignIDs, - SubscribedDefault: &subscribed, - Source: models.ContactSourceSheetSync, - SourceDetail: src.SheetTitle, + Mapping: src.ColumnMapping, + Dedup: src.Dedup, + HasHeader: src.HasHeader, + CategoryIDs: src.CategoryIDs, + CampaignIDs: campaignIDs, + SegmentIDs: src.SegmentIDs, + // A segment deleted after the source was saved is dropped from the run; + // a recurring sync must not stop importing because a target is gone. + SkipMissingSegments: true, + SubscribedDefault: &subscribed, + Source: models.ContactSourceSheetSync, + SourceDetail: src.SheetTitle, } result, ierr := s.contacts.ImportCommit(ctx, triggeringUserID.String(), orgID, bytes.NewReader(csvBytes), "google-sheets-sync.csv", opts) diff --git a/internal/infrastructure/db/migrations/000136_lead_sync_segments.down.sql b/internal/infrastructure/db/migrations/000136_lead_sync_segments.down.sql new file mode 100644 index 00000000..a8248921 --- /dev/null +++ b/internal/infrastructure/db/migrations/000136_lead_sync_segments.down.sql @@ -0,0 +1,2 @@ +ALTER TABLE lead_sync_sources + DROP COLUMN IF EXISTS segment_ids; diff --git a/internal/infrastructure/db/migrations/000136_lead_sync_segments.up.sql b/internal/infrastructure/db/migrations/000136_lead_sync_segments.up.sql new file mode 100644 index 00000000..f6bd30f0 --- /dev/null +++ b/internal/infrastructure/db/migrations/000136_lead_sync_segments.up.sql @@ -0,0 +1,10 @@ +-- Segment targets for a saved Google Sheets sync source. +-- +-- A file import can pin every row into a segment (ContactImportCommit.segment_ids); +-- a sheet sync committed through the same importer could not, so a sync created +-- from a segment's member list produced contacts that were nowhere in it. +-- jsonb for the same reason category_ids is: it is a list handed straight to +-- the importer, never filtered in SQL beyond a containment lookup. + +ALTER TABLE lead_sync_sources + ADD COLUMN segment_ids jsonb NOT NULL DEFAULT '[]'; diff --git a/internal/models/contact_import.go b/internal/models/contact_import.go index 10103aad..87f98871 100644 --- a/internal/models/contact_import.go +++ b/internal/models/contact_import.go @@ -109,6 +109,11 @@ type ContactImportCommit struct { // SegmentIDs pins every imported row into these segments as a manual // include override, the same write the "Add to segment" bulk action does. SegmentIDs []string `json:"segment_ids,omitempty"` + // SkipMissingSegments drops a target segment that no longer exists instead + // of refusing the import. Set by saved recurring sources (the Google Sheets + // sync), where a segment deleted months later must not stop every run; an + // interactive import still gets a 400 on a segment the user just picked. + SkipMissingSegments bool `json:"-"` // SubscribedDefault is what new contacts inherit when no // subscribed column was mapped. Defaults to true server-side. @@ -151,6 +156,13 @@ type ContactImportResult struct { // "showing the first N of M" instead of implying it listed everything. ErrorsTruncated bool `json:"errors_truncated,omitempty"` + // SegmentsPinned is nil when the import had no segment targets to write. + // With targets it is true when every membership write landed and false + // when one did not, with the reason among the notes. A plain bool could + // not carry that third state: omitempty drops false, so a failed pin + // looked exactly like an import that never asked for one. + SegmentsPinned *bool `json:"segments_pinned,omitempty"` + // Quality is what the uploaded addresses look like, measured at import. // Advisory: a bad list is reported here and stopped at launch, never // refused here, because these are the customer's own records. diff --git a/internal/models/lead_sync.go b/internal/models/lead_sync.go index ce92c7d2..49376ee0 100644 --- a/internal/models/lead_sync.go +++ b/internal/models/lead_sync.go @@ -48,9 +48,13 @@ type LeadSyncSource struct { // TargetCampaignID, when set, enrols every new/updated lead into that // campaign on each sync. - TargetCampaignID *uuid.UUID `json:"target_campaign_id,omitempty"` - CategoryIDs []string `json:"category_ids"` - SubscribedDefault bool `json:"subscribed_default"` + TargetCampaignID *uuid.UUID `json:"target_campaign_id,omitempty"` + CategoryIDs []string `json:"category_ids"` + // SegmentIDs pins every synced row into these segments as an include + // override, the same write the file import's segment targets do. A segment + // deleted later is dropped from the run rather than failing it. + SegmentIDs []string `json:"segment_ids"` + SubscribedDefault bool `json:"subscribed_default"` Label string `json:"label,omitempty"` Status LeadSyncStatus `json:"status"` @@ -75,6 +79,7 @@ type CreateLeadSyncSource struct { Dedup ContactImportDedupStrategy `json:"dedup"` TargetCampaignID *uuid.UUID `json:"target_campaign_id"` CategoryIDs []string `json:"category_ids"` + SegmentIDs []string `json:"segment_ids"` SubscribedDefault *bool `json:"subscribed_default"` Label string `json:"label"` } @@ -92,6 +97,7 @@ type UpdateLeadSyncSource struct { TargetCampaignID *uuid.UUID `json:"target_campaign_id"` ClearCampaign bool `json:"clear_campaign"` CategoryIDs *[]string `json:"category_ids"` + SegmentIDs *[]string `json:"segment_ids"` SubscribedDefault *bool `json:"subscribed_default"` Label *string `json:"label"` } diff --git a/internal/repository/lead_sync_segments_live_test.go b/internal/repository/lead_sync_segments_live_test.go new file mode 100644 index 00000000..9c641994 --- /dev/null +++ b/internal/repository/lead_sync_segments_live_test.go @@ -0,0 +1,110 @@ +package repository + +import ( + "context" + "testing" + + "github.com/google/uuid" + "github.com/warmbly/warmbly/internal/models" +) + +// Issue #381: a saved Google Sheets source carries segment targets so a sync +// started from a segment's member list pins its rows into that segment, the +// same way a file import does. These prove the column round-trips and that the +// segment filter finds the sources feeding one segment. +// +// WARMBLY_TEST_DB=postgres://warmbly:warmbly@localhost:15432/warmbly_dev?sslmode=disable \ +// go test ./internal/repository/ -run LiveLeadSyncSegment -v + +func TestLiveLeadSyncSegmentTargets(t *testing.T) { + handle, pool := liveContactDB(t) + f := newSharedOrgFixture(t, pool) + ctx := context.Background() + repo := NewLeadSyncRepository(pool) + segments := NewSegmentRepository(handle) + + t.Cleanup(func() { + if _, err := pool.Exec(context.Background(), `DELETE FROM lead_sync_sources WHERE organization_id = $1`, f.org); err != nil { + t.Errorf("cleanup sources: %v", err) + } + if _, err := pool.Exec(context.Background(), `DELETE FROM segments WHERE organization_id = $1`, f.org); err != nil { + t.Errorf("cleanup segments: %v", err) + } + }) + + newSeg := func(name string) uuid.UUID { + t.Helper() + seg, xerr := segments.Create(ctx, f.org, &f.owner, &models.Segment{ + Name: name, Color: "#0284c7", Match: models.SegmentMatchAll, + }) + if xerr != nil { + t.Fatalf("create segment: %v", xerr) + } + return seg.ID + } + target := newSeg("Sheet target " + uuid.New().String()[:6]) + other := newSeg("Elsewhere " + uuid.New().String()[:6]) + + pinned := &models.LeadSyncSource{ + OrganizationID: f.org, CreatedByUserID: f.owner, ConnectionID: uuid.New(), + SheetID: "sheet-pinned", HasHeader: true, + ColumnMapping: []models.ContactImportColumnMapping{{Index: 0, Target: models.ContactImportTargetEmail}}, + Dedup: models.ContactImportDedupUpdate, + SegmentIDs: []string{target.String()}, + } + loose := &models.LeadSyncSource{ + OrganizationID: f.org, CreatedByUserID: f.owner, ConnectionID: uuid.New(), + SheetID: "sheet-loose", HasHeader: true, + ColumnMapping: []models.ContactImportColumnMapping{{Index: 0, Target: models.ContactImportTargetEmail}}, + Dedup: models.ContactImportDedupUpdate, + } + for _, src := range []*models.LeadSyncSource{pinned, loose} { + if err := repo.Create(ctx, src); err != nil { + t.Fatalf("create source: %v", err) + } + } + + got, err := repo.Get(ctx, f.org, pinned.ID) + if err != nil { + t.Fatalf("get: %v", err) + } + if len(got.SegmentIDs) != 1 || got.SegmentIDs[0] != target.String() { + t.Fatalf("segment targets = %v, want [%s]", got.SegmentIDs, target) + } + // A source with no targets reads back as an empty list, never nil, so the + // importer never sees a null segment list. + back, err := repo.Get(ctx, f.org, loose.ID) + if err != nil { + t.Fatalf("get loose: %v", err) + } + if back.SegmentIDs == nil || len(back.SegmentIDs) != 0 { + t.Fatalf("untargeted source segment ids = %v, want []", back.SegmentIDs) + } + + list, err := repo.List(ctx, f.org, nil, &target) + if err != nil { + t.Fatalf("list by segment: %v", err) + } + if len(list) != 1 || list[0].ID != pinned.ID { + t.Fatalf("segment filter returned %d sources, want only the pinned one", len(list)) + } + if list, err = repo.List(ctx, f.org, nil, &other); err != nil || len(list) != 0 { + t.Fatalf("filter on an unrelated segment returned %d sources (err %v)", len(list), err) + } + if list, err = repo.List(ctx, f.org, nil, nil); err != nil || len(list) != 2 { + t.Fatalf("unfiltered list returned %d sources (err %v), want 2", len(list), err) + } + + // The targets survive an edit that does not mention them. + got.Label = "renamed" + if err := repo.Update(ctx, got); err != nil { + t.Fatalf("update: %v", err) + } + again, err := repo.Get(ctx, f.org, pinned.ID) + if err != nil { + t.Fatalf("get after update: %v", err) + } + if len(again.SegmentIDs) != 1 || again.SegmentIDs[0] != target.String() { + t.Fatalf("segment targets after update = %v", again.SegmentIDs) + } +} diff --git a/internal/repository/pg_lead_sync.go b/internal/repository/pg_lead_sync.go index 69db1343..459e68ce 100644 --- a/internal/repository/pg_lead_sync.go +++ b/internal/repository/pg_lead_sync.go @@ -16,7 +16,9 @@ import ( // reachable by the org that created it. type LeadSyncRepository interface { Create(ctx context.Context, src *models.LeadSyncSource) error - List(ctx context.Context, orgID uuid.UUID, campaignID *uuid.UUID) ([]models.LeadSyncSource, error) + // List filters to the sources feeding one campaign and/or one segment; + // both nil lists every source in the organization. + List(ctx context.Context, orgID uuid.UUID, campaignID, segmentID *uuid.UUID) ([]models.LeadSyncSource, error) Get(ctx context.Context, orgID, id uuid.UUID) (*models.LeadSyncSource, error) Update(ctx context.Context, src *models.LeadSyncSource) error Delete(ctx context.Context, orgID, id uuid.UUID) error @@ -36,7 +38,7 @@ func NewLeadSyncRepository(db *pgxpool.Pool) LeadSyncRepository { const leadSyncCols = ` id, organization_id, created_by_user_id, provider, connection_id, sheet_id, COALESCE(sheet_title, ''), COALESCE(tab_title, ''), COALESCE(a1_range, ''), - has_header, column_mapping, dedup, target_campaign_id, category_ids, + has_header, column_mapping, dedup, target_campaign_id, category_ids, segment_ids, subscribed_default, COALESCE(label, ''), status, last_synced_at, last_result, COALESCE(last_error, ''), created_at, updated_at` @@ -56,32 +58,41 @@ func (r *leadSyncRepository) Create(ctx context.Context, src *models.LeadSyncSou mapping := marshalJSONDefault(src.ColumnMapping, "[]") cats := marshalJSONDefault(src.CategoryIDs, "[]") + segs := marshalJSONDefault(src.SegmentIDs, "[]") _, err := r.db.Exec(ctx, ` INSERT INTO lead_sync_sources ( id, organization_id, created_by_user_id, provider, connection_id, sheet_id, sheet_title, tab_title, a1_range, has_header, - column_mapping, dedup, target_campaign_id, category_ids, + column_mapping, dedup, target_campaign_id, category_ids, segment_ids, subscribed_default, label, status, created_at, updated_at ) VALUES ( $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, - $11, $12, $13, $14, - $15, $16, $17, $18, $18 + $11, $12, $13, $14, $15, + $16, $17, $18, $19, $19 )`, src.ID, src.OrganizationID, src.CreatedByUserID, src.Provider, src.ConnectionID, src.SheetID, nullIfEmptyStr(src.SheetTitle), nullIfEmptyStr(src.TabTitle), nullIfEmptyStr(src.A1Range), src.HasHeader, - mapping, string(src.Dedup), src.TargetCampaignID, cats, + mapping, string(src.Dedup), src.TargetCampaignID, cats, segs, src.SubscribedDefault, nullIfEmptyStr(src.Label), string(src.Status), now, ) return err } -func (r *leadSyncRepository) List(ctx context.Context, orgID uuid.UUID, campaignID *uuid.UUID) ([]models.LeadSyncSource, error) { +func (r *leadSyncRepository) List(ctx context.Context, orgID uuid.UUID, campaignID, segmentID *uuid.UUID) ([]models.LeadSyncSource, error) { + // The segment filter is a jsonb containment test against the id list the + // importer reads, so it needs no extra column. + var wantSegment []byte + if segmentID != nil { + wantSegment = []byte(`["` + segmentID.String() + `"]`) + } rows, err := r.db.Query(ctx, `SELECT `+leadSyncCols+` FROM lead_sync_sources - WHERE organization_id = $1 AND ($2::uuid IS NULL OR target_campaign_id = $2) - ORDER BY created_at DESC`, orgID, campaignID) + WHERE organization_id = $1 + AND ($2::uuid IS NULL OR target_campaign_id = $2) + AND ($3::jsonb IS NULL OR segment_ids @> $3::jsonb) + ORDER BY created_at DESC`, orgID, campaignID, wantSegment) if err != nil { return nil, err } @@ -116,16 +127,17 @@ func (r *leadSyncRepository) Update(ctx context.Context, src *models.LeadSyncSou src.UpdatedAt = now mapping := marshalJSONDefault(src.ColumnMapping, "[]") cats := marshalJSONDefault(src.CategoryIDs, "[]") + segs := marshalJSONDefault(src.SegmentIDs, "[]") _, err := r.db.Exec(ctx, ` UPDATE lead_sync_sources SET sheet_id = $1, sheet_title = $2, tab_title = $3, a1_range = $4, has_header = $5, column_mapping = $6, dedup = $7, target_campaign_id = $8, - category_ids = $9, subscribed_default = $10, label = $11, updated_at = $12 - WHERE organization_id = $13 AND id = $14`, + category_ids = $9, segment_ids = $10, subscribed_default = $11, label = $12, updated_at = $13 + WHERE organization_id = $14 AND id = $15`, src.SheetID, nullIfEmptyStr(src.SheetTitle), nullIfEmptyStr(src.TabTitle), nullIfEmptyStr(src.A1Range), src.HasHeader, mapping, string(src.Dedup), src.TargetCampaignID, - cats, src.SubscribedDefault, nullIfEmptyStr(src.Label), now, + cats, segs, src.SubscribedDefault, nullIfEmptyStr(src.Label), now, src.OrganizationID, src.ID, ) return err @@ -165,6 +177,7 @@ func scanLeadSyncInto(row scanner, s *models.LeadSyncSource) error { var ( mapping []byte cats []byte + segs []byte lastResult []byte status string dedup string @@ -172,7 +185,7 @@ func scanLeadSyncInto(row scanner, s *models.LeadSyncSource) error { if err := row.Scan( &s.ID, &s.OrganizationID, &s.CreatedByUserID, &s.Provider, &s.ConnectionID, &s.SheetID, &s.SheetTitle, &s.TabTitle, &s.A1Range, - &s.HasHeader, &mapping, &dedup, &s.TargetCampaignID, &cats, + &s.HasHeader, &mapping, &dedup, &s.TargetCampaignID, &cats, &segs, &s.SubscribedDefault, &s.Label, &status, &s.LastSyncedAt, &lastResult, &s.LastError, &s.CreatedAt, &s.UpdatedAt, ); err != nil { @@ -189,6 +202,10 @@ func scanLeadSyncInto(row scanner, s *models.LeadSyncSource) error { if len(cats) > 0 { _ = json.Unmarshal(cats, &s.CategoryIDs) } + s.SegmentIDs = []string{} + if len(segs) > 0 { + _ = json.Unmarshal(segs, &s.SegmentIDs) + } if len(lastResult) > 0 { var res models.ContactImportResult if err := json.Unmarshal(lastResult, &res); err == nil { diff --git a/internal/repository/pg_segment.go b/internal/repository/pg_segment.go index 214e819e..cdf1cf9e 100644 --- a/internal/repository/pg_segment.go +++ b/internal/repository/pg_segment.go @@ -127,6 +127,19 @@ func (r *segmentRepository) Get(ctx context.Context, orgID, id uuid.UUID) (*mode return s, nil } +// marshalConditions encodes a condition list for the jsonb column. A nil slice +// encodes as `null`, which the array CHECK rejects, so it becomes an empty list. +func marshalConditions(conds []models.SegmentCondition) []byte { + if len(conds) == 0 { + return []byte("[]") + } + b, err := json.Marshal(conds) + if err != nil { + return []byte("[]") + } + return b +} + func (r *segmentRepository) Create(ctx context.Context, orgID uuid.UUID, createdBy *uuid.UUID, seg *models.Segment) (*models.Segment, *errx.Error) { var total int if err := r.DB.QueryRow(ctx, `SELECT COUNT(*) FROM segments WHERE organization_id = $1`, orgID).Scan(&total); err != nil { @@ -136,7 +149,7 @@ func (r *segmentRepository) Create(ctx context.Context, orgID uuid.UUID, created if total >= models.SegmentsPerOrgMax { return nil, errx.New(errx.BadRequest, fmt.Sprintf("a workspace can have at most %d segments", models.SegmentsPerOrgMax)) } - conds, _ := json.Marshal(seg.Conditions) + conds := marshalConditions(seg.Conditions) var id uuid.UUID err := r.DB.QueryRow(ctx, ` INSERT INTO segments (organization_id, created_by, name, description, color, match, conditions) @@ -153,7 +166,7 @@ func (r *segmentRepository) Create(ctx context.Context, orgID uuid.UUID, created } func (r *segmentRepository) Update(ctx context.Context, orgID uuid.UUID, seg *models.Segment) (*models.Segment, *errx.Error) { - conds, _ := json.Marshal(seg.Conditions) + conds := marshalConditions(seg.Conditions) tag, err := r.DB.Exec(ctx, ` UPDATE segments SET name = $3, description = $4, color = $5, match = $6, conditions = $7, updated_at = now() WHERE organization_id = $1 AND id = $2`, orgID, seg.ID, seg.Name, seg.Description, seg.Color, seg.Match, conds) diff --git a/web/src/app/app/contacts/segments/[id]/page.tsx b/web/src/app/app/contacts/segments/[id]/page.tsx index 2e41856a..38073e89 100644 --- a/web/src/app/app/contacts/segments/[id]/page.tsx +++ b/web/src/app/app/contacts/segments/[id]/page.tsx @@ -127,7 +127,7 @@ function SegmentDetail() { {(s.included_count > 0 || s.excluded_count > 0) && } - + setEditorOpen(false)} segment={s} /> setCampaignOpen(false)} segment={s} /> @@ -190,6 +190,12 @@ function OverridesPanel({ segment }: { segment: Segment }) { } const list = overrides.data ?? []; + // The API caps one listing, so a segment a big import pinned into shows a + // slice of its overrides. Say so rather than implying this is all of them, + // but only once the listing actually arrived: while it is pending `list` + // is empty, and the notice would read "the newest 0 of 5,000". + const pinned = segment.included_count + segment.excluded_count; + const truncated = overrides.isSuccess && pinned > list.length; return (
diff --git a/web/src/components/app/contacts/ContactsTable.tsx b/web/src/components/app/contacts/ContactsTable.tsx index bb35be32..0f85edc2 100644 --- a/web/src/components/app/contacts/ContactsTable.tsx +++ b/web/src/components/app/contacts/ContactsTable.tsx @@ -117,7 +117,7 @@ export default function ContactsTable({ }: { current_campaign?: MiniCampaign; // Scope the list to one segment's members (the segment detail page). - segment?: { id: string; name: string }; + segment?: { id: string; name: string; color?: string }; }) { const confirm = useConfirm(); const segmentMembers = useSetSegmentMembers(); @@ -487,7 +487,7 @@ export default function ContactsTable({ ? "Pick people from your contacts, import a file, or add one by hand. The linked segments could not be loaded." : "Pick people from your contacts, link a segment, import a file, or add one by hand." : segment - ? "Nobody matches its conditions yet. Adjust them or pin contacts in." + ? "Nothing matches it yet. Pin people in from your contacts, import a file, or add one by hand." : "Add or upload contacts to get started." } emptyCta={ @@ -532,6 +532,29 @@ export default function ContactsTable({ Import file + ) : segment ? ( +
+ } + onClick={() => setFromContactsOpen(true)} + > + Add contacts + + } + onClick={() => setImportOpen(true)} + > + Import file + + } + onClick={() => setNewOpen(true)} + > + New contact + +
) : ( } @@ -931,8 +954,9 @@ export default function ContactsTable({ setImportOpen(false)} + lockedSegment={segment} /> - setSyncOpen(false)} /> + setSyncOpen(false)} segment={segment} /> ); } diff --git a/web/src/components/app/contacts/ImportWizard.tsx b/web/src/components/app/contacts/ImportWizard.tsx index 8f30670e..75e53aed 100644 --- a/web/src/components/app/contacts/ImportWizard.tsx +++ b/web/src/components/app/contacts/ImportWizard.tsx @@ -54,6 +54,7 @@ import { import { Label, TextInput } from "@/components/ui/field"; import CategoryPicker from "./CategoryPicker"; import { CampaignMultiPicker, SegmentMultiPicker } from "@/components/app/segments/SegmentPickers"; +import { useSegments } from "@/lib/api/hooks/app/segments"; import { downloadBlob } from "@/lib/api/client/app/contacts/exportContacts"; import { CUSTOM_KEY_RULES, @@ -73,11 +74,16 @@ interface Props { // When set (the campaign Leads tab), imported contacts are attached to this // campaign and the wizard shows a read-only "Adding to …" indicator. lockedCampaign?: { id: string; name: string }; + // When set (a segment's member list), every imported row is pinned into + // this segment, the same way the campaign target works. Without it an + // import started from inside a segment created contacts that were nowhere + // in it (issue #381). + lockedSegment?: { id: string; name: string; color?: string }; } type Step = "upload" | "map" | "options" | "result"; -export default function ImportWizard({ open, onClose, lockedCampaign }: Props) { +export default function ImportWizard({ open, onClose, lockedCampaign, lockedSegment }: Props) { const [step, setStep] = React.useState("upload"); const [file, setFile] = React.useState(null); const [preview, setPreview] = React.useState(null); @@ -91,6 +97,7 @@ export default function ImportWizard({ open, onClose, lockedCampaign }: Props) { const [commitBusy, setCommitBusy] = React.useState(false); const [result, setResult] = React.useState(null); const queryClient = useQueryClient(); + const segments = useSegments(open); function reset() { setStep("upload"); @@ -127,6 +134,20 @@ export default function ImportWizard({ open, onClose, lockedCampaign }: Props) { } } + // The segment the wizard was opened inside always travels with the + // import, whether or not the user opened the options step. + const targetSegmentIds = React.useMemo( + () => (lockedSegment ? [lockedSegment.id, ...segmentIds.filter((id) => id !== lockedSegment.id)] : segmentIds), + [lockedSegment, segmentIds], + ); + + // What the result step names back. A segment the list has not loaded + // falls back to the locked one's name rather than showing an id. + const pinnedSegmentNames = React.useMemo(() => { + const byId = new Map((segments.data ?? []).map((seg) => [seg.id, seg.name])); + return targetSegmentIds.map((id) => byId.get(id) ?? (id === lockedSegment?.id ? lockedSegment.name : "a segment")); + }, [segments.data, targetSegmentIds, lockedSegment]); + async function commit() { if (!file || !preview) return; setCommitBusy(true); @@ -137,11 +158,16 @@ export default function ImportWizard({ open, onClose, lockedCampaign }: Props) { has_header: hasHeader, category_ids: categoryIds.length > 0 ? categoryIds : undefined, campaign_ids: lockedCampaign ? [lockedCampaign.id] : campaignIds.length > 0 ? campaignIds : undefined, - segment_ids: segmentIds.length > 0 ? segmentIds : undefined, + segment_ids: targetSegmentIds.length > 0 ? targetSegmentIds : undefined, }); setResult(res); setStep("result"); - await queryClient.invalidateQueries({ queryKey: ["contacts"] }); + // Segment counts and pinned-member lists move with the import, so + // the page behind the wizard is right before the spine event lands. + await Promise.all([ + queryClient.invalidateQueries({ queryKey: ["contacts"] }), + queryClient.invalidateQueries({ queryKey: ["segments"] }), + ]); if (res.failed === 0) { toast.success(`Imported ${res.imported} · updated ${res.updated} · skipped ${res.skipped}`); } else { @@ -196,6 +222,15 @@ export default function ImportWizard({ open, onClose, lockedCampaign }: Props) { → {lockedCampaign.name} )} + {lockedSegment && ( + + → + {lockedSegment.color && ( + + )} + {lockedSegment.name} + + )} @@ -178,7 +184,8 @@ export default function SyncSourcesPanel({

Connect a Google Sheet and re-run it on demand to pull new and - updated leads into Warmbly{campaign ? ` and into ${campaign.name}` : ""}. + updated leads into Warmbly + {campaign ? ` and into ${campaign.name}` : segment ? ` and into ${segment.name}` : ""}.

) : ( @@ -213,6 +220,7 @@ export default function SyncSourcesPanel({ open={wizardOpen} onClose={() => setWizardOpen(false)} lockedCampaign={campaign} + lockedSegment={segment} onSaved={() => sources.refetch()} /> {editing && ( diff --git a/web/src/lib/api/client/app/contacts/importContacts.ts b/web/src/lib/api/client/app/contacts/importContacts.ts index 5e2a5247..516400e8 100644 --- a/web/src/lib/api/client/app/contacts/importContacts.ts +++ b/web/src/lib/api/client/app/contacts/importContacts.ts @@ -69,6 +69,10 @@ export interface ImportResult { // Set when more rows failed than the API reports back; `errors` then holds // the first slice of them and `failed` is the true count. errors_truncated?: boolean; + // Absent when the import had no segment targets. With targets, true when + // every membership write landed and false when one did not, with the + // reason among the notes in `errors`. + segments_pinned?: boolean; // What the uploaded addresses look like. Advisory: a bad list is reported // here and stopped at launch, never refused here. quality?: ImportQuality; diff --git a/web/src/lib/api/client/app/leadsync/listSources.ts b/web/src/lib/api/client/app/leadsync/listSources.ts index 58ce00f6..473cc0d4 100644 --- a/web/src/lib/api/client/app/leadsync/listSources.ts +++ b/web/src/lib/api/client/app/leadsync/listSources.ts @@ -1,14 +1,20 @@ import type { LeadSyncSource } from "@/lib/api/models/app/leadsync/LeadSync"; import Request from "../../Request"; -// Lists this org's saved sync sources, optionally filtered to one campaign -// (powers both the global Contacts > Sync sources area and the per-campaign -// "Connect a Google Sheet" list). -export default async function listSources(campaignId?: string): Promise<{ data: LeadSyncSource[] }> { +// Lists this org's saved sync sources, optionally filtered to one campaign or +// one segment (powers the global Contacts > Sync sources area, the per-campaign +// "Connect a Google Sheet" list and a segment's own sources). +export default async function listSources( + campaignId?: string, + segmentId?: string, +): Promise<{ data: LeadSyncSource[] }> { + const params: Record = {}; + if (campaignId) params.campaign_id = campaignId; + if (segmentId) params.segment_id = segmentId; return await Request<{ data: LeadSyncSource[] }>({ method: "GET", url: "/lead-sync/sources", - params: campaignId ? { campaign_id: campaignId } : undefined, + params: Object.keys(params).length > 0 ? params : undefined, authorization: true, }); } diff --git a/web/src/lib/api/hooks/app/leadsync/useLeadSyncSources.ts b/web/src/lib/api/hooks/app/leadsync/useLeadSyncSources.ts index 9f003713..2432f63c 100644 --- a/web/src/lib/api/hooks/app/leadsync/useLeadSyncSources.ts +++ b/web/src/lib/api/hooks/app/leadsync/useLeadSyncSources.ts @@ -1,12 +1,13 @@ import { useQuery } from "@tanstack/react-query"; import listSources from "@/lib/api/client/app/leadsync/listSources"; -// Lists saved sync sources, optionally filtered to one campaign. Used by both -// the global Contacts > Sync sources area and the per-campaign list. -export default function useLeadSyncSources(campaignId?: string) { +// Lists saved sync sources, optionally filtered to one campaign or one segment. +// Used by the global Contacts > Sync sources area, the per-campaign list and a +// segment's own sources. +export default function useLeadSyncSources(campaignId?: string, segmentId?: string) { return useQuery({ - queryKey: ["lead-sync", "sources", campaignId ?? null], - queryFn: () => listSources(campaignId), + queryKey: ["lead-sync", "sources", campaignId ?? null, segmentId ?? null], + queryFn: () => listSources(campaignId, segmentId), staleTime: 10_000, }); } diff --git a/web/src/lib/api/models/app/leadsync/LeadSync.ts b/web/src/lib/api/models/app/leadsync/LeadSync.ts index a7372248..558a22d6 100644 --- a/web/src/lib/api/models/app/leadsync/LeadSync.ts +++ b/web/src/lib/api/models/app/leadsync/LeadSync.ts @@ -43,6 +43,8 @@ export interface LeadSyncSource { dedup: ImportDedupStrategy; target_campaign_id?: string; category_ids: string[]; + // Segments every synced row is pinned into on each run. + segment_ids: string[]; subscribed_default: boolean; label?: string; status: LeadSyncStatus; @@ -94,6 +96,7 @@ export interface CreateLeadSyncSource { dedup: ImportDedupStrategy; target_campaign_id?: string; category_ids: string[]; + segment_ids: string[]; subscribed_default?: boolean; label: string; } @@ -111,6 +114,7 @@ export interface UpdateLeadSyncSource { target_campaign_id?: string; clear_campaign?: boolean; category_ids?: string[]; + segment_ids?: string[]; subscribed_default?: boolean; label?: string; }