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

fix(medusa): update create fulfillment flow #3172

Merged
merged 15 commits into from
Feb 28, 2023
Merged

Conversation

pKorsholm
Copy link
Contributor

@pKorsholm pKorsholm commented Feb 3, 2023

What

  • Await adjustments made to reservations and inventory items
  • use correct map methods and db field

Why

  • An error thrown during inventory adjustment when creating a fulfillment would not necessarily cause a rollback
  • The code in pvinventoryService referenced some older fields for the product_variant_inventory table + used the wrong way to get map values

@changeset-bot
Copy link

changeset-bot bot commented Feb 3, 2023

🦋 Changeset detected

Latest commit: a8ebac2

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

This PR includes changesets to release 6 packages
Name Type
@medusajs/medusa Minor
@medusajs/inventory Major
@medusajs/medusa-js Major
medusa-plugin-restock-notification Major
medusa-react Major
@medusajs/stock-location Major

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 changed the title update create fulfillment flow fix(medusa): update create fulfillment flow Feb 6, 2023
@pKorsholm pKorsholm changed the base branch from chore/integration-tests-stock-inventory to develop February 6, 2023 13:47
@pKorsholm pKorsholm marked this pull request as ready for review February 7, 2023 08:37
@pKorsholm pKorsholm requested a review from a team as a code owner February 7, 2023 08:37
Copy link
Contributor

@carlos-r-l-rodrigues carlos-r-l-rodrigues left a comment

Choose a reason for hiding this comment

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

LGTM

non-blocking: We could add an integration test to this flow

})
invItemId = invItem.id

await prodVarInventoryService.attachInventoryItem(variantId, invItem.id)
Copy link
Member

Choose a reason for hiding this comment

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

Question: Deos that mean that the SKU that is used to create the inventory item can be different than the variant SKU? are they meaning different thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you will still have access to all the variants stock related fields, however they wont impact behavior if you use the inventory module.

@olivermrbl
Copy link
Contributor

@pKorsholm Please resolve conflicts when time allows for it :)

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!

@olivermrbl
Copy link
Contributor

Pending @adrien2p's approval :)

Copy link
Member

@adrien2p adrien2p left a comment

Choose a reason for hiding this comment

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

LGTM, one minore todo if needed

Comment on lines +106 to +129
if (isDefined(data.sales_channel)) {
let sc

if (isString(data.sales_channel)) {
sc = await manager.findOne(SalesChannel, {
where: { id: data.sales_channel },
})
}

if (!sc) {
sc = await simpleSalesChannelFactory(
connection,
isString(data.sales_channel)
? { id: data.sales_channel }
: data.sales_channel
)
}

sc_id = sc.id
}

if (sc_id) {
toCreate.sales_channel_id = sc_id
}
Copy link
Member

Choose a reason for hiding this comment

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

todo: if data.sales_channelis defined but is not a string, we don't do anything. I am missing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if data.sales_channel is defined but not a string we pass it into the sales channel factory on line 120

Copy link
Member

Choose a reason for hiding this comment

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

LOL, I did not see the not in the if constraint. I need to clean my glasses

Copy link
Member

@adrien2p adrien2p left a comment

Choose a reason for hiding this comment

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

LGTM

@pKorsholm pKorsholm merged commit 5eb61fa into develop Feb 28, 2023
adrien2p pushed a commit that referenced this pull request Mar 1, 2023
* update create fulfillment flow

* move transaction service creation close to where it's used

* integration tests

* fix feedback

* use transformBody

* add changeset
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants