account - level - wrong computation

Note:

1) I don't think the code makes any assumption that the parent's level is already calculated, because it recursively calls the function field, which should be computed on-demand, as far as I can see. However the process of doing this is a bit inefficient. I think the computed values should be correct, provided the tree hierarchy is correct and did not change, because of 2).
It is not easy to implement this in an efficient way (in terms of DB queries). A possible approach might be to use cr.execute('SELECT id, parent_id WHERE parent_left < %s', account.parent_left), which will return pairs of (id, parent_id) for all possible ancestors of the current account, and then post-process these in python by putting the pairs in a map and walking up from the node's parent.

2) That's quite correct indeed, the values should be recomputed when the hierarchy changes, e.g. via a different "store=..." trigger.

ad 1) for my chart of account the levels are not correct

I dropped the level field to let it recalculate, but it's still wrong.

may be it's due to my poor understanding of python and what (not) happens in detail, but this code seems not to be called recursively, I am missing a while.... condition

IMHO it it calculated once for every account

    def _get_level(self, cr, uid, ids, field_name, arg, context=None):
        res={}
        accounts = self.browse(cr, uid, ids, context=context)
        for account in accounts:
            print >> sys.stderr,'account',account
            level = 0
            if account.parent_id:
                obj = self.browse(cr, uid, account.parent_id.id)
                print >> sys.stderr,'account obj',obj
                level = obj.level + 1
            res[account.id] = level
        return res

account obj browse_record(account.account, 64)
account browse_record(account.account, 78)
account obj browse_record(account.account, 77)
account browse_record(account.account, 79)
account obj browse_record(account.account, 77)
account browse_record(account.account, 210)
account obj browse_record(account.account, 45)
account browse_record(account.account, 211)

On 17. 05. 11 16:10, Ferdinand @ Camptocamp wrote:
> ad 1) for my chart of account the levels are not correct
>
> I dropped the level field to let it recalculate, but it's still wrong.
>
> may be it's due to my poor understanding of python and what (not)
> happens in detail, but this code seems not to be called recursively, I
> am missing a while.... condition
>
> IMHO it it calculated once for every account
>
> def _get_level(self, cr, uid, ids, field_name, arg, context=None):
> res={}
> accounts = self.browse(cr, uid, ids, context=context)
> for account in accounts:
> print >> sys.stderr,'account',account
> level = 0
> if account.parent_id:
> obj = self.browse(cr, uid, account.parent_id.id)
> print >> sys.stderr,'account obj',obj
> level = obj.level + 1
> res[account.id] = level
> return res
>
> account obj browse_record(account.account, 64)
> account browse_record(account.account, 78)
> account obj browse_record(account.account, 77)
> account browse_record(account.account, 79)
> account obj browse_record(account.account, 77)
> account browse_record(account.account, 210)
> account obj browse_record(account.account, 45)
> account browse_record(account.account, 211)
>
I hope that this won't sound too silly, but what's the use of this level
alltogether?? I can't see one.

thank you!
Bogdan

it is (heavily) used in important reports (balance , P&L)

On 05/17/2011 04:10 PM, Ferdinand @ Camptocamp wrote:
> ad 1) for my chart of account the levels are not correct
>
> I dropped the level field to let it recalculate, but it's still wrong.

Alright, after looking at the ORM code, it's because of another issue:
this is an integer field and OpenERP treats NULLs as zero for integer
fields, so it assumes the field value is already computed for the parent
indeed.
In addition, OpenERP function fields are meant to be computed in batch
and not lazily through recursion, so this approach does not work.

> may be it's due to my poor understanding of python and what (not)
> happens in detail, but this code seems not to be called recursively, I
> am missing a while.... condition

A recursive function is simply a function that calls itself, directly or
indirectly [1]. This is the case here because "obj.level" is a call that
will perform read() on the "level" column of the given browse_record,
which in turn is supposed to access the _get_level() function that
computes the value. And so on if the parent has a parent..
However, as you've noticed, it fails to properly compute the level
because the call to obtain the parent's level is not properly handled
(for the reasons explained above).

So you're right, we need to fix the computation, and I think the
suggested approach from comment #1, with a query based on "parent_left"
should work efficiently enough.

Thank you for the follow-up!

[1] http://en.wikipedia.org/wiki/Recursion_(computer_science)

@Olivier
thank you for clarifing the issue of implicit indirect calling.

this patch calculates the correct level

nevertheless the trigger has still to be fixed

Hello Ferdinand,

I have applied your patch and considering the suggestions given by Olivier Dony its been fixed at the following revision:-

Its fixed in https://code.launchpad.net/~openerp-dev/openobject-addons/trunk-bug-783670-mtr branch.
Revision ID: mtr@mtr-20110616093021-n7ihs56l7v08ntwq
Revision no: 4793

