Edit cart rule CQRS command #32115

zuk3975 · 2023-04-11T13:40:57Z

Questions	Answers
Branch?	develop
Description?	Adds CQRS command for editing cart rule and clean up some behat tests to have tables and cleaner implementation instead of single line stateful code.
Type?	refacto
Category?	BO
BC breaks?	no
Deprecations?	no
How to test?	No need to test, added behats to support command and UI wasn't yet changed
Fixed ticket?	part of #10547
Related PRs	If theme, autoupgrade or other module change is needed to make this change work, provide a link to related PRs here.
Sponsor company	Your company or customer's name goes here (if applicable).

BC BREAKS
Changed some arguments/types in EditableCartRule and its subclasses (not sure if I must define them exactly? because some of them might still evolve until the page is fully done). These BC breaks should practically have no influence, because cart rule symfony page was not yet released so the commands was not used at least in core. Ofc in theory someone might have used them from modules)

(Edit 06/06/2024: the page is not released so all the related code is still experimental, therefore this is not a breaking change)

zuk3975 · 2023-04-17T11:48:57Z

src/Adapter/CartRule/CommandHandler/AddCartRuleHandler.php

@@ -59,7 +59,7 @@ public function __construct(
    public function handle(AddCartRuleCommand $command): CartRuleId
    {
        //@todo: restrictions are missing. We should consider dedicated command for restrictions
-        $cartRule = $this->cartRuleRepository->create($this->buildCartRuleFromCommandData($command));
+        $cartRule = $this->cartRuleRepository->add($this->buildCartRuleFromCommandData($command));


optional change. I switched to "add" because I think it makes sense to have add - when providing already filled object model, create -when required properties for creation are passed and the model is built inside

jolelievre · 2023-04-18T10:14:01Z

src/Adapter/CartRule/CommandHandler/AddCartRuleHandler.php

@@ -59,7 +59,7 @@ public function __construct(
    public function handle(AddCartRuleCommand $command): CartRuleId
    {
        //@todo: restrictions are missing. We should consider dedicated command for restrictions
-        $cartRule = $this->cartRuleRepository->create($this->buildCartRuleFromCommandData($command));
+        $cartRule = $this->cartRuleRepository->add($this->buildCartRuleFromCommandData($command));


jolelievre · 2023-04-18T10:20:18Z

src/Core/Domain/CartRule/QueryResult/EditableCartRuleActions.php

@@ -28,9 +28,6 @@

 namespace PrestaShop\PrestaShop\Core\Domain\CartRule\QueryResult;

-use PrestaShop\PrestaShop\Core\Domain\Product\Combination\ValueObject\CombinationId;
-use PrestaShop\PrestaShop\Core\Domain\Product\ValueObject\ProductId;
-
 class EditableCartRuleActions


Suggested change

class EditableCartRuleActions

class CartRuleAction

Suggested change

class EditableCartRuleActions

class CartRuleActionForEditing

I'm not entirely sure, but at least we should drop the s there's only one action no? (although it contains several options)

jolelievre · 2023-04-18T10:23:48Z

src/Core/Domain/CartRule/QueryResult/EditableCartRuleMinimum.php

@@ -29,7 +29,6 @@
 namespace PrestaShop\PrestaShop\Core\Domain\CartRule\QueryResult;

 use PrestaShop\Decimal\DecimalNumber;
-use PrestaShop\PrestaShop\Core\Domain\Currency\ValueObject\CurrencyId;

 class EditableCartRuleMinimum


Suggested change

class EditableCartRuleMinimum

class CartRuleMinimumForEditing

SHould we change all the query result naming with that convention? Since the query itself is GetCartRuleForEditing ?

jolelievre · 2023-04-18T10:26:51Z

tests/Integration/Behaviour/Features/Context/Domain/CartFeatureContext.php

@@ -584,7 +584,7 @@ public function useDiscountVoucherOnCart(string $voucherCode, float $discountAmo
    public function useDiscountByCodeOnCart(string $voucherCode, string $cartReference)
    {
        $cartId = SharedStorage::getStorage()->get($cartReference);
-        $cartRuleId = SharedStorage::getStorage()->get($voucherCode);
+        $cartRuleId = (int) CartRule::getIdByCode($voucherCode);


Why do we rely on DB code value now? If it worked fine ith the reference system we should continue to use it no? it's preferable to rely on a reference bound to a test scenario than a code in database that can be modified, the reference would still point to the same object while we'd need to adapt all following steps to match the new code

Because we don't save cart rule reference by code anymore (as that was custom step), so it seemed like a nice way around it, but I can set the reference everytime we create a rule when code is not empty and that will work as before then.

Hmm then I think we can use another reference to save the ID based on the code like after addition and edition:

if (!empty($cartRule->code)) { $this->getStorage()->set('cart_rule_code' . $cartRule->code, $cartRule->id); }

then there would be to ways to get the cart rule ID:

$cartRuleId = $this->getStorage()->get('cart_rule_code' . $cartRuleCode); $cartRuleId = $this->getStorage()->get($cartRuleReference);

this allows you to keep the previous steps that were based on the code as reference but at least only IDs are stored in every case. And we could even add some helper method getCartRuleIdByReference($reference): int and getCartRuleIdByCode($code): int

I just saved by the code whenever is not empty, that should be enough

jolelievre · 2023-04-18T10:28:16Z

tests/Integration/Behaviour/Features/Context/Domain/CartRule/CartRuleFeatureContext.php

+        $cartRule = $this->getCommandBus()->handle($command);
+
+        //@todo: should be refactored to save only id to shared storage instead of whole object model
+        SharedStorage::getStorage()->set($cartRuleReference, new CartRule($cartRule->getValue()));


Suggested change

SharedStorage::getStorage()->set($cartRuleReference, new CartRule($cartRule->getValue()));

$this->getSharedStorage()->set($cartRuleReference, new CartRule($cartRule->getValue()));

(this should be replaced in the whole file)

We should stop using the singleton method, maybe it would be simpler to actually remove it completely

However I don't think the refacto should be done in another PR, all our references are based on IDs and we should always do it this way. It forces fetching the object instead of relying on an existing object from previous step

jolelievre · 2023-04-18T10:31:08Z

tests/Integration/Behaviour/Features/Context/Domain/CartRule/CartRuleFeatureContext.php

+        foreach ($cartRuleReferenceArray as $carRuleReference) {
+            $cartRuleIds[] = SharedStorage::getStorage()->get($carRuleReference)->id;
+        }


If references are correctly storing IDs it allows using the referencesToIds method

jolelievre · 2023-04-18T10:34:39Z

tests/Integration/Behaviour/Features/Context/Domain/CartRule/EditCartRuleFeatureContext.php

+    private function buildAction(array $data): ?CartRuleActionInterface
+    {
+        $actionWasSet = false;
+        $builder = new CartRuleActionBuilder();


Ok for now but the CartRuleActionBuilder will have to be refactored to be come stateless later

yeah Im still thinking about that stateless. Its hard to do. I've tried similar stuff, but its messy 😄
Further more if we just provide array of $data to build method, it will be coupled with form, meaning we will have to duplicate everything in behats 😓 So I wonder if its really worth it

Well you already have to do the mapping from the behat table format already, when you could also build the CartRuleAction from scratch without using the CartRuleActionBuilder

To make it work you would only need to adapt the behat format to the expected builder format:

$builderData = [ 'gift_product_id' => $tableNodeData['gift product'], 'gift_product_attribute_id' => $tableNodeData['gift combination'], ];

It would actually simplify the behat test because only this mapping would be enough, you wouldn't need to replicate the behaviour of the form data handler: I check that this field and this field is present, so I can call the builder setter with to attributes. Then I check that this fields and that is present and call another setter, ...

So the whole building process would only be handled in the builder, of course its expected format would be based on the form structure because it's easier this way for us. But it's exactly what we did with the product builders, and even then we were able to adapt them to other use cases by relying on form data adapter classes (for combination bulk edition, or list edition)

…thods for less code

jolelievre

Thanks @zuk3975

just a few details and it's all good

jolelievre · 2023-04-18T17:09:30Z

tests/Integration/Behaviour/Features/Context/Domain/CartRule/CartRuleFeatureContext.php

+        );
+
+        if (!empty($data['minimum_amount'])) {
+            $currencyId = SharedStorage::getStorage()->get($data['minimum_amount_currency']);


Suggested change

$currencyId = SharedStorage::getStorage()->get($data['minimum_amount_currency']);

$currencyId = $this->getSharedStorage()->get($data['minimum_amount_currency']);

To be replaced in the whole file

jolelievre · 2023-04-18T17:10:54Z

tests/Integration/Behaviour/Features/Context/Domain/CartRule/CartRuleFeatureContext.php

+
+        if (!empty($data['code'])) {
+            // set cart rule id by code when it is not empty
+            SharedStorage::getStorage()->set($data['code'], $cartRuleId->getValue());


Ah I had almost the same idea (detailed in another comment), I only thought we could add a prefix cart_rule_code_ for example, it's not mandatory but it could prevent some conflicts maybe

yeah I don't think its worth it, I don't think we will have such scenario which will force us to have same name for cart rule and for the code, so as long as we are not doing stupid things in behats we will be fine 😄

Well you'll be aware of it, but a future developer probably not But maybe it's overengineering, let's go with this current colution

tests/Integration/Behaviour/Features/Context/Domain/CartRule/EditCartRuleFeatureContext.php

jolelievre

Thanks @zuk3975

boherm

LGTM, thanks @zuk3975!

zuk3975 · 2023-04-20T13:34:15Z

Thanks for reviews 🚀
Merging this without QA as it is only partial PR containing behat changes and the code that was not yet exposed anywhere in Back Office (and there are more PR's to come related to cart rules migration 🎉 )

prestonBot added develop Branch Refactoring Type: Refactoring BC break Type: Introduces a backwards-incompatible break labels Apr 11, 2023

zuk3975 force-pushed the edit-cart-rule branch from d61e697 to 4999578 Compare April 12, 2023 11:09

zuk3975 added 9 commits April 17, 2023 11:30

introduce edit cart rule command

87a6409

wip behats intorducing EditCartRuleContext

8eb1846

editableCartRule assertion

8795164

some fixes in command and add some steps in behat

ad6a761

rename exclude_discounted_products; Avoid VOs in query result

f4a40e5

add case with gift product

2a5c0e0

assert default values when different reduction set

bffed7f

add scenario for customer

b8cc41c

add cases with discounted specific product

3c52c58

zuk3975 force-pushed the edit-cart-rule branch from 4999578 to 3c52c58 Compare April 17, 2023 08:42

zuk3975 added 2 commits April 17, 2023 12:07

fix todo reminders in handler after rebase

4c26a08

fix remaining todo related to rebase

f78a7da

zuk3975 commented Apr 17, 2023

View reviewed changes

zuk3975 added 2 commits April 17, 2023 16:01

add missing properties to tests

9a44a01

Cleaning up duplicate behat implemetations for create

f40834a

zuk3975 force-pushed the edit-cart-rule branch from ad306f4 to f40834a Compare April 17, 2023 14:40

remove unused/replaced steps from context

adf060d

zuk3975 marked this pull request as ready for review April 18, 2023 07:01

zuk3975 requested a review from a team as a code owner April 18, 2023 07:01

jolelievre reviewed Apr 18, 2023

View reviewed changes

zuk3975 added 2 commits April 18, 2023 14:28

rename Editable{*} to {*}ForEditing

fc2bce1

fix saving cartRuleId in shared storage and unify status toggling me…

1c481c4

…thods for less code

zuk3975 closed this Apr 18, 2023

zuk3975 reopened this Apr 18, 2023

jolelievre reviewed Apr 18, 2023

View reviewed changes

validate cart rule code is unique and add behat scenarios

0f5e0c5

zuk3975 added 2 commits April 19, 2023 11:47

reset cart rule id in storage if code was edited

cd7489e

replace sharedStorage static usages

2592dd2

jolelievre approved these changes Apr 19, 2023

View reviewed changes

boherm approved these changes Apr 20, 2023

View reviewed changes

0x346e3730 approved these changes Apr 20, 2023

View reviewed changes

zuk3975 merged commit 7e71a2d into PrestaShop:develop Apr 20, 2023

jolelievre added this to the 9.0.0 milestone Apr 21, 2023

jolelievre removed the BC break Type: Introduces a backwards-incompatible break label Jun 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Edit cart rule CQRS command #32115

Edit cart rule CQRS command #32115

zuk3975 commented Apr 11, 2023 •

edited by jolelievre

Loading

zuk3975 Apr 17, 2023

jolelievre Apr 18, 2023

jolelievre Apr 18, 2023

jolelievre Apr 18, 2023

jolelievre Apr 18, 2023

jolelievre Apr 18, 2023

jolelievre Apr 18, 2023

jolelievre Apr 18, 2023

zuk3975 Apr 18, 2023 •

edited

Loading

jolelievre Apr 18, 2023

zuk3975 Apr 19, 2023

jolelievre Apr 18, 2023

jolelievre Apr 18, 2023

jolelievre Apr 18, 2023

jolelievre Apr 18, 2023

jolelievre Apr 18, 2023

zuk3975 Apr 18, 2023

jolelievre Apr 18, 2023

jolelievre left a comment

jolelievre Apr 18, 2023

jolelievre Apr 18, 2023

jolelievre Apr 18, 2023

zuk3975 Apr 19, 2023

jolelievre Apr 19, 2023

jolelievre left a comment

boherm left a comment

zuk3975 commented Apr 20, 2023

	class EditableCartRuleMinimum
	class CartRuleMinimumForEditing

	SharedStorage::getStorage()->set($cartRuleReference, new CartRule($cartRule->getValue()));
	$this->getSharedStorage()->set($cartRuleReference, new CartRule($cartRule->getValue()));

	$currencyId = SharedStorage::getStorage()->get($data['minimum_amount_currency']);
	$currencyId = $this->getSharedStorage()->get($data['minimum_amount_currency']);

Edit cart rule CQRS command #32115

Edit cart rule CQRS command #32115

Conversation

zuk3975 commented Apr 11, 2023 • edited by jolelievre Loading

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

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

zuk3975 Apr 18, 2023 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jolelievre left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jolelievre left a comment

Choose a reason for hiding this comment

boherm left a comment

Choose a reason for hiding this comment

zuk3975 commented Apr 20, 2023

zuk3975 commented Apr 11, 2023 •

edited by jolelievre

Loading

zuk3975 Apr 18, 2023 •

edited

Loading