Merge lp:~akretion-team/openobject-addons/5.2-refactor-methods-splits into lp:openobject-addons

[Raphaël Valyi - http://www.akretion.com]

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

This time the right branch targeting trunk addons.
See original comments here for details: https://code.launchpad.net/~openerp-commiter/openobject-addons/account-method-extraction/+merge/15727

Notices that I carefully checked that my merge is still compatible with addons by checking that the bits we extract in methods were still present unmodified in trunk where I branched.

BTW, after months of pain, this is the first decent branch submission because Launchpad is finally using stacked branches now that Tiny updated OpenERP branch formats, so uploading this branch took like 1 min vs 3hours previously...

[Xavier (Open ERP) (xmo-deactivatedaccount)]

A few questions/observations:

* In `compute_invoice_totals`, you take iml/invoice_move_lines as an input and then return it unmodified, is there a reason? Is this a hook for later alterations? Does that mean `compute_invoice_totals` would be able to modify the invoice_move_lines? (reads a bit weird from my perspective, but I'm not quite up to stuff on the functional stuff, so there might be an underlying reason to do that)

* I see that you added nice docstrings for the extracted `group_lines` and `inv_line_characteristic_hashcode` (thank you), is there any possibility to add some to `check_tax_lines` and `compute_invoice_totals`, at least to document the arguments a bit? (the method names read clear enough, though they might be lying, but the arguments aren't as clear). Not the cr & uid stuff as they're "standard", but the rest?

* On `group_lines`'s docstring, why groups "eventually"? From what I read, it seems to merge lines by hashcode (by the way, "merge" might be a better name than "group" given the semantic differences with `itertools.groupby`). I'd also rename `tmp` to `key`.

* Finally, `inv_line_characteristic_hashcode` might be a good case for replacing the current +-based concatenation with a simple string format, it would be more readable and would probably be just as fast if not faster.

review: Needs Information (technical)

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

> A few questions/observations:
>
> * In `compute_invoice_totals`, you take iml/invoice_move_lines as an input and
> then return it unmodified, is there a reason? Is this a hook for later
> alterations?

-> Well, as you can see, in compute_invoice_totals, we iterate over invoice_move_lines. Not me that wanted that, it's the original code behavior. Now, may be you would like to prevent alteration of those params, makes sense, but do you have a way in Python to declare a method variable is constant? I'm not aware of that, but I'm admittedly a Python dilettante...

>
> * I see that you added nice docstrings for the extracted `group_lines` and
> `inv_line_characteristic_hashcode` (thank you), is there any possibility to
> add some to `check_tax_lines` and `compute_invoice_totals`, at least to
> document the arguments a bit? (the method names read clear enough, though they
> might be lying, but the arguments aren't as clear). Not the cr & uid stuff as
> they're "standard", but the rest?

-> I don't claim to understand or be willing to understand all legacy Tiny code here... I just pretend to here to apply basic software engineering principles where methods shouldn't be too fat to be easily over-ridable in a fine grained manner (just like overriders should call super and a few basic OOP rules like that). Yes, for those methods in particular, method names are somewhat clear and that's probably better that the nodoc situation of before. I don't claim I master enough what is going on under the cover to make much smarter docstrings here, you better ask that to Fabien or something I'm afraid...

> * On `group_lines`'s docstring, why groups "eventually"? From what I read, it
> seems to merge lines by hashcode (by the way, "merge" might be a better name
> than "group" given the semantic differences with `itertools.groupby`). I'd
> also rename `tmp` to `key`.

-> Okay, let's go for that one...

>
> * Finally, `inv_line_characteristic_hashcode` might be a good case for
> replacing the current +-based concatenation with a simple string format, it
> would be more readable and would probably be just as fast if not faster.

-> be my guest to improve things further. My philosophy here has been to improve the existing code using basic/automatic code extraction methods without changing the logic in order to avoid confusing Tiny and taking the risk you refuse that clear improvement over that existing crappy piece of code.

Admit that in the past, the most complex a merge proposal was, the more unlikely it was to get merged, no matter how good it was, just because Tiny wouldn't give a shit about third party work. Now, we are okay to adapt to a more open Tiny, but please admit it takes some time because we wait for clear effective signals from Tiny before we are willing to spend and risk to waste so much time again (and ERPScenario and OpenETL wastes aren't that far away, remember?)...

So my take is: merge that first, it's iso-functional and even backward compatible, then use your editor time to fight against Fabien and other Tiny's gods to try to improve the code logic, cause I can't afford spending more time myself to get such basic improvements merged in (con...

[Xavier (Open ERP) (xmo-deactivatedaccount)]

> Well, as you can see, in compute_invoice_totals, we iterate over invoice_move_lines.

Yeah, no problem with that, the part I find weird is the last line of the method:

    return total, total_currency, invoice_move_lines

here, the method *returns* invoice_move_lines, and I don't see the reason for that as the caller already has all the invoice_move_lines it wants (unless `compute_invoice_totals` modifies invoice lines, which from its name it has no reason whatsoever to).

> I don't claim to understand or be willing to understand all legacy Tiny code here... I just pretend to here to apply basic software engineering principles where methods shouldn't be too fat to be easily over-ridable in a fine grained manner

OK.

[Xavier (Open ERP) (xmo-deactivatedaccount)]

as for the next part:

> be my guest to improve things further. My philosophy here has been to improve the existing code using basic/automatic code extraction methods without changing the logic in order to avoid confusing Tiny and taking the risk you refuse that clear improvement over that existing crappy piece of code.

You could just have said that you believe this suggestion to be out of scope for your branch, it'd have been shorter and to the point.

> I hope it doesn't sound rude

Well I'm getting used to your rants so I don't really mind; I'm not sure everybody has as thick a skin as mine though so while I understand your frustration I believe sugar-coating your words a bit (and cutting down on your word count) would get your points across better. Generally speaking and considering the form of your replies there's a high risk people will respond to your tone, not your content, and I'm not sure that's for the best.

As for the merge semantics stuff, I'm not going to claim good enough knowledge of the functional side to comment.

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

> here, the method *returns* invoice_move_lines, and I don't see the reason for
> that as the caller already has all the invoice_move_lines it wants (unless
> `compute_invoice_totals` modifies invoice lines, which from its name it has no
> reason whatsoever to).

Well, unless I'm dumb tonight, it's just what it does: the compute_invoice_totals modifies all invoice lines. When we iterate over them, put them in the i variable (again not my fault for that local name; but again I try to make the minimal change here at least to make sure it gets in and let you make the rest if you can), and then change values of that hash, so we should return that.

Now if you see better method name or code organization please go on. Again that wouldn't be the first place in the OpenERP codebase where could easily find better names (we could speak about the foreign key name conventions and their numerous exceptions for instance)...

[...]
> Generally speaking and considering the form of your replies there's a high risk people will respond > to your tone, not your content, and I'm not sure that's for the best.

The issue is: we have been very very patient for years with Tiny with a very keen tone and the result was a huge incapacity to integrate any third party work no matter the content, such as https://code.launchpad.net/~openerp-commiter/openobject-addons/account-method-extraction/+merge/15727

So the community is tired of words, it now judges by acts only and yet for some months I believe (and see partner exchanges or community meeting or experts lists if you think that was just me, some even left recently as I said).

At the same time we invest pretty heavily on our best ERP that is still OpenERP as you see. And while OpenERP as clearly been in the lead for the last 3 years, we are a bit afraid to see that Tiny is somewhat likely to let Tryton pass within 2 years because they would make the error to dream they are ready to scale with such a poor codebase quality (even if competitors were worse it doesn't mean it's enough to scale, far from it...). That doesn't excite us at the point we would lick Tiny's boots. At this stage, the more friendly stance we can have is rather try to trigger an electroshock in Tiny's megalomania (the +500 module, Saas ready, RAD slideware)...

So don't take it personally cause we appreciate a lot the great work you are throwing in, it has been a long time... But please don't expect either people can work like mad, propose a ton of improvements, get their work largely ignored and keep smiling full of faith while competitors make their way faster... At least we continue to work in Tiny's direction...

So do you still have question or see issues with that merge? I mean yeah, it's probably far from perfect but again I try to be pragmatic: my goal is not to re-make the world here but just modularize existing stuff decently. Then I can do that elsewhere and that would provide at least much better options to extension modules for 2010 than just copy/paste those 170 lines of code... I think it has more value I do similar simple changes elsewhere where it's very much required rather than invest it all upon that one and let the rest ( https:/...

review: Needs Resubmitting

=== modified file 'account/invoice.py'
--- account/invoice.py	2010-02-16 14:27:23 +0000
+++ account/invoice.py	2010-02-23 05:42:20 +0000
@@ -630,8 +630,85 @@
             if res and res['value']:
                 self.write(cr, uid, [inv.id], res['value'])
         return True
+    
+    def check_tax_lines(self, cr, uid, inv, compute_taxes, ait_obj):
+        if not inv.tax_line:
+            for tax in compute_taxes.values():
+                ait_obj.create(cr, uid, tax)
+        else:
+            tax_key = []
+            for tax in inv.tax_line:
+                if tax.manual:
+                    continue
+                key = (tax.tax_code_id.id, tax.base_code_id.id, tax.account_id.id)
+                tax_key.append(key)
+                if not key in compute_taxes:
+                    raise osv.except_osv(_('Warning !'), _('Global taxes defined, but are not in invoice lines !'))
+                base = compute_taxes[key]['base']
+                if abs(base - tax.base) > inv.company_id.currency_id.rounding:
+                    raise osv.except_osv(_('Warning !'), _('Tax base different !\nClick on compute to update tax base'))
+            for key in compute_taxes:
+                if not key in tax_key:
+                    raise osv.except_osv(_('Warning !'), _('Taxes missing !'))
+
+    def compute_invoice_totals(self, cr, uid, inv, company_currency, ref, invoice_move_lines):
+        total = 0
+        total_currency = 0
+        for i in invoice_move_lines:
+            if inv.currency_id.id != company_currency:
+                i['currency_id'] = inv.currency_id.id
+                i['amount_currency'] = i['price']
+                i['price'] = cur_obj.compute(cr, uid, inv.currency_id.id,
+                        company_currency, i['price'],
+                        context={'date': inv.date_invoice or time.strftime('%Y-%m-%d')})
+            else:
+                i['amount_currency'] = False
+                i['currency_id'] = False
+            i['ref'] = ref
+            if inv.type in ('out_invoice','in_refund'):
+                total += i['price']
+                total_currency += i['amount_currency'] or i['price']
+                i['price'] = - i['price']
+            else:
+                total -= i['price']
+                total_currency -= i['amount_currency'] or i['price']
+        return total, total_currency, invoice_move_lines
+    
+    def inv_line_characteristic_hashcode(self, invoice, invoice_line):
+        """Overridable hashcode generation for invoice lines. Lines having the same hashcode
+        will be grouped together if the journal has the 'group line' option. Of course a module
+        can add fields to invoice lines that would need to be tested too before merging lines
+        or not."""
+        code = str(invoice_line['account_id'])
+        code += '-'+str(invoice_line.get('tax_code_id',"False"))
+        code += '-'+str(invoice_line.get('product_id',"False"))
+        code += '-'+str(invoice_line.get('analytic_account_id',"False"))
+        code += '-'+str(invoice_line.get('date_maturity',"False"))
+        return code
+    
+    def group_lines(self, cr, uid, iml, line, inv):
+        """Merge account move lines (and hence analytic lines) if invoice line hashcodes are equals"""
+        if inv.journal_id.group_invoice_lines:
+            line2 = {}
+            for x, y, l in line:
+                tmp = self.inv_line_characteristic_hashcode(inv, l)
+                
+                if tmp in line2:
+                    am = line2[tmp]['debit'] - line2[tmp]['credit'] + (l['debit'] - l['credit'])
+                    line2[tmp]['debit'] = (am > 0) and am or 0.0
+                    line2[tmp]['credit'] = (am < 0) and -am or 0.0
+                    line2[tmp]['tax_amount'] += l['tax_amount']
+                    line2[tmp]['analytic_lines'] += l['analytic_lines']
+                else:
+                    line2[tmp] = l
+            line = []
+            for key, val in line2.items():
+                line.append((0,0,val))
+        
+        return line
 
     def action_move_create(self, cr, uid, ids, *args):
+        """Creates invoice related analytics and financial move lines"""
         ait_obj = self.pool.get('account.invoice.tax')
         cur_obj = self.pool.get('res.currency')
         context = {}
@@ -650,24 +727,7 @@
 
             context.update({'lang': inv.partner_id.lang})
             compute_taxes = ait_obj.compute(cr, uid, inv.id, context=context)
-            if not inv.tax_line:
-                for tax in compute_taxes.values():
-                    ait_obj.create(cr, uid, tax)
-            else:
-                tax_key = []
-                for tax in inv.tax_line:
-                    if tax.manual:
-                        continue
-                    key = (tax.tax_code_id.id, tax.base_code_id.id, tax.account_id.id)
-                    tax_key.append(key)
-                    if not key in compute_taxes:
-                        raise osv.except_osv(_('Warning !'), _('Global taxes defined, but are not in invoice lines !'))
-                    base = compute_taxes[key]['base']
-                    if abs(base - tax.base) > inv.company_id.currency_id.rounding:
-                        raise osv.except_osv(_('Warning !'), _('Tax base different !\nClick on compute to update tax base'))
-                for key in compute_taxes:
-                    if not key in tax_key:
-                        raise osv.except_osv(_('Warning !'), _('Taxes missing !'))
+            self.check_tax_lines(cr, uid, inv, compute_taxes, ait_obj)
 
             if inv.type in ('in_invoice', 'in_refund') and abs(inv.check_total - inv.amount_total) >= (inv.currency_id.rounding/2.0):
                 raise osv.except_osv(_('Bad total !'), _('Please verify the price of the invoice !\nThe real total does not match the computed total.'))
@@ -684,24 +744,7 @@
             # create one move line for the total and possibly adjust the other lines amount
             total = 0
             total_currency = 0
-            for i in iml:
-                if inv.currency_id.id != company_currency:
-                    i['currency_id'] = inv.currency_id.id
-                    i['amount_currency'] = i['price']
-                    i['price'] = cur_obj.compute(cr, uid, inv.currency_id.id,
-                            company_currency, i['price'],
-                            context={'date': inv.date_invoice or time.strftime('%Y-%m-%d')})
-                else:
-                    i['amount_currency'] = False
-                    i['currency_id'] = False
-                i['ref'] = ref
-                if inv.type in ('out_invoice','in_refund'):
-                    total += i['price']
-                    total_currency += i['amount_currency'] or i['price']
-                    i['price'] = - i['price']
-                else:
-                    total -= i['price']
-                    total_currency -= i['amount_currency'] or i['price']
+            total, total_currency, iml = self.compute_invoice_totals(cr, uid, inv, company_currency, ref, iml)
             acc_id = inv.account_id.id
 
             name = inv['name'] or '/'
@@ -756,26 +799,7 @@
 
             line = map(lambda x:(0,0,self.line_get_convert(cr, uid, x, part, date, context={})) ,iml)
 
-            if inv.journal_id.group_invoice_lines:
-                line2 = {}
-                for x, y, l in line:
-                    tmp = str(l['account_id'])
-                    tmp += '-'+str(l.get('tax_code_id',"False"))
-                    tmp += '-'+str(l.get('product_id',"False"))
-                    tmp += '-'+str(l.get('analytic_account_id',"False"))
-                    tmp += '-'+str(l.get('date_maturity',"False"))
-
-                    if tmp in line2:
-                        am = line2[tmp]['debit'] - line2[tmp]['credit'] + (l['debit'] - l['credit'])
-                        line2[tmp]['debit'] = (am > 0) and am or 0.0
-                        line2[tmp]['credit'] = (am < 0) and -am or 0.0
-                        line2[tmp]['tax_amount'] += l['tax_amount']
-                        line2[tmp]['analytic_lines'] += l['analytic_lines']
-                    else:
-                        line2[tmp] = l
-                line = []
-                for key, val in line2.items():
-                    line.append((0,0,val))
+            line = self.group_lines(cr, uid, iml, line, inv)
 
             journal_id = inv.journal_id.id #self._get_journal(cr, uid, {'type': inv['type']})
             journal = self.pool.get('account.journal').browse(cr, uid, journal_id)