From e61639723d715bfde89c6b55f34601b2449234a6 Mon Sep 17 00:00:00 2001 From: Nicolas Lempereur Date: Tue, 21 May 2019 14:57:32 +0000 Subject: [PATCH 1/3] [FIX] web: resequence work w/ discardable lines In 11.0 091c86c21 introduced the unselection of lines when line are resequenced to solve some issue. There seems to be a related issue in the following use case if: - the one2many has a handle field - the lines have same sequence (eg. adding 2 o2m lines on new record) - there is an invalid o2m line (eg. empty line and required field) - without blurring, a valid line is moved then this happen: - the sequence of lines are computed - the invalid line is removed - the computed sequence of lines are applied => this break next onchange, because we try to apply an onchange to data that does not exist anymore (the invalid line). With this changeset, the invalid lines are discarded before sequences are computed so the computation is still correct before being applied. Without the change, the added test fails with: 5. one onchange happens when lines are resequenced (expect: 3, res: 2) 7. one onchange happens when a line is added (expect: 4, res: 2) 8. onchange worked there is 2 lines (expect: 2, res: 1) opw-1966589 closes #33591 Signed-off-by: Aaron Bohy (aab) --- .../js/views/list/list_editable_renderer.js | 70 +++++++++---------- addons/web/static/tests/views/form_tests.js | 61 ++++++++++++++++ 2 files changed, 96 insertions(+), 35 deletions(-) diff --git a/addons/web/static/src/js/views/list/list_editable_renderer.js b/addons/web/static/src/js/views/list/list_editable_renderer.js index f5cb3a2c732..7a4147f540b 100644 --- a/addons/web/static/src/js/views/list/list_editable_renderer.js +++ b/addons/web/static/src/js/views/list/list_editable_renderer.js @@ -549,48 +549,48 @@ ListRenderer.include({ _resequence: function (event, ui) { var self = this; var movedRecordID = ui.item.data('id'); - var rows = this.state.data; - var row = _.findWhere(rows, {id: movedRecordID}); - var index0 = rows.indexOf(row); - var index1 = ui.item.index(); - var lower = Math.min(index0, index1); - var upper = Math.max(index0, index1) + 1; + self.unselectRow().then(function () { + var rows = self.state.data; + var row = _.findWhere(rows, {id: movedRecordID}); + var index0 = rows.indexOf(row); + var index1 = ui.item.index(); + var lower = Math.min(index0, index1); + var upper = Math.max(index0, index1) + 1; - var order = _.findWhere(self.state.orderedBy, {name: self.handleField}); - var asc = !order || order.asc; - var reorderAll = false; - var sequence = (asc ? -1 : 1) * Infinity; + var order = _.findWhere(self.state.orderedBy, {name: self.handleField}); + var asc = !order || order.asc; + var reorderAll = false; + var sequence = (asc ? -1 : 1) * Infinity; - // determine if we need to reorder all lines - _.each(rows, function (row, index) { - if ((index < lower || index >= upper) && - ((asc && sequence >= row.data[self.handleField]) || - (!asc && sequence <= row.data[self.handleField]))) { - reorderAll = true; - } - sequence = row.data[self.handleField]; - }); + // determine if we need to reorder all lines + _.each(rows, function (row, index) { + if ((index < lower || index >= upper) && + ((asc && sequence >= row.data[self.handleField]) || + (!asc && sequence <= row.data[self.handleField]))) { + reorderAll = true; + } + sequence = row.data[self.handleField]; + }); - if (reorderAll) { - rows = _.without(rows, row); - rows.splice(index1, 0, row); - } else { - rows = rows.slice(lower, upper); - rows = _.without(rows, row); - if (index0 > index1) { - rows.unshift(row); + if (reorderAll) { + rows = _.without(rows, row); + rows.splice(index1, 0, row); } else { - rows.push(row); + rows = rows.slice(lower, upper); + rows = _.without(rows, row); + if (index0 > index1) { + rows.unshift(row); + } else { + rows.push(row); + } } - } - var sequences = _.pluck(_.pluck(rows, 'data'), self.handleField); - var rowIDs = _.pluck(rows, 'id'); + var sequences = _.pluck(_.pluck(rows, 'data'), self.handleField); + var rowIDs = _.pluck(rows, 'id'); - if (!asc) { - rowIDs.reverse(); - } - this.unselectRow().then(function () { + if (!asc) { + rowIDs.reverse(); + } self.trigger_up('resequence', { rowIDs: rowIDs, offset: _.min(sequences), diff --git a/addons/web/static/tests/views/form_tests.js b/addons/web/static/tests/views/form_tests.js index b3a87e29ba6..428cffa9a61 100644 --- a/addons/web/static/tests/views/form_tests.js +++ b/addons/web/static/tests/views/form_tests.js @@ -6694,5 +6694,66 @@ QUnit.module('Views', { form.destroy(); }); + QUnit.test('resequence list lines when discardable lines are present', function (assert) { + assert.expect(8); + + var onchangeNum = 0; + + this.data.partner.onchanges = { + p: function (obj) { + onchangeNum++; + obj.foo = obj.p.length.toString(); + }, + }; + + var form = createView({ + View: FormView, + model: 'partner', + data: this.data, + arch: '
' + + '' + + '' + + '', + archs: { + 'partner,false,list': + '' + + '' + + '' + + '', + }, + }); + + assert.strictEqual(onchangeNum, 1, "one onchange happens when form is opened"); + assert.strictEqual(form.$('[name="foo"]').val(), "0", "onchange worked there is 0 line"); + + // Add one line + form.$('.o_field_x2many_list_row_add a').click(); + form.$('.o_field_one2many input:first').focus(); + form.$('.o_field_one2many input:first').val('first line').trigger('input'); + form.$('input[name="foo"]').click(); + assert.strictEqual(onchangeNum, 2, "one onchange happens when a line is added"); + assert.strictEqual(form.$('[name="foo"]').val(), "1", "onchange worked there is 1 line"); + + // Drag and drop second line before first one (with 1 draft and invalid line) + form.$('.o_field_x2many_list_row_add a').click(); + testUtils.dragAndDrop( + form.$('.ui-sortable-handle').eq(0), + form.$('.o_data_row').last(), + {position: 'bottom'} + ); + assert.strictEqual(onchangeNum, 3, "one onchange happens when lines are resequenced") + assert.strictEqual(form.$('[name="foo"]').val(), "1", "onchange worked there is 1 line"); + + // Add a second line + form.$('.o_field_x2many_list_row_add a').click(); + form.$('.o_field_one2many input:first').focus(); + form.$('.o_field_one2many input:first').val('second line').trigger('input'); + form.$('input[name="foo"]').click(); + assert.strictEqual(onchangeNum, 4, "one onchange happens when a line is added"); + assert.strictEqual(form.$('[name="foo"]').val(), "2", "onchange worked there is 2 lines"); + + form.destroy(); + }); + }); }); From 69b18768f44187720b9b114f906745f534480396 Mon Sep 17 00:00:00 2001 From: Uku Lagle Date: Tue, 7 May 2019 18:08:51 +0000 Subject: [PATCH 2/3] [FIX] website_sale_wishlist, js: add to cart qty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The selector used for finding the qty field is simply 'qty', which is useless in most cases. While there is actually no quantity input in a wishlist template, it could be leveraged by extension modules. Support for same markup as found in product page seems appropriate: input[name="add_qty"] closes odoo/odoo#33224 Signed-off-by: Jérémy Kersten (jke) --- .../static/src/js/website_sale_wishlist.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/addons/website_sale_wishlist/static/src/js/website_sale_wishlist.js b/addons/website_sale_wishlist/static/src/js/website_sale_wishlist.js index 153527096a5..a5dbda94698 100644 --- a/addons/website_sale_wishlist/static/src/js/website_sale_wishlist.js +++ b/addons/website_sale_wishlist/static/src/js/website_sale_wishlist.js @@ -138,7 +138,7 @@ var ProductWishlist = Widget.extend({ // can be hidden if empty $('#my_cart').removeClass('hidden'); website_sale_utils.animate_clone($('#my_cart'), tr, 25, 40); - return this.add_to_cart(product, tr.find('qty').val() || 1); + return this.add_to_cart(product, tr.find('input[name="add_qty"]').val() || 1); }, wishlist_mv: function(e){ var tr = $(e.currentTarget).parents('tr'); @@ -146,7 +146,7 @@ var ProductWishlist = Widget.extend({ $('#my_cart').removeClass('hidden'); website_sale_utils.animate_clone($('#my_cart'), tr, 25, 40); - var adding_deffered = this.add_to_cart(product, tr.find('qty').val() || 1); + var adding_deffered = this.add_to_cart(product, tr.find('input[name="add_qty"]').val() || 1); this.wishlist_rm(e, adding_deffered); return adding_deffered; }, From 3e12f2ff56eaa4950fd14d1a4bb29e3721c0268a Mon Sep 17 00:00:00 2001 From: Nans Lefebvre Date: Tue, 28 May 2019 12:15:23 +0000 Subject: [PATCH 3/3] [FIX] mail: fix mail template onchange The stable behaviour of the mail composer is the following: - if a template adds new attachments, the composer only has this list - if a template doesn't add attachments, the list is unchanged - if no template is set, the list is cleared up In the case of templates, we also check that both static and dynamic attachments are added to the list. We clean up after c6d718f2118 and subsequently 516f22c3987 which failed to take into account the fact that the attachement list could be a blend of commands and ids. Transforming that is taken care of by _convert_to_write, which semantics was changed by c6d718f2118. To keep the second behaviour, we thus need to add a (5,) command. opw 2003197 closes odoo/odoo#33706 Signed-off-by: Nans Lefebvre (len) --- addons/mail/tests/test_mail_template.py | 26 +++++++++++----------- addons/mail/wizard/mail_compose_message.py | 7 +++--- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/addons/mail/tests/test_mail_template.py b/addons/mail/tests/test_mail_template.py index 1d618e956e9..34e6e57e1e2 100644 --- a/addons/mail/tests/test_mail_template.py +++ b/addons/mail/tests/test_mail_template.py @@ -72,7 +72,7 @@ class TestMailTemplate(TestMail): static attachments are not duplicated and while reports are re-generated, and that intermediary attachments are dropped.""" - composer = self.env['mail.compose.message'].create({}) + composer = self.env['mail.compose.message'].with_context(default_attachment_ids=[]).create({}) report_template = self.env.ref('web.action_report_externalpreview') template_1 = self.email_template.copy({ 'report_template': report_template.id, @@ -82,20 +82,20 @@ class TestMailTemplate(TestMail): 'report_template': report_template.id, }) - onchange_templates = [template_1, template_2, template_1] - attachments_onchange = [] + onchange_templates = [template_1, template_2, template_1, False] + attachments_onchange = [composer.attachment_ids] # template_1 has two static attachments and one dynamically generated report, # template_2 only has the report, so we should get 3, 1, 3 attachments - attachment_numbers = [3, 1, 3] + attachment_numbers = [0, 3, 1, 3, 0] - for template in onchange_templates: - onchange = composer.onchange_template_id( - template.id, 'comment', 'mail.test', self.test_pigs.id - ) - values = composer._convert_to_record(composer._convert_to_cache(onchange['value'])) - attachments = values['attachment_ids'] - composer.attachment_ids = attachments # we apply the onchange - attachments_onchange.append(attachments) + with self.env.do_in_onchange(): + for template in onchange_templates: + onchange = composer.onchange_template_id( + template.id if template else False, 'comment', 'mail.test', self.test_pigs.id + ) + values = composer._convert_to_record(composer._convert_to_cache(onchange['value'])) + attachments_onchange.append(values['attachment_ids']) + composer.update(onchange['value']) self.assertEqual( [len(attachments) for attachments in attachments_onchange], @@ -103,7 +103,7 @@ class TestMailTemplate(TestMail): ) self.assertTrue( - len(attachments_onchange[0] & attachments_onchange[2]) == 2, + len(attachments_onchange[1] & attachments_onchange[3]) == 2, "The two static attachments on the template should be common to the two onchanges" ) diff --git a/addons/mail/wizard/mail_compose_message.py b/addons/mail/wizard/mail_compose_message.py index 7516f507590..7347e7ad7d5 100644 --- a/addons/mail/wizard/mail_compose_message.py +++ b/addons/mail/wizard/mail_compose_message.py @@ -350,7 +350,6 @@ class MailComposer(models.TransientModel): - normal mode: return rendered values /!\ for x2many field, this onchange return command instead of ids """ - attachment_ids = [] if template_id and composition_mode == 'mass_mail': template = self.env['mail.template'].browse(template_id) fields = ['subject', 'body_html', 'email_from', 'reply_to', 'mail_server_id'] @@ -366,6 +365,7 @@ class MailComposer(models.TransientModel): values = self.generate_email_for_composer(template_id, [res_id])[res_id] # transform attachments into attachment_ids; not attached to the document because this will # be done further in the posting process, allowing to clean database if email not send + attachment_ids = [] Attachment = self.env['ir.attachment'] for attach_fname, attach_datas in values.pop('attachments', []): data_attach = { @@ -377,6 +377,8 @@ class MailComposer(models.TransientModel): 'type': 'binary', # override default_type from context, possibly meant for another model! } attachment_ids.append(Attachment.create(data_attach).id) + if values.get('attachment_ids', []) or attachment_ids: + values['attachment_ids'] = [(5,)] + values.get('attachment_ids', []) + attachment_ids else: default_values = self.with_context(default_composition_mode=composition_mode, default_model=model, default_res_id=res_id).default_get(['composition_mode', 'model', 'res_id', 'parent_id', 'partner_ids', 'subject', 'body', 'email_from', 'reply_to', 'attachment_ids', 'mail_server_id']) values = dict((key, default_values[key]) for key in ['subject', 'body', 'partner_ids', 'email_from', 'reply_to', 'attachment_ids', 'mail_server_id'] if key in default_values) @@ -388,10 +390,7 @@ class MailComposer(models.TransientModel): # ORM handle the assignation of command list on new onchange (api.v8), # this force the complete replacement of x2many field with # command and is compatible with onchange api.v7 - attachment_ids += values.pop('attachment_ids' , []) values = self._convert_to_write(values) - if attachment_ids: - values.update(attachment_ids=[(6, 0, attachment_ids)]) return {'value': values}