MPT-14934 add E2E tests for notifications batches #169
Conversation
albertsola
requested review from
alephsur,
d3rky,
jentyk,
robcsegal,
ruben-sebrango and
svazquezco
as code owners
WalkthroughConfiguration and service refactoring to enable file upload capability in batch notifications. Batches service now uses CreateFileMixin/AsyncCreateFileMixin instead of explicit create methods. E2E test fixtures and tests added for batch operations and category retrieval. Unit test coverage for create-with-file flows removed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
✅ Found Jira issue key in the title: MPT-14934 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
e2e_config.test.json(1 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(1 hunks)tests/e2e/notifications/batch/test_batches.py(1 hunks)tests/e2e/notifications/categories/test_async_categories.py(2 hunks)tests/e2e/notifications/categories/test_sync_categories.py(2 hunks)tests/e2e/notifications/conftest.py(1 hunks)tests/unit/resources/notifications/test_batches.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/unit/resources/notifications/test_batches.py
🧰 Additional context used
🧠 Learnings (2)
📚 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/e2e/notifications/batch/test_batches.pytests/e2e/notifications/batch/test_async_batches.pytests/e2e/notifications/categories/test_sync_categories.pytests/e2e/notifications/batch/conftest.pytests/e2e/notifications/conftest.pytests/e2e/notifications/categories/test_async_categories.py
📚 Learning: 2025-11-27T00:05:36.356Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/conftest.py:0-0
Timestamp: 2025-11-27T00:05:36.356Z
Learning: When reviewing PRs that add fixtures to a parent conftest that may duplicate fixtures in child conftest files, do not suggest removing the duplicates from child conftest files that are outside the scope of the PR's changes.
Applied to files:
tests/e2e/notifications/conftest.py
🧬 Code graph analysis (6)
tests/e2e/notifications/batch/test_batches.py (4)
mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/notifications/batch/conftest.py (3)
batch_service(5-6)batch_data(20-26)batch_id(15-16)mpt_api_client/models/model.py (1)
id(119-121)tests/e2e/conftest.py (1)
logo_fd(111-113)
tests/e2e/notifications/categories/test_sync_categories.py (4)
tests/e2e/notifications/categories/test_async_categories.py (2)
test_get_category(25-30)test_filter_categories(46-52)tests/e2e/conftest.py (1)
mpt_client(43-44)tests/e2e/notifications/conftest.py (1)
category_id(5-6)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/notifications/batch/conftest.py (2)
tests/e2e/conftest.py (4)
mpt_ops(31-32)async_mpt_ops(36-39)e2e_config(89-92)short_uuid(101-102)tests/e2e/notifications/conftest.py (1)
category_id(5-6)
tests/e2e/notifications/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
mpt_api_client/resources/notifications/batches.py (3)
mpt_api_client/http/mixins.py (5)
AsyncCreateFileMixin(187-216)AsyncGetMixin(380-395)CollectionMixin(509-575)CreateFileMixin(115-144)GetMixin(361-377)mpt_api_client/models/file_model.py (1)
FileModel(6-55)mpt_api_client/models/model.py (1)
Model(65-125)
tests/e2e/notifications/categories/test_async_categories.py (5)
tests/e2e/notifications/categories/test_sync_categories.py (3)
test_create_category(20-23)test_get_category(26-31)test_filter_categories(48-54)tests/e2e/notifications/categories/conftest.py (1)
category_data(5-10)tests/e2e/conftest.py (1)
async_mpt_vendor(24-27)tests/e2e/notifications/conftest.py (1)
category_id(5-6)mpt_api_client/models/model.py (1)
id(119-121)
🪛 Ruff (0.14.8)
tests/e2e/notifications/batch/test_batches.py
33-33: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
tests/e2e/notifications/categories/test_async_categories.py
20-20: Unused noqa directive (unknown: AAA01)
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 (16)
mpt_api_client/resources/notifications/batches.py (4)
28-28: Excellent refactoring to standardize file upload handling.The integration of
CreateFileMixin[Batch]cleanly delegates file upload logic to a reusable mixin, replacing the previous explicit implementation. The mixin is properly positioned first in the inheritance chain and correctly parameterized.
54-54: Async mixin integration mirrors sync implementation well.The
AsyncCreateFileMixin[Batch]integration maintains consistency with the synchronous service, providing the same file upload capabilities in an async context.
4-10: The imports are correct. This is a new file, not modifications to existing code.The entire
batches.pyfile was created in this commit, including theget_batch_attachmentmethods that useFileModel. TheFileModelimport on line 10 is appropriately included as part of the new file and is not fixing a missing import. All imports are properly ordered and necessary for the new service classes and methods.Likely an incorrect or invalid review comment.
23-24: The multipart form data key configuration requires verification against the SoftwareOne MPT notifications batches API specification.The configuration defines
_upload_file_key = "attachment"and_upload_data_key = "batch"for multipart uploads to the/public/v1/notifications/batchesendpoint. These keys must match the exact parameter names expected by the API. Verify these values against the official SoftwareOne Marketplace Platform API documentation for the notifications batches endpoint, or confirm through integration tests that the API accepts requests structured with these keys.e2e_config.test.json (1)
49-50: LGTM!The new configuration entries properly support the E2E test fixtures for batch and category operations.
tests/e2e/notifications/conftest.py (1)
1-6: LGTM!The fixture correctly extracts the category ID from the E2E configuration, enabling tests to reference a stable category without dynamic creation.
tests/e2e/notifications/batch/test_batches.py (4)
6-11: LGTM!The skip marker is appropriate given the constraint that batches cannot be deleted.
13-16: LGTM!The test correctly validates batch retrieval by ID.
19-23: LGTM!The test correctly validates RQL-based filtering by batch ID.
26-30: LGTM!The skip marker is appropriate, and the test demonstrates the new file upload capability for batch creation.
tests/e2e/notifications/batch/test_async_batches.py (1)
1-36: LGTM!The async batch tests correctly mirror the synchronous versions with proper async/await usage. The async comprehension on line 20 is appropriately used for the async iterator.
tests/e2e/notifications/batch/conftest.py (1)
1-26: LGTM!The fixtures are well-structured and follow the established patterns in the project. The
batch_datafixture properly generates unique test data using theshort_uuidto avoid conflicts.tests/e2e/notifications/categories/test_async_categories.py (2)
25-30: LGTM!The refactoring to use a stable
category_idfrom config is appropriate for E2E tests and avoids the overhead of creating/deleting categories.
46-52: LGTM!The refactoring to filter directly by
category_idusing RQL is more efficient than the previous approach. The async comprehension is properly used.tests/e2e/notifications/categories/test_sync_categories.py (2)
26-31: LGTM!The refactoring to use
category_idfrom config aligns with the async test changes and provides a more stable test approach.
48-54: LGTM!The RQL-based filtering correctly uses
category_id, mirroring the improvements in the async version.
| @pytest.mark.skip(reason="Batches attachments not implemented") # noqa: AAA01 | ||
| def test_download_attachment(): | ||
| # TODO - Implement get and download E2E tests for attachment | ||
| raise NotImplementedError |
There was a problem hiding this comment.
Remove unused noqa directive.
The # noqa: AAA01 directive references an unknown linting code and should be removed.
🔎 Apply this diff to remove the unused directive:
-@pytest.mark.skip(reason="Batches attachments not implemented") # noqa: AAA01
+@pytest.mark.skip(reason="Batches attachments not implemented")
def test_download_attachment():
# TODO - Implement get and download E2E tests for attachment
raise NotImplementedError📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.mark.skip(reason="Batches attachments not implemented") # noqa: AAA01 | |
| def test_download_attachment(): | |
| # TODO - Implement get and download E2E tests for attachment | |
| raise NotImplementedError | |
| @pytest.mark.skip(reason="Batches attachments not implemented") | |
| def test_download_attachment(): | |
| # TODO - Implement get and download E2E tests for attachment | |
| raise NotImplementedError |
🧰 Tools
🪛 Ruff (0.14.8)
33-33: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
🤖 Prompt for AI Agents
In tests/e2e/notifications/batch/test_batches.py around lines 33 to 36, the test
function has an unused lint suppression comment "# noqa: AAA01" that references
a nonexistent rule; remove that noqa directive so the line reads simply
@pytest.mark.skip(reason="Batches attachments not implemented") and keep the
rest of the test (TODO and raise) unchanged.
| def test_create_category(async_created_category, category_data): # noqa: AAA01 | ||
| assert async_created_category.name == category_data["name"] | ||
| assert async_created_category.description == category_data["description"] |
There was a problem hiding this comment.
Remove unused noqa directive.
The # noqa: AAA01 directive references an unknown linting code and should be removed.
🔎 Apply this diff to remove the unused directive:
-def test_create_category(async_created_category, category_data): # noqa: AAA01
+def test_create_category(async_created_category, category_data):
assert async_created_category.name == category_data["name"]
assert async_created_category.description == category_data["description"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_create_category(async_created_category, category_data): # noqa: AAA01 | |
| assert async_created_category.name == category_data["name"] | |
| assert async_created_category.description == category_data["description"] | |
| def test_create_category(async_created_category, category_data): | |
| assert async_created_category.name == category_data["name"] | |
| assert async_created_category.description == category_data["description"] |
🧰 Tools
🪛 Ruff (0.14.8)
20-20: Unused noqa directive (unknown: AAA01)
Remove unused noqa directive
(RUF100)
🤖 Prompt for AI Agents
In tests/e2e/notifications/categories/test_async_categories.py around lines 20
to 22, the test function declaration contains an unused noqa directive "# noqa:
AAA01"; remove that directive from the function definition so the line reads
without the trailing comment, leaving the test intact and ensuring no unknown
linter code is suppressed.
|
d3rky
deleted the
e2e/MPT-14934/Add-E2E-tests-for-notifications-batches-endpoints
branch
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.