[MPT-16142] Added commerce assets endpoints #143
Conversation
robcsegal
requested review from
albertsola,
alephsur,
d3rky,
jentyk,
ruben-sebrango and
svazquezco
as code owners
WalkthroughAdds Asset and AssetTemplate models and synchronous/asynchronous Assets services (create/update/get/collection) with terminate and render endpoints, integrates assets accessors into Commerce/AsyncCommerce, and adds unit + async tests for the new services and integration. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AssetsService
participant HTTP_API as "Commerce API"
participant Model as "Asset / AssetTemplate"
Client->>AssetsService: render(asset_id) / terminate(asset_id, resource_data?)
AssetsService->>HTTP_API: HTTP request to /public/v1/commerce/assets/... (sync/async)
HTTP_API-->>AssetsService: JSON response
AssetsService->>Model: from_response(json) -> Asset / AssetTemplate
AssetsService-->>Client: return model instance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
✅ Found Jira issue key in the title: MPT-16142 Tests mirroring check (created files only)
|
robcsegal
force-pushed
the
MPT-16142-add-missing-assets-endpoints-to-commerce
branch
from
74aeec2 to
3032b6d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
mpt_api_client/resources/commerce/assets.py (2)
42-50: Guardterminateagainst sendingjson=Nonewhen no payload is requiredBoth sync and async
terminatepassjson=resource_datadirectly into_resource_do_request, which means that whenresource_dataisNone, the underlying client may serialize a literalnullbody instead of omitting the request body entirely. If the terminate endpoint expects an empty body, this could be undesirable.Consider only including the
jsonargument whenresource_datais notNone, e.g.:- def terminate(self, asset_id: str, resource_data: ResourceData | None = None) -> Asset: + def terminate(self, asset_id: str, resource_data: ResourceData | None = None) -> Asset: @@ - response = self._resource_do_request(asset_id, "POST", "terminate", json=resource_data) + kwargs = {"json": resource_data} if resource_data is not None else {} + response = self._resource_do_request(asset_id, "POST", "terminate", **kwargs) @@ - async def terminate(self, asset_id: str, resource_data: ResourceData | None = None) -> Asset: + async def terminate(self, asset_id: str, resource_data: ResourceData | None = None) -> Asset: @@ - response = await self._resource_do_request( - asset_id, "POST", "terminate", json=resource_data - ) + kwargs = {"json": resource_data} if resource_data is not None else {} + response = await self._resource_do_request( + asset_id, "POST", "terminate", **kwargs + )Also applies to: 76-85
52-63: Align render docstrings with the actual return typeBoth sync and async
rendermethods return anAssetTemplatemodel instance, but the docstring says “Render asset template json.” Updating the return description to explicitly mentionAssetTemplatewill better reflect the API and help IDEs/docs:
- E.g., “Returns: AssetTemplate model representing the rendered asset template.”
Also applies to: 89-100
tests/unit/resources/commerce/test_commerce.py (1)
55-65: Useasync_http_clientin async properties test to tighten type coverage
test_async_commerce_propertiesconstructsAsyncCommerce(http_client=http_client), so all async services (including the newassetsaccessor) are built with the synchttp_clientfixture. This passes the currentisinstancechecks but won’t catch interface drift betweenAsyncHTTPClientandHTTPClient.Consider switching the fixture to
async_http_client(as intest_async_commerce_init) so async services are instantiated with the correct client type.-@pytest.mark.parametrize( +@pytest.mark.parametrize( @@ -def test_async_commerce_properties(http_client, attr_name, expected): - commerce = AsyncCommerce(http_client=http_client) +def test_async_commerce_properties(async_http_client, attr_name, expected): + commerce = AsyncCommerce(http_client=async_http_client)tests/unit/resources/commerce/test_assets.py (2)
19-34: Strengthen async tests with explicit markers and route assertionsTwo small improvements to consider for the async tests:
test_async_rendercurrently doesn’t capture therespxroute or assert that it was called, unlike the terminate tests. You can mirror the other tests to verify the mock was hit:- with respx.mock: - respx.get("https://api.example.com/public/v1/commerce/assets/ASSET-123/render").mock( + with respx.mock: + mock_route = respx.get( + "https://api.example.com/public/v1/commerce/assets/ASSET-123/render" + ).mock( @@ - result = await async_assets_service.render("ASSET-123") - - assert result is not None + result = await async_assets_service.render("ASSET-123") + + assert mock_route.called + assert mock_route.call_count == 1 + assert result is not None
- Both
test_async_renderandtest_async_terminateare plainasync deftests without a visible marker. Depending on your pytest/pytest-asyncio configuration, you may need@pytest.mark.asyncioor@pytest.mark.anyioto ensure these tests are actually awaited and executed.Also applies to: 75-92
95-102: Optionally assert that service attributes are callable, not just presentThe parametrized method tests currently only check
hasattr(assets_service, method). To guard against these attributes being accidentally replaced with non-callables, you could also assert callability:-@pytest.mark.parametrize("method", ["create", "update", "get"]) -def test_assets_service_methods(assets_service, method): - assert hasattr(assets_service, method) # act +@pytest.mark.parametrize("method", ["create", "update", "get"]) +def test_assets_service_methods(assets_service, method): + assert hasattr(assets_service, method) + assert callable(getattr(assets_service, method)) @@ -@pytest.mark.parametrize("method", ["create", "update", "get"]) -def test_async_assets_service_methods(async_assets_service, method): - assert hasattr(async_assets_service, method) # act +@pytest.mark.parametrize("method", ["create", "update", "get"]) +def test_async_assets_service_methods(async_assets_service, method): + assert hasattr(async_assets_service, method) + assert callable(getattr(async_assets_service, method))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
mpt_api_client/resources/commerce/assets.py(1 hunks)mpt_api_client/resources/commerce/commerce.py(3 hunks)tests/unit/resources/commerce/test_assets.py(1 hunks)tests/unit/resources/commerce/test_commerce.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
mpt_api_client/resources/commerce/commerce.py (1)
mpt_api_client/resources/commerce/assets.py (2)
AssetsService(32-63)AsyncAssetsService(66-100)
tests/unit/resources/commerce/test_commerce.py (2)
mpt_api_client/resources/commerce/commerce.py (2)
assets(33-35)assets(60-62)mpt_api_client/resources/commerce/assets.py (2)
AssetsService(32-63)AsyncAssetsService(66-100)
mpt_api_client/resources/commerce/assets.py (4)
mpt_api_client/http/async_service.py (1)
AsyncService(12-80)mpt_api_client/http/service.py (1)
Service(12-80)mpt_api_client/http/mixins.py (8)
AsyncCollectionMixin(538-605)AsyncCreateMixin(259-270)AsyncGetMixin(380-395)AsyncUpdateMixin(286-300)CollectionMixin(469-535)CreateMixin(15-26)GetMixin(361-377)UpdateMixin(41-55)mpt_api_client/models/model.py (1)
Model(65-125)
tests/unit/resources/commerce/test_assets.py (3)
mpt_api_client/resources/commerce/commerce.py (2)
assets(33-35)assets(60-62)mpt_api_client/resources/commerce/assets.py (4)
AssetsService(32-63)AsyncAssetsService(66-100)terminate(42-50)terminate(76-87)mpt_api_client/http/types.py (2)
Response(27-42)json(40-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
mpt_api_client/resources/commerce/assets.py (1)
24-39: Assets service config and class composition look consistent
AssetsServiceConfigand theAssetsService/AsyncAssetsServiceinheritance order (mixins, thenService/AsyncService, then config) match the existing patterns from other commerce services, so endpoint, model binding, and collection handling should behave consistently.tests/unit/resources/commerce/test_commerce.py (1)
38-45: Extending property coverage toassetskeeps Commerce API surface consistentAdding
"assets"to the parametrizedtest_commerce_propertiesalongside agreements/orders/subscriptions ensures the new service accessor is exercised in the same way as existing ones.mpt_api_client/resources/commerce/commerce.py (1)
3-3: Newassetsaccessors align with existing Commerce service patternImporting
AssetsService/AsyncAssetsServiceand exposing them viaassetsproperties onCommerceandAsyncCommercemirrors the existing agreements/orders/subscriptions accessors, keeping the public API shape consistent across all commerce sub-services.Also applies to: 32-35, 59-62
robcsegal
force-pushed
the
MPT-16142-add-missing-assets-endpoints-to-commerce
branch
from
3032b6d to
5177ecf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/unit/resources/commerce/test_assets.py (1)
95-102: Extend method-presence tests to coveriterateas wellThe parametrized tests for method presence cover only
create,update, andget. GivenAssetsService/AsyncAssetsServicealso inheritCollectionMixin/AsyncCollectionMixin, consider adding"iterate"to the list to ensure collection iteration remains available on these services:-@pytest.mark.parametrize("method", ["create", "update", "get"]) +@pytest.mark.parametrize("method", ["create", "update", "get", "iterate"]) def test_assets_service_methods(assets_service, method): assert hasattr(assets_service, method) # act -@pytest.mark.parametrize("method", ["create", "update", "get"]) +@pytest.mark.parametrize("method", ["create", "update", "get", "iterate"]) def test_async_assets_service_methods(async_assets_service, method): assert hasattr(async_assets_service, method) # actThis aligns with previous feedback about checking
iterateon the collection mixin-based services.
🧹 Nitpick comments (4)
tests/unit/resources/commerce/test_assets.py (2)
19-33: Align async render test with sync tests by asserting route usage
test_async_renderonly asserts that the result is notNone, while the synctest_renderadditionally checksmock_route.calledandcall_count. For better verification that the correct URL/method are used, consider capturing the route and asserting it was called (and how many times), similar to the sync test:async def test_async_render(async_assets_service): render_response = {"id": "ASSET-123", "title": "Sample Asset"} with respx.mock: - respx.get("https://api.example.com/public/v1/commerce/assets/ASSET-123/render").mock( + mock_route = respx.get( + "https://api.example.com/public/v1/commerce/assets/ASSET-123/render" + ).mock( return_value=httpx.Response( status_code=200, headers={"content-type": APPLICATION_JSON}, json=render_response, ) ) result = await async_assets_service.render("ASSET-123") - assert result is not None + assert mock_route.called + assert mock_route.call_count == 1 + assert result is not None
35-92: Optionally assert model fields, not just non-None resultsThe sync/async
renderandterminatetests currently assert only that the result is non-None(plus route usage for sync). To more tightly couple tests to expected behavior ofAsset/AssetTemplate, you could also assert on key fields (e.g.,result.id,result.status,result.title) to ensure the model parsing is correct:assert result.id == "ASSET-123" # for terminate: assert result.status == "terminated" # for render: assert result.title == "Sample Asset"This would catch mismatches between the API payload and model behavior earlier.
mpt_api_client/resources/commerce/assets.py (2)
42-50: Use_resource_actioninterminatefor consistency and headers
AssetsService.terminatecurrently calls_resource_do_requestdirectly and then wraps the response with_model_class.from_response. SinceServicealready exposes_resource_actionthat handlesAcceptheaders and wraps the response, you can simplify and align with other methods:- def terminate(self, asset_id: str, resource_data: ResourceData | None = None) -> Asset: + def terminate(self, asset_id: str, resource_data: ResourceData | None = None) -> Asset: """Terminate the given Asset id. @@ - response = self._resource_do_request(asset_id, "POST", "terminate", json=resource_data) - return self._model_class.from_response(response) + return self._resource_action( # type: ignore[no-any-return] + asset_id, + "POST", + "terminate", + json=resource_data, + )This reuses the shared logic and keeps behavior (including headers) consistent with other resource actions.
76-87: Mirror the same_resource_actionusage in asyncterminateFor
AsyncAssetsService.terminate, you can similarly delegate toAsyncService._resource_actionto avoid duplicating response handling and ensure headers are applied consistently:- async def terminate(self, asset_id: str, resource_data: ResourceData | None = None) -> Asset: + async def terminate( + self, + asset_id: str, + resource_data: ResourceData | None = None, + ) -> Asset: @@ - response = await self._resource_do_request( - asset_id, "POST", "terminate", json=resource_data - ) - - return self._model_class.from_response(response) + return await self._resource_action( # type: ignore[no-any-return] + asset_id, + "POST", + "terminate", + json=resource_data, + )This keeps sync and async implementations parallel and leverages the existing helper.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
mpt_api_client/resources/commerce/assets.py(1 hunks)mpt_api_client/resources/commerce/commerce.py(3 hunks)tests/unit/resources/commerce/test_assets.py(1 hunks)tests/unit/resources/commerce/test_commerce.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- mpt_api_client/resources/commerce/commerce.py
- tests/unit/resources/commerce/test_commerce.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/resources/commerce/test_assets.py (3)
mpt_api_client/resources/commerce/commerce.py (2)
assets(33-35)assets(60-62)mpt_api_client/resources/commerce/assets.py (6)
AssetsService(32-63)AsyncAssetsService(66-100)render(52-63)render(89-100)terminate(42-50)terminate(76-87)tests/unit/conftest.py (2)
http_client(17-18)async_http_client(22-23)
mpt_api_client/resources/commerce/assets.py (4)
mpt_api_client/http/async_service.py (1)
AsyncService(12-80)mpt_api_client/http/service.py (1)
Service(12-80)mpt_api_client/http/mixins.py (7)
AsyncCreateMixin(259-270)AsyncGetMixin(380-395)AsyncUpdateMixin(286-300)CollectionMixin(469-535)CreateMixin(15-26)GetMixin(361-377)UpdateMixin(41-55)mpt_api_client/models/model.py (1)
Model(65-125)
⏰ 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 (2)
mpt_api_client/resources/commerce/assets.py (2)
16-39: Models and service configuration look consistent with existing patterns
Asset,AssetTemplate, andAssetsServiceConfigare straightforwardModelsubclasses and config with_endpoint,_model_class, and_collection_key. This aligns with other services in the codebase and should integrate cleanly with the shared HTTP/mixins infrastructure.
52-63: Render methods are straightforward and align with HTTP helpersBoth sync and async
rendercorrectly use_resource_do_requestwith the"render"action and wrap the response withAssetTemplate.from_response, which fits the pattern for alternate model representations. The URL construction matches the sharedService/AsyncServicehelpers and the tests’ expectations.Also applies to: 89-100
robcsegal
force-pushed
the
MPT-16142-add-missing-assets-endpoints-to-commerce
branch
from
5177ecf to
4c01907
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
mpt_api_client/resources/commerce/assets.py (1)
16-29: Assets models and services align with existing service/mixin patterns
Asset/AssetTemplateasModelsubclasses, theAssetsServiceConfigendpoint/model/collection configuration, and the sync/asyncAssetsService/AsyncAssetsServiceclasses all look consistent with the rest of the client:
terminatecorrectly goes through_resource_do_requestwith a POST and action"terminate"and returnsAssetviaself._model_class.from_response.rendercorrectly uses the same resource path with action"render"but wraps the response as anAssetTemplate, which is appropriate for a different response schema.- Async counterparts mirror the sync implementations using
await _resource_do_request(...)with matching type hints.The small duplication between sync and async implementations is acceptable and matches the existing style in this client; you could optionally factor shared logic later if more asset-specific operations are added.
Also applies to: 32-100
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
mpt_api_client/resources/commerce/assets.py(1 hunks)mpt_api_client/resources/commerce/commerce.py(3 hunks)tests/unit/resources/commerce/test_assets.py(1 hunks)tests/unit/resources/commerce/test_commerce.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/unit/resources/commerce/test_assets.py
🚧 Files skipped from review as they are similar to previous changes (1)
- mpt_api_client/resources/commerce/commerce.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/resources/commerce/test_commerce.py (2)
mpt_api_client/resources/commerce/commerce.py (2)
assets(33-35)assets(60-62)mpt_api_client/resources/commerce/assets.py (2)
AssetsService(32-63)AsyncAssetsService(66-100)
mpt_api_client/resources/commerce/assets.py (3)
mpt_api_client/http/service.py (1)
Service(12-80)mpt_api_client/http/mixins.py (8)
AsyncCollectionMixin(538-605)AsyncCreateMixin(259-270)AsyncGetMixin(380-395)AsyncUpdateMixin(286-300)CollectionMixin(469-535)CreateMixin(15-26)GetMixin(361-377)UpdateMixin(41-55)mpt_api_client/models/model.py (1)
Model(65-125)
⏰ 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 (1)
tests/unit/resources/commerce/test_commerce.py (1)
6-11: Assets services correctly integrated into Commerce/AsyncCommerce testsThe new imports and parametrized cases for
"assets"cleanly extend the existing pattern for agreements/orders/subscriptions and correctly exercise theCommerce.assets/AsyncCommerce.assetsaccessors returningAssetsServiceandAsyncAssetsService. No issues from a wiring or type perspective.Also applies to: 38-62
robcsegal
force-pushed
the
MPT-16142-add-missing-assets-endpoints-to-commerce
branch
from
4c01907 to
75a7d82
Compare
robcsegal
force-pushed
the
MPT-16142-add-missing-assets-endpoints-to-commerce
branch
from
75a7d82 to
b6593ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/resources/commerce/test_commerce.py (1)
64-69: Consider usingasync_http_clientfixture for async commerce property tests
test_async_commerce_propertiesconstructsAsyncCommerce(http_client=http_client), wherehttp_clientis the sync client fixture. That’s fine for the currentgetattr/isinstancechecks, but if this test ever starts exercising service methods, passing the async HTTP client fixture would better mirror real usage and avoid accidentalawaiton a sync client.mpt_api_client/resources/commerce/assets.py (1)
42-50: Use_resource_actionforterminateto reduce duplication and match other servicesBoth sync and async
terminatecall_resource_do_requestand then manually invokefrom_response. You can simplify and align with existing mixins by delegating to_resource_action, which already wraps_resource_do_requestand sets the JSONAcceptheader:@@ class AssetsService( - def terminate(self, asset_id: str, resource_data: ResourceData | None = None) -> Asset: - """Terminate the given Asset id. - - Args: - asset_id: Asset ID. - resource_data: Resource data will be updated - """ - response = self._resource_do_request(asset_id, "POST", "terminate", json=resource_data) - return self._model_class.from_response(response) + def terminate(self, asset_id: str, resource_data: ResourceData | None = None) -> Asset: + """Terminate the given Asset id. + + Args: + asset_id: Asset ID. + resource_data: Resource data will be updated + """ + return self._resource_action(asset_id, "POST", "terminate", json=resource_data) @@ class AsyncAssetsService( - async def terminate(self, asset_id: str, resource_data: ResourceData | None = None) -> Asset: - """Terminate the given Asset id. - - Args: - asset_id: Asset ID. - resource_data: Resource data will be updated - """ - response = await self._resource_do_request( - asset_id, "POST", "terminate", json=resource_data - ) - - return self._model_class.from_response(response) + async def terminate(self, asset_id: str, resource_data: ResourceData | None = None) -> Asset: + """Terminate the given Asset id. + + Args: + asset_id: Asset ID. + resource_data: Resource data will be updated + """ + return await self._resource_action(asset_id, "POST", "terminate", json=resource_data)Functionality stays the same, but this keeps the implementation closer to other actions and centralizes header handling.
Also applies to: 76-87
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
mpt_api_client/resources/commerce/assets.py(1 hunks)mpt_api_client/resources/commerce/commerce.py(3 hunks)tests/unit/resources/commerce/test_assets.py(1 hunks)tests/unit/resources/commerce/test_commerce.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/resources/commerce/test_assets.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/unit/resources/commerce/test_commerce.py (2)
mpt_api_client/resources/commerce/commerce.py (2)
assets(33-35)assets(60-62)mpt_api_client/resources/commerce/assets.py (2)
AssetsService(32-63)AsyncAssetsService(66-100)
mpt_api_client/resources/commerce/assets.py (4)
mpt_api_client/http/async_service.py (1)
AsyncService(12-80)mpt_api_client/http/service.py (1)
Service(12-80)mpt_api_client/http/mixins.py (8)
AsyncCollectionMixin(538-605)AsyncCreateMixin(259-270)AsyncGetMixin(380-395)AsyncUpdateMixin(286-300)CollectionMixin(469-535)CreateMixin(15-26)GetMixin(361-377)UpdateMixin(41-55)mpt_api_client/models/model.py (1)
Model(65-125)
mpt_api_client/resources/commerce/commerce.py (3)
mpt_api_client/mpt_client.py (2)
commerce(49-51)commerce(98-105)mpt_api_client/resources/commerce/assets.py (2)
AssetsService(32-63)AsyncAssetsService(66-100)tests/unit/conftest.py (1)
http_client(17-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
tests/unit/resources/commerce/test_commerce.py (1)
6-7: Assets services correctly wired into Commerce/AsyncCommerce property testsThe new
AssetsService/AsyncAssetsServiceimports and parametrization entries for"assets"are consistent with the existing agreements/orders/subscriptions patterns and will keep the generic property tests in sync with the new commerce surface.Also applies to: 38-45, 55-62
mpt_api_client/resources/commerce/commerce.py (1)
3-3: Newassetsservice accessors are consistent with existing commerce servicesThe sync and async
assetsproperties mirror the existing agreements/orders/subscriptions patterns (typed accessors, simple service construction withhttp_client). Import and wiring look correct, and this cleanly exposes the new assets services through the Commerce/AsyncCommerce facades.Also applies to: 32-35, 59-62
mpt_api_client/resources/commerce/assets.py (1)
16-39: Asset models and service configuration align with existing HTTP service patterns
Asset/AssetTemplateas thinModelsubclasses plusAssetsServiceConfigwith_endpoint,_model_class = Asset, and_collection_key = "data"are in line with other resources in the client. Combined intoAssetsService/AsyncAssetsServicewith the standard CRUD mixins, this should integrate cleanly with existing collection and action helpers.
|
robcsegal
deleted the
MPT-16142-add-missing-assets-endpoints-to-commerce
branch
Added commerce assets endpoints
https://softwareone.atlassian.net/browse/MPT-16142
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.