MT-22401: Add email campaigns commands - #9
Conversation
Decisions: - Request bodies are flat (no email_campaign wrapper); nested reply_to/template_attributes are assembled from flat flags - Single, list, and stats responses unwrap the data envelope; list pagination metadata is ignored for output - Endpoint is token-scoped (/api/email_campaigns) — no account-id requirement, unlike other resources - Lifecycle actions (start/schedule/cancel/terminate/reset) are subcommands sharing one body-less POST helper - Delete handles 204 No Content with a confirmation message per repo convention
📝 WalkthroughWalkthroughAdds a new ChangesEmail campaigns
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant EmailCampaignCommands
participant APIClient
participant EmailCampaignAPI
participant Output
CLI->>EmailCampaignCommands: execute campaign command
EmailCampaignCommands->>APIClient: create authenticated client
APIClient->>EmailCampaignAPI: request campaign resource or action
EmailCampaignAPI-->>APIClient: campaign or statistics response
APIClient-->>EmailCampaignCommands: decoded response
EmailCampaignCommands->>Output: render configured output
Output-->>CLI: formatted result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@internal/commands/emailcampaigns/list.go`:
- Line 121: Update the perPage flag binding in the campaign list command to use
50 as its default, matching the advertised help text, while preserving the
existing Changed check and surrounding behavior.
In `@internal/commands/emailcampaigns/start.go`:
- Around line 14-33: Update the API request flow in newLifecycleCmd and the
underlying internal/client.Client configuration so Email Campaigns calls cannot
block indefinitely. Replace the unbounded context.Background() request context
with an appropriate deadline or configure the HTTP client with a suitable
timeout, preserving the existing campaignPath and response handling.
In `@internal/commands/emailcampaigns/update.go`:
- Around line 29-32: Update the email-campaign update flow around
buildAttributesBody and the Patch call to validate that at least one mutable
attribute was provided; return a validation error before issuing Patch when the
body contains no changes. Add a regression test covering an update with only
--id and verify no request is sent.
In `@README.md`:
- Line 129: Update the README schedule command’s --datetime value to a future or
clearly placeholder timestamp, and ensure the corresponding Mailtrap app
examples include a matching schedule example.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b5236ce-3368-40c0-9482-c3adc59cede6
📒 Files selected for processing (16)
README.mdcmd/root.gointernal/commands/emailcampaigns/attributes.gointernal/commands/emailcampaigns/cancel.gointernal/commands/emailcampaigns/create.gointernal/commands/emailcampaigns/delete.gointernal/commands/emailcampaigns/emailcampaigns.gointernal/commands/emailcampaigns/emailcampaigns_test.gointernal/commands/emailcampaigns/get.gointernal/commands/emailcampaigns/list.gointernal/commands/emailcampaigns/reset.gointernal/commands/emailcampaigns/schedule.gointernal/commands/emailcampaigns/start.gointernal/commands/emailcampaigns/stats.gointernal/commands/emailcampaigns/terminate.gointernal/commands/emailcampaigns/update.go
| func newLifecycleCmd(f *cmdutil.Factory, action, short string) *cobra.Command { | ||
| var campaignID string | ||
|
|
||
| cmd := &cobra.Command{ | ||
| Use: action, | ||
| Short: short, | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| if err := cmdutil.RequireFlag("id", campaignID); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| c, err := f.NewClient() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| var resp campaignResponse | ||
| if err := c.Post(context.Background(), client.BaseGeneral, campaignPath(campaignID, action), nil, &resp); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether internal/client.Client sets a default request/dial timeout.
ast-grep outline internal/client --items all
rg -n -A5 -B5 'Timeout' internal/clientRepository: mailtrap/mailtrap-cli
Length of output: 1158
🏁 Script executed:
#!/bin/bash
set -u
printf '--- client.go ---\n'
cat -n internal/client/client.go
printf '\n--- relevant command timeout context usages ---\n'
rg -n 'context\.Background|context\.With|Timeout|Deadline|ctx' internal/client internal/commands/emailcampaignsRepository: mailtrap/mailtrap-cli
Length of output: 10909
🌐 Web query:
Go net/http New default http.Client Timeout documentation
💡 Result:
The Go net/http package does not impose a default timeout on the standard http.Client [1][2]. The http.DefaultClient is initialized with a zero value for its Timeout field, which explicitly means "no timeout" [1][3]. In the Go documentation, the Timeout field of the http.Client struct is defined as follows [1][4]: Timeout specifies a time limit for requests made by this Client. The timeout includes connection time, any redirects, and reading the response body. A Timeout of zero means no timeout. Because http.DefaultClient has no timeout, using it or the convenience functions that rely on it (such as http.Get, http.Post, and http.Head) is generally discouraged for production applications, especially those interacting with the public internet, as it can lead to resource leaks and hung requests [5][6]. Developers are encouraged to define a custom http.Client with a specified Timeout or to use context.Context with requests to enforce deadlines [7][5].
Citations:
- 1: https://pkg.go.dev/net/http
- 2: https://go.dev/src/net/http/client.go?m=text
- 3: https://go.googlesource.com/go/+/master/src/net/http/client.go
- 4: https://tip.golang.org/pkg/http
- 5: https://blog.cloudflare.com/the-complete-guide-to-golang-net-http-timeouts/
- 6: https://petr-korobeinikov.com/posts/go-http-defaultclient/
- 7: https://stackoverflow.com/questions/16895294/how-to-set-timeout-for-http-get-requests-in-golang
Add timeout coverage for context.Background() API calls.
internal/client.Client creates &http.Client{} with no Timeout, and these commands pass context.Background() by value, so Email Campaigns requests can block indefinitely if the API response hangs. Add a request deadline or configure the HTTP client with an appropriate timeout.
🤖 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 `@internal/commands/emailcampaigns/start.go` around lines 14 - 33, Update the
API request flow in newLifecycleCmd and the underlying internal/client.Client
configuration so Email Campaigns calls cannot block indefinitely. Replace the
unbounded context.Background() request context with an appropriate deadline or
configure the HTTP client with a suitable timeout, preserving the existing
campaignPath and response handling.
| body := buildAttributesBody(cmd, attrs) | ||
|
|
||
| var resp campaignResponse | ||
| if err := c.Patch(context.Background(), client.BaseGeneral, campaignPath(campaignID), body, &resp); err != nil { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject updates with no changed attributes.
email-campaigns update --id 4567 builds an empty partial-update body and still issues PATCH. Return a validation error before the request when no mutable attribute flag was provided, and add a regression test.
🤖 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 `@internal/commands/emailcampaigns/update.go` around lines 29 - 32, Update the
email-campaign update flow around buildAttributesBody and the Patch call to
validate that at least one mutable attribute was provided; return a validation
error before issuing Patch when the body contains no changes. Add a regression
test covering an update with only --id and verify no request is sent.
Decisions: - Bind --per-page default to 50 (matches API default per OpenAPI spec); Changed check kept so the param is only sent when set explicitly - README schedule example now uses an obviously-future timestamp (2030-01-01) - No timeout added for context.Background() calls: matches repo convention (no command uses timeouts); a CLI-wide timeout belongs in a separate change - Empty updates not rejected: templates/webhooks update commands also send empty partial PATCH; consistency kept
|
Addressed CodeRabbit findings in 06ed4f5: Fixed
Skipped
|
Decisions: - The API treats contact_list_ids/contact_segment_ids as the full set, so an explicit [] clears them — pflag cannot parse an empty slice value, so dedicated --clear-contact-lists/--clear-contact-segments booleans express it; mutually exclusive with the ids flags
|
Follow-up from a cross-SDK contract audit: the API treats audience ids as the full set (explicit |
1 participant
Motivation
MT-22401
Port the Email Campaigns public API (MT-21113) to the CLI.
Changes
mailtrap email-campaignswith 12 subcommands:list(--per-page/--search/--token),get,create,update(PATCH semantics — only changed flags are sent),delete, the five lifecycle actions (start,schedule --datetime,cancel,terminate,reset), andstats(--start-date/--end-date)data-envelope unwrapping, UUID--mailsend-domain-id, nestedreply_to/template_attributes/delivery_optionsbuilt from flat flags, audience id slicesHow to test
mailtrap email-campaigns create --name "Test" --mailsend-domain-id <uuid> --from-local-part news --subject "Hi"with a real token — expect a draft campaign rowschedule --datetime <future ISO>→cancel→start→terminate; verify state transitions and that invalid transitions print the API erroremail-campaigns list --search <name>andstats <id> --start-date ... --end-date ...return sensible tables and--output jsonparsesSummary by CodeRabbit