openerp-community-reviewer team mailing list archive

[Graeme Gellatly <gdgellatly@xxxxxxxxx>]

Review: Disapprove manual testing on real data

Hi,

I just want to put a blanket disapprove here while I debug this module.  It is causing many errors including duplicating menus and syncing the wrong way.

By way of example of one obvious error in the synchronise function of the wizard you have at the start

        pool1 = RPCProxy(server)
        pool2 = pool

a bit further down when synchronising you have

        for dt, id, action in ids:
            iii += 1
            if action == 'd':
                pool_src = pool2
                pool_dest = pool1
            else:
                pool_src = pool1
                pool_dest = pool2

where action 'd' = download - i.e. they are the wrong way around, meaning the server synchronises the wrong object (which after a few hours debugging starts explaining my IndexError's which are silently passed)

Also there is an attempt to sort the ids returned, but as one function returns a list of tuples and the other a list of lists it doesn't actually work meaning older objects can get synced after newer ones causing clashes if the same one os updated.

Also as a style note, and my own personal take, Python has the builtin function enumerate(), which will allow you to set an index on the for loop, rather than incrementing iii.  That said in this case it is pointless as the only thing the counter is used for is

            if not (iii % 50):
                pass

Just another redundant code piece there is
                self.report_total += 1
                self.report_write += 1
and
                self.report_total += 1
                self.report_create += 1
shortly after each other in a mutually exclusive if, then, else.  No need for report_total as it is always self.report_write + self.report_create 

This module probably needs more community actual testing than just me, just looking at code is not enough.
-- 
https://code.launchpad.net/~serpentcs/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.