Fix of wrong ordering logic for totals #49

Closed
wants to merge 2 commits into
from
Jump to file or symbol
Failed to load files and symbols.
+235 −23
Split
@@ -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
@@ -0,0 +1,202 @@
+<?php
+/**
+ * Magento
+ *
+ * NOTICE OF LICENSE
+ *
+ * This source file is subject to the Open Software License (OSL 3.0)
+ * that is bundled with this package in the file LICENSE.txt.
+ * It is also available through the world-wide-web at this URL:
+ * http://opensource.org/licenses/osl-3.0.php
+ * If you did not receive a copy of the license and are unable to
+ * obtain it through the world-wide-web, please send an email
+ * to license@magentocommerce.com so we can send you a copy immediately.
+ *
+ * DISCLAIMER
+ *
+ * Do not edit or add to this file if you wish to upgrade Magento to newer
+ * versions in the future. If you wish to customize Magento for your
+ * needs please refer to http://www.magentocommerce.com for more information.
+ *
+ * @category Magento
+ * @package Mage_Sales
+ * @subpackage integration_tests
+ * @copyright Copyright (c) 2012 Magento Inc. (http://www.magentocommerce.com)
+ * @license http://opensource.org/licenses/osl-3.0.php Open Software License (OSL 3.0)
+ */
+
+/**
+ * Integration test for testing order config class
+ *
+ * Not possible to make as a unit test, since internally app object is called
+ */
+class Mage_Sales_Model_Config_OrderedTest extends PHPUnit_Framework_TestCase
+{
+ /**
+ * Flag for checking if needed restoring of cache usage feature
+ *
+ * @var bool
+ */
+ protected $_restoreUseCache = false;
+
+ /**
+ * Model under test
+ *
+ * @var Mage_Sales_Model_Config_Ordered
+ */
+ protected $_model = null;
+
+ /**
+ * Disables configuration cache, sets up model
+ *
+ */
+ protected function setUp()
+ {
+ $this->_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');
+ }
+ }
+}