[MPT-16423] Moved Render and Template to mixin #163
Conversation
robcsegal
requested review from
albertsola,
alephsur,
d3rky,
jentyk,
ruben-sebrango and
svazquezco
as code owners
WalkthroughRefactors commerce services to remove in-class render/template/terminate implementations and instead compose shared sync/async mixins that provide render and template methods; updates service class bases and adjusts unit tests accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/resources/commerce/test_agreements.py (1)
76-82: Bug: Wrong service class instantiated in async test.
test_async_mixins_presentshould useAsyncAgreementsService, notAgreementsService. Currently, this test verifies the synchronous service instead of the asynchronous one, making it a duplicate oftest_mixins_present.@pytest.mark.parametrize("method", ["create", "update", "get", "render", "template"]) def test_async_mixins_present(async_http_client, method): - service = AgreementsService(http_client=async_http_client) + service = AsyncAgreementsService(http_client=async_http_client) result = hasattr(service, method) assert result is True
🧹 Nitpick comments (2)
mpt_api_client/resources/commerce/orders_asset.py (1)
34-40: Remove unusednoqadirective.Static analysis indicates the
WPS215suppression is no longer needed onAsyncOrdersAssetService.-class AsyncOrdersAssetService( # noqa: WPS215 +class AsyncOrdersAssetService( AsyncRenderMixin[OrdersAsset],mpt_api_client/resources/commerce/subscriptions.py (1)
49-58: Remove unusednoqadirective.Static analysis indicates the
WPS215suppression is no longer needed onAsyncSubscriptionsService.-class AsyncSubscriptionsService( # noqa: WPS215 +class AsyncSubscriptionsService( AsyncCreateMixin[Subscription],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
mpt_api_client/resources/commerce/agreements.py(3 hunks)mpt_api_client/resources/commerce/assets.py(2 hunks)mpt_api_client/resources/commerce/mixins.py(2 hunks)mpt_api_client/resources/commerce/orders.py(3 hunks)mpt_api_client/resources/commerce/orders_asset.py(2 hunks)mpt_api_client/resources/commerce/subscriptions.py(2 hunks)tests/unit/resources/commerce/test_agreements.py(2 hunks)tests/unit/resources/commerce/test_assets.py(0 hunks)tests/unit/resources/commerce/test_mixins.py(7 hunks)tests/unit/resources/commerce/test_orders.py(1 hunks)tests/unit/resources/commerce/test_orders_asset.py(0 hunks)
💤 Files with no reviewable changes (2)
- tests/unit/resources/commerce/test_assets.py
- tests/unit/resources/commerce/test_orders_asset.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/unit/resources/commerce/test_mixins.pytests/unit/resources/commerce/test_orders.pytests/unit/resources/commerce/test_agreements.py
🧬 Code graph analysis (8)
mpt_api_client/resources/commerce/agreements.py (1)
mpt_api_client/resources/commerce/mixins.py (4)
AsyncRenderMixin(68-81)AsyncTemplateMixin(84-97)RenderMixin(20-33)TemplateMixin(36-49)
tests/unit/resources/commerce/test_mixins.py (3)
mpt_api_client/resources/commerce/mixins.py (4)
AsyncRenderMixin(68-81)RenderMixin(20-33)render(23-33)render(71-81)mpt_api_client/http/service.py (1)
Service(12-80)mpt_api_client/http/async_service.py (1)
AsyncService(12-80)
mpt_api_client/resources/commerce/orders_asset.py (2)
mpt_api_client/resources/commerce/mixins.py (2)
AsyncRenderMixin(68-81)RenderMixin(20-33)mpt_api_client/http/service.py (1)
Service(12-80)
mpt_api_client/resources/commerce/mixins.py (2)
mpt_api_client/models/model.py (1)
Model(65-125)mpt_api_client/http/types.py (1)
text(36-38)
mpt_api_client/resources/commerce/subscriptions.py (1)
mpt_api_client/resources/commerce/mixins.py (2)
AsyncRenderMixin(68-81)RenderMixin(20-33)
tests/unit/resources/commerce/test_orders.py (13)
tests/unit/resources/commerce/test_agreements.py (1)
test_mixins_present(68-73)tests/unit/resources/commerce/test_orders_asset.py (1)
test_mixins_present(22-25)tests/unit/resources/catalog/test_items.py (1)
test_mixins_present(29-32)tests/unit/resources/catalog/test_listings.py (1)
test_mixins_present(20-23)tests/unit/resources/catalog/test_pricing_policies.py (1)
test_mixins_present(149-152)tests/unit/resources/catalog/test_products.py (1)
test_mixins_present(64-67)tests/unit/resources/catalog/test_units_of_measure.py (1)
test_mixins_present(20-23)tests/unit/resources/accounts/test_account.py (1)
test_mixins_present(39-42)tests/unit/resources/accounts/test_buyers.py (1)
test_mixins_present(34-37)tests/unit/resources/accounts/test_accounts_user_groups.py (1)
test_mixins_present(51-54)tests/unit/resources/accounts/test_account_user_groups.py (1)
test_mixins_present(45-48)tests/unit/resources/accounts/test_erp_links.py (1)
test_mixins_present(20-23)tests/unit/resources/accounts/test_sellers.py (1)
test_mixins_present(22-25)
mpt_api_client/resources/commerce/orders.py (2)
mpt_api_client/mpt_client.py (2)
commerce(49-51)commerce(98-105)mpt_api_client/resources/commerce/mixins.py (4)
AsyncRenderMixin(68-81)AsyncTemplateMixin(84-97)RenderMixin(20-33)TemplateMixin(36-49)
mpt_api_client/resources/commerce/assets.py (1)
mpt_api_client/resources/commerce/mixins.py (4)
AsyncRenderMixin(68-81)AsyncTerminateMixin(52-65)RenderMixin(20-33)TerminateMixin(4-17)
🪛 Ruff (0.14.8)
mpt_api_client/resources/commerce/orders_asset.py
34-34: Unused noqa directive (unknown: WPS215)
Remove unused noqa directive
(RUF100)
mpt_api_client/resources/commerce/subscriptions.py
49-49: 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 (22)
tests/unit/resources/commerce/test_orders.py (1)
238-249: LGTM!The addition of "render" and "template" to the mixin presence tests correctly verifies that
OrdersServiceandAsyncOrdersServicenow expose these capabilities via mixins. This aligns with the broader refactoring pattern observed across the codebase.mpt_api_client/resources/commerce/agreements.py (3)
13-18: LGTM!Clean import of the new mixins from the commerce mixins module.
33-40: LGTM!
AgreementsServicecorrectly inheritsRenderMixin[Agreement]andTemplateMixin[Agreement], following the established pattern for mixin-based functionality. The MRO is correct with mixins placed before theServicebase class.
58-65: LGTM!
AsyncAgreementsServicecorrectly inheritsAsyncRenderMixin[Agreement]andAsyncTemplateMixin[Agreement], maintaining consistency with the synchronous counterpart.mpt_api_client/resources/commerce/assets.py (3)
13-18: LGTM!Appropriate imports for the asset-specific mixins (Terminate and Render, without Template).
37-48: LGTM!
AssetServicecorrectly inheritsTerminateMixin[Asset]andRenderMixin[Asset]. The MRO is appropriate with mixins positioned before theServicebase class.
50-59: LGTM!
AsyncAssetServicecorrectly mirrors the synchronous version with the async variants of the mixins.tests/unit/resources/commerce/test_mixins.py (4)
31-44: LGTM!
DummyRenderServiceandAsyncDummyRenderServicefollow the established pattern for mixin test scaffolding, consistent with the existingDummyTerminateServiceclasses.
57-64: LGTM!Fixtures correctly instantiate the render service classes with appropriate HTTP clients.
79-79: Good improvement: usingto_dict()for clearer assertions.This change makes the assertion more explicit by comparing dictionaries directly rather than relying on model equality.
129-158: LGTM!The render tests correctly:
- Mock GET requests (matching
RenderMixin._resource_do_requestdefault method)- Return
text/htmlcontent type- Assert that raw text content is returned
Good coverage for both sync and async variants.
mpt_api_client/resources/commerce/orders_asset.py (2)
9-9: LGTM!Import of
AsyncRenderMixinandRenderMixinfrom the commerce mixins module is correct and aligns with the refactoring objective.
24-31: LGTM!The mixin inheritance order is correct—
RenderMixin[OrdersAsset]is placed beforeService[OrdersAsset]in the MRO, ensuring cooperative multiple inheritance works properly.mpt_api_client/resources/commerce/orders.py (3)
12-17: LGTM!Imports for both synchronous and asynchronous render/template mixins are correctly added.
40-47: LGTM!
OrdersServicenow correctly inheritsRenderMixin[Order]andTemplateMixin[Order], following the established mixin pattern. The inheritance order is appropriate for Python's MRO.
145-152: LGTM!
AsyncOrdersServicecorrectly inheritsAsyncRenderMixin[Order]andAsyncTemplateMixin[Order], maintaining symmetry with the synchronous service.mpt_api_client/resources/commerce/subscriptions.py (2)
16-21: LGTM!Imports are correctly updated to include the new render mixins alongside the existing terminate mixins.
36-46: LGTM!
SubscriptionsServicecorrectly includesRenderMixin[Subscription]in its inheritance chain. The mixin ordering is appropriate.mpt_api_client/resources/commerce/mixins.py (4)
20-33: LGTM!
RenderMixinis well-implemented following the established pattern fromTerminateMixin. The method correctly delegates to_resource_do_requestwithaction="render"and returns the text content.
36-49: LGTM!
TemplateMixincorrectly mirrorsRenderMixinwithaction="template". Documentation is clear and consistent.
68-81: LGTM!
AsyncRenderMixincorrectly implements the async version withawaiton_resource_do_request.
84-97: LGTM!
AsyncTemplateMixincompletes the mixin family, providing async template retrieval capability. All four new mixins are consistently implemented.
robcsegal
force-pushed
the
MPT-16423-move-render-to-mixin-in-commerce
branch
from
c7c06b6 to
4df4aba
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/resources/commerce/test_agreements.py (1)
67-82: Async mixin presence test is instantiating the wrong serviceIn
test_async_mixins_presentyou’re still constructingAgreementsService:service = AgreementsService(http_client=async_http_client)so this test only re-checks the sync service and never verifies that
AsyncAgreementsServiceactually exposescreate,update,get,render, andtemplate. It also wires an async client into a sync service, which is conceptually off even though the test only callshasattr.You likely want to instantiate the async service here:
@pytest.mark.parametrize("method", ["create", "update", "get", "render", "template"]) def test_async_mixins_present(async_http_client, method): - service = AgreementsService(http_client=async_http_client) + service = AsyncAgreementsService(http_client=async_http_client) result = hasattr(service, method) assert result is TrueThis will properly guard the async agreements API against regressions in mixin wiring while preserving the existing pytest-asyncio pattern of a synchronous test function using async fixtures without
await.
🧹 Nitpick comments (3)
mpt_api_client/resources/commerce/subscriptions.py (1)
16-21: Render mixins are wired correctly; consider cleaning up obsoletenoqaThe addition of
RenderMixin[Subscription]andAsyncRenderMixin[Subscription]and their placement beforeService/AsyncServicelook correct and keep_resource_do_requestresolution intact.Ruff is flagging the
# noqa: WPS215onAsyncSubscriptionsServiceas an unknown/unused code; if WPS checks are no longer enabled, it’s worth dropping that directive (and similar ones) to avoid RUF100 noise.Also applies to: 36-45, 49-58
tests/unit/resources/commerce/test_mixins.py (1)
7-11: Render/terminate mixin tests are well-factored; optional: assert HTTP call detailsThe new
DummyRenderService/AsyncDummyRenderServiceplus fixtures nicely mirror real services and give focused coverage ofRenderMixin/AsyncRenderMixin. Updating terminate tests to compareresult.to_dict()againstdummy_expectedis consistent with how other tests exercise model responses, and the new render tests correctly exercise the/renderendpoint and assert on the returned HTML.If you want slightly stronger coverage, you could also assert
call_countand the requested URL/method intest_renderandtest_async_render, similar to other tests in this suite, but the current checks are already sufficient for behavior verification.Also applies to: 31-45, 57-65, 67-127, 129-158
mpt_api_client/resources/commerce/orders_asset.py (1)
9-10: Orders asset render mixins are wired correctly; consider removing stalenoqaComposing
RenderMixin[OrdersAsset]andAsyncRenderMixin[OrdersAsset]into the sync/async asset services aligns them with the rest of the commerce API and leverages the shared_resource_do_requestimplementation viaService/AsyncService.Ruff is flagging
# noqa: WPS215onAsyncOrdersAssetServiceas an unknown/unused ignore; if WPS codes are no longer in use, it’s worth dropping that directive to keep lint output clean.Also applies to: 24-31, 34-40
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
mpt_api_client/resources/commerce/agreements.py(3 hunks)mpt_api_client/resources/commerce/assets.py(2 hunks)mpt_api_client/resources/commerce/mixins.py(2 hunks)mpt_api_client/resources/commerce/orders.py(3 hunks)mpt_api_client/resources/commerce/orders_asset.py(2 hunks)mpt_api_client/resources/commerce/subscriptions.py(2 hunks)tests/unit/resources/commerce/test_agreements.py(2 hunks)tests/unit/resources/commerce/test_assets.py(0 hunks)tests/unit/resources/commerce/test_mixins.py(7 hunks)tests/unit/resources/commerce/test_orders.py(1 hunks)tests/unit/resources/commerce/test_orders_asset.py(0 hunks)
💤 Files with no reviewable changes (2)
- tests/unit/resources/commerce/test_assets.py
- tests/unit/resources/commerce/test_orders_asset.py
🚧 Files skipped from review as they are similar to previous changes (1)
- mpt_api_client/resources/commerce/mixins.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/unit/resources/commerce/test_mixins.pytests/unit/resources/commerce/test_orders.pytests/unit/resources/commerce/test_agreements.py
🧬 Code graph analysis (3)
tests/unit/resources/commerce/test_mixins.py (1)
mpt_api_client/resources/commerce/mixins.py (4)
AsyncRenderMixin(68-81)RenderMixin(20-33)render(23-33)render(71-81)
mpt_api_client/resources/commerce/orders.py (2)
mpt_api_client/mpt_client.py (2)
commerce(49-51)commerce(98-105)mpt_api_client/resources/commerce/mixins.py (4)
AsyncRenderMixin(68-81)AsyncTemplateMixin(84-97)RenderMixin(20-33)TemplateMixin(36-49)
mpt_api_client/resources/commerce/subscriptions.py (3)
mpt_api_client/resources/commerce/mixins.py (4)
AsyncRenderMixin(68-81)AsyncTerminateMixin(52-65)RenderMixin(20-33)TerminateMixin(4-17)mpt_api_client/http/service.py (1)
Service(12-80)mpt_api_client/http/mixins.py (2)
AsyncCreateMixin(259-270)AsyncGetMixin(380-395)
🪛 Ruff (0.14.8)
mpt_api_client/resources/commerce/orders_asset.py
34-34: Unused noqa directive (unknown: WPS215)
Remove unused noqa directive
(RUF100)
mpt_api_client/resources/commerce/subscriptions.py
49-49: 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 (5)
tests/unit/resources/commerce/test_orders.py (2)
238-243: Sync OrdersService now correctly asserts render/template mixin surfaceIncluding
"render"and"template"in the parametrizedmethodlist keeps this presence check aligned with the new mixin-based API and provides a simple regression guard that the methods stay exposed onOrdersService.
245-249: Async OrdersService presence test matches new async mixin APIMirroring the sync test for
"render"and"template"onAsyncOrdersServiceis consistent with the mixin change and correctly uses a synchronous test function with an async fixture and noawait, per the established pytest-asyncio pattern.mpt_api_client/resources/commerce/assets.py (1)
13-18: Asset services correctly migrated to terminate/render mixinsImporting and composing
TerminateMixin/RenderMixin(and their async counterparts) intoAssetServiceandAsyncAssetServicekeeps the public API intact while centralizing behavior in the shared commerce mixins. The base-class ordering is appropriate for MRO and reuse of_resource_action/_resource_do_request.Also applies to: 37-46, 50-59
mpt_api_client/resources/commerce/orders.py (1)
12-17: Orders services correctly migrated to shared render/template mixinsImporting and composing
RenderMixin/TemplateMixin(and their async counterparts) intoOrdersServiceandAsyncOrdersServicecleanly centralizes render/template behavior while preserving the existing API. The base-class ordering keeps_resource_do_requestresolution intact, and the updated tests for mixin presence plus the dedicated mixin tests provide good coverage of these changes.Also applies to: 40-47, 145-152
mpt_api_client/resources/commerce/agreements.py (1)
13-18: Agreements services now consistently use shared render/template mixinsAdding
RenderMixin/TemplateMixinand their async equivalents toAgreementsServiceandAsyncAgreementsServicebrings agreements in line with the rest of the commerce API, centralizing render/template behavior while leaving attachments handling untouched. The base-class ordering looks correct for mixin resolution, and the updated tests cover both template behavior and mixin presence.Also applies to: 33-40, 58-65
|
Moved Render and Template to mixin https://softwareone.atlassian.net/browse/MPT-16423 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **New Features** - Standardized render and template endpoints added uniformly (sync + async) across commerce services via shared mixins. * **Refactor** - Replaced per-service render/template implementations with centralized mixin-based behavior for agreements, assets, orders, subscriptions, and order-assets. * **Tests** - Updated test suite to validate mixin-provided render/template capabilities and removed duplicated per-service render/template tests. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Moved Render and Template to mixin
https://softwareone.atlassian.net/browse/MPT-16423
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.