Merge lp:~camptocamp/openobject-addons/trunk-refactor-po-merge-lep into lp:openobject-addons

Proposed by Leonardo Pistone on 2014-04-23
Status: Work in progress
Proposed branch: lp:~camptocamp/openobject-addons/trunk-refactor-po-merge-lep
Merge into: lp:openobject-addons
Diff against target: 438 lines (+302/-105)
3 files modified
purchase/purchase.py (+194/-105)
purchase/tests/__init__.py (+26/-0)
purchase/tests/test_merge_po.py (+82/-0)
To merge this branch: bzr merge lp:~camptocamp/openobject-addons/trunk-refactor-po-merge-lep
Reviewer Review Type Date Requested Status
Leonardo Pistone (community) in github Disapprove on 2014-06-05
Olivier Dony (Odoo) technical+functional 2014-04-23 Needs Fixing on 2014-05-06
Alexandre Fayolle - camptocamp (community) code review, no test Approve on 2014-05-06
Review via email: mp+216841@code.launchpad.net

Description of the change

A refactor of the code to merge purchase orders.

See the related bug: the existing code cannot be extended easily if, for example, a field is added to the order, be it a key for merging or not.

In this refactor, hopefully I keep the old functionality intact while providing hooks for extention. The existing yaml/python test still passes and I added a few other isolated unit tests.

I proposed the same change for v7 in a community module:

https://code.launchpad.net/~camptocamp/purchase-wkfl/7.0-merge-po-hooks-lep

Thanks!

To post a comment you must log in.

some issues reported on the piece of code contributed to OCA purchase_wkfl (https://code.launchpad.net/~camptocamp/purchase-wkfl/7.0-merge-po-hooks-lep/+merge/216745)

Setting this mp to work in progress.

review: Needs Fixing
Leonardo Pistone (lepistone) wrote :

I fixed the problems reported on the other MP, with a new unit test. I put that back in "needs review".

Thanks.

review: Approve (code review, no test)
Olivier Dony (Odoo) (odo-openerp) wrote :

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 ;-)

review: Needs Fixing (technical+functional)
Leonardo Pistone (lepistone) wrote :

Hi Olivier,

in general I agree with your remarks! I probably won't be able to fix that week, but I should next week. Let's just keep in touch so we don't do the same job twice :)

- on history management: it is a mistake, well spotted. I didn't want to change the behaviour.

- method signatures: a bit of a long discussion, and probably useless with the new API. Passing class instances instead of ids allows to easily write (isolated) unit tests like test_merge_origin_and_notes in this module. Here a mock is used, but the same approach can work with a real class instance. The same test for a method that accepts ids is more complex.

thanks!

Olivier Dony (Odoo) (odo-openerp) wrote :

On 05/07/2014 08:59 AM, Leonardo Pistone - camptocamp wrote:
> Hi Olivier,
>
> in general I agree with your remarks! I probably won't be able to fix that
> week, but I should next week. Let's just keep in touch so we don't do the
> same job twice :)

Sure, thanks!

> - method signatures: a bit of a long discussion, and probably useless with
> the new API. Passing class instances instead of ids allows to easily write
> (isolated) unit tests like test_merge_origin_and_notes in this module. Here
> a mock is used, but the same approach can work with a real class instance.
> The same test for a method that accepts ids is more complex.

Oh, perhaps I wasn't quite clear in my previous comment. I didn't mean you
should change your methods to use "ids" instead of browse_records, that's fine.
I meant that you could still pass the cr, uid and context (not the ids!) to
your methods, even if you don't use them. This does not make testing/mocking
the methods any harder, and provides a more flexible/friendly extension point
for other modules using the old API.
This would be similar to what we do in the typical _prepare* methods in other
modules, also working with browse_records, e.g.:
http://bazaar.launchpad.net/~openerp/openobject-addons/7.0/view/10034/purchase/purchase.py#L479
http://bazaar.launchpad.net/~openerp/openobject-addons/7.0/view/10034/sale/sale.py#L348

Whenever the `purchase` module gets ported to the new API those parameters
would indeed become irrelevant, but not all official modules will be ported to
the new API at once. And theoretically the backwards-compatibility layer should
allow cross-API method overriding, by working out the cross-API calls.

I hope this makes sense to you,

Leonardo Pistone (lepistone) wrote :

This proposal is now on github, https://github.com/odoo/odoo/pull/340. We keep the discussion here.
Thanks!

