diff --git a/app/code/core/Mage/Sales/Model/Config/Ordered.php b/app/code/core/Mage/Sales/Model/Config/Ordered.php index bd89d332946a8..4e72804e803ec 100644 --- a/app/code/core/Mage/Sales/Model/Config/Ordered.php +++ b/app/code/core/Mage/Sales/Model/Config/Ordered.php @@ -140,36 +140,46 @@ protected function _getSortedCollectorCodes() $element = current($configArray); if (isset($element['sort_order'])) { uasort($configArray, array($this, '_compareSortOrder')); + $sortedCollectors = array_keys($configArray); } else { - foreach ($configArray as $code => $data) { - foreach ($data['before'] as $beforeCode) { - if (!isset($configArray[$beforeCode])) { + $sortedCollectors = array_keys($configArray); + // Move all totals with before specification in front of related total + + foreach ($configArray as $code => &$data) { + foreach ($data['before'] as $positionCode) { + if (!isset($configArray[$positionCode])) { continue; } - $configArray[$code]['before'] = array_unique(array_merge( - $configArray[$code]['before'], $configArray[$beforeCode]['before'] - )); - $configArray[$beforeCode]['after'] = array_merge( - $configArray[$beforeCode]['after'], array($code), $data['after'] - ); - $configArray[$beforeCode]['after'] = array_unique($configArray[$beforeCode]['after']); - } - foreach ($data['after'] as $afterCode) { - if (!isset($configArray[$afterCode])) { - continue; + if (!in_array($code, $configArray[$positionCode]['after'], true)) { + // Also add additional after condition for related total, + // to keep it always after total with before value specified + $configArray[$positionCode]['after'][] = $code; } - $configArray[$code]['after'] = array_unique(array_merge( - $configArray[$code]['after'], $configArray[$afterCode]['after'] - )); - $configArray[$afterCode]['before'] = array_merge( - $configArray[$afterCode]['before'], array($code), $data['before'] - ); - $configArray[$afterCode]['before'] = array_unique($configArray[$afterCode]['before']); + $currentPosition = array_search($code, $sortedCollectors, true); + $desiredPosition = array_search($positionCode, $sortedCollectors, true); + if ($currentPosition > $desiredPosition) { + // Only if current position is not corresponding to before condition + array_splice($sortedCollectors, $currentPosition, 1); // Removes existent + array_splice($sortedCollectors, $desiredPosition, 0, $code); // Add at new position + } + } + } + // Sort out totals with after position specified + foreach ($configArray as $code => &$data) { + $maxAfter = null; + $currentPosition = array_search($code, $sortedCollectors, true); + + foreach ($data['after'] as $positionCode) { + $maxAfter = max($maxAfter, array_search($positionCode, $sortedCollectors, true)); + } + + if ($maxAfter !== null && $maxAfter > $currentPosition) { + // Moves only if it is in front of after total + array_splice($sortedCollectors, $maxAfter + 1, 0, $code); // Add at new position + array_splice($sortedCollectors, $currentPosition, 1); // Removes existent } } - uasort($configArray, array($this, '_compareTotals')); } - $sortedCollectors = array_keys($configArray); if (Mage::app()->useCache('config')) { Mage::app()->saveCache(serialize($sortedCollectors), $this->_collectorsCacheKey, array( Mage_Core_Model_Config::CACHE_TAG diff --git a/dev/tests/integration/testsuite/Mage/Sales/Model/Config/OrderedTest.php b/dev/tests/integration/testsuite/Mage/Sales/Model/Config/OrderedTest.php new file mode 100644 index 0000000000000..ec73c5ac4c760 --- /dev/null +++ b/dev/tests/integration/testsuite/Mage/Sales/Model/Config/OrderedTest.php @@ -0,0 +1,202 @@ +_restoreUseCache = Mage::app()->useCache('config'); + $this->_model = $this->getMockForAbstractClass('Mage_Sales_Model_Config_Ordered'); + Mage::app()->getCacheInstance()->banUse('config'); + + } + + /** + * Test total collector sorting algorithm + * + * @dataProvider totalCollectors + */ + public function testGetSortedCollectorCodes($totalConfig) + { + $reflection = new ReflectionObject($this->_model); + // Fill in prepared data for test + $property = $reflection->getProperty('_modelsConfig'); + $property->setAccessible(true); + $property->setValue($this->_model, $totalConfig); + $property->setAccessible(false); + + // Calling sorting method + $method = $reflection->getMethod('_getSortedCollectorCodes'); + $method->setAccessible(true); + $result = $method->invoke($this->_model); + + $this->assertInternalType('array', $result, 'Result of method call is not an array'); + + // Evaluating the result + foreach ($totalConfig as $total) { + $totalPosition = array_search($total['_code'], $result); + + // Walking through total after positions, + // to check that our total really placed after them + foreach ($total['after'] as $afterTotal) { + $afterTotalPosition = array_search($afterTotal, $result); + $this->assertLessThan( + $totalPosition, $afterTotalPosition, + sprintf('Total with code "%s" is not after "%s"', $total['_code'], $afterTotal) + ); + } + + // Walking through total before positions, + // to check that our total really placed before them + foreach ($total['before'] as $beforeTotal) { + $beforeTotalPosition = array_search($beforeTotal, $result); + $this->assertGreaterThan( + $totalPosition, $beforeTotalPosition, + sprintf('Total with code "%s" is not before "%s"', $total['_code'], $beforeTotal) + ); + } + } + } + + /** + * Test data provider for testing totals sorting algorithm + * + * @return array + */ + public function totalCollectors() + { + $coreTotals = array( + // Totals defined in Mage_Sales + 'nominal' => array('_code' => 'nominal', + 'before' => array('subtotal'), + 'after' => array()), + + 'subtotal' => array('_code' => 'subtotal', + 'after' => array('nominal'), + 'before' => array('grand_total')), + + 'shipping' => array('_code' => 'shipping', + 'after' => array('subtotal', 'freeshipping', 'tax_subtotal'), + 'before' => array('grand_total')), + + 'grand_total' => array('_code' => 'grand_total', + 'after' => array('subtotal'), + 'before' => array()), + + 'msrp' => array('_code' => 'grand_total', + 'after' => array(), + 'before' => array()), + // Totals defined in Mage_SalesRule + 'freeshipping' => array('_code' => 'freeshipping', + 'after' => array('subtotal'), + 'before' => array('tax_subtotal', 'shipping')), + + 'discount' => array('_code' => 'discount', + 'after' => array('subtotal', 'shipping'), + 'before' => array('grand_total')), + // Totals defined in Mage_Tax + 'tax_subtotal' => array('_code' => 'tax_subtotal', + 'after' => array('freeshipping'), + 'before' => array('tax', 'discount')), + + 'tax_shipping' => array('_code' => 'tax_shipping', + 'after' => array('shipping'), + 'before' => array('tax', 'discount')), + + 'tax' => array('_code' => 'tax', + 'after' => array('subtotal','shipping'), + 'before' => array('grand_total')), + // Totals defined in Mage_Wee + 'wee' => array('_code' => 'wee', + 'after' => array('subtotal','tax','discount','grand_total','shipping'), + 'before' => array()) + ); + return array( + array($coreTotals), // Test case with just core totals + array($coreTotals + array( // Test case with custom totals + 'handling' => array('_code' => 'handling', + 'after' => array('shipping'), + 'before' => array('tax')), + 'handling_tax' => array('_code' => 'handling_tax', + 'after' => array('tax_shipping'), + 'before' => array('tax')) + )), + array($coreTotals + array( // Test case with more custom totals + // (this one fails with non fixed core functionality) + 'handling' => array('_code' => 'handling', + 'after' => array('shipping'), + 'before' => array('tax')), + 'handling_tax' => array('_code' => 'handling_tax', + 'after' => array('tax_shipping'), + 'before' => array('tax')), + 'own_subtotal' => array('_code' => 'own_subtotal', + 'after' => array('nominal'), + 'before' => array('subtotal')), + 'own_total1' => array('_code' => 'own_total1', + 'after' => array('nominal'), + 'before' => array('subtotal')), + 'own_total2' => array('_code' => 'own_total2', + 'after' => array('nominal'), + 'before' => array('subtotal')) + )) + ); + } + + /** + * Restores cache usage options + * + */ + protected function tearDown() + { + if ($this->_restoreUseCache) { + Mage::app()->getCacheInstance()->allowUse('config'); + } + } +}