← Back to team overview

openerp-community-reviewer team mailing list archive

Re: [Merge] lp:~elicoidal/multi-company/multi-company into lp:multi-company

 

Review: Needs Fixing code review, no tests

Hi Eric,

This module sounds very promizing, but it'll really need time to review as it is a big one ! Before going into details, I first suggest to clean a bit the code:

 - Avoid using SQL directly and use the ORM instead (quite lots of places)
 - Some method are very long here, some refactoring would be very appreciated (e.g. 469, 769, 1572,...)

I would also personally loved to have some better explanation in the description of the module and help tooltip of fields. As this may lead to a very complexe behaviour in OpenERP, better to have good docs on how it works in case of cancellation or partial docs for example.

Would you think you'll have time time do this ?

Honestly, this is a very needed module, so I think it worth the pain !

Thanks in advance,

Regards,

Joël
-- 
https://code.launchpad.net/~elicoidal/multi-company/multi-company/+merge/179623
Your team Multi Company Core Editors is subscribed to branch lp:multi-company.


Follow ups