Some fixes for customer group pricing

[rbrown]

Hello, I've noticed what I think are a couple of issues with customer group pricing, here's a pull request that fixes them.

I think there have been some inconsistencies with currencies/special prices etc. I don't think it will fix #1116 but I need to read through that again. I've essentially re-written the end of handlePrice in the Product Entity Helper, I'm going to try and comment on the specific changes to explain some of the things I did.

[rbrown]

I found it really hard to read $customData[$field][$currency_code] everywhere, so I renamed this and added it at the end, I apologise if this breaks your code style

[damcou]

You're right, this is painful to use in the rest of the function. On the other hand $cdfc isn't very meaningful and maybe too short. Could you please consider to change the variable name in something meaningful like $currencyData maybe ? Thx in advance :)

[rbrown]

I hope these changes are acceptable, understandable, the other change I didn't quite flag was that in the handlePrice I check if the special_price is valid or will be valid, if it's not we don't export any thing, otherwise we export the start/end date. I think the original JS assumed that special_prices were valid at the time of export, so a special_price that started in a few days would show up early in the search results.

[rbrown]

Turns out I'm not the world's greatest tester. I'm not sure why I though changing a blank special expiry date to the current day would be a good idea when they day ticked over to the next day, but this latest commit fixes that.

[rbrown]

Re: configurable product prices - I've done some investigation, and I have a suggestion that would probably work.

When adding the prices to the original product collection, the min/max prices are being pulled out of the pricing index already, for group_0.

For a product with price 13.75, group price 4.15 and one child set at +1.00 and one set at -1.00, The index stores prices like this

enity_id	customer_group_id	price	final_price	min_price	max_price
SKU	0	13.75	13.75	12.75	14.75
SKU	1	13.75	4.15	3.15	5.15

for group_0/default prices the min/max are already loaded into the product object. For the later groups you could calculate the offsets (+1.00 -1.00) from group0 and the offsets to the groups.

Even if there is a special_price currently active you can use the three prices (final/min/max) to calculate the correct offsets for the normal / special_price

I won't try and implement this until I hear your opinion on these other changes

[damcou]

Hello @rbrown ,

