openerp-community-reviewer team mailing list archive

[Graeme Gellatly <gdgellatly@xxxxxxxxx>]

Review: Needs Fixing code review, testing

Hi,

Firstly thanks for the module and merge proposal.  I am using it in a current project and therefore have used and tested reasonably extensively.

There is some cut and paste of doc strings and these have been duplicated.

1 example here
class base_synchro_obj(osv.osv):
    '''Class to store the operations done by wizard'''

class base_synchro_obj_line(osv.osv):
    '''Class to store the operations done by wizard'''

Also with that docstring it kind of makes sense for base_synchro_line but in actual use that model can be used by many objects, methods etc and it is not solely for use by "the wizard".  Perhaps a better docstring would be "Model to store mappings between local ids and remote ids of synchronised objects"
The XML could use a quick tidy up on line 3.  It is confusing the way menuitem indents straight in to a view record.

Usability
The menu arrangement is a bit counterintuitive.  You need to go to Synchronisation History to create a new object to sync.
Fields to Avoid is highly counterintuitive in interface
See this XML
<field name="avoid_ids" colspan="4" nolabel="1">
     <tree string="Fields" editable="bottom">
          <field name="name"/>
      </tree>
So a bunch of fields that are to be excluded appear in the view with just the title "fields".
Also it is worth noting that these are char fields and require the technical field name meaning it becomes for very advanced user only and a bit error prone.

I have created an extension module which I would like to merge the functionality in to the base module.
This includes populating the list of fields with a checkbox to select which fields to sync using both technical name and label.
Automation of the wizard to use in cron jobs with optional report sending.
Ability to sequence multiple server syncs so they don't run at same time creating possible merge conflicts.

Alternatively if this MP is dead I could do the cleanup and make a new MP
-- 
https://code.launchpad.net/~serpent-consulting-services/server-env-tools/7.0-base_synchro/+merge/183102
Your team Server Environment And Tools Core Editors is subscribed to branch lp:server-env-tools.