[MPT-14914] Added e2e tests and updated endpoints for billing journal attachments #174
Conversation
robcsegal
requested review from
albertsola,
alephsur,
d3rky,
jentyk,
ruben-sebrango and
svazquezco
as code owners
📝 WalkthroughWalkthroughReplaces composite file-related mixins with granular Create/Update/Download/Delete/Get mixins for journal attachments (sync + async), adds upload config keys, introduces an e2e config ID, new e2e fixtures and sync/async tests, updates lint ignores, and extends unit test method lists. Changes
Sequence Diagram(s)(omitted — changes are API refactor and tests without a new multi-component sequential flow that requires visualization) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e_config.test.jsonmpt_api_client/resources/billing/journal_attachments.pypyproject.tomltests/e2e/billing/journal/attachment/conftest.pytests/e2e/billing/journal/attachment/test_async_journal_attachment.pytests/e2e/billing/journal/attachment/test_sync_journal_attachment.pytests/unit/resources/billing/test_journal_attachments.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-01T10:48:52.586Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 136
File: tests/e2e/notifications/subscribers/conftest.py:14-25
Timestamp: 2025-12-01T10:48:52.586Z
Learning: The repository softwareone-platform/mpt-api-python-client uses wemake-python-styleguide linter, so noqa directives for WPS rules should not be flagged as unused.
Applied to files:
pyproject.toml
📚 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/billing/test_journal_attachments.pytests/e2e/billing/journal/attachment/conftest.pytests/e2e/billing/journal/attachment/test_async_journal_attachment.pytests/e2e/billing/journal/attachment/test_sync_journal_attachment.py
🧬 Code graph analysis (5)
mpt_api_client/resources/billing/journal_attachments.py (1)
mpt_api_client/http/mixins.py (11)
AsyncCreateFileMixin(187-216)AsyncDeleteMixin(273-283)AsyncDownloadFileMixin(303-325)AsyncGetMixin(380-395)AsyncUpdateMixin(286-300)CollectionMixin(509-575)CreateFileMixin(115-144)DeleteMixin(29-38)DownloadFileMixin(58-80)GetMixin(361-377)UpdateMixin(41-55)
tests/unit/resources/billing/test_journal_attachments.py (16)
tests/unit/resources/commerce/test_subscriptions.py (1)
test_methods_present(22-25)tests/unit/resources/catalog/test_price_list_items.py (1)
test_methods_present(40-43)tests/unit/resources/catalog/test_product_term_variants.py (1)
test_methods_present(47-50)tests/unit/resources/catalog/test_pricing_policy_attachments.py (1)
test_methods_present(44-47)tests/unit/resources/catalog/test_product_terms.py (1)
test_methods_present(42-45)tests/unit/resources/catalog/test_products_documents.py (1)
test_methods_present(37-40)tests/unit/resources/catalog/test_products_item_groups.py (1)
test_methods_present(34-37)tests/unit/resources/catalog/test_products_media.py (1)
test_methods_present(37-40)tests/unit/resources/catalog/test_products_parameter_groups.py (1)
test_methods_present(39-42)tests/unit/resources/catalog/test_products_parameters.py (1)
test_methods_present(34-37)tests/unit/resources/commerce/test_agreements_attachments.py (1)
test_methods_present(36-39)tests/unit/resources/accounts/test_accounts_users.py (1)
test_methods_present(43-46)tests/unit/resources/accounts/test_account_users.py (1)
test_methods_present(27-30)tests/unit/resources/billing/test_custom_ledger_attachments.py (1)
test_methods_present(40-43)tests/unit/resources/billing/test_custom_ledger_charges.py (1)
test_methods_present(40-43)tests/unit/resources/billing/test_credit_memo_attachments.py (1)
test_methods_present(42-45)
tests/e2e/billing/journal/attachment/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
tests/e2e/billing/journal/attachment/test_async_journal_attachment.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/conftest.py (2)
async_mpt_ops(36-39)pdf_fd(73-75)tests/e2e/billing/journal/attachment/conftest.py (4)
journal_attachment_factory(20-29)journal_attachment_id(10-11)invalid_journal_attachment_id(5-6)journal_attachment_file_name(15-16)mpt_api_client/models/file_model.py (2)
file_contents(35-45)filename(13-32)
tests/e2e/billing/journal/attachment/test_sync_journal_attachment.py (6)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/conftest.py (1)
pdf_fd(73-75)tests/e2e/billing/journal/attachment/conftest.py (3)
journal_attachment_factory(20-29)journal_attachment_id(10-11)journal_attachment_file_name(15-16)tests/e2e/billing/conftest.py (1)
billing_journal_id(17-18)mpt_api_client/models/file_model.py (2)
file_contents(35-45)filename(13-32)
🪛 Ruff (0.14.10)
tests/e2e/billing/journal/attachment/test_async_journal_attachment.py
27-27: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/billing/journal/attachment/test_sync_journal_attachment.py
23-23: Unused noqa directive (unknown: WPS421)
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)
e2e_config.test.json (1)
14-15: LGTM! Test configuration values added correctly.The new configuration entries for journal attachment testing follow the existing naming conventions and provide the necessary test data for the e2e test suite.
pyproject.toml (1)
122-122: LGTM! Linting suppression aligns with the refactoring.The addition of WPS235 (too many imported names) suppression is appropriate given the refactoring of journal attachments to use granular mixins, which increases the number of imports in the billing resources.
tests/unit/resources/billing/test_journal_attachments.py (2)
40-40: LGTM! Extended test coverage for new methods.The addition of "iterate" and "download" to the parametrize list correctly validates that the refactored service classes expose these methods via the new granular mixins (CollectionMixin provides iterate, DownloadFileMixin provides download).
47-47: LGTM! Async methods coverage extended.The async test suite now correctly validates the "iterate" and "download" methods provided by the async mixin refactor.
tests/e2e/billing/journal/attachment/test_sync_journal_attachment.py (2)
9-24: LGTM! Proper fixture setup with teardown.The fixture correctly creates a journal attachment for testing and includes appropriate teardown with error handling. Good use of try-except to prevent test failures if the attachment was already deleted during the test.
31-93: LGTM! Comprehensive e2e test coverage.The test suite covers all essential operations:
- Retrieval by ID with both success and 404 cases
- Listing with pagination
- Filtering with RQLQuery and field selection
- Create, update, and delete operations
- Download with file content and filename validation
The flaky marker is appropriate for e2e tests that depend on external services.
tests/e2e/billing/journal/attachment/conftest.py (1)
4-29: LGTM! Well-structured test fixtures.The fixtures follow established patterns in the codebase:
- Invalid ID uses the correct format with zeros
- Config-based fixtures properly extract test data
- Factory pattern correctly implemented with default values
tests/e2e/billing/journal/attachment/test_async_journal_attachment.py (2)
69-72: LGTM! Correct use of synchronous test function with async fixture.This correctly uses
definstead ofasync defsince the test body contains no await statements. Pytest-asyncio will automatically resolve the async fixture. This pattern avoids linter complaints about unnecessary async functions.Based on learnings, this is the correct approach.
35-101: LGTM! Comprehensive async e2e test coverage.The async test suite properly mirrors the sync version with appropriate async/await patterns:
- All async operations correctly use await
- Test coverage is comprehensive (get, list, filter, create, update, delete, download)
- Error handling and assertions are appropriate
- Flaky marker is suitable for e2e tests
mpt_api_client/resources/billing/journal_attachments.py (4)
4-14: LGTM! Refactored to granular mixins for better clarity.The change from composite mixins (FilesOperationsMixin, ModifiableResourceMixin) to granular mixins (CreateFileMixin, UpdateMixin, DownloadFileMixin, DeleteMixin, GetMixin) improves the API surface by making the available operations explicit and discoverable.
29-30: LGTM! Upload configuration keys properly defined.The
_upload_file_keyand_upload_data_keyconfiguration keys are correctly defined for use by the CreateFileMixin, which expects these attributes to structure multipart file uploads.
33-43: LGTM! Service class composition updated with granular mixins.The service class now explicitly composes the granular mixins, making the available operations (create, update, download, delete, get, iterate) clear from the class definition. This improves maintainability and API discoverability.
46-56: LGTM! Async service mirrors sync implementation.The async service class correctly mirrors the sync version with async-specific mixins, maintaining consistency across the API surface.
tests/e2e/billing/journal/attachment/test_async_journal_attachment.py
Outdated
Show resolved
Hide resolved
tests/e2e/billing/journal/attachment/test_sync_journal_attachment.py
Outdated
Show resolved
Hide resolved
robcsegal
force-pushed
the
MPT-14914-add-e-2-e-tests-for-billing-journal-attachments
branch
from
7a8e6fc to
4084872
Compare
robcsegal
force-pushed
the
MPT-14914-add-e-2-e-tests-for-billing-journal-attachments
branch
from
4084872 to
7ce9379
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/e2e/billing/journal/attachment/test_sync_journal_attachment.py (1)
9-24: LGTM! Fixture follows proper teardown pattern.The
created_journal_attachmentfixture correctly creates a journal attachment for testing, yields it, and handles cleanup with exception handling.Note: There's a known issue with the print statement on Line 23 that was flagged in previous reviews (the
%splaceholder won't be interpolated). This is already tracked.tests/e2e/billing/journal/attachment/test_async_journal_attachment.py (1)
10-28: LGTM! Async fixture with proper cleanup.The async
created_journal_attachmentfixture correctly creates a journal attachment asynchronously, yields it, and handles cleanup with exception handling.Note: There's a known issue with the print statement on Line 27 that was flagged in previous reviews. This is already tracked.
🧹 Nitpick comments (1)
mpt_api_client/resources/billing/journal_attachments.py (1)
4-14: LGTM! Improved mixin granularity.The refactoring from composite mixins (
FilesOperationsMixin,ModifiableResourceMixin) to granular mixins (CreateFileMixin,UpdateMixin,DownloadFileMixin,DeleteMixin,GetMixin) provides:
- Better separation of concerns (SOLID principle)
- More explicit API surface
- Easier maintenance and testing
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e_config.test.jsonmpt_api_client/resources/billing/journal_attachments.pypyproject.tomltests/e2e/billing/journal/attachment/conftest.pytests/e2e/billing/journal/attachment/test_async_journal_attachment.pytests/e2e/billing/journal/attachment/test_sync_journal_attachment.pytests/unit/resources/billing/test_journal_attachments.py
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e_config.test.json
- pyproject.toml
🧰 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/e2e/billing/journal/attachment/test_sync_journal_attachment.pytests/e2e/billing/journal/attachment/test_async_journal_attachment.pytests/unit/resources/billing/test_journal_attachments.pytests/e2e/billing/journal/attachment/conftest.py
🧬 Code graph analysis (4)
mpt_api_client/resources/billing/journal_attachments.py (1)
mpt_api_client/http/mixins.py (11)
AsyncCreateFileMixin(187-216)AsyncDeleteMixin(273-283)AsyncDownloadFileMixin(303-325)AsyncGetMixin(380-395)AsyncUpdateMixin(286-300)CollectionMixin(509-575)CreateFileMixin(115-144)DeleteMixin(29-38)DownloadFileMixin(58-80)GetMixin(361-377)UpdateMixin(41-55)
tests/e2e/billing/journal/attachment/test_sync_journal_attachment.py (6)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/conftest.py (2)
mpt_ops(31-32)pdf_fd(73-75)tests/e2e/billing/journal/attachment/conftest.py (2)
journal_attachment_factory(15-24)journal_attachment_id(10-11)tests/e2e/billing/conftest.py (1)
billing_journal_id(17-18)mpt_api_client/models/file_model.py (2)
file_contents(35-45)filename(13-32)
tests/unit/resources/billing/test_journal_attachments.py (16)
tests/unit/resources/commerce/test_subscriptions.py (1)
test_methods_present(22-25)tests/unit/resources/catalog/test_price_list_items.py (1)
test_methods_present(40-43)tests/unit/resources/catalog/test_pricing_policy_attachments.py (1)
test_methods_present(44-47)tests/unit/resources/catalog/test_product_term_variants.py (1)
test_methods_present(47-50)tests/unit/resources/catalog/test_products_documents.py (1)
test_methods_present(37-40)tests/unit/resources/catalog/test_products_media.py (1)
test_methods_present(37-40)tests/unit/resources/catalog/test_products_item_groups.py (1)
test_methods_present(34-37)tests/unit/resources/catalog/test_product_terms.py (1)
test_methods_present(42-45)tests/unit/resources/catalog/test_products_parameters.py (1)
test_methods_present(34-37)tests/unit/resources/catalog/test_products_parameter_groups.py (1)
test_methods_present(39-42)tests/unit/resources/commerce/test_agreements_attachments.py (1)
test_methods_present(36-39)tests/unit/resources/accounts/test_account_users.py (1)
test_methods_present(27-30)tests/unit/resources/accounts/test_accounts_users.py (1)
test_methods_present(43-46)tests/unit/resources/billing/test_credit_memo_attachments.py (1)
test_methods_present(42-45)tests/unit/resources/billing/test_custom_ledger_attachments.py (1)
test_methods_present(40-43)tests/unit/resources/billing/test_custom_ledger_charges.py (1)
test_methods_present(40-43)
tests/e2e/billing/journal/attachment/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
🪛 Ruff (0.14.10)
tests/e2e/billing/journal/attachment/test_sync_journal_attachment.py
23-23: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/billing/journal/attachment/test_async_journal_attachment.py
27-27: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (7)
tests/unit/resources/billing/test_journal_attachments.py (1)
40-40: LGTM! Method coverage correctly extended.The unit tests now verify the presence of
iterateanddownloadmethods on both sync and async journal attachments services, aligning with the new granular mixins (DownloadFileMixin,CollectionMixin) introduced in the implementation.Also applies to: 47-47
tests/e2e/billing/journal/attachment/test_sync_journal_attachment.py (1)
31-91: LGTM! Comprehensive test coverage.The test suite thoroughly covers the journal attachments API:
- Get operations (success and 404 scenarios)
- Pagination with
fetch_page- Filtering with RQLQuery and field selection
- CRUD operations (create, update, delete)
- File download with validation of filename and contents
tests/e2e/billing/journal/attachment/conftest.py (1)
1-24: LGTM! Well-structured test fixtures.The fixtures follow standard pytest patterns:
invalid_journal_attachment_id: Provides a hardcoded invalid ID for 404 test scenariosjournal_attachment_id: Retrieves the test attachment ID from e2e configurationjournal_attachment_factory: Factory pattern for creating test attachment payloads with customizable namestests/e2e/billing/journal/attachment/test_async_journal_attachment.py (2)
69-73: LGTM! Correct usage of async fixture in synchronous test.This test correctly uses a
def(synchronous) declaration while consuming the asynccreated_journal_attachmentfixture. Per the retrieved learning, pytest-asyncio will resolve async fixtures automatically when the test function contains no await statements, avoiding unnecessary async function warnings.Based on learnings, this pattern is the recommended approach for tests that only need to access fixture values without performing async operations.
35-99: LGTM! Comprehensive async test coverage.The async test suite properly covers:
- Async get operations with proper
awaitusage- Exception handling for 404 scenarios
- Async pagination and filtering
- Async comprehension for iteration (Line 64)
- CRUD operations with appropriate async/await
- File download validation
mpt_api_client/resources/billing/journal_attachments.py (2)
29-30: LGTM! Upload configuration keys added.The
_upload_file_keyand_upload_data_keyconfiguration keys properly support multipart/form-data file uploads, specifying where the file and attachment metadata should be placed in the request payload.
33-43: LGTM! Consistent sync and async service implementations.Both
JournalAttachmentsServiceandAsyncJournalAttachmentsServicecorrectly compose the granular mixins:
- File operations:
CreateFileMixin/AsyncCreateFileMixin,DownloadFileMixin/AsyncDownloadFileMixin- Resource operations:
UpdateMixin/AsyncUpdateMixin,DeleteMixin/AsyncDeleteMixin,GetMixin/AsyncGetMixin- Collection operations:
CollectionMixin/AsyncCollectionMixinThis provides a complete API surface for journal attachment management.
Also applies to: 46-56
robcsegal
force-pushed
the
MPT-14914-add-e-2-e-tests-for-billing-journal-attachments
branch
from
7ce9379 to
c9745dd
Compare
robcsegal
force-pushed
the
MPT-14914-add-e-2-e-tests-for-billing-journal-attachments
branch
from
c9745dd to
38a2e56
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/e2e/billing/journal/attachment/test_sync_journal_attachment.py (1)
20-23: Fix the print statement formatting.The
print()function doesn't interpolate%splaceholders likeprintf. The current code will output:TEARDOWN - Unable to delete journal attachment: %s <error.title>instead of the intended message.🔎 Proposed fix
try: journal_attachments.delete(created_journal.id) except MPTAPIError as error: - print("TEARDOWN - Unable to delete journal attachment: %s", error.title) # noqa: WPS421 + print(f"TEARDOWN - Unable to delete journal attachment: {error.title}") # noqa: WPS421Note: The
noqa: WPS421directive is valid for wemake-python-styleguide; the Ruff warning about it being unused is a false positive since Ruff doesn't recognize WPS codes.tests/e2e/billing/journal/attachment/test_async_journal_attachment.py (1)
24-27: Fix the print statement formatting.Same issue as the sync version -
print()doesn't interpolate%splaceholders.🔎 Proposed fix
try: await journal_attachments.delete(created_journal.id) except MPTAPIError as error: - print("TEARDOWN - Unable to delete journal attachment: %s", error.title) # noqa: WPS421 + print(f"TEARDOWN - Unable to delete journal attachment: {error.title}") # noqa: WPS421
🧹 Nitpick comments (1)
tests/e2e/billing/journal/attachment/test_sync_journal_attachment.py (1)
63-84: Consider adding stronger assertions for create/update tests.The create and update tests only verify
result is not None. Consider validating that the returned object contains the expected field values (e.g.,result.name == "E2E Created Journal Attachment"for create,result.name == updated_namefor update) to ensure the API behaves correctly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e_config.test.jsonmpt_api_client/resources/billing/journal_attachments.pypyproject.tomltests/e2e/billing/journal/attachment/conftest.pytests/e2e/billing/journal/attachment/test_async_journal_attachment.pytests/e2e/billing/journal/attachment/test_sync_journal_attachment.pytests/unit/resources/billing/test_journal_attachments.py
🚧 Files skipped from review as they are similar to previous changes (4)
- e2e_config.test.json
- tests/unit/resources/billing/test_journal_attachments.py
- pyproject.toml
- tests/e2e/billing/journal/attachment/conftest.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/e2e/billing/journal/attachment/test_async_journal_attachment.pytests/e2e/billing/journal/attachment/test_sync_journal_attachment.py
🧬 Code graph analysis (2)
mpt_api_client/resources/billing/journal_attachments.py (1)
mpt_api_client/http/mixins.py (10)
AsyncCreateFileMixin(187-216)AsyncDeleteMixin(273-283)AsyncDownloadFileMixin(303-325)AsyncGetMixin(380-395)AsyncUpdateMixin(286-300)CreateFileMixin(115-144)DeleteMixin(29-38)DownloadFileMixin(58-80)GetMixin(361-377)UpdateMixin(41-55)
tests/e2e/billing/journal/attachment/test_sync_journal_attachment.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/conftest.py (1)
pdf_fd(73-75)tests/e2e/billing/journal/attachment/conftest.py (3)
journal_attachment_factory(15-24)journal_attachment_id(10-11)invalid_journal_attachment_id(5-6)tests/e2e/billing/conftest.py (1)
billing_journal_id(17-18)mpt_api_client/models/file_model.py (2)
file_contents(35-45)filename(13-32)
🪛 Ruff (0.14.10)
tests/e2e/billing/journal/attachment/test_async_journal_attachment.py
27-27: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/billing/journal/attachment/test_sync_journal_attachment.py
23-23: Unused noqa directive (unknown: WPS421)
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/billing/journal_attachments.py (3)
2-15: LGTM!The import changes correctly bring in the granular mixins to replace the previous composite ones. The organization is clean with async and sync variants properly separated.
29-30: LGTM!The upload configuration keys are correctly added to support the
CreateFileMixinandAsyncCreateFileMixinwhich use_upload_file_keyfor the file field and_upload_data_keyfor the JSON payload in multipart requests.
33-55: LGTM!The service classes correctly compose the granular mixins with proper type parameterization. The MRO places mixins before the base
Service/AsyncServiceclasses, ensuring method resolution works correctly.DeleteMixinandAsyncDeleteMixincorrectly don't use type parameters since they returnNone.tests/e2e/billing/journal/attachment/test_sync_journal_attachment.py (5)
1-6: LGTM!Good structure with appropriate imports and the
pytest.mark.flakymarker for e2e tests that may have transient failures.
31-39: LGTM!Good coverage of the get operation with both success and 404 error handling tests. The error matching pattern
r"404 Not Found"properly validates the expected API response.
42-60: LGTM!Good coverage of listing with pagination and filtering with RQL queries. The filter test validates chaining multiple
filter()calls withselect().
81-84: Note: Intentional double-delete pattern.The test explicitly deletes the attachment, and the
created_journal_attachmentfixture's teardown will attempt to delete it again. The teardown handles this gracefully by catchingMPTAPIError, so this is safe. Just noting this is expected behavior.
87-91: LGTM!Good download test validating both
file_contentsandfilenameare present. This exercises theDownloadFileMixinfunctionality.tests/e2e/billing/journal/attachment/test_async_journal_attachment.py (5)
1-6: LGTM!Good structure with appropriate imports and flaky marker for e2e tests.
35-53: LGTM!Good async test implementations with proper
awaitusage for API calls.
56-66: LGTM!The async iteration pattern with
async foris correctly used for the filter test.
69-72: LGTM!Correct use of a synchronous test function here since no
awaitis needed - the async fixture is resolved automatically by pytest-asyncio. Based on learnings, this is the preferred pattern.
75-99: LGTM!The remaining async tests properly use
awaitfor all API operations. Good coverage of update, delete, and download functionality.
|
robcsegal
deleted the
MPT-14914-add-e-2-e-tests-for-billing-journal-attachments
branch
Added e2e tests and updated endpoints for billing journal attachments
https://softwareone.atlassian.net/browse/MPT-14914
Summary by CodeRabbit
Tests
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.