[MPT-14905] Fixed agreement attachments endpoints, created e2e tests #148
Conversation
robcsegal
requested review from
albertsola,
alephsur,
d3rky,
jentyk,
ruben-sebrango and
svazquezco
as code owners
WalkthroughReplaced consolidated file-operation mixins with explicit per-operation mixins for agreement attachments, added two commerce-related e2e configuration keys, and introduced new e2e tests and fixtures plus unit-test updates validating the attachment service API surface. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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: 1
🧹 Nitpick comments (4)
tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py (1)
79-86: Remove duplicate filename assertion in async download test
test_download_agreement_attachmentassertsresult.filename == "empty.pdf"twice; the second assert is redundant and can be dropped to keep the test concise.- assert result.file_contents is not None - assert result.filename == "empty.pdf" - assert result.filename == "empty.pdf" + assert result.file_contents is not None + assert result.filename == "empty.pdf"tests/e2e/commerce/agreement/attachment/test_sync_agreement_attachment.py (1)
72-79: Makepdf_fda shared, closing fixtureThe
pdf_fdfixture (openingempty.pdfin"rb") would be safer and more reusable if implemented as ayieldfixture that closes the file, and ideally moved intoconftest.pyso the async tests can depend on the same fixture.For example:
- def pdf_fd(): - icon_path = pathlib.Path(__file__).parent / "empty.pdf" - return pathlib.Path.open(icon_path, "rb") + @pytest.fixture + def pdf_fd(): + icon_path = pathlib.Path(__file__).parent / "empty.pdf" + with icon_path.open("rb") as fd: + yield fdThen relocate this fixture into
tests/e2e/commerce/agreement/attachment/conftest.pyso both sync and async suites can use it.mpt_api_client/resources/commerce/agreements_attachments.py (2)
2-15: Remove or adjust unused# noqa: WPS235directiveStatic analysis (Ruff) reports
RUF100for# noqa: WPS235on the mixin import line, meaning that code isn’t recognized by the configured checkers. Unless you rely on a separate tool that actually needs this, it’s better to drop it to avoid confusion.-from mpt_api_client.http.mixins import ( # noqa: WPS235 +from mpt_api_client.http.mixins import (If you do still need to silence a specific rule, consider configuring it in your lint setup rather than keeping an apparently unused
noqa.
23-25: Docstring refers to “Orders” instead of agreements attachments
AgreementsAttachmentServiceConfigcurrently has the docstring"""Orders service config.""", which is misleading for an agreements attachments service.-class AgreementsAttachmentServiceConfig: - """Orders service config.""" +class AgreementsAttachmentServiceConfig: + """Agreements attachments service config."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e_config.test.json(1 hunks)mpt_api_client/resources/commerce/agreements_attachments.py(3 hunks)tests/e2e/commerce/agreement/attachment/conftest.py(1 hunks)tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py(1 hunks)tests/e2e/commerce/agreement/attachment/test_sync_agreement_attachment.py(1 hunks)tests/unit/resources/commerce/test_agreements_attachments.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/e2e/commerce/agreement/attachment/test_sync_agreement_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/commerce/agreement/attachment/conftest.py (3)
agreement_attachment_factory(15-28)agreement_attachment_id(10-11)invalid_agreement_attachment_id(5-6)tests/e2e/commerce/conftest.py (1)
agreement_id(5-6)mpt_api_client/models/file_model.py (2)
file_contents(35-45)filename(13-32)
tests/e2e/commerce/agreement/attachment/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
🪛 Ruff (0.14.7)
mpt_api_client/resources/commerce/agreements_attachments.py
2-2: Unused noqa directive (unknown: WPS235)
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 (4)
tests/unit/resources/commerce/test_agreements_attachments.py (1)
35-46: Unit coverage forupdatemethod looks correctIncluding
"update"in the parametrized method list for both sync and async services matches the newUpdateMixin/AsyncUpdateMixinbases and keeps the public API smoke‑tested.tests/e2e/commerce/agreement/attachment/conftest.py (1)
4-28: Shared attachment fixtures are well structuredThe invalid ID, config‑backed ID, and
agreement_attachment_factorygive the E2E tests a clean, reusable way to construct attachment payloads and IDs; this keeps individual tests focused on behavior rather than setup.mpt_api_client/resources/commerce/agreements_attachments.py (1)
29-37: Per‑operation mixins and upload keys look consistent with testsConfiguring
_upload_file_key = "file"and_upload_data_key = "attachment"plus inheritingCreateFileMixin/AsyncCreateFileMixin,UpdateMixin/AsyncUpdateMixin, andDownloadFileMixin/AsyncDownloadFileMixinlines up with how the tests callcreate(..., file=pdf_fd)andupdate(...), and should give you a clear, explicit attachment API surface.tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py (1)
9-20: No action required. Thepdf_fdfixture is already defined intests/e2e/conftest.pyand is properly accessible to this async module through pytest's standard fixture scope inheritance. No fixture relocation is necessary.Likely an incorrect or invalid review comment.
robcsegal
force-pushed
the
MPT-14905-add-e-2-e-tests-for-commerce-agreements-attachments
branch
from
eb10dde to
a514d5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py (1)
74-76: Consider adding an assertion to verify deletion.The test deletes the attachment but doesn't verify that the deletion was successful. Consider adding an assertion that a subsequent
get()call raises a 404 error, similar totest_get_agreement_attachment_by_id_not_found.async def test_delete_agreement_attachment(agreement_attachments, created_agreement_attachment): result = created_agreement_attachment await agreement_attachments.delete(result.id) + + with pytest.raises(MPTAPIError, match=r"404 Not Found"): + await agreement_attachments.get(result.id)tests/e2e/commerce/agreement/attachment/test_sync_agreement_attachment.py (2)
9-15: Minor inconsistency with the async counterpart fixture.This fixture creates
agreement_attachmentsinline, while the async version (line 10 intest_async_agreement_attachment.py) uses theagreement_attachmentsfixture directly. Consider using theagreement_attachmentsfixture here as well for consistency between sync and async test modules.@pytest.fixture -def created_agreement_attachment(mpt_ops, agreement_attachment_factory, agreement_id, pdf_fd): +def created_agreement_attachment(agreement_attachments, agreement_attachment_factory, pdf_fd): new_agreement_attachment_request_data = agreement_attachment_factory( name="E2E Created Agreement Attachment", ) - agreement_attachments = mpt_ops.commerce.agreements.attachments(agreement_id) return agreement_attachments.create(new_agreement_attachment_request_data, file=pdf_fd)
75-78: Consider adding an assertion to verify deletion.Similar to the async test, this test deletes the attachment but doesn't verify that the deletion succeeded. Consider asserting that a subsequent
get()call raises a 404 error.def test_delete_agreement_attachment(agreement_attachments, created_agreement_attachment): result = created_agreement_attachment agreement_attachments.delete(result.id) + + with pytest.raises(MPTAPIError, match=r"404 Not Found"): + agreement_attachments.get(result.id)mpt_api_client/resources/commerce/agreements_attachments.py (2)
2-15: Re‑alignnoqadirective with current linting (RUF100 warning).Ruff reports RUF100 for
# noqa: WPS235, which suggests this code is no longer a recognized rule in the active lint config. If you’ve fully migrated to Ruff and are no longer running wemake/Flake8 WPS rules, you can safely drop thisnoqato keep the imports clean:-from mpt_api_client.http.mixins import ( # noqa: WPS235 +from mpt_api_client.http.mixins import (If you still rely on WPS235 from another tool, consider disabling RUF100 or scoping the
noqadifferently so the tools don’t fight each other.
23-30: Config upload keys look good; consider fixing misleading docstring.The new
_upload_file_key = "file"and_upload_data_key = "attachment"align well with a typical file + metadata upload shape and should integrate cleanly with the newCreateFileMixin/AsyncCreateFileMixinlogic.The class docstring, however, still states
"Orders service config.", which is confusing in an agreements attachments context. Consider updating for clarity:-class AgreementsAttachmentServiceConfig: - """Orders service config.""" +class AgreementsAttachmentServiceConfig: + """Agreements attachment service config."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e_config.test.json(1 hunks)mpt_api_client/resources/commerce/agreements_attachments.py(3 hunks)tests/e2e/commerce/agreement/attachment/conftest.py(1 hunks)tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py(1 hunks)tests/e2e/commerce/agreement/attachment/test_sync_agreement_attachment.py(1 hunks)tests/unit/resources/commerce/test_agreements_attachments.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e_config.test.json
- tests/e2e/commerce/agreement/attachment/conftest.py
- tests/unit/resources/commerce/test_agreements_attachments.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py (7)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/rql/query_builder.py (1)
RQLQuery(115-524)tests/e2e/commerce/agreement/attachment/conftest.py (1)
agreement_attachment_factory(15-28)tests/e2e/conftest.py (1)
pdf_fd(73-75)tests/e2e/commerce/conftest.py (1)
agreement_id(5-6)mpt_api_client/models/model.py (1)
id(119-121)mpt_api_client/models/file_model.py (2)
file_contents(35-45)filename(13-32)
🪛 Ruff (0.14.7)
mpt_api_client/resources/commerce/agreements_attachments.py
2-2: Unused noqa directive (unknown: WPS235)
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 (5)
tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py (2)
1-6: Well-structured e2e test coverage for async attachment operations.The test module provides comprehensive coverage for the agreement attachment lifecycle including get, list, filter, create, update, and download operations. The use of
pytest.mark.flakyis appropriate for e2e tests that may have transient failures.Also applies to: 9-52, 61-85
55-58: Missingasynckeyword on test function.
test_create_agreement_attachmentis a synchronous function but usescreated_agreement_attachment, which is anasyncfixture defined on lines 9-14. This test should be markedasyncfor consistency with the module and to ensure pytest-asyncio handles the fixture correctly.-def test_create_agreement_attachment(created_agreement_attachment): +async def test_create_agreement_attachment(created_agreement_attachment):⛔ Skipped due to learnings
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.tests/e2e/commerce/agreement/attachment/test_sync_agreement_attachment.py (1)
1-6: Good parity with async test module.The sync test module mirrors the async counterpart well, providing equivalent coverage for all attachment operations. The test structure is clean and follows pytest conventions.
Also applies to: 18-72, 81-85
mpt_api_client/resources/commerce/agreements_attachments.py (2)
33-42: Sync service mixin composition matches new per‑operation design.The switch to explicit
CreateFileMixin,UpdateMixin, andDownloadFileMixinonAgreementsAttachmentServicealongsideDeleteMixin,GetMixin, andCollectionMixinis coherent and makes the capabilities of the service explicit. The generic parameterization withAgreementAttachmentand inclusion ofAgreementsAttachmentServiceConfigas a base are consistent with the rest of the client’s pattern.
46-55: Async service mirrors sync surface correctly.
AsyncAgreementsAttachmentServicemirrors the sync service withAsyncCreateFileMixin,AsyncUpdateMixin, andAsyncDownloadFileMixinplus the async collection/delete/get/service mixins and shared config. This keeps the async API aligned with the sync one and avoids surprises for consumers.
robcsegal
force-pushed
the
MPT-14905-add-e-2-e-tests-for-commerce-agreements-attachments
branch
2 times, most recently
from
b3acd0e to
9518377
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
e2e_config.test.json (1)
16-16: Re-evaluatecommerce.order.iduntil there are E2E consumers
commerce.order.idis defined here but, as previously noted, there still appear to be no E2E fixtures/tests using it. Either add corresponding E2E coverage that reads this key, or remove it from the test config for now to keep it minimal and focused.
🧹 Nitpick comments (3)
mpt_api_client/resources/commerce/agreements_attachments.py (1)
23-30: Clarify service config docstring and acknowledge new upload keysThe new
_upload_file_key/_upload_data_keyfields make the upload contract explicit and align with the file mixins; the only nit is the outdated docstring mentioning “Orders”.-class AgreementsAttachmentServiceConfig: - """Orders service config.""" +class AgreementsAttachmentServiceConfig: + """Agreement attachments service config."""tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py (1)
61-77: Strengthen update/delete assertions for better E2E guarantees
test_update_agreement_attachmentonly asserts that the response is notNone, andtest_delete_agreement_attachmentonly checks that no exception is raised. To catch more regressions, you could assert the actual behavior:
- After update, assert that the returned object reflects the new values.
- After delete, assert that a subsequent
getyields a 404.For example:
async def test_update_agreement_attachment(agreement_attachments, created_agreement_attachment): @@ - result = await agreement_attachments.update(attachment.id, updated_attachment_data) - - assert result is not None + result = await agreement_attachments.update(attachment.id, updated_attachment_data) + + assert result is not None + assert result.name == updated_name + assert result.description == updated_name @@ async def test_delete_agreement_attachment(agreement_attachments, created_agreement_attachment): - result = created_agreement_attachment - await agreement_attachments.delete(result.id) + result = created_agreement_attachment + await agreement_attachments.delete(result.id) + + with pytest.raises(MPTAPIError, match=r"404 Not Found"): + await agreement_attachments.get(result.id)tests/e2e/commerce/agreement/attachment/test_sync_agreement_attachment.py (1)
63-79: Improve update/delete assertions to validate behavior, not just successAs with the async tests, the sync
test_update_agreement_attachmentonly asserts non-None, andtest_delete_agreement_attachmentonly checks that no exception is raised. You can make these tests more meaningful by asserting the observable effects:def test_update_agreement_attachment(agreement_attachments, created_agreement_attachment): @@ - result = agreement_attachments.update(created_agreement_attachment.id, updated_attachment_data) - - assert result is not None + result = agreement_attachments.update(created_agreement_attachment.id, updated_attachment_data) + + assert result is not None + assert result.name == updated_name + assert result.description == updated_name @@ def test_delete_agreement_attachment(agreement_attachments, created_agreement_attachment): result = created_agreement_attachment - agreement_attachments.delete(result.id) + agreement_attachments.delete(result.id) + + with pytest.raises(MPTAPIError, match=r"404 Not Found"): + agreement_attachments.get(result.id)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e_config.test.json(1 hunks)mpt_api_client/resources/commerce/agreements_attachments.py(3 hunks)tests/e2e/commerce/agreement/attachment/conftest.py(1 hunks)tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py(1 hunks)tests/e2e/commerce/agreement/attachment/test_sync_agreement_attachment.py(1 hunks)tests/unit/resources/commerce/test_agreements_attachments.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/commerce/agreement/attachment/conftest.py
- tests/unit/resources/commerce/test_agreements_attachments.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py (3)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/commerce/agreement/attachment/conftest.py (3)
agreement_attachment_factory(15-28)agreement_attachment_id(10-11)invalid_agreement_attachment_id(5-6)tests/e2e/conftest.py (1)
async_mpt_ops(36-39)
🪛 Ruff (0.14.7)
mpt_api_client/resources/commerce/agreements_attachments.py
2-2: Unused noqa directive (unknown: WPS235)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (5)
mpt_api_client/resources/commerce/agreements_attachments.py (2)
33-55: Per-operation mixin composition for attachments looks correctUsing
CreateFileMixin/DownloadFileMixinplusUpdateMixinand the standard CRUD/collection mixins (and their async counterparts) gives a clear, explicit API surface and matches how other resources are structured. No issues spotted with the base-class ordering or generics.
2-15: Remove unusednoqaon mixins importRuff reports
RUF100because# noqa: WPS235does not correspond to any active warning here; it should be dropped (or updated to the actually suppressed rule, if still needed) to keep lint config clean.-from mpt_api_client.http.mixins import ( # noqa: WPS235 +from mpt_api_client.http.mixins import (⛔ Skipped due to learnings
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.e2e_config.test.json (1)
14-15: Attachment config key matches new E2E fixtures
commerce.agreement.attachment.idhooks up cleanly with theagreement_attachment_idfixture and the new attachment E2E tests; this addition looks good.tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py (1)
1-85: Solid async E2E coverage for agreement attachmentsThe async tests nicely cover the main lifecycle and error paths (CRUD, list, filter, download, and 404 handling via
MPTAPIError), and the dedicatedcreated_agreement_attachmentfixture keeps seeded and test-created data separate. No functional issues spotted.tests/e2e/commerce/agreement/attachment/test_sync_agreement_attachment.py (1)
1-85: Synchronous E2E attachment tests mirror async suite effectivelyThis sync suite cleanly mirrors the async tests and exercises the full attachment lifecycle with the same fixtures and RQL-based filtering. Structure and use of fixtures look sound.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
e2e_config.test.json (1)
16-16:commerce.order.idstill unused in E2E tests
"commerce.order.id"remains defined here but there are still no corresponding E2E fixtures or tests consuming it in this PR. Either remove it from the test config for now or add the missing E2E coverage that uses this key.
🧹 Nitpick comments (3)
mpt_api_client/resources/commerce/agreements_attachments.py (2)
2-15: Remove unused/unknownnoqaon mixins importRuff reports
RUF100on# noqa: WPS235; since there’s no such rule configured, the directive is unused and should be dropped to keep lint clean.-from mpt_api_client.http.mixins import ( # noqa: WPS235 +from mpt_api_client.http.mixins import (
23-36: ClarifyAgreementsAttachmentServiceConfigdocstringThe class is specific to agreement attachments, but the docstring still says
"Orders service config.", which is confusing in this module. Consider renaming to something like"Agreement attachments service config."for clarity.tests/e2e/commerce/agreement/attachment/test_sync_agreement_attachment.py (1)
63-72: Optionally assert updated fields in E2E update testThe update test only checks that
update(...)returns a non-None result. If the model exposesname/descriptionattributes, you could assert they matchupdated_nameto better validate the update behavior end-to-end.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e_config.test.json(1 hunks)mpt_api_client/resources/commerce/agreements_attachments.py(3 hunks)tests/e2e/commerce/agreement/attachment/conftest.py(1 hunks)tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py(1 hunks)tests/e2e/commerce/agreement/attachment/test_sync_agreement_attachment.py(1 hunks)tests/unit/resources/commerce/test_agreements_attachments.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/commerce/agreement/attachment/conftest.py
🧬 Code graph analysis (4)
tests/e2e/commerce/agreement/attachment/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
tests/unit/resources/commerce/test_agreements_attachments.py (16)
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/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_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/unit/resources/billing/test_invoice_attachments.py (1)
test_methods_present(41-44)tests/unit/resources/billing/test_journal_attachments.py (1)
test_methods_present(41-44)tests/unit/resources/billing/test_invoices.py (1)
test_methods_present(27-30)tests/unit/resources/billing/test_journal_charges.py (1)
test_methods_present(38-41)tests/unit/resources/billing/test_journal_sellers.py (1)
test_methods_present(38-41)tests/unit/resources/billing/test_journal_upload.py (1)
test_methods_present(36-39)tests/unit/resources/billing/test_ledger_charges.py (1)
test_methods_present(36-39)tests/unit/resources/billing/test_ledger_attachments.py (1)
test_methods_present(41-44)tests/unit/resources/billing/test_statement_charges.py (1)
test_methods_present(40-43)
tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py (3)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/commerce/agreement/attachment/conftest.py (3)
agreement_attachment_factory(15-28)agreement_attachment_id(10-11)invalid_agreement_attachment_id(5-6)mpt_api_client/models/file_model.py (2)
file_contents(35-45)filename(13-32)
mpt_api_client/resources/commerce/agreements_attachments.py (1)
mpt_api_client/http/mixins.py (10)
AsyncCollectionMixin(538-605)AsyncCreateFileMixin(187-216)AsyncDeleteMixin(273-283)AsyncDownloadFileMixin(303-325)CollectionMixin(469-535)CreateFileMixin(115-144)DeleteMixin(29-38)DownloadFileMixin(58-80)GetMixin(361-377)UpdateMixin(41-55)
🪛 Biome (2.1.2)
e2e_config.test.json
[error] 15-15: The key commerce.agreement.id was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 17-17: The key commerce.product.id was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 18-18: The key commerce.product.item.id was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 19-19: The key commerce.product.listing.id was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 20-20: The key commerce.product.template.id was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 21-21: The key commerce.authorization.id was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 22-22: The key commerce.client.id was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
[error] 25-25: The key catalog.product.item.id was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
🪛 Ruff (0.14.7)
mpt_api_client/resources/commerce/agreements_attachments.py
2-2: Unused noqa directive (unknown: WPS235)
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 (3)
tests/unit/resources/commerce/test_agreements_attachments.py (1)
35-46: Method surface checks now cover full APIIncluding
"update"and"iterate"in both sync and async parametrized lists correctly matches the new mixins (UpdateMixin and (Async)CollectionMixin). This keeps unit coverage aligned with the attachment services’ public surface.tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py (1)
9-15: Verify async fixture support for@pytest.fixture
created_agreement_attachmentis declared asasync defunder@pytest.fixture. This relies on your pytest/pytest-asyncio setup to await async fixtures via the standard decorator; in some configurations you must usepytest_asyncio.fixtureinstead.tests/e2e/commerce/agreement/attachment/conftest.py (1)
4-28: Fixtures cleanly model agreement attachment test dataThe invalid ID, config-driven ID, and small factory for attachment payloads give the E2E tests a clear, reusable setup surface and correctly consume
commerce.agreement.attachment.idfrom the config.
tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py
Show resolved
Hide resolved
tests/e2e/commerce/agreement/attachment/test_sync_agreement_attachment.py
Show resolved
Hide resolved
robcsegal
force-pushed
the
MPT-14905-add-e-2-e-tests-for-commerce-agreements-attachments
branch
from
9518377 to
5e40b58
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py (1)
79-85: Invokefile_contents()to actually validate the download payload
file_contentsis a method;result.file_contents is not Noneonly checks that the attribute exists, not that bytes were retrieved. Call the method and assert on the returned content instead.async def test_download_agreement_attachment(agreement_attachments, created_agreement_attachment): attachment = created_agreement_attachment result = await agreement_attachments.download(attachment.id) - - assert result.file_contents is not None - assert result.filename == "empty.pdf" + content = result.file_contents() + assert isinstance(content, bytes) + assert content # non-empty payload + assert result.filename == "empty.pdf"
🧹 Nitpick comments (3)
tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py (2)
61-71: Strengthen the update test by asserting the updated fieldsRight now the update e2e test only checks
result is not None. Asserting that the name was actually changed would better validate the endpoint behaviour.async def test_update_agreement_attachment(agreement_attachments, created_agreement_attachment): updated_name = "E2E Updated Agreement Attachment Name" @@ - result = await agreement_attachments.update(attachment.id, updated_attachment_data) - - assert result is not None + result = await agreement_attachments.update(attachment.id, updated_attachment_data) + + assert result is not None + assert result.name == updated_name + assert result.description == updated_name
9-15: Check async fixture usage with the synctest_create_agreement_attachment
created_agreement_attachmentis defined as anasync deffixture buttest_create_agreement_attachmentis synchronous. Depending on your pytest-asyncio configuration, this can result in the test receiving an unawaited coroutine instead of a created attachment.If your other async e2e tests use
async deftests with async fixtures, consider making this test async as well for consistency:-async def created_agreement_attachment(agreement_attachments, agreement_attachment_factory, pdf_fd): +async def created_agreement_attachment(agreement_attachments, agreement_attachment_factory, pdf_fd): @@ -def test_create_agreement_attachment(created_agreement_attachment): - result = created_agreement_attachment +async def test_create_agreement_attachment(created_agreement_attachment): + result = created_agreement_attachmentPlease double-check how async fixtures are handled in your current pytest / pytest-asyncio setup to ensure this test is exercising the create call as intended.
Also applies to: 55-59
mpt_api_client/resources/commerce/agreements_attachments.py (1)
23-31: Mixin composition and upload config look consistent; consider updating the config docstringThe switch to
{Create,Update,Download}FileMixin(and async counterparts) plus_upload_file_key = "file"/_upload_data_key = "attachment"aligns with the generic file mixin contracts and should support upload/download correctly for this resource.Minor nit:
AgreementsAttachmentServiceConfigstill has the docstring"Orders service config.", which is slightly misleading now that this is clearly for agreement attachments.-class AgreementsAttachmentServiceConfig: - """Orders service config.""" +class AgreementsAttachmentServiceConfig: + """Agreements attachment service config."""Also applies to: 33-56
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e_config.test.json(2 hunks)mpt_api_client/resources/commerce/agreements_attachments.py(3 hunks)tests/e2e/commerce/agreement/attachment/conftest.py(1 hunks)tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py(1 hunks)tests/e2e/commerce/agreement/attachment/test_sync_agreement_attachment.py(1 hunks)tests/unit/resources/commerce/test_agreements_attachments.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e_config.test.json
- tests/e2e/commerce/agreement/attachment/test_sync_agreement_attachment.py
🧰 Additional context used
🧠 Learnings (1)
📚 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/commerce/agreement/attachment/conftest.py
🧬 Code graph analysis (4)
mpt_api_client/resources/commerce/agreements_attachments.py (1)
mpt_api_client/http/mixins.py (11)
AsyncCollectionMixin(538-605)AsyncCreateFileMixin(187-216)AsyncDeleteMixin(273-283)AsyncDownloadFileMixin(303-325)AsyncUpdateMixin(286-300)CollectionMixin(469-535)CreateFileMixin(115-144)DeleteMixin(29-38)DownloadFileMixin(58-80)GetMixin(361-377)UpdateMixin(41-55)
tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/commerce/agreement/attachment/conftest.py (3)
agreement_attachment_factory(15-28)agreement_attachment_id(10-11)invalid_agreement_attachment_id(5-6)tests/e2e/conftest.py (2)
pdf_fd(73-75)async_mpt_ops(36-39)mpt_api_client/models/file_model.py (2)
file_contents(35-45)filename(13-32)
tests/unit/resources/commerce/test_agreements_attachments.py (16)
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/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/unit/resources/billing/test_invoice_attachments.py (1)
test_methods_present(41-44)tests/unit/resources/billing/test_invoices.py (1)
test_methods_present(27-30)tests/unit/resources/billing/test_journal_attachments.py (1)
test_methods_present(41-44)tests/unit/resources/billing/test_journal_charges.py (1)
test_methods_present(38-41)tests/unit/resources/billing/test_journal_sellers.py (1)
test_methods_present(38-41)tests/unit/resources/billing/test_journal_upload.py (1)
test_methods_present(36-39)tests/unit/resources/billing/test_ledger_charges.py (1)
test_methods_present(36-39)tests/unit/resources/billing/test_ledger_attachments.py (1)
test_methods_present(41-44)tests/unit/resources/billing/test_statement_charges.py (1)
test_methods_present(40-43)
tests/e2e/commerce/agreement/attachment/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
🪛 Ruff (0.14.7)
mpt_api_client/resources/commerce/agreements_attachments.py
2-2: Unused noqa directive (unknown: WPS235)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (2)
tests/unit/resources/commerce/test_agreements_attachments.py (1)
35-42: Method presence tests now aligned with service APIIncluding
updateanditerate(alongsideget,create,delete,download) on both sync and async services matches the new mixin composition and keeps these tests consistent with other attachment resources.tests/e2e/commerce/agreement/attachment/conftest.py (1)
4-28: Fixture set for agreement attachments is clear and reusableThe invalid ID, config-driven ID, and payload factory fixtures are well-factored and align with the existing e2e config pattern, making the async/sync attachment tests easy to compose and extend.
|
robcsegal
deleted the
MPT-14905-add-e-2-e-tests-for-commerce-agreements-attachments
branch
Fixed agreement attachments endpoints, created e2e tests
https://softwareone.atlassian.net/browse/MPT-14905
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.