Fix issue #45: Add ContactEventsApi, related models, tests and examples #51
Conversation
WalkthroughAdds contact event support: new models ( Changes
Sequence DiagramsequenceDiagram
participant User
participant Example as Example Script
participant Client as MailtrapClient
participant ContactsAPI as ContactsBaseApi
participant ContactEventsAPI as ContactEventsApi
participant HTTP as HttpClient
participant API as Mailtrap API
User->>Example: run script
Example->>Client: initialize (API_TOKEN, ACCOUNT_ID)
Example->>Client: access contacts_api
Client->>ContactsAPI: return ContactsBaseApi
Example->>ContactsAPI: access contact_events
ContactsAPI->>ContactEventsAPI: instantiate ContactEventsApi
Example->>ContactEventsAPI: create(contact_id, params)
ContactEventsAPI->>HTTP: POST /api/send/contacts/{account_id}/contacts/{contact_id}/events with payload
HTTP->>API: send request
API-->>HTTP: 201 Created + response body
HTTP->>ContactEventsAPI: return parsed response
ContactEventsAPI->>Example: return ContactEvent instance
Example->>User: print event
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 (3)
tests/unit/api/contacts/test_contact_events.py (2)
15-15: Remove unused constant.The
EXPORT_IDconstant is defined but never used in this test file.Apply this diff to remove the unused constant:
ACCOUNT_ID = "321" -EXPORT_ID = 1 CONTACT_IDENTIFIER = "d82d0d9e-bbb7-4656-a591-e64682bffae7"
126-126: Remove debug print statement.The
Apply this diff to remove it:
with pytest.raises(APIError) as exc_info: client.create(CONTACT_IDENTIFIER, sample_create_contact_event_params) - print(str(exc_info.value)) - assert expected_error_message in str(exc_info.value)mailtrap/api/resources/contact_events.py (1)
25-26: Clarify the optionality ofcontact_identifier.The
_api_pathmethod acceptscontact_identifier: Optional[str] = None, but the implementation uses it directly in the f-string without handling theNonecase. IfNoneis passed, it would produce a path like/api/accounts/{id}/contacts/None/events.Currently, the
createmethod always passes a non-None value, so this isn't causing issues. However, for code clarity, consider either:
- Removing the
Optionaland default value if the parameter is always required- Adding proper None handling if optionality is intended for future extensibility
Apply this diff to remove the unnecessary
Optional:- def _api_path(self, contact_identifier: Optional[str] = None) -> str: + def _api_path(self, contact_identifier: str) -> str: return f"/api/accounts/{self._account_id}/contacts/{contact_identifier}/events"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
examples/contacts/contact_events.py(1 hunks)mailtrap/__init__.py(1 hunks)mailtrap/api/contacts.py(2 hunks)mailtrap/api/resources/contact_events.py(1 hunks)mailtrap/models/contacts.py(2 hunks)tests/unit/api/contacts/test_contact_events.py(1 hunks)tests/unit/models/test_contacts.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
mailtrap/api/resources/contact_events.py (3)
mailtrap/http.py (2)
HttpClient(14-106)post(32-34)mailtrap/models/contacts.py (2)
ContactEvent(156-160)ContactEventParams(150-152)tests/unit/api/contacts/test_contact_events.py (1)
client(24-25)
mailtrap/__init__.py (1)
mailtrap/models/contacts.py (1)
ContactEventParams(150-152)
mailtrap/api/contacts.py (3)
mailtrap/api/resources/contact_events.py (1)
ContactEventsApi(8-26)tests/unit/api/contacts/test_contact_events.py (1)
client(24-25)tests/unit/api/contacts/test_contact_exports.py (1)
client(23-24)
examples/contacts/contact_events.py (4)
mailtrap/api/contacts.py (2)
contacts(36-37)contact_events(16-17)mailtrap/models/contacts.py (2)
ContactEvent(156-160)ContactEventParams(150-152)mailtrap/client.py (1)
MailtrapClient(31-170)mailtrap/api/resources/contact_events.py (1)
create(13-23)
tests/unit/api/contacts/test_contact_events.py (4)
mailtrap/api/resources/contact_events.py (2)
ContactEventsApi(8-26)create(13-23)mailtrap/exceptions.py (1)
APIError(10-15)mailtrap/http.py (2)
HttpClient(14-106)post(32-34)mailtrap/models/contacts.py (2)
ContactEvent(156-160)ContactEventParams(150-152)
tests/unit/models/test_contacts.py (2)
mailtrap/models/contacts.py (1)
ContactEventParams(150-152)mailtrap/models/common.py (1)
api_data(15-19)
mailtrap/models/contacts.py (1)
mailtrap/models/common.py (1)
RequestParams(13-19)
🪛 Ruff (0.14.0)
examples/contacts/contact_events.py
4-4: Possible hardcoded password assigned to: "API_TOKEN"
(S105)
⏰ 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). (1)
- GitHub Check: Test python3.10 on windows-latest
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
mailtrap/models/contacts.py (1)
155-160: Consider more restrictive typing forparams.The
ContactEventmodel structure looks correct as a response model. However, theparamsfield usesdict[str, Any], which is more permissive than theUnion[str, int, float, bool]used for similar dictionary fields elsewhere in this file (e.g.,Contact.fields,CreateContactParams.fields).Using
Anyprovides maximum flexibility but sacrifices type safety. Consider whether the more restrictive union type would be appropriate, or if the broaderAnytype is intentionally needed to support complex nested structures (lists, nested dicts, etc.).If you prefer stricter typing, apply this diff:
@dataclass class ContactEvent: contact_id: str contact_email: str name: str - params: Optional[dict[str, Any]] = None + params: Optional[dict[str, Union[str, int, float, bool]]] = NoneNote: Apply the same change to
ContactEventParams(line 152) if you choose this approach for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mailtrap/models/contacts.py(2 hunks)tests/unit/models/test_contacts.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/models/test_contacts.py
🧰 Additional context used
🧬 Code graph analysis (1)
mailtrap/models/contacts.py (1)
mailtrap/models/common.py (1)
RequestParams(13-19)
⏰ 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: Test python3.13 on macos-latest
- GitHub Check: Test python3.12 on windows-latest
- GitHub Check: Test python3.9 on windows-latest
- GitHub Check: Test python3.11 on windows-latest
- GitHub Check: Test python3.10 on windows-latest
- GitHub Check: Test python3.9 on macos-latest
- GitHub Check: Test python3.13 on windows-latest
🔇 Additional comments (2)
mailtrap/models/contacts.py (2)
3-3: LGTM!The
Anyimport is necessary for thedict[str, Any]type annotations in the new dataclasses.
149-152: LGTM!The
ContactEventParamsclass correctly inherits fromRequestParamsand follows the established pattern for request parameter classes. The structure is appropriate with a requirednamefield and an optionalparamsdictionary for flexible event parameters.
Ihor-Bilous
requested review from
IgorDobryn,
VladimirTaytor and
andrii-porokhnavets
2 participants
Motivation
In this PR I added ContactEventsApi, related models, tests, examples
How to test
Summary by CodeRabbit
New Features
Documentation
Tests