Merge lp:~vauxoo/openobject-addons/trunk-bug_887376-account_move_compute_with_btree-moylop260 into lp:openobject-addons

[Moisés López - http://www.vauxoo.com]

[Olivier Dony (Odoo) (odo-openerp)]

(Setting branch status to "Work in Progress" to indicate that it still needs to be updated after more people have a chance to review it, given the fact that it contains test code, TODOs, copy of old code, etc.)

[Moisés López - http://www.vauxoo.com (moylop260)]

Hola Odony, ok. I will work in depurate the code and parameters TODOs.
We will wait test result

> (Setting branch status to "Work in Progress" to indicate that it still needs
> to be updated after more people have a chance to review it, given the fact
> that it contains test code, TODOs, copy of old code, etc.)

[Cristian Salamea (ovnicraft)]

SQL more readable
params = (tuple(children_and_consolidated),) + query_params

Fix to compile the args with '%s %s ' % (params,..)

review: Needs Fixing

[Moisés López - http://www.vauxoo.com (moylop260)]

> SQL more readable
> params = (tuple(children_and_consolidated),) + query_params
>
> Fix to compile the args with '%s %s ' % (params,..)

Hola Cristian. I am agree with you.
But I dont change the current logic. only optimization of query.
the form of params is exactly the same sentence of original.

[Nhomar - Vauxoo (nhomar)]

review: Approve

[Nhomar - Vauxoo (nhomar)]

Hello Oliver.

I'm sorry, i didn't understand before your point about WORK IN PROGRESS!

The intention of this merge in this way IS NOT a proof of concept, we have using it for 6 month in production.

All TODO's are the original ones in the code we JUST optimize the core SQL code.

IOH, the reason because we left both methods is to help to testers to use BOTH methods compare time and over all RESULTS.

Don't you have a real db with real data to tests purpose on your enviroment?, this feature is so so important and we need mae test.

As you can see it is just ONE method for this reason, i don't face problems in for RC1 left both people make tryies and then we take all, do you have my point friend?

Regards!

PS: If you answer me that you can not make merge to RC 1 for this reason i can correct the merge, but i want to make live easyiest to testers.

Regards.

[Nhomar - Vauxoo (nhomar)]

Hello All.

Can somebody tell me what original YAML i can use to test this feature.

LEt me explain :

How we test this feature.

I have 1 year of real data:
We have in the Account CHart ALL internal type of accounts (consolidation was the more difficult to test and solve issues)
We KNOW results for financial reports.
We compare MAnually 5 or 6 key accounts (download account move lines and calculate manually on SpreadSheet)
We see results with OLD and with NEW code.

What shoul be the correct YAML to test it!

[Olivier Dony (Odoo) (odo-openerp)]

On 01/17/2012 03:49 PM, Nhomar Hernandez (Vauxoo) wrote:
> Can somebody tell me what original YAML i can use to test this feature.

There is probably not much to reuse in the existing YAML tests, as none
are specifically designed to verify the most complex cases of
consolidated charts.

Because this patch touches very core accounting features
(credit/debit/balance computation!), it is very sensitive, and proving
that it does not affect the results is the first step towards getting it
merged.

It also bypasses the aggregation logic used by the rest of the system
(get_children_and_..), so its correctness needs to be assessed even more.

> LEt me explain :
>
> How we test this feature.
>
> I have 1 year of real data:
> We have in the Account CHart ALL internal type of accounts (consolidation was the more difficult to test and solve issues)
> We KNOW results for financial reports.
> We compare MAnually 5 or 6 key accounts (download account move lines and calculate manually on SpreadSheet)
> We see results with OLD and with NEW code.
>
> What shoul be the correct YAML to test it!

Ideally OpenERP should already have such test scenarios, but
unfortunately this is not yet the case, so it makes it harder to review
and validate your patch.

A typical scenario for testing your patch would be to inject some sample
data in a separate demo chart of accounts, making sure to cover as much
complicated cases as possible (different consolidation options, multiple
levels of accounts, etc.) and compute the various credit, debit and
balance results for all of them, comparing them against the correct values.

This does not necessarily need to contain huge volumes of data, because
we're not concerned about performance (that is secondary and not the
goal of the scenario), but the correctness of the calculation.

You should probably create a separate chart of account for the purpose
of this test to make sure your results are not impacted by other changes
to demo data.
Actually, it would even be acceptable to create a new demo XML or CSV
file for the definition of that separate chart of accounts and the
related journal entries, which could then be used as the basis for your
tests. In that case your tests would be very simple: just assert the
result of the functions are correct based on the demo data. The tests
only need to pass after a fresh installation of a new DB with demo data.

This may sound like a lot of work but if you have existing test data you
can perhaps grab a small sample of it, anonymize it and use it as demo
data for the tests? You don't need a lot of data, just enough data to be
representative of the most complicated cases of aggregation (again, we
don't care about the volume).

The good thing about this is that these tests would also become core
accounting tests and used to better validate the core OpenERP Accounting
in the future, in addition to confirming the correctness of the new
algorithm.
Of course the tests should first pass without your patch, and then
continue passing after your patch is applied, confirming that it did not
affect the result, while still improving a lot the performance!

This is critical because if your algorithm works it...

Unmerged revisions

6356. By Moisés López - http://www.vauxoo.com on 2012-01-22

[MERGE] Merge from btree changes

=== modified file 'account/account.py'
--- account/account.py	2013-12-09 10:50:31 +0000
+++ account/account.py	2014-01-15 15:52:11 +0000
@@ -269,9 +269,10 @@
         return ids2 + ids3
 
     def __compute(self, cr, uid, ids, field_names, arg=None, context=None,
-                  query='', query_params=()):
+                      query='', query_params=()):
         """ compute the balance, debit and/or credit for the provided
         account ids
+
         Arguments:
         `ids`: account ids
         `field_names`: the fields to compute (a list of any of
@@ -282,6 +283,7 @@
                         (__compute will handle their escaping) as a
                         tuple
         """
+<<<<<<< TREE
         mapping = {
             'balance': "COALESCE(SUM(l.debit),0) - COALESCE(SUM(l.credit), 0) as balance",
             'debit': "COALESCE(SUM(l.debit), 0) as debit",
@@ -292,10 +294,22 @@
         #get all the necessary accounts
         children_and_consolidated = self._get_children_and_consol(cr, uid, ids, context=context)
         #compute for each account the balance/debit/credit from the move lines
+=======
+>>>>>>> MERGE-SOURCE
         accounts = {}
-        res = {}
-        null_result = dict((fn, 0.0) for fn in field_names)
+        aml_query = self.pool.get('account.move.line')._query_get(cr, uid, context=context)
+        
+        wheres = [""]
+        if query.strip():
+            wheres.append(query.strip())
+        if aml_query.strip():
+            wheres.append(aml_query.strip())
+        filters = " AND ".join(wheres)
+        self.logger.notifyChannel('addons.'+self._name, netsvc.LOG_DEBUG,
+                                  'Filters: %s'%filters)
+        children_and_consolidated = ids
         if children_and_consolidated:
+<<<<<<< TREE
             aml_query = self.pool.get('account.move.line')._query_get(cr, uid, context=context)
 
             wheres = [""]
@@ -317,8 +331,48 @@
                        " WHERE l.account_id IN %s " \
                             + filters +
                        " GROUP BY l.account_id")
+=======
+            request= '''SELECT *, debit-credit AS balance
+            FROM (
+                SELECT account_child_and_consolidated.parent_id AS id,
+                    SUM( COALESCE(l.debit,0) ) AS debit, SUM( COALESCE(l.credit,0) ) AS credit,
+                    COALESCE(SUM(l.amount_currency), 0) as foreign_balance
+                FROM (
+                    SELECT account_child_vw.parent_id, COALESCE( account_consolidated_vw.child_id, account_child_vw.child_id) AS child_id
+                    FROM (
+                        SELECT aa_tree_1.id AS parent_id, aa_tree_2.id AS child_id
+                        FROM account_account aa_tree_1
+                        LEFT OUTER JOIN account_account aa_tree_2
+                           ON aa_tree_2.parent_left 
+                              BETWEEN aa_tree_1.parent_left AND aa_tree_1.parent_right
+                    ) account_child_vw
+                    LEFT OUTER JOIN 
+                    (
+                        SELECT aa_tree_1.id AS parent_id, aa_tree_4.id AS child_id
+                        FROM account_account_consol_rel
+                        INNER JOIN account_account aa_tree_1
+                           ON aa_tree_1.id = account_account_consol_rel.child_id
+                        INNER JOIN account_account aa_tree_2
+                           ON aa_tree_2.id = account_account_consol_rel.parent_id
+                        LEFT OUTER JOIN account_account aa_tree_3
+                           ON aa_tree_3.parent_left 
+                              BETWEEN aa_tree_1.parent_left AND aa_tree_1.parent_right
+                        LEFT OUTER JOIN account_account aa_tree_4
+                           ON aa_tree_4.parent_left 
+                              BETWEEN aa_tree_2.parent_left AND aa_tree_2.parent_right
+                    ) account_consolidated_vw
+                    ON account_child_vw.child_id = account_consolidated_vw.parent_id
+                ) account_child_and_consolidated
+                LEFT OUTER JOIN account_move_line l
+                  ON l.account_id = account_child_and_consolidated.child_id
+                ''' \
+               'WHERE account_child_and_consolidated.parent_id IN %s ' \
+                + filters + \
+                'GROUP BY account_child_and_consolidated.parent_id ) subvw'
+>>>>>>> MERGE-SOURCE
             params = (tuple(children_and_consolidated),) + query_params
             cr.execute(request, params)
+<<<<<<< TREE
 
             for row in cr.dictfetchall():
                 accounts[row['id']] = row
@@ -361,6 +415,16 @@
         else:
             for id in ids:
                 res[id] = null_result
+=======
+            self.logger.notifyChannel('addons.'+self._name, netsvc.LOG_DEBUG,
+                                      'Status: %s'%cr.statusmessage)
+            for res1 in cr.dictfetchall():
+                accounts[res1['id']] = res1
+        res = {}
+        for id in ids:
+            for field_name in field_names:
+                res.setdefault(id, {})[field_name] = accounts.get(id, {}).get(field_name, 0.0)
+>>>>>>> MERGE-SOURCE
         return res
 
     def _get_company_currency(self, cr, uid, ids, field_name, arg, context=None):