Merge lp:~wiz-keed/ocb-addons/7.0-delivery-carrier-multi-company-awareness into lp:ocb-addons

[Paul Catinean]

[FIX] Added multi-company awareness to delivery module

[Stefan Rijnhart (Opener) (stefan-opener)]

This could seriously mess up existing installations. I'm not against the new field, but having the default function in OCB means that when this field is added, all carriers get the company of the admin user, making all carriers unavailable instantly when working on other companies before reconfiguration. Even having it in the official distribution is debatable IMHO, sharing carriers seems like a sane default to me. Would you consider removing the default?

Syntactically, your spacing seems off in both python and xml files. Do you mix tabs and spaces?

l.20: required=False is void as this is the default.

review: Needs Fixing

[lp:~wiz-keed/ocb-addons/7.0-delivery-carrier-multi-company-awareness]

10074. By Paul Catinean on 2014-04-04

[FIX] Removed default value for company_id, cleaned up syntax

[Stefan Rijnhart (Opener) (stefan-opener)]

Thanks for your changes. Code looks good now.

One thing I missed earlier, did you allow for carriers with a company one level up from the user's company on purpose? This is encoded in the clause ('company_id.child_ids','child_of',[user.company_id.id]). It is more common not to do so, except for products and documents.

review: Needs Information

[Paul Catinean (wiz-keed)]

I did do it on purpose taking into example indeed the product/pricelist/shop model.The only logic used in here is that if the company is a child of the one set it should inherit it's delivery methods.

I though it would be confusing for someone that made the parent-child structure why it should not appear in a child if the delivery is set on the parent company and if he would want it visible on all he should remove company all together.

Furthermore I think it allows more granular filtration (for example two parent companies and with 2 children each).One could set delivery methods on the two parent companies to make available for all children and company-specific as well

IMO it has no drawbacks and gives only more flexibility, It's up to you on this one though, I have just started working with multi-company 1 week ago and lack experience on this subject

Let me know what I should do

[Stefan Rijnhart (Opener) (stefan-opener)]

Thanks for the response! It's not so much more flexible as simply more accessible. But then, sharing carriers between the parent company and its children is not as problematic as sharing accounting move lines. So I agree with the current, broad rule.

Last couple of details:
- you'll want to set noupdate="1" for the rule as per OpenERP convention, and
- you can remove the assignment of 'global', which is a function field.

Following the policy of OCB, you created an accompanying proposal against upstream openobject-addons. However, you targetted openobject-addons/7.0 but you had better target trunk instead as this could well be regarded as a feature request by OpenERP SA (see https://bugs.launchpad.net/ocb-addons/+bug/1253701 for reference). The upstream proposal needs to be updated with the same changes as this branch, of course.

review: Needs Fixing

[lp:~wiz-keed/ocb-addons/7.0-delivery-carrier-multi-company-awareness]

10075. By Paul Catinean on 2014-04-05

[FIX] Fixed view to match standard conventions

[Paul Catinean (wiz-keed)]

One more I had the feeling that noupdate should have been 1 but I copied from source (will not proceed like this anymore) and with the global="true" I must admit I did not know exactly what it done I just assumed it's a required field in order to concat that to every domain ran on that model

I have made the changes you required but mind you I have no tested them yet

[Stefan Rijnhart (Opener) (stefan-opener)]

Thanks for the changes and proposing the exact same changes to trunk! LGTM.

review: Approve

[Paul Catinean (wiz-keed)]

My pleasure Stefan

[Holger Brunn (Therp) (hbrunn)]

review: Approve (code review)

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

LGTM
Thanks

review: Approve

[Holger Brunn (Therp) (hbrunn)]

Development for 7.0 has moved to github on https://github.com/OCA/ocb - please move your merge proposal there if it is still valid.

(I close and reject this in order to have a cleaner overview for 6.1 MPs which indeed have to be done on launchpad)

review: Disapprove

Unmerged revisions

10075. By Paul Catinean on 2014-04-05

[FIX] Fixed view to match standard conventions

10074. By Paul Catinean on 2014-04-04

[FIX] Removed default value for company_id, cleaned up syntax

10073. By Paul Catinean on 2014-04-04

[FIX] Added multi-company awareness to delivery module

=== modified file 'delivery/__openerp__.py'
--- delivery/__openerp__.py	2014-03-10 08:54:20 +0000
+++ delivery/__openerp__.py	2014-04-05 09:24:49 +0000
@@ -35,6 +35,7 @@
     'depends': ['sale', 'purchase', 'stock'],
     'data': [
         'security/ir.model.access.csv',
+        'security/delivery_security.xml',
         'delivery_report.xml',
         'delivery_view.xml',
         'partner_view.xml',
 
=== modified file 'delivery/delivery.py'
--- delivery/delivery.py	2014-03-10 08:54:20 +0000
+++ delivery/delivery.py	2014-04-05 09:24:49 +0000
@@ -64,6 +64,7 @@
     _columns = {
         'name': fields.char('Delivery Method', size=64, required=True),
         'partner_id': fields.many2one('res.partner', 'Transport Company', required=True, help="The partner that is doing the delivery service."),
+        'company_id': fields.many2one('res.company', 'Company'),
         'product_id': fields.many2one('product.product', 'Delivery Product', required=True),
         'grids_id': fields.one2many('delivery.grid', 'carrier_id', 'Delivery Grids'),
         'price' : fields.function(get_price, string='Price'),
 
=== modified file 'delivery/delivery_view.xml'
--- delivery/delivery_view.xml	2014-03-10 08:54:20 +0000
+++ delivery/delivery_view.xml	2014-04-05 09:24:49 +0000
@@ -32,6 +32,7 @@
                             <field name="product_id"/>
                         </group>
                         <group>
+                        <field name="company_id" groups="base.group_multi_company"/>
                         <field name="active"/>
                         </group>
                     </group>
 
=== added file 'delivery/security/delivery_security.xml'
--- delivery/security/delivery_security.xml	1970-01-01 00:00:00 +0000
+++ delivery/security/delivery_security.xml	2014-04-05 09:24:49 +0000
@@ -0,0 +1,12 @@
+<?xml version="1.0" encoding="utf-8"?>
+<openerp>
+<data noupdate="1">
+
+    <record model="ir.rule" id="delivery_carrier_rule">
+        <field name="name">delivery_carrier multi-company</field>
+        <field name="model_id" search="[('model','=','delivery.carrier')]" model="ir.model"/>
+        <field name="domain_force">['|','|',('company_id.child_ids','child_of',[user.company_id.id]),('company_id','child_of',[user.company_id.id]),('company_id','=',False)]</field>
+    </record>
+
+</data>
+</openerp>