[MPT-15606] Updated mixins for create and update in account #126
Conversation
robcsegal
force-pushed
the
MPT-15606-update-implementation-of-create-and-update-mixin-with-file
branch
2 times, most recently
from
d34bc5a to
ff5ec99
Compare
robcsegal
changed the title
robcsegal
requested review from
albertsola,
alephsur,
d3rky,
jentyk,
ruben-sebrango and
svazquezco
as code owners
robcsegal
force-pushed
the
MPT-15606-update-implementation-of-create-and-update-mixin-with-file
branch
2 times, most recently
from
696e514 to
ed30dfb
Compare
robcsegal
force-pushed
the
MPT-15606-update-implementation-of-create-and-update-mixin-with-file
branch
3 times, most recently
from
d7ead76 to
708f19f
Compare
robcsegal
force-pushed
the
MPT-15606-update-implementation-of-create-and-update-mixin-with-file
branch
from
708f19f to
46c548e
Compare
WalkthroughReplaces icon-specific mixins with file-upload mixins (sync + async), introduces Create/Update File mixins, updates many services to use them and add Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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
🧹 Nitpick comments (3)
mpt_api_client/resources/accounts/mixins.py (1)
140-140: Remove unusednoqadirective.Ruff does not recognize
WPS110as a valid code. Ifwemake-python-styleguideis not being used by Ruff, this directive is unnecessary.- def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model: # noqa: WPS110 + def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model:The same applies to lines 176, 378, and 414.
tests/unit/resources/catalog/test_mixins.py (2)
58-66: Remove unusednoqadirective.Per static analysis (Ruff RUF100), the
# noqa: WPS215directive is not recognized and should be removed.-class DummyUpdateFileService( # noqa: WPS215 +class DummyUpdateFileService( UpdateFileMixin[DummyModel], Service[DummyModel], ):
69-77: Remove unusednoqadirective.Per static analysis (Ruff RUF100), the
# noqa: WPS215directive is not recognized and should be removed.-class DummyAsyncUpdateFileService( # noqa: WPS215 +class DummyAsyncUpdateFileService( AsyncUpdateFileMixin[DummyModel], AsyncService[DummyModel], ):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
mpt_api_client/http/mixins.py(0 hunks)mpt_api_client/resources/accounts/account.py(3 hunks)mpt_api_client/resources/accounts/buyers.py(3 hunks)mpt_api_client/resources/accounts/licensees.py(2 hunks)mpt_api_client/resources/accounts/mixins.py(3 hunks)mpt_api_client/resources/catalog/mixins.py(4 hunks)mpt_api_client/resources/catalog/products.py(3 hunks)seed/catalog/product.py(1 hunks)setup.cfg(1 hunks)tests/e2e/accounts/account/test_async_account.py(4 hunks)tests/e2e/accounts/account/test_sync_account.py(3 hunks)tests/e2e/accounts/buyers/test_async_buyers.py(3 hunks)tests/e2e/accounts/buyers/test_sync_buyers.py(3 hunks)tests/e2e/accounts/licensees/test_async_licensees.py(3 hunks)tests/e2e/accounts/licensees/test_sync_licensees.py(3 hunks)tests/e2e/catalog/product/test_async_product.py(2 hunks)tests/e2e/catalog/product/test_sync_product.py(2 hunks)tests/unit/http/test_mixins.py(2 hunks)tests/unit/resources/accounts/test_account.py(4 hunks)tests/unit/resources/accounts/test_buyers.py(4 hunks)tests/unit/resources/accounts/test_licensees.py(4 hunks)tests/unit/resources/accounts/test_mixins.py(4 hunks)tests/unit/resources/catalog/test_mixins.py(4 hunks)tests/unit/resources/catalog/test_products.py(4 hunks)
💤 Files with no reviewable changes (1)
- mpt_api_client/http/mixins.py
🧰 Additional context used
🧬 Code graph analysis (20)
tests/unit/resources/catalog/test_products.py (2)
mpt_api_client/resources/accounts/mixins.py (2)
create(140-166)create(378-404)mpt_api_client/resources/catalog/mixins.py (2)
create(89-112)create(170-193)
mpt_api_client/resources/accounts/licensees.py (3)
mpt_api_client/resources/accounts/mixins.py (2)
AsyncLicenseeMixin(467-472)LicenseeMixin(229-234)mpt_api_client/http/mixins.py (2)
GetMixin(217-233)DeleteMixin(29-38)mpt_api_client/http/service.py (1)
Service(12-80)
tests/e2e/catalog/product/test_async_product.py (1)
mpt_api_client/resources/catalog/mixins.py (2)
create(89-112)create(170-193)
tests/e2e/catalog/product/test_sync_product.py (1)
mpt_api_client/resources/catalog/mixins.py (2)
create(89-112)create(170-193)
tests/e2e/accounts/account/test_sync_account.py (3)
tests/e2e/accounts/account/conftest.py (2)
account_factory(10-27)invalid_account_id(5-6)tests/e2e/accounts/conftest.py (1)
account_icon(12-13)mpt_api_client/resources/accounts/mixins.py (2)
create(140-166)create(378-404)
tests/unit/resources/accounts/test_licensees.py (3)
mpt_api_client/resources/accounts/mixins.py (2)
create(140-166)create(378-404)mpt_api_client/resources/catalog/mixins.py (2)
create(89-112)create(170-193)mpt_api_client/http/mixins.py (4)
create(18-26)create(86-112)create(118-126)create(187-214)
tests/e2e/accounts/licensees/test_async_licensees.py (3)
tests/e2e/accounts/conftest.py (1)
account_icon(12-13)mpt_api_client/models/model.py (1)
id(119-121)tests/e2e/accounts/licensees/conftest.py (1)
invalid_licensee_id(20-21)
tests/e2e/accounts/buyers/test_async_buyers.py (3)
tests/e2e/accounts/conftest.py (1)
account_icon(12-13)mpt_api_client/models/model.py (1)
id(119-121)tests/e2e/accounts/buyers/conftest.py (1)
invalid_buyer_id(5-6)
tests/e2e/accounts/licensees/test_sync_licensees.py (3)
tests/e2e/accounts/conftest.py (1)
account_icon(12-13)mpt_api_client/models/model.py (1)
id(119-121)tests/e2e/accounts/licensees/conftest.py (1)
invalid_licensee_id(20-21)
tests/e2e/accounts/account/test_async_account.py (1)
mpt_api_client/resources/accounts/mixins.py (2)
create(140-166)create(378-404)
tests/e2e/accounts/buyers/test_sync_buyers.py (3)
mpt_api_client/resources/accounts/mixins.py (2)
create(140-166)create(378-404)mpt_api_client/http/mixins.py (4)
create(18-26)create(86-112)create(118-126)create(187-214)tests/e2e/accounts/conftest.py (1)
account_icon(12-13)
mpt_api_client/resources/catalog/products.py (1)
mpt_api_client/resources/catalog/mixins.py (2)
AsyncProductMixin(159-164)ProductMixin(232-237)
tests/unit/resources/accounts/test_account.py (1)
mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)
mpt_api_client/resources/accounts/buyers.py (1)
mpt_api_client/resources/accounts/mixins.py (2)
AsyncBuyerMixin(457-464)BuyerMixin(219-226)
tests/unit/resources/catalog/test_mixins.py (1)
mpt_api_client/resources/catalog/mixins.py (4)
AsyncUpdateFileMixin(115-148)UpdateFileMixin(196-229)update(118-148)update(199-229)
mpt_api_client/resources/catalog/mixins.py (1)
mpt_api_client/resources/accounts/mixins.py (4)
AsyncUpdateFileMixin(407-444)update(172-206)update(410-444)UpdateFileMixin(169-206)
mpt_api_client/resources/accounts/mixins.py (1)
mpt_api_client/http/mixins.py (6)
create(18-26)create(86-112)create(118-126)create(187-214)update(44-55)update(145-156)
tests/unit/resources/accounts/test_mixins.py (1)
mpt_api_client/resources/accounts/mixins.py (8)
AsyncCreateFileMixin(375-404)AsyncUpdateFileMixin(407-444)CreateFileMixin(137-166)UpdateFileMixin(169-206)create(140-166)create(378-404)update(172-206)update(410-444)
tests/unit/resources/accounts/test_buyers.py (1)
mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)
mpt_api_client/resources/accounts/account.py (1)
mpt_api_client/resources/accounts/mixins.py (2)
AccountMixin(209-216)AsyncAccountMixin(447-454)
🪛 Ruff (0.14.5)
tests/unit/resources/catalog/test_mixins.py
58-58: Unused noqa directive (unknown: WPS215)
Remove unused noqa directive
(RUF100)
69-69: Unused noqa directive (unknown: WPS215)
Remove unused noqa directive
(RUF100)
mpt_api_client/resources/accounts/mixins.py
140-140: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
176-176: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
378-378: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
414-414: Unused noqa directive (unknown: WPS110)
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 (58)
setup.cfg (1)
50-50: LGTM!The wildcard pattern consolidates linter suppressions for test mixin files across different resource types, which is appropriate for test files that often have many local variables and test fixtures.
mpt_api_client/resources/accounts/licensees.py (4)
11-14: LGTM!Clean migration to consolidated
LicenseeMixinandAsyncLicenseeMixin, reducing the number of explicit mixin imports.
27-28: LGTM!Configuration attributes properly define the upload keys for the file-upload mixins.
31-39: LGTM!The service class now uses the consolidated
LicenseeMixin[Licensee]which combines file upload capabilities with enablable behavior.
42-50: LGTM!Async service mirrors the sync service pattern with
AsyncLicenseeMixin[Licensee].mpt_api_client/resources/accounts/buyers.py (4)
12-15: LGTM!Clean migration to consolidated
BuyerMixinandAsyncBuyerMixin.
28-29: LGTM!Configuration attributes properly define the upload keys for the file-upload mixins, consistent with the pattern used in
LicenseesServiceConfig.
32-40: LGTM!The service class uses the consolidated
BuyerMixin[Buyer]which combines file upload with activatable, enablable, and validate behaviors.
61-68: LGTM!Async service mirrors the sync service pattern with
AsyncBuyerMixin[Buyer].mpt_api_client/resources/accounts/mixins.py (2)
209-234: LGTM!The composite mixins (
AccountMixin,BuyerMixin,LicenseeMixin) provide a clean abstraction that combines file upload capabilities with resource-specific behaviors. This consolidation reduces boilerplate in service class definitions.
447-472: LGTM!The async composite mixins mirror the sync pattern appropriately.
mpt_api_client/resources/catalog/mixins.py (2)
159-164: LGTM!
AsyncProductMixinis a clean composite that aggregates file create/update and publishable behaviors for async product resources.
232-237: LGTM!
ProductMixinmirrors the async version appropriately for synchronous product resources.seed/catalog/product.py (1)
52-52: LGTM! Parameter rename aligns with the new file-upload API.The change from
icon=icon_filetofile=icon_fileis consistent with the broader refactoring to use generic file-upload mixins across the codebase.tests/e2e/catalog/product/test_async_product.py (2)
9-9: LGTM! Test updated to use the new file parameter.The parameter rename from
icontofilein the create call is consistent with the new ProductMixin API introduced in this PR.
31-31: LGTM! Update operation aligned with new API.The parameter rename from
icontofileis consistent with the UpdateFileMixin implementation shown in the relevant code snippets.tests/e2e/catalog/product/test_sync_product.py (2)
9-9: LGTM! Sync test aligned with async counterpart.The parameter rename is consistent with the async test and the new file-upload API.
28-28: LGTM! Update operation consistent with new API.The change mirrors the async version and aligns with the consolidated file-upload mixins.
tests/unit/resources/catalog/test_products.py (4)
152-152: LGTM! Unit test updated for new file parameter.The sync create operation now uses
file=icon_file, consistent with the new ProductMixin API.
174-174: LGTM! Async create operation aligned.The parameter rename is consistent with the async CreateFileMixin implementation.
199-199: LGTM! Sync update operation aligned.The change from
icon=icon_filetofile=icon_fileis consistent with the UpdateFileMixin.
225-225: LGTM! Async update operation aligned.All four product operations (sync/async create/update) now consistently use the
fileparameter.tests/e2e/accounts/licensees/test_async_licensees.py (3)
14-14: LGTM! Licensee creation aligned with new file-upload API.The parameter rename from
logotofileis consistent with the new LicenseeMixin composition introduced in this PR.
76-76: LGTM! Update operation consistent with new API.The change aligns with the UpdateFileMixin implementation for licensees.
89-89: LGTM! Error path also updated.Good practice to update the error-handling test path as well, ensuring consistency across all code paths.
tests/unit/resources/accounts/test_licensees.py (4)
48-48: LGTM! Sync create operation updated.The parameter rename from
logotofilealigns with the new LicenseeMixin API.
72-72: LGTM! Sync update operation aligned.The change is consistent with the UpdateFileMixin implementation for licensees.
96-96: LGTM! Async create operation aligned.The parameter rename matches the async CreateFileMixin signature.
120-120: LGTM! Async update operation aligned.All licensee operations now consistently use the
fileparameter across sync and async implementations.tests/e2e/accounts/licensees/test_sync_licensees.py (3)
14-14: LGTM! Sync licensee creation aligned with async version.The parameter rename is consistent with the new file-upload API across all licensee operations.
73-73: LGTM! Update operation consistent.The change aligns with the UpdateFileMixin implementation and the async counterpart.
84-84: LGTM! Error path updated.Comprehensive test coverage maintained with the error path also using the new parameter name.
tests/e2e/accounts/account/test_async_account.py (4)
13-13: LGTM! Account creation aligned with new file-upload API.The parameter rename from
logotofileis consistent with the new AccountMixin composition.
48-48: LGTM! Update operation consistent with new API.The change aligns with the UpdateFileMixin implementation for accounts.
61-61: LGTM! Invalid data error path updated.Good test coverage includes the error-handling paths with the new parameter name.
72-72: LGTM! Not found error path updated.All account operation test paths now consistently use the
fileparameter, ensuring comprehensive test coverage of the new API.tests/e2e/accounts/buyers/test_sync_buyers.py (1)
16-16: LGTM! Parameter rename applied consistently.The keyword argument has been correctly updated from
logotofileacross all buyer create and update operations, aligning with the new generic file-upload API.Also applies to: 74-74, 86-86
tests/e2e/accounts/buyers/test_async_buyers.py (1)
17-17: LGTM! Async parameter rename applied consistently.The keyword argument has been correctly updated from
logotofileacross all async buyer create and update operations, mirroring the sync API changes.Also applies to: 77-77, 90-90
mpt_api_client/resources/catalog/products.py (1)
12-13: LGTM! Mixin consolidation applied correctly.The refactoring successfully consolidates multiple icon-specific mixins into unified
ProductMixinandAsyncProductMixin. The upload configuration keys (_upload_file_key = "icon"and_upload_data_key = "product") are properly defined to support the new file-upload semantics.Also applies to: 55-56, 60-60, 115-115
tests/unit/resources/accounts/test_account.py (1)
90-90: LGTM! Unit test parameter names updated consistently.All account create and update test cases (both sync and async) correctly use the
fileparameter instead oflogo, ensuring test alignment with the refactored API.Also applies to: 114-114, 138-138, 162-162
tests/e2e/accounts/account/test_sync_account.py (1)
10-10: LGTM! Account tests updated consistently.The fixture signature has been cleaned up by removing the unused
loggerparameter, and all account create/update operations now correctly use thefileparameter instead oflogo.Also applies to: 13-13, 48-48, 58-58, 66-66
tests/unit/resources/accounts/test_buyers.py (1)
178-178: LGTM! Buyer unit tests updated consistently.All buyer create and update test cases (both sync and async) correctly use the
fileparameter instead oflogo, maintaining consistency with the refactored API.Also applies to: 202-202, 226-226, 250-250
tests/unit/resources/accounts/test_mixins.py (4)
10-10: LGTM! New file mixin imports added.The new file-upload mixins (
CreateFileMixin,UpdateFileMixin, and their async counterparts) are properly imported to support the refactored file-upload functionality.Also applies to: 13-13, 16-16, 19-19
115-157: LGTM! Test service classes for file mixins added.Four new test service classes are properly defined to validate the file-upload mixins:
- Sync and async create file services
- Sync and async update file services
Each service correctly configures
_upload_file_key = "file"and_upload_data_key = "resource", aligning with the new mixin contracts.
209-227: LGTM! Fixtures for file services added.Fixtures are properly defined for all four file service variants, enabling comprehensive test coverage of the new file-upload mixins.
843-1014: LGTM! Comprehensive test coverage for file mixins.Excellent test coverage added for the new file-upload functionality:
- Create operations (with and without files) for both sync and async
- Update operations (with and without files) for both sync and async
- All tests verify HTTP interactions and response types correctly
This ensures the new mixins work as expected across all scenarios.
mpt_api_client/resources/accounts/account.py (1)
14-15: LGTM! Account service refactored with consolidated mixins.The refactoring successfully consolidates multiple account-specific mixins into unified
AccountMixinandAsyncAccountMixin. The upload configuration keys (_upload_file_key = "logo"and_upload_data_key = "account") are properly defined, ensuring consistent file-upload behavior across the accounts API.Also applies to: 29-30, 34-34, 50-50
tests/unit/resources/catalog/test_mixins.py (4)
122-129: LGTM!The fixtures follow the established pattern in this file and correctly inject the appropriate HTTP clients.
510-537: LGTM!The test correctly verifies the update operation with a file upload, checking the multipart form-data structure and response deserialization.
540-562: LGTM!Good coverage of the no-file scenario, verifying that the request still uses
multipart/form-data(matching theforce_multipart=Truein the mixin implementation) while correctly omitting the file part.
565-619: LGTM!The async variants mirror the sync tests appropriately, ensuring consistent behavior across both implementations.
tests/unit/http/test_mixins.py (7)
9-18: LGTM!The import updates correctly reflect the transition from icon-specific mixins to generic file-operations mixins, and the addition of
APPLICATION_JSONandMPTErrorsupports the new test cases.
45-64: LGTM!The fixtures are well-structured. The media service fixtures correctly demonstrate the use of
endpoint_paramsfor dynamic endpoint configuration.
1011-1052: LGTM!Good coverage of file creation without resource data for both sync and async variants. The tests correctly verify the multipart form structure.
1055-1096: LGTM!Good edge case coverage for the download operation when
content_typeis missing from the resource. The tests correctly verify thatMPTErroris raised with an informative message.
1099-1140: LGTM!Good coverage of the download operation with an explicit
acceptparameter, which bypasses the initial request to fetch the resource's content type.
1143-1180: LGTM!Good coverage of file creation with resource data. The tests correctly verify that the resource data is included in the multipart form under the
_attachment_datakey.
27-42: The review comment is incorrect. TheFilesOperationsMixinat the HTTP level does not require_upload_file_keyor_upload_data_keyclass attributes.These mixins use method parameters instead: the
create()method accepts adata_keyparameter (defaulting to"_attachment_data"), andfilesare passed directly as arguments. The existing tests confirm this works correctly—they successfully callcreate()onDummyFileOperationsServicewithout defining these attributes.This differs from resource-level mixins (in
catalog/andaccounts/subdirectories), which do use these class attributes, but that's a separate design pattern.
robcsegal
force-pushed
the
MPT-15606-update-implementation-of-create-and-update-mixin-with-file
branch
2 times, most recently
from
fc0e125 to
3e51092
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit/http/test_mixins.py (2)
993-1002: Remove extra blank line.The function has too many empty lines (3 where maximum is 2). Remove one blank line after the function definition.
Apply this diff:
def test_modifieable_resource_mixin(method_name): """Test that ModifiableResourceMixin has the required methods.""" - class Service(ModifiableResourceMixin[DummyModel]): # noqa: WPS431 """Dummy service class for testing required methods."""
1036-1045: Remove extra blank line.The function has too many empty lines (3 where maximum is 2). Remove one blank line after the function definition.
Apply this diff:
def test_managed_resource_mixin(method_name): """Test that ManagedResourceMixin has the required methods.""" - class ManagedService(ManagedResourceMixin[DummyModel]): # noqa: WPS431 """Dummy service class for testing required methods."""
♻️ Duplicate comments (2)
mpt_api_client/resources/accounts/mixins.py (1)
137-235: Consolidate file-upload mixins instead of duplicating implementations
CreateFileMixin/UpdateFileMixinand their async counterparts here are effectively copies of the file-upload mixins already defined in other modules (e.g.,http.mixins/catalog.mixins), and a past review has already raised this. To reduce maintenance overhead and keep behavior consistent, consider centralizing these mixins in a single module (likelympt_api_client.http.mixins) and importing them here, rather than redefining them. Also, Ruff reports the# noqa: WPS110directives as unknown; if you’re not running the WPS plugin anymore, you can safely drop those comments.Also applies to: 375-472
mpt_api_client/resources/catalog/mixins.py (1)
115-148: Avoid duplicating file-upload mixins across modules
AsyncUpdateFileMixin/UpdateFileMixinand the newAsyncProductMixin/ProductMixinfollow the right multipart upload pattern and look functionally correct. However, these implementations now overlap with file-upload mixins in bothhttp.mixinsandaccounts.mixins; a previous review already suggested consolidating them. It would be cleaner to keep a single set of Create/Update file mixins (e.g., inmpt_api_client.http.mixins) and import them here, using catalog-specific composites only for composition and naming.Also applies to: 159-165, 196-230, 232-238
🧹 Nitpick comments (4)
tests/e2e/accounts/account/test_async_account.py (1)
83-90: Remove unusedaccount_factoryfixture argument
account_factoryintest_account_enableis never used; consider removing it from the signature to keep the test lean and silence ARG001.tests/e2e/accounts/account/test_sync_account.py (1)
78-85: Drop unusedaccount_factoryintest_account_enable
account_factoryisn’t referenced in the test body; you can remove it from the parameters to avoid the ARG001 warning.tests/unit/resources/catalog/test_mixins.py (2)
58-78: New update-file dummy services and fixtures look correctThe dummy services correctly set
_endpoint,_upload_file_key, and_upload_data_key, and the fixtures wire them into the tests as expected. One minor nit: Ruff reports the# noqa: WPS215on these two new classes as unused; if you’re no longer running wemake, you could drop those directives (or align the rest of the file later).Also applies to: 69-77, 122-130
510-619: Good coverage for update-file mixins; optional extra assertionThe sync/async update-file tests nicely cover both “with file” and “no file” cases, checking the multipart content type and presence/absence of the file part. For even stronger parity with the document-create tests, you could also assert that the JSON
documentpart is present in the multipart body.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
mpt_api_client/http/mixins.py(0 hunks)mpt_api_client/resources/accounts/account.py(3 hunks)mpt_api_client/resources/accounts/buyers.py(3 hunks)mpt_api_client/resources/accounts/licensees.py(2 hunks)mpt_api_client/resources/accounts/mixins.py(3 hunks)mpt_api_client/resources/catalog/mixins.py(4 hunks)mpt_api_client/resources/catalog/products.py(3 hunks)seed/catalog/product.py(1 hunks)setup.cfg(1 hunks)tests/e2e/accounts/account/test_async_account.py(5 hunks)tests/e2e/accounts/account/test_sync_account.py(3 hunks)tests/e2e/accounts/buyers/test_async_buyers.py(5 hunks)tests/e2e/accounts/buyers/test_sync_buyers.py(5 hunks)tests/e2e/accounts/licensees/test_async_licensees.py(5 hunks)tests/e2e/accounts/licensees/test_sync_licensees.py(3 hunks)tests/e2e/catalog/product/test_async_product.py(2 hunks)tests/e2e/catalog/product/test_sync_product.py(2 hunks)tests/unit/http/test_mixins.py(51 hunks)tests/unit/resources/accounts/test_account.py(4 hunks)tests/unit/resources/accounts/test_buyers.py(4 hunks)tests/unit/resources/accounts/test_licensees.py(4 hunks)tests/unit/resources/accounts/test_mixins.py(4 hunks)tests/unit/resources/catalog/test_mixins.py(4 hunks)tests/unit/resources/catalog/test_products.py(4 hunks)
💤 Files with no reviewable changes (1)
- mpt_api_client/http/mixins.py
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/e2e/catalog/product/test_sync_product.py
- tests/e2e/accounts/licensees/test_sync_licensees.py
- tests/e2e/catalog/product/test_async_product.py
- tests/e2e/accounts/buyers/test_async_buyers.py
- seed/catalog/product.py
- tests/unit/resources/catalog/test_products.py
- tests/e2e/accounts/licensees/test_async_licensees.py
🧰 Additional context used
🧬 Code graph analysis (8)
tests/unit/resources/catalog/test_mixins.py (3)
mpt_api_client/resources/catalog/mixins.py (4)
AsyncUpdateFileMixin(115-148)UpdateFileMixin(196-229)update(118-148)update(199-229)tests/unit/conftest.py (3)
DummyModel(10-13)http_client(17-18)async_http_client(22-23)mpt_api_client/http/mixins.py (2)
update(44-55)update(145-156)
mpt_api_client/resources/accounts/account.py (1)
mpt_api_client/resources/accounts/mixins.py (2)
AccountMixin(209-216)AsyncAccountMixin(447-454)
mpt_api_client/resources/accounts/mixins.py (1)
mpt_api_client/resources/catalog/mixins.py (8)
CreateFileMixin(167-193)create(89-112)create(170-193)UpdateFileMixin(196-229)update(118-148)update(199-229)AsyncCreateFileMixin(86-112)AsyncUpdateFileMixin(115-148)
mpt_api_client/resources/accounts/buyers.py (1)
mpt_api_client/resources/accounts/mixins.py (2)
AsyncBuyerMixin(457-464)BuyerMixin(219-226)
tests/unit/http/test_mixins.py (2)
mpt_api_client/http/mixins.py (8)
AsyncFilesOperationsMixin(184-214)FilesOperationsMixin(83-112)create(18-26)create(86-112)create(118-126)create(187-214)download(61-80)download(162-181)mpt_api_client/models/file_model.py (1)
file_contents(35-45)
tests/unit/resources/accounts/test_account.py (1)
mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)
mpt_api_client/resources/accounts/licensees.py (1)
mpt_api_client/resources/accounts/mixins.py (2)
AsyncLicenseeMixin(467-472)LicenseeMixin(229-234)
mpt_api_client/resources/catalog/products.py (1)
mpt_api_client/resources/catalog/mixins.py (2)
AsyncProductMixin(159-164)ProductMixin(232-237)
🪛 GitHub Actions: PR build and merge
tests/unit/http/test_mixins.py
[warning] 993-993: WPS473 Found too many empty lines in def: 3 > 2
[warning] 1036-1036: WPS473 Found too many empty lines in def: 3 > 2
🪛 Ruff (0.14.5)
tests/unit/resources/catalog/test_mixins.py
58-58: Unused noqa directive (unknown: WPS215)
Remove unused noqa directive
(RUF100)
69-69: Unused noqa directive (unknown: WPS215)
Remove unused noqa directive
(RUF100)
mpt_api_client/resources/accounts/mixins.py
140-140: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
176-176: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
378-378: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
414-414: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
tests/unit/http/test_mixins.py
73-73: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/account/test_async_account.py
83-83: Unused function argument: account_factory
(ARG001)
tests/e2e/accounts/account/test_sync_account.py
78-78: Unused function argument: account_factory
(ARG001)
🔇 Additional comments (40)
setup.cfg (1)
50-50: Per-file ignore for WPS210 in test mixins is consistent with existing test lint policyExtending the ignores for
tests/unit/resources/*/test_mixins.pywithWPS210matches how other test modules are configured and keeps style strictness relaxed where tests tend to be more verbose. No functional or tooling issues from this change.tests/unit/http/test_mixins.py (12)
9-10: LGTM!The import updates correctly reflect the refactoring from icon-specific mixins to generic file-operations mixins, and add necessary constants and exceptions for the new test coverage.
Also applies to: 13-13, 16-16
27-44: LGTM!The test service classes are correctly refactored to use the generic
FilesOperationsMixinandAsyncFilesOperationsMixin, with endpoints updated to match the new file-operations semantics.
47-70: LGTM!The fixtures are properly updated to reflect the renamed services and include the necessary media service fixtures for testing file operations with the catalog products media endpoint.
74-74: LGTM!The added docstrings improve test documentation and follow a consistent, descriptive format.
Also applies to: 95-95, 109-109, 126-126, 147-147, 160-160
1071-1092: LGTM!This test correctly validates the file creation flow when only file data is provided without additional resource metadata.
1094-1115: LGTM!The async version correctly mirrors the sync test, properly validating file creation without resource data in an async context.
1117-1138: LGTM!This test properly validates error handling when attempting to download a file without a valid content type, ensuring the appropriate
MPTErroris raised with a clear error message.
1140-1161: LGTM!The async version correctly validates the same error handling scenario in an async context.
1163-1184: LGTM!This test validates the optimized download path where an explicit
Acceptheader is provided, bypassing the need to fetch resource metadata first.
1186-1207: LGTM!The async version properly validates the same optimized download path in an async context.
1209-1227: LGTM!This test correctly validates the complete file creation flow with both file data and resource metadata, ensuring proper multipart encoding with the
_attachment_datafield.
1229-1248: LGTM!The async version correctly validates the same complete file creation flow in an async context.
tests/e2e/accounts/account/test_async_account.py (1)
11-122: Async account tests align with newfile-based APIUsing
file=account_iconin create/update and the added docstrings all look consistent with the new mixins; behavior remains clear and well-covered.tests/e2e/accounts/account/test_sync_account.py (1)
10-117: Sync account tests correctly migrated tofileparameterFixture and test calls now use
file=account_icon, and docstrings describe behavior clearly; this matches the new file-upload mixins and keeps coverage intact.mpt_api_client/resources/catalog/products.py (1)
52-57: Products services correctly switched to generic product file mixinsAdding
_upload_file_key = "icon"and_upload_data_key = "product"and inheriting fromProductMixin/AsyncProductMixincleanly replaces the icon-specific create/update logic with the shared file-upload mixins while preserving existing behaviors. This looks consistent with the updated catalog tests and keeps the public API coherent.Also applies to: 59-66, 114-121
tests/unit/resources/accounts/test_licensees.py (1)
48-49: Licensee tests updated tofileparameter consistentlyThe sync and async licensee tests now pass
file=logo_filefor create/update while keeping the same URL and response assertions, matching the new mixin-based API. Looks good.Also applies to: 72-73, 96-97, 120-121
tests/unit/resources/accounts/test_buyers.py (1)
178-179: Buyer tests correctly migrated tofile-based create/updateAll buyer create/update calls (sync and async) now use
file=logo_filewith the same endpoint and response checks, aligning with the new Buyer mixins and upload configuration. No issues spotted.Also applies to: 202-203, 226-227, 250-251
tests/e2e/accounts/buyers/test_sync_buyers.py (2)
9-24: LGTM! Well-structured fixture with proper cleanup.The parameter rename from
logotofilealigns with the new generic file-upload mixin design. The added teardown logic to delete the buyer after tests is a good practice for E2E test hygiene.
27-124: LGTM! Docstrings and parameter updates are consistent.The added docstrings improve test documentation, and the
file=account_iconparameter changes at lines 83 and 96 are consistent with the API changes.tests/unit/resources/accounts/test_account.py (4)
76-97: LGTM! Sync create test updated correctly.The parameter change to
file=logo_filematches the newCreateFileMixin.create(resource_data, file)signature.
100-121: LGTM! Sync update test updated correctly.The parameter change to
file=logo_filematches the newUpdateFileMixin.update(resource_id, resource_data, file)signature.
124-145: LGTM! Async create test updated correctly.
148-169: LGTM! Async update test updated correctly.mpt_api_client/resources/accounts/account.py (4)
13-16: LGTM! Clean import consolidation.Replacing multiple icon-specific mixins with the consolidated
AccountMixinandAsyncAccountMixinimproves maintainability.
23-30: LGTM! Upload configuration keys properly defined.The
_upload_file_key = "logo"and_upload_data_key = "account"correctly configure how the generic file mixins will handle account-specific uploads.
33-46: LGTM! Simplified service inheritance.The
AccountMixin[Account]composite mixin cleanly replaces the previous separate mixins (CreateWithIconMixin,UpdateWithIconMixin,ActivatableMixin,EnablableMixin,ValidateMixin) while maintaining the same functionality.
49-62: LGTM! Async service follows the same pattern.tests/unit/resources/accounts/test_mixins.py (5)
6-21: LGTM! Import additions for new file mixins.The new imports for
CreateFileMixin,AsyncCreateFileMixin,UpdateFileMixin, andAsyncUpdateFileMixinare properly organized with the existing mixin imports.
115-156: LGTM! Well-structured dummy service classes for testing.The dummy services correctly configure both
_upload_file_keyand_upload_data_key, mirroring the real service implementations. Good use of consistent endpoint naming (create-file,update-file).
209-227: LGTM! Fixtures for new file services.
843-920: LGTM! Comprehensive create file mixin tests.Tests cover both sync and async paths, with and without file data. The tests properly validate HTTP method, endpoint, and response mapping.
923-1014: LGTM! Comprehensive update file mixin tests.Tests cover both sync and async paths, with and without file data. The endpoint path construction (
/OBJ-0000-0001) is correctly validated.mpt_api_client/resources/accounts/buyers.py (4)
12-15: LGTM! Import consolidation consistent with account service.
22-29: LGTM! Upload configuration properly set for buyers.The
_upload_file_key = "logo"and_upload_data_key = "buyer"correctly configure the generic file mixins for buyer-specific uploads.
32-58: LGTM! Clean mixin composition for sync service.The
BuyerMixin[Buyer]composite provides create/update with file handling, while domain-specific methods (synchronize,transfer) are retained inline.
61-89: LGTM! Async service follows the same pattern.The async implementations of
synchronizeandtransfercorrectly useawait self._resource_action().mpt_api_client/resources/accounts/licensees.py (3)
11-14: Imports correctly aligned with new consolidated licensee mixinsSwitching to
LicenseeMixin/AsyncLicenseeMixinsimplifies the surface and matches the new shared mixin design. No issues spotted here.
24-28: Upload config keys look consistent with file-mixin expectationsDefining
_upload_file_key = "logo"and_upload_data_key = "licensee"is consistent with the generic file-upload mixins and keeps the HTTP field naming localized in the config. Assuming these keys match the API contract, this part looks good.
31-38: Service base classes correctly switched to Licensee mixinsUsing
LicenseeMixin[Licensee]/AsyncLicenseeMixin[Licensee]as the primary bases cleanly aggregates create/update-with-file and enable/disable behavior for licensees while keeping the existing get/delete/collection/service stack intact. The MRO here looks reasonable and no functional issues are apparent, assuming the mixins expose the same public methods as the previous composition.Also applies to: 42-49
robcsegal
force-pushed
the
MPT-15606-update-implementation-of-create-and-update-mixin-with-file
branch
2 times, most recently
from
5b13856 to
668ba58
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
mpt_api_client/resources/catalog/mixins.py (1)
115-148: Duplicate mixin implementations exist across modules.This
AsyncUpdateFileMixinis duplicated inmpt_api_client/resources/accounts/mixins.py. As noted in a previous review, consider consolidating all file-upload mixins inmpt_api_client/http/mixins.pyand importing them where needed.mpt_api_client/resources/accounts/mixins.py (1)
137-166: DuplicateCreateFileMixinexists in other modules.As noted in a previous review, this mixin is duplicated in
mpt_api_client/http/mixins.pyandmpt_api_client/resources/catalog/mixins.py. Consider consolidating.Also, remove the unused
# noqa: WPS110directive on line 140 (flagged by Ruff RUF100).- def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model: # noqa: WPS110 + def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model:tests/unit/http/test_mixins.py (1)
73-74: Remove unusednoqadirective.This was already flagged in a previous review. The
# noqa: WPS210directive is unused.
🧹 Nitpick comments (7)
tests/e2e/accounts/account/test_async_account.py (1)
83-89: Remove unused parameter.The
account_factoryparameter is not used in thetest_account_enablefunction. Consider removing it to clean up the test signature.Apply this diff to remove the unused parameter:
-async def test_account_enable(async_mpt_ops, account_factory, async_created_account): +async def test_account_enable(async_mpt_ops, async_created_account): """Test enabling an account asynchronously.""" await async_mpt_ops.accounts.accounts.disable(async_created_account.id)tests/e2e/accounts/account/test_sync_account.py (1)
78-84: Remove unusedaccount_factoryargument.The
account_factoryparameter is not used in this test function. This was flagged by static analysis (Ruff ARG001).-def test_account_enable(mpt_ops, account_factory, created_account): +def test_account_enable(mpt_ops, created_account): """Test enabling an account synchronously.""" mpt_ops.accounts.accounts.disable(created_account.id)tests/unit/resources/catalog/test_mixins.py (2)
58-66: Remove unusednoqadirective.The
# noqa: WPS215comment is flagged as unused by Ruff (RUF100). WPS215 is not a recognized rule in the current linter configuration.-class DummyUpdateFileService( # noqa: WPS215 +class DummyUpdateFileService( UpdateFileMixin[DummyModel], Service[DummyModel], ):
69-77: Remove unusednoqadirective.Same issue as above - the
# noqa: WPS215comment is unused.-class DummyAsyncUpdateFileService( # noqa: WPS215 +class DummyAsyncUpdateFileService( AsyncUpdateFileMixin[DummyModel], AsyncService[DummyModel], ):mpt_api_client/resources/accounts/mixins.py (3)
169-206: Remove unusednoqadirective.The
# noqa: WPS110on line 176 is flagged as unused by Ruff.- file: FileTypes | None = None, # noqa: WPS110 + file: FileTypes | None = None,
375-404: Remove unusednoqadirective.The
# noqa: WPS110on line 378 is flagged as unused by Ruff.- async def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model: # noqa: WPS110 + async def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model:
407-444: Remove unusednoqadirective.The
# noqa: WPS110on line 414 is flagged as unused by Ruff.- file: FileTypes | None = None, # noqa: WPS110 + file: FileTypes | None = None,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
mpt_api_client/http/mixins.py(0 hunks)mpt_api_client/resources/accounts/account.py(3 hunks)mpt_api_client/resources/accounts/buyers.py(3 hunks)mpt_api_client/resources/accounts/licensees.py(2 hunks)mpt_api_client/resources/accounts/mixins.py(3 hunks)mpt_api_client/resources/catalog/mixins.py(4 hunks)mpt_api_client/resources/catalog/products.py(3 hunks)seed/catalog/product.py(1 hunks)setup.cfg(1 hunks)tests/e2e/accounts/account/test_async_account.py(5 hunks)tests/e2e/accounts/account/test_sync_account.py(3 hunks)tests/e2e/accounts/buyers/test_async_buyers.py(5 hunks)tests/e2e/accounts/buyers/test_sync_buyers.py(5 hunks)tests/e2e/accounts/licensees/test_async_licensees.py(5 hunks)tests/e2e/accounts/licensees/test_sync_licensees.py(3 hunks)tests/e2e/catalog/product/test_async_product.py(2 hunks)tests/e2e/catalog/product/test_sync_product.py(2 hunks)tests/unit/http/test_mixins.py(51 hunks)tests/unit/resources/accounts/test_account.py(4 hunks)tests/unit/resources/accounts/test_buyers.py(4 hunks)tests/unit/resources/accounts/test_licensees.py(4 hunks)tests/unit/resources/accounts/test_mixins.py(4 hunks)tests/unit/resources/catalog/test_mixins.py(4 hunks)tests/unit/resources/catalog/test_products.py(4 hunks)
💤 Files with no reviewable changes (1)
- mpt_api_client/http/mixins.py
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/unit/resources/accounts/test_licensees.py
- tests/e2e/catalog/product/test_sync_product.py
- mpt_api_client/resources/accounts/account.py
- tests/e2e/accounts/licensees/test_sync_licensees.py
- setup.cfg
- tests/unit/resources/accounts/test_buyers.py
- tests/e2e/catalog/product/test_async_product.py
🧰 Additional context used
🧬 Code graph analysis (13)
tests/unit/resources/accounts/test_mixins.py (3)
mpt_api_client/resources/accounts/mixins.py (8)
AsyncCreateFileMixin(375-404)AsyncUpdateFileMixin(407-444)CreateFileMixin(137-166)UpdateFileMixin(169-206)create(140-166)create(378-404)update(172-206)update(410-444)mpt_api_client/resources/catalog/mixins.py (8)
AsyncCreateFileMixin(86-112)AsyncUpdateFileMixin(115-148)CreateFileMixin(167-193)UpdateFileMixin(196-229)create(89-112)create(170-193)update(118-148)update(199-229)mpt_api_client/http/mixins.py (6)
create(18-26)create(86-112)create(118-126)create(187-214)update(44-55)update(145-156)
tests/unit/resources/accounts/test_account.py (1)
mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)
tests/e2e/accounts/buyers/test_async_buyers.py (1)
mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)
mpt_api_client/resources/accounts/buyers.py (1)
mpt_api_client/resources/accounts/mixins.py (2)
AsyncBuyerMixin(457-464)BuyerMixin(219-226)
tests/e2e/accounts/buyers/test_sync_buyers.py (2)
mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)mpt_api_client/resources/catalog/mixins.py (4)
create(89-112)create(170-193)update(118-148)update(199-229)
tests/e2e/accounts/account/test_sync_account.py (2)
tests/e2e/accounts/conftest.py (2)
account_icon(12-13)account_id(22-23)mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)
mpt_api_client/resources/catalog/products.py (1)
mpt_api_client/resources/catalog/mixins.py (2)
AsyncProductMixin(159-164)ProductMixin(232-237)
tests/unit/http/test_mixins.py (4)
mpt_api_client/http/mixins.py (10)
AsyncFilesOperationsMixin(184-214)AsyncModifiableResourceMixin(468-471)FilesOperationsMixin(83-112)ModifiableResourceMixin(464-465)create(18-26)create(86-112)create(118-126)create(187-214)download(61-80)download(162-181)mpt_api_client/resources/catalog/products_media.py (2)
AsyncMediaService(39-46)MediaService(29-36)tests/unit/conftest.py (2)
http_client(17-18)async_http_client(22-23)mpt_api_client/models/file_model.py (1)
file_contents(35-45)
tests/unit/resources/catalog/test_mixins.py (4)
mpt_api_client/resources/catalog/mixins.py (4)
AsyncUpdateFileMixin(115-148)UpdateFileMixin(196-229)update(118-148)update(199-229)tests/unit/resources/accounts/test_mixins.py (2)
DummyUpdateFileService(137-145)DummyAsyncUpdateFileService(148-156)tests/unit/conftest.py (3)
DummyModel(10-13)http_client(17-18)async_http_client(22-23)mpt_api_client/http/mixins.py (2)
update(44-55)update(145-156)
tests/e2e/accounts/licensees/test_async_licensees.py (2)
mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)tests/e2e/accounts/conftest.py (1)
account_icon(12-13)
mpt_api_client/resources/accounts/licensees.py (1)
mpt_api_client/resources/accounts/mixins.py (2)
AsyncLicenseeMixin(467-472)LicenseeMixin(229-234)
mpt_api_client/resources/accounts/mixins.py (2)
mpt_api_client/resources/catalog/mixins.py (8)
CreateFileMixin(167-193)create(89-112)create(170-193)UpdateFileMixin(196-229)update(118-148)update(199-229)AsyncCreateFileMixin(86-112)AsyncUpdateFileMixin(115-148)mpt_api_client/http/mixins.py (6)
create(18-26)create(86-112)create(118-126)create(187-214)update(44-55)update(145-156)
mpt_api_client/resources/catalog/mixins.py (3)
mpt_api_client/resources/accounts/mixins.py (2)
update(172-206)update(410-444)mpt_api_client/http/mixins.py (2)
update(44-55)update(145-156)mpt_api_client/http/base_service.py (1)
path(28-30)
🪛 Ruff (0.14.5)
tests/e2e/accounts/account/test_sync_account.py
78-78: Unused function argument: account_factory
(ARG001)
tests/unit/http/test_mixins.py
73-73: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
tests/unit/resources/catalog/test_mixins.py
58-58: Unused noqa directive (unknown: WPS215)
Remove unused noqa directive
(RUF100)
69-69: Unused noqa directive (unknown: WPS215)
Remove unused noqa directive
(RUF100)
mpt_api_client/resources/accounts/mixins.py
140-140: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
176-176: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
378-378: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
414-414: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/account/test_async_account.py
83-83: Unused function argument: account_factory
(ARG001)
🔇 Additional comments (31)
seed/catalog/product.py (1)
52-52: LGTM! Parameter rename aligns with the new API.The parameter rename from
icon=icon_filetofile=icon_filecorrectly reflects the standardized file upload API introduced by the new mixin-based approach.tests/unit/resources/catalog/test_products.py (1)
152-152: LGTM! Test parameters updated consistently.All product create and update test calls correctly updated to use the
fileparameter instead oficon, aligning with the new API surface across both sync and async operations.Also applies to: 174-174, 199-199, 225-225
tests/e2e/accounts/licensees/test_async_licensees.py (2)
11-23: LGTM! Fixture properly updated with improved documentation and teardown.The fixture correctly updates the API call to use
file=account_iconand adds proper teardown logic with error handling. The docstring improves clarity.
85-86: LGTM! Update operations use the correct parameter.Both the success path and error path test cases correctly updated to use
file=account_iconinstead oflogo, aligning with the new API surface.Also applies to: 99-99
tests/e2e/accounts/account/test_async_account.py (1)
14-14: LGTM! API calls correctly updated.All account create and update operations properly use
file=account_iconinstead oflogo, consistent with the new standardized API surface.Also applies to: 53-53, 67-67, 79-79
tests/e2e/accounts/buyers/test_async_buyers.py (2)
11-26: LGTM! Fixture properly updated.The fixture correctly updates the API call to use
file=account_icon, adds proper teardown logic with error handling, and includes a descriptive docstring.
86-87: LGTM! Update operations use the correct parameter.Both success and error path test cases correctly updated to use
file=account_icon, aligning with the new API surface.Also applies to: 100-100
tests/e2e/accounts/buyers/test_sync_buyers.py (2)
11-24: LGTM! Fixture properly updated.The fixture correctly updates the API call to use
file=account_icon, adds proper teardown logic with error handling, and includes a descriptive docstring. This mirrors the async implementation appropriately.
83-84: LGTM! Update operations use the correct parameter.Both success and error path test cases correctly updated to use
file=account_icon, consistent with the new API surface and the async version.Also applies to: 96-96
tests/unit/resources/accounts/test_account.py (1)
90-90: LGTM! Test parameters updated consistently.All account create and update test calls correctly updated to use the
fileparameter instead oflogo, aligning with the new API surface across both sync and async operations.Also applies to: 114-114, 138-138, 162-162
mpt_api_client/resources/catalog/products.py (3)
12-13: LGTM! Upload configuration properly defined.The imports of
AsyncProductMixinandProductMixinalong with the upload configuration keys (_upload_file_key = "icon"and_upload_data_key = "product") correctly establish the foundation for the standardized file upload API.Also applies to: 55-56
59-67: LGTM! Service class properly refactored.The
ProductsServiceclass inheritance correctly updated to useProductMixin[Product], which provides the standardized create and update methods with file upload support. This clean mixin composition eliminates the need for explicit method overrides.
114-121: LGTM! Async service class properly refactored.The
AsyncProductsServiceclass inheritance correctly updated to useAsyncProductMixin[Product], mirroring the synchronous implementation and providing async create/update with file upload support.tests/e2e/accounts/account/test_sync_account.py (1)
9-14: LGTM!The fixture correctly updates from
logo=account_icontofile=account_icon, aligning with the new generic file-based mixin approach. The added docstring improves clarity.tests/unit/resources/catalog/test_mixins.py (2)
510-537: LGTM!The test properly validates the sync update with file functionality, checking multipart content, headers, and response mapping.
565-594: LGTM!The async update file test mirrors the sync version appropriately with proper async/await usage.
tests/unit/resources/accounts/test_mixins.py (3)
115-156: LGTM!The new test service classes are well-structured and follow the existing patterns. They properly define
_upload_file_keyand_upload_data_keyattributes required by the mixins.
843-880: LGTM!The create file service tests properly cover both file upload and no-file scenarios using
tmp_pathfixture for file handling.
923-966: LGTM!The update file service tests are well-structured, covering both with-file and no-file cases for both sync and async variants.
mpt_api_client/resources/accounts/buyers.py (2)
28-38: LGTM!The
BuyersServiceConfigproperly defines the upload keys, andBuyersServicecorrectly uses the new compositeBuyerMixin[Buyer]which encapsulatesCreateFileMixin,UpdateFileMixin,ActivatableMixin,EnablableMixin, andValidateMixin.
61-67: LGTM!The async service mirrors the sync version correctly with
AsyncBuyerMixin[Buyer].mpt_api_client/resources/catalog/mixins.py (2)
159-164: LGTM!The
AsyncProductMixincorrectly composesAsyncCreateFileMixin,AsyncUpdateFileMixin, andAsyncPublishableMixinfor product-related async operations.
232-237: LGTM!The sync
ProductMixinproperly mirrors the async version, composing the appropriate mixins for product operations.mpt_api_client/resources/accounts/mixins.py (2)
209-234: LGTM!The composite mixins (
AccountMixin,BuyerMixin,LicenseeMixin) cleanly encapsulate the required functionality for each domain entity, reducing boilerplate in service classes.
447-471: LGTM!The async composite mixins properly mirror their sync counterparts.
mpt_api_client/resources/accounts/licensees.py (2)
21-39: LGTM!The
LicenseesServicecorrectly uses the newLicenseeMixin[Licensee]composite mixin with properly configured upload keys inLicenseesServiceConfig.
42-50: LGTM!The async service mirrors the sync version correctly with
AsyncLicenseeMixin[Licensee].tests/unit/http/test_mixins.py (4)
27-44: LGTM!The new
DummyFileOperationsServiceandDummyAsyncFileOperationsServiceclasses properly use theFilesOperationsMixinandAsyncFilesOperationsMixinrespectively, with updated endpoints reflecting the file-ops semantics.
1071-1114: LGTM!The file create tests (both sync and async, with and without resource data) properly validate the multipart request structure and response handling.
1117-1160: LGTM!Good edge case coverage - testing the download behavior when content_type is not provided, verifying that
MPTErroris raised with an appropriate message.
1163-1206: LGTM!The download with explicit Accept header tests properly verify that the accept parameter is correctly passed through to the request.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
mpt_api_client/resources/catalog/mixins.py (1)
115-148: Duplicate mixin implementations across modules.
AsyncUpdateFileMixinandUpdateFileMixin(lines 196-229) are duplicated inmpt_api_client/resources/accounts/mixins.py. The same applies toCreateFileMixinandAsyncCreateFileMixin.Consider consolidating all file-upload mixins in a central location such as
mpt_api_client/http/mixins.pyand importing them where needed to eliminate duplication and reduce maintenance burden.mpt_api_client/resources/accounts/mixins.py (1)
137-166: DuplicateCreateFileMixinacross multiple modules.This mixin is duplicated in
mpt_api_client/resources/catalog/mixins.py(lines 167-193). Consider consolidating all file-upload mixins inmpt_api_client/http/mixins.pyand importing them where needed to eliminate code duplication.
🧹 Nitpick comments (5)
tests/e2e/accounts/account/test_sync_account.py (1)
78-78: Remove unusedaccount_factoryparameter.The
account_factoryparameter is not used in this test function.Apply this diff:
-def test_account_enable(mpt_ops, account_factory, created_account): +def test_account_enable(mpt_ops, created_account):tests/e2e/accounts/account/test_async_account.py (1)
83-83: Remove unusedaccount_factoryparameter.The
account_factoryparameter is not used in this test function.Apply this diff:
-async def test_account_enable(async_mpt_ops, account_factory, async_created_account): +async def test_account_enable(async_mpt_ops, async_created_account):mpt_api_client/resources/accounts/mixins.py (1)
140-140: Remove unusednoqadirectives.The
# noqa: WPS110directives on lines 140, 176, 378, and 414 are no longer needed and should be removed.Apply this diff:
- def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model: # noqa: WPS110 + def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model:- file: FileTypes | None = None, # noqa: WPS110 + file: FileTypes | None = None,Apply the same fix to lines 378 and 414.
Also applies to: 176-176, 378-378, 414-414
tests/unit/resources/catalog/test_mixins.py (1)
58-77: Remove unused noqa directives.The
WPS215noqa directives are unused according to static analysis. Modern linters may not recognize this code.Apply this diff:
-class DummyUpdateFileService( # noqa: WPS215 +class DummyUpdateFileService( UpdateFileMixin[DummyModel], Service[DummyModel], ):-class DummyAsyncUpdateFileService( # noqa: WPS215 +class DummyAsyncUpdateFileService( AsyncUpdateFileMixin[DummyModel], AsyncService[DummyModel], ):mpt_api_client/resources/accounts/licensees.py (1)
27-28: Upload config keys look consistent but could use a brief commentDefining
_upload_file_key = "logo"and_upload_data_key = "licensee"matches the expected multipart field names for this resource and fits the newCreateFileMixin/UpdateFileMixincontract. If this pattern will be reused across resources, consider a short docstring or comment onLicenseesServiceConfigclarifying that these control the multipart form keys for file and payload to aid future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
mpt_api_client/http/mixins.py(0 hunks)mpt_api_client/resources/accounts/account.py(3 hunks)mpt_api_client/resources/accounts/buyers.py(3 hunks)mpt_api_client/resources/accounts/licensees.py(2 hunks)mpt_api_client/resources/accounts/mixins.py(3 hunks)mpt_api_client/resources/catalog/mixins.py(4 hunks)mpt_api_client/resources/catalog/products.py(3 hunks)seed/catalog/product.py(1 hunks)setup.cfg(1 hunks)tests/e2e/accounts/account/test_async_account.py(5 hunks)tests/e2e/accounts/account/test_sync_account.py(3 hunks)tests/e2e/accounts/buyers/test_async_buyers.py(5 hunks)tests/e2e/accounts/buyers/test_sync_buyers.py(5 hunks)tests/e2e/accounts/licensees/test_async_licensees.py(5 hunks)tests/e2e/accounts/licensees/test_sync_licensees.py(3 hunks)tests/e2e/catalog/product/test_async_product.py(2 hunks)tests/e2e/catalog/product/test_sync_product.py(2 hunks)tests/unit/http/test_mixins.py(51 hunks)tests/unit/resources/accounts/test_account.py(4 hunks)tests/unit/resources/accounts/test_buyers.py(4 hunks)tests/unit/resources/accounts/test_licensees.py(4 hunks)tests/unit/resources/accounts/test_mixins.py(4 hunks)tests/unit/resources/catalog/test_mixins.py(4 hunks)tests/unit/resources/catalog/test_products.py(4 hunks)
💤 Files with no reviewable changes (1)
- mpt_api_client/http/mixins.py
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/unit/resources/catalog/test_products.py
- tests/unit/resources/accounts/test_buyers.py
- tests/e2e/catalog/product/test_sync_product.py
- setup.cfg
- tests/e2e/accounts/licensees/test_sync_licensees.py
🧰 Additional context used
🧬 Code graph analysis (15)
tests/unit/resources/accounts/test_licensees.py (1)
mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)
tests/unit/resources/accounts/test_account.py (1)
mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)
mpt_api_client/resources/accounts/account.py (2)
mpt_api_client/resources/accounts/mixins.py (2)
AccountMixin(209-216)AsyncAccountMixin(447-454)mpt_api_client/resources/notifications/accounts.py (1)
AccountsService(22-23)
tests/e2e/accounts/licensees/test_async_licensees.py (2)
mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)tests/e2e/accounts/conftest.py (1)
account_icon(12-13)
mpt_api_client/resources/catalog/products.py (1)
mpt_api_client/resources/catalog/mixins.py (2)
AsyncProductMixin(159-164)ProductMixin(232-237)
tests/unit/http/test_mixins.py (2)
mpt_api_client/http/mixins.py (8)
AsyncFilesOperationsMixin(184-214)FilesOperationsMixin(83-112)create(18-26)create(86-112)create(118-126)create(187-214)download(61-80)download(162-181)mpt_api_client/models/file_model.py (1)
file_contents(35-45)
mpt_api_client/resources/catalog/mixins.py (1)
mpt_api_client/resources/accounts/mixins.py (4)
AsyncUpdateFileMixin(407-444)update(172-206)update(410-444)UpdateFileMixin(169-206)
tests/unit/resources/accounts/test_mixins.py (1)
mpt_api_client/resources/catalog/mixins.py (8)
AsyncCreateFileMixin(86-112)AsyncUpdateFileMixin(115-148)CreateFileMixin(167-193)UpdateFileMixin(196-229)create(89-112)create(170-193)update(118-148)update(199-229)
mpt_api_client/resources/accounts/buyers.py (1)
mpt_api_client/resources/accounts/mixins.py (2)
AsyncBuyerMixin(457-464)BuyerMixin(219-226)
tests/unit/resources/catalog/test_mixins.py (2)
mpt_api_client/resources/accounts/mixins.py (4)
AsyncUpdateFileMixin(407-444)UpdateFileMixin(169-206)update(172-206)update(410-444)mpt_api_client/resources/catalog/mixins.py (5)
AsyncUpdateFileMixin(115-148)PublishableMixin(12-46)UpdateFileMixin(196-229)update(118-148)update(199-229)
tests/e2e/accounts/account/test_sync_account.py (2)
tests/e2e/accounts/conftest.py (2)
account_icon(12-13)account_id(22-23)mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)
tests/e2e/accounts/buyers/test_sync_buyers.py (1)
mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)
tests/e2e/catalog/product/test_async_product.py (1)
mpt_api_client/resources/catalog/mixins.py (2)
create(89-112)create(170-193)
mpt_api_client/resources/accounts/licensees.py (2)
mpt_api_client/resources/accounts/accounts.py (2)
accounts(31-33)accounts(93-95)mpt_api_client/resources/accounts/mixins.py (2)
AsyncLicenseeMixin(467-472)LicenseeMixin(229-234)
tests/e2e/accounts/account/test_async_account.py (3)
mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)tests/e2e/accounts/conftest.py (2)
account_icon(12-13)account_id(22-23)mpt_api_client/models/model.py (1)
id(119-121)
🪛 Ruff (0.14.5)
tests/unit/resources/catalog/test_mixins.py
58-58: Unused noqa directive (unknown: WPS215)
Remove unused noqa directive
(RUF100)
69-69: Unused noqa directive (unknown: WPS215)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/account/test_sync_account.py
78-78: Unused function argument: account_factory
(ARG001)
mpt_api_client/resources/accounts/mixins.py
140-140: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
176-176: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
378-378: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
414-414: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/account/test_async_account.py
83-83: Unused function argument: account_factory
(ARG001)
⏰ 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 (39)
seed/catalog/product.py (1)
52-52: LGTM! Parameter name correctly updated.The change from
icon=icon_filetofile=icon_filealigns with the broader refactoring to use generic file-based upload parameters instead of domain-specific ones.tests/e2e/accounts/account/test_sync_account.py (1)
10-14: LGTM! Fixture updated to use file parameter.The fixture now correctly uses the
fileparameter instead oflogo, aligning with the new mixin interface. Removing the unusedloggerparameter also improves the fixture signature.tests/e2e/accounts/account/test_async_account.py (1)
11-14: LGTM! Async fixture updated to use file parameter.The async fixture now correctly uses the
fileparameter, maintaining consistency with the synchronous version and the new mixin interface.mpt_api_client/resources/accounts/account.py (2)
23-30: LGTM! Configuration properly updated for file uploads.The upload configuration keys are correctly defined to support the new file-based upload workflow through the consolidated AccountMixin.
33-39: LGTM! Service refactored to use consolidated AccountMixin.The inheritance structure is clean and leverages the new AccountMixin that consolidates create/update file operations with other account capabilities.
mpt_api_client/resources/catalog/mixins.py (1)
159-164: LGTM! ProductMixin properly combines file operations with publishable capabilities.The composite mixin structure is clean and aligns with the refactoring pattern used across accounts and catalog resources.
tests/unit/http/test_mixins.py (2)
27-56: LGTM! File operations test infrastructure properly refactored.The new
DummyFileOperationsServiceclasses and corresponding fixtures correctly replace the icon-based services and align with the file-based upload workflow.
1071-1114: LGTM! Comprehensive test coverage for file operations.The tests properly cover both sync and async file creation scenarios with and without resource data, validating the multipart form-data handling.
mpt_api_client/resources/accounts/mixins.py (1)
209-216: LGTM! AccountMixin consolidates account capabilities cleanly.The composite mixin properly combines file operations with account-specific capabilities (activate, enable, validate), providing a clean public interface.
mpt_api_client/resources/accounts/buyers.py (2)
22-29: LGTM! Configuration properly updated for buyer file uploads.The upload configuration keys align with the new file-based workflow and the BuyerMixin interface.
32-39: LGTM! BuyersService refactored to use consolidated BuyerMixin.The service now leverages the consolidated BuyerMixin, maintaining a clean separation of concerns while supporting file uploads.
tests/unit/resources/accounts/test_licensees.py (2)
34-55: LGTM! Licensee tests updated to use file parameter.The test correctly uses the
fileparameter instead of the legacyicon/logoparameter, aligning with the new mixin interface.
58-79: LGTM! Update test properly uses file parameter.The update test is consistent with the create test and correctly uses the
fileparameter.tests/e2e/accounts/licensees/test_async_licensees.py (2)
11-24: LGTM! Well-structured fixture with proper teardown.The addition of the docstring and teardown logic improves test reliability. The parameter change from
logo=tofile=aligns with the new file-based mixin approach.
27-126: Excellent addition of docstrings throughout.The descriptive docstrings significantly improve test readability and maintainability. All parameter updates from
logo=tofile=are consistent with the refactoring objectives.tests/e2e/catalog/product/test_async_product.py (2)
9-9: LGTM! Parameter renamed consistently.The change from
icon=tofile=aligns with the new file-based mixin approach across the codebase.
28-32: LGTM! Update call parameter renamed consistently.The change from
icon=tofile=matches the create call and maintains consistency across the API surface.tests/e2e/accounts/buyers/test_async_buyers.py (2)
11-27: LGTM! Well-structured fixture with proper teardown.The addition of the docstring and teardown logic improves test reliability. The parameter change from
logo=tofile=aligns with the new file-based mixin approach.
30-129: Excellent addition of docstrings throughout.The descriptive docstrings significantly improve test readability. All parameter updates from
logo=tofile=are consistent with the refactoring objectives.tests/unit/resources/accounts/test_account.py (4)
90-90: LGTM! Parameter renamed for sync create.The change from
logo=tofile=aligns with the new AccountMixin-based implementation.
114-114: LGTM! Parameter renamed for sync update.Consistent with the create call and maintains API uniformity.
138-138: LGTM! Parameter renamed for async create.The async variant properly mirrors the sync implementation changes.
162-162: LGTM! Parameter renamed for async update.All four test scenarios (sync/async × create/update) now consistently use
file=parameter.tests/e2e/accounts/buyers/test_sync_buyers.py (2)
11-25: LGTM! Well-structured fixture with proper teardown.The addition of the docstring and teardown logic improves test reliability. The parameter change from
logo=tofile=aligns with the new file-based mixin approach.
28-124: Excellent addition of docstrings throughout.The descriptive docstrings significantly improve test readability. All parameter updates from
logo=tofile=are consistent with the refactoring objectives.mpt_api_client/resources/catalog/products.py (4)
12-13: LGTM! Replaced with consolidated mixins.The switch from
CreateWithIconMixin/UpdateWithIconMixintoProductMixinandAsyncProductMixinconsolidates file upload behavior into cleaner composite mixins.
55-56: LGTM! Upload configuration keys properly defined.The
_upload_file_key = "icon"and_upload_data_key = "product"configuration aligns with the new file-based upload pattern used across the codebase.
59-66: LGTM! Simplified class hierarchy.The inheritance from
ProductMixin[Product]replaces multiple icon-specific mixins, improving maintainability while preserving functionality.
114-121: LGTM! Async variant properly updated.The
AsyncProductMixin[Product]inheritance mirrors the sync version, ensuring consistent behavior across async operations.tests/unit/resources/catalog/test_mixins.py (3)
14-17: LGTM! New mixins added to imports.The
UpdateFileMixinandAsyncUpdateFileMixinimports support the new test services added below.
122-129: LGTM! Fixtures properly configured.The new fixtures provide clean test isolation for update-file service testing.
510-619: LGTM! Comprehensive test coverage for update-file functionality.The tests cover both sync and async variants, with and without file uploads. The multipart boundary assertions ensure correct request formatting.
tests/unit/resources/accounts/test_mixins.py (4)
10-20: LGTM! File-based mixins added to imports.The
CreateFileMixin,UpdateFileMixin, and their async counterparts support the new test coverage below.
115-156: LGTM! Well-structured test services for file operations.The four test services provide comprehensive coverage for both create and update operations in sync and async modes, with proper configuration of upload keys.
209-226: LGTM! Fixtures properly configured.The fixtures provide clean test isolation for all file-based service operations.
843-1014: LGTM! Comprehensive test coverage for file operations.The tests cover all scenarios: sync/async × create/update × with-file/without-file (8 tests total). This ensures the file-based mixins work correctly across all use cases.
mpt_api_client/resources/accounts/licensees.py (3)
11-14: Imports correctly updated to new licensee mixinsSwitch to
LicenseeMixin/AsyncLicenseeMixinaligns this service with the new shared account mixin design and keeps licensee-specific behavior centralized. No issues here.
31-38: Service base class change cleanly consolidates licensee behaviorHaving
LicenseesServiceinherit fromLicenseeMixin[Licensee]instead of separate create/update/enablable mixins reduces duplication and keeps the licensee-specific create/update/enable/disable logic behind a single, well-named mixin. MRO ordering (resource mixin first, then HTTP mixins, thenServiceand config) remains sensible.
42-49: Async service base classes now mirror sync service pattern
AsyncLicenseesServicenow usingAsyncLicenseeMixin[Licensee]mirrors the sync service and keeps async create/update/enable behavior in one place. The inheritance order is consistent with the sync variant and with the rest of the client. Looks good.
robcsegal
force-pushed
the
MPT-15606-update-implementation-of-create-and-update-mixin-with-file
branch
from
668ba58 to
17cf8a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
mpt_api_client/resources/catalog/mixins.py (1)
1-1: File-update/product mixins look correct but duplicate existing implementationsThe new
AsyncUpdateFileMixin/UpdateFileMixinandAsyncProductMixin/ProductMixincorrectly:
- Build the update URL as
{path}/{resource_id}.- Always send
resource_datavia multipart (usingjson_file_keyandforce_multipart=True).- Mirror the create mixin behavior for optional
file.However, these file mixins are essentially copies of the ones in
mpt_api_client/resources/accounts/mixins.py(and earlier feedback also mentioned duplication vs HTTP-level mixins). Centralizing the file upload behavior in a single module (e.g.,http.mixins) and importing it here and in accounts would reduce maintenance overhead and keep the multipart logic in one place.Also applies to: 115-148, 159-165, 196-229, 232-237
mpt_api_client/resources/accounts/mixins.py (1)
1-4: File mixins are correct but contain redundant code and noisynoqamarkersFunctionally, the new
CreateFileMixin/UpdateFileMixinand their async counterparts look good:
- They always send
resource_datavia multipart usingjson_file_keyandforce_multipart=True.- They attach the optional
fileunder_upload_file_key.- Update URLs are built as
{self.path}/{resource_id}viaurljoin, matching how tests stub the endpoints.Two follow-ups to consider:
- Remove unused
# noqa: WPS110to satisfy Ruff
Ruff reportsRUF100for the# noqa: WPS110comments on thefileparameters (lines 140, 176, 378, 414 in this file). They’re not suppressing any active rule and just introduce noise. You can safely drop them:- def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model: # noqa: WPS110 + def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model: ... - file: FileTypes | None = None, # noqa: WPS110 + file: FileTypes | None = None, ... - async def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model: # noqa: WPS110 + async def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model: ... - file: FileTypes | None = None, # noqa: WPS110 + file: FileTypes | None = None,
- Avoid duplicating file mixin implementations across modules
These file mixins are effectively the same as those inmpt_api_client/resources/catalog/mixins.py(and earlier feedback already noted duplication with HTTP-level mixins). Centralizing this logic in a single shared file-upload mixin (imported here and from catalog) would make future changes to the multipart/request behavior less error-prone.#!/bin/bash # Verify Ruff warnings are cleared after removing the WPS110 noqa comments. ruff mpt_api_client/resources/accounts/mixins.pyAlso applies to: 137-207, 209-235, 375-445, 447-472
tests/e2e/accounts/account/test_async_account.py (1)
86-86: Remove unusedaccount_factoryparameter.The
account_factoryfixture is not used in this test function.tests/e2e/accounts/account/test_sync_account.py (1)
78-80: Remove unusedaccount_factoryfixture parameter.
test_account_enabledoesn’t useaccount_factory, and Ruff reports it as unused. Drop it from the signature:-def test_account_enable(mpt_ops, account_factory, created_account): +def test_account_enable(mpt_ops, created_account):This keeps the test clean and resolves the ARG001 warning.
🧹 Nitpick comments (2)
tests/e2e/accounts/user_groups/test_async_user_groups.py (1)
31-37: Stronger assertion on list results; consider also checking the limitAsserting
len(result) > 0improves this test by ensuring some data is returned. You might optionally assert that the page respects the requested limit as well:async def test_list_user_groups(async_mpt_ops): """Test listing user groups with a limit.""" limit = 10 result = await async_mpt_ops.accounts.user_groups.fetch_page(limit=limit) - assert len(result) > 0 + assert len(result) > 0 + assert len(result) <= limittests/unit/resources/catalog/test_mixins.py (1)
506-506: Remove unused# noqa: WPS210directives.Ruff reports these
noqacomments as unused (RUF100). The WPS210 rule (too many local variables) is not triggered here, so the directives are unnecessary.Apply this diff:
-def test_sync_update_file(update_file_service): # noqa: WPS210 +def test_sync_update_file(update_file_service):Also remove the same directive from lines 535, 559, and 589:
-def test_update_file_no_file(update_file_service): # noqa: WPS210 +def test_update_file_no_file(update_file_service):-async def test_async_update_file(async_update_file_service): # noqa: WPS210 +async def test_async_update_file(async_update_file_service):-async def test_async_update_file_no_file(async_update_file_service): # noqa: WPS210 +async def test_async_update_file_no_file(async_update_file_service):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
mpt_api_client/http/mixins.py(0 hunks)mpt_api_client/resources/accounts/account.py(3 hunks)mpt_api_client/resources/accounts/buyers.py(3 hunks)mpt_api_client/resources/accounts/licensees.py(2 hunks)mpt_api_client/resources/accounts/mixins.py(3 hunks)mpt_api_client/resources/catalog/mixins.py(4 hunks)mpt_api_client/resources/catalog/products.py(3 hunks)seed/catalog/product.py(1 hunks)tests/e2e/accounts/account/test_async_account.py(6 hunks)tests/e2e/accounts/account/test_sync_account.py(6 hunks)tests/e2e/accounts/api_tokens/test_async_api_tokens.py(7 hunks)tests/e2e/accounts/api_tokens/test_sync_api_tokens.py(7 hunks)tests/e2e/accounts/buyers/test_async_buyers.py(7 hunks)tests/e2e/accounts/buyers/test_sync_buyers.py(6 hunks)tests/e2e/accounts/licensees/test_async_licensees.py(7 hunks)tests/e2e/accounts/licensees/test_sync_licensees.py(3 hunks)tests/e2e/accounts/user_groups/test_async_user_groups.py(5 hunks)tests/e2e/accounts/user_groups/test_sync_user_groups.py(5 hunks)tests/e2e/catalog/product/test_async_product.py(2 hunks)tests/e2e/catalog/product/test_sync_product.py(2 hunks)tests/unit/http/test_mixins.py(46 hunks)tests/unit/resources/accounts/test_account.py(4 hunks)tests/unit/resources/accounts/test_buyers.py(4 hunks)tests/unit/resources/accounts/test_licensees.py(4 hunks)tests/unit/resources/accounts/test_mixins.py(4 hunks)tests/unit/resources/catalog/test_mixins.py(8 hunks)tests/unit/resources/catalog/test_products.py(4 hunks)
💤 Files with no reviewable changes (1)
- mpt_api_client/http/mixins.py
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/e2e/catalog/product/test_sync_product.py
- tests/unit/resources/accounts/test_licensees.py
- tests/e2e/accounts/api_tokens/test_sync_api_tokens.py
- tests/e2e/accounts/licensees/test_async_licensees.py
- tests/unit/resources/accounts/test_buyers.py
- tests/e2e/accounts/user_groups/test_sync_user_groups.py
- seed/catalog/product.py
- tests/e2e/accounts/buyers/test_sync_buyers.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:54.691Z
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.691Z
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/e2e/accounts/api_tokens/test_async_api_tokens.pytests/e2e/accounts/user_groups/test_async_user_groups.pytests/e2e/accounts/account/test_async_account.pytests/e2e/accounts/buyers/test_async_buyers.pytests/e2e/catalog/product/test_async_product.py
🧬 Code graph analysis (16)
tests/e2e/accounts/licensees/test_sync_licensees.py (3)
tests/e2e/accounts/conftest.py (1)
account_icon(12-13)mpt_api_client/models/model.py (1)
id(119-121)tests/e2e/accounts/licensees/conftest.py (1)
invalid_licensee_id(20-21)
tests/e2e/accounts/api_tokens/test_async_api_tokens.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/conftest.py (1)
async_mpt_vendor(24-27)mpt_api_client/http/mixins.py (2)
get(220-233)get(239-251)tests/e2e/accounts/api_tokens/conftest.py (3)
invalid_api_token_id(10-11)api_token_id(5-6)api_token_factory(15-28)mpt_api_client/resources/accounts/mixins.py (2)
disable(47-56)disable(279-288)
tests/unit/resources/accounts/test_account.py (1)
mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)
mpt_api_client/resources/accounts/buyers.py (1)
mpt_api_client/resources/accounts/mixins.py (2)
AsyncBuyerMixin(457-464)BuyerMixin(219-226)
tests/e2e/accounts/user_groups/test_async_user_groups.py (5)
tests/e2e/conftest.py (1)
async_mpt_ops(36-39)mpt_api_client/http/mixins.py (4)
get(220-233)get(239-251)delete(32-38)delete(132-139)tests/e2e/accounts/conftest.py (2)
user_group_id(37-38)user_group_factory(47-62)mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/accounts/user_groups/conftest.py (1)
invalid_user_group_id(5-6)
mpt_api_client/resources/catalog/products.py (1)
mpt_api_client/resources/catalog/mixins.py (2)
AsyncProductMixin(159-164)ProductMixin(232-237)
mpt_api_client/resources/catalog/mixins.py (4)
mpt_api_client/models/model.py (1)
Model(65-125)mpt_api_client/http/mixins.py (2)
update(44-55)update(145-156)mpt_api_client/http/async_client.py (1)
request(61-117)mpt_api_client/http/client.py (1)
request(73-129)
tests/unit/resources/catalog/test_mixins.py (4)
mpt_api_client/resources/accounts/mixins.py (4)
AsyncUpdateFileMixin(407-444)UpdateFileMixin(169-206)update(172-206)update(410-444)mpt_api_client/resources/catalog/mixins.py (4)
AsyncUpdateFileMixin(115-148)UpdateFileMixin(196-229)update(118-148)update(199-229)mpt_api_client/http/mixins.py (2)
update(44-55)update(145-156)mpt_api_client/http/client.py (1)
request(73-129)
mpt_api_client/resources/accounts/account.py (2)
mpt_api_client/resources/accounts/mixins.py (2)
AccountMixin(209-216)AsyncAccountMixin(447-454)mpt_api_client/resources/notifications/accounts.py (1)
AccountsService(22-23)
tests/e2e/accounts/account/test_async_account.py (6)
tests/e2e/accounts/account/conftest.py (1)
account_factory(10-27)mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)mpt_api_client/resources/catalog/mixins.py (4)
create(89-112)create(170-193)update(118-148)update(199-229)mpt_api_client/http/mixins.py (6)
create(18-26)create(86-112)create(118-126)create(187-214)update(44-55)update(145-156)tests/e2e/accounts/conftest.py (2)
account_icon(12-13)account_id(22-23)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/accounts/buyers/test_async_buyers.py (1)
mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)
tests/e2e/catalog/product/test_async_product.py (2)
tests/e2e/conftest.py (1)
logo_fd(111-113)mpt_api_client/resources/catalog/mixins.py (2)
create(89-112)create(170-193)
tests/e2e/accounts/account/test_sync_account.py (3)
tests/e2e/accounts/conftest.py (2)
account_icon(12-13)account_id(22-23)mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)mpt_api_client/http/mixins.py (6)
create(18-26)create(86-112)create(118-126)create(187-214)update(44-55)update(145-156)
mpt_api_client/resources/accounts/mixins.py (1)
mpt_api_client/resources/catalog/mixins.py (6)
CreateFileMixin(167-193)create(89-112)create(170-193)UpdateFileMixin(196-229)update(118-148)update(199-229)
tests/unit/resources/catalog/test_products.py (1)
mpt_api_client/resources/catalog/mixins.py (4)
create(89-112)create(170-193)update(118-148)update(199-229)
mpt_api_client/resources/accounts/licensees.py (2)
mpt_api_client/resources/accounts/mixins.py (2)
AsyncLicenseeMixin(467-472)LicenseeMixin(229-234)mpt_api_client/http/mixins.py (3)
GetMixin(217-233)DeleteMixin(29-38)CollectionMixin(325-391)
🪛 Ruff (0.14.6)
tests/unit/resources/catalog/test_mixins.py
506-506: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
535-535: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
559-559: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
589-589: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
tests/unit/resources/accounts/test_mixins.py
828-828: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
869-869: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
909-909: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
954-954: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/account/test_async_account.py
86-86: Unused function argument: account_factory
(ARG001)
tests/e2e/accounts/account/test_sync_account.py
78-78: Unused function argument: account_factory
(ARG001)
mpt_api_client/resources/accounts/mixins.py
140-140: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
176-176: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
378-378: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
414-414: Unused noqa directive (unknown: WPS110)
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 (44)
tests/e2e/accounts/user_groups/test_async_user_groups.py (7)
9-22: Fixture docstring and teardown remain clear and safeThe added docstring accurately describes the fixture, and the create/teardown flow is unchanged and appropriate for the e2e setup. No issues here.
25-29: Docstring improves intent oftest_get_user_group_by_idThe new docstring makes the test purpose explicit without changing behavior. Looks good as-is.
40-44: 404 not-found get scenario is clearly documentedThe docstring now captures the 404 expectation, aligning well with the
pytest.raises(MPTAPIError, match=r"404 Not Found")check. No further changes needed.
46-57: Filter test docstring nicely explains the multi-step queryThe new docstring clarifies that this test covers filtering with specific criteria (RQL filters + select), which matches the implementation.
70-73: Delete 404 case is well-describedThe added docstring matches the behavior asserted via
pytest.raises(MPTAPIError, match=r"404 Not Found"). This keeps the negative path well documented.
76-85: Update happy-path test now self-documents its intentThe docstring clearly states that this is the standard update flow; the test remains straightforward and appropriate for e2e coverage.
87-96: Update 404 case docstring aligns with the error expectationThe docstring emphasizes the invalid-ID scenario and expected 404, consistent with the
pytest.raisesassertion. All good.tests/e2e/accounts/licensees/test_sync_licensees.py (1)
13-15:file=parameter usage aligns with new file-upload mixinsUsing
file=account_iconforcreateandupdatematches the newCreateFileMixin/UpdateFileMixinsignatures and keeps the tests in sync with the refactored API surface. No issues from the test side.Also applies to: 75-77, 86-88
mpt_api_client/resources/accounts/buyers.py (1)
12-15: Buyer services correctly migrated to shared Buyer mixinsSwitching
BuyersService/AsyncBuyersServicetoBuyerMixin/AsyncBuyerMixinplus_upload_file_key = "logo"and_upload_data_key = "buyer"cleanly wires them into the new file-based create/update behavior while preserving collection/get/delete APIs. MRO still exposes all needed methods (create, update, activate, enable, validate).Also applies to: 28-30, 32-38, 61-68
tests/e2e/accounts/api_tokens/test_async_api_tokens.py (1)
10-22: Improved test clarity and more robust update scenarioThe added docstrings make each scenario and expected 404 behavior explicit, and reusing
created_api_token.idintest_update_api_tokenkeeps the update test isolated from seeded data. The async fixture usage remains compatible with the current pytest-asyncio setup.Also applies to: 31-33, 40-42, 46-48, 70-72, 76-78, 90-91, 105-107, 111-113, 120-122
mpt_api_client/resources/accounts/licensees.py (1)
11-14: Licensees services correctly wired to Licensee mixins and upload configAdopting
LicenseeMixin/AsyncLicenseeMixinand configuring_upload_file_key = "logo"and_upload_data_key = "licensee"cleanly migrates create/update/enable behavior to the new shared file mixins while preserving get/delete/collection behavior.Also applies to: 24-28, 31-38, 42-49
mpt_api_client/resources/accounts/account.py (1)
13-16: Accounts services cleanly migrated to Account mixins with file-upload configUsing
AccountMixin/AsyncAccountMixintogether with_upload_file_key = "logo"and_upload_data_key = "account"correctly routes create/update (and activation/enabling/validation) through the new file-based mixins. Theusers()helper remains intact for both sync and async services.Also applies to: 26-31, 33-39, 49-55
tests/unit/resources/accounts/test_account.py (1)
80-93: Account tests correctly exercise newfile=upload behaviorUpdating the account create/update (sync and async) tests to call
create(..., file=logo_file)andupdate(..., file=logo_file)matches the new mixin signatures and still validates the expected HTTP method and URL path. This keeps unit coverage aligned with the refactored upload logic.Also applies to: 101-114, 122-135, 143-156
tests/e2e/catalog/product/test_async_product.py (2)
9-9: LGTM!The parameter change from
icontofilecorrectly aligns with the newCreateFileMixinsignature which uses a genericfileparameter, while the service configuration internally maps this to the"icon"key via_upload_file_key.
33-33: LGTM!The
file=logo_fdparameter correctly matches the newUpdateFileMixin.update()signature.tests/unit/resources/catalog/test_products.py (4)
151-151: LGTM!The
file=icon_fileparameter correctly aligns with the newCreateFileMixinAPI.
171-171: LGTM!Async create test properly uses the new
fileparameter.
191-195: LGTM!Sync update test correctly uses the new
file=icon_fileparameter with theUpdateFileMixin.update()signature.
215-219: LGTM!Async update test properly uses the new
fileparameter.tests/e2e/accounts/account/test_async_account.py (4)
14-14: LGTM!The
file=account_iconparameter correctly aligns with the newAsyncAccountMixin.create()signature.
56-56: LGTM!Update call correctly uses the new
fileparameter.
70-70: LGTM!Update call with invalid data correctly uses the new
fileparameter.
82-82: LGTM!Update call for not-found test correctly uses the new
fileparameter.tests/e2e/accounts/buyers/test_async_buyers.py (3)
18-18: LGTM!The
file=account_iconparameter correctly aligns with the newAsyncBuyerMixin.create()signature.
87-87: LGTM!Update call correctly uses the new
fileparameter.
101-101: LGTM!Update call for not-found test correctly uses the new
fileparameter.tests/unit/resources/catalog/test_mixins.py (6)
58-77: LGTM!New
DummyUpdateFileServiceandDummyAsyncUpdateFileServicecorrectly composeUpdateFileMixin/AsyncUpdateFileMixinwith the base service classes and define the required upload configuration keys.
122-129: LGTM!Fixtures properly instantiate the new update file services with the appropriate HTTP clients.
506-532: LGTM!Test correctly verifies that
UpdateFileMixin.update()sends a multipart/form-data request with the file part when a file is provided.
535-556: LGTM!Test correctly verifies the no-file scenario, ensuring the file part is absent but multipart encoding is still used.
559-586: LGTM!Async update test correctly verifies the file upload behavior.
589-611: LGTM!Async no-file update test correctly verifies the behavior.
tests/e2e/accounts/account/test_sync_account.py (3)
9-21: Fixture now correctly exercises file-based account creation.Using
file=account_iconincreated_accountmatches the new file-upload mixin contract, and teardown behavior remains safe (errors are caught and logged). No changes needed.
51-76: Update tests correctly migrated tofile=API and keep good coverage.All update paths (happy path, invalid data -> 400, not found -> 404) now use
update(..., file=account_icon)consistently and the expectations still match the new behavior. Looks good.
24-27: Docstrings improve test intent without changing behavior.The added/updated docstrings accurately describe the asserted behavior for not-found, listing with limit, enable/disable, and RQL filter tests. No functional issues.
Also applies to: 36-37, 87-90, 99-102, 105-107
mpt_api_client/resources/catalog/products.py (1)
11-14: Product services correctly adopt file-aware mixins and config.Switching to
ProductMixin[Product]/AsyncProductMixin[Product]and defining_upload_file_key = "icon"/_upload_data_key = "product"gives products the same file-upload behavior as other resources without custom overrides. The MRO and config look sound.Also applies to: 49-57, 59-66, 114-121
tests/unit/http/test_mixins.py (6)
11-18: Dummy file-ops services and fixtures are correctly wired.
DummyFileOperationsService/DummyAsyncFileOperationsServicecombiningFilesOperationsMixin/AsyncFilesOperationsMixinwithService/AsyncServiceand the/public/v1/dummy/file-ops/endpoint give a clean harness for exercising the HTTP file-ops mixins. Config (_model_class,_collection_key) is consistent.Also applies to: 26-44, 46-56
58-70: Create tests thoroughly validate multipart behavior for media/file-ops.The async/sync media tests and the
DummyFileOperationsServicetests verify:
- JSON is sent as a separate multipart part (
_attachment_dataor resource-specific key) only whenresource_datais truthy.- File parts are present with the expected field name, filename, and content type.
This closely matches thecreate(..., files, data_key="_attachment_data")behavior inmpt_api_client/http/mixins.py. Based on relevant_code_snippets, this aligns with the intended contract.Also applies to: 170-216, 371-425
227-295: Async and sync download tests correctly cover headers and payloads.The new download tests assert:
- A metadata request with
Accept: application/json.- A follow-up file request with
Accept: image/jpg.- Returned
file_contents,content_type, andfilenameare propagated as expected.
Using two separaterespx.getmocks per ID makes the behavior explicit and guards against regressions in header handling.Also applies to: 298-369
72-168: Docstrings and layout updates make mixin tests easier to understand.The added docstrings and minor spacing tweaks across the CRUD, queryable, and collection mixin tests clarify Arrange/Act/Assert phases and expected behavior without changing semantics. This should help future contributors quickly see what each mixin guarantees.
Also applies to: 435-448, 451-480, 492-500, 512-521, 555-612, 614-741, 767-779, 781-842, 847-872, 875-939, 941-971, 973-1010
1012-1097: Helper test services for modifiable/managed mixins look correct.
_ModifiableResourceService,_AsyncModifiableResourceService,_ManagedService, and_AsyncManagedServiceare minimal shells over the respective mixins, and the parametrized tests just assert required methods exist and are callable. This is a clean way to lock down the mixin APIs.
1098-1137: File-ops HTTP mixin tests validate_attachment_datapacking.The sync/async
dummy_file_operations_servicetests assert that:
- The request body includes a JSON part named
_attachment_datawith the serializedresource_data.- The service still returns a
DummyModelbuilt from the response.
This is exactly the behavior described forcreate(..., resource_data, files, data_key="_attachment_data")inmpt_api_client/http/mixins.py. Based on relevant_code_snippets, the tests are well aligned with the mixin’s responsibilities.tests/unit/resources/accounts/test_mixins.py (2)
5-21: Create/Update file mixin services are configured correctly.
DummyCreateFileService/DummyUpdateFileServiceand their async counterparts combine the newCreateFileMixin/UpdateFileMixin(and async variants) withService/AsyncService, and set_upload_file_key = "file"/_upload_data_key = "resource". This matches the generic file mixin implementations inmpt_api_client/resources/accounts/mixins.pyand gives you a realistic dummy surface for testing. Based on relevant_code_snippets, this wiring looks spot on.Also applies to: 115-157, 209-227
828-907: Create/Update file tests cover sync/async and file/no-file variants well.The eight tests for
create_file_service/update_file_serviceand their async variants:
- Exercise both with-file and no-file calls.
- Verify the correct endpoint is hit and that responses are mapped back into
DummyModel.- Keep the Arrange/Act/Assert structure clear.
They provide good confidence that the newCreateFileMixin/UpdateFileMixinbehavior is exercised end-to-end.Also applies to: 909-997
robcsegal
force-pushed
the
MPT-15606-update-implementation-of-create-and-update-mixin-with-file
branch
from
94024a0 to
17ab000
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
mpt_api_client/resources/catalog/mixins.py (1)
115-148: Duplicate mixin implementations across modules.
AsyncUpdateFileMixinandUpdateFileMixin(lines 196-229) are duplicated inmpt_api_client/resources/accounts/mixins.py. Consider consolidating all file-upload mixins in a single location (e.g.,mpt_api_client/http/mixins.py) and importing them where needed to avoid maintenance issues.tests/e2e/accounts/account/test_async_account.py (1)
86-86: Remove unusedaccount_factoryparameter.The
account_factoryfixture is not used in this test function.Apply this diff:
-async def test_account_enable(async_mpt_ops, account_factory, async_created_account): +async def test_account_enable(async_mpt_ops, async_created_account):tests/e2e/accounts/account/test_sync_account.py (1)
78-78: Remove unusedaccount_factoryparameter.The
account_factoryfixture is not used in this test function.Apply this diff:
-def test_account_enable(mpt_ops, account_factory, created_account): +def test_account_enable(mpt_ops, created_account):tests/unit/resources/accounts/test_mixins.py (1)
828-828: Remove unusednoqadirectives.Ruff reports these
# noqa: WPS210comments as unused (RUF100). The local variable count is now within limits, so these directives can be removed.Apply this diff:
-def test_create_file_service(create_file_service, tmp_path): # noqa: WPS210 +def test_create_file_service(create_file_service, tmp_path):Also applies to lines 869, 909, and 954.
mpt_api_client/resources/accounts/mixins.py (1)
140-140: Remove unusednoqadirectives.Ruff reports the
# noqa: WPS110directives at lines 140, 176, 378, and 414 as unused (RUF100). These should be removed.Apply this diff:
- def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model: # noqa: WPS110 + def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model: def update( self, resource_id: str, resource_data: ResourceData, - file: FileTypes | None = None, # noqa: WPS110 + file: FileTypes | None = None, ) -> Model: - async def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model: # noqa: WPS110 + async def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model: async def update( self, resource_id: str, resource_data: ResourceData, - file: FileTypes | None = None, # noqa: WPS110 + file: FileTypes | None = None, ) -> Model:
🧹 Nitpick comments (1)
tests/e2e/accounts/api_tokens/test_sync_api_tokens.py (1)
23-23: Consider adding docstrings to remaining tests for consistency.While the added docstrings improve documentation, several tests remain undocumented:
test_get_api_token_by_id,test_create_api_token,test_delete_api_token, andtest_api_token_disable. Adding docstrings to these would create consistent documentation across the test suite.Also applies to: 58-58, 64-64, 91-91
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
mpt_api_client/http/mixins.py(0 hunks)mpt_api_client/resources/accounts/account.py(3 hunks)mpt_api_client/resources/accounts/buyers.py(3 hunks)mpt_api_client/resources/accounts/licensees.py(2 hunks)mpt_api_client/resources/accounts/mixins.py(3 hunks)mpt_api_client/resources/catalog/mixins.py(4 hunks)mpt_api_client/resources/catalog/products.py(3 hunks)seed/catalog/product.py(1 hunks)tests/e2e/accounts/account/test_async_account.py(6 hunks)tests/e2e/accounts/account/test_sync_account.py(6 hunks)tests/e2e/accounts/api_tokens/test_async_api_tokens.py(7 hunks)tests/e2e/accounts/api_tokens/test_sync_api_tokens.py(7 hunks)tests/e2e/accounts/buyers/test_async_buyers.py(7 hunks)tests/e2e/accounts/buyers/test_sync_buyers.py(6 hunks)tests/e2e/accounts/licensees/test_async_licensees.py(7 hunks)tests/e2e/accounts/licensees/test_sync_licensees.py(3 hunks)tests/e2e/accounts/user_groups/test_async_user_groups.py(5 hunks)tests/e2e/accounts/user_groups/test_sync_user_groups.py(5 hunks)tests/e2e/catalog/product/test_async_product.py(2 hunks)tests/e2e/catalog/product/test_sync_product.py(2 hunks)tests/unit/http/test_mixins.py(46 hunks)tests/unit/resources/accounts/test_account.py(4 hunks)tests/unit/resources/accounts/test_buyers.py(4 hunks)tests/unit/resources/accounts/test_licensees.py(4 hunks)tests/unit/resources/accounts/test_mixins.py(4 hunks)tests/unit/resources/catalog/test_mixins.py(8 hunks)tests/unit/resources/catalog/test_products.py(4 hunks)
💤 Files with no reviewable changes (1)
- mpt_api_client/http/mixins.py
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/e2e/catalog/product/test_async_product.py
- tests/unit/resources/accounts/test_account.py
- tests/e2e/accounts/licensees/test_sync_licensees.py
- tests/e2e/accounts/user_groups/test_sync_user_groups.py
- tests/e2e/accounts/buyers/test_async_buyers.py
- tests/e2e/accounts/user_groups/test_async_user_groups.py
- seed/catalog/product.py
- tests/e2e/catalog/product/test_sync_product.py
- tests/e2e/accounts/api_tokens/test_async_api_tokens.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:54.691Z
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.691Z
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/e2e/accounts/licensees/test_async_licensees.pytests/e2e/accounts/account/test_async_account.py
🧬 Code graph analysis (14)
tests/unit/resources/accounts/test_buyers.py (1)
mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)
mpt_api_client/resources/catalog/mixins.py (1)
mpt_api_client/resources/accounts/mixins.py (4)
AsyncUpdateFileMixin(407-444)update(172-206)update(410-444)UpdateFileMixin(169-206)
tests/e2e/accounts/licensees/test_async_licensees.py (2)
mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)tests/e2e/accounts/conftest.py (1)
account_icon(12-13)
tests/unit/http/test_mixins.py (4)
mpt_api_client/http/mixins.py (8)
AsyncFilesOperationsMixin(184-214)FilesOperationsMixin(83-112)download(61-80)download(162-181)create(18-26)create(86-112)create(118-126)create(187-214)mpt_api_client/http/types.py (2)
Response(27-42)json(40-42)mpt_api_client/models/file_model.py (3)
file_contents(35-45)content_type(48-55)filename(13-32)mpt_api_client/resources/catalog/mixins.py (2)
create(89-112)create(170-193)
mpt_api_client/resources/accounts/buyers.py (1)
mpt_api_client/resources/accounts/mixins.py (2)
AsyncBuyerMixin(457-464)BuyerMixin(219-226)
tests/unit/resources/catalog/test_products.py (1)
mpt_api_client/resources/catalog/mixins.py (4)
create(89-112)create(170-193)update(118-148)update(199-229)
mpt_api_client/resources/catalog/products.py (1)
mpt_api_client/resources/catalog/mixins.py (2)
AsyncProductMixin(159-164)ProductMixin(232-237)
mpt_api_client/resources/accounts/mixins.py (5)
mpt_api_client/resources/catalog/mixins.py (3)
CreateFileMixin(167-193)create(89-112)create(170-193)mpt_api_client/models/model.py (1)
Model(65-125)mpt_api_client/http/mixins.py (4)
create(18-26)create(86-112)create(118-126)create(187-214)mpt_api_client/http/base_service.py (1)
path(28-30)mpt_api_client/http/types.py (1)
json(40-42)
mpt_api_client/resources/accounts/account.py (1)
mpt_api_client/resources/accounts/mixins.py (2)
AccountMixin(209-216)AsyncAccountMixin(447-454)
tests/e2e/accounts/buyers/test_sync_buyers.py (3)
tests/e2e/conftest.py (1)
mpt_ops(31-32)mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/accounts/api_tokens/test_sync_api_tokens.py (4)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)mpt_api_client/http/mixins.py (4)
get(220-233)get(239-251)delete(32-38)delete(132-139)tests/e2e/accounts/api_tokens/conftest.py (3)
invalid_api_token_id(10-11)api_token_id(5-6)api_token_factory(15-28)mpt_api_client/resources/accounts/mixins.py (2)
disable(47-56)disable(279-288)
tests/e2e/accounts/account/test_sync_account.py (3)
tests/e2e/accounts/conftest.py (2)
account_icon(12-13)account_id(22-23)mpt_api_client/resources/accounts/mixins.py (2)
create(140-166)create(378-404)mpt_api_client/http/mixins.py (4)
create(18-26)create(86-112)create(118-126)create(187-214)
tests/unit/resources/catalog/test_mixins.py (2)
mpt_api_client/resources/accounts/mixins.py (4)
AsyncUpdateFileMixin(407-444)UpdateFileMixin(169-206)update(172-206)update(410-444)mpt_api_client/resources/catalog/mixins.py (4)
AsyncUpdateFileMixin(115-148)UpdateFileMixin(196-229)update(118-148)update(199-229)
tests/e2e/accounts/account/test_async_account.py (2)
mpt_api_client/resources/accounts/mixins.py (2)
create(140-166)create(378-404)tests/e2e/accounts/conftest.py (1)
account_icon(12-13)
🪛 Ruff (0.14.6)
tests/unit/resources/accounts/test_mixins.py
828-828: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
869-869: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
909-909: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
954-954: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
mpt_api_client/resources/accounts/mixins.py
140-140: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
176-176: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
378-378: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
414-414: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/account/test_sync_account.py
78-78: Unused function argument: account_factory
(ARG001)
tests/unit/resources/catalog/test_mixins.py
506-506: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
535-535: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
559-559: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
589-589: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
tests/e2e/accounts/account/test_async_account.py
86-86: Unused function argument: account_factory
(ARG001)
⏰ 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 (74)
tests/e2e/accounts/api_tokens/test_sync_api_tokens.py (10)
11-11: LGTM - Clear fixture documentation.The docstring accurately describes the fixture's purpose and improves test readability.
30-30: LGTM - Test intent documented.The docstring clearly states the test's purpose.
39-39: LGTM - Well-documented error case.The docstring clearly describes both the test scenario and expected outcome.
45-45: LGTM - Test purpose documented.The docstring appropriately describes the filtering test.
69-69: LGTM - Error case well-documented.The docstring clearly states the test scenario and expected 404 error.
75-75: LGTM - Test documented.The docstring describes the update test appropriately.
84-84: LGTM - Error scenario documented.The docstring clearly describes the invalid ID scenario and expected 404 error.
98-98: LGTM - Disable error case documented.The docstring appropriately describes the test scenario and expected outcome.
104-104: LGTM - Enable test documented.The docstring clearly states the test's purpose.
113-113: LGTM - Error case well-documented.The docstring appropriately describes the invalid ID scenario and expected error.
mpt_api_client/resources/catalog/products.py (4)
11-13: LGTM: Mixin imports updated correctly.The imports now reference the new
ProductMixinandAsyncProductMixinfrom catalog mixins, which consolidate file-upload and publish capabilities.
55-56: LGTM: Upload configuration keys added.The configuration keys
_upload_file_key = "icon"and_upload_data_key = "product"properly support the file-upload semantics introduced by the new mixins.
59-66: LGTM: Service inheritance updated to use ProductMixin.The synchronous
ProductsServicenow inherits fromProductMixin[Product], which provides create/update/publish capabilities through the new file-upload mixins.
114-121: LGTM: Async service inheritance updated to use AsyncProductMixin.The asynchronous
AsyncProductsServicenow inherits fromAsyncProductMixin[Product], maintaining consistency with the synchronous implementation.tests/unit/resources/accounts/test_licensees.py (4)
38-57: LGTM: Test updated to use file parameter.The create test now correctly passes
file=logo_fileinstead oflogo=logo_file, aligning with the new API surface.
59-78: LGTM: Update test uses file parameter.The update test correctly uses
file=logo_fileparameter, consistent with the API changes.
80-98: LGTM: Async create test updated.The async create test correctly uses the
file=logo_fileparameter.
101-119: LGTM: Async update test uses file parameter.The async update test correctly uses
file=logo_file, maintaining consistency across all test cases.tests/unit/resources/catalog/test_products.py (4)
141-158: LGTM: Product create test updated to use file parameter.The synchronous product create test now correctly uses
file=icon_fileinstead oficon=icon_file.
160-178: LGTM: Async product create test updated.The async product create test correctly uses the
file=icon_fileparameter.
180-202: LGTM: Sync product update test uses file parameter.The synchronous update test correctly passes
file=icon_fileto the update method.
204-225: LGTM: Async product update test updated.The async update test correctly uses
file=icon_file, maintaining consistency across all test cases.tests/unit/resources/accounts/test_buyers.py (4)
168-187: LGTM: Buyer create test updated to use file parameter.The test correctly uses
file=logo_fileinstead oflogo=logo_file.
189-208: LGTM: Buyer update test uses file parameter.The update test correctly passes
file=logo_fileto the update method.
210-229: LGTM: Async buyer create test updated.The async create test correctly uses the
file=logo_fileparameter.
231-250: LGTM: Async buyer update test updated.The async update test correctly uses
file=logo_file, maintaining consistency across all test cases.tests/e2e/accounts/buyers/test_sync_buyers.py (9)
9-25: LGTM: Fixture improved with docstring and error handling.The fixture now includes:
- Clear docstring documenting its purpose
- Updated API call using
file=account_icon- Better error handling in teardown
33-40: LGTM: Test documentation added.The docstring clearly describes the test's purpose.
42-46: LGTM: Test documents expected 404 behavior.The docstring appropriately describes the error case being tested.
48-60: LGTM: Filter test documented.The docstring clearly describes the RQL filtering test.
78-85: LGTM: Update test uses file parameter and includes docstring.The test correctly uses
file=account_iconand includes a clear docstring.
87-95: LGTM: Update not-found test documented and uses file parameter.The test correctly uses
file=account_iconand documents the 404 error scenario.
103-107: LGTM: Disable not-found test documented.The docstring clearly describes the expected 404 behavior.
109-116: LGTM: Enable test documented.The docstring describes the enable flow test.
118-121: LGTM: Enable not-found test documented.The docstring clearly describes the expected 404 error case.
tests/e2e/accounts/account/test_async_account.py (11)
9-22: LGTM: Async fixture improved with docstring and file parameter.The fixture correctly uses
file=account_iconand includes a clear docstring and error handling in teardown.
24-28: LGTM: Test documents expected 404 behavior.The docstring appropriately describes the error case being tested.
36-43: LGTM: List test documented.The docstring clearly describes the test's purpose.
51-60: LGTM: Update test uses file parameter and includes docstring.The test correctly uses
file=account_iconand includes a clear docstring.
62-72: LGTM: Invalid data test documented and uses file parameter.The test correctly uses
file=account_iconand documents the 400 error scenario.
74-84: LGTM: Update not-found test documented and uses file parameter.The test correctly uses
file=account_iconand documents the 404 error scenario.
86-93: LGTM: Enable test documented.The docstring describes the enable flow test.
95-99: LGTM: Enable not-found test documented.The docstring clearly describes the expected 404 error case.
101-106: LGTM: Disable test documented.The docstring describes the disable flow test.
108-112: LGTM: Disable not-found test documented.The docstring clearly describes the expected 404 error case.
114-125: LGTM: RQL filter test documented.The docstring clearly describes the RQL filtering test.
mpt_api_client/resources/catalog/mixins.py (5)
1-2: LGTM: Required import added.The
urljoinimport is necessary for constructing resource URLs in the update methods.
86-112: LGTM: AsyncCreateFileMixin implementation is correct.The async create mixin properly handles optional file uploads using the configured
_upload_file_keyand_upload_data_key, withforce_multipart=Truefor consistent request formatting.
159-164: LGTM: AsyncProductMixin composition is appropriate.The composite mixin correctly combines
AsyncCreateFileMixin,AsyncUpdateFileMixin, andAsyncPublishableMixinto provide complete product management capabilities.
196-229: LGTM: UpdateFileMixin implementation is correct.The synchronous update mixin properly handles optional file uploads, mirroring the async implementation.
232-237: LGTM: ProductMixin composition is appropriate.The composite mixin correctly combines
CreateFileMixin,UpdateFileMixin, andPublishableMixinfor synchronous product operations.tests/e2e/accounts/account/test_sync_account.py (10)
9-22: LGTM: Fixture improved with docstring and file parameter.The fixture correctly uses
file=account_iconand includes a clear docstring and error handling in teardown.
24-28: LGTM: Test documents expected 404 behavior.The docstring appropriately describes the error case being tested.
36-43: LGTM: List test documented.The docstring clearly describes the test's purpose.
51-58: LGTM: Update test uses file parameter and includes docstring.The test correctly uses
file=account_iconand includes a clear docstring.
60-66: LGTM: Invalid data test documented and uses file parameter.The test correctly uses
file=account_iconand documents the 400 error scenario.
68-76: LGTM: Update not-found test documented and uses file parameter.The test correctly uses
file=account_iconand documents the 404 error scenario.
78-85: LGTM: Enable test documented.The docstring describes the enable flow test.
87-91: LGTM: Enable not-found test documented.The docstring clearly describes the expected 404 error case.
99-103: LGTM: Disable not-found test documented.The docstring clearly describes the expected 404 error case.
105-116: LGTM: RQL filter test documented.The docstring clearly describes the RQL filtering test.
tests/e2e/accounts/licensees/test_async_licensees.py (2)
10-16: LGTM! Fixture and parameter updates look good.The fixture now properly documents its purpose and the migration from
logotofileparameter aligns with the new file-upload semantics introduced in the mixins.
78-100: LGTM! Test updates are consistent with the new API.The docstrings improve clarity and the
file=account_iconparameter correctly reflects the refactored file-upload interface.mpt_api_client/resources/accounts/buyers.py (2)
22-29: LGTM! Upload configuration properly defined.The
_upload_file_keyand_upload_data_keyattributes correctly configure the file upload behavior for the buyers service, consistent with the pattern used across other account resources.
32-40: LGTM! Mixin consolidation simplifies the service.Replacing individual mixins with
BuyerMixin[Buyer]cleanly consolidates create, update, activate, enable, and validate functionality while maintaining the same public API.tests/unit/http/test_mixins.py (2)
26-43: LGTM! Service refactoring is well-structured.The rename from
DummyIconServicetoDummyFileOperationsServicewith updated endpoints clearly reflects the shift from icon-specific to generic file operations handling.
1098-1117: LGTM! File operations test is thorough.The test properly validates multipart form-data structure with the
_attachment_datakey and confirms file content is correctly included in the request.mpt_api_client/resources/accounts/licensees.py (2)
21-29: LGTM! Upload configuration properly defined.The
_upload_file_key = "logo"and_upload_data_key = "licensee"correctly configure file upload handling for the licensees service.
31-39: LGTM! Mixin consolidation maintains functionality.The
LicenseeMixin[Licensee]properly consolidates create, update, and enable operations while maintaining the same public API surface.mpt_api_client/resources/accounts/account.py (2)
23-31: LGTM! Upload configuration properly defined.The
_upload_file_key = "logo"and_upload_data_key = "account"correctly configure the multipart file upload behavior for accounts.
33-46: LGTM! Mixin consolidation preserves all functionality.The
AccountMixin[Account]consolidates create, update, activate, enable, and validate operations, while theusers()method continues to work correctly.mpt_api_client/resources/accounts/mixins.py (3)
137-166: LGTM! CreateFileMixin correctly implements file upload.The mixin properly:
- Accepts optional file parameter
- Uses
_upload_file_keyfor multipart field naming- Forces multipart encoding with
force_multipart=True- Embeds JSON data via
json_file_key=self._upload_data_keyThis pattern is distinct from the generic
FilesOperationsMixininhttp/mixins.pyand is appropriate for account-specific resources.
169-206: LGTM! UpdateFileMixin correctly constructs URL and handles file upload.The
urljoin(f"{self.path}/", resource_id)properly constructs the resource URL with a trailing slash to ensure correct path joining, and the multipart handling mirrors the create flow appropriately.
209-234: LGTM! Composite mixins consolidate related operations cleanly.The
AccountMixin,BuyerMixin, andLicenseeMixinclasses properly compose the file upload mixins with action mixins (activatable, enableable, validate) to provide unified interfaces for each resource type.
robcsegal
force-pushed
the
MPT-15606-update-implementation-of-create-and-update-mixin-with-file
branch
2 times, most recently
from
3e327a0 to
082b5a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
mpt_api_client/resources/catalog/mixins.py (1)
115-149: Code duplication across modules should be consolidated.The
AsyncUpdateFileMixinandUpdateFileMixinclasses are duplicated inmpt_api_client/resources/accounts/mixins.py. Additionally,CreateFileMixinandAsyncCreateFileMixinare present in multiple locations. This violates the DRY principle and increases maintenance burden.Consider consolidating all file-upload mixins in
mpt_api_client/http/mixins.pyand importing them where needed.Also applies to: 196-230
🧹 Nitpick comments (4)
mpt_api_client/resources/accounts/mixins.py (4)
137-167: Consider consolidating duplicate CreateFileMixin implementations.The
CreateFileMixinimplementation here is nearly identical to the one inmpt_api_client/resources/catalog/mixins.py(lines 166-192). Both have the same logic for handling file uploads via multipart form-data. The only differences are in docstring wording ("Create logo" vs "Create document").Consider moving this shared implementation to a common location (e.g.,
mpt_api_client/http/mixins.py) and importing it in both accounts and catalog modules to reduce duplication and improve maintainability.
169-207: Consider consolidating duplicate UpdateFileMixin implementations.Similar to
CreateFileMixin, thisUpdateFileMixinimplementation is nearly identical to the one inmpt_api_client/resources/catalog/mixins.py(lines 195-228). Consider consolidating these duplicate implementations in a shared location to improve code maintainability.
209-235: LGTM!The composite mixins properly combine file upload capabilities with resource-specific action mixins. Note that
AccountMixinandBuyerMixincurrently have identical composition; if they're expected to remain identical, consider creating a shared base or documenting why they're separate.
375-445: Consider consolidating duplicate async file mixin implementations.The
AsyncCreateFileMixinandAsyncUpdateFileMixinimplementations here are nearly identical to those inmpt_api_client/resources/catalog/mixins.py(lines 85-147). Consider consolidating these async file upload mixins in a shared location alongside the sync versions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
mpt_api_client/http/mixins.py(0 hunks)mpt_api_client/resources/accounts/account.py(3 hunks)mpt_api_client/resources/accounts/buyers.py(3 hunks)mpt_api_client/resources/accounts/licensees.py(2 hunks)mpt_api_client/resources/accounts/mixins.py(3 hunks)mpt_api_client/resources/catalog/mixins.py(4 hunks)mpt_api_client/resources/catalog/products.py(3 hunks)seed/catalog/product.py(1 hunks)tests/e2e/accounts/account/test_async_account.py(6 hunks)tests/e2e/accounts/account/test_sync_account.py(6 hunks)tests/e2e/accounts/api_tokens/test_async_api_tokens.py(7 hunks)tests/e2e/accounts/api_tokens/test_sync_api_tokens.py(7 hunks)tests/e2e/accounts/buyers/test_async_buyers.py(7 hunks)tests/e2e/accounts/buyers/test_sync_buyers.py(6 hunks)tests/e2e/accounts/licensees/test_async_licensees.py(7 hunks)tests/e2e/accounts/licensees/test_sync_licensees.py(3 hunks)tests/e2e/accounts/user_groups/test_async_user_groups.py(5 hunks)tests/e2e/accounts/user_groups/test_sync_user_groups.py(5 hunks)tests/e2e/catalog/product/test_async_product.py(2 hunks)tests/e2e/catalog/product/test_sync_product.py(2 hunks)tests/unit/http/test_mixins.py(46 hunks)tests/unit/resources/accounts/test_account.py(4 hunks)tests/unit/resources/accounts/test_buyers.py(4 hunks)tests/unit/resources/accounts/test_licensees.py(4 hunks)tests/unit/resources/accounts/test_mixins.py(4 hunks)tests/unit/resources/catalog/test_mixins.py(8 hunks)tests/unit/resources/catalog/test_products.py(4 hunks)
💤 Files with no reviewable changes (1)
- mpt_api_client/http/mixins.py
🚧 Files skipped from review as they are similar to previous changes (11)
- tests/unit/resources/accounts/test_licensees.py
- tests/unit/resources/accounts/test_account.py
- tests/unit/resources/catalog/test_products.py
- tests/e2e/accounts/api_tokens/test_async_api_tokens.py
- tests/e2e/catalog/product/test_sync_product.py
- seed/catalog/product.py
- mpt_api_client/resources/accounts/licensees.py
- mpt_api_client/resources/accounts/buyers.py
- tests/e2e/accounts/user_groups/test_sync_user_groups.py
- tests/e2e/accounts/account/test_async_account.py
- tests/unit/resources/accounts/test_buyers.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:54.691Z
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.691Z
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/e2e/accounts/buyers/test_async_buyers.pytests/e2e/accounts/user_groups/test_async_user_groups.pytests/e2e/catalog/product/test_async_product.pytests/e2e/accounts/licensees/test_async_licensees.py
🧬 Code graph analysis (13)
tests/e2e/accounts/api_tokens/test_sync_api_tokens.py (5)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/conftest.py (1)
mpt_vendor(19-20)tests/e2e/accounts/api_tokens/conftest.py (3)
invalid_api_token_id(10-11)api_token_id(5-6)api_token_factory(15-28)tests/e2e/accounts/api_tokens/test_async_api_tokens.py (4)
test_filter_api_tokens(46-57)test_update_api_token(76-84)created_api_token(10-22)test_api_token_enable(111-117)mpt_api_client/resources/accounts/mixins.py (2)
disable(47-56)disable(279-288)
tests/e2e/accounts/licensees/test_sync_licensees.py (3)
tests/e2e/accounts/conftest.py (1)
account_icon(12-13)mpt_api_client/models/model.py (1)
id(119-121)tests/e2e/accounts/licensees/conftest.py (1)
invalid_licensee_id(20-21)
tests/e2e/accounts/buyers/test_sync_buyers.py (2)
tests/e2e/accounts/conftest.py (1)
account_icon(12-13)mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)
mpt_api_client/resources/accounts/account.py (1)
mpt_api_client/resources/accounts/mixins.py (2)
AccountMixin(209-216)AsyncAccountMixin(447-454)
tests/e2e/accounts/buyers/test_async_buyers.py (5)
tests/e2e/accounts/buyers/conftest.py (2)
buyer_factory(15-39)buyer_account_id(10-11)tests/e2e/accounts/conftest.py (2)
account_id(22-23)account_icon(12-13)tests/e2e/conftest.py (1)
async_mpt_ops(36-39)mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/accounts/user_groups/test_async_user_groups.py (5)
tests/e2e/conftest.py (1)
async_mpt_ops(36-39)mpt_api_client/http/mixins.py (2)
get(220-233)get(239-251)tests/e2e/accounts/conftest.py (2)
user_group_id(37-38)user_group_factory(47-62)mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/accounts/user_groups/conftest.py (1)
invalid_user_group_id(5-6)
tests/unit/resources/catalog/test_mixins.py (2)
mpt_api_client/resources/accounts/mixins.py (4)
AsyncUpdateFileMixin(407-444)UpdateFileMixin(169-206)update(172-206)update(410-444)mpt_api_client/resources/catalog/mixins.py (4)
AsyncUpdateFileMixin(115-148)UpdateFileMixin(196-229)update(118-148)update(199-229)
tests/e2e/catalog/product/test_async_product.py (1)
mpt_api_client/resources/catalog/mixins.py (2)
create(89-112)create(170-193)
tests/unit/resources/accounts/test_mixins.py (2)
mpt_api_client/resources/accounts/mixins.py (8)
AsyncCreateFileMixin(375-404)AsyncUpdateFileMixin(407-444)CreateFileMixin(137-166)UpdateFileMixin(169-206)create(140-166)create(378-404)update(172-206)update(410-444)mpt_api_client/resources/catalog/mixins.py (8)
AsyncCreateFileMixin(86-112)AsyncUpdateFileMixin(115-148)CreateFileMixin(167-193)UpdateFileMixin(196-229)create(89-112)create(170-193)update(118-148)update(199-229)
tests/e2e/accounts/licensees/test_async_licensees.py (2)
tests/e2e/accounts/licensees/conftest.py (3)
licensee_factory(25-52)invalid_licensee_id(20-21)licensee_id(15-16)mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)
mpt_api_client/resources/catalog/products.py (1)
mpt_api_client/resources/catalog/mixins.py (2)
AsyncProductMixin(159-164)ProductMixin(232-237)
mpt_api_client/resources/catalog/mixins.py (1)
mpt_api_client/resources/accounts/mixins.py (5)
AsyncUpdateFileMixin(407-444)update(172-206)update(410-444)UpdateFileMixin(169-206)CreateFileMixin(137-166)
tests/unit/http/test_mixins.py (1)
mpt_api_client/http/mixins.py (8)
AsyncFilesOperationsMixin(184-214)FilesOperationsMixin(83-112)download(61-80)download(162-181)create(18-26)create(86-112)create(118-126)create(187-214)
🪛 Ruff (0.14.6)
tests/unit/resources/catalog/test_mixins.py
506-506: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
535-535: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
559-559: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
589-589: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
tests/unit/resources/accounts/test_mixins.py
828-828: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
869-869: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
909-909: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
954-954: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
mpt_api_client/resources/accounts/mixins.py
140-140: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
176-176: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
378-378: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
414-414: Unused noqa directive (unknown: WPS110)
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 (37)
tests/e2e/accounts/api_tokens/test_sync_api_tokens.py (1)
11-11: Docstring additions clearly document test intent and expectationsThe new docstrings accurately describe each fixture/test’s purpose and expected outcome (including the 404 negative cases) and are consistent with the existing implementations and async counterparts. No changes needed.
Also applies to: 30-30, 39-39, 45-45, 69-69, 75-75, 84-84, 98-98, 104-104, 113-113
tests/e2e/accounts/account/test_sync_account.py (10)
10-15: Fixture correctly wired to newfileupload API.The updated
created_accountfixture signature and thefile=account_iconargument align with the new mixin semantics, and the docstring clarifies its purpose. No issues spotted.
24-27: Clearer negative‐path coverage docstring.The new docstring precisely describes the 404 expectation for invalid IDs; implementation remains sound.
36-42: List test documentation improved without altering behavior.Docstring accurately describes the limit behavior being exercised; assertions and call pattern stay unchanged and valid.
51-56: Synchronous update test now exercises file upload path.Passing
file=account_iconinupdateis consistent with the new contract, and the docstring clearly states the intent of the test.
60-66: Invalid‐data update test correctly combines bad payload with file param.Using
name=""alongsidefile=account_iconkeeps the focus on payload validation (400) while still testing the new file argument; docstring reflects this accurately.
68-75: Not‑found update test aligned with newfileparameter.The 404 path now also exercises the file upload signature, matching the API changes; docstring is clear and matches behavior.
78-84: Enable test signature cleanup and description look good.Removing the unused
account_factoryfixture and adding a concise docstring improves readability without changing semantics of disable→enable flow.
87-90: Enable not‑found test remains correct and better documented.Docstring succinctly explains the 404 expectation for enabling a non‑existent account; no behavioral changes otherwise.
99-102: Disable not‑found test documentation added.Docstring now makes the negative case explicit; the
pytest.raisesassertion and call remain appropriate.
105-116: RQL filter test docstring clarifies synchronous filtering scenario.Docstring matches the actual behavior of filtering and iterating; the selected fields and query construction remain consistent with prior intent.
tests/e2e/accounts/user_groups/test_async_user_groups.py (1)
11-11: LGTM! Documentation improvements enhance test clarity.The added docstrings clearly describe the purpose of the fixture and each test function. No functional changes were made.
Also applies to: 26-26, 32-32, 41-41, 47-47, 71-71, 77-77, 90-90
tests/unit/http/test_mixins.py (3)
26-43: LGTM! File operations services properly implement the new mixins.The
DummyFileOperationsServiceandDummyAsyncFileOperationsServiceclasses correctly inherit from the newFilesOperationsMixinandAsyncFilesOperationsMixin, aligning with the PR's shift to generic file-based uploads.
46-69: LGTM! Fixtures are well-documented and properly structured.The fixtures for file operations services and media services are clearly documented with descriptive docstrings.
1098-1117: LGTM! New tests properly verify file operations with resource data.The sync and async tests correctly verify that:
- Files are uploaded via multipart/form-data
- Resource data is included under the
_attachment_datakey- The response is properly parsed
Both tests follow proper AAA layout.
Also applies to: 1119-1137
tests/e2e/accounts/licensees/test_sync_licensees.py (1)
14-14: LGTM! Parameter rename aligns with the new file-based API.The change from
logo=account_icontofile=account_iconcorrectly reflects the updated API signature that consolidates icon/logo handling into a generic file parameter.Also applies to: 76-76, 87-87
tests/e2e/catalog/product/test_async_product.py (1)
9-9: LGTM! Parameter rename aligns with the new file-based API.The change from
icon=logo_fdtofile=logo_fdcorrectly reflects the updated product API signature for file uploads.Also applies to: 33-33
tests/e2e/accounts/buyers/test_async_buyers.py (1)
11-11: LGTM! Documentation and API updates are consistent.The changes include:
- Clear docstrings describing async test scenarios including 404 error cases
- Consistent parameter rename from
logo=account_icontofile=account_iconin create and update operationsAlso applies to: 18-18, 36-36, 45-45, 51-51, 75-75, 83-83, 87-87, 96-96, 101-101, 112-112, 118-118, 127-127
tests/e2e/accounts/buyers/test_sync_buyers.py (1)
11-11: LGTM! Documentation and API updates are consistent.The changes mirror the async tests with:
- Clear docstrings describing sync test scenarios including 404 error cases
- Consistent parameter rename from
logo=account_icontofile=account_iconin create and update operationsAlso applies to: 17-17, 34-34, 43-43, 49-49, 73-73, 79-79, 82-82, 90-90, 94-94, 104-104, 110-110, 119-119
tests/e2e/accounts/licensees/test_async_licensees.py (1)
11-11: LGTM! Documentation and API updates are consistent.The changes include:
- Clear docstrings describing async licensee test scenarios including 404 error cases
- Consistent parameter rename from
logo=account_icontofile=account_iconin create and update operationsAlso applies to: 15-15, 33-33, 42-42, 48-48, 68-68, 73-73, 81-81, 85-85, 94-94, 99-99, 110-110, 116-116, 125-125
mpt_api_client/resources/catalog/mixins.py (1)
159-165: LGTM! Product mixins properly compose file upload with publishable behavior.The
AsyncProductMixinandProductMixinclasses correctly combine the file-based create/update operations with publishable lifecycle methods, providing a clean API for product management.Also applies to: 232-238
mpt_api_client/resources/accounts/account.py (3)
13-16: LGTM!The consolidated mixin imports simplify the service class definitions and improve maintainability by replacing multiple specialized mixins with composite mixins.
29-30: LGTM!The upload configuration keys properly define the multipart form-data field names. The
_upload_file_key = "logo"and_upload_data_key = "account"will be used by theCreateFileMixinandUpdateFileMixinto construct the correct multipart payload.
33-62: LGTM!The service class inheritance consolidation is well-structured. Both
AccountsServiceandAsyncAccountsServicenow use composite mixins (AccountMixinandAsyncAccountMixin) which includeCreateFileMixin,UpdateFileMixin,ActivatableMixin,EnablableMixin, andValidateMixin. The users() sub-service methods are correctly preserved.mpt_api_client/resources/catalog/products.py (3)
11-14: LGTM!The consolidated mixin imports for catalog products follow the same pattern as accounts, providing consistency across the codebase.
55-56: LGTM!The upload configuration keys are correctly set for products. Note that products use
"icon"as the file key (compared to"logo"for accounts), which properly reflects the different API requirements for each resource type.
59-168: LGTM!The service class inheritance consolidation is well-executed. Both
ProductsServiceandAsyncProductsServicenow use composite mixins while preserving all sub-service methods and the customupdate_settingsmethod.tests/unit/resources/accounts/test_mixins.py (4)
10-21: LGTM!The import additions for file mixins are correct and necessary for the new test coverage.
115-157: LGTM!The dummy service classes for testing file operations are well-structured and follow the existing test patterns. All necessary configuration attributes are properly defined.
209-227: LGTM!The fixtures for file service testing are properly configured and follow the established patterns in the test file.
828-997: LGTM!The test coverage for file operations is comprehensive and well-structured:
- Tests cover both create and update operations
- Tests cover both sync and async variants
- Tests cover scenarios with and without file uploads
- Tests properly validate multipart form-data structure
- Tests verify response parsing and model instantiation
The test implementation follows established patterns and provides strong coverage for the new file mixin functionality.
tests/unit/resources/catalog/test_mixins.py (4)
14-18: LGTM!The import additions for update file mixins are correct and necessary for the new test coverage.
58-78: LGTM!The dummy update file service classes are well-structured and follow the existing test patterns. The configuration properly uses
_upload_data_key = "document"for these test services.
122-130: LGTM!The fixtures for update file service testing are properly configured and follow the established patterns.
506-611: LGTM!The test coverage for update file operations is comprehensive and well-structured:
- Tests cover both sync and async variants
- Tests cover scenarios with and without file uploads
- Tests properly validate multipart form-data structure
- Tests verify response parsing and model instantiation
The tests follow established patterns and provide strong coverage for the update file mixin functionality.
mpt_api_client/resources/accounts/mixins.py (2)
1-4: LGTM!The new imports are necessary for the file upload mixin implementations:
urljoinfor URL construction,FileTypesfor type hints, andResourceDatafor request data typing.
447-472: LGTM!The async composite mixins correctly combine async file upload capabilities with async resource-specific action mixins, maintaining the same composition patterns as their synchronous counterparts.
robcsegal
force-pushed
the
MPT-15606-update-implementation-of-create-and-update-mixin-with-file
branch
from
082b5a4 to
778c7a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/unit/resources/accounts/test_mixins.py (1)
828-867: Remove unused# noqa: WPS210to fix Ruff RUF100The four file create/update tests still have
# noqa: WPS210on their function definitions, but Ruff reports these as unused/unknown and they now fail linting.You can safely drop them:
-def test_create_file_service(create_file_service, tmp_path): # noqa: WPS210 +def test_create_file_service(create_file_service, tmp_path): -async def test_async_create_file_service(async_create_file_service, tmp_path): # noqa: WPS210 +async def test_async_create_file_service(async_create_file_service, tmp_path): -def test_update_file_service(update_file_service, tmp_path): # noqa: WPS210 +def test_update_file_service(update_file_service, tmp_path): -async def test_async_update_file_service(async_update_file_service, tmp_path): # noqa: WPS210 +async def test_async_update_file_service(async_update_file_service, tmp_path):This will clear the RUF100 warnings without affecting behavior.
Also applies to: 869-907, 909-952, 954-997
mpt_api_client/resources/accounts/mixins.py (1)
137-207: Clean up unused# noqa: WPS110and modernize docstrings in file mixinsThe sync/async file mixins are functionally sound: they build
filescorrectly, delegate to the HTTP client withjson_file_key=self._upload_data_keyandforce_multipart=True, and useurljoinproperly.Two minor issues:
- Ruff reports the inline
# noqa: WPS110comments as unused/unknown on thefileparameter (sync & async). These should be removed to satisfy linting.- Docstrings still talk about “logo”/“file image”, but these mixins are now generic file helpers used beyond logos.
Suggested change:
- def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model: # noqa: WPS110 + def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model: @@ - file: FileTypes | None = None, # noqa: WPS110 + file: FileTypes | None = None, @@ - async def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model: # noqa: WPS110 + async def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model: @@ - file: FileTypes | None = None, # noqa: WPS110 + file: FileTypes | None = None,And, if you touch these anyway, consider updating the docstrings to refer to “file” generically instead of “logo”.
Also applies to: 375-444
🧹 Nitpick comments (1)
tests/unit/resources/catalog/test_mixins.py (1)
506-611: Update-file tests cover the intended sync/async and file/no-file paths; consider asserting the JSON part in one case.The four tests correctly:
- Hit the expected PUT URL for the given
resource_id,- Verify presence/absence of the file multipart part depending on whether
fileis passed, and- Assert
multipart/form-dataand model deserialization.If you want slightly stronger regression protection (optional), you could mirror the document-create tests by adding an assertion on the JSON
documentpart in at least one of these update-file tests to guard thejson_file_keywiring as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
mpt_api_client/http/mixins.py(0 hunks)mpt_api_client/resources/accounts/account.py(3 hunks)mpt_api_client/resources/accounts/buyers.py(3 hunks)mpt_api_client/resources/accounts/licensees.py(2 hunks)mpt_api_client/resources/accounts/mixins.py(3 hunks)mpt_api_client/resources/catalog/mixins.py(4 hunks)mpt_api_client/resources/catalog/products.py(3 hunks)seed/catalog/product.py(1 hunks)tests/e2e/accounts/account/test_async_account.py(6 hunks)tests/e2e/accounts/account/test_sync_account.py(6 hunks)tests/e2e/accounts/api_tokens/test_async_api_tokens.py(7 hunks)tests/e2e/accounts/api_tokens/test_sync_api_tokens.py(7 hunks)tests/e2e/accounts/buyers/test_async_buyers.py(7 hunks)tests/e2e/accounts/buyers/test_sync_buyers.py(6 hunks)tests/e2e/accounts/licensees/test_async_licensees.py(7 hunks)tests/e2e/accounts/licensees/test_sync_licensees.py(3 hunks)tests/e2e/accounts/user_groups/test_async_user_groups.py(5 hunks)tests/e2e/accounts/user_groups/test_sync_user_groups.py(5 hunks)tests/e2e/catalog/product/test_async_product.py(2 hunks)tests/e2e/catalog/product/test_sync_product.py(2 hunks)tests/unit/http/test_mixins.py(46 hunks)tests/unit/resources/accounts/test_account.py(4 hunks)tests/unit/resources/accounts/test_buyers.py(4 hunks)tests/unit/resources/accounts/test_licensees.py(4 hunks)tests/unit/resources/accounts/test_mixins.py(4 hunks)tests/unit/resources/catalog/test_mixins.py(8 hunks)tests/unit/resources/catalog/test_products.py(4 hunks)
💤 Files with no reviewable changes (1)
- mpt_api_client/http/mixins.py
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/e2e/accounts/user_groups/test_sync_user_groups.py
- seed/catalog/product.py
- tests/e2e/accounts/api_tokens/test_async_api_tokens.py
- mpt_api_client/resources/catalog/products.py
- tests/e2e/accounts/licensees/test_sync_licensees.py
- tests/e2e/accounts/user_groups/test_async_user_groups.py
- tests/unit/resources/accounts/test_buyers.py
- mpt_api_client/resources/catalog/mixins.py
- tests/e2e/accounts/buyers/test_async_buyers.py
- tests/unit/resources/accounts/test_account.py
- tests/e2e/accounts/api_tokens/test_sync_api_tokens.py
- mpt_api_client/resources/accounts/buyers.py
- tests/e2e/catalog/product/test_async_product.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:54.691Z
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.691Z
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/e2e/accounts/licensees/test_async_licensees.pytests/e2e/accounts/account/test_async_account.py
🧬 Code graph analysis (9)
tests/unit/resources/accounts/test_licensees.py (1)
mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)
mpt_api_client/resources/accounts/licensees.py (1)
mpt_api_client/resources/accounts/mixins.py (2)
AsyncLicenseeMixin(467-472)LicenseeMixin(229-234)
tests/unit/resources/catalog/test_mixins.py (2)
mpt_api_client/resources/accounts/mixins.py (4)
AsyncUpdateFileMixin(407-444)UpdateFileMixin(169-206)update(172-206)update(410-444)mpt_api_client/resources/catalog/mixins.py (4)
AsyncUpdateFileMixin(115-148)UpdateFileMixin(196-229)update(118-148)update(199-229)
tests/unit/resources/accounts/test_mixins.py (4)
tests/unit/conftest.py (3)
DummyModel(10-13)http_client(17-18)async_http_client(22-23)mpt_api_client/http/service.py (1)
Service(12-80)mpt_api_client/http/async_service.py (1)
AsyncService(12-80)mpt_api_client/http/mixins.py (6)
create(18-26)create(86-112)create(118-126)create(187-214)update(44-55)update(145-156)
tests/e2e/accounts/account/test_sync_account.py (3)
tests/e2e/accounts/conftest.py (2)
account_icon(12-13)account_id(22-23)mpt_api_client/resources/accounts/mixins.py (2)
create(140-166)create(378-404)mpt_api_client/http/mixins.py (4)
create(18-26)create(86-112)create(118-126)create(187-214)
mpt_api_client/resources/accounts/account.py (2)
mpt_api_client/resources/accounts/mixins.py (2)
AccountMixin(209-216)AsyncAccountMixin(447-454)mpt_api_client/resources/notifications/accounts.py (1)
AccountsService(22-23)
tests/unit/http/test_mixins.py (3)
mpt_api_client/http/mixins.py (10)
AsyncFilesOperationsMixin(184-214)AsyncModifiableResourceMixin(468-471)FilesOperationsMixin(83-112)ModifiableResourceMixin(464-465)download(61-80)download(162-181)create(18-26)create(86-112)create(118-126)create(187-214)mpt_api_client/http/client.py (1)
request(73-129)mpt_api_client/http/async_client.py (1)
request(61-117)
tests/e2e/accounts/account/test_async_account.py (3)
mpt_api_client/http/mixins.py (6)
create(18-26)create(86-112)create(118-126)create(187-214)update(44-55)update(145-156)tests/e2e/accounts/conftest.py (1)
account_icon(12-13)mpt_api_client/models/model.py (1)
id(119-121)
tests/e2e/catalog/product/test_sync_product.py (3)
mpt_api_client/resources/accounts/mixins.py (2)
create(140-166)create(378-404)mpt_api_client/resources/catalog/mixins.py (2)
create(89-112)create(170-193)tests/e2e/catalog/product/conftest.py (1)
product_data(5-6)
🪛 Ruff (0.14.6)
tests/unit/resources/catalog/test_mixins.py
506-506: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
535-535: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
559-559: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
589-589: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
tests/unit/resources/accounts/test_mixins.py
828-828: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
869-869: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
909-909: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
954-954: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
mpt_api_client/resources/accounts/mixins.py
140-140: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
176-176: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
378-378: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
414-414: Unused noqa directive (unknown: WPS110)
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 (14)
tests/unit/resources/catalog/test_mixins.py (1)
58-77: Update-file dummy services and fixtures are wired consistently with the new mixins.The sync/async
DummyUpdateFileServiceclasses correctly set the endpoint, model, collection key, and upload keys, and the corresponding fixtures expose them cleanly for the tests. This matches theUpdateFileMixin/AsyncUpdateFileMixincontract and the URL patterns used elsewhere in this module.Also applies to: 122-129
tests/unit/resources/catalog/test_products.py (1)
141-152: Product create/update tests correctly updated to use the genericfileparameter.Switching from
icon=tofile=on sync/async create and update matches the new mixin signatures while keeping the same endpoint/method/response expectations. The tests remain valid and continue to exercise the product service surface as intended.Also applies to: 160-172, 180-195, 204-219
tests/e2e/catalog/product/test_sync_product.py (1)
7-11: E2E sync product flows now align with the file-based create/update API.Using
file=logo_fdin both the creation fixture and the update test matches the refactored catalog product API while preserving the existing assertions on product data. This keeps the E2E coverage in sync with the underlying mixin changes.Also applies to: 26-31
tests/unit/resources/accounts/test_licensees.py (1)
38-57: Licensees create/update tests now correctly exercise thefile-based APIUsing
file=logo_filefor sync/asynccreateandupdatematches the new mixin signatures and keeps the existing behavior checks (method, URL, response) intact. Looks good and consistent with the broader refactor away fromlogo=....Also applies to: 59-78, 80-99, 101-119
tests/e2e/accounts/buyers/test_sync_buyers.py (1)
9-25: Buyers sync tests aligned with file-based create/update APISwitching from
logotofilein thecreated_buyerfixture and update test, plus adding clear docstrings, correctly reflects the new Buyer mixins without changing behavior. No issues spotted.Also applies to: 33-41, 42-60, 68-95, 97-121
tests/unit/resources/accounts/test_mixins.py (1)
115-157: *Dummy FileService scaffolding matches new file mixins’ contractsThe DummyCreate/UpdateFileService (and async counterparts) correctly set
_endpoint,_model_class,_collection_key,_upload_file_key, and_upload_data_key, and the fixtures wire them into tests cleanly. This is a good, focused way to unit-test the new file mixins.Also applies to: 209-227
mpt_api_client/resources/accounts/mixins.py (1)
209-235: Composite account/buyer/licensee mixins are well factored
AccountMixin,BuyerMixin,LicenseeMixinand their async counterparts compose the file-create/update and activatable/enablable/validate mixins cleanly, so services only need to inherit one mixin per entity type. This keeps behavior consistent across sync/async variants and simplifies service definitions.Also applies to: 447-472
tests/unit/http/test_mixins.py (1)
26-69: File-operations tests nicely cover multipart create and download behaviorThe new
DummyFileOperationsService/DummyAsyncFileOperationsServiceand their fixtures, plus the added sync/async tests for file create and media download, exercise theFilesOperationsMixinand download mixins thoroughly:
- Create tests assert
_attachment_dataand resource JSON are present in multipart content.- Download tests assert correct Accept headers and that
file_contents,content_type, andfilenameare parsed as expected.- Added docstrings throughout make intent of the various HTTP/mixin tests much clearer.
All of this aligns well with the HTTP client and mixin implementations; no issues from my side.
Also applies to: 46-56, 58-69, 170-225, 227-369, 371-425, 498-541, 643-739, 741-779, 781-917, 925-1009, 1098-1137
mpt_api_client/resources/accounts/licensees.py (1)
11-14: Licensees services correctly wired to new file-based mixinsImporting
LicenseeMixin/AsyncLicenseeMixinand adding_upload_file_key = "logo"and_upload_data_key = "licensee"inLicenseesServiceConfigcleanly hook licensee create/update/enable/disable into the new file mixins. Both sync and async services now share the same config and behavior, which matches how the tests exercise them.Also applies to: 21-29, 31-49
tests/e2e/accounts/account/test_async_account.py (1)
9-22: Async account tests correctly usefileand async fixturesThe async account fixture and tests now pass
file=account_iconfor create/update, and the new docstrings clearly describe each scenario (404s, invalid data, enable/disable, RQL filter). Async fixtures are used correctly (never awaited directly), which is consistent with pytest-asyncio’s behavior. No changes needed.Also applies to: 24-43, 51-83, 86-125
tests/e2e/accounts/licensees/test_async_licensees.py (1)
9-23: Async licensee tests updated consistently with file-based APIThe async licensee fixture and tests now use
file=account_iconfor create/update and add clear docstrings around list/filter and 404 scenarios. Async fixtures are consumed correctly, and the tests exercise the new AsyncLicenseeMixin behavior end-to-end. Looks solid.Also applies to: 32-59, 67-76, 78-101, 109-127
mpt_api_client/resources/accounts/account.py (1)
29-35: Account mixin wiring and upload keys look consistent with file-based APIUsing
AccountMixin[Account]/AsyncAccountMixin[Account]together with_upload_file_key = "logo"and_upload_data_key = "account"cleanly hooks the services into the new file-upload mixins. The MRO still provideshttp_client,path,_model_class, and the upload keys as expected, so create/update-with-file should behave correctly for both sync and async services.Also applies to: 50-54
tests/e2e/accounts/account/test_sync_account.py (2)
10-16:created_accountfixture correctly targets the newfile=APISwitching the fixture to call
accounts.create(account_data, file=account_icon)and trimming the signature keeps it aligned with the new file-upload mixin while improving readability with the added docstring. No functional issues spotted.
51-75: Update/enable tests now exercise file-upload path and improve clarityThe update tests consistently pass
file=account_iconfor success, invalid-data, and not-found scenarios, which validates the new file-capable update behavior end-to-end. The simplifiedtest_account_enablesignature plus added docstrings across these tests make intent clearer without changing semantics.Also applies to: 78-89, 100-107
robcsegal
force-pushed
the
MPT-15606-update-implementation-of-create-and-update-mixin-with-file
branch
from
778c7a2 to
2b260cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
mpt_api_client/resources/accounts/mixins.py (1)
137-472: Consider centralizing the file-upload mixins to avoid three parallel implementations.
CreateFileMixin/UpdateFileMixinand their async counterparts here mirror the logic already present inmpt_api_client.http.mixinsand the catalog mixins (same multipart JSON+file behavior, same use of_upload_file_key/_upload_data_key). Functionally this is fine, but it does mean you now have multiple near-identical create/update-file implementations to keep in sync.If feasible, consider:
- Reusing the HTTP-level file mixins directly here, or
- Having a single shared implementation (e.g., in
http.mixins) that both catalog and accounts import/extend,so any future changes to multipart behavior only need to be made once.
🧹 Nitpick comments (7)
tests/e2e/accounts/user_groups/test_sync_user_groups.py (2)
44-55: Consider making filter criteria more explicit in the docstringCurrent docstring is correct but generic; you might optionally mention that it filters by
idand a specific seedednameto make intent clearer for future readers.
74-81: Update test docstring could be slightly more specificDocstring is correct but could optionally mention that it verifies updating the user group name (e.g., to “E2E Updated User Group”) and receiving a non-
Noneresult.tests/unit/http/test_mixins.py (2)
170-199: Align sync media create calls with the newfile=convention.The async media tests already use
file=(e.g.,await async_media_service.create({...}, file=media_image)), but the sync counterparts still pass the file positionally. For consistency with the updated mixins and to make the call sites self-documenting, consider switching the sync tests to the keyword form:- result = media_service.create({"name": "Product image"}, image_file) + result = media_service.create({"name": "Product image"}, file=image_file) ... - result = media_service.create({}, image_file) + result = media_service.create({}, file=image_file)This won’t change behavior but keeps the tests aligned with the intended public API shape.
Also applies to: 201-225, 371-400, 402-425
1012-1097: Minor naming mismatch in assertion messages for modifiable mixin tests.The new helper services (
_ModifiableResourceService,_AsyncModifiableResourceService,_ManagedService,_AsyncManagedService) are a nice way to reduce inline class noise and still validate method presence. One small nit: the failure messages in the modifiable tests still mention “ManagedResourceMixin”:assert hasattr(result, method_name), f"ManagedResourceMixin should have {method_name} method" ... assert hasattr(result, method_name), ( f"AsyncManagedResourceMixin should have {method_name} method" )Consider aligning these with the test intent:
- assert hasattr(result, method_name), f"ManagedResourceMixin should have {method_name} method" + assert hasattr(result, method_name), f"ModifiableResourceMixin should have {method_name} method" ... - assert hasattr(result, method_name), ( - f"AsyncManagedResourceMixin should have {method_name} method" - ) + assert hasattr(result, method_name), ( + f"AsyncModifiableResourceMixin should have {method_name} method" + )This only affects error messages and keeps terminology consistent.
tests/unit/resources/accounts/test_buyers.py (1)
180-180: Buyer create/update tests aligned withfilekeyword; consider asserting multipart behaviorUsing
file=logo_filefor sync/async buyers matches the newBuyerMixin/AsyncBuyerMixinAPI and keeps the existing expectations intact. As an optional strengthening of these unit tests, you could also assert that the request is sent as multipart when a file is provided, e.g. in one of the tests:@@ def test_buyers_create(buyers_service, tmp_path): # noqa: WPS210 - request = mock_route.calls[0].request + request = mock_route.calls[0].request assert mock_route.call_count == 1 assert request.method == "POST" assert request.url.path == "/public/v1/accounts/buyers" + assert "multipart/form-data" in request.headers.get("Content-Type", "") assert result.to_dict() == buyer_dataSame pattern could be applied to an update test if you want coverage on both paths.
Also applies to: 201-201, 222-222, 244-244
tests/unit/resources/accounts/test_mixins.py (1)
828-997: File create/update tests cover sync/async and with/without-file flows adequately.These tests exercise the new mixins end‑to‑end (correct URL, method, and model mapping) for both sync and async services, with and without a file argument. If you ever want stronger assertions, you could mirror the document tests and also inspect the multipart body (JSON + file parts), but this is not required for correctness.
tests/unit/resources/catalog/test_mixins.py (1)
506-611: Update-file tests exercise multipart behavior well.The sync/async tests for update with and without a file correctly verify the URL, multipart content type, and presence/absence of the file part. If you want parity with the document tests, you could also assert on the JSON
"document"part in these tests, but the current checks are already sufficient to validate the mixin behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
mpt_api_client/http/mixins.py(0 hunks)mpt_api_client/resources/accounts/account.py(3 hunks)mpt_api_client/resources/accounts/buyers.py(3 hunks)mpt_api_client/resources/accounts/licensees.py(2 hunks)mpt_api_client/resources/accounts/mixins.py(3 hunks)mpt_api_client/resources/catalog/mixins.py(4 hunks)mpt_api_client/resources/catalog/products.py(3 hunks)seed/catalog/product.py(1 hunks)tests/e2e/accounts/account/test_async_account.py(6 hunks)tests/e2e/accounts/account/test_sync_account.py(6 hunks)tests/e2e/accounts/api_tokens/test_async_api_tokens.py(7 hunks)tests/e2e/accounts/api_tokens/test_sync_api_tokens.py(7 hunks)tests/e2e/accounts/buyers/test_async_buyers.py(7 hunks)tests/e2e/accounts/buyers/test_sync_buyers.py(6 hunks)tests/e2e/accounts/licensees/test_async_licensees.py(7 hunks)tests/e2e/accounts/licensees/test_sync_licensees.py(3 hunks)tests/e2e/accounts/user_groups/test_async_user_groups.py(5 hunks)tests/e2e/accounts/user_groups/test_sync_user_groups.py(5 hunks)tests/e2e/catalog/product/test_async_product.py(2 hunks)tests/e2e/catalog/product/test_sync_product.py(2 hunks)tests/unit/http/test_mixins.py(46 hunks)tests/unit/resources/accounts/test_account.py(4 hunks)tests/unit/resources/accounts/test_buyers.py(4 hunks)tests/unit/resources/accounts/test_licensees.py(4 hunks)tests/unit/resources/accounts/test_mixins.py(4 hunks)tests/unit/resources/catalog/test_mixins.py(8 hunks)tests/unit/resources/catalog/test_products.py(4 hunks)
💤 Files with no reviewable changes (1)
- mpt_api_client/http/mixins.py
✅ Files skipped from review due to trivial changes (1)
- tests/e2e/accounts/user_groups/test_async_user_groups.py
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/unit/resources/accounts/test_account.py
- tests/e2e/catalog/product/test_async_product.py
- mpt_api_client/resources/catalog/mixins.py
- tests/e2e/accounts/licensees/test_async_licensees.py
- tests/e2e/accounts/api_tokens/test_async_api_tokens.py
- tests/e2e/accounts/api_tokens/test_sync_api_tokens.py
- tests/e2e/catalog/product/test_sync_product.py
- tests/e2e/accounts/account/test_async_account.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:54.691Z
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.691Z
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/e2e/accounts/buyers/test_async_buyers.py
🧬 Code graph analysis (13)
tests/e2e/accounts/buyers/test_async_buyers.py (3)
mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)mpt_api_client/resources/catalog/mixins.py (4)
create(89-112)create(170-193)update(118-148)update(199-229)mpt_api_client/http/mixins.py (6)
create(18-26)create(86-112)create(118-126)create(187-214)update(44-55)update(145-156)
mpt_api_client/resources/accounts/account.py (2)
mpt_api_client/resources/accounts/mixins.py (2)
AccountMixin(209-216)AsyncAccountMixin(447-454)mpt_api_client/resources/notifications/accounts.py (1)
AccountsService(22-23)
tests/unit/resources/accounts/test_licensees.py (3)
mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)mpt_api_client/resources/catalog/mixins.py (4)
create(89-112)create(170-193)update(118-148)update(199-229)mpt_api_client/http/mixins.py (6)
create(18-26)create(86-112)create(118-126)create(187-214)update(44-55)update(145-156)
tests/unit/resources/catalog/test_products.py (3)
mpt_api_client/resources/accounts/mixins.py (4)
create(140-166)create(378-404)update(172-206)update(410-444)mpt_api_client/resources/catalog/mixins.py (4)
create(89-112)create(170-193)update(118-148)update(199-229)mpt_api_client/http/mixins.py (6)
create(18-26)create(86-112)create(118-126)create(187-214)update(44-55)update(145-156)
tests/unit/http/test_mixins.py (1)
mpt_api_client/http/mixins.py (8)
AsyncFilesOperationsMixin(184-214)FilesOperationsMixin(83-112)download(61-80)download(162-181)create(18-26)create(86-112)create(118-126)create(187-214)
mpt_api_client/resources/accounts/licensees.py (1)
mpt_api_client/resources/accounts/mixins.py (2)
AsyncLicenseeMixin(467-472)LicenseeMixin(229-234)
tests/unit/resources/accounts/test_mixins.py (3)
mpt_api_client/resources/accounts/mixins.py (8)
AsyncCreateFileMixin(375-404)AsyncUpdateFileMixin(407-444)CreateFileMixin(137-166)UpdateFileMixin(169-206)create(140-166)create(378-404)update(172-206)update(410-444)mpt_api_client/resources/catalog/mixins.py (8)
AsyncCreateFileMixin(86-112)AsyncUpdateFileMixin(115-148)CreateFileMixin(167-193)UpdateFileMixin(196-229)create(89-112)create(170-193)update(118-148)update(199-229)mpt_api_client/http/mixins.py (6)
create(18-26)create(86-112)create(118-126)create(187-214)update(44-55)update(145-156)
mpt_api_client/resources/accounts/buyers.py (1)
mpt_api_client/resources/accounts/mixins.py (2)
AsyncBuyerMixin(457-464)BuyerMixin(219-226)
tests/e2e/accounts/account/test_sync_account.py (2)
tests/e2e/accounts/conftest.py (1)
account_icon(12-13)mpt_api_client/resources/accounts/mixins.py (2)
create(140-166)create(378-404)
mpt_api_client/resources/catalog/products.py (1)
mpt_api_client/resources/catalog/mixins.py (2)
AsyncProductMixin(159-164)ProductMixin(232-237)
tests/e2e/accounts/licensees/test_sync_licensees.py (3)
tests/e2e/accounts/conftest.py (1)
account_icon(12-13)mpt_api_client/models/model.py (1)
id(119-121)tests/e2e/accounts/licensees/conftest.py (1)
invalid_licensee_id(20-21)
tests/e2e/accounts/buyers/test_sync_buyers.py (4)
tests/e2e/accounts/conftest.py (1)
account_icon(12-13)mpt_api_client/resources/accounts/mixins.py (2)
create(140-166)create(378-404)mpt_api_client/resources/catalog/mixins.py (2)
create(89-112)create(170-193)mpt_api_client/http/mixins.py (4)
create(18-26)create(86-112)create(118-126)create(187-214)
mpt_api_client/resources/accounts/mixins.py (4)
mpt_api_client/resources/catalog/mixins.py (5)
CreateFileMixin(167-193)create(89-112)create(170-193)UpdateFileMixin(196-229)AsyncCreateFileMixin(86-112)mpt_api_client/models/model.py (1)
Model(65-125)mpt_api_client/http/mixins.py (4)
create(18-26)create(86-112)create(118-126)create(187-214)mpt_api_client/http/base_service.py (1)
path(28-30)
🪛 Ruff (0.14.6)
tests/unit/resources/accounts/test_mixins.py
828-828: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
869-869: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
909-909: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
954-954: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
tests/unit/resources/catalog/test_mixins.py
506-506: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
535-535: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
559-559: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
589-589: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
mpt_api_client/resources/accounts/mixins.py
140-140: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
176-176: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
378-378: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
414-414: Unused noqa directive (unknown: WPS110)
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 (23)
tests/e2e/accounts/user_groups/test_sync_user_groups.py (5)
9-20: Fixture docstring is clear and aligned with behaviorDocstring accurately describes the fixture purpose and matches the create/yield pattern; the teardown via delete is straightforward and doesn’t need to be called out explicitly.
29-36: Docstring for listing user groups is appropriateDocstring concisely states the test’s intent (listing with a limit) and matches the assertions.
38-41: Not-found get test docstring is accurateDocstring clearly states the scenario (invalid ID) and expected 404 error, matching the
pytest.raisescondition.
68-71: Not-found delete test docstring matches behaviorDocstring correctly documents the invalid-ID scenario and expected 404.
83-88: Not-found update test docstring is clear and correctDocstring accurately reflects the invalid-ID update scenario and expected 404 error.
tests/unit/http/test_mixins.py (5)
11-18: File-ops dummy services and fixtures are wired correctly.
DummyFileOperationsService/DummyAsyncFileOperationsServicecorrectly pairFilesOperationsMixin/AsyncFilesOperationsMixinwithService/AsyncService, and the fixtures passhttp_clientconsistently. The_endpointand_collection_keychoices align with how other dummy services in this file are structured, so these helpers should behave as expected in the new tests.Also applies to: 26-44, 46-69
227-295: File download tests correctly exercise 2-step metadata + file flow.The async and sync download tests now stub two routes (JSON metadata with
Accept: application/jsonand the binary file withAccept: image/jpg) and assert on both the resource request count and theAcceptheader of the download request. This matches the expected behavior of a “inspect-then-download” API and should catch regressions in header handling or URL construction.Also applies to: 298-334, 336-369
427-541: Docstring updates significantly improve test intent clarity.The expanded docstrings across the queryable and collection mixin tests (order/filter/select, fetch_one/fetch_page, iterate variants, error handling, lazy evaluation) make it much easier to understand what each test is asserting without altering behavior. No functional issues spotted.
Also applies to: 555-617, 643-711, 742-770, 782-848, 874-1000
1098-1137: FilesOperationsMixin create tests correctly validate multipart JSON attachment.The new sync/async tests for
DummyFileOperationsServiceverify that passingresource_dataplus afilesdict produces a multipart payload containing an_attachment_datapart with the JSON body and that the response is parsed into the model. This lines up with theFilesOperationsMixin.createcontract (defaultdata_key="_attachment_data"), and the explicit assertions on the encoded JSON inrequest.contentshould catch regressions in how the JSON part is injected.
72-73: Docstring standardization across CRUD and collection tests looks good.The updated docstrings on the basic CRUD tests (create/update/delete/get) and collection helpers (fetch/iterate variants) are concise, follow a consistent style, and don’t affect behavior. They make the suite more self-explanatory, especially for new contributors.
Also applies to: 92-93, 105-106, 121-122, 142-142, 155-155, 171-171, 202-202, 435-435, 451-451, 467-467, 493-493, 512-512, 534-534, 555-555, 572-572, 581-581, 592-592, 615-615, 643-643, 662-662, 681-681, 694-694, 709-709, 742-742, 768-768, 782-782, 799-799, 810-810, 823-823, 847-847, 875-875, 894-894, 913-913, 926-926, 941-941, 974-974, 1000-1000
tests/unit/resources/catalog/test_products.py (1)
151-151: Product create/update now correctly usefilekwargThe updated calls to
products_service.create/update(..., file=icon_file)and their async counterparts match the newProductMixin/AsyncProductMixinsignatures and keep the existing behavior expectations in these tests intact (method, URL, response mapping). Looks good.Also applies to: 171-171, 191-195, 215-219
tests/unit/resources/accounts/test_licensees.py (1)
50-50: Licensee create/update call sites aligned with new file-based APIUsing
file=logo_filein sync and async licensee create/update (including not-found) matches the newLicenseeMixin/AsyncLicenseeMixininterfaces and keeps the existing assertions valid.Also applies to: 71-71, 92-92, 113-113
seed/catalog/product.py (1)
52-52: Seeded product creation updated to newfileparameterThe switch to
await mpt_vendor.catalog.products.create(..., file=icon_file)matches the new async product mixin and keeps the seeding flow unchanged otherwise.tests/e2e/accounts/licensees/test_sync_licensees.py (1)
14-15: E2E licensee tests correctly migrated tofileargumentPassing
file=account_iconintocreate/update(and the not-found case) aligns with the new file-upload mixins while preserving the existing E2E behavior and assertions.Also applies to: 76-77, 87-88
tests/e2e/accounts/buyers/test_async_buyers.py (1)
11-11: Async buyer tests correctly migrated and fixture usage is idiomatic
- The switch to
file=account_iconin async create/update calls matches the new async buyer mixins.- Added docstrings improve test readability without changing behavior.
- The
async_created_buyerasync fixture is used directly in async tests (not awaited), which is the correct pytest-asyncio pattern. Based on learnings, this is the expected fixture behavior.Also applies to: 18-19, 36-36, 45-45, 51-51, 75-75, 83-83, 87-88, 96-96, 101-102, 112-112, 118-118, 127-127
tests/e2e/accounts/buyers/test_sync_buyers.py (1)
11-11: Sync buyer tests now match file-based API and are better documentedUsing
file=account_iconin sync create/update (including not-found) aligns with the new buyer mixins, and the added docstrings make test intent clearer without altering behavior.Also applies to: 17-17, 34-34, 43-43, 49-49, 73-73, 79-79, 82-82, 90-90, 94-94, 104-104, 110-110, 119-119
mpt_api_client/resources/accounts/account.py (1)
14-16: Account services correctly switched to consolidated mixins with upload config
AccountsService/AsyncAccountsServicenow derive fromAccountMixin/AsyncAccountMixin, giving them the file-aware create/update plus activation/enable/validate behavior in a single place._upload_file_key = "logo"and_upload_data_key = "account"inAccountsServiceConfigare consistent with the other account-related services and match the intended multipart payload structure.- Reusing
AccountsServiceConfigin notifications still works, as the new upload keys are only consumed by the account file mixins.Also applies to: 29-30, 34-34, 50-50
tests/unit/resources/accounts/test_mixins.py (1)
10-227: Dummy file services and fixtures are consistent with the new mixins.The four dummy services correctly wire
CreateFileMixin/UpdateFileMixin(and async variants) with matching_endpoint,_upload_file_key="file", and_upload_data_key="resource", and the fixtures expose them cleanly for tests. No changes needed here.tests/unit/resources/catalog/test_mixins.py (1)
58-129: New update-file dummy services and fixtures are wired correctly.
DummyUpdateFileServiceandDummyAsyncUpdateFileServiceare configured with the expected endpoint and upload keys (file/document), matching the catalogUpdateFileMixin/AsyncUpdateFileMixincontract, and the fixtures expose them cleanly.tests/e2e/accounts/account/test_sync_account.py (1)
9-116: E2E account tests now align with the newfile-based API.Switching the fixture and update tests to use
file=account_iconmatches the newAccountMixinfile-upload contract, and removing the unusedaccount_factoryparameter fromtest_account_enablesimplifies the signature without changing behavior. The added docstrings improve clarity.mpt_api_client/resources/catalog/products.py (1)
11-57: Product services correctly adoptProductMixin, but watch the public API change (icon → file).Using
ProductMixin[Product]/AsyncProductMixin[Product]with_upload_file_key="icon"and_upload_data_key="product"is consistent with the new generic file-upload mixins and removes the need for per-service create/update overrides. This does change the public create/update signature tofile=...rather than an icon-specific parameter, so please make sure:
- All internal call sites have been updated, and
- This behavioral/API change is captured in release notes / versioning expectations for library consumers.
mpt_api_client/resources/accounts/licensees.py (1)
11-49: Licensees services cleanly switched to the consolidated licensee mixins.Wiring
LicenseesService/AsyncLicenseesServicethroughLicenseeMixin/AsyncLicenseeMixinplus setting_upload_file_key="logo"and_upload_data_key="licensee"in the config gives them the shared create/update-file and enable/disable behavior without custom methods. Looks consistent with the rest of the accounts refactor.mpt_api_client/resources/accounts/buyers.py (1)
12-68: Buyers services now leverage the shared buyer mixins correctly.
BuyersServiceandAsyncBuyersServiceinheriting fromBuyerMixin/AsyncBuyerMixinplus setting_upload_file_key="logo"and_upload_data_key="buyer"centralizes create/update-file and activation/validation behavior. The existingsynchronizeandtransferhelpers remain intact, so this is a nice simplification of the buyers surface.
robcsegal
force-pushed
the
MPT-15606-update-implementation-of-create-and-update-mixin-with-file
branch
from
2b260cf to
eb87608
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/unit/resources/accounts/test_mixins.py (1)
830-999: Remove now-unused# noqa: WPS210directives from file testsThe four create/update file tests are fine functionally, but Ruff reports the
# noqa: WPS210comments on their definitions as unused (RUF100) since that code is no longer needed. You can safely drop them:-def test_create_file_service(create_file_service, tmp_path): # noqa: WPS210 +def test_create_file_service(create_file_service, tmp_path): @@ -async def test_async_create_file_service(async_create_file_service, tmp_path): # noqa: WPS210 +async def test_async_create_file_service(async_create_file_service, tmp_path): @@ -def test_update_file_service(update_file_service, tmp_path): # noqa: WPS210 +def test_update_file_service(update_file_service, tmp_path): @@ -async def test_async_update_file_service(async_update_file_service, tmp_path): # noqa: WPS210 +async def test_async_update_file_service(async_update_file_service, tmp_path):This resolves the RUF100 warnings while keeping the new tests (with/without file, sync/async) intact.
🧹 Nitpick comments (6)
tests/e2e/accounts/api_tokens/test_sync_api_tokens.py (1)
23-26: Consider adding docstrings to the remaining test functions for consistency.While the added docstrings are great, consider documenting the remaining test functions for consistency:
test_get_api_token_by_id- Describe that it tests retrieving a valid API tokentest_create_api_token- Describe that it tests successful API token creationtest_delete_api_token- Describe that it tests successful API token deletiontest_api_token_disable- Describe that it tests disabling an API tokenAlso applies to: 58-62, 64-66, 91-94
mpt_api_client/resources/accounts/account.py (1)
1-24: Accounts now correctly use the shared file mixinsSwitching to
CreateFileMixin/UpdateFileMixinwith_upload_file_key = "logo"and_upload_data_key = "account"aligns the accounts API with the new file-upload abstraction; the tests usingfile=account_iconmatch this, so behavior looks consistent.As an optional typing improvement, you could parameterize the mixins with
Accountinstead of the baseModelto keep type hints precise and consistent withGetMixin[Account]/CollectionMixin[Account]:class AccountsService( - CreateFileMixin[Model], - UpdateFileMixin[Model], - ActivatableMixin[Model], - EnablableMixin[Model], - ValidateMixin[Model], + CreateFileMixin[Account], + UpdateFileMixin[Account], + ActivatableMixin[Account], + EnablableMixin[Account], + ValidateMixin[Account], @@ class AsyncAccountsService( - AsyncCreateFileMixin[Model], - AsyncUpdateFileMixin[Model], - AsyncActivatableMixin[Model], - AsyncEnablableMixin[Model], - AsyncValidateMixin[Model], + AsyncCreateFileMixin[Account], + AsyncUpdateFileMixin[Account], + AsyncActivatableMixin[Account], + AsyncEnablableMixin[Account], + AsyncValidateMixin[Account],This doesn’t change runtime behavior but makes the returned types more explicit.
Also applies to: 31-78
mpt_api_client/http/mixins.py (1)
115-185: Create/Update file mixins are wired correctly; clean up unusednoqacodesThe new sync/async
CreateFileMixin/UpdateFileMixincorrectly delegate to the HTTP client withforce_multipart=Trueand use_upload_file_key/_upload_data_keyas expected, so services that set those config attributes will get consistent multipart behavior with or without an actual file.Ruff is flagging the
# noqa: WPS110directives on thefileparameters as unused (unknown code), so you can safely remove them:- def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model: # noqa: WPS110 + def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model: @@ - file: FileTypes | None = None, # noqa: WPS110 + file: FileTypes | None = None, @@ - async def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model: # noqa: WPS110 + async def create(self, resource_data: ResourceData, file: FileTypes | None = None) -> Model: @@ - file: FileTypes | None = None, # noqa: WPS110 + file: FileTypes | None = None,This keeps the codebase in sync with the current linting configuration. As per static analysis hints.
Also applies to: 187-257
mpt_api_client/resources/catalog/products.py (1)
1-18: Product services correctly use file mixins; consider tightening generic parametersThe switch to
CreateFileMixin/UpdateFileMixinwith_upload_file_key = "icon"and_upload_data_key = "product"matches the HTTP mixin behavior and keeps the publicfile=parameter in tests consistent with the underlying API.For clearer typing, you may want to parameterize the file and publishable mixins with
Productinstead of the baseModel, in line withGetMixin[Product]andCollectionMixin[Product]:class ProductsService( - CreateFileMixin[Model], - UpdateFileMixin[Model], - PublishableMixin[Model], + CreateFileMixin[Product], + UpdateFileMixin[Product], + PublishableMixin[Product], @@ class AsyncProductsService( - AsyncCreateFileMixin[Model], - AsyncUpdateFileMixin[Model], - AsyncPublishableMixin[Model], + AsyncCreateFileMixin[Product], + AsyncUpdateFileMixin[Product], + AsyncPublishableMixin[Product],This keeps the type hints aligned with the actual resource type without changing runtime behavior.
Also applies to: 53-76, 120-129
seed/catalog/product.py (1)
26-41: LGTM! Improved control flow and file-based upload integration.The refactoring improves code clarity with:
- Better variable naming and explicit type handling in
get_product- Clearer control flow in
init_product,review_product, andpublish_product- Consistent migration from
icon=icon_filetofile=icon_file- Enhanced error handling and logging
The changes align well with the broader migration to file-based upload mixins.
Optional improvement: Static analysis suggests extracting the error message at line 65 to a constant or using a shorter message to comply with TRY003, but this is a minor style consideration and can be deferred.
Also applies to: 51-65, 75-83, 93-101
tests/unit/resources/accounts/test_mixins.py (1)
117-159: Dummy*FileService classes are properly configured for file mixinsEach dummy service defines
_endpoint,_model_class,_collection_key, and the required_upload_file_key/_upload_data_key, so the file mixins can construct multipart requests as intended. If you ever expand this pattern, you could factor a tiny shared base/config for the repeated create/update-file attributes, but it’s not necessary here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
mpt_api_client/http/mixins.py(1 hunks)mpt_api_client/resources/accounts/account.py(3 hunks)mpt_api_client/resources/accounts/buyers.py(3 hunks)mpt_api_client/resources/accounts/licensees.py(2 hunks)mpt_api_client/resources/accounts/mixins.py(0 hunks)mpt_api_client/resources/catalog/mixins.py(1 hunks)mpt_api_client/resources/catalog/pricing_policy_attachments.py(1 hunks)mpt_api_client/resources/catalog/product_term_variants.py(1 hunks)mpt_api_client/resources/catalog/products.py(3 hunks)seed/catalog/product.py(4 hunks)tests/e2e/accounts/account/test_async_account.py(6 hunks)tests/e2e/accounts/account/test_sync_account.py(6 hunks)tests/e2e/accounts/api_tokens/test_async_api_tokens.py(7 hunks)tests/e2e/accounts/api_tokens/test_sync_api_tokens.py(7 hunks)tests/e2e/accounts/buyers/test_async_buyers.py(7 hunks)tests/e2e/accounts/buyers/test_sync_buyers.py(6 hunks)tests/e2e/accounts/licensees/test_async_licensees.py(7 hunks)tests/e2e/accounts/licensees/test_sync_licensees.py(3 hunks)tests/e2e/accounts/user_groups/test_async_user_groups.py(5 hunks)tests/e2e/accounts/user_groups/test_sync_user_groups.py(5 hunks)tests/e2e/catalog/product/test_async_product.py(2 hunks)tests/e2e/catalog/product/test_sync_product.py(2 hunks)tests/unit/http/test_mixins.py(46 hunks)tests/unit/resources/accounts/test_account.py(4 hunks)tests/unit/resources/accounts/test_buyers.py(4 hunks)tests/unit/resources/accounts/test_licensees.py(4 hunks)tests/unit/resources/accounts/test_mixins.py(4 hunks)tests/unit/resources/catalog/test_mixins.py(6 hunks)tests/unit/resources/catalog/test_products.py(4 hunks)
💤 Files with no reviewable changes (1)
- mpt_api_client/resources/accounts/mixins.py
✅ Files skipped from review due to trivial changes (1)
- mpt_api_client/resources/catalog/pricing_policy_attachments.py
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/e2e/accounts/buyers/test_async_buyers.py
- tests/unit/resources/accounts/test_account.py
- tests/e2e/accounts/user_groups/test_async_user_groups.py
- tests/e2e/accounts/api_tokens/test_async_api_tokens.py
- tests/e2e/catalog/product/test_async_product.py
- tests/unit/resources/catalog/test_mixins.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-27T00:05:54.691Z
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.691Z
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/e2e/accounts/licensees/test_async_licensees.pytests/e2e/accounts/account/test_async_account.py
🧬 Code graph analysis (17)
mpt_api_client/resources/accounts/licensees.py (2)
mpt_api_client/http/mixins.py (4)
AsyncCreateFileMixin(187-216)AsyncUpdateFileMixin(219-256)CreateFileMixin(115-144)UpdateFileMixin(147-184)mpt_api_client/resources/accounts/mixins.py (2)
AsyncEnablableMixin(162-185)EnablableMixin(30-53)
tests/e2e/accounts/api_tokens/test_sync_api_tokens.py (3)
mpt_api_client/exceptions.py (1)
MPTAPIError(20-42)tests/e2e/conftest.py (1)
mpt_vendor(19-20)tests/e2e/accounts/api_tokens/conftest.py (3)
invalid_api_token_id(10-11)api_token_id(5-6)api_token_factory(15-28)
tests/unit/resources/accounts/test_mixins.py (1)
mpt_api_client/http/mixins.py (14)
AsyncCreateFileMixin(187-216)AsyncUpdateFileMixin(219-256)CreateFileMixin(115-144)UpdateFileMixin(147-184)create(18-26)create(86-112)create(118-144)create(190-216)create(262-270)create(331-358)update(44-55)update(150-184)update(222-256)update(289-300)
mpt_api_client/resources/accounts/account.py (4)
mpt_api_client/http/mixins.py (5)
AsyncCreateFileMixin(187-216)AsyncUpdateFileMixin(219-256)CreateFileMixin(115-144)GetMixin(361-377)UpdateFileMixin(147-184)mpt_api_client/models/model.py (1)
Model(65-125)mpt_api_client/resources/catalog/mixins.py (2)
ActivatableMixin(109-132)AsyncActivatableMixin(135-160)mpt_api_client/resources/accounts/mixins.py (6)
ActivatableMixin(4-27)EnablableMixin(30-53)ValidateMixin(56-68)AsyncActivatableMixin(134-159)AsyncEnablableMixin(162-185)AsyncValidateMixin(188-200)
mpt_api_client/resources/catalog/products.py (2)
mpt_api_client/http/mixins.py (4)
AsyncCreateFileMixin(187-216)AsyncUpdateFileMixin(219-256)CreateFileMixin(115-144)UpdateFileMixin(147-184)mpt_api_client/resources/catalog/mixins.py (2)
PublishableMixin(11-45)AsyncPublishableMixin(48-82)
tests/e2e/accounts/licensees/test_async_licensees.py (1)
mpt_api_client/http/mixins.py (10)
create(18-26)create(86-112)create(118-144)create(190-216)create(262-270)create(331-358)update(44-55)update(150-184)update(222-256)update(289-300)
tests/unit/resources/catalog/test_products.py (1)
mpt_api_client/http/mixins.py (10)
create(18-26)create(86-112)create(118-144)create(190-216)create(262-270)create(331-358)update(44-55)update(150-184)update(222-256)update(289-300)
tests/unit/resources/accounts/test_buyers.py (1)
mpt_api_client/http/mixins.py (10)
create(18-26)create(86-112)create(118-144)create(190-216)create(262-270)create(331-358)update(44-55)update(150-184)update(222-256)update(289-300)
tests/e2e/accounts/buyers/test_sync_buyers.py (1)
mpt_api_client/http/mixins.py (10)
create(18-26)create(86-112)create(118-144)create(190-216)create(262-270)create(331-358)update(44-55)update(150-184)update(222-256)update(289-300)
tests/unit/http/test_mixins.py (8)
mpt_api_client/http/mixins.py (18)
AsyncFilesOperationsMixin(328-358)AsyncUpdateFileMixin(219-256)FilesOperationsMixin(83-112)UpdateFileMixin(147-184)get(364-377)get(383-395)download(61-80)download(306-325)create(18-26)create(86-112)create(118-144)create(190-216)create(262-270)create(331-358)update(44-55)update(150-184)update(222-256)update(289-300)tests/unit/conftest.py (1)
http_client(17-18)mpt_api_client/http/service.py (1)
Service(12-80)mpt_api_client/http/async_service.py (1)
AsyncService(12-80)tests/unit/http/conftest.py (2)
async_dummy_service(42-43)dummy_service(37-38)mpt_api_client/http/types.py (2)
Response(27-42)json(40-42)mpt_api_client/http/async_client.py (1)
request(61-117)mpt_api_client/http/client.py (1)
request(73-129)
mpt_api_client/resources/catalog/mixins.py (1)
mpt_api_client/http/mixins.py (3)
AsyncCreateFileMixin(187-216)AsyncDownloadFileMixin(303-325)CreateFileMixin(115-144)
mpt_api_client/http/mixins.py (4)
mpt_api_client/models/model.py (2)
Model(65-125)from_response(102-116)mpt_api_client/http/async_client.py (1)
request(61-117)mpt_api_client/http/client.py (1)
request(73-129)mpt_api_client/http/base_service.py (1)
path(28-30)
tests/e2e/accounts/licensees/test_sync_licensees.py (3)
tests/e2e/accounts/conftest.py (1)
account_icon(12-13)mpt_api_client/models/model.py (1)
id(119-121)tests/e2e/accounts/licensees/conftest.py (1)
invalid_licensee_id(20-21)
tests/unit/resources/accounts/test_licensees.py (1)
mpt_api_client/http/mixins.py (10)
create(18-26)create(86-112)create(118-144)create(190-216)create(262-270)create(331-358)update(44-55)update(150-184)update(222-256)update(289-300)
mpt_api_client/resources/accounts/buyers.py (4)
mpt_api_client/http/mixins.py (5)
AsyncCreateFileMixin(187-216)AsyncUpdateFileMixin(219-256)CreateFileMixin(115-144)GetMixin(361-377)UpdateFileMixin(147-184)mpt_api_client/models/model.py (1)
Model(65-125)mpt_api_client/resources/catalog/mixins.py (2)
ActivatableMixin(109-132)AsyncActivatableMixin(135-160)mpt_api_client/resources/accounts/mixins.py (6)
ActivatableMixin(4-27)EnablableMixin(30-53)ValidateMixin(56-68)AsyncActivatableMixin(134-159)AsyncEnablableMixin(162-185)AsyncValidateMixin(188-200)
tests/e2e/accounts/account/test_sync_account.py (2)
tests/e2e/accounts/conftest.py (1)
account_icon(12-13)mpt_api_client/http/mixins.py (10)
create(18-26)create(86-112)create(118-144)create(190-216)create(262-270)create(331-358)update(44-55)update(150-184)update(222-256)update(289-300)
mpt_api_client/resources/catalog/product_term_variants.py (1)
mpt_api_client/http/mixins.py (2)
AsyncCreateFileMixin(187-216)CreateFileMixin(115-144)
🪛 Ruff (0.14.6)
tests/unit/resources/accounts/test_mixins.py
830-830: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
871-871: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
911-911: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
956-956: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
tests/unit/http/test_mixins.py
1174-1174: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
1203-1203: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
1227-1227: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
1257-1257: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
seed/catalog/product.py
65-65: Avoid specifying long messages outside the exception class
(TRY003)
mpt_api_client/http/mixins.py
118-118: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
154-154: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
190-190: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
226-226: Unused noqa directive (unknown: WPS110)
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 (20)
tests/e2e/accounts/api_tokens/test_sync_api_tokens.py (1)
11-11: LGTM! Well-written docstrings.The added docstrings are clear, concise, and accurately describe the test scenarios and expected behaviors. They improve the test documentation quality.
Also applies to: 30-30, 39-39, 45-45, 69-69, 75-75, 84-84, 98-98, 104-104, 113-113
mpt_api_client/resources/catalog/mixins.py (1)
1-6: Centralizing file upload mixins viampt_api_client.http.mixinslooks goodImporting and reusing
CreateFileMixin/AsyncCreateFileMixinhere removes the earlier duplication of file-upload logic across modules while keepingDocumentMixin/MediaMixinbehavior unchanged. No further issues from my side in this file.Also applies to: 85-168
mpt_api_client/resources/accounts/licensees.py (1)
1-18: Licensees file-upload wiring is consistent with the new HTTP mixinsUsing
CreateFileMixin/UpdateFileMixin(and async variants) together with_upload_file_key = "logo"and_upload_data_key = "licensee"cleanly maps the publicfile=parameter to the existing logo upload contract. I don’t see functional issues here.Also applies to: 25-58
tests/e2e/accounts/account/test_async_account.py (1)
9-125: Async account tests are aligned with the newfile=APIThe async fixture and tests now exercise the file-upload path correctly (
create/updatewithfile=account_icon) and the added docstrings make intent clear. Using the asyncasync_created_accountfixture directly in async tests without awaiting it is also appropriate with pytest-asyncio’s fixture resolution model. Based on learnings, this setup looks good.mpt_api_client/resources/catalog/product_term_variants.py (1)
1-33: Term variant services correctly switched to shared file mixinsImporting
CreateFileMixin/AsyncCreateFileMixinfrommpt_api_client.http.mixinsand wiring them with_upload_file_key = "file"/_upload_data_key = "variant"gives a clean, centralized implementation for uploads, with accurate generics ([...] [TermVariant]). All good here.Also applies to: 45-54
tests/e2e/accounts/account/test_sync_account.py (1)
10-21: Sync account tests match the new file-upload behaviorThe
created_accountfixture and all related tests now correctly usefile=account_iconwith the accounts service, and the added docstrings improve readability around 404/400 and filtering scenarios. Removing the unusedaccount_factoryparameter fromtest_account_enablealso addresses the earlier lint warning. Looks good.Also applies to: 24-116
tests/unit/http/test_mixins.py (2)
28-103: LGTM! Well-structured test infrastructure for file operations.The new
DummyFileOperationsService,DummyAsyncFileOperationsService,DummyUpdateFileService, andDummyAsyncUpdateFileServiceclasses, along with their fixtures, provide comprehensive test coverage for the file-based mixins. The configuration with_upload_file_keyand_upload_data_keyproperly reflects the new file upload semantics.
1132-1279: LGTM! Comprehensive test coverage for file operations.The test suite thoroughly covers both synchronous and asynchronous file operations, including:
- Create operations with resource data and files
- Update operations with and without files
- Proper multipart form-data validation
- Request content assertions
The test structure follows AAA (Arrange-Act-Assert) pattern consistently.
tests/unit/resources/accounts/test_licensees.py (1)
50-50: LGTM! Consistent parameter migration fromlogotofile.All licensee create and update operations have been correctly updated to use the
fileparameter instead oflogo, aligning with the new file-based upload mixins across both synchronous and asynchronous service calls.Also applies to: 71-71, 92-92, 113-113
tests/e2e/accounts/licensees/test_sync_licensees.py (1)
14-14: LGTM! E2E tests updated for file-based uploads.The synchronous licensee E2E tests have been correctly updated to use
file=account_iconinstead oflogo=account_icon, covering create, update, and error scenarios consistently.Also applies to: 76-76, 87-87
tests/e2e/accounts/user_groups/test_sync_user_groups.py (1)
11-11: LGTM! Helpful documentation improvements.The added docstrings improve test readability by clearly describing the purpose of each test case, including fixture setup, listing operations, filtering, error handling, and CRUD operations.
Also applies to: 30-30, 39-39, 45-45, 69-69, 75-75, 84-84
tests/unit/resources/accounts/test_buyers.py (1)
180-180: LGTM! Consistent migration to file-based parameters.All buyer create and update operations have been correctly updated to use the
fileparameter, maintaining consistency across both synchronous and asynchronous service implementations.Also applies to: 201-201, 222-222, 244-244
tests/unit/resources/catalog/test_products.py (1)
151-151: LGTM! Product tests updated with improved formatting.The product create and update operations have been correctly migrated from
icontofileparameter. The multiline formatting for update calls improves code readability while maintaining test functionality.Also applies to: 171-171, 191-195, 215-219
tests/e2e/accounts/licensees/test_async_licensees.py (1)
11-11: LGTM! Async licensee tests updated with improved documentation.The asynchronous licensee E2E tests have been correctly updated to use
file=account_iconinstead oflogo=account_icon. The addition of comprehensive docstrings enhances test clarity by describing each test scenario (listing, filtering, error handling, CRUD operations, enable/disable functionality).Also applies to: 15-15, 33-33, 42-42, 48-48, 68-68, 73-73, 81-81, 85-85, 94-94, 99-99, 110-110, 116-116, 125-125
tests/e2e/catalog/product/test_sync_product.py (1)
9-9: Product create/update tests correctly migrated tofile=APIBoth the fixture and the update test now pass the logo via the new
file=parameter, matching the updated file-based mixin signatures without changing test intent.Also applies to: 30-30
tests/e2e/accounts/buyers/test_sync_buyers.py (2)
11-24: Buyer fixture and update path correctly switched tofile=Using
file=account_iconfor both create and update keeps these tests aligned with the new file-based buyers API while preserving the existing setup/teardown semantics.Also applies to: 78-95
34-35: Added buyer test docstrings make scenarios and expectations explicitThe new docstrings succinctly describe list, filter, and 404 behaviors for buyers, improving readability without altering test behavior.
Also applies to: 43-44, 49-50, 73-74, 90-91, 104-105, 110-111, 119-120
tests/unit/resources/accounts/test_mixins.py (2)
6-11: File mixin imports correctly reflect the new HTTP mixin surfaceBringing in the four file-oriented mixins alongside the existing Service types is consistent with the broader migration and keeps this test module self-contained.
211-228: New fixtures cleanly expose the file-capable dummy servicesThe fixtures simply wrap the dummy services with the appropriate HTTP clients, matching the existing pattern in this module and keeping the tests straightforward.
mpt_api_client/resources/accounts/buyers.py (1)
2-13: Buyers services are correctly wired to the new file-based create/update mixinsSwitching BuyersService/AsyncBuyersService over to the *CreateFileMixin/*UpdateFileMixin variants and defining
_upload_file_key = "logo"/_upload_data_key = "buyer"inBuyersServiceConfiggives the multipart behavior needed for logo uploads while preserving the existing create/update calling pattern (now viafile=). The rest of the buyer operations (get, collection, synchronize, transfer, etc.) remain unaffected.Also applies to: 30-38, 40-51, 73-84
|
|
||
| async def test_async_create_with_icon_no_data(async_icon_service): | ||
| resource_data = {"id": "OBJ-0000-0001", "name": "Icon Object"} | ||
| def test_sync_update_file(update_file_service): # noqa: WPS210 |
There was a problem hiding this comment.
Remove unused noqa directives.
Static analysis indicates that the # noqa: WPS210 directives are unused and should be removed from these test function definitions.
Apply this diff:
-def test_sync_update_file(update_file_service): # noqa: WPS210
+def test_sync_update_file(update_file_service):
-def test_update_file_no_file(update_file_service): # noqa: WPS210
+def test_update_file_no_file(update_file_service):
-async def test_async_update_file(async_update_file_service): # noqa: WPS210
+async def test_async_update_file(async_update_file_service):
-async def test_async_update_file_no_file(async_update_file_service): # noqa: WPS210
+async def test_async_update_file_no_file(async_update_file_service):Also applies to: 1203-1203, 1227-1227, 1257-1257
🧰 Tools
🪛 Ruff (0.14.6)
1174-1174: Unused noqa directive (unknown: WPS210)
Remove unused noqa directive
(RUF100)
🤖 Prompt for AI Agents
In tests/unit/http/test_mixins.py around lines 1174, 1203, 1227, and 1257, the
test function definitions include unused "# noqa: WPS210" directives; remove
those trailing noqa comments from each declared test function so the lines
contain just the function definition (e.g., def
test_sync_update_file(update_file_service):) without the unused noqa, and run
linters/tests to confirm no remaining noqa usages.
|
robcsegal
deleted the
MPT-15606-update-implementation-of-create-and-update-mixin-with-file
branch
Update implementation of create and update mixin with file
Updated creating and updating product so previous create and update mixin with logo can be completely removed
https://softwareone.atlassian.net/browse/MPT-15606
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.