openerp-community team mailing list archive

["Olivier Dony \(OpenERP\)" <odo@xxxxxxxxxxx>]

Review: Needs Fixing
Hello,

Thanks for this nice contribution, impressive work!

On first look, the structure of the module looks right, and seems to follow most of the guidelines of http://pad.openerp.com/6-test-accounting-localisation-guidelines

I will let my colleagues review it too before we can merge it in trunk, and include it in the next version.

Note that in the mean time you can already register it on apps.openerp.com to make it visible to everyone for 6.0. To do that you will need to push a new branch with only this module in it, and register it's launchpad URL on http://apps.openerp.com/send_branch/

Here are a few technical issues that you can fix right now:

1) The module descriptor.
In 6.0 and trunk, the module descriptor should be named __openerp__.py, not __terp__.py anymore (it still works in 6.0 but is deprecated, and may be dropped in trunk).
Also, it would be nice if the description of the module included the list of countries that use SYSCOHADA, and perhaps a link to some online official reference.

2) The copyright notice in headers should not have "All rights reserved" anywhere.
This phrase was required by an old international convention, but it is now obsolete, and not appropriate in GPL source code. The normal "Copyright YEAR, AUTHOR" is sufficient and less confusing :-)

3) "CFA" currency should be declared in 'base' and needs fixing.
Usually we try to have currencies in the server's 'base' module (where res.currency is located).
The reason is that otherwise different modules might add the same currencies and it will lead to duplicate entries. For example if we add it now in 'base', people with l10n_syscohada will have 2 different CFA currencies, leading to confusion. Here is how to fix this cleanly:
1. Make a separate merge proposal on openobject-server/trunk to add CFA to base_data.xml - it's a simple change, we'll merge it quickly. Be careful that as of 6.0, res.currency does not have a 'code' field anymore (name==code), and the 'rate' field should not be assigned explicitly (it's read-only), it should be done via a res.currency.rate entry. You might also want to have a 'symbol' field.
2. For compatibility with 6.0 and servers that don't have CFA currency, you can keep it in l10n_syscohada, but please change the XML ID to "base.CFA". This way the master currency will be in base, and it won't be created twice, but instead updated if it's already created by base. And it will still be created if base did not have it.

After fixing the above, please add a comment to this merge proposal to notify us, as launchpad does not notify of code changes in merge proposals by default.

Thanks a lot!
-- 
https://code.launchpad.net/~tsy/openobject-addons/l10n_syscohada/+merge/56816
Your team OpenERP Community is subscribed to branch lp:~tsy/openobject-addons/l10n_syscohada.