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.

Sign up for GitHub

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

Jump to bottom

feat: shipping discount #6802

Merged
merged 20 commits into from
Mar 3, 2023
Merged

feat: shipping discount #6802

merged 20 commits into from
Mar 3, 2023

Conversation

vanpho93
Copy link
Member

@vanpho93 vanpho93 commented Feb 16, 2023 •
edited
Loading

Resolves #6773 #6767
Impact: major
Type: feature

Adding shipping discount with an option to prevent using 2 shipping discounts for one shipment.

Loom: https://www.loom.com/share/0ea5076ca3004ae9b1be6b3880a32e86

@changeset-bot
Copy link

changeset-bot bot commented Feb 16, 2023 •
edited
Loading

⚠️ No Changeset found

Latest commit: 36dd29c

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

This was linked to issues Feb 16, 2023
As an operator I want Shipping Discount promotions to stack unless they are actioned on the same Shipping Method #6767
Closed
Promotions: Add shipping discounts #6773
Closed
brent-hoover
brent-hoover reviewed Feb 20, 2023
View reviewed changes
...es/api-plugin-promotions-discounts/src/discountTypes/shipping/applyShippingDiscountToCart.js
dateApplied: new Date(),
discountedItemType: "shipping",
discountedAmount: discountedItem.amount,
stackability: promotion.stackability,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we applying the shipping discount to items? Not sure that's how we want to do that

Copy link
Member Author

Choose a reason for hiding this comment

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

No, we are applying it to shipping only, as it shows on the Loom demo.

brent-hoover
brent-hoover reviewed Feb 20, 2023
View reviewed changes
Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

Seems ok just have some questions:

  1. We are taking the discount on the shipping charge, not items right?
  2. We are taking the discount only on shipping and not on handling, correct?
  3. Have we taken into account when we may have a split shipment (i.e. two items in the shipping array)?

...es/api-plugin-promotions-discounts/src/discountTypes/shipping/applyShippingDiscountToCart.js Outdated Show resolved Hide resolved
@vanpho93
Copy link
Member Author

vanpho93 commented Feb 20, 2023 •
edited
Loading

Seems ok just have some questions:

  1. We are taking the discount on the shipping charge, not items right?
  2. We are taking the discount only on shipping and not on handling, correct?
  3. Have we taken into account when we may have a split shipment (i.e. two items in the shipping array)?
  1. => Yes, on the shipping charge
  2. => Do we want to exclude handling? Currently handling get discounted too but I can exclude it
  3. => We have it but we aren't able to demo it with the API because we didn't support split shipment in API level

@brent-hoover
Copy link
Collaborator

@vanpho93 Ok, double-checking whether we should discount handling or not, so hold on this.

@vanpho93 vanpho93 force-pushed the feat/shipping-discounts branch from c8f8305 to fb551ee Compare February 20, 2023 08:25
@brent-hoover
Copy link
Collaborator

@vanpho93 Clarified with @brendan-shea-pm , discounts should only be taken against shipping, not handling

brent-hoover
brent-hoover requested changes Feb 22, 2023
View reviewed changes
Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

Discounts don't display in quotes which is critical

@brent-hoover
Copy link
Collaborator

@vanpho93 Can we add a shipping discount promotion to the sample data as well?

brent-hoover
brent-hoover requested changes Feb 27, 2023
View reviewed changes
Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

Shipping quotes are not updated when a promotion becomes active after the cart already has quotes on it. Brian has identified the problem already and is working on a fix

@vanpho93 vanpho93 changed the base branch from feat/promotions to feat/coupons March 1, 2023 09:06
@vanpho93 vanpho93 force-pushed the feat/shipping-discounts branch from eb0f31a to 490044c Compare March 1, 2023 11:51
@vanpho93 vanpho93 requested a review from brent-hoover March 2, 2023 02:39
brent-hoover
brent-hoover approved these changes Mar 2, 2023
View reviewed changes
Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

Great work

packages/api-plugin-promotions/src/startup.js Outdated Show resolved Hide resolved
@vanpho93 vanpho93 merged commit e0c0e7c into feat/coupons Mar 3, 2023
@brent-hoover
Copy link
Collaborator

@vanpho93 In the future please wait for all reviewers I've assigned to approve before merging

@brent-hoover brent-hoover deleted the feat/shipping-discounts branch March 3, 2023 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brent-hoover brent-hoover brent-hoover approved these changes

@sujithvn sujithvn Awaiting requested review from sujithvn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@vanpho93 @brent-hoover