Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Implemented toplogical sorting for totals #65

Closed
wants to merge 1 commit into from

4 participants

@amenk

The sorting of totals is incorrect with causes lots of problem when new modules that define own totals are added.

For details see http://stackoverflow.com/a/9258826/288568

This patch implementes a toplogic sort to fix this problem.

For a unit test I suggest the following code ($result is the result of the _getSortedCollectorCodes array for any input)

        $this->assertEquals(count($configArray), count($result));

        // check if all conditions where matched
        // as we do have only a partial order, it does not make sense
        // to check the definite result
        foreach($configArray as $code=>$data) {
            $_code = $data['_code'];
            $codeIndex = array_search($_code, $result);
            if (!isset($configArray[$_code])) continue;
            foreach($data['before'] as $beforeCode) {
                if (!isset($configArray[$beforeCode])) continue;
                $bIndex = array_search($beforeCode, $result);
                $this->assertTrue($codeIndex < $bIndex, "Condition not fullfilled: $codeIndex < $bIndex");
            }
            foreach($data['after'] as $afterCode) {
                if (!isset($configArray[$afterCode])) continue;
                $aIndex = array_search($afterCode, $result);
                $this->assertTrue($codeIndex > $aIndex, "Condition not fullfilled: $codeIndex > $aIndex");
            }
        }
@IvanChepurnyi

Hi Amenk.
Check this pull request submitted by me: #49

It has already a fixed solution with integration test that confirmes that fix is working.

@amenk

Hi Ivan,

thanks for the comment - I wish I saw that pull request earlier...

Alex

@mage2-team
Collaborator

@amenk
Thank you for yet another contribution.

