Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Saas 12.3 list multi save fix aab #36522

Closed

Conversation

@aab-odoo
Copy link
Contributor

commented Sep 6, 2019

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo robodoo added the seen 🙂 label Sep 6, 2019

@C3POdoo C3POdoo added the RD label Sep 6, 2019

@Arcasias Arcasias force-pushed the odoo-dev:saas-12.3-list-multi-save-fix-aab branch 2 times, most recently from cef953f to 7ebd94a Sep 6, 2019

var self = this;
this.onChangePrevented = true;
window.addEventListener('mouseup', function (mouseupEvent) {
var prevented = self.onChangePrevented;

This comment was marked as resolved.

Copy link
@Arcasias

Arcasias Sep 9, 2019

Contributor

preventedEvent

*/
_onDiscardMousedown: function (ev) {
var self = this;
this.onChangePrevented = true;

This comment was marked as resolved.

Copy link
@Arcasias

Arcasias Sep 9, 2019

Contributor

onChange -> fieldChanged (+ commit message)

self.onChangePrevented = false;
// If there was a prevented onFieldChanged and the mouseup is triggered
// anywhere else than on the button, triggers it.
if (ev.target !== mouseupEvent.target && prevented.stopPropagation) {

This comment was marked as resolved.

Copy link
@Arcasias

Arcasias Sep 9, 2019

Contributor

instanceof OdooEvent

* @param {OdooEvent} ev
*/
_onInputKeyup: function (ev) {
if (ev.which === $.ui.keyCode.ENTER || ev.which === $.ui.keyCode.TAB) {

This comment was marked as resolved.

Copy link
@Arcasias

Arcasias Sep 9, 2019

Contributor

Remove

}
if (this.$input.val() === "") {
return;
} else {

This comment was marked as resolved.

Copy link
@Arcasias

Arcasias Sep 9, 2019

Contributor

No else

//--------------------------------------------------------------------------

/**
* In list views, an _onFieldChanged always triggers the saving of the record(s).

This comment was marked as resolved.

Copy link
@Arcasias

Arcasias Sep 9, 2019

Contributor

is about not triggering onFieldChanged on any list many2one

@@ -777,6 +777,50 @@ var ListFieldMany2One = FieldMany2One.extend({
_renderReadonly: function () {
this.$el.text(this.m2o_value);
},

This comment has been minimized.

Copy link
@Arcasias

Arcasias Sep 9, 2019

Contributor

To test:

  • readonly list : select 1 record > edit > ??
  • click in unset m2o > click outside > ??

This comment has been minimized.

Copy link
@Arcasias

Arcasias Sep 9, 2019

Contributor

Test result for second dot : focusing out an already empty m2o triggers a focusout, but not a field_changed (_setValue does nothing as the value is considered the same)

@robodoo robodoo added the CI 🤖 label Sep 9, 2019

await testUtils.dom.click(list.$('.o_data_row:first() .o_data_cell:first()'));
list.$('.o_data_row:first() .o_data_cell:first() input').val("oof");

// Simulates an actual click (event chain is: mousedown > change > blur > focus > mouseup > click)

This comment has been minimized.

Copy link
@aab-odoo

aab-odoo Sep 9, 2019

Author Contributor

i'd trigger the whole chain


assert.ok($('.modal').text().includes("Warning"), "Modal should ask to discard changes");
await testUtils.dom.click($('.modal .btn-primary'));

This comment has been minimized.

Copy link
@aab-odoo

aab-odoo Sep 9, 2019

Author Contributor

check value here

list.$('.o_data_row:first() .o_data_cell:first() input').val("oof");

// Simulates a pseudo drag and drop
await testUtils.dom.triggerEvents(list.$buttons.find('.o_list_button_discard'), ['mousedown']);

This comment has been minimized.

Copy link
@aab-odoo

aab-odoo Sep 9, 2019

Author Contributor

i'd do this in another test, as this is quite specific
'editable list view (multi edition): mousedown on "Discard", but mouseup somewhere else'

or something like this

await testUtils.dom.triggerEvents(list.$buttons.find('.o_list_button_discard'), ['mousedown']);
await testUtils.dom.triggerEvents(list.$('.o_data_row:first() .o_data_cell:first() input'), ['change']);
window.dispatchEvent(new MouseEvent('mouseup'));
await new Promise(r => setTimeout(r, 0));

This comment has been minimized.

Copy link
@aab-odoo

aab-odoo Sep 9, 2019

Author Contributor

?

@@ -566,6 +566,43 @@ QUnit.module('fields', {}, function () {
form.destroy();
});

QUnit.test("many2one doesn't trigger onchange when being emptied", async function (assert) {

This comment has been minimized.

Copy link
@aab-odoo

aab-odoo Sep 9, 2019

Author Contributor

fieldChanged

@Arcasias Arcasias force-pushed the odoo-dev:saas-12.3-list-multi-save-fix-aab branch from 7ebd94a to eb7a570 Sep 9, 2019

@robodoo robodoo removed the CI 🤖 label Sep 9, 2019

@Arcasias Arcasias force-pushed the odoo-dev:saas-12.3-list-multi-save-fix-aab branch 2 times, most recently from 84a8c62 to 7f4fca7 Sep 9, 2019

@robodoo robodoo added the CI 🤖 label Sep 9, 2019

*/
init: function () {
this._super.apply(this, arguments);
this.nodeOptions.no_open = this.mode === 'readonly' || this.nodeOptions.no_open;

This comment has been minimized.

Copy link
@aab-odoo

aab-odoo Sep 10, 2019

Author Contributor

this is either useless, or it doesn't work, because we never access this.nodeOptions.no_open after init (we use this.noOpen instead)

@Arcasias Arcasias force-pushed the odoo-dev:saas-12.3-list-multi-save-fix-aab branch from 7f4fca7 to 2f3e9ba Sep 10, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Sep 10, 2019

@Arcasias Arcasias force-pushed the odoo-dev:saas-12.3-list-multi-save-fix-aab branch from 2f3e9ba to 6fe9dc9 Sep 10, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Sep 10, 2019

@aab-odoo aab-odoo force-pushed the odoo-dev:saas-12.3-list-multi-save-fix-aab branch 2 times, most recently from 9454664 to 8863457 Sep 11, 2019

@robodoo robodoo removed the CI 🤖 label Sep 11, 2019

@aab-odoo aab-odoo force-pushed the odoo-dev:saas-12.3-list-multi-save-fix-aab branch 3 times, most recently from 2374200 to 793e20f Sep 11, 2019

@Arcasias Arcasias force-pushed the odoo-dev:saas-12.3-list-multi-save-fix-aab branch from 793e20f to 30619a4 Sep 11, 2019

@aab-odoo aab-odoo force-pushed the odoo-dev:saas-12.3-list-multi-save-fix-aab branch from 30619a4 to c7b49f3 Sep 11, 2019

@robodoo robodoo added the CI 🤖 label Sep 11, 2019

@aab-odoo aab-odoo force-pushed the odoo-dev:saas-12.3-list-multi-save-fix-aab branch from c7b49f3 to a99a540 Sep 11, 2019

@robodoo robodoo removed the CI 🤖 label Sep 11, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

Merge method set to rebase and fast-forward

@robodoo robodoo added the r+ 👌 label Sep 18, 2019

@VincentSchippefilt

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

robodoo r-

@robodoo robodoo removed the r+ 👌 label Sep 18, 2019

@aab-odoo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

@KangOl This is the PR not to forwardport

@Arcasias Arcasias force-pushed the odoo-dev:saas-12.3-list-multi-save-fix-aab branch from 46f0997 to cacebef Sep 18, 2019

Arcasias and others added 13 commits Sep 9, 2019
[FIX] web: readonly many2one in list views
Fixes the way many2one fields are rendered in editable list views.

Before this commit, many2ones in readonly mode would render as links
and their widget would be considered as active even though you couldn't
edit it.

Now, they are rendered as plain text and act the same way as readonly char fields.
[FIX] web: list: multi edit: do not create new row
...when validating last row by pressing ENTER.

In single edition, we automatically add a new row when the user
edits the last one and presses ENTER. This is something we don't
want in multi edition.

Task 2068280
[FIX] web: list: multi edition only between selected records
Before this commit, if a record was selected and another one was edited,
it was considered as multi edition.

Now, the multi edition only works if the edited record is included in the
selection; else it will be the only edited record (single edition).

Task 2068280
[FIX] web: list: disable record selectors when editing
Having those inputs enabled can lead to weird situations where a
record has been modified but can't be saved anymore (because the
user clicked on one of those record selectors to focusout the edited
row). Also we don't want the user to start multi-editing a field,
then change the selection, and then validate the changes.

Task 2068280
[FIX] web: date(time) fields: trigger fieldChanged once
Before this rev., the 'fieldChanged' event was triggered each time
the user selected a value (a day, an hour, a minute or a second) in
the datepicker. This means that if there was an onchange on the
field, a lot of RPCs could have been done when a user set a
datetime. In addition, in a list view with multi edition, the user
was asked to save the changes at each step of the datetime
selection, making the feature unusable.

Task 2068280
[FIX] web: list: multi edit input of date(time) fields
The Date(Time)Fields listen to 'input' and 'change' events (like
all InputFields) to keep track of changes. However, the datepicker
already listens to the 'change' event. As a consequence, in multi
edition in a list view, if the user changed the value of a
date(time) field by directly editing the input, the fieldChanged
event was triggered twice, and there were 2 dialogs asking the user
if he wanted to save the change.

This rev. makes the Date(Time)Fields stop listening themselves to
'input' and 'change' events, and instead completely rely on the
datepicker widget.

Task 2068280
[FIX] web: lists: focus last edited cell
Gave back focus to the last edited cell to increase usability with key nav.

Before this commit, after saving several records with multi edition in a list,
focus was lost (given to the body).

Now, last edited cell is given back the focus after multi editing.
[FIX] web: list: multi edition: disable onchanges
Onchanges make no sense in multi edition as they would be computed
on the 'reference' record (the one use to edit), but applied on all
selected records. This rev. simply disables them in that case.

Task 2068280
[FIX] web: list: multi edit boolean with ENTER
This rev. solves the similar issue as 801c603, but with boolean
fields.

Scenario: select several records, select a boolean cell, press
enter to change the boolean value.

Task 2068280
[FIX] web: list: m2o doesn't trigger fieldChange when empty
Gives time to the user to enter an actual value in m2o in list views.

Before this commit, emptying a many2one field (selecting all > BACKSPACE or DELETE)
triggered instantly a field_change. This was an issue in multi edition as a field
changed triggered instantly a save on selected records without giving time to the
user to enter a new value.

Now, many2ones only trigger a fieldChange when selecting a new valid value.
[FIX] web: handling of invalid formats in list views
Added handling of invalid format types in multiple edition in list views.

Before this commit, in multiple edition, a field that was edited with an invalid value format
(e.g. a numeric field receiving a string value) didn't trigger a field_changed and no prompt
was shown; the record was simply idle with an dirty value.

Now, the list controller also listents for set_dirty events and automatically triggers
an error modal + discards all changes when receiving one.
[FIX] web: list selectors synchronisation
Changed the way the header selector behave when the selection changes.

Before this commit, the header selector was only checked when clicked and
was unchecked as soon as any other record was unchecked.

Now, the header selector reflects the current visible selection:
if all of the other visible selectors are checked, it will be checked.

Folded records are not considered visible, thus only records from active groups
and from the current pager will be taken into account.

@Arcasias Arcasias force-pushed the odoo-dev:saas-12.3-list-multi-save-fix-aab branch from cacebef to c48c7c6 Sep 18, 2019

@VincentSchippefilt

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

robodoo r+

robodoo pushed a commit that referenced this pull request Sep 18, 2019
[FIX] web: list selectors synchronisation
Changed the way the header selector behave when the selection changes.

Before this commit, the header selector was only checked when clicked and
was unchecked as soon as any other record was unchecked.

Now, the header selector reflects the current visible selection:
if all of the other visible selectors are checked, it will be checked.

Folded records are not considered visible, thus only records from active groups
and from the current pager will be taken into account.

closes #36522

Signed-off-by: VincentSchippefilt <VincentSchippefilt@users.noreply.github.com>
@robodoo

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

Merged at 31ec900, thanks!

@robodoo robodoo closed this Sep 18, 2019

@aab-odoo aab-odoo deleted the odoo-dev:saas-12.3-list-multi-save-fix-aab branch Sep 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.