RSS Feeds

Registered by Stephane Wirtel (OpenERP) on 2009-01-23

Guys,

recently you changed the sale_order.product_id_change method signature to add the fiscal_position field.
This is probably perfectly legitimate.

However, what you should realize is that you can't just commit such an API change without a rigorous review neither a publicly communicating a warning (at least to partners). I insist, you should really have some RSS or mailing list to communicate about such critical API changes if you want to keep in touch with a large and motivated community.

Indeed, a bunch of modules use to subclass sale_order.product_id_change or override the callback declaration in their xml views.
While you updated some of them, there are a lot of other modules that still don't match the new signature.
It means that if you install one of those modules, then you are caught, the fiscal position will never be passed to your core sale_order.product_id_change, False will be passed indeed, so installing one of those innocent modules will simply break your accounting, not very nice...

I think you should ensure ALL module that override the sale_order.product_id_change method OR override the callback definition in the view are properly migrated, both in addons and extra-addons. The minimal change IMHO is to simply append the fiscal.position to the callback definition in those module AND in the pyhton method, PROPAGATING it to the super.product_id_change call, just like the other arguments.

A simple grep gives me the occurences that need review:

A) In addons:
  In views:

rvalyi@rvalyi-laptop:~/DEV/openobject_trunk$ grep -r "product_id_change" openobject-addons | grep xml | grep -v position
openobject-addons/account_tax_include/invoice_tax_incl.xml: <field name="product_id" on_change="product_id_change(product_id, uos_id, quantity, name, parent.type, parent.partner_id, price_unit, parent.address_invoice_id, parent.price_type)"/>
openobject-addons/mrp/mrp_view.xml: <field name="product_id" on_change="product_id_change(product_id)" select="1"/>
openobject-addons/product/pricelist_view.xml: <field name="product_id" on_change="product_id_change(product_tmpl_id)" select="1"/>
openobject-addons/fleet_maintenance/sale_view.xml: on_change="product_id_change(parent.pricelist_id,product_id,product_uom_qty,product_uom,product_uos_qty,product_uos,name,parent.partner_id, 'lang' in context and context['lang'], False, parent.date_order, product_packaging, is_maintenance, maintenance_product_qty, maintenance_month_qty)"
openobject-addons/mrp_repair/mrp_repair_view.xml: <field name="product_id" on_change="product_id_change(parent.pricelist_id,product_id,product_uom,product_uom_qty, parent.partner_id)" colspan="4"/>
openobject-addons/mrp_repair/mrp_repair_view.xml: <field name="product_id" on_change="product_id_change(parent.pricelist_id,product_id,product_uom,product_uom_qty, parent.partner_id)"/>
openobject-addons/mrp_repair/mrp_repair_view.xml: <field name="product_id" on_change="product_id_change(parent.pricelist_id,product_id,product_uom,product_uom_qty, parent.partner_id,parent.guarantee_limit)" colspan="4"/>
openobject-addons/mrp_repair/mrp_repair_view.xml: <field name="product_id" on_change="product_id_change(parent.pricelist_id,product_id,product_uom,product_uom_qty, parent.partner_id,parent.guarantee_limit)"/>
openobject-addons/stock_booking/sale_view.xml: on_change="product_id_change(parent.pricelist_id,product_id,product_uom_qty,product_uom,product_uos_qty,product_uos,name,parent.partner_id, 'lang' in context and context['lang'], True, parent.date_order, product_packaging)"

  In the code:

rvalyi@rvalyi-laptop:~/DEV/openobject_trunk$ grep -r "def product_id_change" openobject-addons | grep -v position
openobject-addons/account/invoice.py: def product_id_change_unit_price_inv(self, cr, uid, tax_id, price_unit, qty, address_invoice_id, product, partner_id, context=None):
openobject-addons/account_analytic_default/account_analytic_default.py: def product_id_change(self, cr, uid, ids, product, uom, qty=0, name='', type='out_invoice', partner_id=False, price_unit=False, address_invoice_id=False, context={}):
openobject-addons/account_analytic_plans/account_analytic_plans.py: def product_id_change(self, cr, uid, ids, product, uom, qty=0, name='', type='out_invoice', partner_id=False, price_unit=False, address_invoice_id=False, context={}):
openobject-addons/account_tax_include/invoice_tax_incl.py: def product_id_change_unit_price_inv(self, cr, uid, tax_id, price_unit, qty, address_invoice_id, product, partner_id, context=None):
openobject-addons/account_tax_include/invoice_tax_incl.py: def product_id_change(self, cr, uid, ids, product, uom, qty=0, name='', type='out_invoice', partner_id=False, price_unit=False, address_invoice_id=False, price_type='tax_excluded', context=None):
openobject-addons/mrp/mrp.py: def product_id_change(self, cr, uid, ids, product):
openobject-addons/product/pricelist.py: def product_id_change(self, cr, uid, ids, product_id, context={}):
openobject-addons/purchase/purchase.py: def product_id_change(self, cr, uid, ids, pricelist, product, qty, uom,
openobject-addons/sale/sale.py: def product_id_change(self, cr, uid, ids, pricelist, product, qty=0,
openobject-addons/warning/warning.py: def product_id_change(self, cr, uid, ids, pricelist, product, qty=0,
openobject-addons/warning/warning.py: def product_id_change(self,cr, uid, ids, pricelist, product, qty, uom,
openobject-addons/anevia_sale/sale.py.bak: def product_id_change(self, cr, uid, ids, pricelist, product, qty=0,
openobject-addons/fleet_maintenance/sale.py: def product_id_change(self, cr, uid, ids, pricelist, product, qty=0,
openobject-addons/mrp_repair/mrp_repair.py: def product_id_change(self, cr, uid, ids, pricelist, product, uom=False, product_uom_qty=0, partner_id=False, guarantee_limit=False):
openobject-addons/sale_supplier_direct_delivery/sale.py: def product_id_change(self, cr, uid, ids, pricelist, product, qty=0,
openobject-addons/product_visible_discount/product.py: def product_id_change(self, cr, uid, ids, pricelist, product, qty=0,
openobject-addons/product_visible_discount/product.py: def product_id_change(self, cr, uid, ids, product, uom, qty=0, name='', type='out_invoice', partner_id=False, price_unit=False, address_invoice_id=False, context={}):

B) Trunk extra-addons
  In the views

rvalyi@rvalyi-laptop:~/DEV/openobject_trunk$ grep -r "product_id_change" trunk-extra-addons | grep xml | grep -v position
trunk-extra-addons/Ampco/ampco_view.xml: <field name="product_uom_qty" on_change="product_id_change(parent.pricelist_id,product_id,product_uom_qty,product_uom,th_weight,product_uos_qty,product_uos,name,parent.partner_id,'lang' in context and context['lang'], False)" context="partner_id=parent.partner_id,quantity=product_uom_qty,pricelist=parent.pricelist_id,shop=parent.shop_id,uom=product_uom" select="1"/>
trunk-extra-addons/Ampco/ampco_view.xml: <field name="product_id" on_change="product_id_change(parent.pricelist_id,product_id,product_uom_qty,product_uom,product_uos_qty,th_weight,product_uos,name,parent.partner_id,'lang' in context and context['lang'], False)" context="partner_id=parent.partner_id,quantity=product_uom_qty,pricelist=parent.pricelist_id,shop=parent.shop_id,uom=product_uom" select="1"/>
trunk-extra-addons/bookstore/bookstore_view.xml:<!-- <field name="product_uom_qty" on_change="product_id_change(parent.pricelist_id,product_id,product_uom_qty,product_uom,product_uos_qty,product_uos,name,parent.partner_id)" context="partner_id=parent.partner_id,quantity=product_uom_qty,pricelist=parent.pricelist_id,shop=parent.shop_id,uom=product_uom" select="1"/> -->
trunk-extra-addons/bookstore/bookstore_view.xml:<!-- <field name="product_id" on_change="product_id_change(parent.pricelist_id,product_id,product_uom_qty,product_uom,product_uos_qty,product_uos,name,parent.partner_id)" context="partner_id=parent.partner_id,quantity=product_uom_qty,pricelist=parent.pricelist_id,shop=parent.shop_id,uom=product_uom" colspan="3" select="1"/> -->
trunk-extra-addons/cci_mission/cci_mission_view.xml: <field name="product_id" select="1" on_change="product_id_change(product_id)"/>
trunk-extra-addons/cci_mission/cci_mission_view.xml: <field name="product_id" required="1" select="1" on_change="product_id_change(product_id)"/>
trunk-extra-addons/cci_mission/cci_mission_view.xml: <field name="product_id" required="1" select="1" on_change="product_id_change(product_id)"/>
trunk-extra-addons/hotel/hotel_view.xml: on_change="product_id_change(parent.pricelist_id,product_id,product_uom_qty,product_uom,product_uos_qty,product_uos,name,parent.partner_id, 'lang' in context and context['lang'], False, parent.date_order)"
trunk-extra-addons/hotel/hotel_view.xml: on_change="product_id_change(parent.pricelist_id,product_id,product_uom_qty,product_uom,product_uos_qty,product_uos,name,parent.partner_id, 'lang' in context and context['lang'], True, parent.date_order)"
trunk-extra-addons/hotel/hotel_view.xml: on_change="product_id_change(parent.pricelist_id,product_id,product_uom_qty,product_uom,product_uos_qty,product_uos,name,parent.partner_id, 'lang' in context and context['lang'], False, parent.date_order)"
trunk-extra-addons/hotel/hotel_view.xml: on_change="product_id_change(parent.pricelist_id,product_id,product_uom_qty,product_uom,product_uos_qty,product_uos,name,parent.partner_id, 'lang' in context and context['lang'], True, parent.date_order)"
trunk-extra-addons/product_lot_foundry/sale_order_view.xml: <field name="product_id" on_change="product_id_change(parent.pricelist_id,product_id,product_uom_qty,product_uom,product_uos_qty,product_uos,name,parent.partner_id, 'lang' in context and context['lang'], True)" context="partner_id=parent.partner_id,quantity=product_uom_qty,pricelist=parent.pricelist_id,shop=parent.shop_id,uom=product_uom" colspan="4" select="1"/>
trunk-extra-addons/product_lot_foundry/sale_order_view.xml: <field name="product_uom_qty" on_change="product_id_change(parent.pricelist_id,product_id,product_uom_qty,product_uom,product_uos_qty,product_uos,name,parent.partner_id, 'lang' in context and context['lang'], False)" context="partner_id=parent.partner_id,quantity=product_uom_qty,pricelist=parent.pricelist_id,shop=parent.shop_id,uom=product_uom" select="1"/>
trunk-extra-addons/sale_delivery/sale_delivery_view.xml: on_change="product_id_change(product_id,product_qty,product_uom,packaging_id)"
trunk-extra-addons/sale_delivery/sale_delivery_view.xml: on_change="product_id_change(product_id,product_qty,product_uom,packaging_id)"
trunk-extra-addons/sale_delivery/sale_delivery_view.xml: on_change="product_id_change(product_id,product_qty,product_uom,packaging_id)"
trunk-extra-addons/sale_delivery/sale_delivery_view.xml: on_change="product_id_change(product_id,product_qty,product_uom,packaging_id)"
trunk-extra-addons/sale_margin_delivery/sale_margin_delivery_view.xml: on_change="product_id_change(product_id,product_qty,product_uom,packaging_id)"
trunk-extra-addons/sale_margin_delivery/sale_margin_delivery_view.xml: on_change="product_id_change(product_id,product_qty,product_uom,packaging_id)"
trunk-extra-addons/sale_margin_delivery/sale_margin_delivery_view.xml: on_change="product_id_change(product_id,product_qty,product_uom,packaging_id)"
trunk-extra-addons/sale_margin_delivery/sale_margin_delivery_view.xml: on_change="product_id_change(product_id,product_qty,product_uom,packaging_id)"
trunk-extra-addons/stock_planning/stock_planning_view.xml: <field name="product_id" on_change="product_id_change(product_id,product_uom,product_qty,product_amt)" />
trunk-extra-addons/stock_planning/stock_planning_view.xml: <field name="product_qty" on_change="product_id_change(product_id,product_uom,product_qty,product_amt)" />
trunk-extra-addons/stock_planning/stock_planning_view.xml: <field name="product_amt" on_change="product_id_change(product_id,product_uom,product_qty,product_amt)" />
trunk-extra-addons/stock_planning/stock_planning_view.xml: <field name="product_uom" on_change="product_id_change(product_id,product_uom,product_qty,product_amt)" />
trunk-extra-addons/stock_planning/stock_planning_view.xml: <field name="product_id" select="1" on_change="product_id_change(product_id,product_uom)" />
trunk-extra-addons/stock_planning/stock_planning_view.xml: <field name="product_uom" on_change="product_id_change(product_id,product_uom)" />
trunk-extra-addons/stock_planning/stock_planning_view.xml: <field name="product_id" select="1" on_change="product_id_change(product_id,product_uom)" />
trunk-extra-addons/stock_planning/stock_planning_view.xml: <field name="product_uom" on_change="product_id_change(product_id,product_uom)" />
trunk-extra-addons/stock_planning/stock_planning_view.xml: <field name="product_id" select="1" on_change="product_id_change(product_id,product_uom)" />
trunk-extra-addons/stock_planning/stock_planning_view.xml: <field name="product_uom" on_change="product_id_change(product_id,product_uom)" />
trunk-extra-addons/fleet_maintenance/sale_view.xml: on_change="product_id_change(parent.pricelist_id,product_id,product_uom_qty,product_uom,product_uos_qty,product_uos,name,parent.partner_id, 'lang' in context and context['lang'], False, parent.date_order, product_packaging, is_maintenance, maintenance_product_qty, maintenance_month_qty)"
trunk-extra-addons/sale_layout/sale_layout_view.xml: on_change="product_id_change(parent.pricelist_id,product_id,product_uom_qty,product_uom,product_uos_qty,product_uos,name,parent.partner_id, 'lang' in context and context['lang'], True, parent.date_order, product_packaging)"
trunk-extra-addons/sale_layout/sale_layout_view.xml: on_change="product_id_change(parent.pricelist_id,product_id,product_uom_qty,product_uom,product_uos_qty,product_uos,name,parent.partner_id, 'lang' in context and context['lang'], False, parent.date_order, product_packaging)"
trunk-extra-addons/sale_layout/sale_layout_view.xml: on_change="product_id_change(parent.pricelist_id,product_id,product_uom_qty,product_uom,product_uos_qty,product_uos,name,parent.partner_id, 'lang' in context and context['lang'], False, parent.date_order, product_packaging)"

  In the code

rvalyi@rvalyi-laptop:~/DEV/openobject_trunk$ grep -r "def product_id_change" trunk-extra-addons | grep -v position | grep -v '~'
trunk-extra-addons/Ampco/ampco.py: def product_id_change(self, cr, uid, ids, pricelist, product, qty=0,
trunk-extra-addons/bookstore/sale.py: def product_id_change(self, cr, uid, ids, pricelist, product, qty=0,
trunk-extra-addons/cci_mission/cci_mission.py: def product_id_change(self, cr, uid, ids,product_id,):
trunk-extra-addons/hotel/hotel.py: def product_id_change(self, cr, uid, ids, pricelist, product, qty=0,
trunk-extra-addons/hotel/hotel.py: def product_id_change(self, cr, uid, ids, pricelist, product, qty=0,
trunk-extra-addons/product_lot_foundry/sale_order.py: def product_id_change(self, cr, uid, ids, pricelist, product, qty=0,
trunk-extra-addons/product_visible_discount/product.py: def product_id_change(self, cr, uid, ids, pricelist, product, qty=0,
trunk-extra-addons/product_visible_discount/product.py: def product_id_change(self, cr, uid, ids, product, uom, qty=0, name='', type='out_invoice', partner_id=False, price_unit=False, address_invoice_id=False, context={}):
trunk-extra-addons/sale_delivery/sale_delivery.py: def product_id_change(self, cr, uid, ids, product, qty=0, uom=False, packaging=False):
trunk-extra-addons/stock_planning/stock_planning.py: def product_id_change(self, cr, uid, ids, product, uom=False, product_qty = 0, product_amt = 0.0):
trunk-extra-addons/stock_planning/stock_planning.py: def product_id_change(self, cr, uid, ids, product, uom=False):
trunk-extra-addons/fleet_maintenance/sale.py: def product_id_change(self, cr, uid, ids, pricelist, product, qty=0,
trunk-extra-addons/sale_supplier_direct_delivery/sale.py: def product_id_change(self, cr, uid, ids, pricelist, product, qty=0,

Hope this helps, and please I hope you adopt a more responsible API break policicy in the future to avoid big troubles. Breaking an API might be required, but review and communication is needed. Thanks,

Raphaƫl Valyi.

Blueprint information

Status:
Not started
Approver:
None
Priority:
Undefined
Drafter:
None
Direction:
Needs approval
Assignee:
None
Definition:
New
Series goal:
None
Implementation:
Unknown
Milestone target:
None

Related branches

Sprints

Whiteboard

(?)

Work Items

This blueprint contains Public information 
Everyone can see this information.

Subscribers

No subscribers.