price_multi_get() is comparing pricelist_ids to pricelist_version_ids

Bug #693056 reported by snook
12
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Odoo Addons (MOVED TO GITHUB)
Fix Released
Low
OpenERP R&D Addons Team 2

Bug Description

Version 6 trunk
server revno: 3137
addons revno: 4033

I have posted a question a few days ago:
https://answers.launchpad.net/openobject-addons/+question/138016 (no response yet)

I think this is a real bug and deserves to be posted here.

product/pricelist.py appears to have regressed at version 3719 with the introduction of "price_multi_get()"

Prior to this version, the pricelist_ids argument to price_get() was translated to pricelist_version_ids.

In revno:3566 where "ids" are priclist_ids passed on "price_get()":

153 for id in ids:
154 cr.execute('SELECT * ' \
155 'FROM product_pricelist_version ' \
156 'WHERE pricelist_id = %s AND active=True ' \
157 'AND (date_start IS NULL OR date_start <= %s) ' \
158 'AND (date_end IS NULL OR date_end >= %s) ' \
159 'ORDER BY id LIMIT 1', (id, date, date))
160 plversion = cr.dictfetchone()
......
190 cr.execute(
191 'SELECT i.*, pl.currency_id '
192 'FROM product_pricelist_item AS i, '
193 'product_pricelist_version AS v, product_pricelist AS pl '
194 'WHERE (product_tmpl_id IS NULL OR product_tmpl_id = %s) '
195 'AND (product_id IS NULL OR product_id = %s) '
196 'AND (' + categ_where + ' OR (categ_id IS NULL)) '
197 'AND price_version_id = %s '
198 'AND (min_quantity IS NULL OR min_quantity <= %s) '
199 'AND i.price_version_id = v.id AND v.pricelist_id = pl.id '
200 'ORDER BY sequence',
201 (tmpl_id, prod_id, plversion['id'], qty))
202 res1 = cr.dictfetchall()

Above code looks okay:
1. ids are translated to pricelist versions in "plversion"
2. line 197 matches price_version_id to plversion['id']

Current version revno:4033 (since revno:3719):

167 if pricelist_ids:
168 pricelist_version_ids = pricelist_ids
169 else:
170 # all pricelists:
171 pricelist_version_ids = product_pricelist_version_obj.search(cr, uid, [])

**************************
Above code stores "pricelist_ids" and "product_pricelist_version_ids" in the same variable "pricelist_version_ids" ????
**************************

172
173 pricelist_version_ids = list(set(pricelist_version_ids))
174
175 plversions_search_args = [
176 ('pricelist_id', 'in', pricelist_version_ids),
177 '|',
178 ('date_start', '=', False),
179 ('date_start', '<=', date),
180 '|',
181 ('date_end', '=', False),
182 ('date_end', '>=', date),
183 ]
184
185 plversion_ids = product_pricelist_version_obj.search(cr, uid, plversions_search_args)

****************************
line 176 is matching "pricelist_id" to pricelist_version_ids (that may be pricelist_ids ?????)
****************************

202 for product_id, qty, partner in products_by_qty_by_partner:
203 for pricelist_id in pricelist_version_ids:
204 price = False
205
206 tmpl_id = products_dict[product_id].product_tmpl_id and products_dict[product_id].product_tmpl_id.id or False
207
208 categ_id = products_dict[product_id].categ_id and products_dict[product_id].categ_id.id or False
209 categ_ids = _create_parent_category_list(categ_id, [categ_id])
210 if categ_ids:
211 categ_where = '(categ_id IN (' + ','.join(map(str, categ_ids)) + '))'
212 else:
213 categ_where = '(categ_id IS NULL)'
214
215 cr.execute(
216 'SELECT i.*, pl.currency_id '
217 'FROM product_pricelist_item AS i, '
218 'product_pricelist_version AS v, product_pricelist AS pl '
219 'WHERE (product_tmpl_id IS NULL OR product_tmpl_id = %s) '
220 'AND (product_id IS NULL OR product_id = %s) '
221 'AND (' + categ_where + ' OR (categ_id IS NULL)) '
222 'AND price_version_id = %s '
223 'AND (min_quantity IS NULL OR min_quantity <= %s) '
224 'AND i.price_version_id = v.id AND v.pricelist_id = pl.id '
225 'ORDER BY sequence',
226 (tmpl_id, product_id, pricelist_id, qty))
227 res1 = cr.dictfetchall()

******************************
line 222 is matching "price_version_id" against "pricelist_id" from line 203

"pricelist_id" is derived from "pricelist_version_ids" which may contain pricelist_ids or pricelist_version_ids
******************************

My question at https://answers.launchpad.net/openobject-addons/+question/138016
was based on data that resulted in the above sql trying to match object ids of two different object (priclist_ids and pricelist_version_ids)

To me it looks like the code is broken...but my head hurts looking at it.

Related branches

Revision history for this message
DBR (OpenERP) (dbr-openerp) wrote :

Hello Snook,

Would you please elaborate more with proper examples because I did not get the exact idea from piece of code which you have specified in the bug specification.

Thanks.

Changed in openobject-addons:
status: New → Incomplete
Revision history for this message
snook (snook) wrote :

Hi DBR,

Problem becomes apparent when a "pricelist" is linked to a "pricelist_version" and they have different "id" numbers.

Have a look at details posted here: https://answers.launchpad.net/openobject-addons/+question/138016

There is some data and you can see in the generated sql the line:

            "AND price_version_id = 4"

It is using a value of "4" which is from a "pricelist_id" NOT a "pricelist_version".

Hope that helps.

Changed in openobject-addons:
assignee: nobody → OpenERP R&D Addons Team 2 (openerp-dev-addons2)
importance: Undecided → Low
status: Incomplete → Confirmed
Revision history for this message
Rohan Nayani(Open ERP) (ron-tinyerp) wrote :

Hello snook,

Thanks for reporting.
It has been fixed in lp:~openerp-dev/openobject-addons/ron-dev-addons2
Revision ID: <email address hidden>
Revision num: 5103.

It will be available in trunk soon,

Changed in openobject-addons:
status: Confirmed → Fix Committed
Revision history for this message
snook (snook) wrote :

Hi Ron,

Had a quick look at the new code and looks like part of the problem is solved.

However, code below does not seem correct to me. Perhaps better variable naming would not mask the problem:

166 # product.pricelist.version:
167 if pricelist_ids:
168 pricelist_version_ids = pricelist_ids
169 else:
170 # all pricelists:
171 pricelist_version_ids = product_pricelist_version_obj.search(cr, uid, [])
172
173 pricelist_version_ids = list(set(pricelist_version_ids))
174
175 plversions_search_args = [
176 ('pricelist_id', 'in', pricelist_version_ids),
177 '|',
178 ('date_start', '=', False),
179 ('date_start', '<=', date),
180 '|',
181 ('date_end', '=', False),
182 ('date_end', '>=', date),
183 ]
184
185 plversion_ids = product_pricelist_version_obj.search(cr, uid, plversions_search_args)

What does not look correct to be is:

1. Line 168 is placing "pricelist_ids" into a variable called "pricelist_version_ids"
    and line: 171 is placing "pricelist_version_ids" into same variable (variable may contain "ids" of two different objects)

2. Line 176 is matching "pricelist_ids" against "pricelist_version_ids" if line: 171 is executed

Code prior to "price_get_multi()" converted incoming pricelist_ids into associated pricelist_version_ids before continuing.

That seems to be the problem here.

The argument "pricelist_ids" are not converted to related "pricelist_version_ids"

regards

Revision history for this message
Robin (www.cgbs.com.au) (robin-cgbs) wrote :

Hi Ron,

I also appreciate the work being done here.

I am not sure though that the key problem is being addressed:

These lines are the issue and will work in most cases:

167 if pricelist_ids:
168 pricelist_version_ids = pricelist_ids

What these lines of code are doing is placing pricelist ids into the pricelist_VERSION_ids list.

This works whilst pricelist_id matches the pricelist_VERSION_id.

Start deleting some pricelist versions and creating new ones, and get a data set whereby the id's between the pricelist and pricelist versions do not match.

That is where things start to blow up.

Revision history for this message
Rohan Nayani(Open ERP) (ron-tinyerp) wrote :

Hello snook,
Thanks for Reporting.
The problem of wrong reference is fixed in lp:~openerp-dev/openobject-addons/ron-dev-addons2
Revision ID: <email address hidden>
Revision num: 5105.

Changed in openobject-addons:
status: Fix Committed → Fix Released
Changed in openobject-addons:
milestone: none → 6.0
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Related questions

Remote bug watches

Bug watches keep track of this bug in other bug trackers.