Open
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive Email Logs API feature, adding data models, list/fetch endpoints with filtering and pagination support, client integration, documentation updates, and unit tests alongside a version bump to 2.5.0 and GitHub repository namespace migration. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant MCL as MailtrapClient
participant ELBase as EmailLogsBaseApi
participant EL as EmailLogsApi
participant HTTP as HttpClient
participant API as Email Logs API
Client->>MCL: email_logs_api (property)
MCL->>MCL: _validate_account_id("Email Logs API")
MCL->>ELBase: EmailLogsBaseApi(client, account_id)
ELBase-->>MCL: instance
MCL-->>Client: EmailLogsBaseApi
Client->>ELBase: email_logs (property)
ELBase->>EL: EmailLogsApi(client, account_id)
EL-->>ELBase: instance
ELBase-->>Client: EmailLogsApi
Client->>EL: get_list(filters, search_after)
EL->>EL: filters.to_params() if filters exist
EL->>HTTP: GET /accounts/{id}/emails?params
HTTP->>API: HTTP GET request
API-->>HTTP: {messages: [...], total_count, next_page_cursor}
EL->>EL: Transform messages to EmailLogMessage instances
EL-->>Client: EmailLogsListResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/unit/api/general/test_stats.py (1)
165-169: Assert the serialized list values, not just the key names.These checks still pass if the request encodes each list under the right key but with the wrong shape. Since this PR adds list-param serialization, it would be better to assert the exact repeated values in the query string.
Proposed test tightening
+from urllib.parse import parse_qs +from urllib.parse import urlparse + from typing import Any import pytest import responses @@ - request_params = responses.calls[0].request.params - assert "sending_domain_ids[]" in request_params - assert "sending_streams[]" in request_params - assert "categories[]" in request_params - assert "email_service_providers[]" in request_params + request_params = parse_qs(urlparse(responses.calls[0].request.url).query) + assert request_params["sending_domain_ids[]"] == ["1", "2"] + assert request_params["sending_streams[]"] == ["transactional"] + assert request_params["categories[]"] == ["Transactional"] + assert request_params["email_service_providers[]"] == ["Gmail"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/api/general/test_stats.py` around lines 165 - 169, Update the assertions in the test that inspect responses.calls[0].request.params so they assert the serialized list values (not just the presence of keys); for example, replace the four assert "key in request_params" checks with assertions that request_params["sending_domain_ids[]"], request_params["sending_streams[]"], request_params["categories[]"], and request_params["email_service_providers[]"] equal the expected lists (or expected repeated values) used in the test input — this ensures the serialized shape and values produced by the list-param serialization logic (as observed via request_params) are exactly what the call should send.mailtrap/api/resources/stats.py (3)
1-4: Consider consolidating imports from the same module.The three imports from
mailtrap.models.statscan be combined into a single statement for cleaner code.♻️ Suggested consolidation
from mailtrap.http import HttpClient -from mailtrap.models.stats import SendingStatGroup -from mailtrap.models.stats import SendingStats -from mailtrap.models.stats import StatsFilterParams +from mailtrap.models.stats import SendingStatGroup, SendingStats, StatsFilterParams🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mailtrap/api/resources/stats.py` around lines 1 - 4, Consolidate the three separate imports from mailtrap.models.stats into a single import statement: replace the individual imports of SendingStatGroup, SendingStats, and StatsFilterParams with one grouped import from mailtrap.models.stats so the top of the file imports HttpClient from mailtrap.http and imports SendingStatGroup, SendingStats, StatsFilterParams together from mailtrap.models.stats.
6-11: Consider makingGROUP_KEYSa private constant.Since
GROUP_KEYSis an internal implementation detail used only by_grouped_stats, prefixing it with an underscore (_GROUP_KEYS) would signal this intent and prevent external reliance on the mapping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mailtrap/api/resources/stats.py` around lines 6 - 11, Rename the public constant GROUP_KEYS to a private constant named _GROUP_KEYS and update all references to it (notably inside the _grouped_stats function) so the implementation detail is not exposed; ensure any import/usage within the same module uses _GROUP_KEYS and run tests to confirm there are no external references that need updating.
50-65: Consider stricter typing for thegroupparameter.The
groupparameter only accepts values fromGROUP_KEYS. Using aLiteraltype would provide better type safety and IDE support.♻️ Suggested type improvement
+from typing import Literal + +GroupType = Literal["domains", "categories", "email_service_providers", "date"] + def _grouped_stats( - self, account_id: int, group: str, params: StatsFilterParams + self, account_id: int, group: GroupType, params: StatsFilterParams ) -> list[SendingStatGroup]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mailtrap/api/resources/stats.py` around lines 50 - 65, The group parameter to _grouped_stats only accepts keys from GROUP_KEYS, so declare a stricter Literal alias (e.g. GroupKey = Literal['keyA', 'keyB', ...] where the string literals enumerate the keys in GROUP_KEYS), replace the method signature to def _grouped_stats(self, account_id: int, group: GroupKey, params: StatsFilterParams) -> list[SendingStatGroup], and update any callers/type hints accordingly; keep the runtime behavior unchanged (still use GROUP_KEYS[group] and response parsing into SendingStatGroup).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/general/stats.py`:
- Line 7: ACCOUNT_ID is declared as a string but passed to helper functions
typed to accept int; change the declaration of ACCOUNT_ID from a string to an
integer literal (e.g., ACCOUNT_ID = 123456) and update any other sample
declarations/usages (the block around the later occurrences) so they pass an int
to the helper functions referenced in this file; ensure references to ACCOUNT_ID
in functions like the stats helpers now receive an int to keep types consistent.
---
Nitpick comments:
In `@mailtrap/api/resources/stats.py`:
- Around line 1-4: Consolidate the three separate imports from
mailtrap.models.stats into a single import statement: replace the individual
imports of SendingStatGroup, SendingStats, and StatsFilterParams with one
grouped import from mailtrap.models.stats so the top of the file imports
HttpClient from mailtrap.http and imports SendingStatGroup, SendingStats,
StatsFilterParams together from mailtrap.models.stats.
- Around line 6-11: Rename the public constant GROUP_KEYS to a private constant
named _GROUP_KEYS and update all references to it (notably inside the
_grouped_stats function) so the implementation detail is not exposed; ensure any
import/usage within the same module uses _GROUP_KEYS and run tests to confirm
there are no external references that need updating.
- Around line 50-65: The group parameter to _grouped_stats only accepts keys
from GROUP_KEYS, so declare a stricter Literal alias (e.g. GroupKey =
Literal['keyA', 'keyB', ...] where the string literals enumerate the keys in
GROUP_KEYS), replace the method signature to def _grouped_stats(self,
account_id: int, group: GroupKey, params: StatsFilterParams) ->
list[SendingStatGroup], and update any callers/type hints accordingly; keep the
runtime behavior unchanged (still use GROUP_KEYS[group] and response parsing
into SendingStatGroup).
In `@tests/unit/api/general/test_stats.py`:
- Around line 165-169: Update the assertions in the test that inspect
responses.calls[0].request.params so they assert the serialized list values (not
just the presence of keys); for example, replace the four assert "key in
request_params" checks with assertions that
request_params["sending_domain_ids[]"], request_params["sending_streams[]"],
request_params["categories[]"], and request_params["email_service_providers[]"]
equal the expected lists (or expected repeated values) used in the test input —
this ensures the serialized shape and values produced by the list-param
serialization logic (as observed via request_params) are exactly what the call
should send.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae6b307f-d0d4-4d77-8389-241e8f13796b
📒 Files selected for processing (9)
CHANGELOG.mdexamples/general/stats.pymailtrap/__init__.pymailtrap/api/general.pymailtrap/api/resources/stats.pymailtrap/models/common.pymailtrap/models/stats.pypyproject.tomltests/unit/api/general/test_stats.py
mklocek
approved these changes
Mar 9, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (2)
examples/general/stats.py (2)
11-55: Avoid baking January 2026 into every sample call.This example will look stale quickly and often return empty data unless the reader happens to have stats in that exact window. A shared
START_DATE/END_DATEplaceholder pair would keep the sample easier to update.Possible cleanup
+START_DATE = "YYYY-MM-DD" +END_DATE = "YYYY-MM-DD" + def get_stats(account_id: int) -> SendingStats: - params = StatsFilterParams(start_date="2026-01-01", end_date="2026-01-31") + params = StatsFilterParams(start_date=START_DATE, end_date=END_DATE)Apply the same replacement to the other helpers in this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/general/stats.py` around lines 11 - 55, The examples hard-code the January 2026 date range in every helper (e.g., get_stats, get_stats_by_domains, get_stats_by_categories, get_stats_by_email_service_providers, get_stats_by_date, get_stats_with_filters, get_stats_by_domains_with_filters); replace those literal start_date/end_date strings with shared constants (e.g., START_DATE and END_DATE) defined once at the top of the module and use them in all StatsFilterParams instances so the sample is easy to update in one place and avoid stale/empty results.
1-8: Load the sample token from the environment instead of source.A literal placeholder keeps secret scanners noisy and nudges copy-paste users toward editing credentials into the example.
Possible cleanup
+import os + import mailtrap as mt from mailtrap.models.stats import SendingStatGroup, SendingStats, StatsFilterParams -API_TOKEN = "YOUR_API_TOKEN" +API_TOKEN = os.environ["MAILTRAP_API_TOKEN"] ACCOUNT_ID = 123456🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/general/stats.py` around lines 1 - 8, Replace the hard-coded API_TOKEN and ACCOUNT_ID in examples/general/stats.py with environment variables: import os, read API_TOKEN via os.getenv('MAILTRAP_API_TOKEN') and ACCOUNT_ID via os.getenv('MAILTRAP_ACCOUNT_ID') (convert to int as needed), and keep a clear non-secret fallback or guard that raises a helpful error if the env var is missing before constructing MailtrapClient; update references to API_TOKEN and ACCOUNT_ID accordingly (symbols: API_TOKEN, ACCOUNT_ID, MailtrapClient, stats_api).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@examples/general/stats.py`:
- Around line 11-55: The examples hard-code the January 2026 date range in every
helper (e.g., get_stats, get_stats_by_domains, get_stats_by_categories,
get_stats_by_email_service_providers, get_stats_by_date, get_stats_with_filters,
get_stats_by_domains_with_filters); replace those literal start_date/end_date
strings with shared constants (e.g., START_DATE and END_DATE) defined once at
the top of the module and use them in all StatsFilterParams instances so the
sample is easy to update in one place and avoid stale/empty results.
- Around line 1-8: Replace the hard-coded API_TOKEN and ACCOUNT_ID in
examples/general/stats.py with environment variables: import os, read API_TOKEN
via os.getenv('MAILTRAP_API_TOKEN') and ACCOUNT_ID via
os.getenv('MAILTRAP_ACCOUNT_ID') (convert to int as needed), and keep a clear
non-secret fallback or guard that raises a helpful error if the env var is
missing before constructing MailtrapClient; update references to API_TOKEN and
ACCOUNT_ID accordingly (symbols: API_TOKEN, ACCOUNT_ID, MailtrapClient,
stats_api).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56f1334c-0854-40b5-a3f1-6b65a2ff5b92
📒 Files selected for processing (6)
CHANGELOG.mdexamples/general/stats.pymailtrap/api/resources/stats.pymailtrap/models/stats.pypyproject.tomltests/unit/api/general/test_stats.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/unit/api/general/test_stats.py
- pyproject.toml
- mailtrap/models/stats.py
- CHANGELOG.md
There was a problem hiding this comment.
🧹 Nitpick comments (3)
mailtrap/api/resources/email_logs.py (3)
31-37: Consider handling non-dict responses defensively.Per
mailtrap/http.py:51-61,_process_response()can returnNone(empty body), a string (non-JSON), or a dict. If the API returns an unexpected response format, calling.get()on a non-dict will raiseAttributeError.🛡️ Proposed defensive handling
response = self._client.get(self._api_path(), params=params or None) + if not isinstance(response, dict): + response = {} messages = [EmailLogMessage.from_api(msg) for msg in response.get("messages", [])]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mailtrap/api/resources/email_logs.py` around lines 31 - 37, The current code assumes response is a dict and calls response.get(...), which can raise AttributeError if _process_response() returns None or a non-dict; update the logic in the method that builds EmailLogsListResponse (the block that calls self._client.get(self._api_path())) to first validate that response is a dict (e.g., if not isinstance(response, dict): set response = {}), then derive messages = [EmailLogMessage.from_api(msg) for msg in response.get("messages", [])] and default total_count/next_page_cursor from the safe dict so EmailLogsListResponse construction never calls .get on a non-dict. Ensure you reference EmailLogMessage.from_api and EmailLogsListResponse when making the change.
39-42: Same defensive check may be needed forget_by_id.If the single-message endpoint returns a non-dict response,
EmailLogMessage.from_api(response)will fail. Consider validating the response type or letting the error propagate with a clearer message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mailtrap/api/resources/email_logs.py` around lines 39 - 42, The get_by_id method should defensively validate the API response before calling EmailLogMessage.from_api: after calling self._client.get(self._api_path(sending_message_id)) check that response is the expected dict/object (e.g., isinstance(response, dict)) and raise a clear ValueError or APIError including sending_message_id and the actual response type/content if it is not; update get_by_id to perform this check (around the call to EmailLogMessage.from_api) so callers get a descriptive error instead of a cryptic failure inside EmailLogMessage.from_api.
1-1: Docstring uses EN DASH instead of HYPHEN-MINUS.The docstring contains an ambiguous EN DASH character (
–). Consider replacing with a standard hyphen (-) for consistency.🔧 Proposed fix
-"""Email Logs API resource – list and get email sending logs.""" +"""Email Logs API resource - list and get email sending logs."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mailtrap/api/resources/email_logs.py` at line 1, The module docstring in email_logs.py uses an EN DASH (–) instead of a standard hyphen-minus (-); update the top-level docstring string (the module description "Email Logs API resource – list and get email sending logs.") to replace the EN DASH with a regular hyphen so it reads "Email Logs API resource - list and get email sending logs." to ensure consistent ASCII punctuation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mailtrap/api/resources/email_logs.py`:
- Around line 31-37: The current code assumes response is a dict and calls
response.get(...), which can raise AttributeError if _process_response() returns
None or a non-dict; update the logic in the method that builds
EmailLogsListResponse (the block that calls self._client.get(self._api_path()))
to first validate that response is a dict (e.g., if not isinstance(response,
dict): set response = {}), then derive messages = [EmailLogMessage.from_api(msg)
for msg in response.get("messages", [])] and default
total_count/next_page_cursor from the safe dict so EmailLogsListResponse
construction never calls .get on a non-dict. Ensure you reference
EmailLogMessage.from_api and EmailLogsListResponse when making the change.
- Around line 39-42: The get_by_id method should defensively validate the API
response before calling EmailLogMessage.from_api: after calling
self._client.get(self._api_path(sending_message_id)) check that response is the
expected dict/object (e.g., isinstance(response, dict)) and raise a clear
ValueError or APIError including sending_message_id and the actual response
type/content if it is not; update get_by_id to perform this check (around the
call to EmailLogMessage.from_api) so callers get a descriptive error instead of
a cryptic failure inside EmailLogMessage.from_api.
- Line 1: The module docstring in email_logs.py uses an EN DASH (–) instead of a
standard hyphen-minus (-); update the top-level docstring string (the module
description "Email Logs API resource – list and get email sending logs.") to
replace the EN DASH with a regular hyphen so it reads "Email Logs API resource -
list and get email sending logs." to ensure consistent ASCII punctuation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f63d1736-16af-48e2-9000-9e6adf62c113
📒 Files selected for processing (11)
CHANGELOG.mdREADME.mdexamples/email_logs/email_logs.pymailtrap/__init__.pymailtrap/api/email_logs.pymailtrap/api/resources/email_logs.pymailtrap/client.pymailtrap/models/email_logs.pypyproject.tomltests/unit/api/email_logs/test_email_logs.pytests/unit/models/test_email_log_models.py
✅ Files skipped from review due to trivial changes (2)
- README.md
- mailtrap/init.py
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- CHANGELOG.md
Contributor
|
Contributor
mklocek
requested review from
leonid-shevtsov
and removed request for
leonid-shevtsov
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.
Motivation
Changes
How to test
N/A
Summary by CodeRabbit
Release Notes
New Features
Documentation