fix: allow creating campaign drafts without lists assigned#3015
Open
Danimarqz wants to merge 1 commit intoknadh:masterfrom
Open
fix: allow creating campaign drafts without lists assigned#3015Danimarqz wants to merge 1 commit intoknadh:masterfrom
Danimarqz wants to merge 1 commit intoknadh:masterfrom
Conversation
Lists are now only required when starting or scheduling a campaign, not when saving a draft. This unblocks the content editor tab for non-admin users who previously could not save a campaign (and thus access the editor) without first selecting a list. Fixes knadh#3014
Contributor
There was a problem hiding this comment.
Issues Found
Critical (P0/P1)
- [P1] Avoid creating drafts with no lists due to permission checks (
cmd/campaigns.go:705-714)- Removing the
ListIDsnon-empty validation lets a user create a draft campaign with zero lists, but most subsequent endpoints (egUpdateCampaign,UpdateCampaignStatus, delete) callcheckCampaignPerm, which authorizes access by verifying the campaign belongs to one of the user’s permitted lists; a campaign with no lists will failCampaignHasListsand return 403 for non-*_allusers, effectively making the draft uneditable/unusable after creation.
- Removing the
- [P1] Enforce non-empty lists when editing scheduled campaigns (
cmd/campaigns.go:368-377)- List validation is now only done in
UpdateCampaignStatus, so a campaign that is alreadyscheduledcan be updated viaUpdateCampaignto clear all lists without triggering this check (no status transition). When the scheduler runs,store.NextSubscribers()builds list IDs fromcampaign_lists; with none it returnsnil, nil, causing the pipeline to treat it as “no subscribers” and mark the campaign finished with 0 sends (silent failure) and also makes later permission checks fail because the campaign no longer belongs to any list.
- List validation is now only done in
Summary
Total issues: 2 critical, 0 important, 0 minor.
Overall Verdict
Status: Patch has blocking issues
Explanation: The change allows campaigns to exist without any list association, but the authorization model and the sending pipeline assume campaigns belong to at least one list. This can make drafts inaccessible to non-admin users and allows scheduled campaigns to be emptied of lists and then silently complete with zero recipients.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 30 turns, 29 tool calls, 2m 45s
- Tokens: in
22.8K| cached363.6K| out6.7K(total393.2K) - Cost:
$0.1972
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
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.
Lists are now only required when starting or scheduling a campaign, not when saving a draft. This unblocks the content editor tab for non-admin users who previously could not save a campaign (and thus access the editor) without first selecting a list.
Fixes #3014