Code review comment for lp:~openbmsjsc/openobject-addons/addons-extra-trunk-l10n_vn

Hello Phong!

Thank you for contributing to OpenERP, I think it is a very good first merge proposal!

I am not a specialist for Chart of Accounts matters, but I will give you some technical feedback first, and I will ask other colleagues that are experts with l10n modules to review the accounting details afterwards.

Here is a list of the good things and bad things I noticed (in no particular order).

Good:
- Proper use of "templates" object for most cases, without falling for the usual newcomer trap of creating real accounts, taxes, etc. in the l10n module. Good!
- Correct creation of an extra-addons branch with the addition of your module in it, this is an unintuitive part, well done!
- Correct layout of the module files, and module descriptor
- Correct licensing info
- Correct export of l10n_vn.pot and vi.po in i18n directory, nice!
- You have currently proposed the merge of l10n_vn in the extra-trunk branch, which makes sense if you want to make it available as an extra addon. This means that the community (~openerp-commiter) should probably help review your efforts. Also, if you wish to see it included in OpenERP 6.1, you can also later submit it against openobject-addons/trunk, after the cleanup.

Bad:
- You have lots of stuff that is commented out in your files (XML records, module descriptor, etc.). Comments are useful for documentation, but not to keep obsolete data around. For example you are still including the unused "account_chart.xml" file, which appears unused in your module, but takes almost 50% of the merge proposal! See guideline 2.1 in [1].
- You seem to have declared some reporting wizards in l10n_vn_account_chart_wizard_vi.xml using <wizard> but the definition of these wizard is nowhere to be found? It is not clear what you want to do exactly but in 6.0 wizards should be implemented using osv_memory objects. See [2] for details and see other modules for examples, like l10n_be.
- As a consequence of previous item, your l10n_vn.py seems quite strange and useless at the moment, as well as the related security access rights, etc.
- account.account.type's name field is not translatable anymore in 6.0 (as it was most of the time useless), so you can directly use the vietnamese names for your account types, and their translations should not be found anymore in po/pot files next time your export them.
- Unused python imports in your python files (e.g. 'import report' in __init__.py, and others in l10n_vn.py)

The rest seems good to me on a technical side.
Functionally, if you have not yet seen them, please read carefully the guidelines for l10n modules we gave during the l10n-community-sprint last year (see [3]).
Then once the small technical details above are solved, the final review by an accounting expert should be quick and there should be no remaining issue to merge your l10n_vn into official addons!

Congratulations once again for this nice work!

[1] http://doc.openerp.com/v6.0/contribute/15_guidelines/coding_guidelines.html
[2] http://doc.openerp.com/v6.0/developer/3_10_wizard/index.html#guidelines-on-how-to-convert-old-style-wizard-to-new-osv-memory-style
[3] http://pad.openerp.com/6-test-accounting-localisation-guidelines