Review: Needs Fixing
Javascript
* Why are `template` and `dialog_title` set on the instance instead of just
being set on the class?
* Why set `this.parent` in the overridden `init` when parent classes will set
it on `this.widget_parent`? Also why set `this.dataset` when it's never used?
In fact, why override `init` at all?
* In start, why call `this._super` with the argument `false`? As far as I can
tell, the overridden `start` (from `openerp.web.Dialog`) does not take *any*
argument...
* See ids issue from markup
* All selectors should be rooted in the widget/view's root
(this.$element.find), raw `$` calls should *only* be used to wrap nodes fetched
from elsewhere (received in event handlers for instance). This is sometimes
done correctly (e.g. in `start()` or `on_autodetect_data()`) and sometimes done
completely incorrectly (e.g. in `on_import_results()`)
* Underscore's methods should be used for array and object utils (e.g.
`_.contains` instead of `$.inArray`)
* In code like (data_import.js:98):
var sel_fields =[];
$("td #sel_field option:selected").each(function(){
sel_fields.push($(this).val());
});
why not use `.map`, and avoid explicitly pushing into a pre-declared array?
(the case below is even more flagrant, why use `_.each`, a test and an explicit
push into an array instead of `_.filter` and the correct predicate?)
* data_import:71 what's going to happen if the parent is not a listview (e.g.
the import popup has been invoked from the form view)?
--
https://code.launchpad.net/~openerp-dev/openerp-web/trunk-import-kch/+merge/74216
Your team OpenERP R&D Team is subscribed to branch
lp:~openerp-dev/openerp-web/trunk-import-kch.
_______________________________________________
Mailing list: https://launchpad.net/~openerp-dev-gtk
Post to : [email protected]
Unsubscribe : https://launchpad.net/~openerp-dev-gtk
More help : https://help.launchpad.net/ListHelp
