Bugfixes for order base manager

[nvindice]

2 Bugfixes:

• in getCoupons() matching products to coupon codes would not work, because the products are stored by their position index in $products[$baseId] but matched by their product ids.

• in loadFresh() coupon codes were not loaded, even when with \Aimeos\MShop\Order\Item\Base\Base::PARTS_COUPON bit set in the $parts bitmask. There are some extra steps required in order to get this task done, but everything should work as intended and documented now.

[nvindice]

As for the failing tests: It seems to be kind of intended behaviour that no coupons are loaded. Or quick&dirty test writing ("as long as it works..."). Can you please explain the purpose of the $fresh parameter and coupon handling to me?

IMO, not having the opportunity to load coupons with a copied basket makes no sense at all. Problem is: the coupon products are loaded (eg a rebate product), but the coupon is not. So we have no chance to remove the coupon product (because we don't know it belongs to a coupon).

[aimeos]

Coupon shouldn't be loaded for new orders but you are right, this design decision is problematic because you can't remove the coupon product. Alternative to your approach would be to leave out the products with IMMUTABLE flag.

Maybe a combined approach would be the best way.

[nvindice]

Why not let the user decide if he wants to load coupons? In our case, we have built the functionality to store a basket and load it in another session.

[aimeos]

Yes, that's what I've meant with "combined approach". Remove the coupon products if they are not desired and load the complete basket otherwise.

[nvindice]

Can we be sure that only coupon products have the IMMUTABLE flag?

[aimeos]

Currently: Yes

[nvindice]

Another suggestion: We could fetch all products with IDs (not fresh) and all coupon codes, remove the coupon products from the product array and clean the IDs afterwards (as in my current coupon handling routine). This would require more logic, but would be a less intrusive and more downwards-compatible approach. Maybe some users have added their own logic which uses own IMMUTABLE flags in the past?!

So what variant would you like me to build for the fresh-without-coupons case: a) current behaviour: all products are fetched, even coupon products, without their coupon code b) Variant 1: Fetch all products but leave out those with IMMUTABLE flag c) Variant 2: Fetch everything, sort out the coupon products, remove all IDs afterwards (to have a fresh basket) d) Variant 3: I could store the logic of Variant 2 inside the getProducts() function - this would require an additional lookup of the coupons inside this function, but way less logic because the coupon products would be sorted out earlier with $fresh set.

EDIT: Added variant 3

[aimeos]

We would opt for the simplest way which should be variant 1 because:

• Low probability of side effects (IMMUTABLE flag is only used for coupons and loading fresh baskets with coupons wasn't supported up to now while IMMUTABLE products in new baskets are seen as an error)
• Only minimal code change required

[nvindice]

Okay, I implemented this solution. Everything good now, before I start fixing the tests?

[nvindice]

In the tests, the coupon products are not flagged as IMMUTABLE (and therefore loaded twice). How can I achieve that?

[aimeos]

You can try to add the flag in the unit test data provided there are no big side effects (small changes to for existing unit tests are OK): https://github.com/aimeos/aimeos-core/blob/master/lib/mshoplib/setup/unittest/data/order.php#L27-L43

[nvindice]

Should work now. Finally. That was more effort than expected...

[coveralls]

Coverage decreased (-1.9%) to 89.251% when pulling af2063de60c2137f9848cf7edb26a04c0755ac60 on nvindice:2018.10 into 5e34c1b70110cfcfdb8f3e6368ecebcd95d102bc on aimeos:2018.10.

[coveralls]

Coverage decreased (-0.7%) to 90.454% when pulling af2063de60c2137f9848cf7edb26a04c0755ac60 on nvindice:2018.10 into 5e34c1b70110cfcfdb8f3e6368ecebcd95d102bc on aimeos:2018.10.

[aimeos]

Thanks a lot!