Conversation
i7an
force-pushed
the
MT-19861-new-user-management
branch
from
8fc72ae to
191ee05
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds a new Account Accesses feature: a DTO (AccountAccess), an API client class (AccountAccessesAPI) with list and delete operations, VCR fixtures and RSpec tests, an example script, and small README/CHANGELOG documentation updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AccountAccessesAPI
participant BaseAPI as Base API Class
participant Server as Mailtrap Server
rect rgba(100, 150, 255, 0.5)
Note over Client,Server: List Account Accesses Flow
Client->>AccountAccessesAPI: list(domain_ids, inbox_ids, project_ids)
AccountAccessesAPI->>AccountAccessesAPI: build query_params (non-empty filters)
AccountAccessesAPI->>BaseAPI: base_list(query_params)
BaseAPI->>Server: GET /api/accounts/{account_id}/account_accesses?filters
Server-->>BaseAPI: 200 OK [AccountAccess array]
BaseAPI-->>AccountAccessesAPI: parsed response
AccountAccessesAPI-->>Client: [AccountAccess objects]
end
rect rgba(255, 150, 100, 0.5)
Note over Client,Server: Delete Account Access Flow
Client->>AccountAccessesAPI: delete(account_access_id)
AccountAccessesAPI->>BaseAPI: base_delete(account_access_id)
BaseAPI->>Server: DELETE /api/accounts/{account_id}/account_accesses/{id}
Server-->>BaseAPI: 200 OK {id}
BaseAPI-->>AccountAccessesAPI: response hash
AccountAccessesAPI-->>Client: {id} hash
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
i7an
approved these changes
Mar 2, 2026
![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
🧹 Nitpick comments (2)
spec/mailtrap/account_access_spec.rb (1)
28-28: Polish test description grammar.Line 28 should read “creates an account_access with all attributes” for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/mailtrap/account_access_spec.rb` at line 28, The test description string in the RSpec example "it 'creates a account_access with all attributes' do" has incorrect article usage; update that string to "it 'creates an account_access with all attributes' do" (and fix any identical occurrences) so the spec description reads correctly.lib/mailtrap/account_accesses_api.rb (1)
14-32: YARD docs are stale and currently misleading.The comments still refer to “sandbox message,” and the return annotations don’t match these methods’ actual semantics.
📝 Suggested doc update
- # `@return` [AccountAccess] Sandbox message object + # `@return` [Array<AccountAccess>] Account access objects @@ - # Deletes a sandbox message + # Deletes an account access @@ - # `@return` [AccountAccess] Deleted AccountAccess object + # `@return` [Hash] Deleted account access payload (e.g., `{ id: ... }`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mailtrap/account_accesses_api.rb` around lines 14 - 32, The YARD comments are stale: update the docblock for the list method (and the following delete/account access block) to remove the incorrect "sandbox message" wording, correct the parameter and return annotations to reflect the actual semantics (e.g., list returns an Array of AccountAccess objects or a collection, not a sandbox message), and ensure the delete docblock references the correct parameter name account_access_id and returns a single AccountAccess. Locate the list method and its use of base_list, and the delete/account_access_id docblock, then replace misleading text and return types with accurate descriptions like "Array<AccountAccess>" for list and "AccountAccess" for delete.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 1-3: The changelog entry "Add Account Accesses API" is outside the
2.7.0 section; move that bullet so it appears immediately under the "## [2.7.0]
- 2026-02-24" heading in CHANGELOG.md (i.e., cut the line containing "Add
Account Accesses API" and paste it as a new bullet under the "## [2.7.0] -
2026-02-24" section).
In
`@spec/fixtures/vcr_cassettes/Mailtrap_AccountAccessesAPI/_list/maps_response_data_to_AccountAccesses_objects.yml`:
- Around line 71-73: The fixture string value contains real PII
("welcome@rw.com" and "Random User") — replace those literals inside the YAML
string value (the "string" key) with synthetic placeholders (e.g.
"[REDACTED_EMAIL]" and "[REDACTED_NAME]") so the response body no longer
contains identifiable data, and ensure any existing VCR filtering logic that
masks these fields is preserved or adjusted to target the new placeholders (keep
the "string" field and its JSON structure intact while changing only the
email/name tokens).
In `@spec/mailtrap/account_accesses_api_spec.rb`:
- Around line 38-42: The not-found test sets the wrong variable: the context
overrides project_id but the deletion uses account_access_id, so the test never
actually changes the ID being deleted; update the context to set
let(:account_access_id) { 999_999 } (or set both project_id and
account_access_id to a non-existent value) so that the delete call in the spec
exercises the not-found branch for the account_access_id used by the delete
helper.
---
Nitpick comments:
In `@lib/mailtrap/account_accesses_api.rb`:
- Around line 14-32: The YARD comments are stale: update the docblock for the
list method (and the following delete/account access block) to remove the
incorrect "sandbox message" wording, correct the parameter and return
annotations to reflect the actual semantics (e.g., list returns an Array of
AccountAccess objects or a collection, not a sandbox message), and ensure the
delete docblock references the correct parameter name account_access_id and
returns a single AccountAccess. Locate the list method and its use of base_list,
and the delete/account_access_id docblock, then replace misleading text and
return types with accurate descriptions like "Array<AccountAccess>" for list and
"AccountAccess" for delete.
In `@spec/mailtrap/account_access_spec.rb`:
- Line 28: The test description string in the RSpec example "it 'creates a
account_access with all attributes' do" has incorrect article usage; update that
string to "it 'creates an account_access with all attributes' do" (and fix any
identical occurrences) so the spec description reads correctly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CHANGELOG.mdREADME.mdexamples/account_accesses_api.rblib/mailtrap.rblib/mailtrap/account_access.rblib/mailtrap/account_accesses_api.rbspec/fixtures/vcr_cassettes/Mailtrap_AccountAccessesAPI/_delete/returns_deleted_account_access_id.ymlspec/fixtures/vcr_cassettes/Mailtrap_AccountAccessesAPI/_delete/when_account_access_does_not_exist/raises_not_found_error.ymlspec/fixtures/vcr_cassettes/Mailtrap_AccountAccessesAPI/_list/maps_response_data_to_AccountAccesses_objects.ymlspec/fixtures/vcr_cassettes/Mailtrap_AccountAccessesAPI/_list/when_api_key_is_incorrect/raises_authorization_error.ymlspec/mailtrap/account_access_spec.rbspec/mailtrap/account_accesses_api_spec.rb
Comment on lines
+71
to
+73
| string: '[{"id":5190393,"specifier_type":"Invite","resources":[{"resource_type":"account","resource_id":2326475,"access_level":10}],"specifier":{"id":82784,"email": | ||
| "welcome@rw.com"},"permissions":{"can_read":false,"can_update":true,"can_destroy":true,"can_leave":false}},{"id":4717970,"specifier_type":"User","resources":[{"resource_type":"organization","resource_id":1888116,"access_level":1000}],"specifier":{"id":2337943,"email": | ||
| "welcome@rw.com","name":"Random User","two_factor_authentication_enabled":true},"permissions":{"can_read":false,"can_update":false,"can_destroy":false,"can_leave":false}}]' |
There was a problem hiding this comment.
Redact recorded user identity fields in fixture payload.
Lines 71-73 include an email and a person name in the response body. Please replace with synthetic placeholders (and keep VCR filtering in place) to avoid committing identifiable data.
As per coding guidelines, "spec/fixtures/vcr_cassettes/**/*.yml: Act as a data privacy officer... try to identify sensitive data that could potentially be recorded."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@spec/fixtures/vcr_cassettes/Mailtrap_AccountAccessesAPI/_list/maps_response_data_to_AccountAccesses_objects.yml`
around lines 71 - 73, The fixture string value contains real PII
("welcome@rw.com" and "Random User") — replace those literals inside the YAML
string value (the "string" key) with synthetic placeholders (e.g.
"[REDACTED_EMAIL]" and "[REDACTED_NAME]") so the response body no longer
contains identifiable data, and ensure any existing VCR filtering logic that
masks these fields is preserved or adjusted to target the new placeholders (keep
the "string" field and its JSON structure intact while changing only the
email/name tokens).
There was a problem hiding this comment.
Pull request overview
Adds a new Account Accesses API to the mailtrap-ruby client library, enabling consumers to list and delete account access records and providing the corresponding DTO, examples, and VCR-backed specs.
Changes:
- Introduce
Mailtrap::AccountAccessesAPIwithlist(supports domain/inbox/project filters) anddelete. - Add
Mailtrap::AccountAccessDTO and VCR-backed RSpec coverage for API + DTO. - Update README/CHANGELOG and add an example script.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/mailtrap/account_accesses_api_spec.rb | Adds VCR-backed specs for listing and deleting account accesses. |
| spec/mailtrap/account_access_spec.rb | Adds DTO initialization/spec coverage for AccountAccess. |
| spec/fixtures/vcr_cassettes/Mailtrap_AccountAccessesAPI/_list/when_api_key_is_incorrect/raises_authorization_error.yml | Records 401 response for invalid token behavior. |
| spec/fixtures/vcr_cassettes/Mailtrap_AccountAccessesAPI/_list/maps_response_data_to_AccountAccesses_objects.yml | Records successful list response mapping to DTOs. |
| spec/fixtures/vcr_cassettes/Mailtrap_AccountAccessesAPI/_delete/when_account_access_does_not_exist/raises_not_found_error.yml | Records 404 response for delete error path. |
| spec/fixtures/vcr_cassettes/Mailtrap_AccountAccessesAPI/_delete/returns_deleted_account_access_id.yml | Records successful delete response. |
| lib/mailtrap/account_accesses_api.rb | Implements Account Accesses API wrapper with list/delete and query filtering. |
| lib/mailtrap/account_access.rb | Adds AccountAccess struct DTO. |
| lib/mailtrap.rb | Wires the new API into the main require tree. |
| examples/account_accesses_api.rb | Adds usage examples for list/filter/delete operations. |
| README.md | Adds the new example/API to the list of available examples. |
| CHANGELOG.md | Notes the new Account Accesses API addition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/mailtrap/account_accesses_api.rb
Outdated
| # @param domain_ids [Array<Integer>] Optional array of domain IDs to filter by | ||
| # @param inbox_ids [Array<Integer>] Optional array of inbox IDs to filter by | ||
| # @param project_ids [Array<Integer>] Optional array of project IDs to filter by | ||
| # @return [AccountAccess] Sandbox message object |
There was a problem hiding this comment.
The YARD docs for #list have an incorrect return type/description (it returns an array of AccountAccess objects via base_list, not a single object, and it refers to a “Sandbox message”). Please update the @return and description to match the actual behavior.
Suggested change
| # @return [AccountAccess] Sandbox message object | |
| # @return [Array<AccountAccess>] List of account access objects |
lib/mailtrap/account_accesses_api.rb
Outdated
Comment on lines
+29
to
+35
| # Deletes a sandbox message | ||
| # @param account_access_id [Integer] The account access ID | ||
| # @return [AccountAccess] Deleted AccountAccess object | ||
| # @!macro api_errors | ||
| def delete(account_access_id) | ||
| base_delete(account_access_id) | ||
| end |
There was a problem hiding this comment.
The #delete documentation refers to deleting a “sandbox message” and claims it returns a deleted AccountAccess object. BaseAPI#base_delete returns the raw parsed response from Client#delete (e.g., a Hash like { id: ... }), not an AccountAccess struct. Please align the docs (and/or implementation) with the actual return value.
examples/account_accesses_api.rb
Outdated
| # Delete an account access | ||
| account_access_id = 123 | ||
| account_accesses.delete(account_access_id) | ||
| # => #<struct Mailtrap::AccountAccess id=123, ...> (deleted) |
There was a problem hiding this comment.
This example suggests delete returns an AccountAccess struct, but BaseAPI#base_delete returns the parsed JSON response from Client#delete (typically a Hash like { id: account_access_id }). Please update the example output/comments to match the actual return type.
Suggested change
| # => #<struct Mailtrap::AccountAccess id=123, ...> (deleted) | |
| # => { "id" => 123 } # parsed JSON response indicating the deleted account access |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
spec/fixtures/vcr_cassettes/Mailtrap_AccountAccessesAPI/_list/maps_response_data_to_AccountAccesses_objects.yml (1)
71-73:⚠️ Potential issue | 🟠 MajorRedact recorded personal data in fixture response body.
Line 71 and Line 73 still contain identifiable user data (
welcome@rw.com,Random User) and should be replaced with synthetic placeholders.As per coding guidelines, "spec/fixtures/vcr_cassettes/**/*.yml: Act as a data privacy officer. Carefully read all the vcr cassettes with recorded HTTP interactions and try to identify sensitive data that could potentially be recorded."🔒 Suggested redaction patch
- string: '[{"id":5190393,"specifier_type":"Invite","resources":[{"resource_type":"account","resource_id":2326475,"access_level":10}],"specifier":{"id":82784,"email": - "welcome@rw.com"},"permissions":{"can_read":false,"can_update":true,"can_destroy":true,"can_leave":false}},{"id":4717970,"specifier_type":"User","resources":[{"resource_type":"organization","resource_id":1888116,"access_level":1000}],"specifier":{"id":2337943,"email": - "welcome@rw.com","name":"Random User","two_factor_authentication_enabled":true},"permissions":{"can_read":false,"can_update":false,"can_destroy":false,"can_leave":false}}]' + string: '[{"id":5190393,"specifier_type":"Invite","resources":[{"resource_type":"account","resource_id":2326475,"access_level":10}],"specifier":{"id":82784,"email": + "[REDACTED_EMAIL]"},"permissions":{"can_read":false,"can_update":true,"can_destroy":true,"can_leave":false}},{"id":4717970,"specifier_type":"User","resources":[{"resource_type":"organization","resource_id":1888116,"access_level":1000}],"specifier":{"id":2337943,"email": + "[REDACTED_EMAIL]","name":"[REDACTED_NAME]","two_factor_authentication_enabled":true},"permissions":{"can_read":false,"can_update":false,"can_destroy":false,"can_leave":false}}]'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/fixtures/vcr_cassettes/Mailtrap_AccountAccessesAPI/_list/maps_response_data_to_AccountAccesses_objects.yml` around lines 71 - 73, The fixture's recorded response string contains personally identifiable values ("welcome@rw.com" and "Random User") inside the YAML value for the key "string"; replace those real values with synthetic placeholders (for example "redacted@example.com" and "Redacted User" or similar) throughout the JSON in that "string" entry so the cassette no longer contains identifiable user data while preserving structure and types.spec/mailtrap/account_accesses_api_spec.rb (1)
38-40:⚠️ Potential issue | 🟠 MajorFix the not-found delete setup to override
account_access_id.Line 39 defines
project_id, butdeleteusesaccount_access_id, so this context does not reliably test the 404 path.Suggested fix
context 'when account access does not exist' do - let(:project_id) { 999_999 } + let(:account_access_id) { 999_999 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/mailtrap/account_accesses_api_spec.rb` around lines 38 - 40, The context intended to test the 404 delete path sets project_id but not the id used by the delete action; add an override for account_access_id (e.g. let(:account_access_id) { 999_999 }) inside the "when account access does not exist" context so the delete request uses a non-existent account_access_id; ensure the existing delete/request helper still references account_access_id so the spec exercises the not-found branch.CHANGELOG.md (1)
1-3:⚠️ Potential issue | 🟡 MinorMove the new changelog bullet under version 2.7.0.
Line 1 is outside any release section, so this note can be missed by tooling/readers.
Suggested fix
-- Add Account Accesses API - ## [2.7.0] - 2026-02-24 +- Add Account Accesses API - Add Sandbox Messages API🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 1 - 3, The changelog entry "Add Account Accesses API" is placed before any release header and should be moved under the "## [2.7.0] - 2026-02-24" section; update CHANGELOG.md by relocating the line "Add Account Accesses API" so it appears as a bullet (or appropriate entry) immediately beneath the "## [2.7.0] - 2026-02-24" heading, ensuring formatting matches other bullets in that release section.
🧹 Nitpick comments (1)
lib/mailtrap/account_accesses_api.rb (1)
33-35: Aligndeleteresponse handling withresponse_class.
listdeserializes intoAccountAccess, butdeletecurrently delegates straight tobase_delete. Consider normalizing the return shape for API consistency.♻️ Suggested consistency patch
def delete(account_access_id) - base_delete(account_access_id) + handle_response(base_delete(account_access_id)) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mailtrap/account_accesses_api.rb` around lines 33 - 35, The delete method currently forwards to base_delete and returns raw result; change it to normalize its return to the same shape as list by deserializing the response into the API's response_class (AccountAccess) before returning. Locate the delete method (def delete(account_access_id)) and replace the direct base_delete call with code that calls base_delete(account_access_id), then passes the result into the existing response_class deserialization helper (the same helper used by list) or otherwise constructs an AccountAccess instance from the response, ensuring the method returns the same model type as list.
🤖 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/mailtrap/account_accesses_api.rb`:
- Around line 14-19: The YARD for the AccountAccessesApi#list method is
incorrect: update the method comment to state it returns a collection of
AccountAccess objects (e.g., Array<AccountAccess> or AccountAccess[]) instead of
a single AccountAccess and remove the incorrect “Sandbox message” text; adjust
the `@return` tag and brief description above the list method in
lib/mailtrap/account_accesses_api.rb so it clearly says it returns a list/array
of account access objects and includes any relevant filtering behavior.
In `@spec/mailtrap/account_access_spec.rb`:
- Line 4: The spec constructs Mailtrap::AccountAccess with a positional hash
which fails because the class is defined with keyword_init: true; change the
construction in the subject from described_class.new(attributes) to pass
keywords by expanding the hash (use **attributes) so the initializer receives
keyword arguments for Mailtrap::AccountAccess.
---
Duplicate comments:
In `@CHANGELOG.md`:
- Around line 1-3: The changelog entry "Add Account Accesses API" is placed
before any release header and should be moved under the "## [2.7.0] -
2026-02-24" section; update CHANGELOG.md by relocating the line "Add Account
Accesses API" so it appears as a bullet (or appropriate entry) immediately
beneath the "## [2.7.0] - 2026-02-24" heading, ensuring formatting matches other
bullets in that release section.
In
`@spec/fixtures/vcr_cassettes/Mailtrap_AccountAccessesAPI/_list/maps_response_data_to_AccountAccesses_objects.yml`:
- Around line 71-73: The fixture's recorded response string contains personally
identifiable values ("welcome@rw.com" and "Random User") inside the YAML value
for the key "string"; replace those real values with synthetic placeholders (for
example "redacted@example.com" and "Redacted User" or similar) throughout the
JSON in that "string" entry so the cassette no longer contains identifiable user
data while preserving structure and types.
In `@spec/mailtrap/account_accesses_api_spec.rb`:
- Around line 38-40: The context intended to test the 404 delete path sets
project_id but not the id used by the delete action; add an override for
account_access_id (e.g. let(:account_access_id) { 999_999 }) inside the "when
account access does not exist" context so the delete request uses a non-existent
account_access_id; ensure the existing delete/request helper still references
account_access_id so the spec exercises the not-found branch.
---
Nitpick comments:
In `@lib/mailtrap/account_accesses_api.rb`:
- Around line 33-35: The delete method currently forwards to base_delete and
returns raw result; change it to normalize its return to the same shape as list
by deserializing the response into the API's response_class (AccountAccess)
before returning. Locate the delete method (def delete(account_access_id)) and
replace the direct base_delete call with code that calls
base_delete(account_access_id), then passes the result into the existing
response_class deserialization helper (the same helper used by list) or
otherwise constructs an AccountAccess instance from the response, ensuring the
method returns the same model type as list.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CHANGELOG.mdREADME.mdexamples/account_accesses_api.rblib/mailtrap.rblib/mailtrap/account_access.rblib/mailtrap/account_accesses_api.rbspec/fixtures/vcr_cassettes/Mailtrap_AccountAccessesAPI/_delete/returns_deleted_account_access_id.ymlspec/fixtures/vcr_cassettes/Mailtrap_AccountAccessesAPI/_delete/when_account_access_does_not_exist/raises_not_found_error.ymlspec/fixtures/vcr_cassettes/Mailtrap_AccountAccessesAPI/_list/maps_response_data_to_AccountAccesses_objects.ymlspec/fixtures/vcr_cassettes/Mailtrap_AccountAccessesAPI/_list/when_api_key_is_incorrect/raises_authorization_error.ymlspec/mailtrap/account_access_spec.rbspec/mailtrap/account_accesses_api_spec.rb
| # frozen_string_literal: true | ||
|
|
||
| RSpec.describe Mailtrap::AccountAccess do | ||
| subject(:account_access) { described_class.new(attributes) } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if Mailtrap::AccountAccess uses keyword_init: true
fd "account_access.rb" --type f | head -5Repository: mailtrap/mailtrap-ruby
Length of output: 96
🏁 Script executed:
# Search for the AccountAccess struct definition with keyword_init
rg -n "Struct\.new" lib/mailtrap/ -A 3Repository: mailtrap/mailtrap-ruby
Length of output: 2500
🏁 Script executed:
# Check the actual test file to see the current code
cat spec/mailtrap/account_access_spec.rb | head -20Repository: mailtrap/mailtrap-ruby
Length of output: 550
🏁 Script executed:
cat -n lib/mailtrap/account_access.rbRepository: mailtrap/mailtrap-ruby
Length of output: 831
Use keyword splat when constructing Mailtrap::AccountAccess.
Mailtrap::AccountAccess is defined with keyword_init: true (line 18 of lib/mailtrap/account_access.rb). Line 4 passes a positional hash, which will raise ArgumentError on Ruby 3. Expand the hash as keyword arguments using **.
Suggested fix
- subject(:account_access) { described_class.new(attributes) }
+ subject(:account_access) { described_class.new(**attributes) }📝 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
| subject(:account_access) { described_class.new(attributes) } | |
| subject(:account_access) { described_class.new(**attributes) } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/mailtrap/account_access_spec.rb` at line 4, The spec constructs
Mailtrap::AccountAccess with a positional hash which fails because the class is
defined with keyword_init: true; change the construction in the subject from
described_class.new(attributes) to pass keywords by expanding the hash (use
**attributes) so the initializer receives keyword arguments for
Mailtrap::AccountAccess.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
spec/mailtrap/account_access_spec.rb (1)
4-4:⚠️ Potential issue | 🟠 MajorUse keyword splat when constructing
Mailtrap::AccountAccess.This passes a positional hash; with
keyword_init: trueit can raiseArgumentError. Expand as keywords.Suggested fix
- subject(:account_access) { described_class.new(attributes) } + subject(:account_access) { described_class.new(**attributes) }#!/bin/bash set -euo pipefail # Verify DTO initializer mode and spec call site cat -n lib/mailtrap/account_access.rb | sed -n '1,120p' cat -n spec/mailtrap/account_access_spec.rb | sed -n '1,30p'Expected verification result:
lib/mailtrap/account_access.rbshowskeyword_init: true, while current spec line uses positional hash.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/mailtrap/account_access_spec.rb` at line 4, The spec constructs Mailtrap::AccountAccess with a positional hash which can fail if the DTO uses keyword_init: true; update the subject declaration so described_class.new is invoked with keyword splat of attributes (i.e., pass **attributes) instead of a positional hash. Change the subject(:account_access) line that calls described_class.new(attributes) to use keyword expansion for Mailtrap::AccountAccess construction so it matches the initializer expectations.
🧹 Nitpick comments (1)
spec/mailtrap/account_accesses_api_spec.rb (1)
38-46: The not-found test context doesn't overrideaccount_access_id.The test uses the same
account_access_id(5_190_393) for both success and not-found scenarios. While VCR cassette selection by context name makes this work, semantically the not-found case should use a different ID to clearly express intent and avoid confusion.✏️ Suggested fix
context 'when account access does not exist' do + let(:account_access_id) { 999_999 } + it 'raises not found error' doNote: The VCR cassette path would also need updating to match the new ID if changed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/mailtrap/account_accesses_api_spec.rb` around lines 38 - 46, The failing test context "when account access does not exist" should override the shared account_access_id used elsewhere; change the id used in that context (the variable/account_access_id referenced by the delete helper) to a different value that represents a non-existent resource (e.g., a new integer constant) and update any related VCR cassette name/path to match the new ID so the cassette selection remains correct; locate references to account_access_id and the delete helper in this spec and modify only that context's setup to set a different id.
🤖 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/mailtrap/account_accesses_api.rb`:
- Around line 29-32: Update the YARD return description for the delete method in
account_accesses_api.rb (the method that deletes an account access) to reflect
the actual response shape: instead of "Deleted AccountAccess object" state that
the endpoint returns a Hash containing the deleted id (e.g., a hash with the
deleted id as an Integer). Adjust the `@return` line for the delete method to
describe a Hash with the deleted id key and its type so docs match the real
response.
---
Duplicate comments:
In `@spec/mailtrap/account_access_spec.rb`:
- Line 4: The spec constructs Mailtrap::AccountAccess with a positional hash
which can fail if the DTO uses keyword_init: true; update the subject
declaration so described_class.new is invoked with keyword splat of attributes
(i.e., pass **attributes) instead of a positional hash. Change the
subject(:account_access) line that calls described_class.new(attributes) to use
keyword expansion for Mailtrap::AccountAccess construction so it matches the
initializer expectations.
---
Nitpick comments:
In `@spec/mailtrap/account_accesses_api_spec.rb`:
- Around line 38-46: The failing test context "when account access does not
exist" should override the shared account_access_id used elsewhere; change the
id used in that context (the variable/account_access_id referenced by the delete
helper) to a different value that represents a non-existent resource (e.g., a
new integer constant) and update any related VCR cassette name/path to match the
new ID so the cassette selection remains correct; locate references to
account_access_id and the delete helper in this spec and modify only that
context's setup to set a different id.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CHANGELOG.mdREADME.mdexamples/account_accesses_api.rblib/mailtrap.rblib/mailtrap/account_access.rblib/mailtrap/account_accesses_api.rbspec/fixtures/vcr_cassettes/Mailtrap_AccountAccessesAPI/_delete/returns_deleted_account_access_id.ymlspec/fixtures/vcr_cassettes/Mailtrap_AccountAccessesAPI/_delete/when_account_access_does_not_exist/raises_not_found_error.ymlspec/fixtures/vcr_cassettes/Mailtrap_AccountAccessesAPI/_list/maps_response_data_to_AccountAccesses_objects.ymlspec/fixtures/vcr_cassettes/Mailtrap_AccountAccessesAPI/_list/when_api_key_is_incorrect/raises_authorization_error.ymlspec/mailtrap/account_access_spec.rbspec/mailtrap/account_accesses_api_spec.rb
i7an
force-pushed
the
MT-19861-new-user-management
branch
from
7ce654b to
0564ac3
Compare
IgorDobryn
approved these changes
Mar 2, 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
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.
Motivation
Adding new Account Accesses API
Changes
Summary by CodeRabbit
New Features
Documentation
Tests