fix(medusa): Shipping profile CRUD #3154
Conversation
🦋 Changeset detectedLatest commit: f719693 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 |
kasperkristensen
changed the title
| await productServiceTx.update(pId, { | ||
| profile_id: profileId, | ||
| }) |
There was a problem hiding this comment.
thought(non-blocking): Would it make sense to create a dedicated method to handle bulk updating in the service or repo layer? I imagine this operation can get nasty when adding > 10000 products, which is likely to happen when creating new profiles.
There was a problem hiding this comment.
I think that would make sense, as the current way does not scale. I will add a method to the ProductService that can run the update in a single query instead. 😄
There was a problem hiding this comment.
I would suggest to do like we do when we migrate the other methods to accept a collection. Like in the cart, line items etc. The update method could accept an id or a collection of ids so it is aligned with the rest of the codebase. Otherwise in some places we will have two different methods and other places one method. A method that handle a collection can handle a collection of one item as well.
There was a problem hiding this comment.
@adrien2p I think we would need to change up the update product method a bit in order for it to work with more than one product at a time, with regards to how it can be used to update nested relations such as variants.
While it wouldn't be a breaking change as we can keep the endpoint as is for now, we would open up for people to use the update method in a way that would break. We would need to make the update method accept a map of product ids and updates in order for it to work with variants, to make sure the nested updates only apply to the correct product, which would definitely be a breaking change IMO. So I think it's out of scope for this PR.
Nvm just saw that we've removed variants creation/updates from the product method. Still think my point is somewhat valid, in regards to stuff like images and thumbnails as it would overwrite images and thumbnails for all updated products, unless we allowed the users to pass a map, with images they want to keep for each product. But if you think it's a good idea I am open to it @olivermrbl @adrien2p
I've pushed the originally suggested changes for now
What
POST /admin/shipping-profilesPOST /admin/shipping-profiles/:idprofile_idtoShippingOptionService.update()would not update the shipping profile of the option.Testing
simpleshippingProfileFactoryResolves CORE-1065