Skip to content

Vn/campaign summary scheduler#486

Open
neal-bpm wants to merge 20 commits intomainfrom
vn/campaign-summary-scheduler
Open

Vn/campaign summary scheduler#486
neal-bpm wants to merge 20 commits intomainfrom
vn/campaign-summary-scheduler

Conversation

@neal-bpm
Copy link
Copy Markdown
Contributor

@neal-bpm neal-bpm commented Mar 31, 2026

Describe your changes

(#476)

Summary

  • Add scheduled posting of campaign delivery summaries to Slack
  • Post summaries 60-75 minutes after campaign delivery window ends
  • Track posted summaries in database to prevent duplicates

Checklist before requesting a review

  • I have performed a self-review of my code
  • If it is a core feature, I have added tests.
  • Are there other PRs or Issues that I should link to here?
  • Will this be part of a product update? If yes, please write one phrase
    about this update in the description above.

Summary by CodeRabbit

  • New Features

    • Campaign delivery summaries are scheduled (~15 min), posted to Slack with localized date/time ranges, per‑rider breakdowns, completed/total counts, alphabetical ordering, and unassigned deliveries; sent summaries are recorded to prevent duplicates.
  • Chores

    • Added automated review configuration.
  • Database

    • Added storage for Slack campaign summary messages with uniqueness to prevent duplicates.
  • Tests

    • Added comprehensive tests for summary generation, formatting, ordering, escaping, and time-window querying.

neal-bpm added 11 commits March 27, 2026 22:36
#476

This module provides a data structure to consolidate campaign delivery information into a summary format suitable for various use cases.
  * Track total and completed task counts
  * Group assigned deliveries by rider name
  * Collect unassigned deliveries separately
  * Implement Collectable protocol for use with Enum.into/2
  * Include delivery details: dropoff name, status, and item names
#476

This function enables finding campaigns whose delivery window ended within a specified time range. It's designed to support a scheduled job that runs every 15 minutes to process recently ended campaigns.

The function:
- Accepts from_datetime and to_datetime as NaiveDateTime
- Uses inclusive bounds (>= and <=) for the time window
- Returns a list of Campaign structs without preloads

Test coverage includes:
- Finding campaigns within the specified window
- Returning empty list when no campaigns match
  #476

  Introduces a new schema and migration to store records of campaign summary messages posted to Slack. This enables tracking which campaigns have already had summaries sent.

  Schema fields:
  - slack_channel_id: Target Slack channel
  - raw_message: The formatted message content
  - sent_at: When the message was posted
  - campaign_id: Reference to the associated campaign

  The migration includes indexes on campaign_id and sent_at for efficient lookups when checking if a summary was already posted or querying by time range.
The "monday + week" combined filter test was failing because campaigns were created with delivery_start at 12:00 UTC, which is in the future when tests run before noon UTC.
The riders_latest_campaigns view excludes campaigns where delivery_start >= NOW(), so the rider's latest_campaign was NULL.
This caused the "week" filter to exclude the rider even though they had a valid Monday campaign.
Change the test helper to use 00:00:00 instead of 12:00:00 for delivery_start, ensuring campaigns are always in the past relative to test execution time.
…heduler

This is required to fetch ended campaign in the given time interval and used by Campaign Summary Poster function
Used to create the delivery summary structure by Campaign Summary Poster.
  Add name, delivery_start, and delivery_end fields to the struct.
  Add new/1 function that accepts a campaign and pre-populates these metadata fields from campaign.program.name and timestamps.
  Use a consistent timestamp across setup and tests instead of calling NaiveDateTime.utc_now() multiple times
    Introduces a GenServer that runs every 15 minutes to identify recently ended campaigns and persist their delivery summaries to the database.

    - Automates generation of campaign summaries after deliveries are completed
    - Includes a 60–75 minute delay to allow riders time to mark tasks as complete
    - Stores summaries for reveiw
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 31, 2026

📝 Walkthrough

Walkthrough

A GenServer periodically finds campaigns that ended ~60–75 minutes ago, builds per-campaign delivery summaries from tasks/riders, upserts a slack message record, posts a formatted Slack payload, and marks messages as sent. Added schema, payload builder, summary struct, scheduler, migration, tests, and CodeRabbit config.

Changes

Cohort / File(s) Summary
Config & Entrypoint
\.coderabbit\.yaml, lib/bike_brigade/application.ex
Added CodeRabbit config and registered BikeBrigade.CampaignSummaryPoster in the supervision tree.
Scheduler & Query
lib/bike_brigade/campaign_summary_poster.ex, lib/bike_brigade/delivery.ex
New GenServer CampaignSummaryPoster schedules 15‑minute ticks and uses list_campaigns_ended_between/2 to find recent campaign ends.
Summary Domain
lib/bike_brigade/delivery/campaign_delivery_summary.ex
New CampaignDeliverySummary struct with new/1, add_task/2, and Collectable implementation to aggregate totals, completed counts, per‑rider groups, and unassigned tasks.
Slack Messaging & Payloads
lib/bike_brigade/messaging/slack.ex, lib/bike_brigade/slack_api/payload_builder.ex
Added CampaignDeliverySummary.send_summary/2 and build_delivery_summary/2 to construct and post Slack JSON payloads (counts, per‑rider sections, date formatting, mrkdwn escaping).
Persistence
lib/bike_brigade/messaging/slack_campaign_summary_message.ex, priv/repo/migrations/..._add_slack_campaign_summary_messages.exs
New Ecto schema slack_campaign_messages and migration creating table with unique campaign_id, slack_channel_id, raw_message, optional sent_at, and index on sent_at.
Tests & Fixtures
test/bike_brigade/delivery/campaign_delivery_summary_test.exs, test/bike_brigade/delivery_test.exs, test/bike_brigade/slack_api/payload_builder_test.exs, test/bike_brigade/riders/rider_search_test.exs
Added tests for summary aggregation, campaign querying, and payload formatting (ordering, escaping); adjusted fixture times to midnight UTC and added helpers used by tests.

Sequence Diagram

sequenceDiagram
    participant Scheduler as CampaignSummaryPoster (GenServer)
    participant Repo as Database (Ecto/Repo)
    participant DeliverySvc as Delivery.campaign_riders_and_tasks
    participant Collector as CampaignDeliverySummary (Collectable)
    participant SlackApi as Slack API (PayloadBuilder + Poster)

    loop every 15 minutes
        Scheduler->>Repo: list_campaigns_ended_between(from,to)
        Repo-->>Scheduler: campaigns

        alt for each campaign
            Scheduler->>Repo: preload campaign.program
            Scheduler->>DeliverySvc: campaign_riders_and_tasks(campaign)
            DeliverySvc-->>Scheduler: tasks + rider info

            Scheduler->>Collector: new(campaign)
            loop tasks
                Scheduler->>Collector: add_task(task)
            end
            Collector-->>Scheduler: CampaignDeliverySummary

            Scheduler->>Repo: insert SlackCampaignSummaryMessage (on_conflict: :nothing)
            Repo-->>Scheduler: inserted? / existing

            alt newly inserted
                Scheduler->>SlackApi: build_delivery_summary(channel, summary)
                SlackApi-->>Scheduler: payload
                Scheduler->>SlackApi: post_message!(payload)
                SlackApi-->>Scheduler: success / error
                Scheduler->>Repo: update(sent_at)
            else already exists or send failed
                Scheduler->>Scheduler: log and continue
            end
        end
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~55 minutes

Poem

🐰 I hop through tasks and riders, fifteen minutes on the clock,
I gather counts and names in rows, then tap the Slack hub's lock,
I tuck the message in the DB, mark sent when it takes flight,
A little rabbit nods and grins — summaries tucked in tight,
🥕

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 15.38% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'Vn/campaign summary scheduler' is vague and uses non-descriptive terminology, mixing branch naming conventions with implementation details without clearly conveying the main change. Consider a more descriptive title such as 'Add scheduled campaign delivery summary posting to Slack' that clearly explains what functionality is being added.
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description includes the required summary section and completes most checklist items, though some optional items remain unchecked.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch vn/campaign-summary-scheduler

Comment @coderabbitai help to get the list of available commands and usage tips.

Convert the existing campaign_id index to a unique constraint to support atomic inserts with on_conflict: :nothing.
    - Prevents duplicate campaign summaries
    - Enables race-condition-free inserts
Formats CampaignDeliverySummary data into Slack messages using Block Kit for campaign completion updates.

Changes:

- Add build_delivery_summary/2 to PayloadBuilder for Slack Block Kit formatting
- Add campaign_id field to CampaignDeliverySummary struct
- Introduce Slack.CampaignDeliverySummary module with send_summary/2

Message format includes:

- Header with program name and campaign link
- Date/time range (same-day vs multi-day formatting)
- Total and completed delivery counts
- Rider sections with task lists and completion status
- Unassigned deliveries section (when applicable)
- Add CampaignSummaryPoster to application supervision tree
- Post summary to Slack channel after DB insert
- Update sent_at timestamp on successful Slack post
- Handle Slack errors gracefully without crashing
@neal-bpm neal-bpm marked this pull request as ready for review March 31, 2026 17:52
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (3)
lib/bike_brigade/delivery/campaign_delivery_summary.ex (2)

36-39: Redundant pattern match in fallback clause.

The pattern %{completed: _completed} on line 39 is unnecessary since completed is always present in the struct. A simple variable binding would suffice.

♻️ Simplified fallback clause
  defp completed(summary, %{delivery_status: :completed}),
    do: Map.update!(summary, :completed, &(&1 + 1))

- defp completed(%{completed: _completed} = summary, _task), do: summary
+ defp completed(summary, _task), do: summary
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/bike_brigade/delivery/campaign_delivery_summary.ex` around lines 36 - 39,
The fallback clause for the completed/2 function uses an unnecessary map pattern
match (%{completed: _completed}); change it to a simple variable binding to
avoid redundant matching—update defp completed(%{completed: _completed} =
summary, _task) to defp completed(summary, _task) so the function returns
summary when the task isn't :completed.

13-20: Requires program preload—ensure callers preload it.

new/1 accesses campaign.program.name, which will raise Ecto.Association.NotLoaded if program isn't preloaded. The current caller (CampaignSummaryPoster) does preload it, but consider adding a guard or documenting this requirement.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/bike_brigade/delivery/campaign_delivery_summary.ex` around lines 13 - 20,
The new/1 function assumes campaign.program is loaded and will crash with
Ecto.Association.NotLoaded; update new/1 to explicitly check
Ecto.assoc_loaded?(campaign.program) (or pattern-match on %Program{} in the
function head) and raise a clear ArgumentError instructing callers to preload
:program (or alternatively return {:error, ...}) so callers like
CampaignSummaryPoster know the requirement; reference the new/1 function and
campaign.program in your change.
test/bike_brigade/slack_api/payload_builder_test.exs (1)

177-190: Timezone assumption may cause test flakiness.

The test hardcodes UTC times and expects specific EDT output (10:00 AM - 2:00 PM). If the application's configured timezone (America/Toronto) observes DST differently in the test environment or if the configuration changes, this test could fail unexpectedly.

Consider using a timezone-aware assertion or mocking the timezone configuration for deterministic results.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/bike_brigade/slack_api/payload_builder_test.exs` around lines 177 - 190,
The test assumes a specific timezone conversion for
build_delivery_summary_payload which can flake; make the test deterministic by
controlling the app timezone or by asserting timezone-agnostic output: either
set the timezone config to "America/Toronto" for the test (e.g.,
Application.put_env(:your_app, :timezone, "America/Toronto") or call the same
timezone conversion helper used by build_delivery_summary_payload before
building the fixture) or construct campaign times already in the target local
zone and assert on the resulting summary_text from
build_delivery_summary_payload; reference the fixture(:campaign) creation and
the build_delivery_summary_payload call and update the test to mock/override
timezone or to compute expected local times dynamically from the UTC inputs
before asserting summary_text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lib/bike_brigade/campaign_summary_poster.ex`:
- Line 43: The code reads channel_id = campaign.program.slack_channel_id without
checking for nil; add a guard clause in the function that builds/posts the
summary (e.g., inside the CampaignSummaryPoster module/function that assigns
channel_id) to skip processing when campaign.program.slack_channel_id is nil:
return early (or next) and optionally log/debug that the campaign was skipped
due to missing Slack channel instead of attempting to post with a nil
channel_id. Ensure the check references campaign.program.slack_channel_id (or
the local channel_id variable) and prevents the Slack API call that uses
channel_id.

In `@lib/bike_brigade/delivery.ex`:
- Around line 321-326: Change the inclusive lower bound in
list_campaigns_ended_between so the window is exclusive on one side: replace
c.delivery_end >= ^from_utc_datetime with c.delivery_end > ^from_utc_datetime
and keep the upper bound as c.delivery_end <= ^to_utc_datetime; then add a
boundary unit test for Campaign.delivery_end == from_utc_datetime to assert the
record is returned only in the intended window (use the
list_campaigns_ended_between function and Campaign.delivery_end in the test).

In `@lib/bike_brigade/slack_api/payload_builder.ex`:
- Around line 65-69: The header and summary currently interpolate raw cds.name
and task_data.items into mrkdwn; wrap those interpolations with the existing
filter_mrkdwn/1 helper so special characters are escaped. Specifically, update
the header construction (where header = ":bar_chart: #{cds.name} Summary ...")
to use filter_mrkdwn(cds.name) and update the place that inserts task_data.items
(the interpolation referenced around task_data.items) to use
filter_mrkdwn(task_data.items) so both values are escaped for Slack mrkdwn
rendering.

In `@test/bike_brigade/riders/rider_search_test.exs`:
- Around line 125-127: The fixture create_campaign_for_date is creating
datetimes at UTC midnight which become the previous day in America/Toronto and
yield the wrong ISO weekday; change the datetime creation so the stored UTC time
maps to the intended local date—either create the DateTime at noon UTC (so
DateTime.new!(date, 12:00:00, "Etc/UTC")) or create a midnight DateTime in
"America/Toronto" and then shift it to UTC (use DateTime.new!(date, 00:00:00,
"America/Toronto") then shift_zone!/to UTC) so the EXTRACT(ISODOW FROM ? AT TIME
ZONE ?) returns the expected weekday for create_campaign_for_date.

---

Nitpick comments:
In `@lib/bike_brigade/delivery/campaign_delivery_summary.ex`:
- Around line 36-39: The fallback clause for the completed/2 function uses an
unnecessary map pattern match (%{completed: _completed}); change it to a simple
variable binding to avoid redundant matching—update defp completed(%{completed:
_completed} = summary, _task) to defp completed(summary, _task) so the function
returns summary when the task isn't :completed.
- Around line 13-20: The new/1 function assumes campaign.program is loaded and
will crash with Ecto.Association.NotLoaded; update new/1 to explicitly check
Ecto.assoc_loaded?(campaign.program) (or pattern-match on %Program{} in the
function head) and raise a clear ArgumentError instructing callers to preload
:program (or alternatively return {:error, ...}) so callers like
CampaignSummaryPoster know the requirement; reference the new/1 function and
campaign.program in your change.

In `@test/bike_brigade/slack_api/payload_builder_test.exs`:
- Around line 177-190: The test assumes a specific timezone conversion for
build_delivery_summary_payload which can flake; make the test deterministic by
controlling the app timezone or by asserting timezone-agnostic output: either
set the timezone config to "America/Toronto" for the test (e.g.,
Application.put_env(:your_app, :timezone, "America/Toronto") or call the same
timezone conversion helper used by build_delivery_summary_payload before
building the fixture) or construct campaign times already in the target local
zone and assert on the resulting summary_text from
build_delivery_summary_payload; reference the fixture(:campaign) creation and
the build_delivery_summary_payload call and update the test to mock/override
timezone or to compute expected local times dynamically from the UTC inputs
before asserting summary_text.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3b1a5a6d-01bc-4c59-8d8d-514976fb9519

📥 Commits

Reviewing files that changed from the base of the PR and between a27f40f and 83af22f.

📒 Files selected for processing (13)
  • .coderabbit.yaml
  • lib/bike_brigade/application.ex
  • lib/bike_brigade/campaign_summary_poster.ex
  • lib/bike_brigade/delivery.ex
  • lib/bike_brigade/delivery/campaign_delivery_summary.ex
  • lib/bike_brigade/messaging/slack.ex
  • lib/bike_brigade/messaging/slack_campaign_summary_message.ex
  • lib/bike_brigade/slack_api/payload_builder.ex
  • priv/repo/migrations/20260329221529_add_slack_campaign_summary_messages.exs
  • test/bike_brigade/delivery/campaign_delivery_summary_test.exs
  • test/bike_brigade/delivery_test.exs
  • test/bike_brigade/riders/rider_search_test.exs
  • test/bike_brigade/slack_api/payload_builder_test.exs

Comment on lines 125 to 127
defp create_campaign_for_date(date) do
datetime = DateTime.new!(date, ~T[12:00:00], "Etc/UTC")
datetime = DateTime.new!(date, ~T[00:00:00], "Etc/UTC")
fixture(:campaign, %{delivery_start: datetime})
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

fd -i 'rider_search' lib test -x sed -n '1,240p' {}
rg -nP 'monday|week|month|delivery_start|day_of_week|LocalizedDateTime|DateTime|NaiveDateTime' \
  lib/bike_brigade/riders \
  test/bike_brigade/riders

Repository: bikebrigade/dispatch

Length of output: 17366


🏁 Script executed:

sed -n '302,360p' lib/bike_brigade/riders/rider_search.ex

Repository: bikebrigade/dispatch

Length of output: 1935


The fixture creates Sunday campaigns instead of Monday—tests will fail or match wrong dates.

The filter correctly localizes delivery_start to America/Toronto timezone before extracting the ISO weekday (line 321: EXTRACT(ISODOW FROM ? AT TIME ZONE ?)). However, DateTime.new!(date, ~T[00:00:00], "Etc/UTC") at UTC midnight on a Monday becomes Sunday evening in Toronto time (UTC-5/-4). The filter will extract ISODOW 7 (Sunday) instead of 1 (Monday), causing tests that expect Monday campaigns to fail or inadvertently match unrelated deliveries.

Change the fixture to create the datetime at a local time—either noon UTC or explicitly at the date's local midnight:

datetime = DateTime.new!(date, ~T[12:00:00], "Etc/UTC")

or use DateTime.shift_zone!(DateTime.new!(date, ~T[00:00:00], "America/Toronto"), "Etc/UTC").

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/bike_brigade/riders/rider_search_test.exs` around lines 125 - 127, The
fixture create_campaign_for_date is creating datetimes at UTC midnight which
become the previous day in America/Toronto and yield the wrong ISO weekday;
change the datetime creation so the stored UTC time maps to the intended local
date—either create the DateTime at noon UTC (so DateTime.new!(date, 12:00:00,
"Etc/UTC")) or create a midnight DateTime in "America/Toronto" and then shift it
to UTC (use DateTime.new!(date, 00:00:00, "America/Toronto") then shift_zone!/to
UTC) so the EXTRACT(ISODOW FROM ? AT TIME ZONE ?) returns the expected weekday
for create_campaign_for_date.

@neal-bpm neal-bpm requested a review from mveytsman March 31, 2026 18:39
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lib/bike_brigade/slack_api/payload_builder.ex`:
- Around line 112-137: The section text can exceed Slack's 3,000-character limit
because format_task_lines returns an unbounded string; modify build_rider_block
and build_unassigned_block to enforce a max payload length before embedding
task_lines: compute available_space = 3000 - byte_size(header_text) -
safety_margin (e.g. 100), then if byte_size(task_lines) > available_space
truncate it safely (String.slice or :binary.part to preserve UTF-8) and append a
clear suffix like "… (truncated)" or "… (+N more)"; keep references to
format_task_lines (source of the lines) and filter_mrkdwn (used for rider_name)
so truncation happens after formatting but before constructing the Slack block
to prevent invalid_blocks errors.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 120b1746-b6e5-4d69-8ab5-38fb11681b47

📥 Commits

Reviewing files that changed from the base of the PR and between 3b224db and e3f3d90.

📒 Files selected for processing (1)
  • lib/bike_brigade/slack_api/payload_builder.ex

Comment on lines +112 to +137
defp build_rider_block({rider_name, tasks}) do
total = length(tasks)
completed = Enum.count(tasks, &(&1.delivery_status == :completed))
status_text = "(#{completed}/#{total})"
task_lines = format_task_lines(tasks)

%{
type: "section",
text: %{
type: "mrkdwn",
text: ":bicyclist: *#{filter_mrkdwn(rider_name)}* #{status_text}\n#{task_lines}"
}
}
end

defp build_unassigned_block([]), do: []

defp build_unassigned_block(unassigned_tasks) do
task_lines = format_task_lines(unassigned_tasks)

[
%{type: "divider"},
%{
type: "section",
text: %{type: "mrkdwn", text: ":package: *Unassigned Deliveries*\n#{task_lines}"}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

What is the maximum character length for Slack Block Kit section text objects (mrkdwn/plain_text), and what error response does chat.postMessage return when that limit is exceeded?

💡 Result:

  • Max character length (Section block text object, mrkdwn or plain_text): 3,000 characters (min 1, max 3000). The Block Kit Text object (type: mrkdwn|plain_text) itself has a 3000-character max, and the Section block text field references that same limit. [1], [2]

  • chat.postMessage error when you exceed it (e.g., section text.text > 3000): Slack rejects the blocks as invalid and returns an error JSON like:

    { "ok": false, "error": "invalid_blocks" }

    [3]

Sources:
[1] Slack “Text object” (Block Kit composition object) docs
[2] Slack “Section block” docs
[3] Slack chat.postMessage docs (Errors: invalid_blocks)


🏁 Script executed:

cat -n lib/bike_brigade/slack_api/payload_builder.ex | head -150

Repository: bikebrigade/dispatch

Length of output: 5143


🏁 Script executed:

cat -n lib/bike_brigade/slack_api/payload_builder.ex | tail -50

Repository: bikebrigade/dispatch

Length of output: 1594


Guard against Slack section text length overflows for large task lists.

format_task_lines concatenates all tasks without bounds. A rider with many deliveries or long dropoff/item names can cause task_lines to exceed Slack's 3,000-character section text limit, resulting in chat.postMessage failing with {"ok": false, "error": "invalid_blocks"} and dropping the entire summary message.

Implement truncation with a safety margin to prevent this:

Suggested hardening (truncate before building section text)
+  `@slack_section_text_safe_limit` 2900
+
   defp build_rider_block({rider_name, tasks}) do
     total = length(tasks)
     completed = Enum.count(tasks, &(&1.delivery_status == :completed))
     status_text = "(#{completed}/#{total})"
-    task_lines = format_task_lines(tasks)
+    task_lines = tasks |> format_task_lines() |> clamp_slack_text()

     %{
       type: "section",
       text: %{
         type: "mrkdwn",
         text: ":bicyclist: *#{filter_mrkdwn(rider_name)}* #{status_text}\n#{task_lines}"
       }
     }
   end

   defp build_unassigned_block(unassigned_tasks) do
-    task_lines = format_task_lines(unassigned_tasks)
+    task_lines = unassigned_tasks |> format_task_lines() |> clamp_slack_text()

     [
       %{type: "divider"},
       %{
         type: "section",
         text: %{type: "mrkdwn", text: ":package: *Unassigned Deliveries*\n#{task_lines}"}
       }
     ]
   end
+
+  defp clamp_slack_text(text) when is_binary(text) do
+    if String.length(text) > `@slack_section_text_safe_limit` do
+      String.slice(text, 0, `@slack_section_text_safe_limit`) <> "\n…"
+    else
+      text
+    end
+  end

Also applies to: 141-145

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/bike_brigade/slack_api/payload_builder.ex` around lines 112 - 137, The
section text can exceed Slack's 3,000-character limit because format_task_lines
returns an unbounded string; modify build_rider_block and build_unassigned_block
to enforce a max payload length before embedding task_lines: compute
available_space = 3000 - byte_size(header_text) - safety_margin (e.g. 100), then
if byte_size(task_lines) > available_space truncate it safely (String.slice or
:binary.part to preserve UTF-8) and append a clear suffix like "… (truncated)"
or "… (+N more)"; keep references to format_task_lines (source of the lines) and
filter_mrkdwn (used for rider_name) so truncation happens after formatting but
before constructing the Slack block to prevent invalid_blocks errors.

  - Return early with warning log when slack_channel_id is nil
  - Update list_campaigns_ended_between docs to clarify (from, to] bounds
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
lib/bike_brigade/campaign_summary_poster.ex (1)

70-79: Reasonable error handling, but failed sends won't be retried.

When Slack sending fails, the record remains in the database without sent_at, but due to on_conflict: :nothing, it won't be automatically retried on the next scheduler run. This is acceptable for an initial implementation, but consider adding a separate job to retry records where sent_at IS NULL if delivery reliability is important.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/bike_brigade/campaign_summary_poster.ex` around lines 70 - 79, The
current send_and_mark_sent function logs failures from send_to_slack but leaves
records with nil sent_at that will never be retried due to on_conflict:
:nothing; add a retry mechanism by enqueuing a background job (e.g., create or
call a RetrySlackSummary job) whenever send_to_slack returns {:error, reason}
instead of just logging, and ensure mark_as_sent(record) is only called on :ok;
update send_and_mark_sent to call the job enqueuer with the record id (or
relevant payload) on error so records with sent_at IS NULL get retried later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@lib/bike_brigade/campaign_summary_poster.ex`:
- Around line 70-79: The current send_and_mark_sent function logs failures from
send_to_slack but leaves records with nil sent_at that will never be retried due
to on_conflict: :nothing; add a retry mechanism by enqueuing a background job
(e.g., create or call a RetrySlackSummary job) whenever send_to_slack returns
{:error, reason} instead of just logging, and ensure mark_as_sent(record) is
only called on :ok; update send_and_mark_sent to call the job enqueuer with the
record id (or relevant payload) on error so records with sent_at IS NULL get
retried later.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 46cf1dc0-fed3-45f8-8abf-179a6ebb08ee

📥 Commits

Reviewing files that changed from the base of the PR and between e3f3d90 and 9643db8.

📒 Files selected for processing (2)
  • lib/bike_brigade/campaign_summary_poster.ex
  • lib/bike_brigade/delivery.ex
🚧 Files skipped from review as they are similar to previous changes (1)
  • lib/bike_brigade/delivery.ex

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant