openerp-community-reviewer team mailing list archive

[Pedro Manuel Baeza <pedro.baeza@xxxxxxxxx>]

Review: Needs Fixing code review and test

Hi, Joaquin, thanks for the excellent work you are doing with spanish localization and with tools like this! 

There are few things to change that I have seen testing this module:

- It's better to put the renumber wizard as a real wizard, not in view form, because that causes an strange behaviour. You can see an example of this type of wizards in Configuration > Modules > Update module list.
- Please rename menu entry to "Renumber journal entries" to match with current OpenERP terminology.
- test folder is not used by the module itself. Maybe real YAML test can be better solution. Do you dare? ;)

>From technical side, you can improve your code and follow community standards correcting these things:

- In __init__.py files, use "from . import xxxx" instead of "import xxxx" form.
- Remove __author__ variables in files, because authors are put on manifest file (__openerp__.py).
- Put only top-most dependency (account) for clarity, because the subsequent dependencies are calculated automatically.
- Remove 'init_xml' keys, because it's no longer needed in v7.
- Rename 'demo_xml' key to the new standard 'demo'.
- Be PEP8 compliant. The code is almost compliant, but there are few lines whose length is greater than 79. You can use flake8 tool to check this.

And finally, there's one thing from functional side. Use this scenario:

1) Put all the spanish accounting in a fresh BD, including accounts and periods.
2) Create one account entry on "01/2013" and date 01/01/2013.
3) Create one account entry on "Apertura 2013" and date 01/01/2013.
4) Renumber both periods.

In this scenario, the first one will be the entry on "01/2013" instead of the one on "Apertura 2013". This is the case I mentioned previously to you by mail.

Thanks again for your work.

Cheers.
-- 
https://code.launchpad.net/~joaquing-pedrosa/account-financial-tools/account-financial-tools/+merge/187204
Your team OpenERP Community Reviewer is subscribed to branch lp:account-financial-tools.