-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
feat(core-flows, promotion): limit promotion use per promo code #13693
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
base: develop
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 8 Skipped Deployments
|
"title": "Usage", | ||
"description": "Set a limit on how many times the promotion can be used." | ||
}, | ||
"use_by_attribute": { |
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.
Why snake case?
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.
EDIT: some properties are snake and others are camel... huh haha
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.
good point, will fix that
? t("campaigns.budget.fields.limitBudgetAttributeCustomer") | ||
: t("campaigns.budget.fields.limitBudgetAttributeEmail") | ||
? t("campaigns.budget.fields.totalUsedByAttribute", { | ||
attribute: campaign.budget?.attribute, |
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.
The attribute here will always be in English even if we get the translation from another language right?
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.
it is the name of the attribute, it can be anything user defines in theory
const attributeValue = ( | ||
attribute === "promotion_code" | ||
? registrationContext[attribute] | ||
: promotion.code | ||
) as string | null |
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.
thought: Tbh. I am not a huge fan of this; it feels dirty to have hard-coded exceptions like this in the code.
I think we should consider adding a limit directly on the promotion as an alternative solution. That limit could be combined with a campaign budget to solve this use case.
I am curious to hear what you and others think @medusajs/os
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.
For context, the use case we are trying to solve here is:
Some of our users would like to create a single campaign called Customer Service Promos under which their team can create one-time promotions for individual customers. However, the current implementation prevents this because the limit applies at the campaign level rather than per promotion. They are once again left with creating a new campaign for recurring customers (i.e. customers that have already received a code from the campaign in the past).
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.
Yeah wasn't happy with that either.
Wdyt about modifying CampaignBudgetUsageContext
to have scopes that are applied per promotion, for example:
export type CampaignBudgetUsageContext = {
/** applied on all promotions **/
customer_id: string | null
customer_email: string | null
/** applied only on matching promotions **/
promotions_context: {
[promo_id]: {
code: string | null
}
}
}
Then, when we iterate over promotions when computing actions or registering usage, we merge the context that is scoped for that promotion?
6653df7
to
cf767f1
Compare
cf767f1
to
2c5985a
Compare
What