Make delivery summary posting optional per program#498
Open
neal-bpm wants to merge 2 commits into
Open
Conversation
program Programs may have different preferences about whether campaign delivery summaries should be posted to their Slack channel - Added `send_delivery_summaries` boolean field to programs (defaults false) - Conditionally render the checkbox in the form only when a Slack channel is selected, preventing accidental configuration without a channel - Updated CampaignSummaryPoster to check this flag before posting
📝 WalkthroughWalkthroughA new ChangesDelivery Summaries Feature Flag
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
channel configured. This is the existing behaviour.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/bike_brigade/campaign_summary_poster.ex`:
- Around line 69-77: Add a defensive clause to do_post_summary/3 so non-true
values for the send_delivery_summaries flag (nil, false, or any other unexpected
value) don't cause a FunctionClauseError; for example add a clause like defp
do_post_summary(campaign, _channel_id, flag) when flag != true do
Logger.info("Skipping campaign #{campaign.id}: delivery summaries not enabled
for program (flag: #{inspect(flag)})") end to catch nil/false/invalid flag
values before the do_post_summary(campaign, channel_id, true) clause.
In
`@priv/repo/migrations/20260512195708_add_send_delivery_summaries_to_programs.exs`:
- Line 6: The migration currently adds the column send_delivery_summaries with a
default but allows NULLs; update the migration's change function so the add
statement for :send_delivery_summaries uses null: false (e.g. add
:send_delivery_summaries, :boolean, default: false, null: false) to enforce a
non-null boolean at the DB level; ensure the migration's down/rollback behavior
(or change callback) remains correct after this change.
In
`@priv/repo/migrations/20260519000001_enable_send_delivery_summaries_for_programs_with_slack_channel.exs`:
- Around line 8-12: The migration currently references the schema module
BikeBrigade.Delivery.Program which can break replay; change the query to use the
table name string instead (e.g., from(p in "programs", update: [set:
[send_delivery_summaries: true]], where: not is_nil(p.slack_channel_id))) and
keep the Repo.update_all call, so the migration uses raw table-based Ecto
queries rather than the application schema module.
🪄 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: 0231233d-f6ef-4f85-bbc2-b8c75047e0ec
📒 Files selected for processing (5)
lib/bike_brigade/campaign_summary_poster.exlib/bike_brigade/delivery/program.exlib/bike_brigade_web/live/program_live/form_component.html.heexpriv/repo/migrations/20260512195708_add_send_delivery_summaries_to_programs.exspriv/repo/migrations/20260519000001_enable_send_delivery_summaries_for_programs_with_slack_channel.exs
Comment on lines
+69
to
+77
| defp do_post_summary(campaign, _, false) do | ||
| Logger.info("Skipping campaign #{campaign.id}: delivery summaries not enabled for program") | ||
| end | ||
|
|
||
| defp do_post_summary(campaign, nil) do | ||
| Logger.warning("Skipping campaign #{campaign.id}: no Slack channel configured for program") | ||
| Slack.Operations.notify_campaign_error(campaign, "No Slack channel configured") | ||
| defp do_post_summary(campaign, nil, _) do | ||
| Logger.info("Skipping campaign #{campaign.id}: no Slack channel configured for program") | ||
| end | ||
|
|
||
| defp do_post_summary(campaign, channel_id) do | ||
| defp do_post_summary(campaign, channel_id, true) do |
There was a problem hiding this comment.
Handle non-boolean flag values defensively in do_post_summary/3.
If send_delivery_summaries is nil and slack_channel_id is present, no clause matches and this can crash with FunctionClauseError.
Defensive clause update
-defp do_post_summary(campaign, _, false) do
+defp do_post_summary(campaign, _, enabled) when enabled != true do
Logger.info("Skipping campaign #{campaign.id}: delivery summaries not enabled for program")
end
defp do_post_summary(campaign, nil, _) do
Logger.info("Skipping campaign #{campaign.id}: no Slack channel configured for program")
end
defp do_post_summary(campaign, channel_id, true) do📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| defp do_post_summary(campaign, _, false) do | |
| Logger.info("Skipping campaign #{campaign.id}: delivery summaries not enabled for program") | |
| end | |
| defp do_post_summary(campaign, nil) do | |
| Logger.warning("Skipping campaign #{campaign.id}: no Slack channel configured for program") | |
| Slack.Operations.notify_campaign_error(campaign, "No Slack channel configured") | |
| defp do_post_summary(campaign, nil, _) do | |
| Logger.info("Skipping campaign #{campaign.id}: no Slack channel configured for program") | |
| end | |
| defp do_post_summary(campaign, channel_id) do | |
| defp do_post_summary(campaign, channel_id, true) do | |
| defp do_post_summary(campaign, _, enabled) when enabled != true do | |
| Logger.info("Skipping campaign #{campaign.id}: delivery summaries not enabled for program") | |
| end | |
| defp do_post_summary(campaign, nil, _) do | |
| Logger.info("Skipping campaign #{campaign.id}: no Slack channel configured for program") | |
| end | |
| defp do_post_summary(campaign, channel_id, true) do |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/bike_brigade/campaign_summary_poster.ex` around lines 69 - 77, Add a
defensive clause to do_post_summary/3 so non-true values for the
send_delivery_summaries flag (nil, false, or any other unexpected value) don't
cause a FunctionClauseError; for example add a clause like defp
do_post_summary(campaign, _channel_id, flag) when flag != true do
Logger.info("Skipping campaign #{campaign.id}: delivery summaries not enabled
for program (flag: #{inspect(flag)})") end to catch nil/false/invalid flag
values before the do_post_summary(campaign, channel_id, true) clause.
|
|
||
| def change do | ||
| alter table(:programs) do | ||
| add :send_delivery_summaries, :boolean, default: false |
There was a problem hiding this comment.
Enforce non-null boolean for send_delivery_summaries.
This column should be null: false; otherwise NULL can leak in and break boolean-gated logic at runtime.
Suggested migration change
alter table(:programs) do
- add :send_delivery_summaries, :boolean, default: false
+ add :send_delivery_summaries, :boolean, default: false, null: false
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| add :send_delivery_summaries, :boolean, default: false | |
| def change do | |
| alter table(:programs) do | |
| add :send_delivery_summaries, :boolean, default: false, null: false | |
| end | |
| end |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@priv/repo/migrations/20260512195708_add_send_delivery_summaries_to_programs.exs`
at line 6, The migration currently adds the column send_delivery_summaries with
a default but allows NULLs; update the migration's change function so the add
statement for :send_delivery_summaries uses null: false (e.g. add
:send_delivery_summaries, :boolean, default: false, null: false) to enforce a
non-null boolean at the DB level; ensure the migration's down/rollback behavior
(or change callback) remains correct after this change.
Comment on lines
+8
to
+12
| from(prog in BikeBrigade.Delivery.Program, | ||
| update: [set: [send_delivery_summaries: true]], | ||
| where: not is_nil(prog.slack_channel_id) | ||
| ) | ||
| |> Repo.update_all([]) |
There was a problem hiding this comment.
Avoid app schema modules inside migrations.
Using BikeBrigade.Delivery.Program in a migration can break replay on new environments after future schema changes. Use SQL (or raw table-based query) instead.
Safer migration approach
def up do
- from(prog in BikeBrigade.Delivery.Program,
- update: [set: [send_delivery_summaries: true]],
- where: not is_nil(prog.slack_channel_id)
- )
- |> Repo.update_all([])
+ execute("""
+ UPDATE programs
+ SET send_delivery_summaries = TRUE
+ WHERE slack_channel_id IS NOT NULL
+ """)
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| from(prog in BikeBrigade.Delivery.Program, | |
| update: [set: [send_delivery_summaries: true]], | |
| where: not is_nil(prog.slack_channel_id) | |
| ) | |
| |> Repo.update_all([]) | |
| execute(""" | |
| UPDATE programs | |
| SET send_delivery_summaries = TRUE | |
| WHERE slack_channel_id IS NOT NULL | |
| """) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@priv/repo/migrations/20260519000001_enable_send_delivery_summaries_for_programs_with_slack_channel.exs`
around lines 8 - 12, The migration currently references the schema module
BikeBrigade.Delivery.Program which can break replay; change the query to use the
table name string instead (e.g., from(p in "programs", update: [set:
[send_delivery_summaries: true]], where: not is_nil(p.slack_channel_id))) and
keep the Repo.update_all call, so the migration uses raw table-based Ecto
queries rather than the application schema module.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="priv/repo/migrations/20260512195708_add_send_delivery_summaries_to_programs.exs">
<violation number="1" location="priv/repo/migrations/20260512195708_add_send_delivery_summaries_to_programs.exs:6">
P1: Add `null: false` to the column definition to enforce the boolean constraint at the database level. Without it, NULL values can leak in and cause `FunctionClauseError` in `do_post_summary/3` which only matches `true` or `false`.</violation>
</file>
<file name="priv/repo/migrations/20260519000001_enable_send_delivery_summaries_for_programs_with_slack_channel.exs">
<violation number="1" location="priv/repo/migrations/20260519000001_enable_send_delivery_summaries_for_programs_with_slack_channel.exs:8">
P2: Using application schema modules (`BikeBrigade.Delivery.Program`) in migrations can break replay on new environments if the schema later changes. Use raw SQL via `execute/1` instead to keep the migration self-contained.</violation>
</file>
<file name="lib/bike_brigade/campaign_summary_poster.ex">
<violation number="1" location="lib/bike_brigade/campaign_summary_poster.ex:69">
P1: When `send_delivery_summaries` is `nil` (the column permits NULL since the migration lacks `null: false`), and `slack_channel_id` is present, no function clause matches—resulting in a `FunctionClauseError` at runtime. Use a guard like `when enabled != true` instead of matching on the literal `false` to handle nil defensively.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| def change do | ||
| alter table(:programs) do | ||
| add :send_delivery_summaries, :boolean, default: false |
There was a problem hiding this comment.
P1: Add null: false to the column definition to enforce the boolean constraint at the database level. Without it, NULL values can leak in and cause FunctionClauseError in do_post_summary/3 which only matches true or false.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At priv/repo/migrations/20260512195708_add_send_delivery_summaries_to_programs.exs, line 6:
<comment>Add `null: false` to the column definition to enforce the boolean constraint at the database level. Without it, NULL values can leak in and cause `FunctionClauseError` in `do_post_summary/3` which only matches `true` or `false`.</comment>
<file context>
@@ -0,0 +1,9 @@
+
+ def change do
+ alter table(:programs) do
+ add :send_delivery_summaries, :boolean, default: false
+ end
+ end
</file context>
| ) | ||
| end | ||
|
|
||
| defp do_post_summary(campaign, _, false) do |
There was a problem hiding this comment.
P1: When send_delivery_summaries is nil (the column permits NULL since the migration lacks null: false), and slack_channel_id is present, no function clause matches—resulting in a FunctionClauseError at runtime. Use a guard like when enabled != true instead of matching on the literal false to handle nil defensively.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/bike_brigade/campaign_summary_poster.ex, line 69:
<comment>When `send_delivery_summaries` is `nil` (the column permits NULL since the migration lacks `null: false`), and `slack_channel_id` is present, no function clause matches—resulting in a `FunctionClauseError` at runtime. Use a guard like `when enabled != true` instead of matching on the literal `false` to handle nil defensively.</comment>
<file context>
@@ -58,15 +58,23 @@ defmodule BikeBrigade.CampaignSummaryPoster do
+ )
+ end
+
+ defp do_post_summary(campaign, _, false) do
+ Logger.info("Skipping campaign #{campaign.id}: delivery summaries not enabled for program")
end
</file context>
Suggested change
| defp do_post_summary(campaign, _, false) do | |
| defp do_post_summary(campaign, _, enabled) when enabled != true do |
| alias BikeBrigade.Repo | ||
|
|
||
| def up do | ||
| from(prog in BikeBrigade.Delivery.Program, |
There was a problem hiding this comment.
P2: Using application schema modules (BikeBrigade.Delivery.Program) in migrations can break replay on new environments if the schema later changes. Use raw SQL via execute/1 instead to keep the migration self-contained.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At priv/repo/migrations/20260519000001_enable_send_delivery_summaries_for_programs_with_slack_channel.exs, line 8:
<comment>Using application schema modules (`BikeBrigade.Delivery.Program`) in migrations can break replay on new environments if the schema later changes. Use raw SQL via `execute/1` instead to keep the migration self-contained.</comment>
<file context>
@@ -0,0 +1,16 @@
+ alias BikeBrigade.Repo
+
+ def up do
+ from(prog in BikeBrigade.Delivery.Program,
+ update: [set: [send_delivery_summaries: true]],
+ where: not is_nil(prog.slack_channel_id)
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Make delivery summary posting optional per program, allowing programs to control whether campaign
delivery summaries are posted to their configured Slack channel.
Problem
Previously, campaign delivery summaries were automatically posted to Slack whenever a program had a Slack channel configured. This didn't account for programs that may not want this automated notification,
creating unnecessary Slack traffic for some teams.
Solution
send_delivery_summariesboolean field to programs (defaults tofalse)CampaignSummaryPosterto check this flag before posting summariesChanges
Backend:
send_delivery_summariescolumn toprogramstableProgramschema to include the new fieldCampaignSummaryPoster.post_summary_for_campaign/1to check the flag before postingFrontend:
:ifdirective to prevent showing the option when channel is not configuredNote
Programs that want delivery summaries need to explicitly enable the checkbox.
Checklist before requesting a review
about this update in the description above.
Opt-in delivery summaries: programs control whether campaign delivery summaries are posted to Slack channel.
Summary by cubic
Make Slack delivery summaries opt-in per program so teams control whether campaign summaries are posted. Default is off; existing programs with a Slack channel stay enabled to preserve current behavior.
New Features
send_delivery_summarieson programs (defaultfalse).CampaignSummaryPosterrespects the flag; skips and logs info when disabled or no channel.Migration
send_delivery_summariesfor programs that already have a Slack channel; new programs must set a Slack channel and enable “Send Campaign Delivery Summary to Slack.”Written for commit 1ecd4fd. Summary will update on new commits. Review in cubic
Summary by CodeRabbit