Merge lp:~openerp-dev/openobject-client-web/one2many-onchange into lp:openobject-client-web/trunk

Proposed by Sananaz (Open ERP)
Status: Merged
Merged at revision: 3947
Proposed branch: lp:~openerp-dev/openobject-client-web/one2many-onchange
Merge into: lp:openobject-client-web/trunk
Diff against target: 184 lines (+98/-14)
6 files modified
addons/openerp/controllers/listgrid.py (+31/-0)
addons/openerp/static/javascript/form.js (+50/-1)
addons/openerp/static/javascript/openerp/openerp.base.js (+1/-1)
addons/openerp/widgets/form/_form.py (+1/-1)
addons/openerp/widgets/listgrid.py (+1/-1)
addons/view_calendar/controllers/templates/calpopup.mako (+14/-10)
To merge this branch: bzr merge lp:~openerp-dev/openobject-client-web/one2many-onchange
Reviewer Review Type Date Requested Status
Xavier (Open ERP) (community) Approve
Sananaz (Open ERP) Pending
Review via email: mp+41437@code.launchpad.net

Description of the change

Implement onchange for one2many widget.
For testing refer Bug #671608.

To post a comment you must log in.
Revision history for this message
Navrang Oza (noz-tiny) wrote :

@XMO,

I have it, and its working fine. But still want you to check once.
Thanks.

Revision history for this message
Xavier (Open ERP) (xmo-deactivatedaccount) wrote :

Seems to fix the bug, so good.

=On the other hand, bugs:=
* Variable `class` generates a syntax error in Safari, and it's a reserved word in javascript anyway so it should not be used.

=Style/coding in JS code:=
* Is not needed as only used once, and `.attr('class') == 'classname'` is bad code, should use jQuery's `.hasClass` method in case e.g. there is more than one class on the element
* Please do not introduce new mochikit code, we're trying to get rid of it, use jQuery instead (e.g. getElement, getFirstParentByTagAndClassName, INPUT)
* Also why all those temporary variables at line 540? They're only used once, their name is not much clearer than they would be within map itself.
* Same with `request`, it's only used once in `addCallback` call, why not just chain `addCallback` with `postJSON`?
* And in any case, `addCallback` is a mochikit API (and so is `postJSON`, transitively), please use jQuery facilities instead in new code.
* Remember to terminate statements with `;` always (line 571), check IDE to see if can not display warning, should be able to.
* jQuery(fld) is called a lot, should probably be memoized to e.g. `$current_field`
* There is code which looks redundant:

    if (getElement('_terp_default_o2m/'+k) && !value) {
        if (getElement('_terp_default_o2m/'+k).value=='' && !value)
            continue;
        else if(getElement('_terp_default_o2m/'+k).value && !value) {
            getElement('_terp_default_o2m/'+k).value = '';
            new ListView(k).reload();
        }
    }

  First line already mandates `!value` test, why re-test it at both other cases? And if you remove it, tests at line 2 and line 4 are pretty much opposite of each other so should be able to do with only one test (and an `else` clause rather than `else if`) no?

