feat: promotions discounts v2 #6575

vanpho93 · 2022-10-19T06:37:33Z

Resolves #6458 #6462 #6459 #6453 #6468
Impact: major
Type: feature

Issue

Missing discounts in the new promotion framework

Solution

Adding api-plugin-discounts

changeset-bot · 2022-10-19T06:37:36Z

⚠️ No Changeset found

Latest commit: bc2451b

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

apps/reaction/tests/integration/api/mutations/discountCodes/discountCodes.test.js

packages/api-plugin-promotions-discounts/src/discountTypes/order/applyOrderDiscountToCart.js

packages/api-plugin-promotions-discounts/src/discountTypes/shipping/getGroupDisountTotal.js

zenweasel

Need to update the sample data to reflect these changes

zenweasel · 2022-11-10T08:19:13Z

Applied discounts is not coming through on the order.

Also I think should include at least one top level integration test that tests the functionality end to end:

Create a promotion (using the mutation preferably)
Create a cart (there is existing test stuff you can reuse)
Validate that the promotions and discounts are applied
Place an order
Validate that the promotions and discounts are carried through to the order

I think this will really help us as we make tweaks to this to ensure we don't break it

zenweasel

A few typos and things but it looks good

zenweasel · 2022-11-15T04:11:32Z

apps/reaction/tests/integration/api/mutations/checkout/promotionCheckout.test.js