First, apologies for the late answer and most of all, thanks a lot for taking the time to propose such big PRs !
I had a look at your changes, tried to understand everything, I must admit it's not that easy, especially when it's about this part of the extension which is really not easily maintainable :( .

The changes look good to me, but I'm struggling to find ways to test everything, in the description you mentioned "some inconsistencies with currencies/special prices" . Could you please tell me more about it ?

Do you have any easy use-case that I can reproduce on my own, which will lead to those inconsistencies so I can test before/after the PR to see the differences?

So far, I tried to activated the customer groups, setting different customer prices and a special price, and from what I see, the special price is taken into account. Should I choose a particular type of product ? Should I set a particular configuration ?

Thanks again for this.

[rbrown]

Wow, yes, I see I never really said what it was fixing, sorry. Here's some product data



Note the special price is in the future.

This is the dashboard with current develop branch

This is the dashboard with my branch

So, although the current branch knows there's special_from and special_to, it doesn't know what the special price is and won't be able to use it. In the new version I used default et al for the special_price and original for the non special price, then in the js on the front end check if the current date is in the special period. I also include the original for all the groups, so that group 4 can say was £190 now £134, not was £200 because that's never the group4 price. Maybe when there's no special price I should be exporting the default price as the group_original prices

Other problems were the use of product->getFinalPrice() which is a cached call, and product->getPriceModel()->getFinalPrice() which is calculated and writes to final_price. So because the codes loops with/without tax, currency and groups the first time product->getFinalPrice is called in any loop after the very first, it returns the most recently called product->getPriceModel()->getFinalPrice() which will give you the price for the highest numbered group. In this example I set price to 200, and the group price for group 5 ( the highest number i have) to 190, current develop does this where I have one currency but use both with and without prices

This branch does this:

There was also some inconsistency between group_0 and default, some bits seemed to expect to replace group_0 with default and some didn't so transformHit wasn't always getting called.

I hope that's a little clearer, sorry for not providing examples earlier

[damcou]

Hello @rbrown ,

Thanks a lot for these examples, it's more clear to me now!
I tried to make the exact same tests on my own and I could reproduce everything 👍

So if I sum up now, there are two main points :

• The special price timeframes are currently not handled by the extension. If you set a special price in the future, this price is not indexed and won't be displayed once the timeframe is reached.
  Indeed, the changes you made seem to fix this.
  • If the timeframe is reached, we display [price]_original_formated (strikethrough text) + [price]_formated
  • If the timeframe is not reached, we only display [price]_original_formated

You made a test with customer groups enabled, but I also tried the same test with customer groups disabled and it seems to fix it as well, does it make sense to you ?

• There is a bad calculation of the prices when customer groups are enabled AND when we display excl. and incl. tax prices.
  It is due to a bad use of the getFinalPrice() method that returns a cached value when we iterate more than once inside the "taxes fields" loop instead of redoing the calculation.
  To prevent that, you now use the getGroupPrice() method instead.

Did I miss something ?

For me the first point is an improvment of the handlePrice() method (it now handles special prices coming in a future timeframe), and the second point is a fix (price_with_tax object was just wrongly calculated) . Do you agree with that ?

If you confirm that I didn't miss or misunderstood anything, I'll ask one of my colleague to double check (as it's a quite sensitive part of the extension) and if it looks ok to him/her as well, I think we can merge it :) .

Thanks again for this PR.

Have a nice week-end.
Best.

[rbrown]

Hi, @damcou

• The special price timeframes are currently not handled by the extension. If you set a special price in the future, this price is not indexed and won't be displayed once the timeframe is reached.
  Indeed, the changes you made seem to fix this.
  • If the timeframe is reached, we display [price]_original_formated (strikethrough text) + [price]_formated
  • If the timeframe is not reached, we only display [price]original_formate
    You made a test with customer groups enabled, but I also tried the same test with customer groups disabled and it seems to fix it as well, does it make sense to you ?

Yes, it should be doing the right thing for default_ prices, and then if customer groups are enabled, it should go through each one and do that properly too. Plus it should be consistent in how it generates this timestamp for the end/start times and the current time that is sent on each page load

There is a bad calculation of the prices when customer groups are enabled AND when we display excl. and incl. tax prices.
It is due to a bad use of the getFinalPrice() method that returns a cached value when we iterate more than once inside the "taxes fields" loop instead of redoing the calculation.
To prevent that, you now use the getGroupPrice() method instead.

Yes, to what was happening. The fix is sort of the side effect of not using finalPrice. We can't use getFinalPrice, because it's affected by whether any special price is currently active. To handle future special prices correctly, and the group ones, we end up always using getPrice, getSpecialPrice and getGroupPrice

If you confirm that I didn't miss or misunderstood anything

I think you've understood it correctly, thank you. There is one thing I must point out, excluding changes because I re-named variables, my changes all stop at the point where handlePrice tries to calculate the price of configurable products. Because the existing code is 'wrong' and I'm not sure how to fix it.

We discussed this last year in #1043. I think, if you're happy with this pull request, that I can rewrite the configurable (and hopefully bundle/grouped we don't use them internally) to use the price index to get the correct offsets for the children prices and then calculate the correct prices in all groups. The problems that are obvious to me are:

• It relies on the price index for that product being correct
• It fixes the configurable pricing for vanilla magento installs, I am aware that there extensions that use the price of the simple children, and you may have customers using an extension like that, for whom the current behaviour is correct.

If we can't use the index, I can't see a solution that I'm happy with, short of copying wholesale the code magento already uses to calculate the configurable prices, and that makes my head hurt.

I hope you have a great weekend too

[damcou]

Hi @rbrown ,

My colleague just had a second look at the PR and everything seems fine ! (except the name of the $cdfc variable as mentioned in the corresponding conversation.
Once it's changed, we will be able to merge it ! :)

Regarding the conflgurable price calculation, yes I remember seeing your previous PR and it's still in our backlog to be processed in the future, but I can't guarantee any timeframe for it :( .
But if you want to submit a new PR about it, feel free to do it, that would be very nice !

Best.
Damien

[rbrown]

Hey @damcou The rename is done, I'll add the configurable prices to my backlog as well :p