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(core-flows, types): update shipping methods upon cart ops #10382

Merged
merged 20 commits into from
Dec 8, 2024

Conversation

riqwan
Copy link
Contributor

@riqwan riqwan commented Dec 1, 2024

what:

  • when cart items change, the shipping methods are changed to reflect the new pricing

RESOLVES CMRC-752

Copy link

vercel bot commented Dec 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
api-reference 🔄 Building (Inspect) Visit Preview 💬 Add feedback Dec 8, 2024 0:50am
medusa-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 8, 2024 0:50am
5 Skipped Deployments
Name Status Preview Comments Updated (UTC)
api-reference-v2 ⬜️ Ignored (Inspect) Visit Preview Dec 8, 2024 0:50am
docs-ui ⬜️ Ignored (Inspect) Visit Preview Dec 8, 2024 0:50am
docs-v2 ⬜️ Ignored (Inspect) Visit Preview Dec 8, 2024 0:50am
medusa-docs ⬜️ Ignored (Inspect) Visit Preview Dec 8, 2024 0:50am
resources-docs ⬜️ Ignored (Inspect) Visit Preview Dec 8, 2024 0:50am

Copy link

changeset-bot bot commented Dec 1, 2024

⚠️ No Changeset found

Latest commit: 858edc9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@riqwan riqwan marked this pull request as ready for review December 2, 2024 16:17
@riqwan riqwan requested a review from a team as a code owner December 2, 2024 16:17
Co-authored-by: Oli Juhl <59018053+olivermrbl@users.noreply.github.com>
Co-authored-by: Oli Juhl <59018053+olivermrbl@users.noreply.github.com>
@@ -71,7 +88,7 @@ export const listShippingOptionsForCartWorkflow = createWorkflow(
)

const customerGroupIds = when({ cart }, ({ cart }) => {
return !!cart.id
return !!cart.customer_group_id
Copy link
Contributor

Choose a reason for hiding this comment

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

q: does the cart have a customer_group_id? 🤔

@@ -271,9 +258,11 @@ medusaIntegrationTestRunner({
expect.objectContaining({
id: shippingOption.id,
name: "Test shipping option",
amount: 500,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Think amount is still there right?

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 it is, will add that back.

@@ -39,6 +30,7 @@ export const listShippingOptionsForCartWorkflow = createWorkflow(
"id",
"sales_channel_id",
"currency_code",
"customer.groups.id",
Copy link
Contributor

Choose a reason for hiding this comment

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

question: If I am not mistaken, we have changed how customer group ID is passed in the pricing context – just want to make sure this is intentional.

We used to pass is as a top-level property:

{ ..., customer_group_id: "cusgrp_1234" }

Now, it seems we pass it via the cart:

{ cart: { customer: { groups: [ { id: "cusgrp_1234" } ] } } }

Is that correct, or have I missed something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was leak from the draft work I had for the changes related to the customer groups i was talking about. Will revert this.

All of our customer group rules are set as the attribute customer_group_id, which doesn't exist in the cart context. The second version you listed should be the actual rule path - customer.groups.id. This needs a data migration on price lists and prices, along with some changes in the pricing module, so thought will open a separate PR.

@riqwan riqwan merged commit 9e797dc into develop Dec 8, 2024
23 checks passed
@riqwan riqwan deleted the feat/cart-ops branch December 8, 2024 13:06
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.

2 participants