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(): added compute actions for buyget promotions #6255

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

riqwan
Copy link
Contributor

@riqwan riqwan commented Jan 29, 2024

what:

  • computes actions for buyget promotion (RESOLVES CORE-1700)

Copy link

changeset-bot bot commented Jan 29, 2024

⚠️ No Changeset found

Latest commit: 9abd7f1

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

Copy link

vercel bot commented Jan 29, 2024

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

Name Status Preview Comments Updated (UTC)
medusa-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 30, 2024 10:18am
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
api-reference ⬜️ Ignored (Inspect) Jan 30, 2024 10:18am
docs-ui ⬜️ Ignored (Inspect) Visit Preview Jan 30, 2024 10:18am
medusa-docs ⬜️ Ignored (Inspect) Visit Preview Jan 30, 2024 10:18am

@riqwan riqwan changed the title chore: added compute actions for buyget promotions feat(): added compute actions for buyget promotions Jan 29, 2024
@riqwan riqwan marked this pull request as ready for review January 29, 2024 12:07
@riqwan riqwan requested review from a team as code owners January 29, 2024 12:07
Comment on lines +53 to +55
product_category: {
id: "catg_cotton",
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

thought(non-blocking): would probably change this shape to be product_categories: [{...}]

We should do this in a different API but let's add a task to settle on the final shape of the inputs to the promotion module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it works both ways already. You can pass product_categories: [] or product_category: {}. We've only added very few definition restrictions here.

Let me know if thats not what you meant

allocation: "each",
max_quantity: 1,
apply_to_quantity: 1,
buy_rules_min_quantity: 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

thought: would want this to be part of the buy_rules instead. e.g., { attribute: "quantity", operator: "gt", values: 1 }. All buy rules would then be evaluated with an AND operator on for each item. Wdyt of this idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this might be quite limiting when we want to expand cases. It would only allow one BuyRule for item targeting and will also result in some relationship validations as min_qty is a required field.

I think there are 2 cases here:

  1. each rule needs to conform to a grouped quantity
    ex: buy a cotton shirt and a silk shirt and get a sweater for free
    or
    ex: buy 2 cotton shirt and get a sweater for free

In this case, we need to use an OR operator.

buy_rules_type: "across",
buy_rules_min_quantity: 2,
buy_rules: [{
  attribute: "product_category.id",
  operator: "eq",
  values: ["catg_cotton"],
}, {
  attribute: "product_category.id",
  operator: "eq",
  values: ["catg_silk"],
}],
  1. each rule conforms to its own min_quantity
    ex: buy 2 cotton tshirts and 1 silk shirt and get a sweater free.

In this case, we need to use an AND operator.

buy_rules_type: "each",
buy_rules_min_quantity: null,
buy_rules: [{
  attribute: "product_category.id",
  operator: "eq",
  values: ["catg_cotton"],
  min_quantity: 2 
}, {
  attribute: "product_category.id",
  operator: "eq",
  values: ["catg_silk"],
  min_quantity: 1 
}],

wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes sense! Kinda hard to wrap your head around, but it makes sense - would be amazing if this could self-document somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently only case 1 is implemented. When implementing case 2, I can rework the code to make it more easily readable with some inline comments.

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!

@kodiakhq kodiakhq bot merged commit 328eb85 into develop Jan 30, 2024
17 checks passed
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