Code review comment for lp:~camptocamp/openobject-addons/trunk-refactor-po-merge-lep

Hi,

The refactoring looks good, and the extra test are always useful, thanks Leonardo! I see it's green on runbot at the moment.

Note: as requested by Alexandre on Twitter, I confirm that this should not be proposed for stable, as it is technically not a bugfix and is a non-trivial change (even if it looks relatively safe). For trunk it's great.

Some (mostly minor) remarks:

- It seems 2 lines related to history management were lost after the refactoring. If this was not on purpose, I suggest preserving them to keep the original behavior:
  + l.233: context.update({'mail_create_nolog': True})
  + l.235: self.message_post(cr, uid, [neworder_id], body=_("RFQ created"), context=context)
- do_merge(): why name the argument `input_order_ids` instead of the typical `ids`? It seems shorter and quite appropriate here.
- _can_merge(): I'd suggest renaming it to _can_be_merged() to make its use more obvious. Better names mean less comments and shorter docstrings ;-)
- method signatures: Many/Most of the new methods do not take the usual cr, uid and context parameters. Wouldn't it be better to have them just in case? Other modules might need data from other models at any point even if the base implementation doesn't, and having those parameters directly available would be much more convenient. Sometimes the methods receive a browse_record which gives indirect access to cr, uid and context, but that's not a very straightforward API.
- l.98, l.210: while you're refactoring the code, you could get rid of the getattr(browse_record, field) instances; browse_record support getitem too so browse_record[field] works just fine and is more readable.
- This comment in do_merge()'s docstring can presumably be dropped as it comes from your separate module:
   "This method replaces the original one in the purchase module
    because it did not provide any hooks for customization...."
- Grammar/Spellchecking in docstrings: 'surcharge' should usually be 'override' in the context of [Python] methods.

If you'd like I can make the changes myself in a separate branch before merging, but I can't promise it I will get around to doing it this week. We're currently doing a final review + merge of the trunk-wms branch (new v8 WMS) right now and it definitely conflicts with your changes... so whichever gets merged first will leave the conflict resolution burden to the other ;-)