Code review comment for lp:~gkliska/openobject-addons/l10n_hr

Hi,

Thanks for contributing a chart of accounts for Croatia!

The definition of the chart looks good to me, well done! You've even correctly adapted the trigger of the installation wizard to 7.0 style, great :-)

I cannot review the actual contents of the Chart of Account on an accounting/legal point of view (we'll trust you there) but on a technical point of view, I see a few minor things that should be fixed before merging:

- l10n_hr/data/account.fiscal.position.template.csv looks like a temporary export file, it's not used and should be removed, as it's redundant with l10n_hr/data/fiscal_position_template.xml

- the i18n/ sub-directory can be dropped, since the Chart of Accounts data is not meant to be translated. You seem to (correctly) use local names for the account types, so there is no need for translation at all, and no need to require all OpenERP translators to translate them in their language ;-)

- l10n_hr/l10n_hr_chart_template.xml: using the "search=[domain]" syntax for referencing an account.account.template is dangerous here, because you could end up selecting one from a different Chart of Account (if several are installed and have similar codes). Why not use the ref="xml_id" syntax that you have used for bank_account_view_id (the XML IDs are guaranteed to be unique)?

Once the above is fixed I'll be glad to merge it for 7.0 (provided the branch passes our testsuite, which it should)

Thanks!