Re: [Openerp-community-reviewer] [Merge] lp:~gdgellatly/server-env-tools/base-synchro-7.0 into lp:server-env-tools
Review: Needs Fixing code review, no tests If people are willing to maintain this, then I'm not opposed to merging in server-env-tools. 2 points, though, to gain my approval: * the various size constraints seem overzealous to me (esp. server URL and domain which are obviously too short for no good reason) * there are no automated tests, and for functionality such as this one, this is definitely needed. -- https://code.launchpad.net/~gdgellatly/server-env-tools/base-synchro-7.0/+merge/200127 Your team Server Environment And Tools Core Editors is subscribed to branch lp:server-env-tools. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~gdgellatly/server-env-tools/base-synchro-7.0 into lp:server-env-tools
Review: Abstain Hello, I would say we could give a chance for people interested to support it until something better is made. Like many, I will probably not invest on it, if I invest on that I would invest on something different indeed. But if it does the job for some people... It isn't a lot of code either to maintain. -- https://code.launchpad.net/~gdgellatly/server-env-tools/base-synchro-7.0/+merge/200127 Your team Server Environment And Tools Core Editors is subscribed to branch lp:server-env-tools. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~gdgellatly/server-env-tools/base-synchro-7.0 into lp:server-env-tools
Hi, I'm then also in favor of keeping this module if 2 entities are together to maintain it. Just for those who may miss that : https://code.launchpad.net/~openerp-community/openobject-addons/elico-7.0 ElicoCorp has made a synchronization tools based on the generic connector we built as a base for Magento sync. This tools allow to sync PO -SO, invoices, etc... It misses the XML-RPC stuff and work only on a single DB between different companies, but I found the principle of using it great ! It's planned to improve that in the next version. Let's keep that module then. Graeme, thank you for your work. Regards, Joël -- https://code.launchpad.net/~gdgellatly/server-env-tools/base-synchro-7.0/+merge/200127 Your team Server Environment And Tools Core Editors is subscribed to branch lp:server-env-tools. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~gdgellatly/server-env-tools/base-synchro-7.0 into lp:server-env-tools
Review: Needs Information Hi, First of all, thank you a lot for all this work, my further comment doesn't criticize any of your investment in here. As this module has been abandoned by OpenERP, I'm not sure it's good to have it in our community modules. For what I know about this module he really had lots of bugs. It only work for small range and simple stuffs, from the point your start making real cases you face lots of troubles because it's root design is not really appropriate to support your business cases (translation support, hell hard to debug,...). So, I ask for your opinion to you all : would you try to assume such module here ? I'm not convinced, but I'm open to discuss it with you all. My main arguments in favor of rejecting it here is: * No atomic job to import object, if one failed, everything stop. We need Atomicity in the synchronization (as we do for the generic connector here https://launchpad.net/openerp-connector * Very difficult to debug as it's not atomic. When something fail, you have to investigate everything * As far as I know/remember you don't have any logs of what is done or not * As it was bad designed and abandoned by the editors, I will rather prefer to develop a clean one base on the community generic connector rather than trying to support this one. What do you guys think ? Regards, Joël -- https://code.launchpad.net/~gdgellatly/server-env-tools/base-synchro-7.0/+merge/200127 Your team Server Environment And Tools Core Editors is subscribed to branch lp:server-env-tools. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
Re: [Openerp-community-reviewer] [Merge] lp:~gdgellatly/server-env-tools/base-synchro-7.0 into lp:server-env-tools
Oops, meant to mention the insertion of the cr.commit() after each object is synced within the main synchronise method of the wizard. I don't really know any other way of doing it. The RPCProxy commits on each record. Provided one record commits correctly it is highly unlikely future records for an object will not, as the object model matches on both, so as a middle ground I put a database commit on the base_server after each object. If it isn't there and a sync fails on an object/model for any reason, then all prior synced object/model will duplicate because the mapping between record ids fails to write to the db. Happy to take advice on a better way of doing this, SAVEPOINT just didn't seem to do what I wanted but I could of got it wrong. Graeme -- https://code.launchpad.net/~gdgellatly/server-env-tools/base-synchro-7.0/+merge/200127 Your team Server Environment And Tools Core Editors is requested to review the proposed merge of lp:~gdgellatly/server-env-tools/base-synchro-7.0 into lp:server-env-tools. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp
