Fix of wrong ordering logic for totals #49

Closed
wants to merge 2 commits into
from

3 participants

@IvanChepurnyi

During custom development, there were discovered a bug with ordering process.

If you create custom total like this one:

<handling>
      <class>Full_Total_Class_Name</class>
      <after>shipping</after>
      <before>tax</before>
</handling>
<handling_tax>
     <class>Full_Total_Class_Name</class>
     <after>tax_shipping</after>
     <before>tax</before>
</handling_tax>

It will not be placed between tax and tax_shipping totals, the actual result was like the following:

Array
(
    [0] => nominal
    [1] => subtotal
    [2] => tax_subtotal
    [6] => freeshipping
    [7] => handling_tax
    [8] => handling
    [9] => weee
    [10] => shipping
    [11] => tax_shipping
    [12] => discount
    [14] => tax
    [17] => grand_total
    [19] => msrp
)

The problem itself in sorting method that were used. Since only close items were compared to each other and if you have total that is not next to desired one, its before and after attributes get ignored.

Apparently this commit not only fixes an issue with sorting, it also works 3 times faster then current method in core. The test was done on 1000 iteration five times and average results are the following:

  • Current core functionality 0.83539700508118 seconds
  • Patched functionality 0.26063299179077 seconds
@IvanChepurnyi IvanChepurnyi Fixed core issue related to invalid totals sorting in the quote. The …
…problem is visible if you have additional custom totals that need to be placed in between core ones.
9a3631e
@magento-team

@IvanChepurnyi
Thank you for yet another valuable contribution.
In order to accept it, all code changes should be supplied with the corresponding unit/integration tests.
Also, to simplify further code review, the conventional bug report would be greatly appreciated:
1. "Steps to reproduce"
2. "Expected result"
3. "Actual result".

@IvanChepurnyi

Added integration test. It fails on 3rd data-set with non-fixed core code and on passes if my fix is applied.

The expected result is correctly sorted totals by before and after properties. The actual results and steps to reproduce already specified in original bug report.

@magento-team

@IvanChepurnyi
Thank you for adding lacking tests.

The contribution has been accepted with the following deviations:

  • minimized usage of Reflections for testing
  • implemented additional tests for ambiguous situations during sorting
    • replaced testing logic to compare against expected results in order to be able to validate ambiguous cases

Again, thank you for all the effort and valuable contribution.

Changes will become available with one of the nearest merges from the internal repository. Closing the ticket.

@amenk

@IvanChepurnyi Is this a toplogical sort algorithm? I am wondering what happens if the specified orders are contradictory (total foo after bar, total bar after foo). Shouldn't that be detected and an exception thrown?

@IvanChepurnyi

It is not a good idea to throw an exception, where it was not thrown before, as far as I know the fixes from Magento 2.0 go to current version as well, and if at some Magento version conflict were resolved by some of the properties but later on an exception were thrown, then existent shops who updates via Magento connect are in danger of crashing down...

@amenk

But if there a contradictions the sort order might be completely undefined ... so I think an exception is much better than an undefinied state that could cause to further problems such as wrongly calculated totals (you might sell your stuff without tax then an so on)

@IvanChepurnyi

For this case after has always higher priority. Since grand_total gets after tax, tax goes after shipping, discount and so on, you will never get a situation of wrongly calculated taxes. It only affects custom totals that manually setting up positions for themselves.

@amenk

How is the result defined if A after B , B after A was defined?

@IvanChepurnyi

If A is processed after B then A will be placed after B, if B is processed after A, then B will be after A.

@amenk

So one of the conditions is not met. For those cases also the testcase would fail (if you provide it with such "invalid input").

I still have the opinion that this can cause business critical problems that are very hard to track down and should be detected well. Everything else makes the system unreliable.

If someone updates Magento and has incompatible modules that cause such a problem, an exception would be noticed very soon (I hope nobody does an Magento update on a live system without testing).
But if the error is such a hidden one, it might go unnoticed and cause several business damage.

@IvanChepurnyi

Sorry, Alexander, but I don't see any reason to argue with you about the exception, the patch was already submitted to core team, covered with integration tests and accepted by them into the next release. If you do not agree with my contribution, you are always free to create another pull request with your exception throw statement.

For my opinion it should not be thrown, to not create fatal error on the frontend, maybe it better just to log debug message somewhere. At least, in another core functionality with before/after sorting, there is no exception thrown if two blocks have cycling reference in before and after attributes (layouts).

As for store owners who update live version via Magento connect, it is true, and I've helped such people enormous amount of time during all the 5 years of my Magento development experience. Of course it is not recommended to update Magento version without testing on staging environment, but a lot of people just do so.

@amenk

No offense. My intention is also not to argue. Its just about constructively improving your pull request.

This total sorting problem caused a lot of people lots of headaches and I am thankful that you fixed it.

But if it does not reliably fulfil the conditions giving in the XML configuration and silently ignores some of them I am just afraid that a similar issue in Magento2 will waste a lot of peoples time who have to debug this.

