magepal / magento2-google-tag-manager

Google Tag Manager is a user-friendly, yet powerful and cost-effective solution that is a must-have integration for every Magento store. It simplifies the process of adding and managing third-party JavaScript tags. With dozens of custom events and hundreds of data points our extensions the #1 GTM solution for Magento.
https://www.magepal.com/google-tag-manager.html
258 stars 88 forks source link

Price on ProductPage event is zero for configurable products #69

Open ericclaeren opened 3 years ago

ericclaeren commented 3 years ago

Magento version 2.3.5-p2 #:

Edition EE

Expected behavior:

The min price to be displayed for a configurable product.

Actual behavior:

Price is always displayed as 0.

Steps to reproduce:

  1. Visit configurable product page
  2. Inspect Datalayer ProductPage

What happened?

The change was introduced here: https://github.com/magepal/magento2-google-tag-manager/compare/2.4.0...2.5.1#diff-4431d83985d4d014ab1c33820ee280c9ebd92203b337426d3bedb772c2018831R104

It has changed from getFinalPrice() to getPrice() which broke this.

srenon commented 3 years ago

@ericclaeren .... sorry about this... I will push a new update shortly

srenon commented 3 years ago

@ericclaeren... please upgrade to the latest version

ericclaeren commented 3 years ago

Wow that was incredibly fast, thanks @srenon.

I'm afraid there's still something not right. I'm was expecting a price with the same amount as shown on the configurable page. Except I'm getting the price excluding VAT. That's probably due to the getBaseAmount() change and not the real final price.

Would you mind looking into that? Thanks

srenon commented 3 years ago

Do you have Opengraph enabled? What price does it show vs our extension? image

I'm using identical code to what use for Opengraph see

https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/Catalog/view/frontend/templates/product/view/opengraph/general.phtml#L18

Can you check the value for $product->getPrice() and $product->getFinalPrice() and let me know if any return the carrect value you are looking for?

ericclaeren commented 3 years ago

Hi @srenon

OpenGraph is not enabled. This issue is occuring in the datalayer push, not in the part you're showing in the screenshot. Although it uses the same code.

https://github.com/magepal/magento2-google-tag-manager/blob/master/Block/Data/Product.php#L73 https://github.com/magepal/magento2-google-tag-manager/blob/master/Block/Data/Product.php#L88

image

ericclaeren commented 3 years ago

The initial commit already had logic for simple/configurable based on getFinalPrice() any idea what that was removed? And would reverting that commit won't fix this issue?

https://github.com/magepal/magento2-google-tag-manager/commit/22a4b76d2839c1fc00159981284260186d7802c3

srenon commented 3 years ago

@ericclaeren ... It was not working correct for discount/special price

srenon commented 3 years ago

@ericclaeren .... is VAT 15%?

ericclaeren commented 3 years ago

Hi @srenon No it's 21% in The Netherlands (34.95/121 * 100). Magento and prices, still don't know every difference. Saw some mentions of price in multiple posts.

https://magento.stackexchange.com/a/235623 Which says final is equal to special, but don't know if that's the case. But also think that getPrice() may not take catalog rules into account.

This post says there isn't one method to get the correct price and depends on the type of product. https://magento.stackexchange.com/a/283015

Think the last post may be correct, my guess is that you always want to match the price displayed on the page, from my understanding that is the final price, because of the applying of catalog rules and special prices.

ericclaeren commented 3 years ago

@srenon Are there any developments going concerning this issue? Maybe re-open this issue so other members may participate in providing a solution?

srenon commented 3 years ago

I will figure out a fix for the by the end of the week

ericclaeren commented 3 years ago

Hi @srenon How are you? Is there any progress on this issue?

srenon commented 3 years ago

@ericclaeren ... Could you let me know if this temporary workaround works?

    /** @var $product ProductInterface */
    if ($this->getProduct()) {
        $price = $this->getProduct()
            ->getPriceInfo()
            ->getPrice(FinalPrice::PRICE_CODE)
            ->getAmount()
            ->getBaseAmount() ?: 0;
    }

    if (!$price) {
        if ($product->getTypeId() == Type::TYPE_SIMPLE) {
            $price = $tm->formatPrice($product->getPrice());
        } else {
            $price = $tm->formatPrice($product->getFinalPrice());
        }
    }

Also, all the other European/vat sites that I have work on does not seem to have this issue

image

image

srenon commented 3 years ago

@ericclaeren ... are you still having issues with the latest release?

ericclaeren commented 3 years ago

Hi @srenon Sorry I didn't respond earlier.

I have checked the latest release 2.6.0 against the same configurable product and a simple product and now both seem broken or at least don't return the expected display price.

Simple product type: image

Configurable product type: image

I have updated all magepal packages, ran setup upgrade and clear all my caches. composer require magepal/magento2-core magepal/magento2-enhanced-ecommerce magepal/magento2-googletagmanager

The code in 2.5.0 gives the expected result, isn't it possible to just revert to that or test against that code?

srenon commented 3 years ago

If you take a look at https://github.com/magepal/magento2-google-tag-manager/blob/master/Helper/Data.php#L265

If the price is zero then it would use the old logic. So it should be identical to the previous code

public function getProductPrice($product)
{
    $price = 0;

    /** @var $product ProductInterface */
    if ($product) {
        $price = $product
            ->getPriceInfo()
            ->getPrice(FinalPrice::PRICE_CODE)
            ->getAmount()
            ->getBaseAmount() ?: 0;
    }

    if (!$price) {
        if ($product->getTypeId() == Type::TYPE_SIMPLE) {
            $price = $product->getPrice();
        } else {
            $price = $product->getFinalPrice();
        }
    }

    return $this->formatPrice($price);
ericclaeren commented 3 years ago

Hi @srenon

Yes, that's true, but if the price isn't zero, which is probably the case, the logic is incorrect. Why don't return to the old logic? That seemed to work.

/** @var $product ProductInterface */
  if ($product = $this->getProduct()) {
      if ($product->getTypeId() == Type::TYPE_SIMPLE) {
          $price = $product->getPrice();
      } else {
          $price = $product->getFinalPrice();
      }
  }
ericclaeren commented 3 years ago

Hi @srenon After some time away from this issue, ran into this again and decided to search for a possible solution. Could find any cases in which the PR didn't work, but you might have some? In case you do, please let me know.

Cheers, Eric