From ffd27bc46d2eb07bb0e1ccae186406ecd670e116 Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Mon, 14 Sep 2026 12:20:47 -0700 Subject: [PATCH] fix: stop every out-of-office notice and bounce opening a high-priority CRM follow-up by classifying machine replies from their headers and gating the task on a per-intent setting, and give the Tasks page multi-select with select-all-matching, bulk status, priority and delete over new PATCH and DELETE /crm/tasks endpoints (issue #471) --- docs/content/docs/api/endpoints.mdx | 1 + docs/content/docs/api/error-codes.mdx | 4 + docs/content/docs/api/reference/analytics.mdx | 3 + docs/content/docs/api/reference/campaigns.mdx | 3 +- docs/content/docs/api/reference/crm.mdx | 84 ++- .../docs/api/reference/deliverability-ops.mdx | 5 + docs/content/docs/api/reference/webhooks.mdx | 2 +- docs/content/docs/guides/contacts-crm.mdx | 34 ++ docs/public/openapi.json | 310 ++++++++++- internal/api/handler/crm.go | 78 +++ internal/api/routes.go | 5 + internal/app/advanced/reply_intent_test.go | 89 ++++ internal/app/advanced/service.go | 75 ++- internal/app/crm/service.go | 93 ++++ internal/app/replyclassify/classifier.go | 8 + internal/models/advanced_outreach.go | 118 ++++- internal/models/advanced_outreach_test.go | 79 +++ internal/models/crm.go | 80 +++ internal/models/crm_task_search_test.go | 47 ++ .../repository/crm_task_bulk_live_test.go | 486 ++++++++++++++++++ internal/repository/pg_advanced_outreach.go | 4 +- internal/repository/pg_crm.go | 179 ++++++- web/src/app/app/crm/tasks/page.tsx | 421 ++++++++++++++- web/src/app/app/settings/sending/page.tsx | 98 +++- web/src/components/app/contacts/selection.ts | 77 +-- .../client/app/crm/tasks/bulkDeleteTasks.ts | 15 + .../client/app/crm/tasks/bulkUpdateTasks.ts | 11 + .../hooks/app/crm/tasks/useBulkDeleteTasks.ts | 17 + .../hooks/app/crm/tasks/useBulkUpdateTasks.ts | 17 + .../models/app/analytics/Deliverability.ts | 3 + .../lib/api/models/app/crm/TaskSelection.ts | 23 + .../models/app/integrations/Integration.ts | 1 + .../models/app/outreach/OutreachSettings.ts | 55 +- web/src/lib/helper/rowSelection.test.ts | 84 +++ web/src/lib/helper/rowSelection.ts | 112 ++++ 35 files changed, 2603 insertions(+), 118 deletions(-) create mode 100644 internal/app/advanced/reply_intent_test.go create mode 100644 internal/models/crm_task_search_test.go create mode 100644 internal/repository/crm_task_bulk_live_test.go create mode 100644 web/src/lib/api/client/app/crm/tasks/bulkDeleteTasks.ts create mode 100644 web/src/lib/api/client/app/crm/tasks/bulkUpdateTasks.ts create mode 100644 web/src/lib/api/hooks/app/crm/tasks/useBulkDeleteTasks.ts create mode 100644 web/src/lib/api/hooks/app/crm/tasks/useBulkUpdateTasks.ts create mode 100644 web/src/lib/api/models/app/crm/TaskSelection.ts create mode 100644 web/src/lib/helper/rowSelection.test.ts create mode 100644 web/src/lib/helper/rowSelection.ts diff --git a/docs/content/docs/api/endpoints.mdx b/docs/content/docs/api/endpoints.mdx index 970d5032e..34964b173 100644 --- a/docs/content/docs/api/endpoints.mdx +++ b/docs/content/docs/api/endpoints.mdx @@ -247,6 +247,7 @@ The `agent-drafts` endpoints back the [inbox agent](/guides/inbox-agent/): the a | POST/PATCH/DELETE | `/crm/deals[/:id]` | `WRITE_CRM` | | GET | `/crm/tasks`, `/crm/tasks/:id` | `READ_CRM` | | POST/PATCH/DELETE | `/crm/tasks[/:id]` | `WRITE_CRM` | +| PATCH/DELETE | `/crm/tasks` (bulk, over an id list or a whole filter) | `WRITE_CRM` | ### Analytics and audit diff --git a/docs/content/docs/api/error-codes.mdx b/docs/content/docs/api/error-codes.mdx index 6ef876b58..b9eb28619 100644 --- a/docs/content/docs/api/error-codes.mdx +++ b/docs/content/docs/api/error-codes.mdx @@ -87,6 +87,10 @@ Returned when the request cannot be processed due to invalid syntax. | `empty_step_body` | `POST /campaigns/:id/start` found an email step with nothing in either body, so it would send a blank message to every lead it reached. Write the step's body and start again | | `no_leads` | `POST /campaigns/:id/start` on a campaign that has never had a lead, with `continuous` off. Add contacts, or set `continuous` so it starts empty and waits for them. A campaign whose leads have all finished is a different case: it starts and waits | | `no_remaining_leads` | A platform-initiated restart of a campaign with nothing left to send and `continuous` off found nothing to do; the campaign is `completed` again. A start you request never answers this: it turns `continuous` on and waits | +| `too_many_tasks` | `PATCH /crm/tasks` or `DELETE /crm/tasks` was given more than `1000` ids in one request, or more than `50,000` exclusions. Split it into batches | +| `selection_too_large` | A `"all": true` bulk selection resolved to more than `50,000` rows. Narrow the filter and run it in parts; nothing was changed | +| `invalid_filter` | A task filter carried an id that is not one: `assigned_to`, `contact_id` and `deal_id` name records, and are matched against id columns. Sent by `POST /crm/tasks/search`, `POST /crm/tasks/summary`, and the `filters` of a `"all": true` bulk selection | +| `invalid_setting` | `PATCH /outreach/settings` (or a campaign's advanced settings) carried a value outside the documented vocabulary, for example a `reply_intent.crm_task_intents` entry that is not a reply intent | | `no_organization` | The request needs a workspace and the caller has none selected. Every entitlement, limit and suppression rule is scoped to a workspace, so a write that would run unscoped is refused rather than run without those checks. API keys always carry their workspace; a dashboard session picks one at sign-in, so this normally means the session predates the workspace being chosen. Select a workspace and retry | ### 401 Unauthorized diff --git a/docs/content/docs/api/reference/analytics.mdx b/docs/content/docs/api/reference/analytics.mdx index 626a732ea..5af0d1794 100644 --- a/docs/content/docs/api/reference/analytics.mdx +++ b/docs/content/docs/api/reference/analytics.mdx @@ -103,6 +103,7 @@ Returns the organization's deliverability posture for a time window: bounce, com "intent_out_of_office": 11, "intent_question": 9, "intent_neutral": 13, + "intent_automated": 22, "emails_sent": 1240, "bounce_rate": 0.73, "complaint_rate": 0.08, @@ -175,6 +176,8 @@ Returns the organization's deliverability posture for a time window: bounce, com } ``` +The `intent_*` counters and `reply_count` are two different things and do not add up to each other. `reply_count` counts `reply` deliverability events, which are the ones a provider or your own integration reports to [record an event](/api/reference/deliverability-ops/). The `intent_*` counters are the replies Warmbly classified as they arrived in a connected mailbox, one per reply, including the machine ones: a vacation notice lands in `intent_out_of_office` and an autoresponder, ticket acknowledgement or bounce notice in `intent_automated`. A workspace that reports no reply events sees `reply_count` at zero with intents counted normally. + `spam_placement_rate` and `inbox_placement_rate` are omitted when there are no seed samples in the window. `by_provider` rolls the same seed samples up per recipient provider; `warmup_placement` is the continuous warmup signal per recipient domain, where `delivered` counts verified warmup arrivals and `spam` the subset the recipient's provider filed into junk. `score` starts at `100` and subtracts saturating penalties for bounce rate (up to `40` points, maxed at `10%`), complaint rate (up to `30` points, maxed at `0.30%`), and spam placement (up to `40` points, maxed at `40%`). ## Get warmup analytics diff --git a/docs/content/docs/api/reference/campaigns.mdx b/docs/content/docs/api/reference/campaigns.mdx index 64086a9e2..0e77716ef 100644 --- a/docs/content/docs/api/reference/campaigns.mdx +++ b/docs/content/docs/api/reference/campaigns.mdx @@ -353,6 +353,7 @@ A `CampaignAdvancedSettings` object: the campaign id, the `overrides` block, and "out_of_office_keywords": ["out of office", "vacation"], "question_keywords": ["?", "how", "price"], "auto_create_crm_task": true, + "crm_task_intents": ["positive", "question", "neutral", "negative"], "auto_pause_on_negative": false, "auto_suppress_on_unsubscribe_keyword": true, "hold_on_out_of_office": true, @@ -403,7 +404,7 @@ Replace the campaign's advanced overrides. **Scope** `WRITE_CAMPAIGNS` · **Org "bounce_pipeline": { "enabled": true, "auto_suppress_on_bounce": true, "auto_suppress_on_complaint": true, "auto_suppress_on_unsubscribe": true, "auto_pause_campaign_on_spike": true, "pause_bounce_rate_threshold": 8, "pause_complaint_rate_threshold": 1.5 }, "task_reliability": { "enabled": true, "dlq_enabled": true, "max_attempts": 5, "execution_window_seconds": 300 }, "ab_testing": { "enabled": true, "default_winning_rule": "reply_rate", "auto_promote_winner": false, "min_sample_size": 30 }, - "reply_intent": { "enabled": true, "positive_keywords": [], "negative_keywords": [], "out_of_office_keywords": [], "question_keywords": [], "auto_create_crm_task": true, "auto_pause_on_negative": false, "auto_suppress_on_unsubscribe_keyword": true, "hold_on_out_of_office": true, "out_of_office_hold_days": 7 }, + "reply_intent": { "enabled": true, "positive_keywords": [], "negative_keywords": [], "out_of_office_keywords": [], "question_keywords": [], "auto_create_crm_task": true, "crm_task_intents": ["positive", "question", "neutral", "negative"], "auto_pause_on_negative": false, "auto_suppress_on_unsubscribe_keyword": true, "hold_on_out_of_office": true, "out_of_office_hold_days": 7 }, "send_time_optimization": { "enabled": true, "use_contact_timezone": true, "default_contact_timezone": "UTC", "preferred_hours": [9, 14], "weekend_weight_multiplier": 0.5 }, "preflight": { "enabled": true, "check_tracking_domain": true, "check_unsubscribe_header": true, "check_ab_variant_configured": false, "check_daily_limit": true, "check_schedule_window": true, "check_content_score": true, "min_content_score": 60 }, "dashboard": { "enabled": true, "show_suppression_log": true, "show_intent_summary": true, "show_dlq_stats": true } diff --git a/docs/content/docs/api/reference/crm.mdx b/docs/content/docs/api/reference/crm.mdx index 98a330549..a0ad96af1 100644 --- a/docs/content/docs/api/reference/crm.mdx +++ b/docs/content/docs/api/reference/crm.mdx @@ -924,7 +924,7 @@ Returns the task object (same shape as a list item). `PATCH /crm/tasks/:id` -Update a CRM task. Setting `status` to `completed` stamps the completion timestamp. +Update a CRM task. Setting `status` to `completed` stamps the completion timestamp, and any other status clears it. Auth: **Scope** `WRITE_CRM` · **Org permission** `manage_contacts` @@ -971,6 +971,88 @@ Auth: **Scope** `WRITE_CRM` · **Org permission** `manage_contacts` `204 No Content`. +## Selecting many tasks + +The two bulk endpoints below take the same **selection**, in one of two forms. Either an explicit list of ids: + +```json +{ "tasks": ["3b4c5d6e-7f80-4912-a3b4-c5d6e7f80912"] } +``` + +Or every task a search matches, minus the ones taken back out: + +```json +{ + "all": true, + "filters": { "statuses": ["pending"], "query": "out_of_office" }, + "exclude": ["3b4c5d6e-7f80-4912-a3b4-c5d6e7f80912"] +} +``` + +| Field | Type | Required | Description | +| --- | --- | --- | --- | +| `tasks` | string[] | with `all` unset | Task ids. At most `1000` per request. | +| `all` | boolean | no | Switches the selection from `tasks` to `filters`. | +| `filters` | object | with `all` | The same body [search tasks](#search-tasks) takes, so the set acted on is exactly the set the search returns. | +| `exclude` | string[] | no | Ids to drop from the resolved set. Ignored unless `all` is set. | + +A selection resolving to more than `50,000` tasks is refused with `selection_too_large` rather than half applied; narrow the filter and run it in parts. A `filters` block naming something that is not an id in `assigned_to`, `contact_id` or `deal_id` is refused with `invalid_filter`. + +A bulk update raises one `crm.task_updated` [webhook](/api/reference/webhooks/) for the whole call rather than one per task, carrying `metadata.bulk` and `metadata.count` and no `entity_id`. A bulk delete raises none, because deleting a task raises none either way. + +Neither endpoint takes an `Idempotency-Key`: both write an end state rather than a delta, so repeating one converges on the same result. They differ in what the repeat reports. A repeated delete finds fewer rows and `affected` falls to `0`. A repeated update still touches every row in the selection, because it stamps `updated_at`, so `affected` does not fall; the status and priority simply do not change, and a repeated completion adds no second entry to the contact timeline and does not move `completed_at`. + +## Bulk update tasks + +`PATCH /crm/tasks` + +Write a status, a priority, or both onto every task in a selection. At least one of the two fields is required; setting `status` to `completed` stamps the completion timestamp and records the completion on each linked contact's timeline, exactly as the single-task update does. Any other status clears `completed_at`, so a task moved back to `pending` does not keep the time it was finished, again matching the single-task update. + +Auth: **Scope** `WRITE_CRM` · **Org permission** `manage_contacts` + +### Request body + +The selection above, plus: + +| Field | Type | Required | Description | +| --- | --- | --- | --- | +| `status` | string | no | One of `pending`, `in_progress`, `completed`, `cancelled`. | +| `priority` | string | no | One of `low`, `medium`, `high`, `urgent`. | + +```json +{ + "all": true, + "filters": { "statuses": ["pending"], "priorities": ["high"] }, + "status": "completed" +} +``` + +### Response + +The number of tasks written, rather than the rows: a select-all can cover tens of thousands. + +```json +{ "affected": 128 } +``` + +## Bulk delete tasks + +`DELETE /crm/tasks` + +Delete every task in a selection. The body is the selection object, or a bare array of ids. + +Auth: **Scope** `WRITE_CRM` · **Org permission** `manage_contacts` + +```json +{ "all": true, "filters": { "query": "out_of_office" } } +``` + +### Response + +```json +{ "affected": 128 } +``` + ## Errors All endpoints return the standard error envelope on failure, for example a malformed UUID path param or an invalid request body. See [error codes](/api/error-codes/) for the full list. diff --git a/docs/content/docs/api/reference/deliverability-ops.mdx b/docs/content/docs/api/reference/deliverability-ops.mdx index ac493678e..0d49d289f 100644 --- a/docs/content/docs/api/reference/deliverability-ops.mdx +++ b/docs/content/docs/api/reference/deliverability-ops.mdx @@ -49,6 +49,7 @@ Returns the `AdvancedOutreachSettings` object directly (not wrapped in an envelo "out_of_office_keywords": ["out of office", "ooo", "vacation", "abwesenheitsnotiz"], "question_keywords": ["?", "how", "price"], "auto_create_crm_task": true, + "crm_task_intents": ["positive", "question", "neutral", "negative"], "auto_pause_on_negative": false, "auto_suppress_on_unsubscribe_keyword": true, "hold_on_out_of_office": true, @@ -89,6 +90,10 @@ Returns the `AdvancedOutreachSettings` object directly (not wrapped in an envelo The `unsubscribe` block is the workspace default for the opt-out appended after the signature of every campaign email: `mode` is `text` (a reply-to-opt-out sentence, the default), `link` (a sentence with the recipient's signed unsubscribe link) or `off`; a campaign's own `unsubscribe_mode` (`inherit` by default) overrides it. Copy fields are single lines of at most 300 characters and fall back to the defaults when blank. See the [unsubscribe guide](/guides/unsubscribe/). + +`reply_intent.auto_create_crm_task` opens a CRM task when a classified reply lands, and `reply_intent.crm_task_intents` names which intents are worth one. Omit the field (or send `null`) and the default applies: `positive`, `question`, `neutral`, `negative`, which is every human intent and no automated one. Send `["positive"]` to narrow it, `["positive", "question", "neutral", "negative", "out_of_office", "automated"]` to include auto-responders and bounces, or `[]` to open nothing (the same as turning the switch off). An intent outside that vocabulary is refused with `400`. See [Tasks](/guides/contacts-crm/#tasks). + + `preflight.min_content_score` is stored clamped to `1`-`100`. A value outside that range is corrected on write rather than rejected, so a floor above `100` cannot flag every campaign permanently. Set `preflight.check_content_score` to `false` to turn the check off; the score never blocks or delays a send either way. See [Content checks](/guides/campaigns/). diff --git a/docs/content/docs/api/reference/webhooks.mdx b/docs/content/docs/api/reference/webhooks.mdx index 134003ac3..076a82f09 100644 --- a/docs/content/docs/api/reference/webhooks.mdx +++ b/docs/content/docs/api/reference/webhooks.mdx @@ -520,7 +520,7 @@ The full catalog is available at `GET /webhooks/event-types`. Below is the compl | `crm.deal_updated` | A deal's fields or stage change. | | | `crm.deal_deleted` | A deal is deleted. | | | `crm.task_created` | A CRM task is created. | | -| `crm.task_updated` | A CRM task changes. | | +| `crm.task_updated` | A CRM task changes. A bulk write ([bulk update tasks](/api/reference/crm/#bulk-update-tasks)) raises it once for the whole call, with no `entity_id` and a `metadata` of `bulk` and `count`, so treat a missing id as "re-read the tasks you track" rather than as one task. `contact.updated` behaves the same way for a bulk contact edit. | | | `crm.note_created` | A note is added to a contact or deal. | | | `crm.pipeline_updated` | A pipeline or its stages change. | | diff --git a/docs/content/docs/guides/contacts-crm.mdx b/docs/content/docs/guides/contacts-crm.mdx index b1bb2749a..96103a4f9 100644 --- a/docs/content/docs/guides/contacts-crm.mdx +++ b/docs/content/docs/guides/contacts-crm.mdx @@ -148,6 +148,40 @@ A deal has a name, stage, value and currency (default `USD`), expected close dat Campaigns can create and advance deals automatically with **Create deal** and **Move deal stage** actions, so the CRM stays current without manual entry. +## Tasks + +**CRM > Tasks** is the follow-up list for the whole workspace: everything anyone has to do about a contact or a deal, in one place. A task carries a title, a description, a type (`Call`, `Email`, `Meeting` by default, and any type you add), a due date, a priority of `low`, `medium`, `high` or `urgent`, a status of `pending`, `in progress`, `completed` or `cancelled`, and an assignee who can be a person, a team, or nobody. + +Two views share one list. **Table** is the flat list with a column per field; **By due date** groups it into Overdue, Today, Tomorrow, This week, Later and No due date. Both filter, sort and page on the server, so the Overdue, High priority, Pending, Active and Completed totals across the top count the whole matching set rather than the rows loaded so far. The status tabs, the search box, the assignee, team and type facets and the filter popover all narrow the same set. + +Clicking a row opens it for editing. Each row carries two controls before its title: the checkbox at the far left selects it for a bulk action, and the square next to the title marks it done on its own. + +### Selecting many tasks + +Tick a row to select it, the checkbox in the table header to select every row loaded so far, or a group's header in the by-due-date view to select that group. When more tasks match than are loaded, a bar appears offering **Select all N matching**, which hands the whole filtered set to the next action however many pages that is. Unticking a row afterwards takes just that task out and the count follows. + +The selection means the filter it was made under, so changing any filter clears it. A selection past `50,000` tasks is refused rather than half applied; add a filter and work through it in parts. + +With rows selected, a bar appears at the bottom: **Mark done**, **Status** (any of the four), **Priority** (any of the four), and **Delete**, which confirms first and names the count. This is how you clear a backlog of tasks nobody needs without opening each one. To clear the follow-ups a workspace collected before it narrowed the setting below, search for `out_of_office`, tick the header checkbox, take **Select all N matching** and delete them in one step. + +### Follow-up tasks from replies + +Warmbly can open a task by itself when a reply lands, so a prospect who answers ends up on this list rather than only in the inbox. The task is assigned to the owner of the mailbox that received the reply, due in 24 hours, and titled with the intent and the sender: **Follow up: positive reply from dana@acme.com**. + +Which replies get one is up to you, in **Settings > Sending > Reply follow-ups**. Every reply is classified first, and the classifier can tell a person from a machine: an out-of-office notice, a helpdesk autoresponder, a bounce and a delivery report are recognized from the message headers, not only from the words in the body, so an auto-responder that never mentions a vacation is still caught. + +| Intent | Opens a task by default | +| --- | --- | +| Positive: interested, wants a call | Yes | +| Question: asked something | Yes | +| Neutral: a human reply we could not bucket | Yes | +| Negative: not interested | Yes | +| Automated: out of office, autoresponders, bounces | No | + +Automated replies are off deliberately. A vacation notice is not follow-up work, and a week of sending makes enough of them to bury the real replies. Tick **Automated** if you do want one per machine reply, or untick the human ones you do not need. Unticking everything is the same as turning the switch off. + +The classification does more than decide the task: an automated reply never counts as a reply for **stop on reply** or for a **replied** branch in a sequence, and its notification goes to the **Out-of-office detected** preference rather than **New reply**, so muting one does not mute the other. + ## Suppression A suppressed contact receives no further mail. Unsubscribed contacts are skipped by every campaign automatically; toggle subscription on the Details tab, and filter the list by Subscribed or Unsubscribed. diff --git a/docs/public/openapi.json b/docs/public/openapi.json index bbf23326b..7ae0a66dc 100644 --- a/docs/public/openapi.json +++ b/docs/public/openapi.json @@ -11228,6 +11228,171 @@ } } } + }, + "patch": { + "operationId": "crm_bulk_update_tasks", + "summary": "Bulk update tasks", + "description": "Write a status, a priority, or both onto every task in a selection: the ids given, or every task the supplied filter matches minus the ids in exclude.", + "tags": [ + "crm" + ], + "security": [ + { + "bearerAuth": [] + } + ], + "requestBody": { + "required": true, + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/BulkUpdateTasks" + } + } + } + }, + "responses": { + "200": { + "description": "Number of tasks updated.", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/BulkTasksResponse" + } + } + } + }, + "400": { + "description": "Invalid body, an unknown status or priority, a selection with no ids, more than 1000 ids or more than 50000 exclusions (too_many_tasks), a filter naming something that is not an id (invalid_filter), or a selection resolving to more than 50000 tasks (selection_too_large).", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "401": { + "description": "Missing or invalid credentials.", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "403": { + "description": "Missing required scope or permission.", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "429": { + "description": "Rate limited.", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + } + } + }, + "delete": { + "operationId": "crm_bulk_delete_tasks", + "summary": "Bulk delete tasks", + "description": "Delete every task in a selection. The body is the selection object, or a bare array of task ids.", + "tags": [ + "crm" + ], + "security": [ + { + "bearerAuth": [] + } + ], + "requestBody": { + "required": true, + "content": { + "application/json": { + "schema": { + "oneOf": [ + { + "$ref": "#/components/schemas/TaskSelection" + }, + { + "type": "array", + "minItems": 1, + "maxItems": 1000, + "items": { + "type": "string", + "format": "uuid" + }, + "description": "A bare list of task ids, equivalent to sending {\"tasks\": [...]}." + } + ], + "description": "The selection object, or a bare array of task ids." + } + } + } + }, + "responses": { + "200": { + "description": "Number of tasks deleted.", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/BulkTasksResponse" + } + } + } + }, + "400": { + "description": "Invalid body, a task id that is not an id, a selection with no ids, more than 1000 ids or more than 50000 exclusions (too_many_tasks), a filter naming something that is not an id (invalid_filter), or a selection resolving to more than 50000 tasks (selection_too_large).", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "401": { + "description": "Missing or invalid credentials.", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "403": { + "description": "Missing required scope or permission.", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + }, + "429": { + "description": "Rate limited.", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + } + } } }, "/crm/tasks/search": { @@ -22822,7 +22987,26 @@ } }, "auto_create_crm_task": { - "type": "boolean" + "type": "boolean", + "description": "Open a CRM follow-up task when a classified reply lands." + }, + "crm_task_intents": { + "type": [ + "array", + "null" + ], + "description": "Which intents are worth a follow-up task. Omit or send null for the default set (positive, question, neutral, negative: every human intent and no automated one). An empty array opens nothing, the same as turning the switch off.", + "items": { + "type": "string", + "enum": [ + "positive", + "negative", + "question", + "neutral", + "out_of_office", + "automated" + ] + } }, "auto_pause_on_negative": { "type": "boolean" @@ -27788,6 +27972,10 @@ "intent_neutral": { "type": "integer" }, + "intent_automated": { + "type": "integer", + "description": "Machine replies that are not vacation notices: autoresponders, ticket acknowledgements, bounces, delivery reports." + }, "emails_sent": { "type": "integer" }, @@ -31487,6 +31675,126 @@ "description": "Also replace the mailbox's stored signature with the one configured at the provider, for whichever identity the mailbox sends as. An empty signature at the provider changes nothing." } } + }, + "TaskSelection": { + "type": "object", + "description": "Names the tasks a bulk action applies to: either an explicit id list, or every task matching a search (all + filters) minus the ids in exclude. The filter form lets one call cover far more tasks than a page, and is refused with selection_too_large past 50000 matches.", + "oneOf": [ + { + "required": [ + "tasks" + ], + "properties": { + "all": { + "enum": [ + false + ] + } + } + }, + { + "required": [ + "all", + "filters" + ], + "properties": { + "all": { + "enum": [ + true + ] + } + } + } + ], + "properties": { + "tasks": { + "type": "array", + "minItems": 1, + "maxItems": 1000, + "items": { + "type": "string", + "format": "uuid" + }, + "description": "Task ids (1 to 1000). Required unless all is set." + }, + "all": { + "type": "boolean", + "description": "Resolve the selection from filters instead of tasks." + }, + "filters": { + "allOf": [ + { + "$ref": "#/components/schemas/SearchTasks" + } + ], + "description": "The search whose matches the action applies to. Required when all is true." + }, + "exclude": { + "type": "array", + "items": { + "type": "string", + "format": "uuid" + }, + "maxItems": 50000, + "description": "Task ids to drop from the resolved set. Ignored unless all is set." + } + } + }, + "BulkUpdateTasks": { + "allOf": [ + { + "$ref": "#/components/schemas/TaskSelection" + } + ], + "description": "A task selection plus the fields to write on every task in it. At least one of status or priority is required.", + "anyOf": [ + { + "required": [ + "status" + ] + }, + { + "required": [ + "priority" + ] + } + ], + "properties": { + "status": { + "type": "string", + "enum": [ + "pending", + "in_progress", + "completed", + "cancelled" + ], + "description": "Status to set. completed stamps completed_at and records the completion on each linked contact's timeline." + }, + "priority": { + "type": "string", + "enum": [ + "low", + "medium", + "high", + "urgent" + ], + "description": "Priority to set." + } + } + }, + "BulkTasksResponse": { + "type": "object", + "description": "How many tasks the bulk action touched. The count is returned rather than the rows: a select-all can cover tens of thousands.", + "properties": { + "affected": { + "type": "integer", + "format": "int64", + "description": "Tasks written or deleted." + } + }, + "required": [ + "affected" + ] } } } diff --git a/internal/api/handler/crm.go b/internal/api/handler/crm.go index 96dca2b72..68ef2f369 100644 --- a/internal/api/handler/crm.go +++ b/internal/api/handler/crm.go @@ -1,6 +1,8 @@ package handler import ( + "encoding/json" + "fmt" "net/http" "strconv" @@ -902,6 +904,82 @@ func (h *Handler) UpdateCRMTask(c *gin.Context) { c.JSON(http.StatusOK, task) } +// BulkUpdateCRMTasks writes one status or priority onto a whole selection: +// the ids the user ticked, or every task the current filter matches minus the +// ones unticked afterwards. The response carries the count rather than the +// rows, because a select-all can cover tens of thousands. +func (h *Handler) BulkUpdateCRMTasks(c *gin.Context) { + orgID := middleware.GetOrganizationID(c) + if orgID == nil { + errx.Handle(c, errx.New(errx.BadRequest, "no organization selected")) + return + } + + var data models.BulkUpdateTasks + if err := c.ShouldBindJSON(&data); err != nil { + errx.Handle(c, errx.ErrInvalid) + return + } + + // The completion activity carries the actor, and contact_activities.user_id + // is a nullable FK: an unresolvable actor travels as NULL rather than as a + // zero uuid the insert would be refused for, which would take the whole + // bulk write down with it. + var actor *uuid.UUID + if userID, err := middleware.GetUserUUID(c); err == nil { + actor = &userID + } + affected, xerr := h.CRMService.BulkUpdateCRMTasks(c.Request.Context(), *orgID, actor, &data) + if xerr != nil { + errx.Handle(c, xerr) + return + } + + h.auditOrg(c, models.AuditActionUpdate, models.AuditEntityCRMTask, nil, nil, map[string]string{ + "bulk": "true", + "count": fmt.Sprintf("%d", affected), + }) + + c.JSON(http.StatusOK, models.BulkTasksResponse{Affected: affected}) +} + +// BulkDeleteCRMTasks deletes a whole selection. Two bodies are accepted: a +// bare id array, and the selection object ({"all":true,"filters":{...}}) the +// Tasks page sends for "select all matching". +func (h *Handler) BulkDeleteCRMTasks(c *gin.Context) { + orgID := middleware.GetOrganizationID(c) + if orgID == nil { + errx.Handle(c, errx.New(errx.BadRequest, "no organization selected")) + return + } + + var raw json.RawMessage + if err := c.ShouldBindJSON(&raw); err != nil { + errx.Handle(c, errx.ErrInvalid) + return + } + var sel models.TaskSelection + if err := json.Unmarshal(raw, &sel.Tasks); err != nil { + if err := json.Unmarshal(raw, &sel); err != nil { + errx.Handle(c, errx.ErrInvalid) + return + } + } + + affected, xerr := h.CRMService.BulkDeleteCRMTasks(c.Request.Context(), *orgID, sel) + if xerr != nil { + errx.Handle(c, xerr) + return + } + + h.auditOrg(c, models.AuditActionDelete, models.AuditEntityCRMTask, nil, nil, map[string]string{ + "bulk": "true", + "count": fmt.Sprintf("%d", affected), + }) + + c.JSON(http.StatusOK, models.BulkTasksResponse{Affected: affected}) +} + func (h *Handler) DeleteCRMTask(c *gin.Context) { orgID := middleware.GetOrganizationID(c) if orgID == nil { diff --git a/internal/api/routes.go b/internal/api/routes.go index fbb436c65..30c97a48b 100644 --- a/internal/api/routes.go +++ b/internal/api/routes.go @@ -1137,6 +1137,11 @@ func Run( { crmTasks.GET("", m.RequireAccess(models.PermViewContacts, models.APIPermReadCRM), h.ListCRMTasks) crmTasks.POST("", m.RequireAccess(models.PermManageContacts, models.APIPermWriteCRM), h.CreateCRMTask) + // Bulk status/priority and bulk delete over a selection: the + // ids ticked, or the whole current filter. Same scope as + // the single-task routes they stand in for. + crmTasks.PATCH("", m.RequireAccess(models.PermManageContacts, models.APIPermWriteCRM), h.BulkUpdateCRMTasks) + crmTasks.DELETE("", m.RequireAccess(models.PermManageContacts, models.APIPermWriteCRM), h.BulkDeleteCRMTasks) crmTasks.POST("/search", m.RequireAccess(models.PermViewContacts, models.APIPermReadCRM), h.SearchCRMTasks) crmTasks.POST("/summary", m.RequireAccess(models.PermViewContacts, models.APIPermReadCRM), h.TasksSummary) crmTasks.GET("/:id", m.RequireAccess(models.PermViewContacts, models.APIPermReadCRM), h.GetCRMTask) diff --git a/internal/app/advanced/reply_intent_test.go b/internal/app/advanced/reply_intent_test.go new file mode 100644 index 000000000..f9b13baa8 --- /dev/null +++ b/internal/app/advanced/reply_intent_test.go @@ -0,0 +1,89 @@ +package advanced + +import ( + "testing" + + "github.com/warmbly/warmbly/internal/app/replyclassify" + "github.com/warmbly/warmbly/internal/models" +) + +// Issue #471: the keyword classifier reads words, so an auto-responder that +// says nothing about vacations came back "neutral" and opened a high-priority +// follow-up. The header layer sees the markers instead, and the intent it +// produces is what the task gate reads. +func TestAutomatedRepliesNeverReachTheDefaultTaskGate(t *testing.T) { + settings := models.DefaultAdvancedOutreachSettings().ReplyIntent + for _, tc := range []struct { + name string + in replyclassify.Input + want models.ReplyIntentType + human bool + }{ + { + name: "exchange out of office", + in: replyclassify.Input{Subject: "Automatic reply: Quick question"}, + want: models.ReplyIntentOutOfOffice, + }, + { + name: "rfc 3834 auto-generated", + in: replyclassify.Input{Headers: map[string][]string{"Auto-Submitted": {"auto-generated"}}, Subject: "Ticket 8812 received"}, + want: models.ReplyIntentAutomated, + }, + { + name: "delivery status report", + in: replyclassify.Input{ + Headers: map[string][]string{"Content-Type": {"multipart/report; report-type=delivery-status; boundary=x"}}, + Subject: "Undeliverable", + }, + want: models.ReplyIntentAutomated, + }, + { + name: "mailer-daemon bounce", + in: replyclassify.Input{Headers: map[string][]string{"From": {"MAILER-DAEMON@mx.example.com"}}, Subject: "Returned mail"}, + want: models.ReplyIntentAutomated, + }, + { + name: "a person writing back", + in: replyclassify.Input{Subject: "Re: Quick question", BodyText: "Sounds interesting, can you send pricing?"}, + human: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + res := replyclassify.ClassifyOffline(tc.in) + if tc.human { + if replyclassify.IsAutomated(res.Class) { + t.Fatalf("a human reply was classified %q", res.Class) + } + return + } + if !replyclassify.IsAutomated(res.Class) { + t.Fatalf("classified %q, want an automated class", res.Class) + } + intent, _ := automatedIntent(res) + if intent != tc.want { + t.Fatalf("intent %q, want %q", intent, tc.want) + } + if settings.CreatesTaskFor(intent) { + t.Errorf("%q opened a follow-up task on default settings", intent) + } + }) + } +} + +// The wording is what a person reads on the Tasks page, so it stays plain +// rather than echoing the classifier's vocabulary. +func TestReplyTaskTitle(t *testing.T) { + for _, tc := range []struct { + intent models.ReplyIntentType + want string + }{ + {models.ReplyIntentPositive, "Follow up: positive reply from a@b.com"}, + {models.ReplyIntentNeutral, "Follow up: reply from a@b.com"}, + {models.ReplyIntentOutOfOffice, "Follow up: out-of-office reply from a@b.com"}, + {models.ReplyIntentAutomated, "Follow up: automatic reply from a@b.com"}, + } { + if got := replyTaskTitle(tc.intent, "a@b.com"); got != tc.want { + t.Errorf("%s: got %q, want %q", tc.intent, got, tc.want) + } + } +} diff --git a/internal/app/advanced/service.go b/internal/app/advanced/service.go index 5bca175c6..3fe990d0e 100644 --- a/internal/app/advanced/service.go +++ b/internal/app/advanced/service.go @@ -251,6 +251,9 @@ func (s *service) UpdateOrganizationSettings(ctx context.Context, organizationID return errx.New(errx.BadRequest, "settings are required") } settings.Normalize() + if err := settings.Validate(); err != nil { + return errx.NewWithIdentifier(errx.BadRequest, "invalid_setting", err.Error()) + } if err := s.repo.UpsertOutreachSettings(ctx, organizationID, updatedBy, settings); err != nil { return toErrx(err) } @@ -277,6 +280,9 @@ func (s *service) UpdateCampaignSettings(ctx context.Context, campaignID uuid.UU return errx.New(errx.BadRequest, "settings are required") } settings.Normalize() + if err := settings.Validate(); err != nil { + return errx.NewWithIdentifier(errx.BadRequest, "invalid_setting", err.Error()) + } if err := s.repo.UpsertCampaignAdvancedSettings(ctx, campaignID, settings); err != nil { return toErrx(err) } @@ -916,6 +922,31 @@ func buildReplyHeaders(msg *models.EmailMessageStoreData) map[string][]string { return h } +// replyTaskTitle words the follow-up the way it is read in a task list, rather +// than as the classifier's own vocabulary. +func replyTaskTitle(intent models.ReplyIntentType, sender string) string { + switch intent { + case models.ReplyIntentOutOfOffice: + return "Follow up: out-of-office reply from " + sender + case models.ReplyIntentAutomated: + return "Follow up: automatic reply from " + sender + case models.ReplyIntentNeutral: + return "Follow up: reply from " + sender + default: + return fmt.Sprintf("Follow up: %s reply from %s", intent, sender) + } +} + +// automatedIntent maps a machine-reply verdict onto the recorded intent +// vocabulary: a vacation notice keeps its own bucket, everything else machine +// (autoresponders, ticket acknowledgements, bounces) is "automated". +func automatedIntent(r replyclassify.Result) (models.ReplyIntentType, float64) { + if r.Class == replyclassify.ClassOutOfOffice { + return models.ReplyIntentOutOfOffice, r.Confidence + } + return models.ReplyIntentAutomated, r.Confidence +} + func firstNonEmpty(vals ...string) string { for _, v := range vals { if strings.TrimSpace(v) != "" { @@ -1028,12 +1059,12 @@ func (s *service) ProcessIncomingReply(ctx context.Context, emailAccountID uuid. text := strings.TrimSpace(msg.Snippet) text = strings.TrimSpace(text + "\n" + msg.Subject) - // Layered reply classification (header -> lexicon -> optional model) is run + // The full layered classification (including the optional model layer) runs // further down, once the campaign context is known to store it on. Classifying // only inside that block means a reply with no campaign match never spends a - // model call. replyClass is what it decided, read after the block; held is + // model call. verdict is what it decided, read after the block; held is // when an out-of-office hold lifts, for the notification to name. - var replyClass string + var verdict replyclassify.Result var held *time.Time var campaignID *uuid.UUID @@ -1108,7 +1139,7 @@ func (s *service) ProcessIncomingReply(ctx context.Context, emailAccountID uuid. // it (including reply_automated for OOO / autoresponders). Layers 1-2 run // for every reply, so OOO/unsubscribe stay correct even when the gate // skipped the model. - replyClass = replyResult.Class + verdict = replyResult _ = s.campaignProgressRepo.RecordReplyClassification(ctx, cID, ctID, sID, replyResult.Class, replyResult.Source, replyResult.Confidence) // Out of office: park the contact's next step until they are back @@ -1180,13 +1211,26 @@ func (s *service) ProcessIncomingReply(ctx context.Context, emailAccountID uuid. s.fireInstantActions(ctx, cID, ctID, sID, "reply") } + // A reply with no campaign behind it was never classified above, and a + // machine announces itself in the headers (RFC 3834, Precedence, a null + // Return-Path, a delivery-status report) whether or not we ever mailed the + // address. The header and lexicon layers are free, so answer "is this a + // human" for every inbound message; only the model layer is worth gating. + if verdict.Class == "" { + verdict = replyclassify.ClassifyOffline(replyclassify.Input{ + Headers: buildReplyHeaders(msg), + Subject: msg.Subject, + BodyText: firstNonEmpty(msg.BodyText, msg.Snippet), + }) + } + intent, confidence := classifyReply(text, settings.ReplyIntent) // The layered classifier reads auto-reply headers and a multilingual // out-of-office vocabulary the workspace's own keyword list does not, so // its verdict settles the case the keywords missed. Only the automated // classes are folded in: sentiment stays the keyword list's call. - if replyClass == replyclassify.ClassOutOfOffice { - intent, confidence = models.ReplyIntentOutOfOffice, 0.95 + if replyclassify.IsAutomated(verdict.Class) { + intent, confidence = automatedIntent(verdict) } actionTaken := "" @@ -1227,10 +1271,13 @@ func (s *service) ProcessIncomingReply(ctx context.Context, emailAccountID uuid. } } - if settings.ReplyIntent.AutoCreateCRMTask && s.crmRepo != nil && contactID != nil { + // Per-intent, so an out-of-office or a bounce does not become a + // high-priority follow-up nobody asked for. The default set is human + // replies only; a workspace can add the automated ones back. + if settings.ReplyIntent.CreatesTaskFor(intent) && s.crmRepo != nil && contactID != nil { owner, parseErr := uuid.Parse(account.UserID) if parseErr == nil { - title := fmt.Sprintf("Follow up reply intent: %s (%s)", intent, sender) + title := replyTaskTitle(intent, sender) _, _ = s.crmRepo.CreateCRMTask(ctx, *account.OrganizationID, owner, &models.CreateCRMTask{ ContactID: contactID, Title: title, @@ -1256,6 +1303,10 @@ func (s *service) ProcessIncomingReply(ctx context.Context, emailAccountID uuid. ActionTaken: actionTaken, Metadata: map[string]interface{}{ "subject": msg.Subject, + // What decided it, so an intent nobody expected can be explained + // without re-running the message through the classifier. + "reply_class": verdict.Class, + "classified_by": verdict.Source, }, }) @@ -1299,9 +1350,15 @@ func (s *service) ProcessIncomingReply(ctx context.Context, emailAccountID uuid. cat := models.NotifInboundReply title := "New reply from " + sender body := msg.Subject - if intent == models.ReplyIntentOutOfOffice { + if models.IsAutomatedIntent(intent) { + // The "out-of-office detected" preference is what someone mutes to + // stop hearing about auto-responders, so every machine reply goes + // through it rather than only the vacation-worded ones. cat = models.NotifInboundOOO title = "Out-of-office from " + sender + if intent == models.ReplyIntentAutomated { + title = "Automatic reply from " + sender + } // Say what happened to their sequence, not only that mail arrived. if held != nil { body = "Held until " + held.Format("2 Jan") + " · " + msg.Subject diff --git a/internal/app/crm/service.go b/internal/app/crm/service.go index d6c7b1e55..b3cc00e10 100644 --- a/internal/app/crm/service.go +++ b/internal/app/crm/service.go @@ -3,6 +3,7 @@ package crm import ( "context" "errors" + "fmt" "strings" "github.com/google/uuid" @@ -51,6 +52,8 @@ type CRMService interface { TasksSummary(ctx context.Context, orgID uuid.UUID, filters models.SearchTasks) (*models.TasksSummary, *errx.Error) UpdateCRMTask(ctx context.Context, orgID, taskID uuid.UUID, userID *uuid.UUID, data *models.UpdateCRMTask) (*models.CRMTask, *errx.Error) DeleteCRMTask(ctx context.Context, orgID, taskID uuid.UUID) *errx.Error + BulkUpdateCRMTasks(ctx context.Context, orgID uuid.UUID, userID *uuid.UUID, data *models.BulkUpdateTasks) (int64, *errx.Error) + BulkDeleteCRMTasks(ctx context.Context, orgID uuid.UUID, sel models.TaskSelection) (int64, *errx.Error) // CRM Task Types (user-managed) ListTaskTypes(ctx context.Context, orgID uuid.UUID) ([]models.CRMTaskType, *errx.Error) @@ -59,6 +62,10 @@ type CRMService interface { DeleteTaskType(ctx context.Context, orgID, typeID uuid.UUID) *errx.Error } +// maxTaskBulkBatch bounds an explicit id list in one bulk request body. A +// select-all is bounded separately, by models.MaxTaskBulkSelection. +const maxTaskBulkBatch = 1000 + type crmService struct { repo repository.CRMRepository } @@ -405,6 +412,9 @@ func (s *crmService) ListCRMTasks(ctx context.Context, orgID uuid.UUID, contactI } func (s *crmService) SearchTasks(ctx context.Context, orgID uuid.UUID, filters models.SearchTasks, limit, offset int) (*models.TasksSearchResult, *errx.Error) { + if err := filters.Validate(); err != nil { + return nil, errx.NewWithIdentifier(errx.BadRequest, "invalid_filter", err.Error()) + } if limit <= 0 || limit > 200 { limit = 50 } @@ -419,6 +429,9 @@ func (s *crmService) SearchTasks(ctx context.Context, orgID uuid.UUID, filters m } func (s *crmService) TasksSummary(ctx context.Context, orgID uuid.UUID, filters models.SearchTasks) (*models.TasksSummary, *errx.Error) { + if err := filters.Validate(); err != nil { + return nil, errx.NewWithIdentifier(errx.BadRequest, "invalid_filter", err.Error()) + } result, err := s.repo.TasksSummary(ctx, orgID, filters) if err != nil { return nil, toErrx(err) @@ -443,6 +456,86 @@ func (s *crmService) UpdateCRMTask(ctx context.Context, orgID, taskID uuid.UUID, return task, nil } +// checkTaskSelection validates what the request body says before it reaches the +// database. How many rows the selection actually resolves to is checked by the +// mutation itself (see tooLarge), because a count taken here could be stale by +// the time the write runs. +func (s *crmService) checkTaskSelection(sel models.TaskSelection) *errx.Error { + if !sel.All { + if len(sel.Tasks) == 0 { + return errx.New(errx.BadRequest, "no tasks provided") + } + if len(sel.Tasks) > maxTaskBulkBatch { + return errx.NewWithIdentifier(errx.BadRequest, "too_many_tasks", + fmt.Sprintf("too many tasks, maximum is %d per batch", maxTaskBulkBatch)) + } + return nil + } + if sel.Filters == nil { + return errx.New(errx.BadRequest, "a select-all request must carry the filters it applies to") + } + if err := sel.Filters.Validate(); err != nil { + return errx.NewWithIdentifier(errx.BadRequest, "invalid_filter", err.Error()) + } + if len(sel.Exclude) > models.MaxTaskBulkSelection { + return errx.NewWithIdentifier(errx.BadRequest, "too_many_tasks", + fmt.Sprintf("too many exclusions, maximum is %d", models.MaxTaskBulkSelection)) + } + return nil +} + +// tooLarge turns the row count the mutation refused to act on into the error a +// caller sees. The statement wrote nothing, so the action is refused whole +// rather than half applied. The count stops at the cap plus one, which is all +// the mutation needs to look at to know the selection is over it. +func tooLarge(matched int64) *errx.Error { + if matched <= models.MaxTaskBulkSelection { + return nil + } + return errx.NewWithIdentifier(errx.BadRequest, "selection_too_large", + fmt.Sprintf("that selection matches more than %d tasks; narrow it with a filter and try again", models.MaxTaskBulkSelection)) +} + +func (s *crmService) BulkUpdateCRMTasks(ctx context.Context, orgID uuid.UUID, userID *uuid.UUID, data *models.BulkUpdateTasks) (int64, *errx.Error) { + if data == nil { + return 0, errx.New(errx.BadRequest, "a bulk update needs a body") + } + if data.Status == nil && data.Priority == nil { + return 0, errx.New(errx.BadRequest, "a bulk update needs a status or a priority") + } + if data.Status != nil && !models.ValidCRMTaskStatus(*data.Status) { + return 0, errx.New(errx.BadRequest, "invalid task status") + } + if data.Priority != nil && !models.ValidCRMTaskPriority(*data.Priority) { + return 0, errx.New(errx.BadRequest, "invalid task priority") + } + if xerr := s.checkTaskSelection(data.TaskSelection); xerr != nil { + return 0, xerr + } + matched, affected, err := s.repo.BulkUpdateCRMTasks(ctx, orgID, userID, data.TaskSelection, data, models.MaxTaskBulkSelection) + if err != nil { + return 0, toErrx(err) + } + if xerr := tooLarge(matched); xerr != nil { + return 0, xerr + } + return affected, nil +} + +func (s *crmService) BulkDeleteCRMTasks(ctx context.Context, orgID uuid.UUID, sel models.TaskSelection) (int64, *errx.Error) { + if xerr := s.checkTaskSelection(sel); xerr != nil { + return 0, xerr + } + matched, affected, err := s.repo.BulkDeleteCRMTasks(ctx, orgID, sel, models.MaxTaskBulkSelection) + if err != nil { + return 0, toErrx(err) + } + if xerr := tooLarge(matched); xerr != nil { + return 0, xerr + } + return affected, nil +} + func (s *crmService) ListTaskTypes(ctx context.Context, orgID uuid.UUID) ([]models.CRMTaskType, *errx.Error) { types, err := s.repo.ListTaskTypes(ctx, orgID) if err != nil { diff --git a/internal/app/replyclassify/classifier.go b/internal/app/replyclassify/classifier.go index bf71b18c5..e37b858d7 100644 --- a/internal/app/replyclassify/classifier.go +++ b/internal/app/replyclassify/classifier.go @@ -116,6 +116,14 @@ func ClassifyGated(ctx context.Context, in Input, gate ModelGate) Result { return Result{Class: ClassUnknown, Confidence: 0, Source: ""} } +// ClassifyOffline runs only the deterministic, free layers (headers, then +// lexicon). It never calls a model and never touches the network, so it is +// safe on every inbound message rather than only the ones with a campaign +// behind them. An inconclusive verdict comes back as ClassUnknown. +func ClassifyOffline(in Input) Result { + return ClassifyGated(context.Background(), in, func() bool { return false }) +} + // WorthModeling is a cheap content-sanity pre-check for the model layer: a reply // with too little human text to carry sentiment the lexicon already missed is not // worth a paid classification (trivial "ok"/"thanks" acks, empty/whitespace diff --git a/internal/models/advanced_outreach.go b/internal/models/advanced_outreach.go index 1c96084e4..46459c6c9 100644 --- a/internal/models/advanced_outreach.go +++ b/internal/models/advanced_outreach.go @@ -1,6 +1,7 @@ package models import ( + "fmt" "math" "strings" "time" @@ -33,14 +34,18 @@ type ABTestingSettings struct { } type ReplyIntentSettings struct { - Enabled bool `json:"enabled"` - PositiveKeywords []string `json:"positive_keywords"` - NegativeKeywords []string `json:"negative_keywords"` - OutOfOfficeKeywords []string `json:"out_of_office_keywords"` - QuestionKeywords []string `json:"question_keywords"` - AutoCreateCRMTask bool `json:"auto_create_crm_task"` - AutoPauseOnNegative bool `json:"auto_pause_on_negative"` - AutoSuppressOnUnsubWord bool `json:"auto_suppress_on_unsubscribe_keyword"` + Enabled bool `json:"enabled"` + PositiveKeywords []string `json:"positive_keywords"` + NegativeKeywords []string `json:"negative_keywords"` + OutOfOfficeKeywords []string `json:"out_of_office_keywords"` + QuestionKeywords []string `json:"question_keywords"` + AutoCreateCRMTask bool `json:"auto_create_crm_task"` + // CRMTaskIntents narrows the switch above to the intents worth a + // follow-up. Absent means DefaultCRMTaskIntents (human replies only); an + // explicit empty list means none, same as turning the switch off. + CRMTaskIntents []ReplyIntentType `json:"crm_task_intents"` + AutoPauseOnNegative bool `json:"auto_pause_on_negative"` + AutoSuppressOnUnsubWord bool `json:"auto_suppress_on_unsubscribe_keyword"` // HoldOnOutOfOffice parks the contact's next step when an auto-reply says // they are away, and resumes it when they are back. Without it the // follow-up goes out on schedule to an empty desk and the sequence is over @@ -51,6 +56,42 @@ type ReplyIntentSettings struct { OutOfOfficeHoldDays int `json:"out_of_office_hold_days"` } +// DefaultCRMTaskIntents is the task-worthy set a workspace gets when it has +// never chosen one: every human intent, and no automated one. A vacation +// notice or a bounce is not follow-up work, and one week of sending makes +// enough of them to bury the real replies. +func DefaultCRMTaskIntents() []ReplyIntentType { + return []ReplyIntentType{ + ReplyIntentPositive, + ReplyIntentQuestion, + ReplyIntentNeutral, + ReplyIntentNegative, + } +} + +// TaskIntents resolves the configured set, falling back to the default when +// the workspace has never set one. +func (s ReplyIntentSettings) TaskIntents() []ReplyIntentType { + if s.CRMTaskIntents == nil { + return DefaultCRMTaskIntents() + } + return s.CRMTaskIntents +} + +// CreatesTaskFor reports whether a reply classified as intent should open a +// CRM follow-up task. +func (s ReplyIntentSettings) CreatesTaskFor(intent ReplyIntentType) bool { + if !s.AutoCreateCRMTask { + return false + } + for _, want := range s.TaskIntents() { + if want == intent { + return true + } + } + return false +} + // Bounds on the fallback out-of-office hold. A hold of zero days would send // into the away message it was triggered by; one of months would silently // abandon a lead nobody thinks to check on. @@ -112,6 +153,45 @@ func (s *AdvancedOutreachSettings) Normalize() { s.Unsubscribe.Text = clampLine(s.Unsubscribe.Text) s.Unsubscribe.LinkIntro = clampLine(s.Unsubscribe.LinkIntro) s.Unsubscribe.LinkText = clampLine(s.Unsubscribe.LinkText) + s.ReplyIntent.CRMTaskIntents = normalizeIntents(s.ReplyIntent.CRMTaskIntents) +} + +// Validate reports the settings a caller may not store. Normalize handles what +// can be clamped; this covers what can only be refused, so a bad value is a +// 400 rather than a silently different setting. +func (s *AdvancedOutreachSettings) Validate() error { + if s == nil { + return nil + } + for _, intent := range s.ReplyIntent.CRMTaskIntents { + if !ValidReplyIntent(intent) { + return fmt.Errorf("%q is not a reply intent", intent) + } + } + return nil +} + +// normalizeIntents lower-cases, trims and de-duplicates an intent list while +// keeping the caller's order. A nil list stays nil: absent means "the default +// set", which an empty list would not. +func normalizeIntents(in []ReplyIntentType) []ReplyIntentType { + if in == nil { + return nil + } + out := make([]ReplyIntentType, 0, len(in)) + seen := make(map[ReplyIntentType]struct{}, len(in)) + for _, v := range in { + v = ReplyIntentType(strings.ToLower(strings.TrimSpace(string(v)))) + if v == "" { + continue + } + if _, dup := seen[v]; dup { + continue + } + seen[v] = struct{}{} + out = append(out, v) + } + return out } // clampLine trims a one-line copy field and caps it; the email footer is not @@ -391,8 +471,29 @@ const ( ReplyIntentOutOfOffice ReplyIntentType = "out_of_office" ReplyIntentQuestion ReplyIntentType = "question" ReplyIntentNeutral ReplyIntentType = "neutral" + // ReplyIntentAutomated is a machine reply that is not a vacation notice: + // an autoresponder, a ticket acknowledgement, a bounce or a delivery + // report. Recorded from the header layer of the reply classifier, which + // sees markers the keyword lists never could. + ReplyIntentAutomated ReplyIntentType = "automated" ) +// ValidReplyIntent reports whether v is an intent the classifier can record +// and a setting may name. +func ValidReplyIntent(v ReplyIntentType) bool { + switch v { + case ReplyIntentPositive, ReplyIntentNegative, ReplyIntentOutOfOffice, + ReplyIntentQuestion, ReplyIntentNeutral, ReplyIntentAutomated: + return true + } + return false +} + +// IsAutomatedIntent reports whether an intent describes a machine reply. +func IsAutomatedIntent(v ReplyIntentType) bool { + return v == ReplyIntentOutOfOffice || v == ReplyIntentAutomated +} + type ReplyIntentRecord struct { ID uuid.UUID `json:"id"` OrganizationID uuid.UUID `json:"organization_id"` @@ -442,6 +543,7 @@ type DeliverabilityDashboard struct { IntentOOO int `json:"intent_out_of_office"` IntentQuestion int `json:"intent_question"` IntentNeutral int `json:"intent_neutral"` + IntentAutomated int `json:"intent_automated"` // Computed rates (percent, 0-100; 0 when no sends in the window). EmailsSent // is the count of completed campaign sends in the window (rate denominator). diff --git a/internal/models/advanced_outreach_test.go b/internal/models/advanced_outreach_test.go index db20d61d3..2bdcd0c80 100644 --- a/internal/models/advanced_outreach_test.go +++ b/internal/models/advanced_outreach_test.go @@ -31,3 +31,82 @@ func TestNormalizeLeavesTheDefaultsAlone(t *testing.T) { t.Errorf("defaults changed under Normalize: %+v -> %+v", before.Preflight, s.Preflight) } } + +// The bug behind issue #471: every classified reply opened a high-priority +// follow-up, including vacation notices and bounces, and one week of sending +// buried the real replies. A default workspace must open a task for a human +// reply and refuse one for a machine. +func TestDefaultSettingsOpenTasksForHumanRepliesOnly(t *testing.T) { + s := DefaultAdvancedOutreachSettings().ReplyIntent + for _, intent := range []ReplyIntentType{ + ReplyIntentPositive, ReplyIntentQuestion, ReplyIntentNeutral, ReplyIntentNegative, + } { + if !s.CreatesTaskFor(intent) { + t.Errorf("%s should open a follow-up task by default", intent) + } + } + for _, intent := range []ReplyIntentType{ReplyIntentOutOfOffice, ReplyIntentAutomated} { + if s.CreatesTaskFor(intent) { + t.Errorf("%s should not open a follow-up task by default", intent) + } + } +} + +// An unset list means the default set, an empty one means none, and the switch +// still overrides both. All three have to stay distinguishable through a JSON +// round trip, because that is how the setting is stored. +func TestCreatesTaskForHonoursTheConfiguredSet(t *testing.T) { + off := DefaultAdvancedOutreachSettings().ReplyIntent + off.AutoCreateCRMTask = false + if off.CreatesTaskFor(ReplyIntentPositive) { + t.Error("the switch being off must beat any intent list") + } + + none := DefaultAdvancedOutreachSettings().ReplyIntent + none.CRMTaskIntents = []ReplyIntentType{} + for _, intent := range []ReplyIntentType{ReplyIntentPositive, ReplyIntentOutOfOffice} { + if none.CreatesTaskFor(intent) { + t.Errorf("an empty list must open nothing, opened for %s", intent) + } + } + + ooo := DefaultAdvancedOutreachSettings().ReplyIntent + ooo.CRMTaskIntents = []ReplyIntentType{ReplyIntentOutOfOffice} + if !ooo.CreatesTaskFor(ReplyIntentOutOfOffice) { + t.Error("a workspace that asks for out-of-office tasks must get them") + } + if ooo.CreatesTaskFor(ReplyIntentPositive) { + t.Error("an explicit list must not fall back to the default set") + } +} + +func TestValidateRefusesAnUnknownReplyIntent(t *testing.T) { + s := DefaultAdvancedOutreachSettings() + s.ReplyIntent.CRMTaskIntents = []ReplyIntentType{ReplyIntentPositive, "interested?"} + s.Normalize() + if err := s.Validate(); err == nil { + t.Fatal("an unknown intent must be refused rather than silently dropped") + } + + s.ReplyIntent.CRMTaskIntents = []ReplyIntentType{" Positive ", ReplyIntentPositive, ReplyIntentNeutral} + s.Normalize() + if err := s.Validate(); err != nil { + t.Fatalf("normalized values must validate: %v", err) + } + if got := s.ReplyIntent.CRMTaskIntents; len(got) != 2 || got[0] != ReplyIntentPositive || got[1] != ReplyIntentNeutral { + t.Errorf("Normalize did not trim, lower-case and de-duplicate: %v", got) + } +} + +// Absent must survive a round trip as absent: it is what makes an existing +// workspace, stored before the setting existed, pick up the new default. +func TestNilIntentListStaysNilThroughNormalize(t *testing.T) { + s := DefaultAdvancedOutreachSettings() + if s.ReplyIntent.CRMTaskIntents != nil { + t.Fatal("the default settings must leave the list unset") + } + s.Normalize() + if s.ReplyIntent.CRMTaskIntents != nil { + t.Error("Normalize turned an unset list into an empty one, which means none") + } +} diff --git a/internal/models/crm.go b/internal/models/crm.go index f42e044d2..07bfa6fae 100644 --- a/internal/models/crm.go +++ b/internal/models/crm.go @@ -1,6 +1,8 @@ package models import ( + "fmt" + "strings" "time" "github.com/google/uuid" @@ -381,6 +383,32 @@ type SearchTasks struct { Reverse bool `json:"reverse"` // true = ASC, false = DESC (default) } +// Validate refuses the id-valued facets before they reach SQL. They are +// compared against uuid columns, so a malformed one is an error from the +// driver mid-query, which reads as a 500 on what is a bad request. +func (f SearchTasks) Validate() error { + ids := map[string][]string{ + "assigned_to": f.AssignedTo, + "contact_id": derefOne(f.ContactID), + "deal_id": derefOne(f.DealID), + } + for field, vals := range ids { + for _, v := range vals { + if _, err := uuid.Parse(strings.TrimSpace(v)); err != nil { + return fmt.Errorf("%s: %q is not an id", field, v) + } + } + } + return nil +} + +func derefOne(v *string) []string { + if v == nil || strings.TrimSpace(*v) == "" { + return nil + } + return []string{*v} +} + // TasksSearchResult is the result of POST /crm/tasks/search. Offset pagination // under the hood (the sortable nullable due_date rules out a keyset cursor), but // it exposes the standard {total, next_cursor, has_more} envelope with an OPAQUE @@ -390,6 +418,58 @@ type TasksSearchResult struct { Pagination Pagination `json:"pagination"` } +// MaxTaskBulkSelection bounds how many tasks one "select all matching" bulk +// action may touch. Past it the action is refused and the user narrows the +// filter, so a stray click can never walk a whole workspace's task list. +const MaxTaskBulkSelection = 50000 + +// TaskSelection names the tasks a bulk action applies to. Either an explicit +// id list (Tasks), or every task matching a search (All + Filters) minus the +// rows unticked afterwards (Exclude), which is what the Tasks page's "select +// all matching" sends. A selection that names both prefers the filter. +type TaskSelection struct { + Tasks []string `json:"tasks"` + // All switches the selection from the id list to Filters. + All bool `json:"all,omitempty"` + // Filters is the same search body /crm/tasks/search takes, so the set + // resolved here is exactly the set the list was showing. + Filters *SearchTasks `json:"filters,omitempty"` + // Exclude drops ids from the resolved set: the rows unticked after a + // select-all. Ignored unless All is set. + Exclude []string `json:"exclude,omitempty"` +} + +// BulkUpdateTasks is the body of PATCH /crm/tasks: a selection plus the fields +// to write on every task in it. At least one field is required. +type BulkUpdateTasks struct { + TaskSelection + Status *string `json:"status,omitempty"` + Priority *string `json:"priority,omitempty"` +} + +// BulkTasksResponse reports how many tasks a bulk action touched. +type BulkTasksResponse struct { + Affected int64 `json:"affected"` +} + +// ValidCRMTaskStatus reports whether v is a status a task may hold. +func ValidCRMTaskStatus(v string) bool { + switch CRMTaskStatus(v) { + case CRMTaskStatusPending, CRMTaskStatusInProgress, CRMTaskStatusCompleted, CRMTaskStatusCancelled: + return true + } + return false +} + +// ValidCRMTaskPriority reports whether v is a priority a task may hold. +func ValidCRMTaskPriority(v string) bool { + switch CRMTaskPriority(v) { + case CRMTaskPriorityLow, CRMTaskPriorityMedium, CRMTaskPriorityHigh, CRMTaskPriorityUrgent: + return true + } + return false +} + // TasksSummary is the server-side aggregate over the SAME filter body as a // search, so every header total is a true COUNT over the whole matching set // rather than a client reduce over a truncated page. diff --git a/internal/models/crm_task_search_test.go b/internal/models/crm_task_search_test.go new file mode 100644 index 000000000..18316f8fe --- /dev/null +++ b/internal/models/crm_task_search_test.go @@ -0,0 +1,47 @@ +package models + +import "testing" + +func strptr(v string) *string { return &v } + +// The id facets are compared against uuid columns, so a malformed one has to be +// refused before it becomes an error from the driver halfway through a query. +func TestSearchTasksValidateRefusesIdsThatAreNotIds(t *testing.T) { + good := "8f14e45f-ceea-467a-9f2a-4d29f9f0f5f3" + for _, tc := range []struct { + name string + in SearchTasks + ok bool + }{ + {name: "empty body", in: SearchTasks{}, ok: true}, + {name: "real ids", in: SearchTasks{ + AssignedTo: []string{good}, + ContactID: strptr(good), + DealID: strptr(good), + }, ok: true}, + {name: "absent id is not a filter", in: SearchTasks{ContactID: nil}, ok: true}, + {name: "blank id is not a filter", in: SearchTasks{ContactID: strptr(" ")}, ok: true}, + {name: "assignee is a name", in: SearchTasks{AssignedTo: []string{"me"}}, ok: false}, + {name: "one bad assignee among good ones", in: SearchTasks{ + AssignedTo: []string{good, "nonsense"}, + }, ok: false}, + {name: "blank assignee", in: SearchTasks{AssignedTo: []string{""}}, ok: false}, + {name: "contact is not an id", in: SearchTasks{ContactID: strptr("42")}, ok: false}, + {name: "deal is not an id", in: SearchTasks{DealID: strptr("deal-1")}, ok: false}, + // Only the uuid columns are checked: these are text and a filter that + // matches nothing is a legitimate answer, not a bad request. + {name: "status and type are free text", in: SearchTasks{ + Statuses: []string{"nonsense"}, Types: []string{"nonsense"}, + }, ok: true}, + } { + t.Run(tc.name, func(t *testing.T) { + err := tc.in.Validate() + if tc.ok && err != nil { + t.Fatalf("refused a valid filter: %v", err) + } + if !tc.ok && err == nil { + t.Fatal("accepted a filter Postgres would reject") + } + }) + } +} diff --git a/internal/repository/crm_task_bulk_live_test.go b/internal/repository/crm_task_bulk_live_test.go new file mode 100644 index 000000000..1d091d51a --- /dev/null +++ b/internal/repository/crm_task_bulk_live_test.go @@ -0,0 +1,486 @@ +package repository + +import ( + "context" + "os" + "testing" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5/pgxpool" + + "github.com/warmbly/warmbly/internal/models" +) + +// The bulk task actions run their WHERE inside a DELETE and an UPDATE, reusing +// the aliased clause the search builds. Nothing but a real database proves the +// alias is accepted there and the $N numbering survives the SET args being +// appended after the filter's, so this runs against one. +// +// WARMBLY_TEST_DB=postgres://warmbly:warmbly@localhost:15432/warmbly_dev?sslmode=disable \ +// go test ./internal/repository/ -run Live -v + +type bulkTaskFixture struct { + pool *pgxpool.Pool + repo CRMRepository + org uuid.UUID + user uuid.UUID + tasks []uuid.UUID +} + +func newBulkTaskFixture(t *testing.T) *bulkTaskFixture { + t.Helper() + dsn := os.Getenv("WARMBLY_TEST_DB") + if dsn == "" { + t.Skip("WARMBLY_TEST_DB not set") + } + ctx := context.Background() + pool, err := pgxpool.New(ctx, dsn) + if err != nil { + t.Fatalf("connect: %v", err) + } + t.Cleanup(pool.Close) + + f := &bulkTaskFixture{pool: pool, repo: NewCRMRepository(pool), org: uuid.New(), user: uuid.New()} + exec := func(sql string, args ...any) { + t.Helper() + if _, err := pool.Exec(ctx, sql, args...); err != nil { + t.Fatalf("fixture: %v", err) + } + } + exec(`INSERT INTO users (id, email, first_name, last_name) VALUES ($1, $2, 'Bulk', 'Test')`, + f.user, "bulk-"+f.user.String()[:8]+"@test.local") + exec(`INSERT INTO organizations (id, name, slug, owner_user_id) VALUES ($1, 'Bulk Test', $2, $3)`, + f.org, "bulk-"+f.org.String()[:8], f.user) + t.Cleanup(func() { + _, _ = pool.Exec(context.Background(), `DELETE FROM crm_tasks WHERE organization_id = $1`, f.org) + _, _ = pool.Exec(context.Background(), `DELETE FROM organizations WHERE id = $1`, f.org) + _, _ = pool.Exec(context.Background(), `DELETE FROM users WHERE id = $1`, f.user) + }) + return f +} + +func (f *bulkTaskFixture) contact(t *testing.T) uuid.UUID { + t.Helper() + id := uuid.New() + if _, err := f.pool.Exec(context.Background(), + `INSERT INTO contacts (id, user_id, organization_id, email, first_name, last_name, company, phone, custom_fields, subscribed, updated_at, created_at) + VALUES ($1, $2, $3, $4, 'Bulk', 'Live', '', '', '{}'::jsonb, true, NOW(), NOW())`, + id, f.user, f.org, "bulk-"+id.String()[:8]+"@test.local"); err != nil { + t.Fatalf("fixture contact: %v", err) + } + t.Cleanup(func() { + _, _ = f.pool.Exec(context.Background(), `DELETE FROM contact_activities WHERE contact_id = $1`, id) + _, _ = f.pool.Exec(context.Background(), `DELETE FROM contacts WHERE id = $1`, id) + }) + return id +} + +func (f *bulkTaskFixture) activities(t *testing.T, contactID uuid.UUID) int { + t.Helper() + var n int + if err := f.pool.QueryRow(context.Background(), + `SELECT COUNT(*) FROM contact_activities WHERE contact_id = $1 AND activity_type = $2`, + contactID, models.ActivityTaskCompleted).Scan(&n); err != nil { + t.Fatalf("count activities: %v", err) + } + return n +} + +func (f *bulkTaskFixture) task(t *testing.T, title, priority string) uuid.UUID { + t.Helper() + return f.contactTask(t, title, priority, nil) +} + +func (f *bulkTaskFixture) contactTask(t *testing.T, title, priority string, contactID *uuid.UUID) uuid.UUID { + t.Helper() + created, err := f.repo.CreateCRMTask(context.Background(), f.org, f.user, &models.CreateCRMTask{ + ContactID: contactID, + Title: title, + Priority: priority, + }) + if err != nil { + t.Fatalf("create task: %v", err) + } + f.tasks = append(f.tasks, created.ID) + return created.ID +} + +func TestLiveBulkUpdateTasksByIDList(t *testing.T) { + f := newBulkTaskFixture(t) + ctx := context.Background() + a := f.task(t, "Follow up: reply from a@b.com", "high") + b := f.task(t, "Follow up: reply from c@d.com", "high") + untouched := f.task(t, "Leave me alone", "low") + + status := string(models.CRMTaskStatusCompleted) + sel := models.TaskSelection{Tasks: []string{a.String(), b.String()}} + _, n, err := f.repo.BulkUpdateCRMTasks(ctx, f.org, &f.user, sel, &models.BulkUpdateTasks{ + TaskSelection: sel, + Status: &status, + }, models.MaxTaskBulkSelection) + if err != nil { + t.Fatalf("bulk update: %v", err) + } + if n != 2 { + t.Fatalf("updated %d, want 2", n) + } + for _, id := range []uuid.UUID{a, b} { + got, err := f.repo.GetCRMTask(ctx, f.org, id) + if err != nil { + t.Fatalf("read back: %v", err) + } + if got.Status != models.CRMTaskStatusCompleted { + t.Errorf("status %q, want completed", got.Status) + } + if got.CompletedAt == nil { + t.Error("completing a task must stamp completed_at") + } + } + left, err := f.repo.GetCRMTask(ctx, f.org, untouched) + if err != nil { + t.Fatalf("read back: %v", err) + } + if left.Status == models.CRMTaskStatusCompleted { + t.Error("a task outside the selection was written") + } +} + +// The select-all form is the one that reuses the search's WHERE, exclusions +// and all, so it is the one that can go wrong silently. +func TestLiveBulkDeleteTasksByFilter(t *testing.T) { + f := newBulkTaskFixture(t) + ctx := context.Background() + f.task(t, "Follow up: out-of-office reply from a@b.com", "high") + f.task(t, "Follow up: out-of-office reply from c@d.com", "high") + kept := f.task(t, "Follow up: out-of-office reply from e@f.com", "high") + other := f.task(t, "Call the customer", "medium") + + sel := models.TaskSelection{ + All: true, + Filters: &models.SearchTasks{Query: "out-of-office", ContactID: nil}, + Exclude: []string{kept.String()}, + } + // The filter is title-only, so scope it to this fixture's org through the + // repo call itself; a shared dev database holds other organizations' rows. + _, n, err := f.repo.BulkDeleteCRMTasks(ctx, f.org, sel, models.MaxTaskBulkSelection) + if err != nil { + t.Fatalf("bulk delete: %v", err) + } + if n != 2 { + t.Fatalf("deleted %d, want 2", n) + } + if _, err := f.repo.GetCRMTask(ctx, f.org, kept); err != nil { + t.Errorf("the excluded task was deleted: %v", err) + } + if _, err := f.repo.GetCRMTask(ctx, f.org, other); err != nil { + t.Errorf("a task the filter never matched was deleted: %v", err) + } +} + +// A filter with a team facet binds a uuid[] once and reuses its $N; the SET +// args are numbered after it. This is the shape that breaks when a clause is +// appended without counting. +func TestLiveBulkUpdateTasksWithEveryFilterBound(t *testing.T) { + f := newBulkTaskFixture(t) + ctx := context.Background() + id := f.task(t, "Follow up: reply from a@b.com", "low") + + priority := string(models.CRMTaskPriorityUrgent) + status := string(models.CRMTaskStatusInProgress) + sel := models.TaskSelection{ + All: true, + Filters: &models.SearchTasks{ + Query: "Follow up", + Statuses: []string{"pending"}, + Priorities: []string{"low"}, + TeamIDs: []uuid.UUID{uuid.New()}, + }, + } + // The team facet matches nothing, so this must write nothing rather than + // error or fall through to the whole org. + _, n, err := f.repo.BulkUpdateCRMTasks(ctx, f.org, &f.user, sel, &models.BulkUpdateTasks{ + TaskSelection: sel, Status: &status, Priority: &priority, + }, models.MaxTaskBulkSelection) + if err != nil { + t.Fatalf("bulk update: %v", err) + } + if n != 0 { + t.Fatalf("updated %d rows through a facet that matches nothing", n) + } + + sel.Filters.TeamIDs = nil + _, n, err = f.repo.BulkUpdateCRMTasks(ctx, f.org, &f.user, sel, &models.BulkUpdateTasks{ + TaskSelection: sel, Status: &status, Priority: &priority, + }, models.MaxTaskBulkSelection) + if err != nil { + t.Fatalf("bulk update: %v", err) + } + if n != 1 { + t.Fatalf("updated %d, want 1", n) + } + got, err := f.repo.GetCRMTask(ctx, f.org, id) + if err != nil { + t.Fatalf("read back: %v", err) + } + if got.Status != models.CRMTaskStatusInProgress || got.Priority != models.CRMTaskPriorityUrgent { + t.Errorf("wrote %q/%q, want in_progress/urgent", got.Status, got.Priority) + } +} + +// Completing in bulk has to leave the same trail as completing one at a time, +// and exactly once: the activity is written for the tasks THIS call completed, +// not for the ones already done that the filter happened to cover. +func TestLiveBulkCompleteRecordsTheContactActivityOnce(t *testing.T) { + f := newBulkTaskFixture(t) + ctx := context.Background() + contact := f.contact(t) + a := f.contactTask(t, "Follow up: reply from a@b.com", "high", &contact) + b := f.contactTask(t, "Follow up: reply from c@d.com", "high", &contact) + f.task(t, "Follow up: reply with no contact", "high") + + status := string(models.CRMTaskStatusCompleted) + sel := models.TaskSelection{Tasks: []string{a.String(), b.String()}} + if _, _, err := f.repo.BulkUpdateCRMTasks(ctx, f.org, &f.user, sel, &models.BulkUpdateTasks{ + TaskSelection: sel, Status: &status, + }, models.MaxTaskBulkSelection); err != nil { + t.Fatalf("bulk update: %v", err) + } + if got := f.activities(t, contact); got != 2 { + t.Fatalf("recorded %d completions, want 2", got) + } + + // Running it again completes nothing, so it must record nothing. + if _, _, err := f.repo.BulkUpdateCRMTasks(ctx, f.org, &f.user, sel, &models.BulkUpdateTasks{ + TaskSelection: sel, Status: &status, + }, models.MaxTaskBulkSelection); err != nil { + t.Fatalf("bulk update (repeat): %v", err) + } + if got := f.activities(t, contact); got != 2 { + t.Errorf("recorded %d completions after a repeat, want 2", got) + } +} + +// A selection routinely covers tasks that are already done, so repeating a +// bulk complete must leave their completion time alone. Stamping NOW() on +// every row in the selection rewrote history every time the action ran. +func TestLiveBulkCompleteStampsOnlyTheTransition(t *testing.T) { + f := newBulkTaskFixture(t) + ctx := context.Background() + done := f.task(t, "Already finished", "low") + fresh := f.task(t, "Still pending", "low") + + status := string(models.CRMTaskStatusCompleted) + first := models.TaskSelection{Tasks: []string{done.String()}} + if _, _, err := f.repo.BulkUpdateCRMTasks(ctx, f.org, &f.user, first, &models.BulkUpdateTasks{ + TaskSelection: first, Status: &status, + }, models.MaxTaskBulkSelection); err != nil { + t.Fatalf("first complete: %v", err) + } + was, err := f.repo.GetCRMTask(ctx, f.org, done) + if err != nil { + t.Fatalf("read back: %v", err) + } + if was.CompletedAt == nil { + t.Fatal("completing a task must stamp completed_at") + } + + // Both rows in one call: one already completed, one not. + both := models.TaskSelection{Tasks: []string{done.String(), fresh.String()}} + if _, _, err := f.repo.BulkUpdateCRMTasks(ctx, f.org, &f.user, both, &models.BulkUpdateTasks{ + TaskSelection: both, Status: &status, + }, models.MaxTaskBulkSelection); err != nil { + t.Fatalf("second complete: %v", err) + } + + again, err := f.repo.GetCRMTask(ctx, f.org, done) + if err != nil { + t.Fatalf("read back: %v", err) + } + if again.CompletedAt == nil || !again.CompletedAt.Equal(*was.CompletedAt) { + t.Errorf("completed_at moved from %v to %v on a task that was already done", was.CompletedAt, again.CompletedAt) + } + now, err := f.repo.GetCRMTask(ctx, f.org, fresh) + if err != nil { + t.Fatalf("read back: %v", err) + } + if now.CompletedAt == nil { + t.Error("the task that transitioned in the same call was not stamped") + } +} + +// Reopening a task and completing it again is a real transition, so it takes a +// fresh stamp rather than keeping the old one. +func TestLiveReopenedTaskIsStampedAgain(t *testing.T) { + f := newBulkTaskFixture(t) + ctx := context.Background() + id := f.task(t, "Round trip", "low") + + done := string(models.CRMTaskStatusCompleted) + pending := string(models.CRMTaskStatusPending) + sel := models.TaskSelection{Tasks: []string{id.String()}} + apply := func(status string) { + t.Helper() + if _, _, err := f.repo.BulkUpdateCRMTasks(ctx, f.org, &f.user, sel, &models.BulkUpdateTasks{ + TaskSelection: sel, Status: &status, + }, models.MaxTaskBulkSelection); err != nil { + t.Fatalf("apply %s: %v", status, err) + } + } + apply(done) + first, err := f.repo.GetCRMTask(ctx, f.org, id) + if err != nil { + t.Fatalf("read back: %v", err) + } + apply(pending) + apply(done) + second, err := f.repo.GetCRMTask(ctx, f.org, id) + if err != nil { + t.Fatalf("read back: %v", err) + } + if first.CompletedAt == nil || second.CompletedAt == nil { + t.Fatal("both completions must stamp") + } + if !second.CompletedAt.After(*first.CompletedAt) { + t.Errorf("a reopened task kept its old completion time: %v then %v", first.CompletedAt, second.CompletedAt) + } +} + +// The cap is a promise that an over-large selection is refused WHOLE. Checking +// it with a COUNT before the write left a gap a concurrent insert could widen, +// so the statement itself carries the bound: over it, the row count comes back +// and nothing is written. +func TestLiveBulkOverTheCapWritesNothing(t *testing.T) { + f := newBulkTaskFixture(t) + ctx := context.Background() + ids := []string{} + for i := 0; i < 3; i++ { + ids = append(ids, f.task(t, "Capped", "low").String()) + } + + status := string(models.CRMTaskStatusCompleted) + sel := models.TaskSelection{Tasks: ids} + matched, affected, err := f.repo.BulkUpdateCRMTasks(ctx, f.org, &f.user, sel, &models.BulkUpdateTasks{ + TaskSelection: sel, Status: &status, + }, 2) + if err != nil { + t.Fatalf("bulk update: %v", err) + } + if matched != 3 || affected != 0 { + t.Fatalf("matched %d / wrote %d, want 3 matched and nothing written", matched, affected) + } + for _, raw := range ids { + id, _ := uuid.Parse(raw) + got, err := f.repo.GetCRMTask(ctx, f.org, id) + if err != nil { + t.Fatalf("read back: %v", err) + } + if got.Status == models.CRMTaskStatusCompleted { + t.Error("a task was written by a refused bulk update") + } + } + + matched, affected, err = f.repo.BulkDeleteCRMTasks(ctx, f.org, sel, 2) + if err != nil { + t.Fatalf("bulk delete: %v", err) + } + if matched != 3 || affected != 0 { + t.Fatalf("matched %d / deleted %d, want 3 matched and nothing deleted", matched, affected) + } + for _, raw := range ids { + id, _ := uuid.Parse(raw) + if _, err := f.repo.GetCRMTask(ctx, f.org, id); err != nil { + t.Errorf("a task was deleted by a refused bulk delete: %v", err) + } + } + + // Exactly at the cap it goes through, so the bound is <= and not <. + _, affected, err = f.repo.BulkDeleteCRMTasks(ctx, f.org, sel, 3) + if err != nil { + t.Fatalf("bulk delete at the cap: %v", err) + } + if affected != 3 { + t.Errorf("deleted %d at the cap, want 3", affected) + } +} + +// The refusal is decided from cap+1 candidates, never from the whole matching +// set: a broad filter on a big workspace must not lock every row it matches +// only to write nothing. +func TestLiveBulkOverTheCapLocksNoMoreThanItNeeds(t *testing.T) { + f := newBulkTaskFixture(t) + ctx := context.Background() + ids := []string{} + for i := 0; i < 5; i++ { + ids = append(ids, f.task(t, "Bounded", "low").String()) + } + + status := string(models.CRMTaskStatusCompleted) + sel := models.TaskSelection{Tasks: ids} + matched, affected, err := f.repo.BulkUpdateCRMTasks(ctx, f.org, &f.user, sel, &models.BulkUpdateTasks{ + TaskSelection: sel, Status: &status, + }, 2) + if err != nil { + t.Fatalf("bulk update: %v", err) + } + if matched != 3 || affected != 0 { + t.Fatalf("matched %d / wrote %d, want 3 (the cap plus one) matched and nothing written", matched, affected) + } + + matched, affected, err = f.repo.BulkDeleteCRMTasks(ctx, f.org, sel, 2) + if err != nil { + t.Fatalf("bulk delete: %v", err) + } + if matched != 3 || affected != 0 { + t.Fatalf("matched %d / deleted %d, want 3 (the cap plus one) matched and nothing deleted", matched, affected) + } +} + +// completed_at is when the task was finished. Moving a task back off completed +// leaves it with none, rather than showing a pending task as finished days ago, +// and the bulk path and the single-task path agree about that. +func TestLiveMovingOffCompletedClearsTheStamp(t *testing.T) { + f := newBulkTaskFixture(t) + ctx := context.Background() + id := f.task(t, "Reopened", "low") + + done := string(models.CRMTaskStatusCompleted) + pending := string(models.CRMTaskStatusPending) + sel := models.TaskSelection{Tasks: []string{id.String()}} + if _, _, err := f.repo.BulkUpdateCRMTasks(ctx, f.org, &f.user, sel, &models.BulkUpdateTasks{ + TaskSelection: sel, Status: &done, + }, 10); err != nil { + t.Fatalf("bulk complete: %v", err) + } + got, err := f.repo.GetCRMTask(ctx, f.org, id) + if err != nil { + t.Fatalf("read back: %v", err) + } + if got.CompletedAt == nil { + t.Fatal("completing left no completion time") + } + + if _, _, err := f.repo.BulkUpdateCRMTasks(ctx, f.org, &f.user, sel, &models.BulkUpdateTasks{ + TaskSelection: sel, Status: &pending, + }, 10); err != nil { + t.Fatalf("bulk reopen: %v", err) + } + got, err = f.repo.GetCRMTask(ctx, f.org, id) + if err != nil { + t.Fatalf("read back: %v", err) + } + if got.CompletedAt != nil { + t.Errorf("a pending task kept completed_at = %v", got.CompletedAt) + } + + // The single-task update answers the same way. + if _, err := f.repo.UpdateCRMTask(ctx, f.org, id, &models.UpdateCRMTask{Status: &done}); err != nil { + t.Fatalf("update to completed: %v", err) + } + reopened, err := f.repo.UpdateCRMTask(ctx, f.org, id, &models.UpdateCRMTask{Status: &pending}) + if err != nil { + t.Fatalf("update to pending: %v", err) + } + if reopened.CompletedAt != nil { + t.Errorf("single-task update left completed_at = %v on a pending task", reopened.CompletedAt) + } +} diff --git a/internal/repository/pg_advanced_outreach.go b/internal/repository/pg_advanced_outreach.go index f73c24146..c9768df68 100644 --- a/internal/repository/pg_advanced_outreach.go +++ b/internal/repository/pg_advanced_outreach.go @@ -642,7 +642,8 @@ func (r *advancedOutreachRepository) GetDeliverabilityDashboard(ctx context.Cont COUNT(*) FILTER (WHERE intent = 'negative') AS negative, COUNT(*) FILTER (WHERE intent = 'out_of_office') AS ooo, COUNT(*) FILTER (WHERE intent = 'question') AS question, - COUNT(*) FILTER (WHERE intent = 'neutral') AS neutral + COUNT(*) FILTER (WHERE intent = 'neutral') AS neutral, + COUNT(*) FILTER (WHERE intent = 'automated') AS automated FROM reply_intents WHERE organization_id = $1 AND created_at >= $2 @@ -654,6 +655,7 @@ func (r *advancedOutreachRepository) GetDeliverabilityDashboard(ctx context.Cont &out.IntentOOO, &out.IntentQuestion, &out.IntentNeutral, + &out.IntentAutomated, ); err != nil { return nil, err } diff --git a/internal/repository/pg_crm.go b/internal/repository/pg_crm.go index 94bde5f0b..67b7505d3 100644 --- a/internal/repository/pg_crm.go +++ b/internal/repository/pg_crm.go @@ -56,6 +56,8 @@ type CRMRepository interface { TasksSummary(ctx context.Context, orgID uuid.UUID, filters models.SearchTasks) (*models.TasksSummary, error) UpdateCRMTask(ctx context.Context, orgID, taskID uuid.UUID, data *models.UpdateCRMTask) (*models.CRMTask, error) DeleteCRMTask(ctx context.Context, orgID, taskID uuid.UUID) error + BulkDeleteCRMTasks(ctx context.Context, orgID uuid.UUID, sel models.TaskSelection, cap int) (matched, affected int64, err error) + BulkUpdateCRMTasks(ctx context.Context, orgID uuid.UUID, userID *uuid.UUID, sel models.TaskSelection, data *models.BulkUpdateTasks, cap int) (matched, affected int64, err error) // CRM Task Types (user-managed) ListTaskTypes(ctx context.Context, orgID uuid.UUID) ([]models.CRMTaskType, error) @@ -1243,12 +1245,14 @@ func taskSearchWhere(orgID uuid.UUID, f models.SearchTasks) ([]string, []any) { pos++ } - if f.ContactID != nil { + // Empty means "not filtering", not "the task whose contact is the empty + // string": the column is a uuid and Postgres refuses the comparison. + if f.ContactID != nil && strings.TrimSpace(*f.ContactID) != "" { clauses = append(clauses, fmt.Sprintf("t.contact_id = $%d", pos)) args = append(args, *f.ContactID) pos++ } - if f.DealID != nil { + if f.DealID != nil && strings.TrimSpace(*f.DealID) != "" { clauses = append(clauses, fmt.Sprintf("t.deal_id = $%d", pos)) args = append(args, *f.DealID) pos++ @@ -1387,6 +1391,172 @@ func (r *crmRepository) TasksSummary(ctx context.Context, orgID uuid.UUID, filte return &s, nil } +// taskSelectionWhere builds the WHERE clause naming the tasks a bulk action +// applies to, aliased `t` like taskSearchWhere. An id list becomes a plain +// ANY(); a select-all reuses the search's own WHERE so the set acted on is +// exactly the set the list was showing, minus the rows unticked afterwards. +func taskSelectionWhere(orgID uuid.UUID, sel models.TaskSelection) (string, []any, error) { + if !sel.All { + ids, err := parseUUIDs(sel.Tasks) + if err != nil { + return "", nil, err + } + return "t.organization_id = $1 AND t.id = ANY($2)", []any{orgID, ids}, nil + } + if sel.Filters == nil { + return "", nil, errx.New(errx.BadRequest, "a select-all request must carry the filters it applies to") + } + clauses, args := taskSearchWhere(orgID, *sel.Filters) + if len(sel.Exclude) > 0 { + excluded, err := parseUUIDs(sel.Exclude) + if err != nil { + return "", nil, err + } + clauses = append(clauses, fmt.Sprintf("t.id <> ALL($%d)", len(args)+1)) + args = append(args, excluded) + } + return strings.Join(clauses, " AND "), args, nil +} + +func parseUUIDs(raw []string) ([]uuid.UUID, error) { + out := make([]uuid.UUID, 0, len(raw)) + for _, v := range raw { + id, err := uuid.Parse(strings.TrimSpace(v)) + if err != nil { + return nil, errx.ErrUuid + } + out = append(out, id) + } + return out, nil +} + +// BulkDeleteCRMTasks deletes every task in the selection, returning how many +// the selection matched and how many were deleted. The cap is enforced inside +// the statement, so an over-cap selection deletes nothing. +// +// The candidate set stops at cap+1 rows: one more than the cap is all it takes +// to know the selection is over it, and a broad filter would otherwise lock +// every matching row in the workspace only to refuse the delete. +func (r *crmRepository) BulkDeleteCRMTasks(ctx context.Context, orgID uuid.UUID, sel models.TaskSelection, cap int) (matched, affected int64, err error) { + where, args, err := taskSelectionWhere(orgID, sel) + if err != nil { + return 0, 0, err + } + capPos := len(args) + 1 + args = append(args, cap, cap+1) + query := fmt.Sprintf(` + WITH candidate AS ( + SELECT t.id FROM crm_tasks t WHERE %s FOR UPDATE LIMIT $%d + ), bounded AS ( + SELECT c.id, (SELECT COUNT(*) FROM candidate) AS matched FROM candidate c + ), del AS ( + DELETE FROM crm_tasks t + USING bounded b + WHERE t.id = b.id AND b.matched <= $%d + RETURNING t.id + ) + SELECT (SELECT COUNT(*) FROM candidate), (SELECT COUNT(*) FROM del) + `, where, capPos+1, capPos) + + if err := r.db.QueryRow(ctx, query, args...).Scan(&matched, &affected); err != nil { + return 0, 0, err + } + return matched, affected, nil +} + +// BulkUpdateCRMTasks writes the given fields onto every task in the selection, +// returning how many the selection matched and how many were written. +// +// One statement, not a read followed by a write: it locks the selected rows, +// counts them, updates them, and records the completion activity for exactly +// the rows this statement moved into "completed". Reading the candidates +// separately let two concurrent bulk completes both see the same pending row +// and both log it, left the stamp below keyed on a status that could change +// underneath it, and let a selection grow past the cap between the count and +// the write. Over the cap, matched comes back and nothing is written. +func (r *crmRepository) BulkUpdateCRMTasks(ctx context.Context, orgID uuid.UUID, userID *uuid.UUID, sel models.TaskSelection, data *models.BulkUpdateTasks, cap int) (matched, affected int64, err error) { + where, args, err := taskSelectionWhere(orgID, sel) + if err != nil { + return 0, 0, err + } + + completing := data.Status != nil && *data.Status == string(models.CRMTaskStatusCompleted) + setClauses := []string{} + pos := len(args) + 1 + if data.Status != nil { + setClauses = append(setClauses, fmt.Sprintf("status = $%d", pos)) + args = append(args, *data.Status) + pos++ + if completing { + // Only the transition stamps the time. A selection routinely covers + // rows that are already done, and repeating the action must not + // rewrite when they were finished. + setClauses = append(setClauses, + "completed_at = CASE WHEN b.old_status <> 'completed' THEN NOW() ELSE b.old_completed_at END") + } else { + // Off completed there is no completion time, same as the + // single-task update. + setClauses = append(setClauses, "completed_at = NULL") + } + } + if data.Priority != nil { + setClauses = append(setClauses, fmt.Sprintf("priority = $%d", pos)) + args = append(args, *data.Priority) + pos++ + } + if len(setClauses) == 0 { + return 0, 0, errx.ErrNotEnough + } + setClauses = append(setClauses, "updated_at = NOW()") + + // A data-modifying CTE always runs to completion whether or not the outer + // query reads it, so the activity insert needs no second round trip. + activity := "" + if completing { + activity = fmt.Sprintf(`, act AS ( + INSERT INTO contact_activities (contact_id, organization_id, user_id, activity_type, metadata) + SELECT u.contact_id, u.organization_id, $%d, $%d, + jsonb_build_object('task_id', u.id::text, 'task_title', u.title) + FROM upd u + WHERE u.contact_id IS NOT NULL AND u.old_status <> 'completed' + )`, pos, pos+1) + args = append(args, userID, models.ActivityTaskCompleted) + } + + // The cap is enforced HERE rather than by a COUNT beforehand: between a + // separate count and this write, a concurrent action can add matching rows + // and carry the selection past the limit. Joining on `matched <= cap` makes + // the refusal part of the same statement, so an over-cap selection writes + // nothing at all. FOR UPDATE cannot sit beside a window function, hence the + // second CTE for the count. Stopping the candidate set at cap+1 keeps the + // lock footprint bounded: over the cap is over the cap, and the rows past + // it are never written. + capPos := len(args) + 1 + args = append(args, cap, cap+1) + query := fmt.Sprintf(` + WITH candidate AS ( + SELECT t.id, t.status AS old_status, t.completed_at AS old_completed_at + FROM crm_tasks t + WHERE %s + FOR UPDATE + LIMIT $%d + ), bounded AS ( + SELECT c.*, (SELECT COUNT(*) FROM candidate) AS matched FROM candidate c + ), upd AS ( + UPDATE crm_tasks t SET %s + FROM bounded b + WHERE t.id = b.id AND b.matched <= $%d + RETURNING t.id, t.contact_id, t.organization_id, t.title, b.old_status + )%s + SELECT (SELECT COUNT(*) FROM candidate), (SELECT COUNT(*) FROM upd) + `, where, capPos+1, strings.Join(setClauses, ", "), capPos, activity) + + if err := r.db.QueryRow(ctx, query, args...).Scan(&matched, &affected); err != nil { + return 0, 0, err + } + return matched, affected, nil +} + func (r *crmRepository) UpdateCRMTask(ctx context.Context, orgID, taskID uuid.UUID, data *models.UpdateCRMTask) (*models.CRMTask, error) { setClauses := []string{} args := []any{orgID, taskID} @@ -1434,6 +1604,11 @@ func (r *crmRepository) UpdateCRMTask(ctx context.Context, orgID, taskID uuid.UU if *data.Status == "completed" { setClauses = append(setClauses, "completed_at = NOW()") + } else { + // completed_at is when the task was finished, so a task moved back + // off completed has no such time; leaving the old one behind shows + // a pending task as having been finished days ago. + setClauses = append(setClauses, "completed_at = NULL") } } diff --git a/web/src/app/app/crm/tasks/page.tsx b/web/src/app/app/crm/tasks/page.tsx index 9fcc3641c..e0503f8c7 100644 --- a/web/src/app/app/crm/tasks/page.tsx +++ b/web/src/app/app/crm/tasks/page.tsx @@ -24,7 +24,9 @@ import { AlertTriangleIcon, ArrowUpDownIcon, CalendarClockIcon, + CheckIcon, CheckSquareIcon, + FlagIcon, LayoutListIcon, ListTreeIcon, Loader2Icon, @@ -59,6 +61,7 @@ import { PopoverMenu, PopoverMenuContent, PopoverMenuItem, + PopoverMenuLabel, PopoverMenuTrigger, } from "@/components/ui/popover-menu"; import useSearchTasks from "@/lib/api/hooks/app/crm/tasks/useSearchTasks"; @@ -66,6 +69,8 @@ import useTasksSummary from "@/lib/api/hooks/app/crm/tasks/useTasksSummary"; import useCreateCRMTask from "@/lib/api/hooks/app/crm/tasks/useCreateCRMTask"; import useUpdateCRMTask from "@/lib/api/hooks/app/crm/tasks/useUpdateCRMTask"; import useDeleteCRMTask from "@/lib/api/hooks/app/crm/tasks/useDeleteCRMTask"; +import useBulkDeleteTasks from "@/lib/api/hooks/app/crm/tasks/useBulkDeleteTasks"; +import useBulkUpdateTasks from "@/lib/api/hooks/app/crm/tasks/useBulkUpdateTasks"; import useTaskTypes from "@/lib/api/hooks/app/crm/taskTypes/useTaskTypes"; import useMembers from "@/lib/api/hooks/app/organizations/useMembers"; import { useQueryClient } from "@tanstack/react-query"; @@ -79,6 +84,9 @@ import type { CRMTaskPriority, CRMTaskStatus } from "@/lib/api/models/app/crm/CR import type SearchTasks from "@/lib/api/models/app/crm/SearchTasks"; import type { TaskSortBy } from "@/lib/api/models/app/crm/SearchTasks"; import { EMPTY_TASK_SEARCH } from "@/lib/api/models/app/crm/SearchTasks"; +import type TaskSelection from "@/lib/api/models/app/crm/TaskSelection"; +import * as rowSelection from "@/lib/helper/rowSelection"; +import type { RowSelection } from "@/lib/helper/rowSelection"; import type OrganizationMember from "@/lib/api/models/app/organizations/OrganizationMember"; import type Team from "@/lib/api/models/app/teams/Team"; import type { AppError } from "@/lib/api/client/normalizeError"; @@ -93,6 +101,15 @@ const PRIORITIES: { id: CRMTaskPriority; label: string; dot: string; text: strin { id: "low", label: "Low", dot: "bg-slate-400", text: "text-slate-600" }, ]; +// Status names as this page says them ("Active", not "in progress"), for the +// bulk-action toasts. +const STATUS_LABELS: Record = { + pending: "Pending", + in_progress: "Active", + completed: "Done", + cancelled: "Cancelled", +}; + const STATUS_TABS: { id: "all" | CRMTaskStatus; label: string }[] = [ { id: "all", label: "All" }, { id: "pending", label: "Pending" }, @@ -165,7 +182,9 @@ export default function TasksPage() { const search = useSearchTasks({ filters, limit: 50 }); const summary = useTasksSummary(filters); - const tasks = search.tasks ?? []; + // Memoised: the loaded-id list and the grouped buckets both derive from it, + // and a fresh [] on every render would rebuild them every render. + const tasks = React.useMemo(() => search.tasks ?? [], [search.tasks]); const total = search.total; const sum = summary.data; @@ -192,6 +211,90 @@ export default function TasksPage() { const { data: types = [] } = useTaskTypes(); + // ── Multi-select ─────────────────────────────────────────────────────── + // Either the rows ticked, or every task the current filter matches minus + // the ones unticked afterwards. Only the second reaches past the pages + // loaded so far, and only the server can resolve it, so it travels as the + // filter itself. + const [rowSel, setRowSel] = React.useState(rowSelection.emptySelection); + const bulkDelete = useBulkDeleteTasks(); + const bulkUpdate = useBulkUpdateTasks(); + const confirm = useConfirm(); + + const loadedIDs = React.useMemo(() => tasks.map((t) => t.id), [tasks]); + const clearSelection = React.useCallback(() => setRowSel(rowSelection.emptySelection), []); + const isRowSelected = React.useCallback((id: string) => rowSelection.isRowSelected(rowSel, id), [rowSel]); + const selectionCount = rowSelection.selectionCount(rowSel, total); + const loadedAllSelected = rowSelection.allLoadedSelected(rowSel, loadedIDs); + const canSelectAllMatching = rowSelection.canSelectAllMatching(rowSel, loadedIDs, total); + + // A teammate deleting a row the user had unticked would otherwise leave its + // id in `excluded` for good, counting a task that no longer exists against + // the selection and eventually emptying it on screen while rows stay + // selected. Only sound once every matching row is loaded. + React.useEffect(() => { + if (search.hasNextPage) return; + setRowSel((sel) => rowSelection.pruneExcluded(sel, loadedIDs)); + }, [search.hasNextPage, loadedIDs]); + + // A selection means what the filter meant when it was made, so changing + // the filter drops it rather than silently applying to a different set. + const filterKey = JSON.stringify(filters); + React.useEffect(() => { + clearSelection(); + }, [filterKey, clearSelection]); + + const selection = React.useMemo( + () => (rowSel.all ? { tasks: [], all: true, filters, exclude: rowSel.excluded } : { tasks: rowSel.ids }), + [rowSel, filters], + ); + + const busy = bulkDelete.isPending || bulkUpdate.isPending; + + // The server reports what it wrote, which is not always what was selected: + // a teammate can have deleted or already completed a row in between. Say + // the real number rather than the one on the button. + function report(affected: number, verb: string) { + if (affected === 0) { + toast("Nothing changed: those tasks are gone, or already in that state."); + return; + } + toast.success(`${affected.toLocaleString()} ${affected === 1 ? "task" : "tasks"} ${verb}`); + } + + // The rows stay tickable while the request is in flight, so clear only the + // selection that was actually sent. Clearing unconditionally threw away a + // selection the user had started building while waiting. + function clearIfUnchanged(submitted: RowSelection) { + setRowSel((current) => (current === submitted ? rowSelection.emptySelection : current)); + } + + async function applyBulk(patch: { status?: CRMTaskStatus; priority?: CRMTaskPriority }, verb: string) { + if (selectionCount === 0) return; + const submitted = rowSel; + try { + const res = await bulkUpdate.mutateAsync({ ...selection, ...patch }); + clearIfUnchanged(submitted); + report(res.affected, verb); + } catch (err) { + toast.error(buildError(err as AppError)); + } + } + + function deleteSelected() { + if (selectionCount === 0) return; + const submitted = rowSel; + confirm?.show(deletePrompt(selectionCount), async () => { + try { + const res = await bulkDelete.mutateAsync(selection); + clearIfUnchanged(submitted); + report(res.affected, "deleted"); + } catch (err) { + toast.error(buildError(err as AppError)); + } + }); + } + const statusTab: "all" | CRMTaskStatus = filters.statuses.length === 1 ? filters.statuses[0] : "all"; @@ -297,22 +400,44 @@ export default function TasksPage() { onCreate={() => setNewOpen(true)} onClear={() => setFilters(EMPTY_TASK_SEARCH)} /> - ) : view === "grouped" ? ( - ) : ( - + <> + setRowSel(rowSelection.selectAllMatching())} + onClear={clearSelection} + grouped={view === "grouped"} + /> + {view === "grouped" ? ( + setRowSel((sel) => rowSelection.toggleRow(sel, id, on))} + onToggleMany={(ids) => setRowSel((sel) => rowSelection.toggleGroup(sel, ids))} + allSelected={(ids) => rowSelection.allLoadedSelected(rowSel, ids)} + /> + ) : ( + setRowSel((sel) => rowSelection.toggleRow(sel, id, on))} + allLoadedSelected={loadedAllSelected} + onToggleAll={() => setRowSel((sel) => rowSelection.toggleLoaded(sel, loadedIDs))} + /> + )} + )} {!search.isPending && tasks.length > 0 && ( @@ -326,6 +451,15 @@ export default function TasksPage() { )} + applyBulk({ status }, status === "completed" ? "marked done" : `set to ${STATUS_LABELS[status]}`)} + onPriority={(priority) => applyBulk({ priority }, `set to ${priority} priority`)} + onDelete={deleteSelected} + onClear={clearSelection} + /> + setNewOpen(false)} @@ -343,6 +477,177 @@ export default function TasksPage() { ); } +// ── Selection ─────────────────────────────────────────────── + +// deletePrompt words the confirm so a 4,000-row select-all does not read the +// same as three ticked rows. +function deletePrompt(count: number): string { + if (count === 1) return "Delete this task?"; + return `Delete ${count.toLocaleString()} tasks? This cannot be undone.`; +} + +// The bridge between "every row on screen" and "every row that matches". The +// list only ever holds the pages it has loaded, so ticking the header can never +// mean the whole filtered set on its own; this says what is selected and offers +// the rest in one click. +function SelectAllBanner({ + selectAll, + count, + loadedCount, + total, + canSelectAllMatching, + onSelectAllMatching, + onClear, + grouped, +}: { + selectAll: boolean; + count: number; + loadedCount: number; + total: number; + canSelectAllMatching: boolean; + onSelectAllMatching: () => void; + onClear: () => void; + grouped: boolean; +}) { + if (!selectAll && !canSelectAllMatching) return null; + const plural = (n: number) => (n === 1 ? "task" : "tasks"); + return ( +
+ {selectAll ? ( + <> + + All {count.toLocaleString()} {plural(count)} matching this + view are selected. + + + + ) : ( + <> + + The {loadedCount.toLocaleString()} {plural(loadedCount)}{" "} + loaded here are selected. + + + + )} +
+ ); +} + +// Floating bulk-action bar, same shape as the contacts one: it appears with the +// first ticked row and names the count before any action reads it. +function TaskSelectionBar({ + count, + busy, + onStatus, + onPriority, + onDelete, + onClear, +}: { + count: number; + busy: boolean; + onStatus: (status: CRMTaskStatus) => void; + onPriority: (priority: CRMTaskPriority) => void; + onDelete: () => void; + onClear: () => void; +}) { + if (count === 0) return null; + return ( +
+
+ + {count.toLocaleString()} selected +
+ + + + + + + Set {count.toLocaleString()} to + {STATUS_TABS.filter((t) => t.id !== "all").map((t) => ( + onStatus(t.id as CRMTaskStatus)}> + {t.label} + + ))} + + + + + + + + Set {count.toLocaleString()} to + {PRIORITIES.map((p) => ( + onPriority(p.id)} + icon={} + > + {p.label} + + ))} + + + +
+ +
+ ); +} + // ── Views ──────────────────────────────────────────────────────────────── function FlatView({ @@ -351,17 +656,34 @@ function FlatView({ teamById, types, onOpen, + isRowSelected, + onToggle, + allLoadedSelected, + onToggleAll, }: { tasks: CRMTask[]; memberByUser: Map; teamById: Map; types: { name: string; color: string }[]; onOpen: (t: CRMTask) => void; + isRowSelected: (id: string) => boolean; + onToggle: (id: string, on: boolean) => void; + allLoadedSelected: boolean; + onToggleAll: () => void; }) { return ( + @@ -380,6 +702,8 @@ function FlatView({ team={t.assigned_team_id ? teamById.get(t.assigned_team_id) : undefined} types={types} onOpen={() => onOpen(t)} + selected={isRowSelected(t.id)} + onToggle={(on) => onToggle(t.id, on)} /> ))} @@ -393,12 +717,16 @@ function FlatRow({ team, types, onOpen, + selected, + onToggle, }: { task: CRMTask; member?: OrganizationMember; team?: Team; types: { name: string; color: string }[]; onOpen: () => void; + selected: boolean; + onToggle: (on: boolean) => void; }) { const update = useUpdateCRMTask(); const del = useDeleteCRMTask(); @@ -432,8 +760,19 @@ function FlatRow({ return ( +
+ + Task Type Assignee
e.stopPropagation()}> + onToggle(!selected)} + /> +
+ ); + })} +
+