review: Disapprove (in github)

Unmerged revisions

9375. By Leonardo Pistone on 2014-04-29

[imp] merge po: make all keys tuples

9374. By Leonardo Pistone on 2014-04-29

[fix] merge po: correctly merge origin and notes, with a test

9373. By Leonardo Pistone on 2014-04-23

[add] some tests for the refactored merge po functionality

9372. By Leonardo Pistone on 2014-04-22

[imp] refactor the merge PO feature to allow it to be extended in other modules.

Forward port of the purchase_group_hooks module from lp:purchase-wkfl

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'purchase/purchase.py'
2--- purchase/purchase.py 2014-04-18 06:12:56 +0000
3+++ purchase/purchase.py 2014-04-29 09:08:54 +0000
4@@ -724,121 +724,210 @@
5 })
6 return super(purchase_order, self).copy(cr, uid, id, default, context)
7
8- def do_merge(self, cr, uid, ids, context=None):
9- """
10- To merge similar type of purchase orders.
11- Orders will only be merged if:
12- * Purchase Orders are in draft
13- * Purchase Orders belong to the same partner
14- * Purchase Orders are have same stock location, same pricelist
15- Lines will only be merged if:
16- * Order lines are exactly the same except for the quantity and unit
17-
18- @param self: The object pointer.
19- @param cr: A database cursor
20- @param uid: ID of the user currently logged in
21- @param ids: the ID or list of IDs
22- @param context: A standard dictionary
23-
24- @return: new purchase order id
25-
26- """
27- #TOFIX: merged order line should be unlink
28- def make_key(br, fields):
29- list_key = []
30- for field in fields:
31- field_val = getattr(br, field)
32- if field in ('product_id', 'move_dest_id', 'account_analytic_id'):
33- if not field_val:
34- field_val = False
35- if isinstance(field_val, browse_record):
36- field_val = field_val.id
37- elif isinstance(field_val, browse_null):
38- field_val = False
39- elif isinstance(field_val, list):
40- field_val = ((6, 0, tuple([v.id for v in field_val])),)
41- list_key.append((field, field_val))
42- list_key.sort()
43- return tuple(list_key)
44-
45- if context is None:
46- context = {}
47-
48- # Compute what the new orders should contain
49-
50- new_orders = {}
51-
52- for porder in [order for order in self.browse(cr, uid, ids, context=context) if order.state == 'draft']:
53- order_key = make_key(porder, ('partner_id', 'location_id', 'pricelist_id'))
54- new_order = new_orders.setdefault(order_key, ({}, []))
55- new_order[1].append(porder.id)
56- order_infos = new_order[0]
57- if not order_infos:
58- order_infos.update({
59- 'origin': porder.origin,
60- 'date_order': porder.date_order,
61- 'partner_id': porder.partner_id.id,
62- 'dest_address_id': porder.dest_address_id.id,
63- 'warehouse_id': porder.warehouse_id.id,
64- 'location_id': porder.location_id.id,
65- 'pricelist_id': porder.pricelist_id.id,
66- 'state': 'draft',
67- 'order_line': {},
68- 'notes': '%s' % (porder.notes or '',),
69- 'fiscal_position': porder.fiscal_position and porder.fiscal_position.id or False,
70- })
71+ def _key_fields_for_grouping(self):
72+ """Return a list of fields used to identify orders that can be merged.
73+
74+ Orders that have this fields equal can be merged.
75+
76+ This function can be extended by other modules to modify the list.
77+ """
78+ return ('partner_id', 'location_id', 'pricelist_id')
79+
80+ def _key_fields_for_grouping_lines(self):
81+ """Return a list of fields used to identify order lines that can be
82+ merged.
83+
84+ Lines that have this fields equal can be merged.
85+
86+ This function can be extended by other modules to modify the list.
87+ """
88+ return ('name', 'date_planned', 'taxes_id', 'price_unit', 'product_id',
89+ 'move_dest_id', 'account_analytic_id')
90+
91+ def _make_key_for_grouping(self, order, fields):
92+ """From an order, return a tuple to be used as a key.
93+
94+ If two orders have the same key, they can be merged.
95+ """
96+ key_list = []
97+ for field in fields:
98+ field_value = getattr(order, field)
99+
100+ if isinstance(field_value, browse_record):
101+ field_value = field_value.id
102+ elif isinstance(field_value, browse_null):
103+ field_value = False
104+ elif isinstance(field_value, list):
105+ field_value = ((6, 0, tuple([v.id for v in field_value])),)
106+ key_list.append((field, field_value))
107+ key_list.sort()
108+ return tuple(key_list)
109+
110+ def _can_merge(self, order):
111+ """Can the order be considered for merging with others?
112+
113+ This method can be surcharged in other modules.
114+ """
115+ return order.state == 'draft'
116+
117+ def _initial_merged_order_data(self, order):
118+ """Build the initial values of a merged order."""
119+ return {
120+ 'origin': order.origin,
121+ 'date_order': order.date_order,
122+ 'partner_id': order.partner_id.id,
123+ 'dest_address_id': order.dest_address_id.id,
124+ 'warehouse_id': order.warehouse_id.id,
125+ 'location_id': order.location_id.id,
126+ 'pricelist_id': order.pricelist_id.id,
127+ 'state': 'draft',
128+ 'order_line': {},
129+ 'notes': '%s' % (order.notes or '',),
130+ 'fiscal_position': (
131+ order.fiscal_position and order.fiscal_position.id or False
132+ ),
133+ }
134+
135+ def _update_merged_order_data(self, merged_data, order):
136+ if order.date_order < merged_data['date_order']:
137+ merged_data['date_order'] = order.date_order
138+ if order.notes:
139+ merged_data['notes'] = (
140+ (merged_data['notes'] or '') + ('\n%s' % (order.notes,))
141+ )
142+ if order.origin:
143+ if (
144+ order.origin not in merged_data['origin']
145+ and merged_data['origin'] not in order.origin
146+ ):
147+ merged_data['origin'] = (
148+ (merged_data['origin'] or '') + ' ' + order.origin
149+ )
150+ return merged_data
151+
152+ def _group_orders(self, input_orders):
153+ """Return a dictionary where each element is in the form:
154+
155+ tuple_key: (dict_of_new_order_data, list_of_old_order_ids)
156+
157+ """
158+ key_fields = self._key_fields_for_grouping()
159+ grouped_orders = {}
160+
161+ if len(input_orders) < 2:
162+ return {}
163+
164+ for input_order in input_orders:
165+ key = self._make_key_for_grouping(input_order, key_fields)
166+ if key in grouped_orders:
167+ grouped_orders[key] = (
168+ self._update_merged_order_data(
169+ grouped_orders[key][0],
170+ input_order
171+ ),
172+ grouped_orders[key][1] + [input_order.id]
173+ )
174 else:
175- if porder.date_order < order_infos['date_order']:
176- order_infos['date_order'] = porder.date_order
177- if porder.notes:
178- order_infos['notes'] = (order_infos['notes'] or '') + ('\n%s' % (porder.notes,))
179- if porder.origin:
180- order_infos['origin'] = (order_infos['origin'] or '') + ' ' + porder.origin
181+ grouped_orders[key] = (
182+ self._initial_merged_order_data(input_order),
183+ [input_order.id]
184+ )
185+ grouped_order_data = grouped_orders[key][0]
186
187- for order_line in porder.order_line:
188- line_key = make_key(order_line, ('name', 'date_planned', 'taxes_id', 'price_unit', 'product_id', 'move_dest_id', 'account_analytic_id'))
189- o_line = order_infos['order_line'].setdefault(line_key, {})
190+ for input_line in input_order.order_line:
191+ line_key = self._make_key_for_grouping(
192+ input_line,
193+ self._key_fields_for_grouping_lines()
194+ )
195+ o_line = grouped_order_data['order_line'].setdefault(
196+ line_key, {}
197+ )
198 if o_line:
199 # merge the line with an existing line
200- o_line['product_qty'] += order_line.product_qty * order_line.product_uom.factor / o_line['uom_factor']
201+ o_line['product_qty'] += (
202+ input_line.product_qty
203+ * input_line.product_uom.factor
204+ / o_line['uom_factor']
205+ )
206 else:
207 # append a new "standalone" line
208 for field in ('product_qty', 'product_uom'):
209- field_val = getattr(order_line, field)
210+ field_val = getattr(input_line, field)
211 if isinstance(field_val, browse_record):
212 field_val = field_val.id
213 o_line[field] = field_val
214- o_line['uom_factor'] = order_line.product_uom and order_line.product_uom.factor or 1.0
215-
216-
217-
218- allorders = []
219- orders_info = {}
220- for order_key, (order_data, old_ids) in new_orders.iteritems():
221- # skip merges with only one order
222- if len(old_ids) < 2:
223- allorders += (old_ids or [])
224- continue
225-
226- # cleanup order line data
227- for key, value in order_data['order_line'].iteritems():
228- del value['uom_factor']
229- value.update(dict(key))
230- order_data['order_line'] = [(0, 0, value) for value in order_data['order_line'].itervalues()]
231-
232- # create the new order
233- context.update({'mail_create_nolog': True})
234- neworder_id = self.create(cr, uid, order_data)
235- self.message_post(cr, uid, [neworder_id], body=_("RFQ created"), context=context)
236- orders_info.update({neworder_id: old_ids})
237- allorders.append(neworder_id)
238-
239- # make triggers pointing to the old orders point to the new order
240- for old_id in old_ids:
241- self.redirect_workflow(cr, uid, [(old_id, neworder_id)])
242- self.signal_purchase_cancel(cr, uid, [old_id]) # TODO Is it necessary to interleave the calls?
243- return orders_info
244-
245+ o_line['uom_factor'] = (
246+ input_line.product_uom
247+ and input_line.product_uom.factor
248+ or 1.0)
249+
250+ return self._cleanup_merged_line_data(grouped_orders)
251+
252+ def _cleanup_merged_line_data(self, grouped_orders):
253+ """Remove keys from merged lines, and merges of 1 order."""
254+ result = {}
255+ for order_key, (order_data, old_ids) in grouped_orders.iteritems():
256+ if len(old_ids) > 1:
257+ for key, value in order_data['order_line'].iteritems():
258+ del value['uom_factor']
259+ value.update(dict(key))
260+ order_data['order_line'] = [
261+ (0, 0, value)
262+ for value in order_data['order_line'].itervalues()
263+ ]
264+ result[order_key] = (order_data, old_ids)
265+ return result
266+
267+ def _create_new_orders(self, cr, uid, grouped_orders, context=None):
268+ """Create the new merged orders in the database.
269+
270+ Return a dictionary that puts the created order ids in relation to the
271+ original ones, in the form
272+
273+ new_order_id: [old_order_1_id, old_order_2_id]
274+
275+ """
276+ new_old_rel = {}
277+ for key in grouped_orders:
278+ new_order_data, old_order_ids = grouped_orders[key]
279+ new_id = self.create(cr, uid, new_order_data, context=context)
280+ new_old_rel[new_id] = old_order_ids
281+ return new_old_rel
282+
283+ def _fix_workflow(self, cr, uid, new_old_rel):
284+ """Fix the workflow of the old and new orders.
285+
286+ Specifically, cancel the old ones and assign workflows to the new ones.
287+
288+ """
289+ for new_order_id in new_old_rel:
290+ old_order_ids = new_old_rel[new_order_id]
291+ for old_id in old_order_ids:
292+ self.redirect_workflow(cr, uid, [(old_id, new_order_id)])
293+ self.signal_purchase_cancel(cr, uid, [old_id])
294+
295+ def do_merge(self, cr, uid, input_order_ids, context=None):
296+ """Merge Purchase Orders.
297+
298+ This method replaces the original one in the purchase module because
299+ it did not provide any hooks for customization.
300+
301+ Receive a list of order ids, and return a dictionary where each
302+ element is in the form:
303+
304+ new_order_id: [old_order_1_id, old_order_2_id]
305+
306+ New orders are created, and old orders are deleted.
307+
308+ """
309+ input_orders = self.browse(cr, uid, input_order_ids, context=context)
310+ mergeable_orders = filter(self._can_merge, input_orders)
311+ grouped_orders = self._group_orders(mergeable_orders)
312+
313+ new_old_rel = self._create_new_orders(cr, uid, grouped_orders,
314+ context=context)
315+ self._fix_workflow(cr, uid, new_old_rel)
316+ return new_old_rel
317
318 class purchase_order_line(osv.osv):
319 def _amount_line(self, cr, uid, ids, prop, arg, context=None):
320
321=== added directory 'purchase/tests'
322=== added file 'purchase/tests/__init__.py'
323--- purchase/tests/__init__.py 1970-01-01 00:00:00 +0000
324+++ purchase/tests/__init__.py 2014-04-29 09:08:54 +0000
325@@ -0,0 +1,26 @@
326+# -*- coding: utf-8 -*-
327+##############################################################################
328+#
329+# Author: Leonardo Pistone
330+# Copyright 2014 Camptocamp SA
331+#
332+# This program is free software: you can redistribute it and/or modify
333+# it under the terms of the GNU Affero General Public License as
334+# published by the Free Software Foundation, either version 3 of the
335+# License, or (at your option) any later version.
336+#
337+# This program is distributed in the hope that it will be useful,
338+# but WITHOUT ANY WARRANTY; without even the implied warranty of
339+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
340+# GNU Affero General Public License for more details.
341+#
342+# You should have received a copy of the GNU Affero General Public License
343+# along with this program. If not, see <http://www.gnu.org/licenses/>.
344+#
345+##############################################################################
346+
347+from . import test_merge_po
348+
349+checks = [
350+ test_merge_po,
351+]
352
353=== added file 'purchase/tests/test_merge_po.py'
354--- purchase/tests/test_merge_po.py 1970-01-01 00:00:00 +0000
355+++ purchase/tests/test_merge_po.py 2014-04-29 09:08:54 +0000
356@@ -0,0 +1,82 @@
357+from mock import Mock
358+
359+from openerp.tests.common import BaseCase
360+from openerp.osv.orm import browse_record
361+import openerp
362+from openerp.modules.registry import RegistryManager
363+
364+DB = openerp.tools.config['db_name']
365+
366+
367+class TestGroupOrders(BaseCase):
368+
369+ def setUp(self):
370+ super(TestGroupOrders, self).setUp()
371+
372+ self.order1 = Mock()
373+ self.order2 = Mock()
374+
375+ self.order1.order_line = self.order2.order_line = []
376+ self.order1.origin = self.order2.origin = ''
377+ self.order1.notes = self.order2.notes = ''
378+
379+ # I have to use the registry to get an instance of a model. I cannot
380+ # use the class constructor because that is modified to return nothing.
381+ self.registry = RegistryManager.get(DB)
382+ self.po = self.registry('purchase.order')
383+
384+ def test_no_orders(self):
385+ """Group an empty list of orders as an empty dictionary."""
386+
387+ grouped = self.po._group_orders([])
388+ self.assertEquals(grouped, {})
389+
390+ def test_one_order(self):
391+ """A single order will not be grouped."""
392+ grouped = self.po._group_orders([self.order1])
393+ self.assertEquals(grouped, {})
394+
395+ def test_two_similar_orders(self):
396+ """Two orders with the right conditions can be merged.
397+
398+ We do not care about the order lines here.
399+ """
400+ self.order1.partner_id = self.order2.partner_id = Mock(
401+ spec=browse_record, id=1)
402+ self.order1.location_id = self.order2.location_id = Mock(
403+ spec=browse_record, id=2)
404+ self.order1.pricelist_id = self.order2.pricelist_id = Mock(
405+ spec=browse_record, id=3)
406+
407+ self.order1.id = 51
408+ self.order2.id = 52
409+
410+ grouped = self.po._group_orders([self.order1, self.order2])
411+ expected_key = (('location_id', 2), ('partner_id', 1),
412+ ('pricelist_id', 3))
413+ self.assertEquals(grouped.keys(), [expected_key])
414+ self.assertEquals(grouped[expected_key][1], [51, 52])
415+
416+ def test_merge_origin_and_notes(self):
417+ self.order1.origin = 'ORIGIN1'
418+ self.order2.origin = 'ORIGIN2'
419+
420+ self.order1.notes = 'Notes1'
421+ self.order2.notes = 'Notes2'
422+
423+ self.order1.partner_id = self.order2.partner_id = Mock(
424+ spec=browse_record, id=1)
425+ self.order1.location_id = self.order2.location_id = Mock(
426+ spec=browse_record, id=2)
427+ self.order1.pricelist_id = self.order2.pricelist_id = Mock(
428+ spec=browse_record, id=3)
429+
430+ grouped = self.po._group_orders([self.order1, self.order2])
431+
432+ expected_key = (('location_id', 2), ('partner_id', 1),
433+ ('pricelist_id', 3))
434+
435+ merged_data = grouped[expected_key][0]
436+
437+ self.assertEquals(merged_data['origin'], 'ORIGIN1 ORIGIN2')
438+ self.assertEquals(merged_data['notes'], 'Notes1\nNotes2')

Subscribers

People subscribed via source and target branches

to all changes: