search

aimeos / aimeos-core

Aimeos PHP e-commerce framework for ultra fast online shops, scalable marketplaces, complex B2B applications and #gigacommerce
https://aimeos.org
Other
3.38k stars 116 forks source link

Register order plugins earlier when loading an order #280

Closed nvindice closed 2 years ago

nvindice commented 2 years ago

When loading an order with the order base manager as a fresh order, the order elements (address, products, services, coupons) are added successively. However, the order plugins are not registered yet. Because of this behaviour, coupons are not added properly, as the coupon item is not added and therefore the rebate is not considered in the basket. Other plugins cannot provide their functionality, too, eg. address based order manipulation etc.

nvindice commented 2 years ago

Can you help me understand why the tests fail? IMO, the check in the Coupon plugin provider should not throw an exception (as the coupon code from the order table should be valid).

aimeos commented 2 years ago

One of the coupon codes isn't available any more: https://github.com/aimeos/aimeos-core/blob/master/lib/mshoplib/setup/unittest/data/coupon.php#L14-L15

nvindice commented 2 years ago

I see, so these tests should never have succeeded in the first place...

What is your favored solution? Shall I extend the availability date or is the coupon unavailable on purpose? Shall I change the coupon used in this demo order?

aimeos commented 2 years ago

The coupon code is unavailable on purpose.

Not yet sure what the best solution is but the customer should be notified about the coupon problem. If an exception is thrown, the coupons should be added at last to the basket so the products, adresses and services are already in the basket.

It's no problem for subscriptions because the situation is already handled there: https://github.com/aimeos/ai-controller-jobs/blob/master/controller/jobs/src/Controller/Jobs/Subscription/Process/Renew/Standard.php#L214-L237

Loading an order as new is only used when customers want to reorder the same products. In that case, an exception is OK to inform them about the problem so the solution might be to add another test for loading an order into the basket without an expired coupon code.

nvindice commented 2 years ago

Well... I started fixing this by adding another coupon code and discovered a lot of failing stuff (which should never have succeeded as far as I can tell). So I fixed a couple of other tests. However, with the last failing test I am not sure what to do:

-> 1) Aimeos\MShop\Order\Manager\Base\StandardTest::testStoreExisting:710 This error results from the comparison of two orders by their product array's keys. I am not sure why this would be a good idea or part of a test. The plugins change the key of the coupon product so this test fails.

What should I do with this test?

aimeos commented 2 years ago

If you change the unit test data, several other tests must be adapted too if they depend on the same unit test data - especially if the new coupon code changes the order details completely. An alternative would be to use a coupon whose requirements are not met so the order details stays the same but it's still valid.

nvindice commented 2 years ago

That's not really the point here...

First of all, most problems did not result from the changed unit test data, but from my initial commit which registers the order plugins before loading a fresh order. IMHO, this change makes a lot of sense, since some plugins like Coupon, FreeShipping etc. should run as early as possible to prevent invalid or incorrect order baskets. That's why they used hooks like addCoupon.after, right?!

Registering these plugins showed, that a couple of tests should not have succeeded before. Eg. the invalid coupon should have been removed (or thrown an exception or whatever) while loading the order instead of being stored as an active coupon in the order basket. This led to a couple of wrong assertions in the tests.

In order to fix this, I added another - valid - coupon code and used this one in some of the tests instead, eg. in testGetUsedRebate(), because I don't see any value in a test that tests an invalid (!) coupon and succeeds if it gets loaded (incorrectly).

Another problem was the FreeShipping configuration, which (should have) removed shipping costs in all test orders (but did not, because the plugin was not loaded before). This led to invalid assertions regarding the costs when the plugin was loaded correctly.

So, the problems were not introduced by the changed unit test data in the first place, but by the correct implementation of the order plugins. This just showed that several tests assumed wrong data.

The only problem left is at Aimeos\MShop\Order\Manager\Base\StandardTest::testStoreExisting - I do not understand the idea behind comparing two orders which have been loaded (and processed!) in different ways by comparing their basket product array index (!). Two orders may still be the same if the array index keys differ, right? If so, this test should not compare this way.

aimeos commented 2 years ago

The reason behind this test is to check if the order is exaclty the same if it's stored again. There must be no differences and keys or something else in that case because otherwise, it's likely that there's another problem lurking. Can you find out why those keys are different?

nvindice commented 2 years ago

Yes, the keys are different because $basket is loaded as a fresh order, which (now) triggers the Coupon plugin and the coupon update routine multiple times. Each time this happens, the coupon product is unset() and created newly, see: https://github.com/aimeos/aimeos-core/blob/f05ec302c367f2b5200d82e8c3dcf7ffe13a0eb5/lib/mshoplib/src/MShop/Order/Item/Base/Base.php#L525

The reference order $newBasket gets loaded without the $fresh parameter, so all existing products (including the coupon product) are added without any modification.

aimeos commented 2 years ago

OK, if it's only the keys that are different, the tests should suceed nevertheless. Can you adapt the test to exclude testing the keys? You can use array_keys() to streamline the keys.

aimeos commented 2 years ago

Thanks a lot!

aimeos commented 2 years ago

We've merged your PR to the master branch but there's something fishy because we had to undo your order price changes to make the tests succeed again: https://github.com/aimeos/aimeos-core/commit/15976ce2f38dfe2ed10c02681aeb3e7d5d518e7a

nvindice commented 2 years ago

Well, as far as I can tell the EUR 5 difference comes from the shipping service costs. When the coupon code CDEF is added in testLoadStoreCoupons(), the shipping costs are reduced by 5 EUR (=> 1.50 EUR) and the rebate is increased from 9.50 EUR -> 14.50 EUR. I can't see why the order costs should be 1.50 EUR instead of 6.50 EUR before adding the coupon code. Maybe the shipping service gets removed somehow, eg. by the Shipping plugin which does not consider the correct (new) threshold?!

EDIT: This plugin configuration (the increased Shipping plugin threshold value) was the original reason for the price changes in the unit tests IIRC.