Merge lp:~pedro.baeza/ocb-addons/7.0-bug-1253701 into lp:ocb-addons

[Pedro Manuel Baeza]

Fix for the bug 1253701. All the explanations are in the bug description.

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

LGTM

review: Approve

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

Hi Pedro,

thank you for this fix! Changing the rule seems like a good idea. Seems to me that the limits to multi-company BOM are any company-specific routes.

However, putting this incidental security rule in an updating data block would be a deviation from the OpenERP standard to have security rules in noupdate so that they can be changed by users and consultants. I don't think it is a good idea to introduce such inconsistency.

review: Needs Fixing

[Pedro Manuel Baeza (pedro.baeza)]

Yeah, I balance the two options, but decide it to put it in this way to allow all the current v7 user base to have this rule updated. For trunk, obviously it must go the other way.

Do you think we must keep it also for v7 in the noupdate=1 section?

Regards.

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

Yes, I do believe you should keep it in the noupdate section. I understand that this means that the behaviour is not adapted automatically, but that is a generic problem with fixes in 'noupdate' data and the use of 'noupdate' data is relatively undisputed AFAIK.

Your solution is problematic because it will overwrite the rule with every module update from now on. Now I ccan think of a technical solution to that. In a project that was under our own control, I would suggest something like the following: increase the module version number and create a migration script that checks the value of the existing rule, then overwrite the value of the existing rule if it was unmodified wrt. the previous version. Else, if the rule was customized, log a verbose message.

As we are torn between following official OpenERP on one side, and trying to fix what we think is broken on the other side I can't say if it would be a good idea to introduce such a policy for OCB to deal with changes in noupdate data. Note that it would be inconsistent if we would only do it for OCB specific changes, because fixes in upstream code in 'noupdate' data would not follow this policy. We would have to track such changes to inject the update mechanism and I don't think that is going to happen.

[Pedro Manuel Baeza (pedro.baeza)]

You are very right, and the technical solution sounds good, except for one thing: OpenERP 7.0 modules update mechanism checks version number that differs from the official one, both lower or higher values, so this change will warn user that there is an update for mrp module (rather it is a downgrade). I don't know how it's going to behave if you decide to update...

For now, I have put the rule on the noupdate=1 section.

Regards.

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

Thanks for your change! Have you any pointers to the alternate versioning mechanism? I thought it was simply the version key from __openerp__.py

review: Approve

[Pedro Manuel Baeza (pedro.baeza)]

Yeah, indeed it's only the version key from __openerp__.py, but it compares the same for lower or higher values. I discovered it working on a refactorisation to l10n_es module. Current version in the apps repository is 3.0, and I have changed the version to 4.0 (because it's a huge refactorisation). My surprise was that on Updates section, it appears l10n_es to update, although the version number was higher than the official one:

http://imageshack.com/i/13ihip

Regards.

[Holger Brunn (Therp) (hbrunn)]

review: Approve (code review)

=== modified file 'mrp/security/mrp_security.xml'
--- mrp/security/mrp_security.xml	2012-10-23 16:05:04 +0000
+++ mrp/security/mrp_security.xml	2013-11-23 13:27:24 +0000
@@ -44,7 +44,7 @@
         <field name="name">mrp_bom multi-company</field>
         <field name="model_id" search="[('model','=','mrp.bom')]" model="ir.model"/>
         <field name="global" eval="True"/>
-        <field name="domain_force">['|',('company_id','child_of',[user.company_id.id]),('company_id','=',False)]</field>
+        <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>
 
     <record model="ir.rule" id="mrp_routing_rule">