Fix issue #23: Add SuppressionsApi, related models, tests and examples #39
Conversation
WalkthroughIntroduces account-level Suppressions API: new models, resource client with list/delete endpoints, a base API accessor, client integration method, example usage, and unit tests validating GET with optional email filter and DELETE behaviors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer Code
participant MC as MailtrapClient
participant SBA as SuppressionsBaseApi
participant SA as SuppressionsApi
participant HC as HttpClient (GENERAL_HOST)
rect rgb(235,245,255)
note right of Dev: Obtain API accessor
Dev->>MC: suppressions_api(account_id)
MC->>SBA: construct(account_id, HttpClient)
MC-->>Dev: SuppressionsBaseApi
Dev->>SBA: suppressions
SBA-->>Dev: SuppressionsApi
end
alt List suppressions (optional email)
Dev->>SA: get_list(email?)
SA->>HC: GET /api/accounts/{id}/suppressions[?email=...]
HC-->>SA: 200 JSON list
SA-->>Dev: list[Suppression]
else Delete suppression
Dev->>SA: delete(suppression_id)
SA->>HC: DELETE /api/accounts/{id}/suppressions/{suppression_id}
HC-->>SA: 200 JSON object
SA-->>Dev: Suppression
end
note over HC,SA: Errors propagate from HttpClient
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
✨ 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. 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: 5
🧹 Nitpick comments (8)
mailtrap/models/suppressions.py (1)
32-32: Normalize message_created_at to datetimeAPI returns ISO timestamps; let Pydantic coerce to datetime instead of allowing str|datetime.
- message_created_at: Optional[Union[str, datetime]] = None + message_created_at: Optional[datetime] = Nonemailtrap/client.py (1)
137-140: Broaden the validation error messagenow mentions only “Testing API” but this check guards Contacts/Templates/Suppressions too.
- 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 (Testing, Contacts, Templates, Suppressions)")mailtrap/api/suppressions.py (2)
10-12: Optionally cache the SuppressionsApi instanceAvoid re-instantiation per access (micro). Safe to keep as-is if consistency with other accessors is preferred.
class SuppressionsBaseApi: def __init__(self, client: HttpClient, account_id: str) -> None: self._account_id = account_id self._client = client + self._suppressions: SuppressionsApi | None = None @property def suppressions(self) -> SuppressionsApi: - return SuppressionsApi(account_id=self._account_id, client=self._client) + if self._suppressions is None: + self._suppressions = SuppressionsApi( + account_id=self._account_id, client=self._client + ) + return self._suppressions
10-12: Rename parameter to suppression_id for consistency (resources)Small naming inconsistency: “suppressions_id” → “suppression_id”.
Apply in mailtrap/api/resources/suppressions.py:
- def delete(self, suppressions_id: str) -> Suppression: - response = self._client.delete(self._api_path(suppressions_id)) + def delete(self, suppression_id: str) -> Suppression: + response = self._client.delete(self._api_path(suppression_id)) return Suppression(**response) @@ - def _api_path(self, suppressions_id: Optional[str] = None) -> str: + def _api_path(self, suppression_id: Optional[str] = None) -> str: path = f"/api/accounts/{self._account_id}/suppressions" - if suppressions_id is not None: - return f"{path}/{suppressions_id}" + if suppression_id is not None: + return f"{path}/{suppression_id}" return pathtests/unit/api/test_suppressions.py (1)
79-80: Ruff S101 in testsIf your CI enforces S101, add per-file ignores for tests or disable S101 under tests in ruff config.
Example pyproject entry:
[tool.ruff.lint.per-file-ignores] "tests/**/*" = ["S101"]Also applies to: 93-95, 110-113, 152-152, 166-170
examples/suppressions.py (1)
6-7: Nit: fix placeholders and avoid hardcoding secretsUse env vars and correct “YOUR_...” placeholders.
-from typing import Optional +from typing import Optional +import os @@ -API_TOKEN = "YOU_API_TOKEN" -ACCOUNT_ID = "YOU_ACCOUNT_ID" +API_TOKEN = os.environ.get("MAILTRAP_API_TOKEN", "YOUR_API_TOKEN") +ACCOUNT_ID = os.environ.get("MAILTRAP_ACCOUNT_ID", "YOUR_ACCOUNT_ID")mailtrap/api/resources/suppressions.py (2)
16-24: Renamesuppressions_id→suppression_idfor clarity/consistency.Reads better and matches singular resource semantics.
- def delete(self, suppressions_id: str) -> Suppression: - response = self._client.delete(self._api_path(suppressions_id)) + def delete(self, suppression_id: str) -> Suppression: + response = self._client.delete(self._api_path(suppression_id)) return Suppression(**response) - def _api_path(self, suppressions_id: Optional[str] = None) -> str: + def _api_path(self, suppression_id: Optional[str] = None) -> str: path = f"/api/accounts/{self._account_id}/suppressions" - if suppressions_id is not None: - return f"{path}/{suppressions_id}" + if suppression_id is not None: + return f"{path}/{suppression_id}" return path
7-11: Add a brief class docstring.Helps discoverability in IDEs and docs.
class SuppressionsApi: + """Account-level Suppressions resource: list and delete suppressions.""" def __init__(self, client: HttpClient, account_id: str) -> None: self._account_id = account_id self._client = client
📜 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 (6)
examples/suppressions.py(1 hunks)mailtrap/api/resources/suppressions.py(1 hunks)mailtrap/api/suppressions.py(1 hunks)mailtrap/client.py(2 hunks)mailtrap/models/suppressions.py(1 hunks)tests/unit/api/test_suppressions.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
mailtrap/api/suppressions.py (2)
mailtrap/api/resources/suppressions.py (1)
SuppressionsApi(7-24)mailtrap/http.py (1)
HttpClient(13-96)
examples/suppressions.py (4)
mailtrap/api/suppressions.py (1)
suppressions(11-12)mailtrap/models/suppressions.py (1)
Suppression(22-38)mailtrap/client.py (1)
suppressions_api(77-82)mailtrap/api/resources/suppressions.py (2)
get_list(12-14)delete(16-18)
tests/unit/api/test_suppressions.py (5)
mailtrap/api/resources/suppressions.py (3)
SuppressionsApi(7-24)get_list(12-14)delete(16-18)mailtrap/exceptions.py (1)
APIError(10-15)mailtrap/http.py (2)
HttpClient(13-96)get(25-29)mailtrap/models/suppressions.py (3)
SendingStream(16-18)Suppression(22-38)SuppressionType(9-13)mailtrap/client.py (1)
suppressions_api(77-82)
mailtrap/api/resources/suppressions.py (3)
mailtrap/http.py (2)
HttpClient(13-96)get(25-29)mailtrap/api/suppressions.py (1)
suppressions(11-12)mailtrap/models/suppressions.py (1)
Suppression(22-38)
mailtrap/client.py (2)
mailtrap/api/suppressions.py (2)
suppressions(11-12)SuppressionsBaseApi(5-12)mailtrap/http.py (1)
HttpClient(13-96)
🪛 Ruff (0.12.2)
examples/suppressions.py
6-6: Possible hardcoded password assigned to: "API_TOKEN"
(S105)
tests/unit/api/test_suppressions.py
79-79: Use of assert detected
(S101)
93-93: Use of assert detected
(S101)
94-94: Use of assert detected
(S101)
95-95: Use of assert detected
(S101)
110-110: Use of assert detected
(S101)
111-111: Use of assert detected
(S101)
112-112: Use of assert detected
(S101)
113-113: Use of assert detected
(S101)
152-152: Use of assert detected
(S101)
166-166: Use of assert detected
(S101)
167-167: Use of assert detected
(S101)
168-168: Use of assert detected
(S101)
169-169: Use of assert detected
(S101)
170-170: Use of assert detected
(S101)
🔇 Additional comments (4)
mailtrap/models/suppressions.py (1)
9-18: Verify enum string values against APIPlease confirm the exact server values for SuppressionType and SendingStream to avoid casing/spacing mismatches at runtime.
mailtrap/client.py (1)
76-82: LGTM: new suppressions_api accessor mirrors existing patternsConstructor, host, and headers are consistent with other API accessors.
tests/unit/api/test_suppressions.py (2)
62-80: 401 behavior: ensure test expectation matches exceptions.AuthorizationError subtypeHttpClient raises AuthorizationError for 401. If it’s not a subclass of APIError, this test will miss it. Please confirm inheritance or assert the precise type for 401.
154-171: Consider delete 204 No Content scenarioIf the API returns 204, current code constructs Suppression from None. Either adjust delete() to handle None or add a test to lock behavior.
Ihor-Bilous
force-pushed
the
ISSUE-23
branch
from
e3b1f44 to
f837f7f
Compare
Ihor-Bilous
requested review from
IgorDobryn,
i7an,
VladimirTaytor and
andrii-porokhnavets
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
examples/suppressions.py (1)
17-18: Good: delete() typed to return Suppression (not Optional).Matches API behavior returning the deleted entity body.
tests/unit/api/test_suppressions.py (1)
103-108: Nice: assert the email query param with responses.matchers.Prevents false positives and locks the contract.
🧹 Nitpick comments (1)
tests/unit/api/test_suppressions.py (1)
1-1: Silence Ruff S101 for test assertions (optional).If CI flags S101 in tests, disable it file-wide.
+# ruff: noqa: S101 from datetime import datetime
📜 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 (6)
examples/suppressions.py(1 hunks)mailtrap/api/resources/suppressions.py(1 hunks)mailtrap/api/suppressions.py(1 hunks)mailtrap/client.py(2 hunks)mailtrap/models/suppressions.py(1 hunks)tests/unit/api/test_suppressions.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- mailtrap/api/suppressions.py
- mailtrap/models/suppressions.py
- mailtrap/api/resources/suppressions.py
- mailtrap/client.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Ihor-Bilous
PR: railsware/mailtrap-python#39
File: mailtrap/api/resources/suppressions.py:16-18
Timestamp: 2025-09-04T16:28:41.593Z
Learning: Mailtrap Suppressions API: DELETE /api/accounts/{account_id}/suppressions/{id} returns HTTP 200 OK with a JSON body of the deleted suppression. Keep SDK delete() typed as -> Suppression (not Optional).
📚 Learning: 2025-09-04T16:28:41.593Z
Learnt from: Ihor-Bilous
PR: railsware/mailtrap-python#39
File: mailtrap/api/resources/suppressions.py:16-18
Timestamp: 2025-09-04T16:28:41.593Z
Learning: Mailtrap Suppressions API: DELETE /api/accounts/{account_id}/suppressions/{id} returns HTTP 200 OK with a JSON body of the deleted suppression. Keep SDK delete() typed as -> Suppression (not Optional).
Applied to files:
examples/suppressions.pytests/unit/api/test_suppressions.py
🧬 Code graph analysis (2)
examples/suppressions.py (4)
mailtrap/api/suppressions.py (1)
suppressions(11-12)mailtrap/models/suppressions.py (1)
Suppression(22-38)mailtrap/client.py (2)
MailtrapClient(25-151)suppressions_api(77-82)mailtrap/api/resources/suppressions.py (2)
get_list(12-15)delete(17-19)
tests/unit/api/test_suppressions.py (4)
mailtrap/api/resources/suppressions.py (3)
SuppressionsApi(7-25)get_list(12-15)delete(17-19)mailtrap/exceptions.py (1)
APIError(10-15)mailtrap/http.py (2)
HttpClient(13-96)get(25-29)mailtrap/models/suppressions.py (3)
SendingStream(16-18)Suppression(22-38)SuppressionType(9-13)
🪛 Ruff (0.12.2)
examples/suppressions.py
6-6: Possible hardcoded password assigned to: "API_TOKEN"
(S105)
tests/unit/api/test_suppressions.py
80-80: Use of assert detected
(S101)
94-94: Use of assert detected
(S101)
95-95: Use of assert detected
(S101)
96-96: Use of assert detected
(S101)
112-112: Use of assert detected
(S101)
113-113: Use of assert detected
(S101)
114-114: Use of assert detected
(S101)
115-115: Use of assert detected
(S101)
116-116: Use of assert detected
(S101)
155-155: Use of assert detected
(S101)
169-169: Use of assert detected
(S101)
170-170: Use of assert detected
(S101)
171-171: Use of assert detected
(S101)
172-172: Use of assert detected
(S101)
173-173: Use of assert detected
(S101)
⏰ 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). (5)
- GitHub Check: Test python3.11 on windows-latest
- GitHub Check: Test python3.13 on windows-latest
- GitHub Check: Test python3.12 on windows-latest
- GitHub Check: Test python3.9 on windows-latest
- GitHub Check: Test python3.10 on windows-latest
🔇 Additional comments (1)
tests/unit/api/test_suppressions.py (1)
167-173: Delete path test covers enums and typing correctly.Validates id, type, email, and sending_stream per model.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/api/suppressions/test_suppressions.py (1)
1-174: Add explicit parsing or converters for datetime fields
TheSuppressionsmodel is a plain@dataclass, soSuppression(**payload)won’t convert ISO-8601 strings todatetime—tests expectingcreated_at(andmessage_created_at) asdatetimewill fail. You need to either:
- Change
Suppressionsto inherit from Pydantic’sBaseModel(which auto-parsesdatetime), or- Add a
__post_init__in the dataclass to manuallydatetime.fromisoformatthose fields.
🧹 Nitpick comments (6)
examples/suppressions/suppressions.py (2)
21-22: Make example runnable with optional email filter and nicer output.Small UX improvement for readers trying the example.
-if __name__ == "__main__": - print(list_suppressions()) +if __name__ == "__main__": + email = os.environ.get("MAILTRAP_EMAIL_FILTER") + suppressions = list_suppressions(email=email) + for s in suppressions: + print(f"{s.id} {s.type} {s.email} {s.sending_stream} {s.created_at}")
6-7: Fix placeholders typo.Change "YOU_API_TOKEN/ID" to "YOUR_API_TOKEN/ID" if you keep placeholders anywhere.
tests/unit/api/suppressions/test_suppressions.py (4)
1-2: Silence Ruff S101 in tests file-wide.Asserts are fine in tests; add a per-file ignore to keep CI green.
-from datetime import datetime + # ruff: noqa: S101 +from datetime import datetime
92-97: Also assert enum/value mapping on list endpoint.You already validate this on DELETE; mirroring it here increases confidence in parsing.
- assert suppressions[0].id == SUPPRESSION_ID + assert suppressions[0].id == SUPPRESSION_ID + assert suppressions[0].type == SuppressionType.UNSUBSCRIPTION + assert suppressions[0].sending_stream == SendingStream.TRANSACTIONAL
82-91: Add an empty-list case for GET.Covers edge-case and guards against None being returned.
@responses.activate def test_get_suppressions_should_return_empty_list(self, suppressions_api: SuppressionsApi) -> None: responses.get(BASE_SUPPRESSIONS_URL, json=[], status=200) suppressions = suppressions_api.get_list() assert suppressions == []
26-44: Optional: validate datetime parsing for nested field.Since model allows
message_created_atto be str or datetime, add an assertion it’s parsed to datetime if your model normalizes it.assert isinstance(suppressions[0].created_at, datetime) # If model normalizes nested timestamp: # assert isinstance(suppressions[0].message_created_at, datetime)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/suppressions/suppressions.py(1 hunks)mailtrap/api/resources/suppressions.py(1 hunks)tests/unit/api/suppressions/test_suppressions.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mailtrap/api/resources/suppressions.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Ihor-Bilous
PR: railsware/mailtrap-python#39
File: mailtrap/api/resources/suppressions.py:16-18
Timestamp: 2025-09-04T16:28:41.602Z
Learning: Mailtrap Suppressions API: DELETE /api/accounts/{account_id}/suppressions/{id} returns HTTP 200 OK with a JSON body of the deleted suppression. Keep SDK delete() typed as -> Suppression (not Optional).
📚 Learning: 2025-09-04T16:28:41.602Z
Learnt from: Ihor-Bilous
PR: railsware/mailtrap-python#39
File: mailtrap/api/resources/suppressions.py:16-18
Timestamp: 2025-09-04T16:28:41.602Z
Learning: Mailtrap Suppressions API: DELETE /api/accounts/{account_id}/suppressions/{id} returns HTTP 200 OK with a JSON body of the deleted suppression. Keep SDK delete() typed as -> Suppression (not Optional).
Applied to files:
tests/unit/api/suppressions/test_suppressions.pyexamples/suppressions/suppressions.py
🧬 Code graph analysis (2)
tests/unit/api/suppressions/test_suppressions.py (4)
mailtrap/api/resources/suppressions.py (3)
SuppressionsApi(7-33)get_list(12-19)delete(21-27)mailtrap/exceptions.py (1)
APIError(10-15)mailtrap/http.py (2)
HttpClient(13-96)get(25-29)mailtrap/models/suppressions.py (3)
SendingStream(16-18)Suppression(22-38)SuppressionType(9-13)
examples/suppressions/suppressions.py (4)
mailtrap/api/suppressions.py (1)
suppressions(11-12)mailtrap/models/suppressions.py (1)
Suppression(22-38)mailtrap/client.py (1)
MailtrapClient(25-151)mailtrap/api/resources/suppressions.py (2)
get_list(12-19)delete(21-27)
🪛 Ruff (0.12.2)
tests/unit/api/suppressions/test_suppressions.py
80-80: Use of assert detected
(S101)
94-94: Use of assert detected
(S101)
95-95: Use of assert detected
(S101)
96-96: Use of assert detected
(S101)
112-112: Use of assert detected
(S101)
113-113: Use of assert detected
(S101)
114-114: Use of assert detected
(S101)
115-115: Use of assert detected
(S101)
116-116: Use of assert detected
(S101)
155-155: Use of assert detected
(S101)
169-169: Use of assert detected
(S101)
170-170: Use of assert detected
(S101)
171-171: Use of assert detected
(S101)
172-172: Use of assert detected
(S101)
173-173: Use of assert detected
(S101)
examples/suppressions/suppressions.py
6-6: 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). (5)
- GitHub Check: Test python3.9 on windows-latest
- GitHub Check: Test python3.12 on windows-latest
- GitHub Check: Test python3.13 on windows-latest
- GitHub Check: Test python3.10 on windows-latest
- GitHub Check: Test python3.11 on windows-latest
🔇 Additional comments (3)
examples/suppressions/suppressions.py (2)
6-10: Avoid hardcoded secrets; load from environment and fail fast.Use env vars to satisfy S105 and prevent accidental leaks. Also validate presence before creating the client.
-from typing import Optional +from typing import Optional +import os -API_TOKEN = "YOU_API_TOKEN" -ACCOUNT_ID = "YOU_ACCOUNT_ID" +API_TOKEN = os.environ.get("MAILTRAP_API_TOKEN", "") +ACCOUNT_ID = os.environ.get("MAILTRAP_ACCOUNT_ID", "") -client = mt.MailtrapClient(token=API_TOKEN, account_id=ACCOUNT_ID) +if not API_TOKEN or not ACCOUNT_ID: + raise SystemExit("Set MAILTRAP_API_TOKEN and MAILTRAP_ACCOUNT_ID environment variables.") + +client = mt.MailtrapClient(token=API_TOKEN, account_id=ACCOUNT_ID)⛔ Skipped due to learnings
Learnt from: Ihor-Bilous PR: railsware/mailtrap-python#39 File: examples/suppressions.py:1-10 Timestamp: 2025-09-04T19:31:01.169Z Learning: In the mailtrap-python repository, all example files consistently use placeholder strings like `API_TOKEN = "YOU_API_TOKEN"` and `ACCOUNT_ID = "YOU_ACCOUNT_ID"` instead of environment variable lookups. This pattern should be maintained for consistency across examples.Learnt from: Ihor-Bilous PR: railsware/mailtrap-python#39 File: mailtrap/api/resources/suppressions.py:16-18 Timestamp: 2025-09-04T16:28:41.602Z Learning: Mailtrap Suppressions API: DELETE /api/accounts/{account_id}/suppressions/{id} returns HTTP 200 OK with a JSON body of the deleted suppression. Keep SDK delete() typed as -> Suppression (not Optional).
9-11: No changes required:suppressionsis a property
Thesuppressionsaccessor in SuppressionsBaseApi is decorated with@property(mailtrap/api/suppressions.py:10–12), so usingclient.suppressions_api.suppressionscorrectly returns the API instance.tests/unit/api/suppressions/test_suppressions.py (1)
158-174: Good coverage of DELETE happy-path and error cases.Return typing as Suppression aligns with API behavior (200 + JSON body). LGTM.
| @@ -72,6 +73,14 @@ def contacts_api(self) -> ContactsBaseApi: | |||
| client=HttpClient(host=GENERAL_HOST, headers=self.headers), | |||
| ) | |||
|
|
|||
| @property | |||
| def suppressions_api(self) -> SuppressionsBaseApi: | |||
There was a problem hiding this comment.
Based on the API dosc, the suppressions_api should be part of the sending API 🤔. On the other hand, the NodeJS SDK uses the same design (suppressions api is a separate resource). So I guess we are good here, let's better be aligned with other SDKs
There was a problem hiding this comment.
Yes, I followed the same design
Motivation
In this PR I added SuppressionsApi.
Changes
How to test
Summary by CodeRabbit
New Features
Documentation
Tests