+ {value.length === 0 + ? "Nothing opens a task. Same as turning the switch off." + : `A reply classified ${REPLY_INTENT_CHOICES.filter((c) => has(c.id)) + .map((c) => c.label.toLowerCase()) + .join(", ")} opens a task.`} +

+ + ); +} + function UnsubscribeRows({ value, onChange, diff --git a/web/src/components/app/contacts/selection.ts b/web/src/components/app/contacts/selection.ts index d0878422c..51981ea9a 100644 --- a/web/src/components/app/contacts/selection.ts +++ b/web/src/components/app/contacts/selection.ts @@ -1,80 +1,13 @@ -// Row selection for the contacts table, its campaign Leads and segment -// members variants. -// -// A selection is one of two things and the difference matters at every call -// site: a list of rows the user ticked, or "everything the current search -// matches" minus the rows unticked afterwards. Only the second reaches past -// the pages the table has loaded, and only the server can resolve it, so it -// travels as the search itself. +// Contacts' row selection: the shared table-selection state, plus the wire +// shape the contact bulk endpoints take. import type SearchContacts from "@/lib/api/models/app/contacts/SearchContacts"; import type ContactSelection from "@/lib/api/models/app/contacts/ContactSelection"; +import type { RowSelection } from "@/lib/helper/rowSelection"; -export interface RowSelection { - /** Every contact the current search matches, rather than `ids`. */ - all: boolean; - /** Ticked rows. Empty while `all` is set. */ - ids: string[]; - /** Rows unticked after a select-all. Empty while `all` is not set. */ - excluded: string[]; -} +export * from "@/lib/helper/rowSelection"; -export const emptySelection: RowSelection = { all: false, ids: [], excluded: [] }; - -export function isRowSelected(s: RowSelection, id: string): boolean { - return s.all ? !s.excluded.includes(id) : s.ids.includes(id); -} - -/** How many contacts the selection covers. `total` is the search's own total. */ -export function selectionCount(s: RowSelection, total: number): number { - return s.all ? Math.max(total - s.excluded.length, 0) : s.ids.length; -} - -export function isEmpty(s: RowSelection, total: number): boolean { - return selectionCount(s, total) === 0; -} - -export function toggleRow(s: RowSelection, id: string, on: boolean): RowSelection { - if (s.all) { - return { ...s, excluded: on ? s.excluded.filter((x) => x !== id) : [...s.excluded, id] }; - } - return { ...s, ids: on ? [...s.ids, id] : s.ids.filter((x) => x !== id) }; -} - -/** Whether every row on screen is selected: the header checkbox's state. */ -export function allLoadedSelected(s: RowSelection, loaded: string[]): boolean { - return loaded.length > 0 && loaded.every((id) => isRowSelected(s, id)); -} - -/** - * The header checkbox, which reads the rows on screen. In select-all mode an - * unchecked box means some of them were unticked, so it puts those back rather - * than dropping a selection the user never asked to lose; a checked one clears. - */ -export function toggleLoaded(s: RowSelection, loaded: string[]): RowSelection { - if (s.all) { - if (allLoadedSelected(s, loaded)) return emptySelection; - return { ...s, excluded: s.excluded.filter((id) => !loaded.includes(id)) }; - } - if (allLoadedSelected(s, loaded)) { - return { ...s, ids: s.ids.filter((id) => !loaded.includes(id)) }; - } - return { ...s, ids: Array.from(new Set([...s.ids, ...loaded])) }; -} - -export function selectAllMatching(): RowSelection { - return { all: true, ids: [], excluded: [] }; -} - -/** - * Whether the "select all N matching" bar has anything to offer: every loaded - * row is ticked and more rows match than are ticked. - */ -export function canSelectAllMatching(s: RowSelection, loaded: string[], total: number): boolean { - return !s.all && allLoadedSelected(s, loaded) && total > s.ids.length; -} - -/** The wire shape every bulk endpoint takes. */ +/** The wire shape every contact bulk endpoint takes. */ export function toRequest(s: RowSelection, filters: SearchContacts): ContactSelection { if (!s.all) return { contacts: s.ids }; return { contacts: [], all: true, filters, exclude: s.excluded }; diff --git a/web/src/lib/api/client/app/crm/tasks/bulkDeleteTasks.ts b/web/src/lib/api/client/app/crm/tasks/bulkDeleteTasks.ts new file mode 100644 index 000000000..b5ff882fa --- /dev/null +++ b/web/src/lib/api/client/app/crm/tasks/bulkDeleteTasks.ts @@ -0,0 +1,15 @@ +import type TaskSelection from "@/lib/api/models/app/crm/TaskSelection"; +import type { BulkTasksResponse } from "@/lib/api/models/app/crm/TaskSelection"; +import Request from "../../../Request"; + +// The endpoint also accepts a bare id array; the client always sends the +// selection object so "select all matching" is one request instead of a page +// walk. +export default async function bulkDeleteTasks(selection: TaskSelection): Promise { + return await Request({ + method: "DELETE", + url: "/crm/tasks", + data: selection, + authorization: true, + }); +} diff --git a/web/src/lib/api/client/app/crm/tasks/bulkUpdateTasks.ts b/web/src/lib/api/client/app/crm/tasks/bulkUpdateTasks.ts new file mode 100644 index 000000000..0cc2ab603 --- /dev/null +++ b/web/src/lib/api/client/app/crm/tasks/bulkUpdateTasks.ts @@ -0,0 +1,11 @@ +import type { BulkTasksResponse, BulkUpdateTasks } from "@/lib/api/models/app/crm/TaskSelection"; +import Request from "../../../Request"; + +export default async function bulkUpdateTasks(data: BulkUpdateTasks): Promise { + return await Request({ + method: "PATCH", + url: "/crm/tasks", + data, + authorization: true, + }); +} diff --git a/web/src/lib/api/hooks/app/crm/tasks/useBulkDeleteTasks.ts b/web/src/lib/api/hooks/app/crm/tasks/useBulkDeleteTasks.ts new file mode 100644 index 000000000..81d82bcfd --- /dev/null +++ b/web/src/lib/api/hooks/app/crm/tasks/useBulkDeleteTasks.ts @@ -0,0 +1,17 @@ +import { useMutation, useQueryClient } from "@tanstack/react-query"; +import bulkDeleteTasks from "@/lib/api/client/app/crm/tasks/bulkDeleteTasks"; +import type TaskSelection from "@/lib/api/models/app/crm/TaskSelection"; +import { useLivePatch } from "@/hooks/useLivePatch"; + +export default function useBulkDeleteTasks() { + const queryClient = useQueryClient(); + const { pushPatch } = useLivePatch("crm_tasks"); + + return useMutation({ + mutationFn: (selection: TaskSelection) => bulkDeleteTasks(selection), + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: ["crm", "tasks"] }); + pushPatch({ kind: "task_change" }); + }, + }); +} diff --git a/web/src/lib/api/hooks/app/crm/tasks/useBulkUpdateTasks.ts b/web/src/lib/api/hooks/app/crm/tasks/useBulkUpdateTasks.ts new file mode 100644 index 000000000..10afa16d0 --- /dev/null +++ b/web/src/lib/api/hooks/app/crm/tasks/useBulkUpdateTasks.ts @@ -0,0 +1,17 @@ +import { useMutation, useQueryClient } from "@tanstack/react-query"; +import bulkUpdateTasks from "@/lib/api/client/app/crm/tasks/bulkUpdateTasks"; +import type { BulkUpdateTasks } from "@/lib/api/models/app/crm/TaskSelection"; +import { useLivePatch } from "@/hooks/useLivePatch"; + +export default function useBulkUpdateTasks() { + const queryClient = useQueryClient(); + const { pushPatch } = useLivePatch("crm_tasks"); + + return useMutation({ + mutationFn: (data: BulkUpdateTasks) => bulkUpdateTasks(data), + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: ["crm", "tasks"] }); + pushPatch({ kind: "task_change" }); + }, + }); +} diff --git a/web/src/lib/api/models/app/analytics/Deliverability.ts b/web/src/lib/api/models/app/analytics/Deliverability.ts index 1fd4c55d6..4d6195e84 100644 --- a/web/src/lib/api/models/app/analytics/Deliverability.ts +++ b/web/src/lib/api/models/app/analytics/Deliverability.ts @@ -74,6 +74,9 @@ export default interface DeliverabilityDashboard { intent_out_of_office: number; intent_question: number; intent_neutral: number; + // Machine replies that are not vacation notices: autoresponders, ticket + // acknowledgements, bounces, delivery reports. + intent_automated: number; emails_sent: number; bounce_rate: number; diff --git a/web/src/lib/api/models/app/crm/TaskSelection.ts b/web/src/lib/api/models/app/crm/TaskSelection.ts new file mode 100644 index 000000000..fe67bfe45 --- /dev/null +++ b/web/src/lib/api/models/app/crm/TaskSelection.ts @@ -0,0 +1,23 @@ +import type SearchTasks from "./SearchTasks"; +import type { CRMTaskPriority, CRMTaskStatus } from "./CRMTask"; + +// The tasks a bulk action applies to. Either an explicit list of ticked ids, +// or "select all matching": the same search the list ran, minus the rows +// unticked after it. The filter form is what lets an action reach past the +// pages the table has loaded. +export default interface TaskSelection { + tasks: string[]; + all?: boolean; + filters?: SearchTasks; + exclude?: string[]; +} + +// PATCH /crm/tasks: a selection plus the fields to write on every task in it. +export interface BulkUpdateTasks extends TaskSelection { + status?: CRMTaskStatus; + priority?: CRMTaskPriority; +} + +export interface BulkTasksResponse { + affected: number; +} diff --git a/web/src/lib/api/models/app/integrations/Integration.ts b/web/src/lib/api/models/app/integrations/Integration.ts index ca99b94ed..158ff7e07 100644 --- a/web/src/lib/api/models/app/integrations/Integration.ts +++ b/web/src/lib/api/models/app/integrations/Integration.ts @@ -305,6 +305,7 @@ export const REPLY_INTENT_OPTIONS: { value: string; label: string }[] = [ { value: "neutral", label: "Neutral" }, { value: "negative", label: "Negative" }, { value: "out_of_office", label: "Out of office" }, + { value: "automated", label: "Automated" }, ]; // Human labels for the Warmbly event vocabulary (subset surfaced as triggers). diff --git a/web/src/lib/api/models/app/outreach/OutreachSettings.ts b/web/src/lib/api/models/app/outreach/OutreachSettings.ts index 4ac271ea7..2a644ce11 100644 --- a/web/src/lib/api/models/app/outreach/OutreachSettings.ts +++ b/web/src/lib/api/models/app/outreach/OutreachSettings.ts @@ -21,6 +21,59 @@ export interface PreflightSettings { min_content_score: number; } +// The classifier buckets a reply can land in. "automated" is a machine reply +// that is not a vacation notice: an autoresponder, a ticket acknowledgement, +// a bounce or a delivery report. +export type ReplyIntent = + | "positive" + | "question" + | "neutral" + | "negative" + | "out_of_office" + | "automated"; + +export interface ReplyIntentSettings { + enabled: boolean; + positive_keywords: string[]; + negative_keywords: string[]; + out_of_office_keywords: string[]; + question_keywords: string[]; + auto_create_crm_task: boolean; + // Which intents get a follow-up task. null/absent means the default set. + crm_task_intents?: ReplyIntent[] | null; + auto_pause_on_negative: boolean; + auto_suppress_on_unsubscribe_keyword: boolean; + // Park a contact's next step while their mailbox says they are away. + hold_on_out_of_office: boolean; + // The fallback hold, used when the auto-reply names no return date. + out_of_office_hold_days: number; +} + +// Matches models.DefaultCRMTaskIntents: every human intent, no automated one. +export const DEFAULT_CRM_TASK_INTENTS: ReplyIntent[] = [ + "positive", + "question", + "neutral", + "negative", +]; + +// The two machine classes are one choice in the UI: a vacation notice and a +// helpdesk autoresponder are the same kind of noise on a task list. +export const AUTOMATED_INTENTS: ReplyIntent[] = ["out_of_office", "automated"]; + +export const REPLY_INTENT_CHOICES: { id: ReplyIntent; label: string; hint: string }[] = [ + { id: "positive", label: "Positive", hint: "Interested, wants a call" }, + { id: "question", label: "Question", hint: "Asked something" }, + { id: "neutral", label: "Neutral", hint: "A human reply we could not bucket" }, + { id: "negative", label: "Negative", hint: "Not interested" }, + { id: "out_of_office", label: "Automated", hint: "Out of office, autoresponders, bounces" }, +]; + +/** The effective set: an unset list means the default, not "none". */ +export function taskIntents(s?: ReplyIntentSettings): ReplyIntent[] { + return s?.crm_task_intents ?? DEFAULT_CRM_TASK_INTENTS; +} + // The in-body opt-out every campaign email carries unless a campaign // overrides it. "text" is a reply-to-opt-out sentence, "link" a sentence with // a real unsubscribe link, "off" nothing. @@ -44,7 +97,7 @@ export interface OutreachSettings { bounce_pipeline: Record; task_reliability: Record; ab_testing: Record; - reply_intent: Record; + reply_intent: ReplyIntentSettings; send_time_optimization: SendTimeOptimizationSettings; preflight: PreflightSettings; dashboard: Record; diff --git a/web/src/lib/helper/rowSelection.test.ts b/web/src/lib/helper/rowSelection.test.ts new file mode 100644 index 000000000..d73b29f0c --- /dev/null +++ b/web/src/lib/helper/rowSelection.test.ts @@ -0,0 +1,84 @@ +import { describe, expect, it } from "vitest"; + +import * as sel from "./rowSelection"; +import type { RowSelection } from "./rowSelection"; + +// One group of a larger, server-paged list: the tasks in one due-date bucket. +const bucket = ["a", "b"]; +const rest = ["c", "d"]; +const TOTAL = 400; + +describe("a checkbox over one group of rows", () => { + it("ticks the group without touching the rest", () => { + let s = sel.toggleRow(sel.emptySelection, "c", true); + s = sel.toggleGroup(s, bucket); + expect(sel.selectionCount(s, TOTAL)).toBe(3); + for (const id of [...bucket, "c"]) expect(sel.isRowSelected(s, id)).toBe(true); + expect(sel.isRowSelected(s, "d")).toBe(false); + }); + + it("unticks only its own rows", () => { + let s = sel.toggleGroup(sel.emptySelection, [...bucket, ...rest]); + s = sel.toggleGroup(s, bucket); + expect(sel.selectionCount(s, TOTAL)).toBe(2); + expect(sel.isRowSelected(s, "a")).toBe(false); + expect(sel.isRowSelected(s, "c")).toBe(true); + }); + + // The bug this guards: routing a group header through toggleLoaded, which + // reads an all-ticked box as "clear everything", threw away a select-all of + // thousands because one group of two was unticked. + it("excludes the group from a select-all instead of dropping it", () => { + let s = sel.selectAllMatching(); + s = sel.toggleGroup(s, bucket); + expect(s.all).toBe(true); + expect(sel.selectionCount(s, TOTAL)).toBe(TOTAL - bucket.length); + expect(sel.isRowSelected(s, "a")).toBe(false); + expect(sel.isRowSelected(s, "c")).toBe(true); + }); + + it("puts an excluded group back", () => { + let s = sel.toggleGroup(sel.selectAllMatching(), bucket); + s = sel.toggleGroup(s, bucket); + expect(sel.selectionCount(s, TOTAL)).toBe(TOTAL); + expect(s.excluded).toEqual([]); + }); + + it("does not double-count a group ticked twice over", () => { + let s = sel.toggleRow(sel.emptySelection, "a", true); + s = sel.toggleGroup(s, bucket); + expect(s.ids).toEqual(["a", "b"]); + }); +}); + +// toggleLoaded keeps its own meaning: it is the header over EVERY loaded row, +// where an all-ticked box means "clear the selection". +describe("the header checkbox over every loaded row", () => { + it("clears a select-all rather than excluding the page", () => { + const s = sel.toggleLoaded(sel.selectAllMatching(), bucket); + expect(s).toEqual(sel.emptySelection); + }); +}); + +describe("pruneExcluded", () => { + it("drops exclusions the search no longer returns", () => { + const s: RowSelection = { all: true, ids: [], excluded: ["a", "b"] }; + expect(sel.pruneExcluded(s, ["b", "c"])).toEqual({ all: true, ids: [], excluded: ["b"] }); + }); + + it("returns the same object when every exclusion is still there", () => { + const s: RowSelection = { all: true, ids: [], excluded: ["a"] }; + expect(sel.pruneExcluded(s, ["a", "b"])).toBe(s); + }); + + it("leaves an id-list selection alone", () => { + const s: RowSelection = { all: false, ids: ["a"], excluded: [] }; + expect(sel.pruneExcluded(s, [])).toBe(s); + }); + + it("keeps the selection usable when the rows it excluded are all gone", () => { + const s: RowSelection = { all: true, ids: [], excluded: ["a", "b", "c"] }; + const pruned = sel.pruneExcluded(s, ["d", "e"]); + expect(sel.selectionCount(pruned, 2)).toBe(2); + }); +}); diff --git a/web/src/lib/helper/rowSelection.ts b/web/src/lib/helper/rowSelection.ts new file mode 100644 index 000000000..b760c8570 --- /dev/null +++ b/web/src/lib/helper/rowSelection.ts @@ -0,0 +1,112 @@ +// Row selection for a server-paged table. +// +// A selection is one of two things and the difference matters at every call +// site: a list of rows the user ticked, or "everything the current search +// matches" minus the rows unticked afterwards. Only the second reaches past +// the pages the table has loaded, and only the server can resolve it, so it +// travels as the search itself. +// +// The wire shape differs per resource (contacts send `contacts`, tasks send +// `tasks`), so each table keeps its own `toRequest`; everything else here is +// shared. + +export interface RowSelection { + /** Every row the current search matches, rather than `ids`. */ + all: boolean; + /** Ticked rows. Empty while `all` is set. */ + ids: string[]; + /** Rows unticked after a select-all. Empty while `all` is not set. */ + excluded: string[]; +} + +export const emptySelection: RowSelection = { all: false, ids: [], excluded: [] }; + +export function isRowSelected(s: RowSelection, id: string): boolean { + return s.all ? !s.excluded.includes(id) : s.ids.includes(id); +} + +/** How many rows the selection covers. `total` is the search's own total. */ +export function selectionCount(s: RowSelection, total: number): number { + return s.all ? Math.max(total - s.excluded.length, 0) : s.ids.length; +} + +export function isEmpty(s: RowSelection, total: number): boolean { + return selectionCount(s, total) === 0; +} + +export function toggleRow(s: RowSelection, id: string, on: boolean): RowSelection { + if (s.all) { + return { ...s, excluded: on ? s.excluded.filter((x) => x !== id) : [...s.excluded, id] }; + } + return { ...s, ids: on ? [...s.ids, id] : s.ids.filter((x) => x !== id) }; +} + +/** Whether every row on screen is selected: the header checkbox's state. */ +export function allLoadedSelected(s: RowSelection, loaded: string[]): boolean { + return loaded.length > 0 && loaded.every((id) => isRowSelected(s, id)); +} + +/** + * The header checkbox, which reads the rows on screen. In select-all mode an + * unchecked box means some of them were unticked, so it puts those back rather + * than dropping a selection the user never asked to lose; a checked one clears. + */ +export function toggleLoaded(s: RowSelection, loaded: string[]): RowSelection { + if (s.all) { + if (allLoadedSelected(s, loaded)) return emptySelection; + return { ...s, excluded: s.excluded.filter((id) => !loaded.includes(id)) }; + } + if (allLoadedSelected(s, loaded)) { + return { ...s, ids: s.ids.filter((id) => !loaded.includes(id)) }; + } + return { ...s, ids: Array.from(new Set([...s.ids, ...loaded])) }; +} + +/** + * A checkbox over a SUBSET of the loaded rows (a group header, say). Unlike + * toggleLoaded it never clears the whole selection: ticking adds this group, + * unticking takes only this group back out, which in select-all mode means + * excluding it rather than dropping every other row with it. + */ +export function toggleGroup(s: RowSelection, ids: string[]): RowSelection { + const on = allLoadedSelected(s, ids); + if (s.all) { + return { + ...s, + excluded: on + ? Array.from(new Set([...s.excluded, ...ids])) + : s.excluded.filter((id) => !ids.includes(id)), + }; + } + return { + ...s, + ids: on ? s.ids.filter((id) => !ids.includes(id)) : Array.from(new Set([...s.ids, ...ids])), + }; +} + +/** + * Drops exclusions for rows the search no longer returns, so a teammate + * deleting an unticked row does not keep counting it against the selection. + * + * Only sound with the COMPLETE result set: an id missing from a partial page + * is a row on a page nobody has loaded, not one that stopped matching. + * Returns the same object when nothing changed, so callers comparing a + * submitted selection by reference still recognise it. + */ +export function pruneExcluded(s: RowSelection, present: string[]): RowSelection { + if (!s.all || s.excluded.length === 0) return s; + const keep = s.excluded.filter((id) => present.includes(id)); + return keep.length === s.excluded.length ? s : { ...s, excluded: keep }; +} + +export function selectAllMatching(): RowSelection { + return { all: true, ids: [], excluded: [] }; +} + +/** + * Whether the "select all N matching" bar has anything to offer: every loaded + * row is ticked and more rows match than are ticked. + */ +export function canSelectAllMatching(s: RowSelection, loaded: string[], total: number): boolean { + return !s.all && allLoadedSelected(s, loaded) && total > s.ids.length; +}