Open
Conversation
📝 WalkthroughWalkthroughThis PR adds comprehensive Email Logs API support to the Mailtrap Python SDK, including API endpoints, structured data models with advanced filtering, client integration, example code, and full test coverage with updated documentation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (1)
tests/unit/api/email_logs/test_email_logs.py (1)
96-117: Consider verifying that filter parameters are actually sent in the request.The test mocks the response and calls the API with filters, but doesn't assert that the request actually contains the expected filter query parameters. The
responseslibrary supports request matching or inspection viaresponses.calls.This is optional, but would strengthen the test by confirming the filters are properly serialized and sent.
💡 Example: Assert request parameters
# After the API call, verify the request: assert len(responses.calls) == 1 request_url = responses.calls[0].request.url assert "filters%5Bsent_after%5D" in request_url # URL-encoded filters[sent_after] assert "search_after=b2c3" in request_url🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/api/email_logs/test_email_logs.py` around lines 96 - 117, Enhance the test_get_list_with_filters_and_search_after test to assert the outgoing HTTP request includes the serialized filter and search_after parameters: after calling email_logs_api.get_list(...) inspect responses.calls (ensure len == 1) and examine responses.calls[0].request.url to assert it contains the URL-encoded filters keys (e.g., filters[sent_after], filters[sent_before], filters[to]) and the search_after=b2c3 parameter so the EmailLogsApi.get_list serialization is verified.
🤖 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`:
- Line 27: Fix the typo in the changelog entry that currently reads
"releated"—open CHANGELOG.md and replace the word "releated" with "related" in
the line containing "Fix issue `#26`: Add MessagesApi, releated models, examples,
tests by `@Ihor-Bilous` in https://github.com/mailtrap/mailtrap-python/pull/43" so
the sentence reads "related".
In `@mailtrap/client.py`:
- Around line 106-112: The email_logs_api property is calling
_validate_account_id() which raises a hardcoded "account_id is required for
Testing API" message; change _validate_account_id to accept an optional api_name
parameter (or add a new helper like _validate_account_id_for(api_name: str)) and
use that value in the raised error so callers can pass "Email Logs API" from the
email_logs_api property; update email_logs_api to call
_validate_account_id("Email Logs API") (and update any other callers to pass
appropriate names or rely on the default) so the remediation hint is correct.
In `@mailtrap/models/email_logs.py`:
- Around line 59-76: MessageEvent.details uses a bare Union so Pydantic v2 can
mis-select between similar detail types; update the models so selection is
deterministic by either 1) adding an event_type: Literal[...] field to each
detail model (e.g., EventDetailsOpen, EventDetailsUnsubscribe,
EventDetailsDelivery, EventDetailsBounce, etc.) and annotate
MessageEvent.details with Field(discriminator='event_type') so Pydantic can
discriminate, or 2) change MessageEvent.from_api() to inspect the top-level
MessageEvent.event_type and explicitly instantiate the correct EventDetails*
class before constructing MessageEvent; reference MessageEvent, details,
event_type, the EventDetails* classes, and from_api() when making the change.
---
Nitpick comments:
In `@tests/unit/api/email_logs/test_email_logs.py`:
- Around line 96-117: Enhance the test_get_list_with_filters_and_search_after
test to assert the outgoing HTTP request includes the serialized filter and
search_after parameters: after calling email_logs_api.get_list(...) inspect
responses.calls (ensure len == 1) and examine responses.calls[0].request.url to
assert it contains the URL-encoded filters keys (e.g., filters[sent_after],
filters[sent_before], filters[to]) and the search_after=b2c3 parameter so the
EmailLogsApi.get_list serialization is verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4957264-78fb-44a0-8a02-edd5d6a66239
📒 Files selected for processing (12)
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.pytests/unit/test_client.py
mklocek
force-pushed
the
email-logs-api
branch
4 times, most recently
from
6cf1ce7 to
655c6f5
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mailtrap/api/resources/email_logs.py (1)
1-1: Minor: Replace EN DASH with HYPHEN-MINUS in docstring.The docstring uses an EN DASH (
–) instead of a standard HYPHEN-MINUS (-). While this doesn't affect functionality, it's flagged by static analysis tools.📝 Suggested 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 HYPHEN-MINUS (-); open the top-level docstring (the triple-quoted string at the top of the module in email_logs.py) and replace the EN DASH character with a standard hyphen-minus so the string reads "Email Logs API resource - list and get email sending logs." to satisfy static analysis.
🤖 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`:
- Line 1: The module docstring in email_logs.py uses an EN DASH (–) instead of a
HYPHEN-MINUS (-); open the top-level docstring (the triple-quoted string at the
top of the module in email_logs.py) and replace the EN DASH character with a
standard hyphen-minus so the string reads "Email Logs API resource - list and
get email sending logs." to satisfy static analysis.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d286002-7524-4f44-9c4c-5e466e9144b5
📒 Files selected for processing (9)
README.mdexamples/email_logs/email_logs.pymailtrap/__init__.pymailtrap/api/email_logs.pymailtrap/api/resources/email_logs.pymailtrap/client.pymailtrap/models/email_logs.pytests/unit/api/email_logs/test_email_logs.pytests/unit/models/test_email_log_models.py
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- tests/unit/models/test_email_log_models.py
- tests/unit/api/email_logs/test_email_logs.py
piobeny
approved these changes
Mar 18, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
CHANGELOG.md (1)
27-27:⚠️ Potential issue | 🟡 MinorFix changelog typo on Line 27 (
releated→related).This typo is still present in the MessagesApi entry.
✏️ Proposed fix
-- Fix issue `#26`: Add MessagesApi, releated models, examples, tests by `@Ihor-Bilous` in https://github.com/mailtrap/mailtrap-python/pull/43 +- Fix issue `#26`: Add MessagesApi, related models, examples, tests by `@Ihor-Bilous` in https://github.com/mailtrap/mailtrap-python/pull/43🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 27, Typo in the CHANGELOG entry for "Fix issue `#26`: Add MessagesApi, releated models, examples, tests by `@Ihor-Bilous` in https://github.com/mailtrap/mailtrap-python/pull/43" — update the word "releated" to "related" in the MessagesApi changelog line so it reads "related models, examples, tests" (locate the exact string "MessagesApi" or the quoted changelog line to apply the change).
🧹 Nitpick comments (2)
examples/email_logs/email_logs.py (1)
13-17: Prefer environment variables over editable credential constants.At Line 13 and Line 14, this pattern can lead to accidental commits of real secrets after local edits. Use environment variables and fail fast when missing.
🔐 Suggested refactor
+import os + -API_TOKEN = "YOUR_API_TOKEN" -ACCOUNT_ID = "YOUR_ACCOUNT_ID" +API_TOKEN = os.getenv("MAILTRAP_API_TOKEN") +ACCOUNT_ID = os.getenv("MAILTRAP_ACCOUNT_ID") + +if not API_TOKEN or not ACCOUNT_ID: + raise RuntimeError( + "Set MAILTRAP_API_TOKEN and MAILTRAP_ACCOUNT_ID before running this example." + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/email_logs/email_logs.py` around lines 13 - 17, Replace the hard-coded API_TOKEN and ACCOUNT_ID constants with environment-based configuration: read os.environ["MAILTRAP_API_TOKEN"] and os.environ["MAILTRAP_ACCOUNT_ID"] (or use os.getenv with explicit checks) before creating the MailtrapClient; if either value is missing, raise a clear exception or exit early so the program fails fast. Update the code around the API_TOKEN/API_ACCOUNT_ID declarations and the client = mt.MailtrapClient(...) instantiation to use the env vars and validate them prior to using email_logs_api.tests/unit/models/test_email_log_models.py (1)
46-187: Reduce duplicated message payload setup across tests.The repeated base payload makes updates error-prone. Consider a small helper/fixture that returns a default message dict and accepts overrides per test.
♻️ Suggested refactor
+def _base_message_data(**overrides: Any) -> dict[str, Any]: + base: dict[str, Any] = { + "message_id": "a1b2c3d4-e5f6-7890-abcd-ef1234567890", + "status": "delivered", + "subject": "Welcome", + "from": "sender@example.com", + "to": "recipient@example.com", + "sent_at": "2025-01-15T10:30:00Z", + "client_ip": None, + "category": None, + "custom_variables": {}, + "sending_stream": "transactional", + "sending_domain_id": 3938, + "template_id": None, + "template_variables": {}, + "opens_count": 0, + "clicks_count": 0, + "raw_message_url": None, + "events": [], + } + base.update(overrides) + return base🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/models/test_email_log_models.py` around lines 46 - 187, Create a shared factory/fixture that returns the common base payload and allow per-test overrides instead of repeating the dict literal in each test; implement a helper function (e.g., default_email_log_data or a pytest fixture default_email_log_data) and use it in tests before calling EmailLogMessage.from_api, merging specific fields (like "events", "raw_message_url", "clicks_count") to customize each test case so tests such as test_from_api_parses_events_with_deterministic_details_type, test_from_api_parses_click_and_bounce_events, and test_from_api_parses_delivery_event reuse the same baseline payload and only supply the differing keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mailtrap/api/resources/email_logs.py`:
- Line 1: The module docstring at the top of the file contains an en dash (–);
replace it with a standard hyphen (-) so the docstring reads "Email Logs API
resource - list and get email sending logs." Update the module-level string (the
top-of-file docstring) accordingly to satisfy Ruff RUF002.
- Around line 31-37: Guard against non-dict responses from HttpClient.get() in
get_list() and get_by_id(): ensure the value returned from
self._client.get(self._api_path(), ...) is a dict before accessing keys or
passing to EmailLogMessage.from_api; if it's not a dict, convert it to an empty
dict or raise a clear SDK error, then proceed to build EmailLogsListResponse
(using response.get(...)) and to call EmailLogMessage.from_api only for dict
items; reference the methods get_list(), get_by_id(), EmailLogMessage.from_api,
EmailLogsListResponse and the call site self._client.get(...) when making the
change.
---
Duplicate comments:
In `@CHANGELOG.md`:
- Line 27: Typo in the CHANGELOG entry for "Fix issue `#26`: Add MessagesApi,
releated models, examples, tests by `@Ihor-Bilous` in
https://github.com/mailtrap/mailtrap-python/pull/43" — update the word
"releated" to "related" in the MessagesApi changelog line so it reads "related
models, examples, tests" (locate the exact string "MessagesApi" or the quoted
changelog line to apply the change).
---
Nitpick comments:
In `@examples/email_logs/email_logs.py`:
- Around line 13-17: Replace the hard-coded API_TOKEN and ACCOUNT_ID constants
with environment-based configuration: read os.environ["MAILTRAP_API_TOKEN"] and
os.environ["MAILTRAP_ACCOUNT_ID"] (or use os.getenv with explicit checks) before
creating the MailtrapClient; if either value is missing, raise a clear exception
or exit early so the program fails fast. Update the code around the
API_TOKEN/API_ACCOUNT_ID declarations and the client = mt.MailtrapClient(...)
instantiation to use the env vars and validate them prior to using
email_logs_api.
In `@tests/unit/models/test_email_log_models.py`:
- Around line 46-187: Create a shared factory/fixture that returns the common
base payload and allow per-test overrides instead of repeating the dict literal
in each test; implement a helper function (e.g., default_email_log_data or a
pytest fixture default_email_log_data) and use it in tests before calling
EmailLogMessage.from_api, merging specific fields (like "events",
"raw_message_url", "clicks_count") to customize each test case so tests such as
test_from_api_parses_events_with_deterministic_details_type,
test_from_api_parses_click_and_bounce_events, and
test_from_api_parses_delivery_event reuse the same baseline payload and only
supply the differing keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3151adc8-6531-49ce-a123-9b71917b170a
📒 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 as they are similar to previous changes (1)
- tests/unit/api/email_logs/test_email_logs.py
| @@ -0,0 +1,48 @@ | |||
| """Email Logs API resource – list and get email sending logs.""" | |||
There was a problem hiding this comment.
Replace EN DASH in module docstring to satisfy Ruff RUF002.
Line 1 uses –; replace it with standard -.
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 1-1: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
🤖 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 at the
top of the file contains an en dash (–); replace it with a standard hyphen (-)
so the docstring reads "Email Logs API resource - list and get email sending
logs." Update the module-level string (the top-of-file docstring) accordingly to
satisfy Ruff RUF002.
Comment on lines
+31
to
+37
| response = self._client.get(self._api_path(), params=params or None) | ||
| messages = [EmailLogMessage.from_api(msg) for msg in response.get("messages", [])] | ||
| return EmailLogsListResponse( | ||
| messages=messages, | ||
| total_count=response.get("total_count", 0), | ||
| next_page_cursor=response.get("next_page_cursor"), | ||
| ) |
There was a problem hiding this comment.
Guard response shape before dict access/model parsing.
HttpClient.get() can return non-dict successful payloads (e.g., empty body/text). get_list() and get_by_id() currently assume dicts, which can fail with runtime errors instead of a clear SDK exception.
🛡️ Proposed fix
-"""Email Logs API resource – list and get email sending logs."""
+"""Email Logs API resource - list and get email sending logs."""
-from typing import Optional
+from typing import Any
+from typing import Optional
@@
class EmailLogsApi:
@@
+ def _expect_dict_response(self, response: Any, operation: str) -> dict[str, Any]:
+ if not isinstance(response, dict):
+ raise TypeError(
+ f"Unexpected response type for {operation}: {type(response).__name__}"
+ )
+ return response
+
def get_list(
@@
- response = self._client.get(self._api_path(), params=params or None)
+ response = self._expect_dict_response(
+ self._client.get(self._api_path(), params=params or None),
+ "email logs list",
+ )
messages = [EmailLogMessage.from_api(msg) for msg in response.get("messages", [])]
@@
def get_by_id(self, sending_message_id: str) -> EmailLogMessage:
"""Get a single email log message by its UUID."""
- response = self._client.get(self._api_path(sending_message_id))
+ response = self._expect_dict_response(
+ self._client.get(self._api_path(sending_message_id)),
+ "email log by id",
+ )
return EmailLogMessage.from_api(response)Also applies to: 41-42
🤖 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, Guard against
non-dict responses from HttpClient.get() in get_list() and get_by_id(): ensure
the value returned from self._client.get(self._api_path(), ...) is a dict before
accessing keys or passing to EmailLogMessage.from_api; if it's not a dict,
convert it to an empty dict or raise a clear SDK error, then proceed to build
EmailLogsListResponse (using response.get(...)) and to call
EmailLogMessage.from_api only for dict items; reference the methods get_list(),
get_by_id(), EmailLogMessage.from_api, EmailLogsListResponse and the call site
self._client.get(...) when making the change.
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
2 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.
Changes
Summary by CodeRabbit
New Features
Documentation
Tests
Chores