Merge lp:~akretion-team/openobject-addons/clean_main_supplier_for_product into lp:openobject-addons

[Sébastien BEAU - http://www.akretion.com]

Hello Olivier
We made the change and implement your solution.
You can review it.

Have a nice day

[Sébastien BEAU - http://www.akretion.com (sebastien.beau)]

We update the branch due to the last change done in addons. Also we fix a bug in our merge.
Can you test it? We really need this merge for next version.

Best regards

[Olivier Dony (Odoo) (odo-openerp)]

Hi Benoit, Sebastien,

I hope I'm reviewing the right version of your merge proposal (rev 6130) ;-) BTW you can simply choose the "Resubmit merge proposal" option on the top-right when you want to re-submit. This way the link between the 2 MPs is automatically shown, the comments are carried over to the new MP, and the old MP is closed as 'Superseded'.

Some remarks:

- If you don't mind, I'd prefer to keep the existing seller_{id,delay,qty} fields for backwards-compatibility, especially on a master data model like Products. As I mentioned earlier, having a single function for 'seller_info_id' and 3 related fields would work too, but you'd have to take care of the default values. Perhaps the simplest thing to do is to keep a single [multi-field] function computing the 4 fields at once, but delegating the choice of the main_supplier to a new overridable method?

- l.60: previous implementation did take care of possible NULL sequences, which apparently used to trigger bugs (see the bzr history). I don't understand why you removed the test?

- l.65: such a 'strange' proxy might need at least a docstring to explain its purpose, in order to prevent accidental removal. But maybe you don't actually need it if you keep the existing '_calc_seller' wrapper function to compute the rest of the fields...

So, to be more concrete, I mean you could achieve the desired effect with something along those lines (example untested code):

    def _get_main_product_supplier(self, cr, uid, product, context=None):
        sellers = [(seller_info.sequence, seller_info)
                       for seller_info in product.seller_ids or []
                       if seller_info and isinstance(seller_info.sequence, (int, long))]
        return sellers and sellers[0][1] or False

    def _calc_seller(self, cr, uid, ids, fields, arg, context=None):
        result = {}
        for product in self.browse(cr, uid, ids, context=context):
            main_supplier = self._get_main_product_supplier(cr, uid, product, context=context)
            result[product.id]['seller_info_id'] = main_supplier and main_supplier.id or False
            result[product.id]['seller_delay'] = main_supplier and main_supplier.delay or 1
            result[product.id]['seller_qty'] = main_supplier and main_supplier.qty or 0.0
            result[product.id]['seller_id'] = main_supplier and main_supplier.name.id or False
        return result

while keeping the existing seller* fields (set the same multi key everywhere, e.g. seller_info), plus one new field:

     'seller_info_id': fields.function(_calc_seller, type='many2one', relation="product.supplierinfo",
                                       multi="seller_info"),

Not sure if that's clear, but I think it could be a minimal patch that stays 100% backwards compatible (no need to patch any other module), while still allowing to easily override the supplier choice in the way you wanted.

What do you think?

PS: if you take my suggested approach, you could "pull --overwrite" from trunk into your branch to start again from scratch, and then "push --overwrite" into LP, so that we simpler/cleaner history when merging. But that's being nitpicky ;-)

review: Needs Fixing

[Sébastien BEAU - http://www.akretion.com (sebastien.beau)]

Ok so in order to be more backwards compatible, we chose your option, we will implement it and test it today or tomorrow and propose a new merge.
Regarding the --overwrite method I already know this tips ;) (very usefull sometime)

Thanks for all

[Benoit Guillot - http://www.akretion.com (benoit-guillot-z)]

Hello Olivier,

I have made the refactor according to your advice. I have tested it, it seems to work.

Best regards,

[Olivier Dony (Odoo) (odo-openerp)]

Just landed in trunk at rev. 6441 rev-id: <email address hidden>, with only a few minor fixes:

- added a docstring for _get_main_product_supplier(), for overriders
- moved the new seller_info_id next to the other fields implemented by the same function
- fixed the "multi" attribute of all these fields - they need to have the *same* value in order to be computed in one call, otherwise they will be computed in different calls, losing the advantage of 'multi'

Thanks for your contributions and your patience, as always!

review: Approve

=== modified file 'product/product.py'
--- product/product.py	2012-01-06 09:29:30 +0000
+++ product/product.py	2012-01-16 11:33:27 +0000
@@ -257,20 +257,23 @@
 class product_template(osv.osv):
     _name = "product.template"
     _description = "Product Template"
+
+    def _get_main_product_supplier(self, cr, uid, product, context=None):
+        sellers = [(seller_info.sequence, seller_info)
+                       for seller_info in product.seller_ids or []
+                       if seller_info and isinstance(seller_info.sequence, (int, long))]
+        return sellers and sellers[0][1] or False
+
     def _calc_seller(self, cr, uid, ids, fields, arg, context=None):
         result = {}
         for product in self.browse(cr, uid, ids, context=context):
-            for field in fields:
-                result[product.id] = {field:False}
-            result[product.id]['seller_delay'] = 1
-            if product.seller_ids:
-                partner_list = [(partner_id.sequence, partner_id)
-                                       for partner_id in  product.seller_ids
-                                       if partner_id and isinstance(partner_id.sequence, (int, long))]
-                main_supplier = partner_list and partner_list[0] and partner_list[0][1] or False
-                result[product.id]['seller_delay'] =  main_supplier and main_supplier.delay or 1
-                result[product.id]['seller_qty'] =  main_supplier and main_supplier.qty or 0.0
-                result[product.id]['seller_id'] = main_supplier and main_supplier.name.id or False
+            main_supplier = self._get_main_product_supplier(cr, uid, product, context=context)
+            result[product.id] = {
+                'seller_info_id': main_supplier and main_supplier.id or False,
+                'seller_delay': main_supplier and main_supplier.delay or 1,
+                'seller_qty': main_supplier and main_supplier.qty or 0.0,
+                'seller_id': main_supplier and main_supplier.name.id or False
+                }
         return result
 
     _columns = {
@@ -317,6 +320,7 @@
         'loc_row': fields.char('Row', size=16),
         'loc_case': fields.char('Case', size=16),
         'company_id': fields.many2one('res.company', 'Company',select=1),
+        'seller_info_id': fields.function(_calc_seller, type='many2one', relation="product.supplierinfo", multi="seller_info"),
     }
 
     def _get_uom_id(self, cr, uid, *args):