MPT-15844 Add catalog product items service #145
Conversation
albertsola
requested review from
alephsur,
d3rky,
jentyk,
robcsegal,
ruben-sebrango and
svazquezco
as code owners
WalkthroughAdds product-item-level services and accessors to the catalog client (sync and async), introduces a new product-items service module with tests, and increases default HTTP client timeouts from 5.0s to 10.0s. 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 due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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 (2)
Comment |
✅ Found Jira issue key in the title: MPT-15844 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: 0
🧹 Nitpick comments (3)
tests/unit/resources/catalog/test_products_items.py (1)
21-30: Consider simplifying the assertions.The tests correctly verify the endpoint paths, but the assertion pattern is verbose.
Apply this diff to simplify:
def test_endpoint(product_items_service): - result = product_items_service.path == "/public/v1/catalog/products/PRD-001/items" - - assert result is True + assert product_items_service.path == "/public/v1/catalog/products/PRD-001/items" def test_async_endpoint(async_product_items_service): - result = async_product_items_service.path == "/public/v1/catalog/products/PRD-001/items" - - assert result is True + assert async_product_items_service.path == "/public/v1/catalog/products/PRD-001/items"mpt_api_client/resources/catalog/products.py (2)
79-83: Remove unused noqa directive.The implementation correctly follows the established pattern for sub-service accessors. However, the
noqa: WPS110directive is flagged as unused by Ruff.Based on static analysis hints, apply this diff:
- def items(self, product_id: str) -> ProductItemService: # noqa: WPS110 + def items(self, product_id: str) -> ProductItemService: """Return product items service.""" return ProductItemService( http_client=self.http_client, endpoint_params={"product_id": product_id} )If the WPS110 suppression is still needed for wemake-python-styleguide, consider configuring Ruff to recognize it or adjusting your linter setup.
142-146: Remove unused noqa directive.The async implementation correctly mirrors the synchronous version. However, the
noqa: WPS110directive is flagged as unused by Ruff.Based on static analysis hints, apply this diff:
- def items(self, product_id: str) -> AsyncProductItemService: # noqa: WPS110 + def items(self, product_id: str) -> AsyncProductItemService: """Return product items service.""" return AsyncProductItemService( http_client=self.http_client, endpoint_params={"product_id": product_id} )If the WPS110 suppression is still needed for wemake-python-styleguide, consider configuring Ruff to recognize it or adjusting your linter setup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
mpt_api_client/resources/catalog/products.py(3 hunks)mpt_api_client/resources/catalog/products_items.py(1 hunks)tests/unit/resources/catalog/test_products.py(3 hunks)tests/unit/resources/catalog/test_products_items.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/unit/resources/catalog/test_products.py (2)
tests/unit/resources/catalog/test_catalog.py (1)
catalog(26-27)mpt_api_client/resources/catalog/products_items.py (2)
AsyncProductItemService(28-34)ProductItemService(19-25)
tests/unit/resources/catalog/test_products_items.py (3)
mpt_api_client/resources/catalog/products_items.py (2)
AsyncProductItemService(28-34)ProductItemService(19-25)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/resources/catalog/products.py (3)
mpt_api_client/resources/catalog/products_items.py (2)
AsyncProductItemService(28-34)ProductItemService(19-25)mpt_api_client/resources/catalog/price_lists.py (2)
items(35-39)items(50-54)mpt_api_client/resources/catalog/catalog.py (2)
items(60-62)items(102-104)
mpt_api_client/resources/catalog/products_items.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)AsyncGetMixin(380-395)CollectionMixin(469-535)GetMixin(361-377)mpt_api_client/resources/catalog/products.py (2)
items(79-83)items(142-146)mpt_api_client/resources/catalog/items.py (1)
Item(15-16)
🪛 Ruff (0.14.7)
mpt_api_client/resources/catalog/products.py
79-79: Unused noqa directive (unknown: WPS110)
Remove unused noqa directive
(RUF100)
142-142: 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 (10)
tests/unit/resources/catalog/test_products.py (2)
18-21: LGTM!The imports are correctly added and follow the established import structure in the test file.
73-73: LGTM!The test cases correctly verify that the new
itemsaccessor returns the appropriate service classes and passes the product_id parameter correctly.Also applies to: 93-93
tests/unit/resources/catalog/test_products_items.py (3)
1-6: LGTM!The imports are clean and include only what's necessary for testing the product items services.
9-18: LGTM!The fixtures correctly instantiate the services with the required endpoint_params for testing.
33-44: LGTM!The parameterized tests correctly verify that both sync and async services expose the expected methods from their mixins.
mpt_api_client/resources/catalog/products.py (1)
31-34: LGTM!The imports are correctly added and follow the alphabetical ordering of the products_* modules.
mpt_api_client/resources/catalog/products_items.py (4)
1-8: LGTM!The imports are clean and include all necessary components: service base classes, mixins for retrieval and iteration, and the Item model.
11-16: LGTM!The service configuration correctly defines the endpoint template with the product_id parameter, uses the Item model class, and follows the standard "data" collection key convention.
19-25: LGTM!The ProductItemService class correctly combines the necessary mixins (GetMixin for individual retrieval, CollectionMixin for iteration) with the Service base class and configuration.
28-34: LGTM!The AsyncProductItemService class correctly mirrors the synchronous version using async mixins and base classes, maintaining consistency with the async service patterns in the codebase.
albertsola
force-pushed
the
MPT-15844-catalog-product-items-service
branch
from
0088838 to
9379b07
Compare
| *, | ||
| base_url: str | None = None, | ||
| api_token: str | None = None, | ||
| timeout: float = 5.0, |
There was a problem hiding this comment.
Created a ticket to revert this back here:
|
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.