openerp-community-reviewer team mailing list archive

[Pedro Manuel Baeza <pedro.baeza@xxxxxxxxx>]

Review: Needs Fixing code review

Hi, Alex, thank you very much for the contribution. Some remarks:

- l.137: Please close string before \ to avoid that spaces on translations (you will have to export again translations):

help="International Commercial Terms are a series of predefined " \
    "commercial terms used in international transactions."

- l.135,382,392: Is it necessary to put such a complicated xpath? This makes more difficult to migrate code to future versions. If there is only one 'date_due' field, you can put directly <field name="date_due".../>. If there is more than one, at least you can put relative xpath to make it more portable: <xpath expr="//page[@string='Other Info']//field[@name='date_due']"

- l.302,311,333,339: indentation of the second line of methods must be indented higher than the indentation of the nested block according PEP8 (http://legacy.python.org/dev/peps/pep-0008/#indentation). This is my suggestion:

def _prepare_invoice_group(
        self, cr, uid, picking, partner,
        invoice, context=None):
    invoice_vals = ...

- Don't you think that it's convenient to copy incoterm (if any) from sale order to picking?

Very good detail including a test.

Regards.
-- 
https://code.launchpad.net/~agilebg/account-invoicing/adding_stock_invoice_picking_incoterm_7/+merge/213987
Your team Account Core Editors is subscribed to branch lp:account-invoicing.