Merge lp:~akretion-team/carriers-deliveries/7-change-generate_shipping_labels-arg-dbl into lp:~camptocamp/carriers-deliveries/7.0-pending-merges-20140331

Proposed by David BEAL (ak)
Status: Rejected
Rejected by: Yannick Vaucher @ Camptocamp
Proposed branch: lp:~akretion-team/carriers-deliveries/7-change-generate_shipping_labels-arg-dbl
Merge into: lp:~camptocamp/carriers-deliveries/7.0-pending-merges-20140331
Diff against target: 40 lines (+5/-4)
1 file modified
base_delivery_carrier_label/ (+5/-4)
To merge this branch: bzr merge lp:~akretion-team/carriers-deliveries/7-change-generate_shipping_labels-arg-dbl
Reviewer Review Type Date Requested Status
Yannick Vaucher @ Camptocamp Disapprove
Leonardo Pistone Abstain
Review via email:

Description of the change

set picking in arg of generate_ship_label method
it's easier to inherit of this method

To post a comment you must log in.
Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote :

Not sure it is a good idea.

At least method shouldn't be public anymore as openerp standard method should use ids.

How is it supposed to be easier ?

review: Needs Information
Revision history for this message
David BEAL (ak) (davidbeal) wrote :

I override generate_shipping_labels().

I use picking directly without doing again browse(cr, uid, ids) to have my picking ever defined one line before the method call.

Ok to have _generate_shipping_labels()

Revision history for this message
Leonardo Pistone (lepistone) wrote :

I agree with Yannick there is a convention of sorts of passing lists of integers.

Since we will soon switch to the new API, I do not have a strong opinion now.

In the future (new API?), I would prefer passing some form of instance (and not ids) because in an isolated test you can easily build a test double. On the other hand, with the list of ids you would need to mock self.pool out, which is probably a pain.

review: Abstain
Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote :

Well self.browse(cr, uid, ids, context=context) is not that much a pain to write.

This change doesn't make it possible to keep the simple call pick.generate_shipping_labels(tracking_ids=tracking_ids)

I set this as disapprove unless you tell me there is a real gain with it, I think this is much a stylistic question.

review: Disapprove

Unmerged revisions

61. By David BEAL (ak) on 2014-03-18

[IMP] change signature generate_shipping_labels

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'base_delivery_carrier_label/'
2--- base_delivery_carrier_label/ 2014-03-17 12:41:18 +0000
3+++ base_delivery_carrier_label/ 2014-03-18 16:16:31 +0000
4@@ -53,7 +53,7 @@
5 string='Options'),
6 }
8- def generate_default_label(self, cr, uid, ids, tracking_ids=None,
9+ def generate_default_label(self, cr, uid, picking, tracking_ids=None,
10 context=None):
11 """ Abstract method
13@@ -68,7 +68,7 @@
14 'Error',
15 'No label is configured for selected delivery method.')
17- def generate_shipping_labels(self, cr, uid, ids, tracking_ids=None,
18+ def generate_shipping_labels(self, cr, uid, picking, tracking_ids=None,
19 context=None):
20 """Generate a shipping label by default
22@@ -90,7 +90,7 @@
23 pack
25 """
26- default_label = self.generate_default_label(cr, uid, ids,
27+ default_label = self.generate_default_label(cr, uid, picking,
28 tracking_ids=tracking_ids,
29 context=None)
30 if not tracking_ids:
31@@ -114,7 +114,8 @@
32 pickings = self.browse(cr, uid, ids, context=context)
34 for pick in pickings:
35- shipping_labels = pick.generate_shipping_labels(tracking_ids=tracking_ids)
36+ shipping_labels = self.generate_shipping_labels(
37+ cr, uid, pick, tracking_ids=tracking_ids, context=context)
38 for label in shipping_labels:
39 # map types with models
40 types = {'in': '',


People subscribed via source and target branches

to all changes: