openerp-community team mailing list archive

["Stefan Rijnhart \(Therp\)" <stefan@xxxxxxxx>]

Review: Needs Fixing

My review is a bit compact but feel free to discuss anything that you disagree about.

l. 128 e.a. Please replace the whole stringified ID list thing by a regular many2many field. Adapt onchange_model to return just a list of ids. Replace the eval'ed string clause in the domain by model_ids[0][2], because you will find the representation of the many2many field in the regular [(6, 0, [1,2,3])] notation.

l.131 In the onchange method, clear the list of selected fields if the model really changes (or make the model unchangeable after saving for the first time), just to prevent mixing fields from several unrelated models.

l. 150: do not loop but deal with first ID. Line 171 is not robust against handling multiple resources anyway (or put the write in the loop of course!)

l. 213 spelling error 'refenrence' -> 'reference'
l. 215 spelling error 'Advance' -> 'Advanced'

l. 227 Please allow only read access to every users and CRUD only to an administrator group

l. 368 and all other occurences: take this line out of the loop, call fields_get only once with a list of all relevant fields as its argument as it gets called in all code paths inside the loop and sometimes even twice (l.389/391)

l. 369 and other assignments of all_fields[field.name]: you can probably put this as the first line of the loop and remove all its variants in the code paths below. These different forms of this line vary between things like 'field.field_description' (which provides the English field title) and 'field_info[field.name]['string']' (which provides the field title in the context lang). I do not see why it cannot be a reference to field_info[field.name] in every case (although you may find out trying ;-)

l. 370 and further: the use of the prefix 'selection_' can lead to a namespace clash relatively easily. Use one or two leading underscores to avoid that. Maybe use a constant in case it needs changing again.

l. 377 remove this condition if it looks as random to you as it does to me

l. 350 This module actually displays the Serpent company logo in every wizard. As we agreed for community projects that module names should not contain company names, I feel that this should not be accepted either, or I want my logo in their too given the time that I spent reviewing this module by now.

l. 431 Dataloss issue with many2many fields. You think you select a single item for removal but you clear the whole field. The 7.0 version of this branch seems to have fixed this one at least.

All the regular style conventions and deprecated API stuff applies but I think the above is more important.

-- 
https://code.launchpad.net/~openerp-community/server-env-tools/6.1-mass_editing/+merge/161619
Your team OpenERP Community is subscribed to branch lp:~openerp-community/server-env-tools/6.1-mass_editing.