+    opaqueFulfillmentMethodId = option.fulfillmentMethod._id;
+  });
+
+  test("select the `Standard mockMethod` fulfillment option", async () => {


These are set up like separate tests but in fact there is only one test, which should describe what it's testing, e.g. "place an order with discount and get the correct values"

zenweasel · 2022-11-15T04:13:07Z

packages/api-plugin-discounts/package.json

@@ -34,5 +34,10 @@
  },
  "publishConfig": {
    "access": "public"
+  },
+  "scripts": {
+    "test": "jest --passWithNoTests",


We should remove this passWithNoTests because it should not pass with no tests

packages/api-plugin-discounts/src/util/setDiscountsOnCart.test.js

packages/api-plugin-promotions-discounts/src/actions/discountAction.js

packages/api-plugin-promotions-discounts/src/discountTypes/item/applyItemDiscountToCart.test.js

packages/api-plugin-promotions-discounts/src/methods/index.js

Signed-off-by: Brent Hoover <brent@thebuddhalodge.com>

traykov1

For some reason when i diff the working tree of the base feat/promotions branch with this PR's branch, it says that it deletes the defaultRoles for the promotions.

"reaction:legacy:promotions/create",
"reaction:legacy:promotions/read",
"reaction:legacy:promotions/update"

zenweasel · 2022-11-15T13:11:54Z

I think Brian just hasn't rebased against the feature branch as that just got merged today

tedraykov · 2022-11-15T15:40:11Z

packages/api-plugin-promotions-discounts/src/discountTypes/order/applyOrderDiscountToCart.js

+ * @returns {Number} - The discount amount
+ */
+export function getCartDiscountAmount(context, items, discount) {
+  const merchandiseTotal = getTotalEligibleItemsAmount(items);


Why do we calculate the merchandise total here, when the enhancer is doing the same?
Another question is if we calculate it here, why do we need the enhancer?

this variable should be rename to totalEligibleItemsAmount, this total is different, it counts inclusion and exclusion and used for discount value calculation.

Enhancer can do sth more dynamic like total of item quantity. Enhancer is at general level of the framework. this totalEligibleItemsAmount is in detail of discount plugin.

tedraykov

In the api-plugin-promotions graphql schema the PromotionFilter input should be suffixed by Input to match the style of all graphql inputs.
The createPromotion mutation should use PromotionInput simple schema for the input. Also the following code in the same mutation:

  const { triggerKey } = promotions.triggers[0];
  const trigger = promotions.triggers.find((tr) => tr.triggerKey === triggerKey);
  promotion.triggerType = trigger.type;

Can't it be simplified to:

promotion.triggerType = promotions.triggers[0].type;

tedraykov · 2022-11-15T15:56:04Z

Regarding the success field in the PromotionCreateUpdatePayload I would suggest we create and accept an ARD before we start introducing new schema styles.

tedraykov · 2022-11-15T16:07:47Z

packages/api-plugin-discounts/src/util/setDiscountsOnCart.js

@@ -7,6 +7,9 @@ import getDiscountsTotalForCart from "../queries/getDiscountsTotalForCart.js";
 * @returns {undefined}
 */
 export default async function setDiscountsOnCart(context, cart) {
-  const { total } = await getDiscountsTotalForCart(context, cart);
-  cart.discount = total;
+  // check if promotion discounts are enabled


Don't we want to remove this plugin altogether?

I think adding this code is safer. Someone may still install both dependencies.

In the api-plugin-promotions graphql schema the PromotionFilter input should be suffixed by Input to match the style of all graphql inputs.

The createPromotion mutation should use PromotionInput simple schema for the input. Also the following code in the same mutation:

const { triggerKey } = promotions.triggers[0]; const trigger = promotions.triggers.find((tr) => tr.triggerKey === triggerKey); promotion.triggerType = trigger.type;

Can't it be simplified to:

promotion.triggerType = promotions.triggers[0].type;

No, because it may be an array with multiple triggers at some point

Don't we want to remove this plugin altogether?

The plugin should be removed but I would like to retain backwards compatibility if possible

tedraykov · 2022-11-15T16:17:13Z

Overall, the plugin looks excellent with the sample data. I still don't have a settled mindmap of the whole framework but it seems like it's really extendable.
In this PR I don't see being addressed what happens with existing carts with applied promotions when existing promotion parameters are changed but I assume it will be addressed separately.

zenweasel · 2022-11-16T00:25:34Z

Regarding the success field in the PromotionCreateUpdatePayload I would suggest we create and accept an ARD before we start introducing new schema styles.

I agree about putting together an (an ADR is what I am assuming you meant) but I don't think we want to introduce new legacy code here

zenweasel · 2022-11-16T00:26:59Z

In this PR I don't see being addressed what happens with existing carts with applied promotions when existing promotion parameters are changed but I assume it will be addressed separately.

Yes, this is just discounts. This is being addressed here

vanpho93 · 2022-11-16T03:17:54Z

In the api-plugin-promotions graphql schema the PromotionFilter input should be suffixed by Input to match the style of all graphql inputs.

The createPromotion mutation should use PromotionInput simple schema for the input. Also the following code in the same mutation:
  const { triggerKey } = promotions.triggers[0];
  const trigger = promotions.triggers.find((tr) => tr.triggerKey === triggerKey);
  promotion.triggerType = trigger.type;
Can't it be simplified to:
promotion.triggerType = promotions.triggers[0].type;

@zenweasel Do you have opinion on this, it seems like the API part. I think it should address these in another PR.

zenweasel · 2022-11-16T03:19:32Z

Yes, all the comments about the api should be dealt in a different pr

…nts-v2 feat: promotions discounts v2

vanpho93 changed the base branch from trunk to feat/promotion-framework-secound-approach October 19, 2022 06:37

vanpho93 force-pushed the feat/promotions-discounts-v2 branch from f928e8e to 8d4f6ea Compare October 19, 2022 06:41

Base automatically changed from feat/promotion-framework-secound-approach to feat/promotions October 25, 2022 01:14

vanpho93 force-pushed the feat/promotions-discounts-v2 branch 2 times, most recently from f327784 to 3eca2c1 Compare October 31, 2022 11:15

vanpho93 marked this pull request as ready for review November 7, 2022 07:11

zenweasel reviewed Nov 7, 2022

View reviewed changes

zenweasel requested review from tedraykov and zenweasel November 7, 2022 08:26

zenweasel requested changes Nov 10, 2022

View reviewed changes

vanpho93 added 14 commits November 14, 2022 10:15

feat: add the promotion-discounts plugin

822bbdc

fix: fix query and mutation tests fail

4c936b7

feat: add rules for promotion trigger (uncompleted)

b798517

feat: inclusion and exclusion for discount item

5ebb6ac

fix: make getEligibleItems shorter

0c979c9

feat: refactor promotion discount plugin

0eac0d9

fix: update pnpm lock

6271a1a

feat: add test for promotion discounts

03b90cb

fix: fix integration mutation test fail

f59a564

fix: calculate percentage discount

64030bd

fix: split order discount for cart items

1baccb1

fix: fix calculate the order total

01b77aa

fix: fix test fail for order plugin

0cd9287

feat: update the promotion data on sample-data plugin

47bf3c7

vanpho93 force-pushed the feat/promotions-discounts-v2 branch from f14f2a1 to 1efb2de Compare November 15, 2022 02:15

feat: add integration test for promotions

719002a

vanpho93 force-pushed the feat/promotions-discounts-v2 branch from 1efb2de to 7d1ba64 Compare November 15, 2022 02:57

feat: update lockfile

f4cf8fd

vanpho93 force-pushed the feat/promotions-discounts-v2 branch from 7d1ba64 to f4cf8fd Compare November 15, 2022 03:22

vanpho93 requested a review from zenweasel November 15, 2022 03:58

zenweasel approved these changes Nov 15, 2022

View reviewed changes

zenweasel and others added 2 commits November 15, 2022 05:03

fix: fix sample-data records

cbf069f

Signed-off-by: Brent Hoover <brent@thebuddhalodge.com>

fix: fix typo on the tests

cc6a9c2

traykov1 reviewed Nov 15, 2022

View reviewed changes

tedraykov reviewed Nov 15, 2022

View reviewed changes

chore: rename variable

bc2451b

zenweasel merged commit e8f5762 into feat/promotions Nov 16, 2022

zenweasel deleted the feat/promotions-discounts-v2 branch November 16, 2022 04:00

vanpho93 pushed a commit that referenced this pull request Apr 24, 2023

Merge pull request #6575 from reactioncommerce/feat/promotions-discou…

f397372

…nts-v2 feat: promotions discounts v2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: promotions discounts v2 #6575

feat: promotions discounts v2 #6575

vanpho93 commented Oct 19, 2022 •

edited by zenweasel

changeset-bot bot commented Oct 19, 2022 •

edited

zenweasel left a comment

zenweasel commented Nov 10, 2022

zenweasel left a comment

zenweasel Nov 15, 2022

zenweasel Nov 15, 2022

traykov1 left a comment

zenweasel commented Nov 15, 2022

tedraykov Nov 15, 2022

vanpho93 Nov 15, 2022

tedraykov left a comment •

edited

tedraykov commented Nov 15, 2022

tedraykov Nov 15, 2022

vanpho93 Nov 15, 2022

zenweasel Nov 16, 2022

zenweasel Nov 16, 2022

tedraykov commented Nov 15, 2022 •

edited

zenweasel commented Nov 16, 2022

zenweasel commented Nov 16, 2022 •

edited

vanpho93 commented Nov 16, 2022

zenweasel commented Nov 16, 2022

feat: promotions discounts v2 #6575

feat: promotions discounts v2 #6575

Conversation

vanpho93 commented Oct 19, 2022 • edited by zenweasel

Issue

Solution

changeset-bot bot commented Oct 19, 2022 • edited

⚠️ No Changeset found

zenweasel left a comment

Choose a reason for hiding this comment

zenweasel commented Nov 10, 2022

zenweasel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

traykov1 left a comment

Choose a reason for hiding this comment

zenweasel commented Nov 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tedraykov left a comment • edited

Choose a reason for hiding this comment

tedraykov commented Nov 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tedraykov commented Nov 15, 2022 • edited

zenweasel commented Nov 16, 2022

zenweasel commented Nov 16, 2022 • edited

vanpho93 commented Nov 16, 2022

zenweasel commented Nov 16, 2022

vanpho93 commented Oct 19, 2022 •

edited by zenweasel

changeset-bot bot commented Oct 19, 2022 •

edited

tedraykov left a comment •

edited

tedraykov commented Nov 15, 2022 •

edited

zenweasel commented Nov 16, 2022 •

edited