MPT-16325 Publish product during the seeding process #159
Conversation
albertsola
requested review from
alephsur,
d3rky,
jentyk,
robcsegal,
ruben-sebrango and
svazquezco
as code owners
WalkthroughAdds an async product publication step: Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant publish_product
participant VendorClient
participant OpsClient
Caller->>publish_product: publish_product(context, vendor, ops)
publish_product->>VendorClient: get(product_id)
VendorClient-->>publish_product: product (status)
alt status == Draft
publish_product->>VendorClient: review(product_id)
VendorClient-->>publish_product: reviewed (status -> Pending)
publish_product->>OpsClient: publish(product_id)
OpsClient-->>publish_product: published (status -> Published)
else status == Pending or Unpublished
publish_product->>OpsClient: publish(product_id)
OpsClient-->>publish_product: published (status -> Published)
else status == Published
note right of publish_product: no action required
end
publish_product->>publish_product: validate final status == Published
alt final == Published
publish_product-->>Caller: return success
else
publish_product-->>Caller: raise error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
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 |
albertsola
force-pushed
the
MPT-16325-Product-listing-seeding-fail-due-to-product-not-being-published
branch
from
ed72157 to
d8cd2ac
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: 1
🧹 Nitpick comments (3)
tests/seed/catalog/test_product.py (1)
32-46: Test design consideration: using the same client for both vendor and operations.The test passes
vendor_clientfor both thempt_vendorandmpt_operationsparameters (line 41), which doesn't reflect the production scenario where different clients with different permissions are used. While this is acceptable for unit testing the workflow logic, consider whether an integration test with separate client instances would provide additional confidence that the operations client is properly configured.seed/catalog/product.py (2)
60-67: Consider adding logging for state transitions.The function performs important state transitions (Draft → Review → Pending → Published) but doesn't log these operations. For a seeding process, logging state transitions would help with debugging and monitoring progress.
For example:
logger.debug(f"Publishing product {product_id}, current status: {product.status}") if product.status == "Draft": logger.debug(f"Submitting product {product_id} for review") product = await mpt_vendor.catalog.products.review(product_id) if product.status in {"Pending", "Unpublished"}: logger.debug(f"Publishing product {product_id}") product = await mpt_operations.catalog.products.publish(product_id) logger.debug(f"Product {product_id} published successfully")
67-67: Consider defining a custom exception class.The static analysis tool suggests avoiding long inline exception messages. While this is a minor style issue, defining a custom exception class would improve consistency and make error handling more maintainable.
For example, you could define:
class ProductPublishError(RuntimeError): """Raised when product publication fails.""" def __init__(self, product_id: str, status: str): super().__init__(f"Product {product_id} is not published (status: {status})") self.product_id = product_id self.status = statusThen use:
raise ProductPublishError(product_id, product.status)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
seed/catalog/product.py(2 hunks)seed/seed_api.py(1 hunks)tests/seed/catalog/test_product.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
seed/catalog/product.py (4)
seed/context.py (1)
Context(12-33)seed/container.py (1)
Container(7-30)seed/helper.py (1)
require_context_id(23-40)mpt_api_client/resources/catalog/mixins.py (2)
publish(25-34)publish(62-71)
seed/seed_api.py (1)
tests/e2e/conftest.py (1)
logger(63-64)
🪛 Ruff (0.14.8)
seed/catalog/product.py
67-67: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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)
seed/seed_api.py (1)
23-23: LGTM!The completion log message appropriately indicates successful seeding and is correctly placed after both seeding operations complete.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
seed/catalog/product.py (2)
41-41: This call will fail due to missing@injectdecorator onpublish_product.The
publish_product()function is invoked without arguments, expecting dependency injection to providecontext,mpt_vendor, andmpt_operations. However, the function definition at line 54 is missing the@injectdecorator, causing aTypeErrorat runtime.
54-58: Critical: Missing@injectdecorator causes runtime failure.As flagged in the previous review, the function uses dependency injection with
Provide[Container.*]defaults but lacks the@injectdecorator. This prevents the DI framework from injecting dependencies when called at line 41, resulting in aTypeError.Apply this diff:
+@inject async def publish_product( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], mpt_operations: AsyncMPTClient = Provide[Container.mpt_operations], ) -> None:
🧹 Nitpick comments (1)
seed/catalog/product.py (1)
67-67: Optional: Consider extracting error message to a constant.Static analysis suggests avoiding inline error messages in exception raises for better maintainability and consistency.
Define a constant at module level:
PRODUCT_NOT_PUBLISHED_ERROR = "Product {product_id} is not published"Then use it:
- raise RuntimeError(f"Product {product_id} is not published") + raise RuntimeError(PRODUCT_NOT_PUBLISHED_ERROR.format(product_id=product_id))As per static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
seed/catalog/product.py(2 hunks)seed/seed_api.py(1 hunks)tests/seed/catalog/test_product.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/seed/catalog/test_product.py
🧰 Additional context used
🧬 Code graph analysis (1)
seed/seed_api.py (1)
tests/e2e/conftest.py (1)
logger(63-64)
🪛 Ruff (0.14.8)
seed/catalog/product.py
67-67: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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)
seed/seed_api.py (1)
23-23: LGTM!The success log provides clear feedback that the seeding process completed without errors, complementing the existing exception logging on line 25.
seed/catalog/product.py (1)
62-67: The product state transitions are well-defined in the API client. Thereview()method explicitly transitions to"Pending"(as documented inmpt_api_client/resources/catalog/mixins.py), andpublish()transitions to"Published". The codebase only defines four product states:Draft,Pending,Published, andUnpublished. No intermediate states like"UnderReview"or"Rejected"exist, so the state flow logic in lines 62–67 correctly handles all possible product states without risk of skipped steps.
albertsola
force-pushed
the
MPT-16325-Product-listing-seeding-fail-due-to-product-not-being-published
branch
from
d8cd2ac to
ef9fe59
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
seed/catalog/product.py (1)
54-69: LGTM: Solid publication workflow with correct dependency injection.The
@injectdecorator is now present (addressing the previous critical review), and the state transition logic is sound:
- Fetches the product and advances through Draft→Review→Pending/Unpublished→Publish as needed
- Final status validation ensures the product reaches Published state
- Idempotent design handles already-published products gracefully
Optional: Add debug logging for observability.
Consider adding logger.debug statements at key transitions (e.g., "Reviewing product {product_id}", "Publishing product {product_id}") to aid debugging if the seeding fails at the publication step.
async def publish_product( context: Context = Provide[Container.context], mpt_vendor: AsyncMPTClient = Provide[Container.mpt_vendor], mpt_operations: AsyncMPTClient = Provide[Container.mpt_operations], ) -> None: """Publish product.""" product_id = require_context_id(context, "catalog.product.id", "publish product") + logger.debug("Publishing product %s ...", product_id) product = await mpt_vendor.catalog.products.get(product_id) if product.status == "Draft": + logger.debug("Reviewing product %s (status: Draft)", product_id) product = await mpt_vendor.catalog.products.review(product_id) if product.status in {"Pending", "Unpublished"}: + logger.debug("Publishing product %s (status: %s)", product_id, product.status) product = await mpt_operations.catalog.products.publish(product_id) if product.status != "Published": raise RuntimeError(f"Product {product_id} is not published") + logger.debug("Product %s published successfully", product_id)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
seed/catalog/product.py(2 hunks)seed/seed_api.py(1 hunks)tests/seed/catalog/test_product.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/seed/catalog/test_product.py
🧰 Additional context used
🧬 Code graph analysis (1)
seed/seed_api.py (1)
tests/e2e/conftest.py (1)
logger(63-64)
🪛 Ruff (0.14.8)
seed/catalog/product.py
68-68: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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)
seed/seed_api.py (1)
23-23: LGTM: Useful success logging added.The log statement correctly indicates successful completion of the seeding process and is appropriately placed within the try block after all operations succeed.
seed/catalog/product.py (1)
41-41: LGTM: Product publishing integrated into seeding flow.The call to
publish_product()correctly leverages dependency injection and appropriately sequences the publication step immediately after product creation.
|
| """Publish product.""" | ||
| product_id = require_context_id(context, "catalog.product.id", "publish product") | ||
| product = await mpt_vendor.catalog.products.get(product_id) | ||
| if product.status == "Draft": |
There was a problem hiding this comment.
Should we use a Enum for those statuses?
| mpt_operations: AsyncMPTClient = Provide[Container.mpt_operations], | ||
| ) -> None: | ||
| """Publish product.""" | ||
| product_id = require_context_id(context, "catalog.product.id", "publish product") |
There was a problem hiding this comment.
Why not use an ActionEnum instead of passing a str? It would avoid having different str for the same action, I can see different patterns like starting with uppercase and other with lowercase
albertsola
deleted the
MPT-16325-Product-listing-seeding-fail-due-to-product-not-being-published
branch
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.