Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(medusa): Add ProductVariantInventoryService #2883

Merged
merged 41 commits into from
Dec 30, 2022

Conversation

pKorsholm
Copy link
Contributor

@pKorsholm pKorsholm commented Dec 22, 2022

What

  • Add services:
    • sales-channel-inventory
    • sales-channel-location
    • product-variant-inventory
  • remove inventoryService
  • adjust remaining services to match

picks out the core parts of #2459
Fixes CORE-914, CORE-912

@pKorsholm pKorsholm requested a review from a team as a code owner December 22, 2022 15:20
@changeset-bot
Copy link

changeset-bot bot commented Dec 22, 2022

🦋 Changeset detected

Latest commit: a18936c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@medusajs/medusa Patch

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

@pKorsholm pKorsholm marked this pull request as draft December 22, 2022 15:21
@pKorsholm pKorsholm force-pushed the feat/add-product-variant-inventory-service branch from c791cca to cf5063d Compare December 22, 2022 15:22
@pKorsholm pKorsholm requested a review from srindom December 27, 2022 13:34
Copy link
Collaborator

@srindom srindom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Mostly looks good had a few nits and thoughts that are non blocking - otherwise just a bit of docs.

packages/medusa/src/models/store.ts Show resolved Hide resolved
packages/medusa/src/services/cart.ts Outdated Show resolved Hide resolved
packages/medusa/src/services/cart.ts Outdated Show resolved Hide resolved
packages/medusa/src/services/cart.ts Outdated Show resolved Hide resolved
packages/medusa/src/services/cart.ts Outdated Show resolved Hide resolved
packages/medusa/src/services/cart.ts Outdated Show resolved Hide resolved
packages/medusa/src/services/product-variant-inventory.ts Outdated Show resolved Hide resolved
packages/medusa/src/services/order.ts Outdated Show resolved Hide resolved
@@ -14,8 +14,14 @@ import {
AbstractCartCompletionStrategy,
CartCompletionResponse,
} from "../interfaces"
import {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought (this-file): this flow is great for @carlos-r-l-rodrigues' transaction orchestrator.

Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great piece of work! Have added a couple of suggestions and minor todos 💪

packages/medusa/src/services/cart.ts Outdated Show resolved Hide resolved
packages/medusa/src/services/cart.ts Outdated Show resolved Hide resolved
packages/medusa/src/services/cart.ts Outdated Show resolved Hide resolved
packages/medusa/src/services/product-variant-inventory.ts Outdated Show resolved Hide resolved
packages/medusa/src/services/sales-channel-inventory.ts Outdated Show resolved Hide resolved
packages/medusa/src/services/sales-channel-inventory.ts Outdated Show resolved Hide resolved
packages/medusa/src/services/sales-channel-location.ts Outdated Show resolved Hide resolved
packages/medusa/src/strategies/cart-completion.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - great work! Pending @srindom's approval :)

} else {
await lineItemServiceTx.delete(item.id)
return
return item
Copy link
Contributor

@carlos-r-l-rodrigues carlos-r-l-rodrigues Dec 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion to make this block easier to read

cart.items = (
  await Promise.all(
    cart.items.map(async (item) => {
      if (!item.variant_id) {
        return item
      }
      
      const availablePrice = await this.priceSelectionStrategy_
        .withTransaction(transactionManager)
        .calculateVariantPrice(item.variant_id, {
          region_id: region.id,
          currency_code: region.currency_code,
          quantity: item.quantity,
          customer_id: customer_id || cart.customer_id,
          include_discount_prices: true,
        })
        .catch(() => undefined)

      if (
        !isDefined(availablePrice) ||
        availablePrice.calculatedPrice === null
      ) {
        return await lineItemServiceTx.delete(item.id)
      }

      return await lineItemServiceTx.update(item.id, {
        has_shipping: false,
        unit_price: availablePrice.calculatedPrice,
      })
      
    })
  )
)

.withTransaction(manager)
.update(variantId, {
inventory_quantity: variant.inventory_quantity + quantity,
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion:
return here and remove the else block

@pKorsholm pKorsholm force-pushed the feat/add-product-variant-inventory-service branch from 4c8245b to b296651 Compare December 29, 2022 12:44
@pKorsholm pKorsholm force-pushed the feat/add-product-variant-inventory-service branch from d5f5a24 to b296651 Compare December 29, 2022 13:53
Copy link
Collaborator

@srindom srindom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM w. comments - also think @carlos-r-l-rodrigues' suggestions are worth considering including

Comment on lines +282 to +310
await Promise.all(
cart.items.map(async (item) => {
if (item.variant_id) {
const inventoryConfirmed =
await productVariantInventoryServiceTx.confirmInventory(
item.variant_id,
item.quantity,
{ salesChannelId: cart.sales_channel_id }
)

if (!inventoryConfirmed) {
throw new MedusaError(
MedusaError.Types.NOT_ALLOWED,
`Variant with id: ${item.variant_id} does not have the required inventory`,
MedusaError.Codes.INSUFFICIENT_INVENTORY
)
}

await productVariantInventoryServiceTx.reserveQuantity(
item.variant_id,
item.quantity,
{
lineItemId: item.id,
salesChannelId: cart.sales_channel_id,
}
)
}
})
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: this should probably go into its own step -- but let's not do anything, as we will refactor to the transaction orchestrator in any case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@olivermrbl olivermrbl changed the title Feat(medusa): add product variant inventory service feat(medusa): Add ProductVariantInventoryService Dec 30, 2022
@olivermrbl
Copy link
Contributor

Ready to merge? 💪

@pKorsholm pKorsholm merged commit b9680b6 into develop Dec 30, 2022
@pKorsholm pKorsholm deleted the feat/add-product-variant-inventory-service branch December 30, 2022 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants