Code review comment for lp:~pedro.baeza/account-financial-tools/7.0-account_chart_update-enhanced

Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote :

Hello thanks,

I see a lot of improvements here.

My remarks:

Why renaming tax_template to tax_templ? Readability counts. And it makes me wish I could auto complete those while reading the code.

It seams strange to me that you removed required=True of chart_template_id

When shouldn't it be required ? Code shouldn't rely on the view code to ensure a field is require.

An empty chart_template_id will raise errors in those methods:

name_get
_map_tax_code_template
_find_tax_codes
_find_accounts

There are some missing context:
l.1332
l.1341
l.1516
l.1646
l.2031

review: Needs Fixing (code review, no test)

« Back to merge proposal