Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect values for fields price and price_incl in feed for products with prices below 1 EUR #218

Closed
s-renz opened this issue Mar 5, 2024 · 5 comments
Assignees
Labels
Fixed and planned for next release Fix will be included in the next release.

Comments

@s-renz
Copy link

s-renz commented Mar 5, 2024

Hi,

we recently got some notifications in Google Merchant Center for products that have prices differ in feed and on the page.

When we looked further into it we noticed that the common thing between these products is that they are all below 1 EUR. In this case the min_price and max_price correctly show the price without tax, however the fields "price" and "price_excl" already include the tax and as such the field "price_incl" will include the already taxed price including tax.

image

We traced the issue back to this line:
https://github.com/magmodules/magento2-channable/blob/master/Helper/Product.php#L1006

Since the price is below 1 and we're talking floats here the else-branch is executed. So instead of using

$price = $product->getPrice();

this code is used:

$price = $product->getPriceInfo()->getPrice('regular_price')->getAmount()->getValue();

Which returns the price already taxed, which is then put into processPrice and taxed again due to how the store configuration is set. As far as I can see there are two changes that work, however I don't know enough about Magento's price calculation to know if this is indeed a bug in Magento or in the Channable Extension, which is why I didn't want to do a pull request directly.

As far as I can see it, the AmountInterface always returns the price with all things included, unless you explicitly tell getValue() what to exclude:

$price = $product->getPriceInfo()->getPrice('regular_price')->getAmount()->getValue('tax');
or
$price = $product->getPriceInfo()->getPrice('regular_price')->getAmount()->getValue(['tax']);

The correct price is also returned when not using the Amount interface but instead get the Value directly:

$price = $product->getPriceInfo()->getPrice('regular_price')->getValue();

But again I am not qualified enough to know the difference or if that would break anything.

Could someone look into this please?

@Frank-Magmodules
Copy link
Member

Hello @s-renz, thanks for bringing up this issue. We're currently looking into it, taking into account your detailed description and thorough review of the affected code lines and locations. I'll update you shortly, hopefully with a PR and/or a release where we've addressed this.

@Frank-Magmodules Frank-Magmodules self-assigned this Mar 12, 2024
@Frank-Magmodules Frank-Magmodules added Question / Issue Further information is requested Investigating We are working on this issue together with the customer. and removed Question / Issue Further information is requested labels Mar 12, 2024
@s-renz
Copy link
Author

s-renz commented Mar 19, 2024

Any news on this yet?

@Frank-Magmodules
Copy link
Member

Hello @s-renz, Yes, sorry for the delay but we have good news! We've addressed this issue with a fix that will be included in the upcoming release later this month. Thank you for your patience, and I've also ensured that the appropriate label has been added to this issue.

@Frank-Magmodules Frank-Magmodules added Fixed and planned for next release Fix will be included in the next release. and removed Investigating We are working on this issue together with the customer. labels Mar 19, 2024
@s-renz
Copy link
Author

s-renz commented Mar 20, 2024

Hello @s-renz, Yes, sorry for the delay but we have good news! We've addressed this issue with a fix that will be included in the upcoming release later this month. Thank you for your patience, and I've also ensured that the appropriate label has been added to this issue.

Thanks! Looking forward to the new version

Marvin-Magmodules added a commit that referenced this issue Mar 25, 2024
@Frank-Magmodules
Copy link
Member

Hi There @s-renz ! Thank you again for opening this issue. I'm happy to share that we have covered this issue in our new release. I'm marking this issue as resolved for now. Please don't hesitate to reopen it if necessary. Thank you again for opening this issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed and planned for next release Fix will be included in the next release.
Projects
None yet
Development

No branches or pull requests

2 participants