-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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,pricing): Cart pricing context with customer group #10579
Conversation
… lists (#10493) *What* * Fixes #10490 * Expands any available customer_id into its customer_group_ids for cart updates that add line items. *Why* * Cart updates from the storefront were overriding any valid price lists that were correctly being shown in the storefront's product pages. *How* * Adds a new workflow step that expands an optional customer_id into the customer_group_ids it belongs to. * Uses this step in the addToCartWorkflow and updateLineItemInCartWorkflow workflows. *Testing* * Using medusa-dev to test on a local backend. * Adds integration tests for the addToCart and updateLineItemInCart workflows. Co-authored-by: Riqwan Thamir <rmthamir@gmail.com>
The latest updates on your projects. Learn more about Vercel for Git ↗︎
6 Skipped Deployments
|
🦋 Changeset detectedLatest commit: 713955b The changes in this PR will be included in the next version bump. This PR includes changesets to release 65 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 |
Well spotted ;) |
4d6345f
to
554e45c
Compare
@riqwan same situation here Order add line item WF |
@docloulou thanks, there are 10 different workflows in orders that needs to be updated, but that will come in a different PR. 🤞🏻 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, just a comment on the attribute change
import { confirmVariantInventoryWorkflow } from "./confirm-variant-inventory" | ||
import { refreshCartItemsWorkflow } from "./refresh-cart-items" | ||
|
||
const cartFields = cartFieldsForPricingContext.concat(["items.*"]) | ||
|
||
export const updateLineItemInCartWorkflowId = "update-line-item-in-cart" | ||
/** | ||
* This workflow updates a cart's line item. | ||
*/ | ||
export const updateLineItemInCartWorkflow = createWorkflow( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note to self: Input type change is a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, the filter utilities are a bit daunting to review, so primarily looked at the tests, and they seem good :)
@@ -0,0 +1,86 @@ | |||
import { isDefined } from "./is-defined" | |||
|
|||
export function filterObjectByKeys(obj, paths) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: A description and example here would be nice to have
[key: string]: any | ||
} | ||
|
||
export function flattenObjectToKeyValuePairs(obj: NestedObject): NestedObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same suggestion as the previous one
keys.forEach((key) => { | ||
const values = arrayObjects | ||
.map((item) => { | ||
if (item && typeof item === "object") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: for those type of check would the util isObject
be more convenient?
|
||
if (values.length > 0) { | ||
const newPath = `${pathStr}.${key}` | ||
if (values.every((v) => typeof v === "object" && !Array.isArray(v))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: should we use isObject?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adrien2p sorry, just saw this. Will open a new PR with these changes to get this PR into the release.
thanks @riqwan! do you know when a new version might get released with this fix? |
@sergiocampama today! |
what:
customer_group_id
rule is renamedcustomer.groups.id
in line with cart shapeThanks to initial contribution from @sergiocampama - #10493
RESOLVES CMRC-745