Thanks for the contribution.

Thanks,
mtr

it seems not to be merged into 6.0.3 ?

On 08/26/2011 12:36 PM, Ferdinand @ Camptocamp wrote:
> it seems not to be merged into 6.0.3 ?

Hello,

This bug is still only "Fix Committed", the fix is not merged yet - it
still needs to be approved by a reviewer/team leader. Feel free to
comment on it to approve or give feedback about the implemented solution :-)

And btw, following the bug management policy, bugs reported on LP are
fixed on trunk only by default, unless the importance is High/Critical.
See the bug management policy for more details, rationale and FAQ at
http://doc.openerp.com/v6.0/contribute/11_bug_tracker.html#bug-management-policy

@Olivier
as I mentioned in comment#4 these reports
./report/account_balance_landscape.py
./report/account_balance.py
./report/account_balance_sheet.py
./report/account_profit_loss.py
./report/account_tax_report.py

use "level" and if the level is wrong they will produce at least unwanted / unpredictable /wrong results

I realy would be happy to see OpenERP taking accounting issues more seriously as a service for partners, who will be exposed to customer complaints especially if financial authorities might doubt that OpenERP accounting provides reliable data.

On 08/28/2011 10:58 AM, Ferdinand @ Camptocamp wrote:
> I realy would be happy to see OpenERP taking accounting issues more
> seriously as a service for partners, who will be exposed to customer
> complaints especially if financial authorities might doubt that OpenERP
> accounting provides reliable data.

We are taking accounting issues very seriously, like all others.

In the bug management policy I linked to, you will see that partners or
customers who need a fast and guaranteed response time should report
bugs via the OpenERP Enterprise/OPW *service* channel, not Launchpad.
This is the guaranteed service channel for partners and customers, and
it's also the right way to request fixing a stable branch.
The policy has not changed since November 2010.

To be very clear: there is *no guarantee* on response time nor backport
in stable branches for bugs that are reported on Launchpad (for all the
reasons explained in the bug management policy - please read it).

Thanks for your understanding

PS: nothing prevents reporting bugs on Launchpad as well, but you should
not expect any kind of guaranteed service for these unless you report
them using the proper channel (you could simply mention the LP bug number)

fixed by revision 5464

thanks

Hello Olivier,

let me just suggest to encourage your OPW customers to also report bugs on
Launchpad (aside from reporting via OPW channel): it encourages synergies
(it's quite likely that an OPW bug is actually patched by the community) and
it will show the community that purchasing an OPW contract tend to make a
difference, otherwise it's pretty hard to show the added value to our
customers.

--
Raphaël Valyi
Founder and consultant
http://twitter.com/rvalyi <http://twitter.com/#!/rvalyi>
+55 21 2516 2954
www.akretion.com

On Mon, Aug 29, 2011 at 6:20 AM, Olivier Dony (OpenERP) <
<email address hidden>> wrote:

> On 08/28/2011 10:58 AM, Ferdinand @ Camptocamp wrote:
> > I realy would be happy to see OpenERP taking accounting issues more
> > seriously as a service for partners, who will be exposed to customer
> > complaints especially if financial authorities might doubt that OpenERP
> > accounting provides reliable data.
>
> We are taking accounting issues very seriously, like all others.
>
> In the bug management policy I linked to, you will see that partners or
> customers who need a fast and guaranteed response time should report
> bugs via the OpenERP Enterprise/OPW *service* channel, not Launchpad.
> This is the guaranteed service channel for partners and customers, and
> it's also the right way to request fixing a stable branch.
> The policy has not changed since November 2010.
>
> To be very clear: there is *no guarantee* on response time nor backport
> in stable branches for bugs that are reported on Launchpad (for all the
> reasons explained in the bug management policy - please read it).
>
> Thanks for your understanding
>
> PS: nothing prevents reporting bugs on Launchpad as well, but you should
> not expect any kind of guaranteed service for these unless you report
> them using the proper channel (you could simply mention the LP bug number)
>
> --
> You received this bug notification because you are a member of OpenERP
> Drivers, which is subscribed to OpenERP Addons.
> https://bugs.launchpad.net/bugs/783670
>
> Title:
> account - level - wrong computation
>
> Status in OpenERP Modules (addons):
> Fix Committed
>
> Bug description:
> account/account.py
>
> def _get_level
>
> currently the calculated level (except level 1) is very much random,
> depending on the sequence of processed records
>
> 2 issues:
> 1) IMHO the function makes the wrong assumption that the level of the
> parent account is already calculated, which is not necessarily the case
> 2 ) the level (stored=True) of all childs must be recalculated if the
> level of a parent changes. IMHO this would also solve issue 1).
> change of structure may be done any time manually.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/openobject-addons/+bug/783670/+subscriptions
>