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

Proposed by Paul Catinean on 2014-04-04
Status: Rejected
Rejected by: Holger Brunn (Therp) on 2014-11-24
Proposed branch: lp:~wiz-keed/ocb-addons/7.0-delivery-carrier-multi-company-awareness
Merge into: lp:ocb-addons
Diff against target: 52 lines (+15/-0)
4 files modified
delivery/__openerp__.py (+1/-0)
delivery/delivery.py (+1/-0)
delivery/delivery_view.xml (+1/-0)
delivery/security/delivery_security.xml (+12/-0)
To merge this branch: bzr merge lp:~wiz-keed/ocb-addons/7.0-delivery-carrier-multi-company-awareness
Reviewer Review Type Date Requested Status
Holger Brunn (Therp) Disapprove on 2014-11-24
Guewen Baconnier @ Camptocamp Approve on 2014-05-26
Stefan Rijnhart (Opener) 2014-04-04 Approve on 2014-04-05
Review via email: mp+214176@code.launchpad.net

Description of the change

[FIX] Added multi-company awareness to delivery module

To post a comment you must log in.

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
10074. By Paul Catinean on 2014-04-04

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

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) wrote :

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

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
10075. By Paul Catinean on 2014-04-05

[FIX] Fixed view to match standard conventions

Paul Catinean (wiz-keed) wrote :

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

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

review: Approve
Paul Catinean (wiz-keed) wrote :

My pleasure Stefan

review: Approve (code review)

LGTM
Thanks

review: Approve
Holger Brunn (Therp) (hbrunn) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'delivery/__openerp__.py'
2--- delivery/__openerp__.py 2014-03-10 08:54:20 +0000
3+++ delivery/__openerp__.py 2014-04-05 09:24:49 +0000
4@@ -35,6 +35,7 @@
5 'depends': ['sale', 'purchase', 'stock'],
6 'data': [
7 'security/ir.model.access.csv',
8+ 'security/delivery_security.xml',
9 'delivery_report.xml',
10 'delivery_view.xml',
11 'partner_view.xml',
12
13=== modified file 'delivery/delivery.py'
14--- delivery/delivery.py 2014-03-10 08:54:20 +0000
15+++ delivery/delivery.py 2014-04-05 09:24:49 +0000
16@@ -64,6 +64,7 @@
17 _columns = {
18 'name': fields.char('Delivery Method', size=64, required=True),
19 'partner_id': fields.many2one('res.partner', 'Transport Company', required=True, help="The partner that is doing the delivery service."),
20+ 'company_id': fields.many2one('res.company', 'Company'),
21 'product_id': fields.many2one('product.product', 'Delivery Product', required=True),
22 'grids_id': fields.one2many('delivery.grid', 'carrier_id', 'Delivery Grids'),
23 'price' : fields.function(get_price, string='Price'),
24
25=== modified file 'delivery/delivery_view.xml'
26--- delivery/delivery_view.xml 2014-03-10 08:54:20 +0000
27+++ delivery/delivery_view.xml 2014-04-05 09:24:49 +0000
28@@ -32,6 +32,7 @@
29 <field name="product_id"/>
30 </group>
31 <group>
32+ <field name="company_id" groups="base.group_multi_company"/>
33 <field name="active"/>
34 </group>
35 </group>
36
37=== added file 'delivery/security/delivery_security.xml'
38--- delivery/security/delivery_security.xml 1970-01-01 00:00:00 +0000
39+++ delivery/security/delivery_security.xml 2014-04-05 09:24:49 +0000
40@@ -0,0 +1,12 @@
41+<?xml version="1.0" encoding="utf-8"?>
42+<openerp>
43+<data noupdate="1">
44+
45+ <record model="ir.rule" id="delivery_carrier_rule">
46+ <field name="name">delivery_carrier multi-company</field>
47+ <field name="model_id" search="[('model','=','delivery.carrier')]" model="ir.model"/>
48+ <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>
49+ </record>
50+
51+</data>
52+</openerp>