MPT-16512 E2E for Notification batch attachments #171
Conversation
albertsola
requested review from
alephsur,
d3rky,
jentyk,
robcsegal,
ruben-sebrango and
svazquezco
as code owners
📝 WalkthroughWalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test/E2E
participant Client as BatchesService (sync/async)
participant API as Remote Notifications API
participant Storage as File storage / Download endpoint
rect rgb(235,245,255)
Note over Test,Client: Metadata flow (new)
Test->>Client: get(batch_id, select=["attachments"])
Client->>API: GET /notifications/batches/{batch_id}?select=attachments
API-->>Client: 200 JSON (batch + attachments metadata)
Client-->>Test: Batch with attachments (including attachment ids)
end
rect rgb(245,255,235)
Note over Test,Client: Attachment metadata vs binary download
Test->>Client: get_attachment(batch_id, attachment_id)
Client->>API: GET /notifications/batches/{batch_id}/attachments/{attachment_id}/metadata
API-->>Client: 200 JSON (attachment metadata -> BatchAttachment)
Client-->>Test: BatchAttachment
Test->>Client: download_attachment(batch_id, attachment_id)
Client->>Storage: GET /notifications/batches/{batch_id}/attachments/{attachment_id}/file
Storage-->>Client: 200 Binary (file bytes + headers)
Client-->>Test: FileModel (filename, content)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-12-12T15:02:20.732ZApplied to files:
🧬 Code graph analysis (4)tests/e2e/notifications/batch/test_async_batches.py (5)
tests/unit/resources/notifications/test_batches.py (4)
mpt_api_client/resources/notifications/batches.py (3)
tests/e2e/notifications/batch/conftest.py (1)
🪛 Ruff (0.14.10)tests/unit/resources/notifications/test_batches.py114-114: Unused Remove unused (RUF100) ⏰ 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)
🔇 Additional comments (14)
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: 0
🧹 Nitpick comments (1)
tests/unit/resources/notifications/test_batches.py (1)
62-77: Consider removing the unusednoqadirective.The test correctly validates async attachment download behavior. However, Ruff flags the
noqa: WPS210directive as unused because it's specific to wemake-python-styleguide, which may not be active in your linting pipeline.🔎 Suggested fix
-async def test_async_download_attachment( # noqa: WPS210 +async def test_async_download_attachment( mocker, async_batches_service, attachment_response ):
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e_config.test.json(1 hunks)mpt_api_client/resources/notifications/batches.py(2 hunks)tests/e2e/notifications/batch/conftest.py(1 hunks)tests/e2e/notifications/batch/test_async_batches.py(2 hunks)tests/e2e/notifications/batch/test_batches.py(2 hunks)tests/unit/resources/notifications/test_batches.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/unit/resources/notifications/test_batches.pytests/e2e/notifications/batch/conftest.pytests/e2e/notifications/batch/test_batches.pytests/e2e/notifications/batch/test_async_batches.py
🧬 Code graph analysis (5)
tests/unit/resources/notifications/test_batches.py (5)
mpt_api_client/http/async_client.py (1)
AsyncHTTPClient(22-117)mpt_api_client/http/client.py (1)
HTTPClient(35-129)mpt_api_client/http/types.py (1)
Response(27-42)mpt_api_client/models/file_model.py (1)
FileModel(6-55)mpt_api_client/resources/notifications/batches.py (3)
BatchesService(27-50)download_attachment(36-50)download_attachment(62-75)
mpt_api_client/resources/notifications/batches.py (1)
mpt_api_client/models/file_model.py (1)
FileModel(6-55)
tests/e2e/notifications/batch/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
tests/e2e/notifications/batch/test_batches.py (3)
tests/unit/resources/notifications/test_batches.py (1)
test_download_attachment(47-59)mpt_api_client/resources/notifications/batches.py (2)
download_attachment(36-50)download_attachment(62-75)mpt_api_client/models/file_model.py (1)
filename(13-32)
tests/e2e/notifications/batch/test_async_batches.py (5)
tests/e2e/notifications/batch/conftest.py (3)
async_batch_service(10-11)batch_id(15-16)batch_attachment_id(30-31)tests/e2e/notifications/batch/test_batches.py (1)
test_download_attachment(33-36)tests/unit/resources/notifications/test_batches.py (1)
test_download_attachment(47-59)mpt_api_client/resources/notifications/batches.py (2)
download_attachment(36-50)download_attachment(62-75)mpt_api_client/models/file_model.py (1)
filename(13-32)
🪛 Ruff (0.14.8)
tests/unit/resources/notifications/test_batches.py
62-62: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
⏰ 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: build
🔇 Additional comments (13)
mpt_api_client/resources/notifications/batches.py (2)
36-50: LGTM! Semantic improvement with the method rename.The rename from
get_batch_attachmenttodownload_attachmentbetter conveys the action being performed. The implementation is correct and the docstring has been appropriately updated.
62-75: LGTM! Async variant correctly implements the renamed method.The async version mirrors the synchronous implementation correctly with proper
awaitusage and consistent return type.e2e_config.test.json (1)
49-49: LGTM! Configuration entry added for E2E attachment testing.The new attachment ID follows the existing pattern and integrates well with the E2E test fixtures.
tests/e2e/notifications/batch/conftest.py (1)
29-31: LGTM! Fixture properly exposes attachment ID for E2E tests.The new fixture follows the established pattern and provides the attachment ID from the E2E configuration for testing attachment download functionality.
tests/e2e/notifications/batch/test_batches.py (2)
14-14: LGTM! Testing selective field inclusion.The test now exercises the
selectparameter to ensure attachments are included in the batch response when requested.
33-36: LGTM! E2E test validates attachment download functionality.The test correctly exercises the
download_attachmentmethod and verifies the returned filename matches the expected attachment.tests/e2e/notifications/batch/test_async_batches.py (2)
14-14: LGTM! Async test exercises selective field inclusion.The async test correctly includes the
selectparameter to fetch attachments with the batch.
33-36: LGTM! Async E2E test properly validates attachment download.The test correctly uses
awaitand validates the same behavior as the synchronous version.tests/unit/resources/notifications/test_batches.py (5)
3-5: LGTM! Improved type safety with explicit imports.Adding type annotations and the FileModel import enhances code clarity and type checking.
13-19: LGTM! Type annotations improve fixture clarity.The explicit type hints for both fixtures enhance code readability and enable better IDE support.
22-28: LGTM! Well-constructed fixture for attachment testing.The mock response includes appropriate headers and content for testing attachment download behavior.
31-44: LGTM! Parametrized tests cover the renamed method.Both sync and async tests now verify the presence of the
download_attachmentmethod on their respective service classes.
47-59: LGTM! Unit test validates synchronous attachment download.The test correctly mocks the HTTP client, verifies the FileModel return type, and ensures the correct endpoint is called.
albertsola
force-pushed
the
e2e/MPT-16512/notifications-batches-attachments-endpoints
branch
2 times, most recently
from
51b8a57 to
4ce3954
Compare
albertsola
force-pushed
the
e2e/MPT-16512/notifications-batches-attachments-endpoints
branch
2 times, most recently
from
5f5b678 to
29da3a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/resources/notifications/test_batches.py (1)
114-116: Remove unusednoqadirective.Static analysis indicates
WPS210is not triggered here. Remove the unnecessary suppression comment.🔎 Proposed fix
-async def test_async_download_attachment( # noqa: WPS210 +async def test_async_download_attachment( mocker, async_batches_service, attachment_response ) -> None:
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
e2e_config.test.json(1 hunks)mpt_api_client/http/async_client.py(0 hunks)mpt_api_client/resources/notifications/batches.py(3 hunks)tests/e2e/notifications/batch/conftest.py(1 hunks)tests/e2e/notifications/batch/test_async_batches.py(2 hunks)tests/e2e/notifications/batch/test_batches.py(2 hunks)tests/unit/http/test_async_client.py(0 hunks)tests/unit/resources/notifications/test_batches.py(1 hunks)
💤 Files with no reviewable changes (2)
- tests/unit/http/test_async_client.py
- mpt_api_client/http/async_client.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/e2e/notifications/batch/conftest.py
- e2e_config.test.json
- tests/e2e/notifications/batch/test_async_batches.py
- tests/e2e/notifications/batch/test_batches.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/unit/resources/notifications/test_batches.py
🧬 Code graph analysis (1)
mpt_api_client/resources/notifications/batches.py (3)
mpt_api_client/models/model.py (2)
Model(65-125)from_response(102-116)mpt_api_client/http/base_service.py (1)
path(28-30)mpt_api_client/models/file_model.py (1)
FileModel(6-55)
🪛 Ruff (0.14.8)
tests/unit/resources/notifications/test_batches.py
114-114: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
⏰ 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: build
🔇 Additional comments (11)
mpt_api_client/resources/notifications/batches.py (5)
17-18: LGTM!The new
BatchAttachmentmodel follows the established pattern and correctly extendsModelto inherit thefrom_responseclass method and other base functionality.
40-56: LGTM!Clean implementation with explicit
Accept: application/jsonheader to differentiate metadata retrieval from binary download.
58-72: LGTM!The download method correctly omits the Accept header to receive binary content and wraps the response in
FileModelfor file handling.
84-99: LGTM!Async implementation correctly mirrors the synchronous version with proper
awaitusage.
101-114: LGTM!Async implementation is consistent with the synchronous version.
tests/unit/resources/notifications/test_batches.py (6)
1-39: LGTM!Well-structured fixtures with appropriate mock responses for both JSON metadata (
batch_attachment_response) and binary download (attachment_response) scenarios.
41-58: LGTM!Parameterized tests correctly extended to verify the new attachment methods exist on both sync and async services.
61-76: LGTM!Comprehensive test that verifies both the return type and the HTTP call details including the
Accept: application/jsonheader.
79-96: LGTM!Async test correctly uses
awaitand mirrors the sync test coverage.
99-111: LGTM!Test correctly verifies the download method returns
FileModeland makes the HTTP call without the Accept header.
117-129: LGTM!Test correctly verifies the async download method behavior.
albertsola
force-pushed
the
e2e/MPT-16512/notifications-batches-attachments-endpoints
branch
from
29da3a4 to
1a287eb
Compare
|
d3rky
deleted the
e2e/MPT-16512/notifications-batches-attachments-endpoints
branch
Summary by CodeRabbit
API Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.