Skip to content

Commit eb6dafa

Browse files
bubasumabubasuma
committed
ACP2E-3973: [Cloud] Free Shipping discount not correctly removed when cart no longer meets requirements
1 parent 8f92307 commit eb6dafa

File tree

4 files changed

+184
-65
lines changed

4 files changed

+184
-65
lines changed

‎app/code/Magento/SalesRule/Model/Quote/Discount.php‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -191,8 +191,6 @@ public function collect(
191191
$address->setBaseDiscountAmount(0);
192192
/** @var Rule $rule */
193193
foreach ($rules as $rule) {
194-
$ruleTotalDiscount = 0;
195-
$ruleDiscountAmount = 0;
196194
/** @var Item $item */
197195
foreach ($itemsToApplyRules as $key => $item) {
198196
if ($item->getNoDiscount() || !$this->calculator->canApplyDiscount($item) || $item->getParentItem()) {
@@ -221,19 +219,27 @@ public function collect(
221219
if ($rule->getStopRulesProcessing() && in_array($rule->getId(), $appliedRuleIds)) {
222220
unset($itemsToApplyRules[$key]);
223221
}
224-
222+
}
223+
$baseDiscountAmount = 0;
224+
$discountAmount = 0;
225+
// $itemsAggregate are items specific to the current shipping address
226+
foreach ($itemsAggregate as $item) {
227+
if ($item->getParentItem()) {
228+
continue;
229+
}
225230
if ($item->getChildren() && $item->isChildrenCalculated()) {
226231
foreach ($item->getChildren() as $child) {
227-
$ruleTotalDiscount += $child->getBaseDiscountAmount();
228-
$ruleDiscountAmount += $child->getDiscountAmount();
232+
$baseDiscountAmount += $child->getBaseDiscountAmount();
233+
$discountAmount += $child->getDiscountAmount();
229234
}
230235
}
231-
$ruleTotalDiscount += $item->getBaseDiscountAmount();
232-
$ruleDiscountAmount += $item->getDiscountAmount();
236+
$baseDiscountAmount += $item->getBaseDiscountAmount();
237+
$discountAmount += $item->getDiscountAmount();
233238
}
234-
$address->setBaseDiscountAmount($ruleTotalDiscount);
235-
$address->setBaseSubtotalWithDiscount($address->getBaseSubtotal() - $ruleTotalDiscount);
236-
$address->setSubtotalWithDiscount($address->getSubtotal() - $ruleDiscountAmount);
239+
$address->setBaseDiscountAmount(-$baseDiscountAmount);
240+
$address->setDiscountAmount(-$discountAmount);
241+
$address->setBaseSubtotalWithDiscount($address->getBaseSubtotal() - $baseDiscountAmount);
242+
$address->setSubtotalWithDiscount($address->getSubtotal() - $discountAmount);
237243
}
238244
$this->calculator->initTotals($items, $address);
239245
foreach ($items as $item) {

‎app/code/Magento/SalesRule/Test/Fixture/Rule.php‎

Lines changed: 48 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -145,32 +145,17 @@ public function revert(DataObject $data): void
145145
private function prepareData(array $data): array
146146
{
147147
$data = array_merge($this->prepareDefaultData(), $data);
148-
$data['conditions'] = $data['conditions'] ?? [];
149-
$data['actions'] = $data['actions'] ?? [];
150-
151-
if ($data['conditions'] instanceof DataObject) {
152-
$data['conditions'] = $data['conditions']->toArray();
153-
} else {
154-
$conditions = $data['conditions'];
155-
$data['conditions'] = Conditions::DEFAULT_DATA;
156-
foreach ($conditions as $condition) {
157-
$data['conditions']['conditions'][] = $condition instanceof DataObject
158-
? $condition->toArray()
159-
: $condition;
160-
}
161-
}
162148

163-
if ($data['actions'] instanceof DataObject) {
164-
$data['actions'] = $data['actions']->toArray();
165-
} else {
166-
$conditions = $data['actions'];
167-
$data['actions'] = ProductConditions::DEFAULT_DATA;
168-
foreach ($conditions as $condition) {
169-
$data['actions']['conditions'][] = $condition instanceof DataObject
170-
? $condition->toArray()
171-
: $condition;
172-
}
173-
}
149+
$data['conditions'] = $this->prepareConditionsData(
150+
$data['conditions'] instanceof DataObject ? $data['conditions']->toArray() : $data['conditions'],
151+
Conditions::DEFAULT_DATA,
152+
AddressCondition::DEFAULT_DATA
153+
);
154+
$data['actions'] = $this->prepareConditionsData(
155+
$data['actions'] instanceof DataObject ? $data['actions']->toArray() : $data['actions'],
156+
ProductConditions::DEFAULT_DATA,
157+
ProductCondition::DEFAULT_DATA
158+
);
174159

175160
if (!empty($data['coupon_code'])) {
176161
$data['coupon_type'] = \Magento\SalesRule\Model\Rule::COUPON_TYPE_SPECIFIC;
@@ -179,6 +164,44 @@ private function prepareData(array $data): array
179164
return $this->dataProcessor->process($this, $data);
180165
}
181166

167+
/**
168+
* Prepare conditions data
169+
*
170+
* @param array $conditions
171+
* @param array $defaultConditionsData
172+
* @param array $defaultConditionData
173+
* @return array
174+
*/
175+
private function prepareConditionsData(
176+
array $conditions,
177+
array $defaultConditionsData,
178+
array $defaultConditionData
179+
): array {
180+
$conditionsArray = array_is_list($conditions)
181+
? ['conditions' => $conditions]
182+
: $conditions;
183+
$conditionsArray += $defaultConditionsData;
184+
$subConditions = $conditionsArray['conditions'];
185+
$conditionsArray['conditions'] = [];
186+
foreach ($subConditions as $condition) {
187+
$conditionArray = $condition instanceof DataObject
188+
? $condition->toArray()
189+
: $condition;
190+
// Condition is a combine
191+
if (array_is_list($conditionArray) || isset($conditionArray['conditions'])) {
192+
$conditionArray = $this->prepareConditionsData(
193+
$conditionArray,
194+
$defaultConditionsData,
195+
$defaultConditionData
196+
);
197+
} else {
198+
$conditionArray += $defaultConditionData;
199+
}
200+
$conditionsArray['conditions'][] = $conditionArray;
201+
}
202+
return $conditionsArray;
203+
}
204+
182205
/**
183206
* Prepares rule default data
184207
*

‎app/code/Magento/SalesRule/Test/Unit/Model/Quote/DiscountTest.php‎

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
use Magento\Framework\Event\Manager;
1212
use Magento\Framework\Event\ManagerInterface;
1313
use Magento\Framework\Pricing\PriceCurrencyInterface;
14+
use Magento\Framework\TestFramework\Unit\Matcher\MethodInvokedAtIndex;
1415
use Magento\Quote\Api\Data\ShippingAssignmentInterface;
1516
use Magento\Quote\Api\Data\ShippingInterface;
1617
use Magento\Quote\Model\Quote;
@@ -426,7 +427,7 @@ public function testCollectAddressBaseDiscountAmountIncludingItemChildren(): voi
426427
)
427428
->disableOriginalConstructor()
428429
->getMock();
429-
$total->expects($this->any())->method('getBaseDiscountAmount')->willReturn(20.00);
430+
$total->expects($this->any())->method('getBaseDiscountAmount')->willReturn(-20.00);
430431

431432
$store = $this->createMock(Store::class);
432433
$this->storeManagerMock->expects($this->once())->method('getStore')->with($storeId)->willReturn($store);
@@ -463,8 +464,8 @@ public function testCollectAddressBaseDiscountAmountIncludingItemChildren(): voi
463464
->addMethods(['getBaseDiscountAmount'])
464465
->disableOriginalConstructor()
465466
->getMock();
466-
$item->expects($this->exactly(2))->method('getChildren')->willReturn([$child]);
467-
$item->expects($this->once())->method('isChildrenCalculated')->willReturn(true);
467+
$item->expects($this->any())->method('getChildren')->willReturn([$child]);
468+
$item->expects($this->any())->method('isChildrenCalculated')->willReturn(true);
468469
$index = 1;
469470
$child->expects($this->any())->method('getBaseDiscountAmount')->willReturnCallback(function () use (&$index) {
470471
$value = $index * 10;
@@ -479,14 +480,23 @@ public function testCollectAddressBaseDiscountAmountIncludingItemChildren(): voi
479480
->willReturnArgument(0);
480481

481482
$this->addressMock->expects($this->exactly(5))
483+
->method('setBaseDiscountAmount');
484+
485+
$this->addressMock->expects(new MethodInvokedAtIndex(0))
486+
->method('setBaseDiscountAmount')
487+
->with(0);
488+
$this->addressMock->expects(new MethodInvokedAtIndex(1))
489+
->method('setBaseDiscountAmount')
490+
->with(0);
491+
$this->addressMock->expects(new MethodInvokedAtIndex(2))
492+
->method('setBaseDiscountAmount')
493+
->with(-10);
494+
$this->addressMock->expects(new MethodInvokedAtIndex(3))
495+
->method('setBaseDiscountAmount')
496+
->with(-20);
497+
$this->addressMock->expects(new MethodInvokedAtIndex(4))
482498
->method('setBaseDiscountAmount')
483-
->with($this->logicalOr(
484-
$this->equalTo(0),
485-
$this->equalTo(10),
486-
$this->equalTo(20),
487-
$this->equalTo(20),
488-
$this->equalTo(20.00)
489-
));
499+
->with(-20);
490500

491501
$this->discount->collect($quote, $this->shippingAssignmentMock, $total);
492502
}

‎dev/tests/integration/testsuite/Magento/SalesRule/Model/Quote/DiscountTest.php‎

Lines changed: 100 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@
3434
use Magento\SalesRule\Model\Rule;
3535
use Magento\SalesRule\Model\Rule\Condition\Combine as CombineCondition;
3636
use Magento\SalesRule\Model\Rule\Condition\Product as ProductCondition;
37-
use Magento\SalesRule\Test\Fixture\AddressCondition;
3837
use Magento\SalesRule\Test\Fixture\ProductCondition as ProductConditionFixture;
3938
use Magento\SalesRule\Test\Fixture\Rule as RuleFixture;
4039
use Magento\TestFramework\Fixture\AppIsolation;
@@ -868,11 +867,6 @@ public function testDiscountOnSimpleProductWhenBuyXGetYRuleHasDiscountQtyStepSpe
868867

869868
#[
870869
DataFixture(ProductFixture::class, ['price' => 100], 'p1'),
871-
DataFixture(
872-
AddressCondition::class,
873-
['attribute' => 'base_subtotal_with_discount', 'operator' => '>', 'value' => 75],
874-
'cond1'
875-
),
876870
DataFixture(
877871
RuleFixture::class,
878872
[
@@ -892,7 +886,9 @@ public function testDiscountOnSimpleProductWhenBuyXGetYRuleHasDiscountQtyStepSpe
892886
'simple_action' => Rule::BY_FIXED_ACTION,
893887
'sort_order' => 2,
894888
'apply_to_shipping' => 1,
895-
'conditions' => ['$cond1$'],
889+
'conditions' => [
890+
['attribute' => 'base_subtotal_with_discount', 'operator' => '>', 'value' => 75]
891+
],
896892
],
897893
'rule2'
898894
),
@@ -943,17 +939,6 @@ public function testSubtotalWithDiscountShouldReflectPreviouslyAppliedRules(): v
943939

944940
#[
945941
DataFixture(ProductFixture::class, ['price' => 100], 'p1'),
946-
DataFixture(
947-
AddressCondition::class,
948-
['attribute' => 'base_subtotal_with_discount', 'operator' => '>', 'value' => 75],
949-
'cond1'
950-
),
951-
DataFixture(
952-
ProductConditionFixture::class,
953-
// Ensures that the discount is applied to shipping only
954-
['attribute' => 'quote_item_price', 'operator' => '<', 'value' => 0],
955-
'cond2'
956-
),
957942
DataFixture(
958943
RuleFixture::class,
959944
[
@@ -973,8 +958,13 @@ public function testSubtotalWithDiscountShouldReflectPreviouslyAppliedRules(): v
973958
'simple_action' => Rule::BY_FIXED_ACTION,
974959
'sort_order' => 2,
975960
'apply_to_shipping' => 1,
976-
'conditions' => ['$cond1$'],
977-
'actions' => ['$cond2$'],
961+
'conditions' => [
962+
['attribute' => 'base_subtotal_with_discount', 'operator' => '>', 'value' => 75]
963+
],
964+
'actions' => [
965+
// Ensures that the discount is applied to shipping only
966+
['attribute' => 'quote_item_price', 'operator' => '<', 'value' => 0],
967+
],
978968
],
979969
'rule2'
980970
),
@@ -1023,4 +1013,94 @@ public function testSubtotalWithDiscountShouldReflectPreviouslyAppliedRulesForSh
10231013
$this->assertEquals([$rule1Id], explode(',', $cart->getAppliedRuleIds()));
10241014
$this->assertEquals([$rule1Id], explode(',', current($cart->getItems())->getAppliedRuleIds()));
10251015
}
1016+
1017+
#[
1018+
DataFixture(ProductFixture::class, ['price' => 100], 'p1'),
1019+
DataFixture(ProductFixture::class, ['price' => 100], 'p2'),
1020+
DataFixture(ProductFixture::class, ['price' => 100], 'p3'),
1021+
DataFixture(
1022+
RuleFixture::class,
1023+
[
1024+
'stop_rules_processing'=> 1,
1025+
'discount_amount' => 50,
1026+
'simple_action' => Rule::BY_PERCENT_ACTION,
1027+
'sort_order' => 1,
1028+
'actions' => [
1029+
['attribute' => 'sku', 'value' => '$p1.sku$']
1030+
],
1031+
],
1032+
'rule1'
1033+
),
1034+
DataFixture(
1035+
RuleFixture::class,
1036+
[
1037+
'stop_rules_processing'=> 0,
1038+
'discount_amount' => 25,
1039+
'simple_action' => Rule::BY_PERCENT_ACTION,
1040+
'sort_order' => 2,
1041+
'apply_to_shipping' => 0,
1042+
'conditions' => [
1043+
['attribute' => 'base_subtotal_with_discount', 'operator' => '<', 'value' => 300]
1044+
],
1045+
'actions' => [
1046+
['attribute' => 'sku', 'value' => '$p2.sku$']
1047+
],
1048+
],
1049+
'rule2'
1050+
),
1051+
DataFixture(
1052+
RuleFixture::class,
1053+
[
1054+
'stop_rules_processing'=> 0,
1055+
'discount_amount' => 30,
1056+
'simple_action' => Rule::BY_FIXED_ACTION,
1057+
'sort_order' => 3,
1058+
'conditions' => [
1059+
['attribute' => 'base_subtotal_with_discount', 'operator' => '>', 'value' => 250]
1060+
],
1061+
],
1062+
'rule3'
1063+
),
1064+
DataFixture(GuestCartFixture::class, as: 'cart1'),
1065+
DataFixture(AddProductToCartFixture::class, ['cart_id' => '$cart1.id$', 'product_id' => '$p1.id$']),
1066+
DataFixture(AddProductToCartFixture::class, ['cart_id' => '$cart1.id$', 'product_id' => '$p2.id$']),
1067+
DataFixture(AddProductToCartFixture::class, ['cart_id' => '$cart1.id$', 'product_id' => '$p3.id$']),
1068+
]
1069+
public function testSubtotalWithDiscountShouldReflectPreviouslyAppliedRulesWithStopRulesProcessing(): void
1070+
{
1071+
$rule1Id = (int)$this->fixtures->get('rule1')->getId();
1072+
$rule2Id = (int)$this->fixtures->get('rule2')->getId();
1073+
$p1Id = (int)$this->fixtures->get('p1')->getId();
1074+
$p2Id = (int)$this->fixtures->get('p2')->getId();
1075+
$p3Id = (int)$this->fixtures->get('p3')->getId();
1076+
// Reset the state of the rule validator to ensure that it does not cache any previous state
1077+
$this->objectManager->get(\Magento\SalesRule\Model\Validator::class)->_resetState();
1078+
$cart = $this->quoteRepository->get((int)$this->fixtures->get('cart1')->getId());
1079+
$cart->collectTotals();
1080+
1081+
$this->assertEquals(225, $cart->getSubtotalWithDiscount());
1082+
$this->assertEquals(225, $cart->getBaseSubtotalWithDiscount());
1083+
// Bypass getter methods for subtotal_with_discount and base_subtotal_with_discount to retrieve stored values
1084+
$this->assertEquals(225, $cart->getShippingAddress()->getData('subtotal_with_discount'));
1085+
$this->assertEquals(225, $cart->getShippingAddress()->getData('base_subtotal_with_discount'));
1086+
$this->assertEquals(-75, $cart->getShippingAddress()->getBaseDiscountAmount());
1087+
$this->assertEquals(-75, $cart->getShippingAddress()->getDiscountAmount());
1088+
$this->assertEquals(0, $cart->getShippingAddress()->getShippingDiscountAmount());
1089+
$this->assertEquals(0, $cart->getShippingAddress()->getBaseShippingDiscountAmount());
1090+
// Check that rule3 is not applied because it requires subtotal with discount to be greater than 250
1091+
$this->assertEquals([$rule1Id, $rule2Id], explode(',', $cart->getAppliedRuleIds()));
1092+
// rule2 is applied shipping only
1093+
$items= [];
1094+
foreach ($cart->getAllItems() as $item) {
1095+
$items[$item->getProductId()] = $item;
1096+
}
1097+
$this->assertEquals([$rule1Id], explode(',', $items[$p1Id]->getAppliedRuleIds()));
1098+
$this->assertEquals(50, $items[$p1Id]->getBaseDiscountAmount());
1099+
1100+
$this->assertEquals([$rule2Id], explode(',', $items[$p2Id]->getAppliedRuleIds()));
1101+
$this->assertEquals(25, $items[$p2Id]->getBaseDiscountAmount());
1102+
1103+
$this->assertEmpty($items[$p3Id]->getAppliedRuleIds());
1104+
$this->assertEquals(0, $items[$p3Id]->getBaseDiscountAmount());
1105+
}
10261106
}

0 commit comments

Comments
 (0)