= In listgrid.py =
* Why is simplejson imported within method (and therefore re-imported at each execution) rather than just at module toplevel?
* Why do you need copy.deepcopy?
* Please avoid using eval unless you have to. What is content of o2m_view_id? Why can't you use `literal_eval` or an explicit call constructor call?
* Same question for o2m_context and o2m_values (which is first loaded via simplejson and then eval'ed?)
* line 39, doesn't make sense to use `.get`:

    view = view['fields'].get(name).get('views').get(o2m_view_type)

  if `name` is not in `view['fields']`. `.get(name)` is going to return `None` which is going to raise an error when trying to call `.get('views')` on it. So why use `.get`, why not just `[name]`? Same question for the calls after that.

review: Needs Fixing
3924. By Sananaz (Open ERP)

[FIX] Fixed json code.

3925. By Sananaz (Open ERP)

[FIX] Removed ajaxStop to prevent recursive onchange (Call onchange for default values).

3926. By Sananaz (Open ERP)

[FIX] Change mochikit call to jQuery for default onchange on calendar date.

3927. By Sananaz (Open ERP)

[FIX] Fixed reviewed Code for o2m defaults.

Revision history for this message
Sananaz (Open ERP) (sma-tiny) wrote :

Hello XMO,

I have improved all changes suggested by you and related to other issues also.
I request you to please check it and merge with trunk branch.

Thank you.

3928. By Xavier (Open ERP)

[FIX] id selector for default_o2m, condition not checking for length on jQuery object, condition on val() content

3929. By Xavier (Open ERP)

[IMP] use jQuery.post instead of jQuery.ajax as we're not setting the error handlers

3930. By Xavier (Open ERP)

[IMP] id selectors (use escaping function instead of arbitrary attribute selector and string munging)

3931. By Xavier (Open ERP)

[IMP] use .val() instead of .attr('value')

3932. By Xavier (Open ERP)

[IMP] cleanup some jquery code

Revision history for this message
Xavier (Open ERP) (xmo-deactivatedaccount) wrote :

Works for me, sma could you check the changes I added, just to be sure i didn't break anything?

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'addons/openerp/controllers/listgrid.py'
--- addons/openerp/controllers/listgrid.py 2010-11-17 12:21:33 +0000
+++ addons/openerp/controllers/listgrid.py 2010-11-24 11:46:04 +0000
@@ -26,6 +26,9 @@
26# You can see the MPL licence at: http://www.mozilla.org/MPL/MPL-1.1.html26# You can see the MPL licence at: http://www.mozilla.org/MPL/MPL-1.1.html
27#27#
28###############################################################################28###############################################################################
29import copy
30import simplejson
31
29import cherrypy32import cherrypy
30from openerp.controllers import SecuredController33from openerp.controllers import SecuredController
31from openerp.utils import rpc, TinyDict, TinyForm, TinyFormError, context_with_concurrency_info, cache34from openerp.utils import rpc, TinyDict, TinyForm, TinyFormError, context_with_concurrency_info, cache
@@ -141,6 +144,34 @@
141 o2m_value.pop(int(index))144 o2m_value.pop(int(index))
142 return dict(o2m_value=ustr(o2m_value))145 return dict(o2m_value=ustr(o2m_value))
143146
147 @expose('json')
148 def get_o2m_defaults(self, o2m_values, model, o2m_model, name, view_type, view_id,
149 o2m_view_type, o2m_view_id, editable, limit, offset, o2m_context, o2m_domain):
150
151 view_id = view_id or False
152 o2m_view_id = ast.literal_eval(o2m_view_id) or False
153 o2m_view_type = o2m_view_type or 'form'
154 context = dict(ast.literal_eval(o2m_context), **rpc.session.context)
155 o2m_values = simplejson.loads(o2m_values)
156
157 for o2m in o2m_values:
158 o2m['id'] = 0
159
160 if o2m_view_id:
161 view = cache.fields_view_get(o2m_model, o2m_view_id, o2m_view_type, context)
162 else:
163 view = cache.fields_view_get(model, view_id, view_type, rpc.session.context)
164 view = view['fields'][name]['views'][o2m_view_type]
165
166 list_view = listgrid.List(name, model, view, ids=None, domain=o2m_domain, context=context, default_data=copy.deepcopy(o2m_values), limit=20, editable= editable,o2m=1)
167 view=ustr(list_view.render())
168 formated_o2m_values = []
169 for o2m in o2m_values:
170 o2m.pop('id', None)
171 formated_o2m_values.append((0, 0, o2m))
172
173 return dict(view=view, formated_o2m_values=ustr(formated_o2m_values))
174
144 @expose()175 @expose()
145 def multiple_groupby(self, model, name, grp_domain, group_by, view_id, view_type, parent_group, group_level, groups, no_leaf, **kw):176 def multiple_groupby(self, model, name, grp_domain, group_by, view_id, view_type, parent_group, group_level, groups, no_leaf, **kw):
146 grp_domain = ast.literal_eval(grp_domain)177 grp_domain = ast.literal_eval(grp_domain)
147178
=== modified file 'addons/openerp/static/javascript/form.js'
--- addons/openerp/static/javascript/form.js 2010-11-24 09:27:06 +0000
+++ addons/openerp/static/javascript/form.js 2010-11-24 11:46:04 +0000
@@ -522,8 +522,57 @@
522522
523 if ((fld.value !== value) || flag) {523 if ((fld.value !== value) || flag) {
524 fld.value = value;524 fld.value = value;
525 var $current_field = jQuery(fld);
526 var kind = $current_field.attr('kind');
525527
526 var kind = jQuery(fld).attr('kind');528 if (!kind) {
529 var $default_o2m = jQuery(idSelector('_terp_default_o2m/'+k));
530 if ($current_field.hasClass('gridview')){
531 if ($default_o2m.length && !value) {
532 if($default_o2m.val()) {
533 $default_o2m.val('');
534 new ListView(k).reload();
535 } else {
536 continue;
537 }
538 }
539 else {
540 jQuery.post(
541 '/openerp/listgrid/get_o2m_defaults', {
542 o2m_values: serializeJSON(value),
543 model: jQuery('#_terp_model').val(),
544 o2m_model: jQuery(idSelector(prefix+k+'/_terp_model')).val(),
545 name: k,
546 view_type: jQuery('#_terp_view_type').val(),
547 view_id: jQuery('#_terp_view_id').val(),
548 o2m_view_type: jQuery(idSelector(prefix+k+'/_terp_view_type')).val(),
549 o2m_view_id: jQuery(idSelector(prefix+k+'/_terp_view_id')).val(),
550 editable: jQuery(idSelector(prefix+k+'/_terp_editable')).val(),
551 limit: jQuery(idSelector(prefix+k+'/_terp_limit')).val(),
552 offset: jQuery(idSelector(prefix+k+'/_terp_offset')).val(),
553 o2m_context: jQuery(idSelector(prefix+k+'/_terp_context')).val(),
554 o2m_domain: jQuery(idSelector(prefix+k+'/_terp_domain')).val()
555 }, function(obj) {
556 $current_field.closest('.list-a').replaceWith(obj.view);
557 if ($default_o2m.length) {
558 $default_o2m.val(obj.formated_o2m_values);
559 }
560 else {
561 jQuery(idSelector(k)).parents('td.o2m_cell').append(
562 jQuery('<input>', {
563 id: '_terp_default_o2m/'+k,
564 type: 'hidden',
565 name:'_terp_default_o2m/'+k,
566 value: obj.formated_o2m_values
567 })
568 );
569 }
570 }, 'json');
571 }
572 }
573 fld.__lock_onchange = true;
574 return;
575 }
527576
528 switch (kind) {577 switch (kind) {
529 case 'picture':578 case 'picture':
530579
=== modified file 'addons/openerp/static/javascript/openerp/openerp.base.js'
--- addons/openerp/static/javascript/openerp/openerp.base.js 2010-11-23 14:01:24 +0000
+++ addons/openerp/static/javascript/openerp/openerp.base.js 2010-11-24 11:46:04 +0000
@@ -216,7 +216,7 @@
216 }216 }
217});217});
218218
219jQuery(document).bind('ready ajaxStop', function (){219jQuery(document).bind('ready', function (){
220 var $caller = jQuery('[callback]:not([type="hidden"]):not([value=""]):not([disabled]):not([readonly]))');220 var $caller = jQuery('[callback]:not([type="hidden"]):not([value=""]):not([disabled]):not([readonly]))');
221 $caller.each(function(){221 $caller.each(function(){
222 if (jQuery(this).attr('kind') == 'boolean') {222 if (jQuery(this).attr('kind') == 'boolean') {
223223
=== modified file 'addons/openerp/widgets/form/_form.py'
--- addons/openerp/widgets/form/_form.py 2010-11-22 10:09:57 +0000
+++ addons/openerp/widgets/form/_form.py 2010-11-24 11:46:04 +0000
@@ -438,7 +438,7 @@
438# self.default = 0.0438# self.default = 0.0
439439
440 def set_value(self, value):440 def set_value(self, value):
441 self.default = value441 self.default = value or 0.0
442442
443register_widget(Float, ["float"])443register_widget(Float, ["float"])
444444
445445
=== modified file 'addons/openerp/widgets/listgrid.py'
--- addons/openerp/widgets/listgrid.py 2010-11-22 13:31:06 +0000
+++ addons/openerp/widgets/listgrid.py 2010-11-24 11:46:04 +0000
@@ -153,7 +153,7 @@
153153
154 proxy = rpc.RPCProxy(model)154 proxy = rpc.RPCProxy(model)
155155
156 if not self.o2m and not self.m2m and not terp_params.get('_terp_search_text'):156 if not kw.get('default_data') and not self.o2m and not self.m2m and not terp_params.get('_terp_search_text'):
157 if self.limit > 0:157 if self.limit > 0:
158 if self.sort_key:158 if self.sort_key:
159 ids = proxy.search(search_param, self.offset, self.limit, self.sort_key + ' ' +self.sort_order, context)159 ids = proxy.search(search_param, self.offset, self.limit, self.sort_key + ' ' +self.sort_order, context)
160160
=== modified file 'addons/view_calendar/controllers/templates/calpopup.mako'
--- addons/view_calendar/controllers/templates/calpopup.mako 2010-10-20 12:41:41 +0000
+++ addons/view_calendar/controllers/templates/calpopup.mako 2010-11-24 11:46:04 +0000
@@ -23,16 +23,20 @@
23 '_terp_context': openobject.dom.get('_terp_context').value23 '_terp_context': openobject.dom.get('_terp_context').value
24 };24 };
2525
26 var req = openobject.http.postJSON('/view_calendar/calpopup/get_defaults', params);26 jQuery.ajax({
27 req.addCallback(function(obj){27 url:'/view_calendar/calpopup/get_defaults',
28 forEach(items(obj), function(item){28 dataType: 'json',
29 var k = item[0];29 type: 'POST',
30 var v = item[1];30 data: params,
3131 success: function(obj) {
32 var e = openobject.dom.get(k);32 forEach(items(obj), function(item){
3333 var k = item[0];
34 if (e) e.value = v;34 var v = item[1];
35 });35 var e = openobject.dom.get(k);
36
37 if (e) e.value = v;
38 });
39 }
36 });40 });
37 }41 }
3842