[MPT-14927] Added e2e tests for custom ledgers endpoints #187
Conversation
📝 WalkthroughWalkthroughThis PR removes the dedicated CustomLedgerUpload resource module and integrates file upload functionality directly into the CustomLedgersService with new upload methods. It adds XLSX mimetype support, introduces test configuration entries for custom ledger identifiers, and adds comprehensive E2E test coverage for custom ledger operations including charges and attachments. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 (4)
tests/e2e/billing/custom_ledger/attachment/test_async_custom_ledger_attachment.py (1)
27-27: Remove unusednoqadirective.Ruff does not recognize
WPS421(a wemake-python-styleguide rule). Remove the directive or replace with a Ruff-compatible suppression if needed.Suggested fix
- print(f"TEARDOWN - Unable to delete custom ledger attachment: {error.title}") # noqa: WPS421 + print(f"TEARDOWN - Unable to delete custom ledger attachment: {error.title}")tests/e2e/billing/custom_ledger/attachment/test_sync_custom_ledger_attachment.py (2)
27-27: Remove unusednoqadirective.Same issue as the async counterpart -
WPS421is not recognized by Ruff.Suggested fix
- print(f"TEARDOWN - Unable to delete custom ledger attachment: {error.title}") # noqa: WPS421 + print(f"TEARDOWN - Unable to delete custom ledger attachment: {error.title}")
83-88: Consider asserting the updated name value.The async version asserts
updated_attachment.name == updated_name, but this sync version only checksresult is not None. Consider aligning assertions for consistency.Suggested fix
result = custom_ledger_attachments.update( created_custom_ledger_attachment.id, update_data, ) - assert result is not None + assert result.name == updated_namempt_api_client/resources/billing/custom_ledgers.py (1)
49-82: Hardcoded XLSX mimetype differs from other upload implementations.The upload method hardcodes
MIMETYPE_EXCEL_XLSXfor all uploads (lines 67, 126) to prevent 415 errors. Note that this differs fromJournalsService.upload(), which sends the file's actual mimetype. If users upload CSV or other formats, they will be transmitted with an incorrect XLSX mimetype header. Verify whether the API validates file content or relies on the mimetype header, and confirm this hardcoding is the intended design.Extract shared upload logic from sync/async methods.
The sync and async
upload()methods (lines 49–82 and 108–141) are nearly identical except for theawaitkeyword. Consider extracting the file preparation into a shared helper:Suggested refactor
@staticmethod def _prepare_upload_files( custom_ledger_id: str, file: FileTypes, file_key: str, id_key: str, ) -> dict[str, FileTypes]: """Prepare files dict for multipart upload.""" files: dict[str, FileTypes] = {} filename = pathlib.Path(getattr(file, "name", "uploaded_file.xlsx")).name files[file_key] = ( filename, cast("FileContent", file), MIMETYPE_EXCEL_XLSX, ) files[id_key] = custom_ledger_id return filesBoth
upload()methods can then call this helper, eliminating duplication.
📜 Review details
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/data/test_custom_ledger.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (16)
e2e_config.test.jsonmpt_api_client/constants.pympt_api_client/resources/billing/custom_ledger_upload.pympt_api_client/resources/billing/custom_ledgers.pypyproject.tomltests/e2e/billing/custom_ledger/attachment/conftest.pytests/e2e/billing/custom_ledger/attachment/test_async_custom_ledger_attachment.pytests/e2e/billing/custom_ledger/attachment/test_sync_custom_ledger_attachment.pytests/e2e/billing/custom_ledger/charge/conftest.pytests/e2e/billing/custom_ledger/charge/test_async_custom_ledger_charges.pytests/e2e/billing/custom_ledger/charge/test_sync_custom_ledger_charges.pytests/e2e/billing/custom_ledger/conftest.pytests/e2e/billing/custom_ledger/test_async_custom_ledger.pytests/e2e/billing/custom_ledger/test_sync_custom_ledger.pytests/unit/resources/billing/test_custom_ledger_upload.pytests/unit/resources/billing/test_custom_ledgers.py
💤 Files with no reviewable changes (2)
- mpt_api_client/resources/billing/custom_ledger_upload.py
- tests/unit/resources/billing/test_custom_ledger_upload.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not suggest formatting changes or mention code formatting using Black. Instead use Ruff or Flake8
Files:
mpt_api_client/constants.pytests/e2e/billing/custom_ledger/test_sync_custom_ledger.pytests/e2e/billing/custom_ledger/attachment/test_async_custom_ledger_attachment.pytests/e2e/billing/custom_ledger/conftest.pytests/e2e/billing/custom_ledger/attachment/conftest.pytests/e2e/billing/custom_ledger/charge/test_async_custom_ledger_charges.pytests/e2e/billing/custom_ledger/attachment/test_sync_custom_ledger_attachment.pytests/e2e/billing/custom_ledger/charge/test_sync_custom_ledger_charges.pytests/e2e/billing/custom_ledger/test_async_custom_ledger.pytests/e2e/billing/custom_ledger/charge/conftest.pytests/unit/resources/billing/test_custom_ledgers.pympt_api_client/resources/billing/custom_ledgers.py
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing subsequent commits, always check if previous review comments have been addressed and acknowledge resolved issues.
Files:
mpt_api_client/constants.pypyproject.tomle2e_config.test.jsontests/e2e/billing/custom_ledger/test_sync_custom_ledger.pytests/e2e/billing/custom_ledger/attachment/test_async_custom_ledger_attachment.pytests/e2e/billing/custom_ledger/conftest.pytests/e2e/billing/custom_ledger/attachment/conftest.pytests/e2e/billing/custom_ledger/charge/test_async_custom_ledger_charges.pytests/e2e/billing/custom_ledger/attachment/test_sync_custom_ledger_attachment.pytests/e2e/billing/custom_ledger/charge/test_sync_custom_ledger_charges.pytests/e2e/billing/custom_ledger/test_async_custom_ledger.pytests/e2e/billing/custom_ledger/charge/conftest.pytests/unit/resources/billing/test_custom_ledgers.pympt_api_client/resources/billing/custom_ledgers.py
🧠 Learnings (4)
📚 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: 2026-01-08T08:34:05.465Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 183
File: tests/e2e/catalog/price_lists/conftest.py:29-30
Timestamp: 2026-01-08T08:34:05.465Z
Learning: In end-to-end tests (e.g., tests/e2e/...), reuse existing API resources for read-only operations and safe mutations to speed up test execution. Reserve isolated fixtures (e.g., created_price_list) for destructive tests that require per-test creation and cleanup. Ensure tests document when a fixture creates/destroys data and clearly indicate which operations are destructive, so tests stay fast and properly isolated.
Applied to files:
tests/e2e/billing/custom_ledger/test_sync_custom_ledger.pytests/e2e/billing/custom_ledger/attachment/test_async_custom_ledger_attachment.pytests/e2e/billing/custom_ledger/conftest.pytests/e2e/billing/custom_ledger/attachment/conftest.pytests/e2e/billing/custom_ledger/charge/test_async_custom_ledger_charges.pytests/e2e/billing/custom_ledger/attachment/test_sync_custom_ledger_attachment.pytests/e2e/billing/custom_ledger/charge/test_sync_custom_ledger_charges.pytests/e2e/billing/custom_ledger/test_async_custom_ledger.pytests/e2e/billing/custom_ledger/charge/conftest.py
📚 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/custom_ledger/test_sync_custom_ledger.pytests/e2e/billing/custom_ledger/attachment/test_async_custom_ledger_attachment.pytests/e2e/billing/custom_ledger/conftest.pytests/e2e/billing/custom_ledger/attachment/conftest.pytests/e2e/billing/custom_ledger/charge/test_async_custom_ledger_charges.pytests/e2e/billing/custom_ledger/attachment/test_sync_custom_ledger_attachment.pytests/e2e/billing/custom_ledger/charge/test_sync_custom_ledger_charges.pytests/e2e/billing/custom_ledger/test_async_custom_ledger.pytests/e2e/billing/custom_ledger/charge/conftest.pytests/unit/resources/billing/test_custom_ledgers.py
📚 Learning: 2025-11-27T00:05:54.701Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 131
File: tests/e2e/accounts/accounts_users/test_async_accounts_users.py:0-0
Timestamp: 2025-11-27T00:05:54.701Z
Learning: In pytest-asyncio, async fixtures are automatically resolved before being passed to test functions (both sync and async). Test functions receive the yielded value from async fixtures, not coroutines, so fixture parameters should never be awaited. A sync test function can use async fixtures without any special handling.
Applied to files:
tests/unit/resources/billing/test_custom_ledgers.py
🧬 Code graph analysis (9)
tests/e2e/billing/custom_ledger/test_sync_custom_ledger.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)
mpt_ops(31-32)tests/e2e/billing/custom_ledger/conftest.py (3)
custom_ledger_factory(19-43)custom_ledger_id(8-9)billing_custom_ledger_fd(47-53)mpt_api_client/models/model.py (1)
id(119-121)mpt_api_client/resources/billing/custom_ledgers.py (2)
upload(49-82)upload(108-141)
tests/e2e/billing/custom_ledger/attachment/test_async_custom_ledger_attachment.py (5)
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)
async_mpt_ops(36-39)pdf_fd(73-75)tests/e2e/billing/custom_ledger/attachment/conftest.py (3)
custom_ledger_attachment_factory(15-24)custom_ledger_attachment_id(5-6)invalid_custom_ledger_attachment_id(10-11)mpt_api_client/models/file_model.py (1)
file_contents(35-45)
tests/e2e/billing/custom_ledger/conftest.py (2)
tests/e2e/conftest.py (3)
e2e_config(89-92)seller_id(127-128)account_id(122-123)tests/e2e/billing/custom_ledger/attachment/conftest.py (1)
factory(16-22)
tests/e2e/billing/custom_ledger/charge/test_async_custom_ledger_charges.py (5)
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)
async_mpt_ops(36-39)mpt_api_client/resources/billing/custom_ledgers.py (2)
charges(84-89)charges(143-148)tests/e2e/billing/custom_ledger/charge/conftest.py (2)
custom_ledger_charge_id(5-6)invalid_custom_ledger_charge_id(10-11)
tests/e2e/billing/custom_ledger/charge/test_sync_custom_ledger_charges.py (3)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/resources/billing/custom_ledgers.py (2)
charges(84-89)charges(143-148)tests/e2e/billing/custom_ledger/charge/conftest.py (2)
custom_ledger_charge_id(5-6)invalid_custom_ledger_charge_id(10-11)
tests/e2e/billing/custom_ledger/test_async_custom_ledger.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)
async_mpt_ops(36-39)tests/e2e/billing/custom_ledger/conftest.py (3)
custom_ledger_factory(19-43)custom_ledger_id(8-9)billing_custom_ledger_fd(47-53)mpt_api_client/models/model.py (1)
id(119-121)mpt_api_client/resources/billing/custom_ledgers.py (2)
upload(49-82)upload(108-141)
tests/e2e/billing/custom_ledger/charge/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
tests/unit/resources/billing/test_custom_ledgers.py (1)
mpt_api_client/resources/billing/custom_ledgers.py (2)
upload(49-82)upload(108-141)
mpt_api_client/resources/billing/custom_ledgers.py (3)
mpt_api_client/resources/billing/journals.py (2)
upload(50-77)upload(108-135)mpt_api_client/models/file_model.py (1)
filename(13-32)mpt_api_client/http/base_service.py (1)
path(28-30)
🪛 Ruff (0.14.10)
tests/e2e/billing/custom_ledger/attachment/test_async_custom_ledger_attachment.py
27-27: Unused noqa directive (unknown: WPS421)
Remove unused noqa directive
(RUF100)
tests/e2e/billing/custom_ledger/attachment/test_sync_custom_ledger_attachment.py
27-27: 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 (32)
pyproject.toml (2)
122-122: LGTM!Adding WPS110 (wrong variable names) to the billing module ignore list is appropriate, especially given the domain-specific naming conventions in billing/ledger code.
129-129: LGTM!The ignore rules for e2e billing tests are appropriate:
- WPS202 (too many module members) is common in comprehensive e2e test suites
- WPS421 (print statements) helps with debugging e2e tests
- WPS118 (long line comparisons) is typical in test assertions
e2e_config.test.json (1)
15-17: LGTM!The new billing configuration entries follow the established naming pattern and provide the necessary test identifiers for the custom ledger e2e tests.
mpt_api_client/constants.py (1)
2-2: LGTM!The XLSX MIME type constant is correctly defined and follows the established pattern in this file. The value matches the standard MIME type for Excel XLSX files.
tests/e2e/billing/custom_ledger/charge/test_async_custom_ledger_charges.py (5)
1-6: LGTM!The imports are appropriate and the module-level flaky marker is a good practice for e2e tests that may experience intermittent failures due to network or service availability issues.
9-11: LGTM!The fixture correctly provides the custom ledger charges service by accessing the charges accessor method with the appropriate custom_ledger_id.
14-24: LGTM!Both test cases correctly cover the success and error scenarios:
- The success case validates retrieval of an existing charge
- The error case properly asserts the 404 exception for an invalid ID
The async/await usage is correct, and the error matching pattern is appropriate.
27-32: LGTM!The pagination test correctly validates that the charges service can fetch a page of results with the specified limit.
35-44: LGTM!The filtering test demonstrates proper use of RQLQuery with multiple filters and field selection. The async iteration pattern is correct, and the test validates that the combined filters produce the expected single result.
tests/e2e/billing/custom_ledger/charge/conftest.py (1)
4-11: LGTM!The fixtures are well-structured and follow the established pattern:
custom_ledger_charge_idcorrectly reads from the e2e configurationinvalid_custom_ledger_charge_idprovides a predictable invalid ID for negative test casestests/e2e/billing/custom_ledger/attachment/test_async_custom_ledger_attachment.py (2)
79-93: Consider asserting the updated name value.The update test asserts
updated_attachment.name == updated_name, which is good. The test is well-structured.
1-110: LGTM overall.The async e2e tests follow established patterns correctly. The sync
test_create_billing_custom_ledger_attachmentfunction properly leverages pytest-asyncio's automatic async fixture resolution. Based on learnings, this is the correct approach.tests/e2e/billing/custom_ledger/test_sync_custom_ledger.py (2)
9-20: Fixture teardown is well-structured.The fixture properly handles cleanup with error catching. Consistent with async counterpart.
81-87: Upload test covers the new functionality.The test properly exercises the new
uploadmethod integrated intoCustomLedgersService, aligning with the PR's changes that moved upload functionality from a separate resource to the main service.tests/unit/resources/billing/test_custom_ledgers.py (3)
25-26: Fixture parameter renamed correctly.The rename from
http_clienttoasync_http_clientaligns with async service requirements.
29-31: Upload method correctly added to mixin checks.Both sync and async services now verify the
uploadmethod is present, reflecting the new upload capability.Also applies to: 38-40
77-105: Upload tests properly mock the HTTP layer.The tests validate upload routing and non-null response. The mock URL
https://api.example.com/public/v1/billing/custom-ledgers/LDG-0000-0001/uploadcorrectly matches the expected endpoint pattern:http_clientfixture configures base URL ashttps://api.example.com, and theCustomLedgersServiceendpoint is/public/v1/billing/custom-ledgers, with the upload path constructed as{custom_ledger_id}/upload.tests/e2e/billing/custom_ledger/attachment/test_sync_custom_ledger_attachment.py (1)
1-105: Tests follow established patterns.The sync e2e tests mirror the async counterpart appropriately.
tests/e2e/billing/custom_ledger/test_async_custom_ledger.py (2)
57-60: Correct pattern for sync test with async fixture.The sync
def test_create_custom_ledgercorrectly leverages pytest-asyncio's automatic async fixture resolution. Based on learnings, this is the proper approach when the test body has no await.
83-89: Upload test covers async upload functionality.The test exercises the async
uploadmethod on the custom ledgers service, validating the new upload integration.tests/e2e/billing/custom_ledger/attachment/conftest.py (1)
1-24: LGTM.The fixtures are well-structured and follow established conventions in the codebase. The factory pattern is consistent with other e2e fixtures like
custom_ledger_factory.tests/e2e/billing/custom_ledger/conftest.py (3)
17-18: Verify@freeze_timeon fixture is intentional.Using
@freeze_timeon a fixture is uncommon. Since the factory returns hardcoded date strings ("2025-12-01T07:00:00.000Z"), the decorator may be unnecessary. If time freezing is needed for API calls, consider applying it to individual tests instead.
46-53: File descriptor management is correct.The
try/finallypattern ensures the file is properly closed after the test completes.
7-14: ID fixtures follow established patterns.The fixtures correctly retrieve IDs from e2e config and provide an invalid ID for 404 testing.
tests/e2e/billing/custom_ledger/charge/test_sync_custom_ledger_charges.py (5)
1-11: LGTM! Clean test setup.The imports, flaky marker, and fixture configuration are appropriate for E2E testing. The
custom_ledger_chargesfixture correctly creates a charges service scoped to each test function, which aligns with the pattern for read-only E2E operations.
14-17: LGTM!The test correctly verifies basic retrieval functionality. The assertion is sufficient for E2E validation.
20-24: LGTM!The test properly validates 404 error handling with appropriate exception matching.
35-45: LGTM! Filtering logic is well-tested.The test effectively validates RQLQuery filtering, field selection, and iteration. The specific filter values (e.g.,
description__value2="Description 2") indicate reliance on controlled test data, which is appropriate for E2E testing.
27-32: No changes needed.The test correctly follows standard E2E testing practices. The reliance on pre-configured test data via the
e2e_configfixture (specificallybilling.custom_ledger.idand data within that ledger) is expected behavior for end-to-end tests, not a defect. E2E tests inherently depend on a properly configured test environment rather than creating data dynamically. All related tests in this file (test_get_custom_ledger_charge_by_id,test_filter_custom_ledger_charges) follow the same pattern with fixtures sourcing frome2e_config.mpt_api_client/resources/billing/custom_ledgers.py (3)
1-5: LGTM! Imports are appropriate.The new imports support file upload functionality with proper typing and URL/path handling.
Also applies to: 13-13
36-37: LGTM!Configuration keys align with the existing pattern in
journals.pyand are appropriate for multipart upload.
108-141: Async upload implementation is correct.The async version correctly mirrors the sync implementation with appropriate async/await patterns. The same verification concern regarding hardcoded XLSX mimetype applies here as well (see comment on lines 49-82).
|
Added e2e tests for custom ledgers, its charges, and its attachments endpoints
Closes MPT-14927
Changes:
CustomLedgerUploadservice class and consolidating the upload method directly into theCustomLedgersService, simplifying the API surfaceCustomLedgersService.upload()to accept a file parameter and perform direct file uploads with multipart form data to the{path}/{custom_ledger_id}/uploadendpointMIMETYPE_EXCEL_XLSXconstant for Excel file handlingCustomLedgerUploadservice class and the newupload()method signature