Recently (see pull request #49) we have implemented a library Magento_Data_Graph, which allows depth-first search (DFS). The topological search algorithm is known to be based on the DFS algorithm -- would you try redo your contribution to add the search algorithm to graph class and then utilize it for sorting sales totals?

@amenk

Okay. Can you give me a ping as soon as the Magento_Data_Graph is available in the git - I did not find it.

But I think the "drawback" of using the topological sort will be that if the ordering is not possible it would fail. See the discussion in #49 - so I do not know if it will be possible to still "sell" if there are problems.

@mage2-team
Collaborator

@amenk Magento_Data_Graph is available since Sep 5, 2012.

As you mentioned, there are drawbacks in a case of ambiguous input data. Feel free to close the ticket, if you think this ticket is no longer actual after the contribution #49 has been accepted.

@amenk

I will try to implement a check when sorting the data as an addition to #49 and log an exception when there are contradictions.

@mage2-team
Collaborator

Hello Amenk, any luck with implementing this one?

@amenk

Did not have a time yet.

I thought again about your suggestion in the other ticket to implement it in a CI test. Not sure about that.

I think if I integrate it with the Magento_Data_Graph you would use my functionality only to verify and log an exception but use Ivan's patch to do the actual sorting?

Is such a double-check and double-sorting intended ?

What do you think?

@mage2-team
Collaborator

In the particular case with checkout -- yes, we'd have to use this algorithm only to verify and log an exception.

But also we intend to utilize topological sorting in future for other places:

  • Across the board, where sorting elements is based on "before" and "after" attributes
  • In core layout and sales totals, admin menu, PEAR packages (Mage_Connect), module dependencies, admin system configuration (system.xml)
@mage2-team
Collaborator

Is such a double-check and double-sorting intended ?
Yes, it is intended as we described in Ivan's ticket: #49 (comment)

@amenk

Alright. What algorithms are used in those cases currently.

Please be aware, that the algorithm I suggest will fail if there are any contradictions. (which I do not think is a problem - but basically the discussion is the same like in #49) You mentioned also sales totals - isn't that the same use case?

@kirmorozov

Here's the code for sorting one way,
http://pubsvn.ez.no/doxygen/trunk/html/ezptopologicalsort_8php_source.html
And before is the same is opposite depends on for constructor.
Cant you just copy over GPL code?

@amenk

I actually used the code from http://www.calcatraz.com/blog/php-topological-sort-function-384 which is without license, means public domain.

@mage2-team
Collaborator

But also we intend to utilize topological sorting in future for other places:
... sales totals ...

You mentioned also sales totals - isn't that the same use case?

Right. It was listed there by mistake, please disregard that part.

@mage2-team
Collaborator

Hi. Would you be able to contribute topological sorting in Magento_Data_Graph? (and if you can add test/cases that would be great)

@mage2-team
Collaborator

Closing the thread due to no recent activity.

@mage2-team mage2-team closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 15, 2012
  1. Implemented toplogical sorting for totals

    Alexander Menk authored
This page is out of date. Refresh to see the latest.
Showing with 54 additions and 44 deletions.
  1. +54 −44 app/code/core/Mage/Sales/Model/Config/Ordered.php
View
98 app/code/core/Mage/Sales/Model/Config/Ordered.php
@@ -121,6 +121,43 @@ protected function _prepareConfigArray($code, $totalConfig)
return $totalConfig;
}
+
+ /**
+ * Topological sort
+ *
+ * @param $nodeids Node Ids - example: array('subtotal','grand_total');
+ * @param $edges Array of Edges. Each edge is specified as an array with two elements: The source and destination node of the edge
+ * Example: array(array('subtotal','grand_total')); -> subtotal comes before grand_total
+ * @return array|null
+ */
+ public function _topologicalSort($nodeids, $edges) {
+ $L = $S = $nodes = array();
+ foreach($nodeids as $id) {
+ $nodes[$id] = array('in'=>array(), 'out'=>array());
+ foreach($edges as $e) {
+ if ($id==$e[0]) { $nodes[$id]['out'][]=$e[1]; }
+ if ($id==$e[1]) { $nodes[$id]['in'][]=$e[0]; }
+ }
+ }
+ foreach ($nodes as $id=>$n) { if (empty($n['in'])) $S[]=$id; }
+ while ($id = array_shift($S)) {
+ if (!in_array($id, $L)) {
+ $L[] = $id;
+ foreach($nodes[$id]['out'] as $m) {
+ $nodes[$m]['in'] = array_diff($nodes[$m]['in'], array($id));
+ if (empty($nodes[$m]['in'])) { $S[] = $m; }
+ }
+ $nodes[$id]['out'] = array();
+ }
+ }
+ foreach($nodes as $n) {
+ if (!empty($n['in']) or !empty($n['out'])) {
+ return null; // not sortable as graph is cyclic
+ }
+ }
+ return $L;
+ }
+
/**
* Aggregate before/after information from all items and sort totals based on this data
*
@@ -140,36 +177,30 @@ 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) {
+ // prepare data for topological sort
+ $nodes = array_keys($configArray);
+ $edges = array();
+
+ foreach ($configArray as $data) {
+ $_code = $data['_code'];
+ if (!isset($configArray[$_code])) continue;
foreach ($data['before'] as $beforeCode) {
- if (!isset($configArray[$beforeCode])) {
- 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']);
+ if (!isset($configArray[$beforeCode])) continue;
+ $edges[] = array($_code, $beforeCode);
}
foreach ($data['after'] as $afterCode) {
- if (!isset($configArray[$afterCode])) {
- continue;
- }
- $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']);
+ if (!isset($configArray[$afterCode])) continue;
+ $edges[] = array($afterCode, $_code);
}
}
- uasort($configArray, array($this, '_compareTotals'));
+ $sortedCollectors = $this->_topologicalSort($nodes, $edges);
+
+ if (is_null($sortedCollectors)) {
+ throw new Mage_Sales_Exception('Total ordering before/after conditions can not be complied with');
+ }
}
- $sortedCollectors = array_keys($configArray);
if (Mage::app()->useCache('config')) {
Mage::app()->saveCache(serialize($sortedCollectors), $this->_collectorsCacheKey, array(
Mage_Core_Model_Config::CACHE_TAG
@@ -196,27 +227,6 @@ protected function _initCollectors()
}
/**
- * Callback that uses after/before for comparison
- *
- * @param array $a
- * @param array $b
- * @return int
- */
- protected function _compareTotals($a, $b)
- {
- $aCode = $a['_code'];
- $bCode = $b['_code'];
- if (in_array($aCode, $b['after']) || in_array($bCode, $a['before'])) {
- $res = -1;
- } elseif (in_array($bCode, $a['after']) || in_array($aCode, $b['before'])) {
- $res = 1;
- } else {
- $res = 0;
- }
- return $res;
- }
-
- /**
* Callback that uses sort_order for comparison
*
* @param array $a
Something went wrong with that request. Please try again.