Code review comment for lp:~pexego/account-financial-tools/adding_account_tools_from_extra_addons_to

Revision history for this message
Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c) wrote :

Dear Omar,

First of all thank you for your contribution on this project ! Happy to see them landing here. I have analyzed your module here, more from a functional / features point of view, rather than technical, but still I have some remarks:

 * account_chart_update : Seems the right place to land. The menu where it stands is good for me as it'll be the same people that will use it than the one who will create the account chart.

 * account_renumber : Seems useful in some cases for an administrator of the system, but it could definitely be dangerous for a lambda user. Move number are now related to invoice when they're issue by them. So using this by a non-administrator could lead to dangerous result. I would really prefer to have the menu entry in the administration menu for that purpose.

 * account_admin_tools : On this one, my opinion is that the import feature (for both account and move) should be made in a new module. No need to mix accounting importation features with the "repair" tools. Then, for the repair tools, as far as I can see, it's more repair tools used on past version (I say version 5.0 probably, but I'm may be wrong). Most of the provided features (but may be not all) are a bit obsolete from my point of view.

What I would suggest to you if, you agree, is to:

 * Make a merge proposal for both account_chart_update and account_renumber and moving the menu entry

 * Extract the importation tool form the account_admin_tools in a new module and make a MP for it in this project

 * For the remaining "check and repair" tools, may be we need opinion from others as well. On my side, I don't think it's a good module to include here. Or at least not with the current included feature.

Other comments welcome here. We also said that serie 6.1 should include old modules, so if you think I'm to "hard" here, please let me know. For now, I put my vote on disapprove, but I'm open to discussion here.

In any case, thank you for your MP !

Regards,

Joël

review: Disapprove

« Back to merge proposal