/dev/notes

I fixed a nasty little bug in GoogleCheckout (now Wallet) today. Basically if a customer has a free or zero priced product in their cart, GoogleCheckout will return an error looking something like this:

Google Checkout: Error parsing XML; message from parser is: cvc-datatype-valid.1.2.1: '' is not a valid value for 'decimal'.

I have developed custom modules which add free or bonus items to a customer's cart if they use coupons, meet certain cart criteria or belong to particular customer groups. Buy x, get y rules also work this way. So this is a nuisance. Luckily few customers opt to use GoogleCheckout, but still, I don’t Live with Broken Windows[1].

Chasing the problem down the call stack leads to app/code/core/Mage/GoogleCheckout/Model/Api/Xml/Checkout.php and specifically the _getItemsXml() method.

$unitPrice = $item->getBaseCalculationPrice();
  if (Mage::helper('weee')->includeInSubtotal()) {
      $unitPrice += $item->getBaseWeeeTaxAppliedAmount();
  }
  // ...
  <unit-price currency="{$this->getCurrency()}">{$unitPrice}</unit-price>
  

Now, if the product's baseprice is 0, then for some unfathomable reason it's set to '', not 0. As the unit-price element expects a decimal value, an empty string fails validation.

The fix is pretty trivial

$unitPrice = $item->getBaseCalculationPrice();
  if (Mage::helper('weee')->includeInSubtotal()) {
      $unitPrice += $item->getBaseWeeeTaxAppliedAmount();
  }
  
  $unitPrice = ((float) $unitPrice > 0) ? $unitPrice : 0.00;
  

The store I needed to fix only used US dollars so I haven't tested how the use of other currencies or locales might affect this fix.

To apply the fix, don't modify the core codepool, but instead take advantage of the local and community codepool's higher classloader priority[2] and place the amended code in app/code/local/Mage/GoogleCheckout/Model/Api/Xml/Checkout.php.

[1]: 'Don't Live With Broken Windows' is a tip I first read about in The Pragmatic Programmer. It is used to help fight Software Entropy (software's tendency to lose structure over time). This concept has parallels with the real world as urban areas with broken windows tend to see higher levels of vandalism when compared to areas where windows are constantly maintained.

When you ignore small problems it becomes easier to let more significant problems slide too. Hence the rule of thumb, 'Dont Live With Broken Windows'.

[2]: Magento resolves classes in this order local, community then core. This means if two classes have the name Mage_Core_Model_Foo one exists in local the other in core, then the version in local is used.

I came across a particularly nasty bug in Magento 1.6.2.0 last night where calling Mage::getSingleton('cataloginventory/stock_status')->rebuild() would set all grouped products to be out of stock. This didn't happen in 1.5 -however the cataloginventory status handling changed dramatically between 1.5 and 1.6

Forcing the cataloginventory_stock indexer to re-run fixes the situation but if you want to script the status update of many stock items, you can have a short period where your store's products will be unavailable.

Stepping through the issue I found myself in app/code/core/Mage/Catalog/Model/Resource/Product/Status.php and specifically the getProductStatusMethod()

/**
   * Retrieve Product(s) status for store
   * Return array where key is a product_id, value - status
   *
   * @param array|int $productIds
   * @param int $storeId
   * @return array
   */
  public function getProductStatus($productIds, $storeId = null)
  {
     $statuses = array();
  
     $attribute      = $this->_getProductAttribute('status');
     $attributeTable = $attribute->getBackend()->getTable();
     $adapter        = $this->_getReadAdapter();
  
     if (!is_array($productIds)) {
         $productIds = array($productIds);
     }
  
     if ($storeId === null || $storeId == Mage_Catalog_Model_Abstract::DEFAULT_STORE_ID) {
         $select = $adapter->select()
             ->from($attributeTable, array('entity_id', 'value'))
             ->where('entity_id IN (?)', $productIds)
             ->where('attribute_id = ?', $attribute->getAttributeId())
             ->where('store_id = ?', Mage_Catalog_Model_Abstract::DEFAULT_STORE_ID);
  
         $rows = $adapter->fetchPairs($select);
     } else {
         $valueCheckSql = $adapter->getCheckSql('t2.value_id > 0', 't2.value', 't1.value');
  
         $select = $adapter->select()
             ->from(
                 array('t1' => $attributeTable),
                 array('value' => $valueCheckSql))
             ->joinLeft(
                 array('t2' => $attributeTable),
                 't1.entity_id = t2.entity_id AND t1.attribute_id = t2.attribute_id AND t2.store_id = ' . (int)$storeId,
                 array('t1.entity_id')
             )
             ->where('t1.store_id = ?', Mage_Core_Model_App::ADMIN_STORE_ID)
             ->where('t1.attribute_id = ?', $attribute->getAttributeId())
             ->where('t1.entity_id IN(?)', $productIds);
         $rows = $adapter->fetchPairs($select);
     }
  
     foreach ($productIds as $productId) {
         if (isset($rows[$productId])) {
             $statuses[$productId] = $rows[$productId];
         } else {
             $statuses[$productId] = -1;
         }
     }
  
     return $statuses;
  }
  

This method goes through a list of productIds, and will assign a status id to them, this is typically used on grouped products when determining if all their children stock items are out of stock.

In testing, the status ids were all coming back as -1, i.e. not valid, and so therefore the group was out of stock.

In my code the store id was neither null, nor the default store id, so execution fell through to the else branch. At first I inserted a print_r($select->assemble()) to see the SQL being generated. The SQL was fine and when pasting it into MySQL I got a bunch of valid looking results. Funnily though, the status column was first, and the product id column was second (unlike the if branch, where they were in reverse order). This presents a problem when we reach the fetchPairs() statement.

Zend DB's fetchPairs returns an associative array resultset where column a is the key, and column b is the value. Because the SQL was returning the status column first (i.e. the key value) the result set consisted of just 2 rows (for each unique status code). In order for this code to work as you would expect the entity id (product id) needs to be first in the result set, then it gets used as a key.

The fix is straight forward enough, replace

$select = $adapter->select()
              ->from(
                  array('t1' => $attributeTable),
                  array('value' => $valueCheckSql))
  

with

$select = $adapter->select()
              ->from(
                  array('t1' => $attributeTable),
                  array('entity_id', 'value' => $valueCheckSql))
  

This way the product id is always used as the key in fetchPairs and you get a status result for each product.