Fix stats grouped commands: response types and query param#2
Open
solovey23 wants to merge 2 commits intomailtrap:mainfrom
Open
Fix stats grouped commands: response types and query param#2solovey23 wants to merge 2 commits intomailtrap:mainfrom
solovey23 wants to merge 2 commits intomailtrap:mainfrom
Conversation
…fields
- tokens/create.go: remove erroneous 'api_token' wrapper from POST body;
API expects flat {"name": ..., "resources": [...]} at the top level
- contact_fields/create.go: add required --merge-tag flag (merge_tag is
required by the Mailtrap API but was not exposed in the CLI)
- Update tests for both commands to reflect corrected behaviour; add
TestContactFieldsCreateMissingMergeTag coverage
- Update README, TEST_PLAN, and skill reference docs to match
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add proper nested response types (DomainStats, CategoryStats, ESPStats, DateStats) that match the API's wrapped format where stats are under a "stats" key alongside the grouping field - Fix query parameter name from "streams[]" to "sending_streams[]" per API spec - Add grouping-key columns (domain ID, category, ESP, date) to output - Update test mock responses to use the correct nested API format Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe changes update three main CLI command areas: contact fields now require a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/commands/stats/get.go (1)
221-223:⚠️ Potential issue | 🟡 MinorUpdate
streams[]parameter tosending_streams[]for consistency with grouped stats endpoints.All grouped stats commands (
by-domain,by-category,by-esp,by-date) consistently usesending_streams[]as the parameter name. The basestats getcommand still uses the outdatedstreams[]parameter, creating an inconsistency. This should be corrected to match the grouped stats endpoints.Proposed fix
for _, s := range opts.Streams { - params.Add("streams[]", s) + params.Add("sending_streams[]", s) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/stats/get.go` around lines 221 - 223, The loop adding stream params uses the wrong parameter name; in internal/commands/stats/get.go locate the loop that iterates over opts.Streams and change the params.Add call from "streams[]" to "sending_streams[]" so the base stats get command matches grouped endpoints (update the params.Add("streams[]", s) invocation referenced in that loop to params.Add("sending_streams[]", s)).
🧹 Nitpick comments (3)
skills/mailtrap-cli/references/contacts.md (1)
193-193: Consider listing allowed--data-typevalues in update docs too.Line 193 currently says “New data type” only; mirroring the enum from create improves consistency and reduces guesswork.
Suggested doc refinement
-| `--data-type` | string | No | New data type | +| `--data-type` | string | No | New data type: `text`, `integer`, `float`, `boolean`, `date` |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/mailtrap-cli/references/contacts.md` at line 193, The docs for the update endpoint currently list `--data-type` as "New data type" only; update the contacts update docs to enumerate the allowed `--data-type` values (the same enum used in the create docs) so users see valid options. Find the `--data-type` entry in the update reference and replace the vague "New data type" with the explicit list of allowed values (matching the create/contacts create enum), keeping the same formatting as the create docs for consistency.internal/commands/contact_fields/create.go (1)
62-62: Align--merge-taghelp text with update command for consistency.Line 62 can include the same constraint wording used elsewhere (e.g., max length) to keep CLI UX consistent.
Suggested docs/help-text tweak
-cmd.Flags().StringVar(&mergeTag, "merge-tag", "", "Merge tag for the field (required)") +cmd.Flags().StringVar(&mergeTag, "merge-tag", "", "Merge tag for the field (max 80 chars) (required)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/contact_fields/create.go` at line 62, Update the help string for the --merge-tag flag in create command to match the update command's wording (include the same constraint wording such as max length); edit the call that registers the flag (StringVar(&mergeTag, "merge-tag", "", "...")) so the third argument description mirrors the update command's help text exactly, referencing the same constraints used there to keep CLI UX consistent.internal/commands/tokens/tokens_test.go (1)
179-188: Strengthen payload contract assertions forresourcesentries.You already verify array presence/size; adding field-level checks (
resource_type,resource_id,access_level) would better protect against silent shape regressions.Suggested assertion expansion
if len(resources) != 1 { t.Errorf("expected 1 resource, got %d", len(resources)) } + resource, ok := resources[0].(map[string]interface{}) + if !ok { + t.Fatal("expected first resource to be an object") + } + if resource["resource_type"] != "account" { + t.Errorf("expected resource_type 'account', got %v", resource["resource_type"]) + } + if resource["resource_id"] != float64(123) { + t.Errorf("expected resource_id 123, got %v", resource["resource_id"]) + } + if resource["access_level"] != float64(100) { + t.Errorf("expected access_level 100, got %v", resource["access_level"]) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/tokens/tokens_test.go` around lines 179 - 188, Extend the test's assertions after converting payload["resources"] to resources to validate each resource object's shape: cast resources[0] to map[string]interface{} (or iterate over resources), assert the presence and types of the keys "resource_type", "resource_id", and "access_level" (e.g., non-empty strings or expected enum values), and fail the test if any key is missing or has an unexpected type/value; update the test around the payload/resources checks in tokens_test.go to perform these field-level assertions so regressions in resource shape are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/commands/stats/get.go`:
- Around line 221-223: The loop adding stream params uses the wrong parameter
name; in internal/commands/stats/get.go locate the loop that iterates over
opts.Streams and change the params.Add call from "streams[]" to
"sending_streams[]" so the base stats get command matches grouped endpoints
(update the params.Add("streams[]", s) invocation referenced in that loop to
params.Add("sending_streams[]", s)).
---
Nitpick comments:
In `@internal/commands/contact_fields/create.go`:
- Line 62: Update the help string for the --merge-tag flag in create command to
match the update command's wording (include the same constraint wording such as
max length); edit the call that registers the flag (StringVar(&mergeTag,
"merge-tag", "", "...")) so the third argument description mirrors the update
command's help text exactly, referencing the same constraints used there to keep
CLI UX consistent.
In `@internal/commands/tokens/tokens_test.go`:
- Around line 179-188: Extend the test's assertions after converting
payload["resources"] to resources to validate each resource object's shape: cast
resources[0] to map[string]interface{} (or iterate over resources), assert the
presence and types of the keys "resource_type", "resource_id", and
"access_level" (e.g., non-empty strings or expected enum values), and fail the
test if any key is missing or has an unexpected type/value; update the test
around the payload/resources checks in tokens_test.go to perform these
field-level assertions so regressions in resource shape are caught.
In `@skills/mailtrap-cli/references/contacts.md`:
- Line 193: The docs for the update endpoint currently list `--data-type` as
"New data type" only; update the contacts update docs to enumerate the allowed
`--data-type` values (the same enum used in the create docs) so users see valid
options. Find the `--data-type` entry in the update reference and replace the
vague "New data type" with the explicit list of allowed values (matching the
create/contacts create enum), keeping the same formatting as the create docs for
consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47ba3dec-9361-4751-97db-9d416b556eee
📒 Files selected for processing (14)
README.mddocs/TEST_PLAN.mdinternal/commands/contact_fields/contact_fields_test.gointernal/commands/contact_fields/create.gointernal/commands/stats/by_category.gointernal/commands/stats/by_date.gointernal/commands/stats/by_domain.gointernal/commands/stats/by_esp.gointernal/commands/stats/get.gointernal/commands/stats/stats_test.gointernal/commands/tokens/create.gointernal/commands/tokens/tokens_test.goskills/mailtrap-cli/references/accounts.mdskills/mailtrap-cli/references/contacts.md
leonid-shevtsov
approved these changes
Apr 17, 2026
IgorDobryn
approved these changes
Apr 17, 2026
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
3 participants
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
stats by-domain,by-category,by-esp,by-datecommands that returned empty/zero values due to wrong response types — the API wraps stats under a nested"stats"key, but the CLI was deserializing into flat structsDomainStats,CategoryStats,ESPStats,DateStats) with grouping-key columns (domain ID, category, ESP, date) in the outputstreams[]tosending_streams[]per API specTest plan
go test ./...)mailtrap stats by-domain --start-date 2024-01-01 --end-date 2024-12-31returns rows with domain IDs and statsmailtrap stats by-category,by-esp,by-datesimilarly show grouping keys--streamsflag filters correctly (now sendssending_streams[]to API)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--merge-tagflag for contact field creation to specify merge tag templates.--data-typeflag for contact fields with supported values: text, integer, float, boolean, date.Bug Fixes
Documentation