← Back to team overview

openerp-dev-web team mailing list archive

Re: [Merge] lp:~vda-tiny/openobject-client-web/drag-drop_groupby into lp:~openerp-dev/openobject-client-web/trunk-dev-web

 

> Technical:
> 
> * I think this branch could have been in more than a single commit (for
> instance moving the import in listgrid.mako should have been its own commit).
> Doesn't matter for now, but remember for future if possible.
> * attributes spacing (PEP8) e.g. listgroup.mako, line 125, there should be
> spaces around `!=`. Likewise line 150, there should be a space after the `,`
> in the itertools.chain call, or line 37 in the `map` call.
> * listgrid.js, in dragRow, `var args` should be moved down a few lines and
> created entirely via literal syntax:
> 
>     var args = {
>         '_terp_model': _terp_model.value,
>         '_terp_ids': _terp_ids.value,
>         '_terp_id': getNodeAttribute(drag, 'record'),
>         '_terp_swap_id': getNodeAttribute(drop, 'record'),
>     };
> 

ok, i will do it.

> * listgrid.py lines 248+
>     - return should be `{}` (simpler, faster than `dict`)
>     - don't use **kw when you know all arguments passed to method, clearer to
> specify keyword arguments:
> 
>         def groupbyDrag(self, **kw):
>             domain = eval(kw.get('domain'))[0]
>             model = kw.get('model')
>             children = eval(kw.get('children'))
>         =>
>         def groupbyDrag(self, model, domain, children):
>     - what happens if kw['model'] is empty? Or domain, or children? No need to
> use .get if it's going to blow up anyway, just do direct getitem (kw['model'])
> to get error as clear as possible (e.g. KeyError, not SyntaxError or
> TypeError)
>     - we're just evaluating literal Python data types (list, strings,
> integers, …) => please use ast.literal_eval
> (http://docs.python.org/library/ast.html#ast.literal_eval) not eval as eval is
> more risky (if client performs direct json call to method using eval, can
> create damage or even control server, not good, with literal_eval no chance to
> do that as it only accepts lists, dicts, strings, numbers, booleans and None,
> no risk of unrestricted object access or anything)
> 
Nice.
> UI/UX:
> * so it's only to move tasks from one groupby to another?
> * page reloads and all groups redisplay closed (except the one that
> disappeared when dragging a group on another one), very bad experience
> * not very fast
> * it's very weird that moving a groupby onto another groupby merges them, is
> that really what's supposed to happen? Is there spec somewhere?

we are following gtk. so it will work like gtk.

-- 
https://code.launchpad.net/~vda-tiny/openobject-client-web/drag-drop_groupby/+merge/22799
Your team OpenERP SA's Web Client R&D is subscribed to branch lp:~openerp-dev/openobject-client-web/trunk-dev-web.