[MPT-16319] Added commerce orders assets endpoints #156
Conversation
robcsegal
requested review from
albertsola,
alephsur,
d3rky,
jentyk,
ruben-sebrango and
svazquezco
as code owners
WalkthroughAdds a new OrdersAsset resource: a model and parallel sync/async service implementations, plus Changes
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)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (2)mpt_api_client/resources/commerce/orders.py (2)
mpt_api_client/resources/commerce/orders_asset.py (5)
🪛 Ruff (0.14.8)mpt_api_client/resources/commerce/orders_asset.py23-23: Unused Remove unused (RUF100) 48-48: Unused Remove unused (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)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Found Jira issue key in the title: MPT-16319 Tests mirroring check (created files only)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/unit/resources/commerce/test_orders_asset.py (1)
49-82: Optionally assert respx route invocation in render testsThe render tests verify returned markdown content, but they don’t assert that the mocked
respx.getroute was actually hit. You could capture the route into a variable and assertcall_count == 1(similar to other tests) to make the tests more robust against future refactors.- with respx.mock: - respx.get( - "https://api.example.com/public/v1/commerce/orders/ORD-123/assets/ASSET-456/render" - ).mock( + with respx.mock: + mock_route = respx.get( + "https://api.example.com/public/v1/commerce/orders/ORD-123/assets/ASSET-456/render" + ).mock( return_value=httpx.Response( @@ - result = asset_service.render("ORD-123", "ASSET-456") - - assert result == template_content + result = asset_service.render("ORD-123", "ASSET-456") + + assert mock_route.call_count == 1 + assert result == template_content(Same pattern could be applied to
test_async_render.)mpt_api_client/resources/commerce/orders_asset.py (1)
23-28: Clean up unused# noqa: WPS215directivesRuff flags these
# noqa: WPS215comments as unknown/unused (RUF100). Unless you still run a separate linter that depends onWPS215, it’s simpler to drop them from both service classes.-class OrdersAssetService( # noqa: WPS215 +class OrdersAssetService( @@ -class AsyncOrdersAssetService( # noqa: WPS215 +class AsyncOrdersAssetService(Also applies to: 49-55
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
mpt_api_client/resources/commerce/orders.py(3 hunks)mpt_api_client/resources/commerce/orders_asset.py(1 hunks)tests/unit/resources/commerce/test_orders.py(2 hunks)tests/unit/resources/commerce/test_orders_asset.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
mpt_api_client/resources/commerce/orders_asset.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/models/model.py (1)
Model(65-125)mpt_api_client/http/types.py (1)
text(36-38)
tests/unit/resources/commerce/test_orders_asset.py (4)
mpt_api_client/resources/commerce/orders_asset.py (3)
OrdersAssetService(23-46)render(31-46)render(57-72)tests/unit/conftest.py (2)
http_client(17-18)async_http_client(22-23)mpt_api_client/http/base_service.py (1)
path(28-30)mpt_api_client/http/types.py (1)
Response(27-42)
🪛 Ruff (0.14.8)
mpt_api_client/resources/commerce/orders_asset.py
23-23: Unused noqa directive (unknown: WPS215)
Remove unused noqa directive
(RUF100)
31-31: Unused method argument: order_id
(ARG002)
49-49: Unused noqa directive (unknown: WPS215)
Remove unused noqa directive
(RUF100)
57-57: Unused method argument: order_id
(ARG002)
⏰ 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 (8)
mpt_api_client/resources/commerce/orders.py (3)
12-15: Asset service imports wired correctlyImporting
AsyncOrdersAssetServiceandOrdersAssetServicehere cleanly exposes the new assets functionality for both sync and async order services; placement and usage are consistent with existing subscription imports.
122-134: Syncassets()factory mirrors existingsubscriptions()pattern
OrdersService.assets()correctly constructs anOrdersAssetServicewithhttp_clientandendpoint_params={"order_id": order_id}, matching howsubscriptions()scopes child resources to an order.
240-252: Asyncassets()factory is consistent with sync API
AsyncOrdersService.assets()returnsAsyncOrdersAssetServicewith the expectedendpoint_params, giving a symmetric async surface to the syncassets()helper.tests/unit/resources/commerce/test_orders_asset.py (1)
11-47: Service fixtures and basic capability tests look solidThe fixtures, mixin presence checks, and endpoint path assertions correctly validate that both sync and async asset services are scoped to
order_idand expose the expected CRUD+render surface.tests/unit/resources/commerce/test_orders.py (3)
6-9: Asset service imports align with new Orders APIImporting
OrdersAssetServiceandAsyncOrdersAssetServicehere matches the newassets()helpers on the orders services; this keeps tests in sync with the production API surface.
251-258: Syncassets()factory test correctly validates wiring
test_asset_serviceasserts both the returned type andendpoint_params == {"order_id": "ORD-123"}, which is exactly what we want to guarantee fromOrdersService.assets().
260-266: Asyncassets()factory test mirrors the sync coverage
test_async_asset_serviceprovides the same checks for the async variant, ensuringAsyncOrdersService.assets()is wired identically.mpt_api_client/resources/commerce/orders_asset.py (1)
11-21: Model and service config follow existing resource conventions
OrdersAssetplusOrdersAssetServiceConfig(endpoint, model, collection key) are in line with other commerce resources and correctly encode the nested/orders/{order_id}/assetspath.
robcsegal
force-pushed
the
MPT-16319-add-missing-commerce-orders-assets-endpoints
branch
from
36a63cd to
3e6f9ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/resources/commerce/test_orders.py (1)
6-9: Assets tests correctly validate service wiringThe new tests exercise
OrdersService.assets()/AsyncOrdersService.assets()as expected, asserting both the concrete service type andendpoint_params={"order_id": "ORD-123"}, which matches the production implementations. This gives good coverage of the new entry points. If you ever want to DRY things up, you could mirror the subscription tests by reusing theorders_service/async_orders_servicefixtures here too, but that’s purely optional.Also applies to: 251-267
mpt_api_client/resources/commerce/orders_asset.py (1)
1-70: OrdersAsset services and render endpoints are wired correctly; consider cleaning upnoqamarkersThe model, config, and sync/async services follow the established pattern in this client: the endpoint template, mixins, and
render()/async render()calls into_resource_do_request(..., "render")are all consistent and should produce the expected/orders/{order_id}/assets/{asset_id}/renderURLs returning markdown text.The only minor clean-up is the
# noqa: WPS215on the two service class definitions: if you’re no longer relying on that rule from your style checker, you can drop those comments to avoid “unused noqa” noise:-class OrdersAssetService( # noqa: WPS215 +class OrdersAssetService( @@ -class AsyncOrdersAssetService( # noqa: WPS215 +class AsyncOrdersAssetService(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
mpt_api_client/resources/commerce/orders.py(3 hunks)mpt_api_client/resources/commerce/orders_asset.py(1 hunks)tests/unit/resources/commerce/test_orders.py(2 hunks)tests/unit/resources/commerce/test_orders_asset.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/resources/commerce/test_orders_asset.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/resources/commerce/test_orders.py (3)
mpt_api_client/resources/commerce/orders_asset.py (2)
AsyncOrdersAssetService(48-70)OrdersAssetService(23-45)tests/unit/conftest.py (2)
http_client(17-18)async_http_client(22-23)mpt_api_client/resources/commerce/orders.py (2)
assets(122-134)assets(240-252)
mpt_api_client/resources/commerce/orders_asset.py (5)
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 (4)
AsyncCollectionMixin(538-605)AsyncManagedResourceMixin(622-625)CollectionMixin(469-535)ManagedResourceMixin(618-619)mpt_api_client/models/model.py (1)
Model(65-125)mpt_api_client/http/types.py (1)
text(36-38)
🪛 Ruff (0.14.8)
mpt_api_client/resources/commerce/orders_asset.py
23-23: Unused noqa directive (unknown: WPS215)
Remove unused noqa directive
(RUF100)
48-48: Unused noqa directive (unknown: WPS215)
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 (1)
mpt_api_client/resources/commerce/orders.py (1)
12-15: Assets accessors mirror existing subscriptions pattern and look correctThe sync and async
assets(order_id)methods cleanly constructOrdersAssetService/AsyncOrdersAssetServicewithendpoint_params={"order_id": order_id}, consistent with the subscriptions helpers and theOrdersAssetServiceConfigendpoint template. The imports and type hints line up with the new module. No issues from my side.Also applies to: 122-135, 240-252
d3rky
force-pushed
the
MPT-16319-add-missing-commerce-orders-assets-endpoints
branch
from
3e6f9ff to
8d31267
Compare
|
robcsegal
deleted the
MPT-16319-add-missing-commerce-orders-assets-endpoints
branch
Added commerce orders assets endpoints
https://softwareone.atlassian.net/browse/MPT-16319
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.