openerp-dev-web team mailing list archive

["Xavier \(Open ERP\)" <xmo@xxxxxxxxxxx>]

Review: Needs Fixing
Tech review from reading code (I'll try it out a bit later):

* In the JS, there are a few instances of `[id={some_value}]`, those should be ID selectors `#{some_value}`

* in addons/openerp/controllers/listgrid.py @3080, you added `from openerp import widgets as tw` in the `multiple_groupby` function,why not just add `listgroup` to the existing `from openerp.widgets import listgrid` in the toplevel imports?

* In the group_by js method (addons/openerp/static/javascript/listgrid.js), code would probably be simplified by creating an intermediate variable for `jQuery('[records="'+record+'"]')` instead of re-fetching it every time.

* Also, as far as I can tell in the second contional (`if(jQuery('[records="'+record+'"]').attr('parent'))`) both branches are exactly the same, am I wrong? Is this a mistake in the branches or is it just that the test isn't actually useful?

* In addons/openerp/static/javascript/search.js, you commended a few lines, I think they should just be removed.

* In addons/openerp/widgets/listgroup.py method parse, after the `for hidden in hiddens:` you check hidden[0] and hidden[1]. Is hidden a pair (2-tuple)? If it is, why not unpack it directly?

* Also a few lines below, there is a `if padding is None` test, but both branches seem to do the same thing. Same in the next group of iterations

* In parse_groups,

    if len(group_by)  > 1:
        child = False
    else:
        child = True

  could be replaced by
    child = len(group_by) > 1

  and I'm not quite sure about the name, what does the child flag mean?

* At the end of the method, is `rec` a `dict` or something else (e.g. `TinyDict`)? If `dict`, why not use `.update` method to set all three keys at the same time?

* And big loop above could probably be a list comprehension, something along the lines of:

    if child:
        ch_ids = [d for id in proxy.search(dom, offset, limit, 0, context)
                    for  d in data
                    if d.get('id') == id]
    else:
        ch_ids = []

  no?
-- 
https://code.launchpad.net/~openerp-dev-web/openobject-client-web/group_by/+merge/25140
Your team OpenERP SA's Web Client R&D is subscribed to branch lp:~openerp-dev/openobject-client-web/trunk-dev-web.