I also understand your point, that fatal errors for endusers are not nice. I think there is room for discussion what is better: A order that does not work, or an order with possibly wronlgy caluclates totals (if a total B that was specified to run after A is not run after A, the required total fields in the quote might not be available and the total might return 0).
Both is not good - so I think just logging an exception to exception.log might be a good compromise.

Actually I did not yet have the time to reconstruct the algorithm used in your pull request to identify the place where we could log an exception. That is one reason I did not enhance your pull request myself.

(My pull request #65 does not provide the feature to generate a "nearly correct" order - it just fails completly so I could only throw an Exception with a fatal error to the end user).

Alexander

@magento-team magento-team reopened this Aug 28, 2012
@magento-team

We discussed the question of "to throw or not to throw" with product management and agreed on the following:

  • During checkout or anywhere on the frontend, the contradictory or ambiguous totals must not trigger errors and must not prevent customer from buying. The justification for this argument is that it is better to sell with slightly wrong price, rather than not to sell at all.
  • When totals are calculated, add validation of their declaration. The validation errors must be logged to system/error log
  • Implement an integrity test that would invoke validation of declared totals by the currently deployed Magento instance (it might include 3rd-party extensions with own totals). If ambiguous or contradictory declarations are declared -- fail the test. In such a way the developer or system administrator, when introducing an extension with new totals, will know if they break the system or not.

All the mentioned above has been implemented. Representation of totals is implemented as a graph (new library Magento_Data_Graph, but used only in validation for now). In this graph, we search for loops in various ways. If a loop is found, it means that it is logically impossible to transform it to a simple graph (or sort graph nodes).

The topological sorting algorithm should perfectly fit into the new model Magento_Data_Graph. We'll recommend it to author in the pull request #65. Changes will be rolled out later with next code updates.

@amenk

Alright - thanks for elaborating this.

"The justification for this argument is that it is better to sell with slightly wrong price, rather than not to sell at all." - makes sense to me. But I think we can not be sure, that such problems cause only slightly wrong prices... - depending on the totals to be affected.

Integrity Testing and logging to system/error.log sounds good to me and might be a good trade off. So we could make it up to the users if they conduct such tests and check the error log.

@magento-team magento-team added a commit that referenced this pull request Sep 6, 2012
@magento-team magento-team Update as of 9/05/2012
* Implemented encryption of the credit card name and expiration date for the payment method "Credit Card (saved)"
* Implemented console utility `dev/tools/migration/get_aliases_map.php`, which generates map file "M1 class alias" to "M2 class name"
* Implemented automatic data upgrades for replacing "M1 class aliases" to "M2 class names" in a database
* Implemented recursive `chmod` in the library class `Varien_Io_File`
* Improved verbosity of the library class `Magento_Shell`
* Migrated client-side translation mechanism to jQuery
* Performance tests:
  * Improved assertion for number of created orders for the checkout performance testing scenario
    * Reverted the feature of specifying PHP scenarios to be executed before and after a JMeter scenario
    * Implemented validation for the number of created orders as a part of the JMeter scenario
    * Implemented the "Admin Login" user activity as a separate file to be reused in the performance testing scenarios
  * Implemented fixture of 100k customers for the performance tests
  * Implemented fixture of 100k products for the performance tests
    * Enhanced module `Mage_ImportExport` in order to utilize it for the fixture implementation
  * Implemented back-end performance testing scenario, which covers Dashboard, Manage Products, Manage Customers pages
* Fixes:
  * Fixed Magento console installer to enable write permission recursively to the `var` directory
  * Fixed performance tests to enable write permission recursively to the `var` directory
  * Fixed integration test `Mage_Adminhtml_Model_System_Config_Source_Admin_PageTest::testToOptionArray` to not produce "Warning: DOMDocument::loadHTML(): htmlParseEntityRef: expecting ';' in Entity" in the developer mode
* GitHub requests:
  * [#43](#43) -- implemented logging of executed setup files
  * [#44](#44)
    * Implemented support of writing logs into wrappers (for example, `php://output`)
    * Enforced a log writer model to be an instance of `Zend_Log_Writer_Stream`
  * [#49](#49)
    * Fixed sorting of totals according to "before" and "after" properties
    * Introduced `Magento_Data_Graph` library class and utilized it for finding cycles in "before" and "after" declarations
    * Implemented tests for totals sorting including the ambiguous cases
c0cf1af
@mmansoorebay mmansoorebay pushed a commit that referenced this pull request Jan 30, 2014
@sshymko sshymko Implemented MAGETWO-2910: GitHub Tickets Processing 41
- accepted contribution #49
-- fixed sorting of totals according to "before" and "after" characteristics
-- removed method _compareTotals() since it's no longer used in totals sorting algorithm
-- integration test compares sorting results against hard-coded expected values instead of using validation algorithm, proposed in the original contribution
--- implemented additional validations for ambiguous situations
87c43ac
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment