search

tinyerp / erppeek

A versatile tool for Odoo / OpenERP. *** Forked as Odooly ⟶
https://github.com/tinyerp/odooly
Other
171 stars 99 forks source link

ambiguous Model.browse([]) #42

Closed lepistone closed 10 years ago

lepistone commented 10 years ago

Hi,

the Model.browse() method accepts an integer, a list or a domain. I realized that when passing a list, there is the risk that an empty list is considered as an empty domain and finds all records instead of none.

This looks just a bit confusing because of the different style between openerp and erppeek: the openerp idiom

ids = model.search(domain)
records = model.browse(ids)

should not be translated as is to erppeek because if nothing is found, we get all records.

Instead, in erppeek it could just be written:

records = model.browse(domain)

IMO this issue can be addressed just with an extra remark in the doc.

Thanks!

matrixise commented 10 years ago

good catch, and in this case, it's problematic because erppeek does not match the browse method of the ORM.

nbessi commented 10 years ago

Empty list is indeed considered as an empty domain.

With usage it become quite natural but it would be nice to simply having browse without any parameter to browse all and empty list will return empty record list. The problem is that altering this behaviour will break a lot of code.

Named parameters can be a solution too.

My two cents

Nicolas

florentx commented 10 years ago

Actually, ERPpeek browse() was designed as the search_read method of the json API in v7.

Now it matches the behavior of the search() method of the new Odoo API.

I'll add some lines from @lepistone in the documentation to insist on this point ... My own recommendation is to replace all usages of search with browse when using ERPpeek. It makes things simpler, and it gives no surprise.