Hello, Thank you for your prompt and nice inputs. I have a new revision which fixes the issues you raised. Can you have a look please ? Regards, Tidiane Sy(Baamtu) > 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.
_______________________________________________ Mailing list: https://launchpad.net/~openerp-community Post to : [email protected] Unsubscribe : https://launchpad.net/~openerp-community More help : https://help.launchpad.net/ListHelp
