Fix issue #19: Add ContactFieldsApi, related models, tests, examples #35
Conversation
WalkthroughAdds contact field support: new Pydantic models, ContactFieldsApi resource with CRUD methods, a ContactsBaseApi facade, MailtrapClient.contacts_api accessor, package re-exports, an example script, and unit tests for API and models. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Client as MailtrapClient
participant Contacts as ContactsBaseApi
participant Fields as ContactFieldsApi
participant HTTP as HttpClient
Dev->>Client: access contacts_api
Client->>Contacts: instantiate with client, account_id
Dev->>Contacts: access contact_fields
Contacts->>Fields: instantiate with client, account_id
rect rgba(200,230,255,0.25)
Dev->>Fields: get_list() / get_by_id(id)
Fields->>HTTP: GET /api/accounts/{account_id}/contacts/fields[/{id}]
HTTP-->>Fields: JSON -> ContactField(s)
Fields-->>Dev: ContactField or list[ContactField]
end
rect rgba(200,255,200,0.18)
Dev->>Fields: create(params)
Fields->>HTTP: POST .../fields (body: params.api_data)
HTTP-->>Fields: 201 JSON -> ContactField
Fields-->>Dev: ContactField
end
rect rgba(255,240,200,0.18)
Dev->>Fields: update(id, params)
Fields->>HTTP: PATCH .../fields/{id} (body)
HTTP-->>Fields: JSON -> ContactField
Fields-->>Dev: ContactField
end
rect rgba(255,220,220,0.18)
Dev->>Fields: delete(id)
Fields->>HTTP: DELETE .../fields/{id}
HTTP-->>Fields: 204 No Content
Fields-->>Dev: DeletedObject(id)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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: 3
🧹 Nitpick comments (9)
mailtrap/models/contacts.py (3)
8-13: CreateContactFieldParams looks good; consider constraining data_typeImplementation is straightforward and compatible with RequestParams.api_data. If the API accepts a closed set of data_type values (e.g., "integer", "string", "boolean", ...), consider Literal/Enum to catch mistakes early and improve editor assistance.
16-22: UpdateContactFieldParams validation: solid; optionally normalize blank inputThe at-least-one check is correct and matches tests. To avoid sending empty strings (common 422s), trim values and treat blanks as None before the check.
Apply this diff to normalize and re-validate:
def __post_init__(self) -> None: - if all(value is None for value in [self.name, self.merge_tag]): + # Normalize blank strings to None to avoid empty updates + if isinstance(self.name, str): + self.name = self.name.strip() or None + if isinstance(self.merge_tag, str): + self.merge_tag = self.merge_tag.strip() or None + if all(value is None for value in (self.name, self.merge_tag)): raise ValueError("At least one field must be provided for update action")
25-30: Consider making ContactField immutableIf ContactField is a response DTO, freezing helps prevent accidental mutation after parsing.
-@dataclass +@dataclass(frozen=True) class ContactField:tests/unit/models/test_contacts.py (1)
24-53: LGTM: UpdateContactFieldParams behavior covered; consider one more caseGreat coverage: error on empty, partial update, and exclude_none serialization. Optionally add a case where only merge_tag is provided, and (if you accept the normalization diff) a case where whitespace-only values raise the same error.
mailtrap/client.py (1)
67-74: Error message context for missing account_id may confuse userscontacts_api reuses _validate_account_id(), which raises: "
account_idis required for Testing API". This can mislead when accessing Contacts or Templates APIs.Two low-risk options:
- Generalize the message in _validate_account_id to "
account_idis required" (may affect tests relying on the old string).- Or add an optional context parameter:
- def _validate_account_id(self) -> None: - if not self.account_id: - raise ClientConfigurationError("`account_id` is required for Testing API") + def _validate_account_id(self, context: str = "") -> None: + if not self.account_id: + suffix = f" for {context}" if context else "" + raise ClientConfigurationError(f"`account_id` is required{suffix}")…and call it here as
_validate_account_id("Contacts API"). If you prefer not to change the signature, consider leaving as-is to keep parity with other properties.mailtrap/__init__.py (1)
7-9: Re-exports trigger Ruff F401; declare them in all (preferred) or silence per-lineStatic analysis flags these imports as unused, even though they’re intended for re-export.
Option A (preferred): ensure these names are in all.
from .models.contacts import ContactField from .models.contacts import CreateContactFieldParams from .models.contacts import UpdateContactFieldParams + +# Keep __all__ authoritative for the public API surface +try: + __all__ +except NameError: + __all__ = [] +__all__ += ["ContactField", "CreateContactFieldParams", "UpdateContactFieldParams"]Option B (minimal): silence Ruff for these lines.
-from .models.contacts import ContactField -from .models.contacts import CreateContactFieldParams -from .models.contacts import UpdateContactFieldParams +from .models.contacts import ContactField # noqa: F401 +from .models.contacts import CreateContactFieldParams # noqa: F401 +from .models.contacts import UpdateContactFieldParams # noqa: F401tests/unit/api/test_contacts.py (1)
96-101: Strengthen assertions for list mapping.You only assert
id. Since the response includes full field data, assert all mapped attributes to ensure model construction remains correct.Apply this diff:
contact_fields = contact_fields_api.get_list() assert isinstance(contact_fields, list) assert all(isinstance(f, ContactField) for f in contact_fields) assert contact_fields[0].id == FIELD_ID + assert contact_fields[0].name == "First name" + assert contact_fields[0].data_type == "text" + assert contact_fields[0].merge_tag == "first_name"mailtrap/api/resources/contact_fields.py (1)
9-11: Constructor argument order consistency.Other APIs typically accept
account_idfirst, thenclient(and the facade uses keyword args). Consider normalizing the signature to__init__(self, account_id: str, client: HttpClient)for consistency. Since all current call sites use keywords, this is a safe change.If you prefer, I can submit a follow-up patch across resources to standardize parameter order.
examples/contacts/contact_fields.py (1)
7-8: Nit: fix placeholder strings.Use “YOUR_” in placeholders to avoid confusing readers.
Apply this diff:
-API_TOKEN = "YOU_API_TOKEN" -ACCOUNT_ID = "YOU_ACCOUNT_ID" +API_TOKEN = "YOUR_API_TOKEN" +ACCOUNT_ID = "YOUR_ACCOUNT_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 (8)
examples/contacts/contact_fields.py(1 hunks)mailtrap/__init__.py(1 hunks)mailtrap/api/contacts.py(1 hunks)mailtrap/api/resources/contact_fields.py(1 hunks)mailtrap/client.py(2 hunks)mailtrap/models/contacts.py(1 hunks)tests/unit/api/test_contacts.py(1 hunks)tests/unit/models/test_contacts.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
mailtrap/client.py (2)
mailtrap/api/contacts.py (1)
ContactsBaseApi(5-12)mailtrap/http.py (1)
HttpClient(13-96)
mailtrap/api/resources/contact_fields.py (3)
mailtrap/http.py (4)
HttpClient(13-96)get(25-29)post(31-33)patch(39-41)mailtrap/models/common.py (2)
DeletedObject(22-23)api_data(14-18)mailtrap/models/contacts.py (3)
ContactField(26-30)CreateContactFieldParams(9-12)UpdateContactFieldParams(16-22)
tests/unit/models/test_contacts.py (2)
mailtrap/models/contacts.py (2)
CreateContactFieldParams(9-12)UpdateContactFieldParams(16-22)mailtrap/models/common.py (1)
api_data(14-18)
mailtrap/__init__.py (1)
mailtrap/models/contacts.py (3)
ContactField(26-30)CreateContactFieldParams(9-12)UpdateContactFieldParams(16-22)
examples/contacts/contact_fields.py (5)
mailtrap/models/common.py (1)
DeletedObject(22-23)mailtrap/models/contacts.py (3)
ContactField(26-30)CreateContactFieldParams(9-12)UpdateContactFieldParams(16-22)mailtrap/client.py (2)
MailtrapClient(24-142)contacts_api(68-73)mailtrap/api/contacts.py (1)
contact_fields(11-12)mailtrap/api/resources/contact_fields.py (5)
create(23-28)update(30-37)get_list(13-15)get_by_id(17-21)delete(39-43)
tests/unit/api/test_contacts.py (6)
mailtrap/api/contacts.py (1)
contact_fields(11-12)mailtrap/api/resources/contact_fields.py (6)
ContactFieldsApi(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 (4)
HttpClient(13-96)get(25-29)post(31-33)patch(39-41)mailtrap/models/common.py (1)
DeletedObject(22-23)mailtrap/models/contacts.py (3)
ContactField(26-30)CreateContactFieldParams(9-12)UpdateContactFieldParams(16-22)
mailtrap/models/contacts.py (1)
mailtrap/models/common.py (1)
RequestParams(12-18)
mailtrap/api/contacts.py (2)
mailtrap/api/resources/contact_fields.py (1)
ContactFieldsApi(8-43)mailtrap/http.py (1)
HttpClient(13-96)
🪛 Ruff (0.12.2)
mailtrap/__init__.py
7-7: .models.contacts.ContactField imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
8-8: .models.contacts.CreateContactFieldParams imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
9-9: .models.contacts.UpdateContactFieldParams imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
🔇 Additional comments (7)
tests/unit/models/test_contacts.py (1)
7-22: LGTM: api_data for CreateContactFieldParamsTest clearly asserts the serialized payload matches expectations.
mailtrap/client.py (1)
67-74: contacts_api accessor is consistent with existing client surfaceAccount validation + GENERAL_HOST HttpClient mirrors other APIs. Looks good.
mailtrap/api/contacts.py (1)
5-12: ContactsBaseApi is a clean, minimal facadeConstructor and contact_fields property are straightforward and align with the resource API. No issues.
tests/unit/api/test_contacts.py (1)
23-26: Fixture construction looks good.Instantiating
ContactFieldsApiwith keyword args avoids the ctor-parameter-order issue and keeps the fixture readable.mailtrap/api/resources/contact_fields.py (2)
39-43: Delete flow is sensible.Returning a lightweight
DeletedObjectafter a 204 aligns with the rest of the SDK patterns.
23-28: Bug: sending a method object instead of JSON payload (missing api_data() call).
field_params.api_datais a callable that must be invoked; otherwiserequestsreceives an unserializable function object and the call breaks at runtime.Apply this diff:
def create(self, field_params: CreateContactFieldParams) -> ContactField: response = self._client.post( f"/api/accounts/{self._account_id}/contacts/fields", - json=field_params.api_data, + json=field_params.api_data(), ) return ContactField(**response) @@ response = self._client.patch( f"/api/accounts/{self._account_id}/contacts/fields/{field_id}", - json=field_params.api_data, + json=field_params.api_data(), ) return ContactField(**response)Also applies to: 33-36
⛔ Skipped due to learnings
Learnt from: Ihor-Bilous PR: railsware/mailtrap-python#34 File: mailtrap/api/resources/templates.py:33-36 Timestamp: 2025-08-21T10:37:54.214Z Learning: In the mailtrap-python codebase, api_data is a property on ParametersObject (and classes that inherit from it), not a method. It should be accessed without parentheses: template_params.api_data, not template_params.api_data().examples/contacts/contact_fields.py (1)
14-25: Example create flow looks correct.Parameter construction and delegation to the API are straightforward.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
examples/contacts/contact_fields.py (1)
27-45: Good: IDs typed as int across helper functionsAligns with API signatures and reduces misuse. This also addresses prior feedback.
🧹 Nitpick comments (5)
mailtrap/api/resources/projects.py (1)
41-43: Safer None-check for ID in path builderUsing “if project_id:” treats 0 as falsy. While IDs are typically positive, prefer an explicit None-check for correctness and consistency with future changes.
- if project_id: + if project_id is not None: return f"{path}/{project_id}"mailtrap/api/resources/templates.py (1)
45-47: Prefer explicit None-check in _api_pathSame rationale as for Projects/ContactFields: avoid truthiness checks on IDs.
- if template_id: + if template_id is not None: return f"{path}/{template_id}"mailtrap/api/resources/contact_fields.py (2)
25-31: Payload shape: confirm server expects flat body (not wrapped)Templates use {"email_template": ...}, while ContactFields POST uses a flat payload (field_params.api_data). If the API expects wrapping (e.g., {"contact_field": ...}), this would fail at runtime.
If not already covered by unit tests against a mocked response, please confirm with the API docs or integration tests that POST/PUT/PATCH for contact fields are flat.
I can adjust the implementation once confirmed.
45-49: Explicit None-check in _api_pathUse is not None to avoid edge cases and match other resources.
- if field_id: + if field_id is not None: return f"{path}/{field_id}"examples/contacts/contact_fields.py (1)
7-11: Use env vars and fix placeholders for safer examplesAvoid hardcoding tokens and prefer YOUR_* placeholders. This prevents accidental credential leaks and mirrors common SDK examples.
-from mailtrap.models.common import DeletedObject +from mailtrap.models.common import DeletedObject +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") @@ -client = mt.MailtrapClient(token=API_TOKEN, account_id=ACCOUNT_ID) -contact_fields_api = client.contacts_api.contact_fields +client = mt.MailtrapClient(token=API_TOKEN, account_id=ACCOUNT_ID) +contact_fields_api = client.contacts_api.contact_fields
📜 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 (4)
examples/contacts/contact_fields.py(1 hunks)mailtrap/api/resources/contact_fields.py(1 hunks)mailtrap/api/resources/projects.py(2 hunks)mailtrap/api/resources/templates.py(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Ihor-Bilous
PR: railsware/mailtrap-python#35
File: examples/contacts/contact_fields.py:11-11
Timestamp: 2025-08-22T13:51:31.381Z
Learning: In mailtrap/api/contacts.py, ContactsBaseApi.contact_fields is defined as a property, not a method, so it can be accessed without parentheses like client.contacts_api.contact_fields.
Learnt from: Ihor-Bilous
PR: railsware/mailtrap-python#35
File: examples/contacts/contact_fields.py:11-11
Timestamp: 2025-08-22T13:51:31.381Z
Learning: In mailtrap/api/contacts.py, ContactsBaseApi.contact_fields is defined as a property, not a method, so it can be accessed without parentheses like client.contacts_api.contact_fields.
📚 Learning: 2025-08-22T13:51:31.381Z
Learnt from: Ihor-Bilous
PR: railsware/mailtrap-python#35
File: examples/contacts/contact_fields.py:11-11
Timestamp: 2025-08-22T13:51:31.381Z
Learning: In mailtrap/api/contacts.py, ContactsBaseApi.contact_fields is defined as a property, not a method, so it can be accessed without parentheses like client.contacts_api.contact_fields.
Applied to files:
examples/contacts/contact_fields.pymailtrap/api/resources/contact_fields.py
🧬 Code graph analysis (4)
mailtrap/api/resources/projects.py (5)
mailtrap/http.py (4)
get(25-29)post(31-33)patch(39-41)delete(43-45)mailtrap/api/resources/contact_fields.py (5)
_api_path(45-49)get_by_id(19-23)create(25-30)update(32-39)delete(41-43)mailtrap/api/resources/templates.py (5)
_api_path(43-47)get_by_id(19-21)create(23-28)update(30-37)delete(39-41)mailtrap/models/projects.py (1)
Project(16-21)mailtrap/models/common.py (1)
DeletedObject(22-23)
mailtrap/api/resources/templates.py (5)
mailtrap/http.py (3)
get(25-29)post(31-33)delete(43-45)mailtrap/api/resources/contact_fields.py (4)
_api_path(45-49)get_by_id(19-23)create(25-30)delete(41-43)mailtrap/api/resources/projects.py (4)
_api_path(39-43)get_by_id(17-19)create(21-26)delete(35-37)mailtrap/models/templates.py (2)
EmailTemplate(40-49)CreateEmailTemplateParams(9-14)mailtrap/models/common.py (2)
api_data(14-18)DeletedObject(22-23)
examples/contacts/contact_fields.py (4)
mailtrap/models/common.py (1)
DeletedObject(22-23)mailtrap/models/contacts.py (3)
ContactField(26-30)CreateContactFieldParams(9-12)UpdateContactFieldParams(16-22)mailtrap/client.py (1)
contacts_api(68-73)mailtrap/api/resources/contact_fields.py (5)
create(25-30)update(32-39)get_list(15-17)get_by_id(19-23)delete(41-43)
mailtrap/api/resources/contact_fields.py (5)
mailtrap/http.py (4)
HttpClient(13-96)get(25-29)post(31-33)patch(39-41)mailtrap/models/common.py (2)
DeletedObject(22-23)api_data(14-18)mailtrap/models/contacts.py (3)
ContactField(26-30)CreateContactFieldParams(9-12)UpdateContactFieldParams(16-22)mailtrap/api/resources/projects.py (6)
get_list(13-15)_api_path(39-43)get_by_id(17-19)create(21-26)update(28-33)delete(35-37)mailtrap/api/resources/templates.py (6)
get_list(15-17)_api_path(43-47)get_by_id(19-21)create(23-28)update(30-37)delete(39-41)
⏰ 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). (6)
- GitHub Check: Test python3.13 on windows-latest
- GitHub Check: Test python3.12 on windows-latest
- GitHub Check: Test python3.11 on windows-latest
- GitHub Check: Test python3.9 on macos-latest
- GitHub Check: Test python3.10 on windows-latest
- GitHub Check: Test python3.9 on windows-latest
🔇 Additional comments (6)
mailtrap/api/resources/projects.py (1)
13-19: Nice refactor: centralized path construction improves maintainabilityReplacing inline strings with _api_path() reduces duplication and the risk of drift across methods. Good consistency with other resources.
Also applies to: 21-33
mailtrap/api/resources/templates.py (1)
15-21: Good move: adopting _api_path across CRUD methodsThe centralized path builder makes these methods leaner and less error-prone. Parity with Projects/ContactFields APIs looks solid.
Also applies to: 23-37
mailtrap/api/resources/contact_fields.py (3)
10-17: Solid resource scaffoldingConstructor and get_list mapping mirror existing patterns (Templates/Projects). Clear and consistent.
41-43: LGTM: delete semantics consistent with TemplatesIgnoring the body and returning DeletedObject(field_id) matches HttpClient behavior for 204 responses and avoids None unpacking errors.
32-39: Review comment update: Pydantic dataclass__post_init__guard is effectiveThe original comment assumed
UpdateContactFieldParamswas a PydanticBaseModel, but it’s actually a Pydantic dataclass (imported viafrom pydantic.dataclasses import dataclass), so its__post_init__method will be called after validation under Pydantic v2 and enforce the “at least one field” check as written (docs.pydantic.dev).If you prefer to follow Pydantic v2 idiomatic hooks, you may optionally migrate this guard to a
@model_validator(mode="after"), but it’s not required for correctness —__post_init__in Pydantic dataclasses still runs after validation (docs.pydantic.dev).Likely an incorrect or invalid review comment.
examples/contacts/contact_fields.py (1)
11-11: Correct: contact_fields is a property (no parentheses needed)Matches ContactsBaseApi design. Thanks for keeping this ergonomic.
Motivation
In this PR I added ContactFieldsApi.
Changes
How to test
Images and GIFs
Summary by CodeRabbit