fix(core-flows): set SalesChannels on Product update #7272
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
🦋 Changeset detectedLatest commit: 5b19b01 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
fPolic
changed the title
packages/core/core-flows/src/sales-channel/steps/set-products-sales-channels.ts
Outdated
Show resolved
Hide resolved
packages/core/core-flows/src/product/workflows/update-products.ts
Outdated
Show resolved
Hide resolved
packages/core/core-flows/src/product/workflows/update-products.ts
Outdated
Show resolved
Hide resolved
packages/core/core-flows/src/product/steps/get-product-sales-channel-link.ts
Outdated
Show resolved
Hide resolved
packages/core/core-flows/src/product/workflows/update-products.ts
Outdated
Show resolved
Hide resolved
adrien2p
force-pushed
the
feat/product-sales-channel-update-link
branch
from
db67c48
to
2d21e2e
Compare
| products: ProductTypes.UpsertProductDTO[] | ||
| } | ||
| | UpdateProductsStepInputSelector | ||
| | UpdateProductsStepInputProducts | ||
|
|
||
| type WorkflowInput = UpdateProductsStepInput |
There was a problem hiding this comment.
Q: We have a mix of workflow types in the types package and in the core-flows package, it will require a cleanup at some point, not now but just raising it. we also have different places in the types package where the workflow types are define, workflows, workflow, and in some dir. maybe we can create a ticket
There was a problem hiding this comment.
prepareUpdateProductInput, updateProductIds, and prepareToDeleteLinks are all named as if they were generic transformers for updating products, but they are coupled with sales channel management as far as I can tell. So what I mean is, if we add another link to product, we will need to refactor these to account for that.
There was a problem hiding this comment.
Oh sorry maybe i ve named them wrongly, but we can rework that, i just based the name on the workflow step they were used for. I didnt have a better idea at the moment but the main thing was to extract them (not necessarily generic) to have a better readability of the workflow and transformers themselves
There was a problem hiding this comment.
Also, those transformers are not exported they are only local so very specific here, i am fine with finding another name or rework the implementation a bit
|
@fPolic I have done a commit proposal, can I get you to look at it and let me know what you think |
adrien2p
force-pushed
the
feat/product-sales-channel-update-link
branch
from
2d21e2e
to
1eb0529
Compare
packages/core/core-flows/src/product/workflows/update-products.ts
Outdated
Show resolved
Hide resolved
packages/core/core-flows/src/product/workflows/update-products.ts
Outdated
Show resolved
Hide resolved
| ) | ||
|
|
||
| const currentLinks = useRemoteQueryStep({ | ||
| entry_point: "product_sales_channel", |
There was a problem hiding this comment.
suggestion: service: Links.productSalesChannel
There was a problem hiding this comment.
just saw that it requires a little adaptation of that step but it should be good to have so that we don't rely on the string for the link themselves wdyt?
packages/core/core-flows/src/product/workflows/update-products.ts
Outdated
Show resolved
Hide resolved
| // TODO Update sales channel links | ||
| return updateProductsStep(input) | ||
|
|
||
| const toUpdateInput = transform({ input }, prepareUpdateProductInput) |
There was a problem hiding this comment.
thought: I don't know if I like, that the steps that were added here are so specific to sales channel management. E.g. as soon as we add another link to products, we will need to revisit all of these. Or create separate steps.
There was a problem hiding this comment.
I need to relook into it but your comment is on the transformer, are you referring to the transformer of the next step or the step itself just to be sure
|
Should we merge this and revisit later? |
|
It is fine to me, @fPolic wdyt? |
|
Sounds good! |
WHAT