Fix issue #18: Add api for EmailTemplates, add tests and examples #34
Conversation
WalkthroughAdds Email Templates support: new template models, TemplatesApi resource and EmailTemplatesApi wrapper, MailtrapClient.email_templates_api property, example script and unit tests; renames RequestModel → RequestParams and updates mail models; HttpClient returns None for empty successful responses; re-exports template param classes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant MC as MailtrapClient
participant ETA as EmailTemplatesApi
participant TA as TemplatesApi
participant HTTP as HttpClient
participant API as Mailtrap REST API
Dev->>MC: access email_templates_api
MC->>ETA: construct(account_id, client)
Dev->>ETA: templates
ETA->>TA: construct(account_id, client)
rect #E6F5FF
note right of Dev: List templates
Dev->>TA: get_list()
TA->>HTTP: GET /api/accounts/{id}/email_templates
HTTP->>API: Request
API-->>HTTP: 200 OK + JSON
HTTP-->>TA: parsed list
TA-->>Dev: list[EmailTemplate]
end
rect #E9FFE9
note right of Dev: Create/Update template
Dev->>TA: create(params) / update(id, params)
TA->>HTTP: POST/PATCH {"email_template": params.api_data}
API-->>HTTP: 200/201 OK + JSON
HTTP-->>TA: parsed object
TA-->>Dev: EmailTemplate
end
rect #FFF0E6
note right of Dev: Delete template (204)
Dev->>TA: delete(id)
TA->>HTTP: DELETE /email_templates/{id}
API-->>HTTP: 204 No Content
HTTP-->>TA: None (empty body)
TA-->>Dev: DeletedObject(id)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (16)
mailtrap/http.py (1)
54-56: Graceful no-content handling is good; consider narrowing the condition to explicit no-content statuses.Returning None for successful but empty bodies solves 204/No Content cases and avoids JSONDecodeError. To prevent masking accidental contract regressions (e.g., an endpoint that should return JSON but returns an empty 200 by mistake), gate on explicit no-content statuses and truly empty bodies.
Apply this minimal diff:
- if not response.content.strip(): + # Treat explicit no-content status codes and truly empty bodies as None + if response.status_code in (204, 205) or not response.content or not response.content.strip(): return NoneOptionally, update the signature and docstring to reflect the possibility of None:
def _process_response(self, response: Response) -> Optional[Any]: """ - Non-OK: raises via _handle_failed_response. - 204/205 or empty body: returns None. - Otherwise: returns parsed JSON. """mailtrap/models/mail/attachment.py (1)
25-27: UTF-8-only decode may raise on binary attachments; optional defensive tweak.This follows your documented preference to keep value.decode() (referencing the retrieved learning), so keeping it is fine. If you want to avoid runtime decode errors on arbitrary binary payloads without switching to base64, consider a tolerant decode.
Apply this diff to fail-soft while preserving human-readable output:
- return value.decode() + # Prefer UTF-8 text; avoid exceptions on arbitrary bytes + return value.decode("utf-8", errors="replace")Alternatively, keep strict decode and document that only UTF-8 textual attachments are supported.
mailtrap/__init__.py (1)
13-14: Silence Ruff F401 for import-as-public-API, or export via all.These imports exist for re-export; Ruff flags them as unused. Two options:
- Minimal: add noqa on the import lines.
- Preferred: include them in all to make the public surface explicit.
Minimal change:
-from .models.templates import CreateEmailTemplateParams -from .models.templates import UpdateEmailTemplateParams +from .models.templates import CreateEmailTemplateParams # noqa: F401 +from .models.templates import UpdateEmailTemplateParams # noqa: F401Or add to all:
__all__ = [ # ...existing exports... "CreateEmailTemplateParams", "UpdateEmailTemplateParams", ]mailtrap/client.py (1)
119-122: Error message mentions only “Testing API” but now guards Email Templates tooThis validator now serves multiple account-scoped APIs. Suggest generalizing the error to prevent confusing users hitting
email_templates_api.Apply this diff:
- if not self.account_id: - raise ClientConfigurationError("`account_id` is required for Testing API") + if not self.account_id: + raise ClientConfigurationError( + "`account_id` is required for account-scoped APIs (e.g., Testing, Email Templates)" + )mailtrap/models/common.py (1)
12-18: Rename to ParametersObject — consider a backwards-compat alias for RequestModelRenaming a public class can be a breaking change for downstream users importing
RequestModel. A lightweight alias preserves compatibility while allowing the new name to be used everywhere internally.Apply this diff:
@dataclass class ParametersObject: @property def api_data(self: T) -> dict[str, Any]: return cast( dict[str, Any], TypeAdapter(type(self)).dump_python(self, by_alias=True, exclude_none=True), ) + +# Backwards-compat alias (scheduled for deprecation/removal in a future major release) +RequestModel = ParametersObjecttests/unit/models/test_templates.py (2)
35-41: Exception type might differ across Pydantic versions; make test resilient
__post_init__currently raisesValueError. Depending on the Pydantic dataclasses wrapper, this may surface asValueErroror be wrapped intopydantic.ValidationError. To avoid spurious CI failures across environments, accept both.Also, fix the minor typo in the asserted substring (“actio” → “action”) or match a shorter stable prefix.
Apply this diff:
- def test_raise_error_when_all_fields_are_missing(self) -> None: - with pytest.raises(ValidationError) as exc: + def test_raise_error_when_all_fields_are_missing(self) -> None: + # Accept both pydantic.ValidationError and plain ValueError to be robust to pydantic dataclass behavior. + with pytest.raises((ValidationError, ValueError)) as exc: _ = UpdateEmailTemplateParams() - assert "At least one field must be provided for update actio" in str(exc) + assert "At least one field must be provided for update action" in str(exc.value)If you prefer always raising
ValidationError, we can move this constraint into a Pydantic validator on a model rather than relying on__post_init__. Let me know and I can draft that refactor.
41-47: Test name: “required props only” is misleading for Update paramsUpdate params don’t have required props; the test asserts only provided props are serialized. Consider renaming for clarity.
Apply this diff:
- def test_api_data_should_return_dict_with_required_props_only(self) -> None: + def test_api_data_should_return_dict_with_provided_props_only(self) -> None:mailtrap/models/mail/base.py (1)
13-20: BaseMail fields extended — sensible defaults and aliases
fromalias on sender is correct.- Optional cc/bcc/attachments/headers/custom_variables are fine and will be omitted from
api_datawhen None.Optional: enforce at least one recipient among to/cc/bcc to prevent malformed sends early.
Here’s a minimal guard using
__post_init__:@dataclass class BaseMail(ParametersObject): sender: Address = Field(..., serialization_alias="from") to: list[Address] = Field(...) cc: Optional[list[Address]] = None bcc: Optional[list[Address]] = None attachments: Optional[list[Attachment]] = None headers: Optional[dict[str, str]] = None custom_variables: Optional[dict[str, Any]] = None + + def __post_init__(self) -> None: + if not (self.to or self.cc or self.bcc): + raise ValueError("At least one recipient must be provided (to/cc/bcc)")If you prefer Pydantic-native validation, we can switch BaseMail to a BaseModel for richer constraints.
tests/unit/api/test_email_templates.py (3)
1-14: Add json import to assert request payloadsTo validate that create/update send the expected JSON body, import json for request body parsing.
Apply this diff:
from typing import Any import pytest import responses +import json
187-205: Assert create payload shape to catch regressionsAdd checks that the POST body wraps params under "email_template" and excludes None fields.
Apply this diff:
template = client.create( template_params=CreateEmailTemplateParams( name=sample_template_dict["name"], subject=sample_template_dict["subject"], category=sample_template_dict["category"], ) ) assert isinstance(template, EmailTemplate) assert template.name == sample_template_dict["name"] assert template.subject == sample_template_dict["subject"] assert template.category == sample_template_dict["category"] assert template.uuid == sample_template_dict["uuid"] assert template.created_at == sample_template_dict["created_at"] assert template.updated_at == sample_template_dict["updated_at"] assert template.body_html == sample_template_dict["body_html"] assert template.body_text == sample_template_dict["body_text"] + + # Verify request payload shape + raw_body = responses.calls[-1].request.body + sent = json.loads(raw_body.decode() if isinstance(raw_body, (bytes, bytearray)) else raw_body) + assert "email_template" in sent + assert sent["email_template"] == { + "name": sample_template_dict["name"], + "subject": sample_template_dict["subject"], + "category": sample_template_dict["category"], + }
272-278: Assert update payload shape (only provided fields should be sent)Ensure PATCH body nests under "email_template" and contains only the updated fields.
Apply this diff:
template = client.update( TEMPLATE_ID, template_params=UpdateEmailTemplateParams(name=updated_name) ) assert isinstance(template, EmailTemplate) assert template.name == updated_name + + # Verify request payload shape + raw_body = responses.calls[-1].request.body + sent = json.loads(raw_body.decode() if isinstance(raw_body, (bytes, bytearray)) else raw_body) + assert sent == {"email_template": {"name": updated_name}}examples/email_templates/templates.py (3)
7-11: Avoid hardcoding secrets; read token/account from environment and fix placeholder typos.Hardcoding tokens in examples is risky if copied verbatim. Prefer env vars and use "YOUR_*" placeholders. Also, instantiating the client at import time can trigger network calls when this module is imported—fine for examples, but consider gating under main if you expect people to import these helpers.
Apply this minimal diff to use env vars and clearer placeholders:
-from typing import Optional +from typing import Optional +import os @@ -API_TOKEN = "YOU_API_TOKEN" -ACCOUNT_ID = "YOU_ACCOUNT_ID" +API_TOKEN = os.getenv("MAILTRAP_API_TOKEN", "YOUR_API_TOKEN") +ACCOUNT_ID = os.getenv("MAILTRAP_ACCOUNT_ID", "YOUR_ACCOUNT_ID")
39-46: Aligntemplate_idtype with API (int, notstr).All TemplatesApi methods expect
intIDs. Keep the example consistent to avoid confusion and runtime surprises when IDs are used arithmetically or validated.-def update_template( - template_id: str, +def update_template( + template_id: int, name: Optional[str] = None, subject: Optional[str] = None, category: Optional[str] = None, body_text: Optional[str] = None, body_html: Optional[str] = None, ) -> EmailTemplate:
57-58: Aligndelete_templateparameter type with API (int).-def delete_template(template_id: str) -> DeletedObject: +def delete_template(template_id: int) -> DeletedObject: return templates_api.delete(template_id)mailtrap/models/templates.py (1)
39-49: Consider parsing timestamps asdatetimefor better ergonomics.Returning
strforcreated_at/updated_atmakes consumers parse manually. If API format is stable (e.g., ISO 8601), usingdatetimeimproves UX; Pydantic v2 handles parsing well. This is optional—strings are acceptable if you want to avoid downstream breaking changes.Example change (may require adjusting tests/serializers):
-from pydantic.dataclasses import dataclass +from pydantic.dataclasses import dataclass +from datetime import datetime @@ class EmailTemplate: @@ - created_at: str - updated_at: str + created_at: datetime + updated_at: datetimemailtrap/api/resources/templates.py (1)
13-16: Guard against empty successful responses (204) to avoid TypeError.
HttpClientnow returnsNonefor empty bodies. If the list endpoint ever returns 204, iteratingNonewill raise. Cheap defensive fix below.- response = self._client.get(f"/api/accounts/{self._account_id}/email_templates") - return [EmailTemplate(**template) for template in response] + response = self._client.get(f"/api/accounts/{self._account_id}/email_templates") + templates = response or [] + return [EmailTemplate(**template) for template in templates]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
examples/email_templates/templates.py(1 hunks)mailtrap/__init__.py(1 hunks)mailtrap/api/resources/templates.py(1 hunks)mailtrap/api/templates.py(1 hunks)mailtrap/client.py(2 hunks)mailtrap/http.py(1 hunks)mailtrap/models/common.py(1 hunks)mailtrap/models/mail/address.py(1 hunks)mailtrap/models/mail/attachment.py(2 hunks)mailtrap/models/mail/base.py(1 hunks)mailtrap/models/templates.py(1 hunks)tests/unit/api/test_email_templates.py(1 hunks)tests/unit/models/test_templates.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-18T14:05:35.060Z
Learnt from: Ihor-Bilous
PR: railsware/mailtrap-python#33
File: mailtrap/models/mail/attachment.py:25-27
Timestamp: 2025-08-18T14:05:35.060Z
Learning: In the mailtrap-python codebase, the user prefers to use simple value.decode() for attachment content serialization in the Attachment model rather than base64 encoding, maintaining consistency with the previous implementation approach.
Applied to files:
mailtrap/models/mail/attachment.py
🧬 Code graph analysis (11)
mailtrap/client.py (2)
mailtrap/api/templates.py (2)
templates(11-12)EmailTemplatesApi(5-12)mailtrap/http.py (1)
HttpClient(13-96)
mailtrap/api/resources/templates.py (5)
mailtrap/http.py (5)
HttpClient(13-96)get(25-29)post(31-33)patch(39-41)delete(43-45)mailtrap/models/common.py (2)
DeletedObject(22-23)api_data(14-18)mailtrap/api/templates.py (1)
templates(11-12)mailtrap/models/templates.py (3)
CreateEmailTemplateParams(9-14)EmailTemplate(40-49)UpdateEmailTemplateParams(18-36)tests/unit/api/test_email_templates.py (1)
client(22-23)
mailtrap/api/templates.py (3)
mailtrap/api/resources/templates.py (1)
TemplatesApi(8-43)mailtrap/http.py (1)
HttpClient(13-96)tests/unit/api/test_email_templates.py (1)
client(22-23)
mailtrap/models/templates.py (1)
mailtrap/models/common.py (1)
ParametersObject(12-18)
tests/unit/api/test_email_templates.py (5)
mailtrap/api/resources/templates.py (6)
TemplatesApi(8-43)get_list(13-15)get_by_id(17-21)create(23-28)update(30-37)delete(39-43)mailtrap/exceptions.py (1)
APIError(10-15)mailtrap/http.py (5)
HttpClient(13-96)get(25-29)post(31-33)patch(39-41)delete(43-45)mailtrap/models/common.py (1)
DeletedObject(22-23)mailtrap/models/templates.py (3)
CreateEmailTemplateParams(9-14)EmailTemplate(40-49)UpdateEmailTemplateParams(18-36)
mailtrap/models/mail/address.py (1)
mailtrap/models/common.py (1)
ParametersObject(12-18)
mailtrap/models/mail/attachment.py (1)
mailtrap/models/common.py (1)
ParametersObject(12-18)
mailtrap/__init__.py (2)
mailtrap/api/templates.py (1)
templates(11-12)mailtrap/models/templates.py (2)
CreateEmailTemplateParams(9-14)UpdateEmailTemplateParams(18-36)
examples/email_templates/templates.py (5)
mailtrap/models/common.py (1)
DeletedObject(22-23)mailtrap/models/templates.py (3)
EmailTemplate(40-49)CreateEmailTemplateParams(9-14)UpdateEmailTemplateParams(18-36)mailtrap/client.py (1)
email_templates_api(59-64)mailtrap/api/resources/templates.py (5)
get_list(13-15)create(23-28)get_by_id(17-21)update(30-37)delete(39-43)mailtrap/http.py (1)
delete(43-45)
tests/unit/models/test_templates.py (2)
mailtrap/models/templates.py (2)
CreateEmailTemplateParams(9-14)UpdateEmailTemplateParams(18-36)mailtrap/models/common.py (1)
api_data(14-18)
mailtrap/models/mail/base.py (3)
mailtrap/models/common.py (1)
ParametersObject(12-18)mailtrap/models/mail/address.py (1)
Address(9-11)mailtrap/models/mail/attachment.py (1)
Attachment(18-27)
🪛 Ruff (0.12.2)
mailtrap/__init__.py
13-13: .models.templates.CreateEmailTemplateParams imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
14-14: .models.templates.UpdateEmailTemplateParams imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
🔇 Additional comments (15)
mailtrap/models/mail/address.py (2)
5-5: Refactor to ParametersObject base matches the repo-wide rename.Import switch to ParametersObject is consistent with mailtrap.models.common and the new api_data path. No issues.
9-12: Address now extends ParametersObject — compatible with pydantic dataclass dumping.The dataclass + ParametersObject combo works with TypeAdapter dump in api_data. Looks good.
mailtrap/models/mail/attachment.py (1)
18-24: Attachment now extends ParametersObject — consistent with the RequestModel → ParametersObject migration.Inheritance and field aliases remain intact; this keeps serialization behavior stable.
mailtrap/api/templates.py (1)
5-12: Simple, compositional wrapper looks good and aligns with the resources API.Constructor mirrors TemplatesApi (client, account_id), and the property composes the account-scoped resource cleanly. No issues.
mailtrap/client.py (2)
9-9: Import for EmailTemplatesApi — looks goodThe import aligns with the new API surface and mirrors the existing pattern for other APIs.
58-65: New email_templates_api property mirrors existing patterns — solid
- Validates account_id and instantiates an HttpClient with GENERAL_HOST, same as testing_api.
- Uses explicit keyword args to avoid constructor ordering issues. Good.
No functional concerns here.
mailtrap/models/common.py (1)
8-8: TypeVar bound updated to ParametersObject — correctBound now reflects the renamed base class; generic typing of
api_dataremains intact.tests/unit/models/test_templates.py (1)
8-16: CreateEmailTemplateParams api_data tests — good coverageValidates minimal vs. full payloads and that
exclude_none=Truebehavior is respected.mailtrap/models/mail/base.py (1)
7-7: Switch to ParametersObject import — consistent with renameKeeps the inheritance tree aligned with the new common base.
mailtrap/models/templates.py (2)
8-15: Create params model looks solid.Fields match the API surface and default Nones ensure
api_dataexcludes absent keys viaexclude_none=True. No issues spotted.
25-36: Good validation inUpdateEmailTemplateParams.__post_init__.Enforcing "at least one field" prevents sending empty PATCH bodies. Clear error message as well.
mailtrap/api/resources/templates.py (4)
17-21: LGTM:get_by_idpath and model mapping are correct.Relies on
HttpClientto surface errors; straightforward mapping toEmailTemplate.
23-29: LGTM:createcorrectly wraps payload underemail_templateand maps response.
30-37: LGTM:updatemirrorscreatesemantics and uses PATCH with filteredapi_data.
39-43: Incorrect instantiation concern : The callDeletedObject(template_id)is valid—DeletedObjectis defined as a simple@dataclasswith a single fieldid: int, so its generated constructor signature is__init__(self, id: int)and accepts a positional argument. NoTypeErrorwill be raised at runtime.• In mailtrap/models/common.py:
@dataclass class DeletedObject: id: int
DeletedObject(5)correctly setsid=5via the positional parameter.
• Existing code in mailtrap/api/resources/projects.py already uses keyword unpacking (DeletedObject(**response)), but that doesn’t invalidate positional construction here.
• You may choose to switch toDeletedObject(id=template_id)for stylistic clarity, but it’s not required for correctness.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
mailtrap/http.py (1)
54-57: Broaden the “no content” check and avoid unnecessary buffering.Minor robustness tweak: explicitly handle 204/205 and skip calling
.strip()on large payloads whencontentis already empty. This keeps behavior identical for current callers but is safer across endpoints.Apply this diff:
- if not response.content.strip(): - return None + # Treat 204/205 responses or empty/whitespace-only bodies as "no content" + if response.status_code in (204, 205) or not response.content or not response.content.strip(): + return Nonemailtrap/__init__.py (1)
13-14: Silence F401 by marking these imports as re-exports.Ruff flags these as unused. Mark them as re-exports or add to
__all__to keep top-level exposure clean.Apply this diff to use redundant aliases (recognized by Ruff as re-exports):
-from .models.templates import CreateEmailTemplateParams -from .models.templates import UpdateEmailTemplateParams +from .models.templates import CreateEmailTemplateParams as CreateEmailTemplateParams +from .models.templates import UpdateEmailTemplateParams as UpdateEmailTemplateParamsOptionally, also add them to
__all__if you maintain it elsewhere.tests/unit/models/test_templates.py (1)
35-41: Tighten the expected error message.The assertion uses a truncated substring ("actio") which could mask accidental message changes. Prefer matching the full message for clarity.
Apply this diff:
- assert "At least one field must be provided for update actio" in str(exc) + assert "At least one field must be provided for update action" in str(exc)tests/unit/api/test_email_templates.py (2)
187-214: Create: assert response mapping; consider also asserting request payload.You validate the returned model well. Adding a quick payload assertion ensures the outgoing schema (
{"email_template": template_params.api_data}) doesn’t regress.Apply this diff:
@@ def test_create_should_return_new_template( self, client: TemplatesApi, sample_template_dict: dict ) -> None: + import json responses.post( BASE_TEMPLATES_URL, json=sample_template_dict, status=201, ) @@ ) assert isinstance(template, EmailTemplate) @@ assert template.body_text == sample_template_dict["body_text"] + + # Also validate request payload shape + sent = json.loads(responses.calls[0].request.body) + assert sent == { + "email_template": { + "name": sample_template_dict["name"], + "subject": sample_template_dict["subject"], + "category": sample_template_dict["category"], + } + }
257-278: Update: validate response mapping; consider asserting request payload.Mirroring the create test, asserting the outgoing JSON helps catch schema drift.
Apply this diff:
@@ def test_update_should_return_updated_template( self, client: TemplatesApi, sample_template_dict: dict ) -> None: + import json url = f"{BASE_TEMPLATES_URL}/{TEMPLATE_ID}" updated_name = "Updated Promotion Template" updated_template_dict = sample_template_dict.copy() updated_template_dict["name"] = updated_name @@ template = client.update( TEMPLATE_ID, template_params=UpdateEmailTemplateParams(name=updated_name) ) @@ assert isinstance(template, EmailTemplate) assert template.name == updated_name + + # Also validate request payload shape + sent = json.loads(responses.calls[0].request.body) + assert sent == {"email_template": {"name": updated_name}}mailtrap/api/resources/templates.py (4)
39-43: Make delete robust and future-proof; prefer keyword args forDeletedObject
HttpClient.delete()returns None on empty bodies (see mailtrap/http.py), which is typical for 204 responses. If the API ever returns a JSON body with the deleted id, we can use it; otherwise, fall back to the providedtemplate_id. Also, prefer keyword args for clarity and compatibility with dataclass/pydantic models.Apply this diff:
- self._client.delete( - f"/api/accounts/{self._account_id}/email_templates/{template_id}" - ) - return DeletedObject(template_id) + response = self._client.delete( + f"/api/accounts/{self._account_id}/email_templates/{template_id}" + ) + if isinstance(response, dict): + return DeletedObject(**response) + return DeletedObject(id=template_id)
13-16: Guard against empty responses in list endpointIf the backend ever responds with 204/empty body,
HttpClient.get()returns None, making the list comprehension raise. Short-circuit to an empty list.Apply this diff:
- response = self._client.get(f"/api/accounts/{self._account_id}/email_templates") - return [EmailTemplate(**template) for template in response] + response = self._client.get(f"/api/accounts/{self._account_id}/email_templates") + if not response: + return [] + return [EmailTemplate(**template) for template in response]
9-11: Type ofaccount_id: widen to support int or strAccount IDs are commonly integers. Widening the type avoids mypy/pyright friction when callers pass ints.
Apply this diff:
- def __init__(self, client: HttpClient, account_id: str) -> None: + def __init__(self, client: HttpClient, account_id: int | str) -> None:
8-12: Minor: centralize path construction to reduce duplicationNot blocking, but a tiny helper avoids string repetition and reduces chances of typos.
You can add inside the class:
def _path(self, tail: str = "") -> str: base = f"/api/accounts/{self._account_id}/email_templates" return f"{base}/{tail.lstrip('/')}" if tail else baseThen use:
- list: self._client.get(self._path())
- get: self._client.get(self._path(str(template_id)))
- create: self._client.post(self._path(), json=...)
- update: self._client.patch(self._path(str(template_id)), json=...)
- delete: self._client.delete(self._path(str(template_id)))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
examples/email_templates/templates.py(1 hunks)mailtrap/__init__.py(1 hunks)mailtrap/api/resources/templates.py(1 hunks)mailtrap/api/templates.py(1 hunks)mailtrap/client.py(2 hunks)mailtrap/http.py(1 hunks)mailtrap/models/common.py(1 hunks)mailtrap/models/mail/address.py(1 hunks)mailtrap/models/mail/attachment.py(2 hunks)mailtrap/models/mail/base.py(1 hunks)mailtrap/models/templates.py(1 hunks)tests/unit/api/test_email_templates.py(1 hunks)tests/unit/models/test_templates.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- mailtrap/models/mail/attachment.py
- mailtrap/api/templates.py
- mailtrap/client.py
- mailtrap/models/templates.py
- examples/email_templates/templates.py
- mailtrap/models/common.py
- mailtrap/models/mail/address.py
🧰 Additional context used
🧬 Code graph analysis (5)
tests/unit/api/test_email_templates.py (5)
mailtrap/api/resources/templates.py (6)
TemplatesApi(8-43)get_list(13-15)get_by_id(17-21)create(23-28)update(30-37)delete(39-43)mailtrap/exceptions.py (1)
APIError(10-15)mailtrap/http.py (5)
HttpClient(13-96)get(25-29)post(31-33)patch(39-41)delete(43-45)mailtrap/models/common.py (1)
DeletedObject(22-23)mailtrap/models/templates.py (3)
CreateEmailTemplateParams(9-14)EmailTemplate(40-49)UpdateEmailTemplateParams(18-36)
mailtrap/api/resources/templates.py (5)
mailtrap/http.py (5)
HttpClient(13-96)get(25-29)post(31-33)patch(39-41)delete(43-45)mailtrap/models/common.py (2)
DeletedObject(22-23)api_data(14-18)mailtrap/api/templates.py (1)
templates(11-12)mailtrap/models/templates.py (3)
CreateEmailTemplateParams(9-14)EmailTemplate(40-49)UpdateEmailTemplateParams(18-36)tests/unit/api/test_email_templates.py (1)
client(22-23)
tests/unit/models/test_templates.py (2)
mailtrap/models/templates.py (2)
CreateEmailTemplateParams(9-14)UpdateEmailTemplateParams(18-36)mailtrap/models/common.py (1)
api_data(14-18)
mailtrap/models/mail/base.py (3)
mailtrap/models/common.py (1)
ParametersObject(12-18)mailtrap/models/mail/address.py (1)
Address(9-11)mailtrap/models/mail/attachment.py (1)
Attachment(18-27)
mailtrap/__init__.py (2)
mailtrap/api/templates.py (1)
templates(11-12)mailtrap/models/templates.py (2)
CreateEmailTemplateParams(9-14)UpdateEmailTemplateParams(18-36)
🪛 Ruff (0.12.2)
mailtrap/__init__.py
13-13: .models.templates.CreateEmailTemplateParams imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
14-14: .models.templates.UpdateEmailTemplateParams imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (actions)
- GitHub Check: Test python3.10 on windows-latest
- GitHub Check: Test python3.13 on windows-latest
- GitHub Check: Test python3.11 on windows-latest
- GitHub Check: Test python3.12 on windows-latest
- GitHub Check: Test python3.9 on windows-latest
🔇 Additional comments (16)
mailtrap/http.py (1)
54-56: Good addition: gracefully handle empty successful responses.Returning
Nonefor empty bodies avoidsJSONDecodeErroron 204/empty responses and fits the delete flow in TemplatesApi.mailtrap/models/mail/base.py (1)
7-16: Rename to ParametersObject looks consistent and serialization-safe.Using
ParametersObject+@dataclasspreservesapi_dataserialization, andserialization_alias="from"onsenderwill be honored thanks toby_alias=TrueinParametersObject.api_data.tests/unit/models/test_templates.py (4)
8-16: Create params: required-only serialization covered.Solid check that
api_dataexcludesNonefields and includes required ones.
17-31: Create params: full serialization covered.Covers both
body_textandbody_html; looks good.
41-47: Update params: required-only serialization covered.Verifies selective inclusion as intended.
49-63: Update params: full serialization covered.Good parity with create-case assertions.
tests/unit/api/test_email_templates.py (8)
43-76: Error handling matrix for list endpoint is thorough.401/403 cases validated against unified error extraction; nice coverage.
77-92: Happy path for list endpoint looks good.Type-checks and ID assertion are sufficient here.
93-132: Error handling matrix for get_by_id is complete.Covers 401/403/404. Good.
133-148: Happy path for get_by_id validates model mapping.Checks type and key field; aligns with
EmailTemplate(**response).
149-186: Create: error coverage is adequate.401/403 cases validated for create.
215-256: Update: error coverage looks good.Covers 401/403/404; matches API surface.
279-318: Delete: error matrix is solid.Validates 401/403/404 for delete.
319-331: Delete: success path aligns with new HttpClient behavior.Constructing
DeletedObjectlocally and not relying on response content plays well with theNonereturn for 204.mailtrap/api/resources/templates.py (2)
17-22: Confirm response shape for get_by_idAssuming the API returns a flat object (not wrapped) is fine if covered by tests/fixtures. Some endpoints wrap payloads (e.g., {"email_template": {...}}). If that’s the case, unwrap before constructing the model.
If needed, the safe variant is:
data = response.get("email_template", response) return EmailTemplate(**data)Please confirm the real response shape used in tests/recordings.
24-27: Ignore invocation suggestion forapi_data
api_datais defined with@propertyinmailtrap/models/common.py, so it should be accessed without parentheses. All existing references (including in tests) correctly usetemplate_params.api_dataand will return adict. Invoking it asapi_data()would raise anAttributeErrorrather than aTypeError. No changes are needed here.Likely an incorrect or invalid review comment.
Ihor-Bilous
requested review from
IgorDobryn,
VladimirTaytor and
andrii-porokhnavets
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
mailtrap/models/common.py (1)
11-12: Public rename: consider a deprecation alias for RequestModel → RequestParams.This rename is a breaking change for downstream imports. To smooth migration, add a runtime alias for at least one release and update docs/changelog accordingly.
Add near the end of the module:
+# Backward-compat alias — scheduled for removal in a future minor release +RequestModel = RequestParams # deprecated; use RequestParams insteadTo double-check if anything still uses
RequestModel, you can run:#!/bin/bash # Find any lingering references to RequestModel rg -n -C2 --hidden '\bRequestModel\b' -g '!**/.git/**'
🧹 Nitpick comments (3)
mailtrap/models/common.py (2)
8-8: Drop the TypeVar here; it’s not buying us anything.
Tisn’t used in the return type, and constrainingselfvia aTypeVaradds indirection without value. Simplify the signature and avoid the forward-ref bound.Apply:
-T = TypeVar("T", bound="RequestParams") @@ - def api_data(self: T) -> dict[str, Any]: + def api_data(self) -> dict[str, Any]:Note: if you adopt this, consider removing the now-unused
TypeVarimport at the top to keep things tidy.Also applies to: 14-14
11-18: Micro-optimization: cache the TypeAdapter per class.
TypeAdapter(type(self))is constructed on every access. Ifapi_datais called frequently (e.g., in tight loops or batch operations), cache the adapter per class to avoid repeated construction.Apply within this block:
- return cast( - dict[str, Any], - TypeAdapter(type(self)).dump_python(self, by_alias=True, exclude_none=True), - ) + return cast( + dict[str, Any], + _type_adapter_for(type(self)).dump_python(self, by_alias=True, exclude_none=True), + )And add once in this module (outside the selected lines):
from functools import lru_cache from typing import Any, Type @lru_cache(maxsize=None) def _type_adapter_for(cls: Type[Any]): return TypeAdapter(cls)mailtrap/models/mail/address.py (1)
9-11: Optional: enforce email format with EmailStr.If you want validation on
pydantic.EmailStr. This keeps behavior consistent with other pydantic-backed fields and catches obvious typos early.Apply:
- email: str + email: EmailStrAnd add once at the top of this file:
from pydantic import EmailStr
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
mailtrap/models/common.py(1 hunks)mailtrap/models/mail/address.py(1 hunks)mailtrap/models/mail/attachment.py(2 hunks)mailtrap/models/mail/base.py(1 hunks)mailtrap/models/templates.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- mailtrap/models/mail/base.py
- mailtrap/models/templates.py
- mailtrap/models/mail/attachment.py
🧰 Additional context used
🧬 Code graph analysis (1)
mailtrap/models/mail/address.py (1)
mailtrap/models/common.py (1)
RequestParams(12-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (actions)
- GitHub Check: Test python3.13 on windows-latest
- GitHub Check: Test python3.12 on windows-latest
- GitHub Check: Test python3.10 on windows-latest
- GitHub Check: Test python3.9 on windows-latest
- GitHub Check: Test python3.11 on windows-latest
🔇 Additional comments (1)
mailtrap/models/mail/address.py (1)
5-5: Import change looks good.Switching the base to
RequestParamsaligns this model with the rest of the refactor. No issues here.
Motivation
In this PR I added EmailTemplatesApi.
Changes
How to test
see examples/email_templates/templates.py
Summary by CodeRabbit