[REF] web: restore editable list views behaviors

Since the new views, the editable list view lost some behaviors. These
behaviors were indeed implemented in the form controller/renderer but
as the new editable list view does not use an inline form view, these
behaviors had to be implemented in the basic controller/renderer.
For this to work, the list editable renderer had to be changed as it
was doing work that should be done by the controller.

The problem is even more complex because the x2m fields are using a
list renderer but not a list controller. So moving code from the
renderer to the controller obviously broke the x2m fields. Right now,
the problem is solved by catching renderer events and forwarding new
ones to the form controller (which handles the x2m specifically).

The initial goal of this commit was to share the validation of records
on save. Indeed, the fields were marked as invalid in the form view but
not in the list view. Also, before the new views, the editable list
view had different behaviors if they were used for a x2m field or not.
As these behaviors are making sense, this commit tries to restore them.

Basically, what we want is:
- When a record is saved (form view save or list view line leaving),
the invalid fields are marked (in red), the names of the related fields
are notified to the user and the record is not left.
- When a record is discarded (form view discard or list view discard),
the user is asked to confirm if the record is dirty before making the
record readonly.
- When a record is discarded, if the record is a new one, then the
record should be abandoned (removed as if never existed). In the form
view this induces to go back in the history and in the list view, to
remove a row.
- For x2m fields, the notification of invalid fields is not triggered
but the user is instead asked if he wants to discard the changes made
to the row (indeed, this replaces the list "Discard" button, as non
existent for x2m lists).
- ...

Saving, discarding, marking the fields as invalid and other behaviors
are thus now shared behavior of basic views.

The management of the dirty flag has also been moved to the model
as it was handle by the controller for the form view but by the
renderer for the list view. Now this flag is directly managed in the
basic model (the model can have changes thanks to the `_changes`
property but not be dirty (this is the case for creations)).
This change however created a problem. The view manager is currently
keeping asking if there are changes to discard at each action which
might lead to leave a dirty record (appswitcher / url change / ...).
It however did not discard anything as leaving if the user is ok with
it will lead to an implicit discard. However, as the view manager might
ask for this discard multiple times by second, the controller was
marking the record as not dirty the first time but without discarding
the changes. This is more complex to do now, as the dirty state is
part of the model and that the renderer should match the model data.
To solve this problem, the view manager now actually discard changes
explicitely when asked to. Even though this had been optimized to not
cause any rerender in some cases where it is not needed, this could
cause some performance decreasing. However, this makes some cases more
logical (opening the app switcher on a dirty form view then going back
to the form view by hitting the "go back" button left the form view
untouched although the user asked to discard it). This solution will
be improved with the view manager refactoring.

This commit is also making use of the `commitChanges` system which had
been implemented for HTML fields. Indeed, these fields cannot know
about all of their changes, so when hitting the save button, we asked
those fields to commit their value. Using this system is a great way
to make the `isValid` method of x2m fields synchronous. Indeed, before
this commit, the method was sometimes asking the user if he wants to
discard an invalid line before save. That case can be handled by the
x2m `commitChanges` method: we consider that saving the lines of x2m is
an operation that has to be done before considering the save, so we ask
all the x2m fields to do so at that time. Also, the system was broken
since a recent commit: we indeed protected the changes - save order
with a mutex but unfortunately, the `commitChanges` method was part of
the save and the changes it triggered were not able to be considered
because of this mutex. This had not been detected by tests as there is
not current way to test html fields.

1 files changed, 205 insertions, 9 deletions

diff --git a/addons/web/static/src/js/views/basic/basic_controller.js b/addons/web/static/src/js/views/basic/basic_controller.js
index 97e0ca0f4be..33f6727987e 100644
--- a/addons/web/static/src/js/views/basic/basic_controller.js
+++ b/addons/web/static/src/js/views/basic/basic_controller.js
@@ -16,7 +16,7 @@ var Pager = require('web.Pager');
 
 var _t = core._t;
 
-return AbstractController.extend(FieldManagerMixin, {
+var BasicController = AbstractController.extend(FieldManagerMixin, {
     custom_events: _.extend({}, AbstractController.prototype.custom_events, FieldManagerMixin.custom_events, {
         reload: '_onReload',
     }),
@@ -34,7 +34,7 @@ return AbstractController.extend(FieldManagerMixin, {
         this.hasButtons = params.hasButtons;
         FieldManagerMixin.init.call(this, this.model);
         this.handle = params.initialState.id;
-        this.isDirty = false;
+        this.mode = params.mode || 'readonly';
         this.mutex = new concurrency.Mutex();
     },
     /**
@@ -51,6 +51,82 @@ return AbstractController.extend(FieldManagerMixin, {
     //--------------------------------------------------------------------------
 
     /**
+     * Determines if we can discard the current changes. If the model is not
+     * dirty, that is not a problem. However, if it is dirty, we have to ask
+     * the user for confirmation.
+     *
+     * @override
+     * @param {string} [recordID] - default to main recordID
+     * @returns {Deferred<boolean>}
+     *          resolved if can be discarded, a boolean value is given to tells
+     *          if there is something to discard or not
+     *          rejected otherwise
+     */
+    canBeDiscarded: function (recordID) {
+        if (!this.model.isDirty(recordID || this.handle)) {
+            return $.when(false);
+        }
+
+        var message = _t("The record has been modified, your changes will be discarded. Are you sure you want to ?");
+        var def = $.Deferred();
+        var dialog = Dialog.confirm(this, message, {
+            title: _t("Warning"),
+            confirm_callback: def.resolve.bind(def, true),
+            cancel_callback: def.reject.bind(def),
+        });
+        dialog.on('closed', def, def.reject);
+        return def;
+    },
+    /**
+     * Ask the renderer if all associated field widget are in a valid state for
+     * saving (valid value and non-empty value for required fields). If this is
+     * not the case, this notifies the user with a warning containing the names
+     * of the invalid fields.
+     *
+     * Note: changing the style of invalid fields is the renderer's job.
+     *
+     * @param {string} [recordID] - default to main recordID
+     * @return {boolean}
+     */
+    canBeSaved: function (recordID) {
+        var fieldNames = this.renderer.canBeSaved(recordID || this.handle);
+        if (fieldNames.length) {
+            this._notifyInvalidFields(fieldNames);
+            return false;
+        }
+        return true;
+    },
+    /**
+     * Discards the changes made to the record whose ID is given, if necessary.
+     * Automatically leaves to default mode for the given record.
+     *
+     * @param {string} [recordID] - default to main recordID
+     * @param {Object} [options]
+     * @param {boolean} [options.readonlyIfRealDiscard=false]
+     *        After discarding record changes, the usual option is to make the
+     *        record readonly. However, the view manager calls this function
+     *        at inappropriate times in the current code and in that case, we
+     *        don't want to go back to readonly if there is nothing to discard
+     *        (e.g. when switching record in edit mode in form view, we expect
+     *        the new record to be in edit mode too, but the view manager calls
+     *        this function as the URL changes...) @todo get rid of this when
+     *        the view manager is improved.
+     * @returns {Deferred}
+     */
+    discardChanges: function (recordID, options) {
+        var self = this;
+        recordID = recordID || this.handle;
+        return this.canBeDiscarded(recordID).then(function (needDiscard) {
+            if (needDiscard) { // Just some optimization
+                self.model.discardChanges(recordID);
+            }
+            if (options && options.readonlyIfRealDiscard && !needDiscard) {
+                return;
+            }
+            self._setMode('readonly', recordID);
+        });
+    },
+    /**
      * @override
      */
     renderPager: function ($node, options) {
@@ -74,11 +150,32 @@ return AbstractController.extend(FieldManagerMixin, {
         this._updatePager();  // to force proper visibility
     },
     /**
+     * Saves the record whose ID is given if necessary (@see _saveRecord).
+     *
+     * @param {string} [recordID] - default to main recordID
+     * @param {Object} [options]
+     * @returns {Deferred}
+     */
+    saveRecord: function (recordID, options) {
+        // Some field widgets (e.g. 'html') can't detect (all) their changes, so
+        // we ask them to commit their current value before saving. This has to
+        // be done outside of the mutex protection of saving because
+        // commitChanges will trigger changes and these are also protected. So
+        // the actual saving has to be done after these changes. Also the
+        // commitChanges operation might not be synchronous for other reason
+        // (e.g. the x2m fields will ask the user if some discarding has to be
+        // made). This operation must also be mutex-protected as commitChanges
+        // function of x2m has to be aware of all final changes made to a row.
+        this.mutex.exec(this.renderer.commitChanges.bind(this.renderer, recordID || this.handle)); // TODO write a test for this
+        return this.mutex.exec(this._saveRecord.bind(this, recordID, options));
+    },
+    /**
      * @override
      * @returns {Deferred}
      */
     update: function (params, options) {
         var self = this;
+        this.mode = params.mode || this.mode;
         return this._super(params, options).then(function () {
             self._updateEnv();
             self._updatePager();
@@ -90,6 +187,24 @@ return AbstractController.extend(FieldManagerMixin, {
     //--------------------------------------------------------------------------
 
     /**
+     * Does the necessary action when trying to "abandon" a given record (e.g.
+     * when trying to make a new record readonly without having saved it). By
+     * default, if the abandoned record is the main view one, the only possible
+     * action is to leave the current view. Otherwise, it is a x2m line, ask the
+     * model to remove it.
+     *
+     * @private
+     * @param {string} [recordID] - default to main recordID
+     */
+    _abandonRecord: function (recordID) {
+        recordID = recordID || this.handle;
+        if (recordID === this.handle) {
+            this.trigger_up('switch_to_previous_view');
+        } else {
+            this.model.removeLine(recordID);
+        }
+    },
+    /**
      * We override applyChanges (from the field manager mixin) to protect it
      * with a mutex.
      *
@@ -184,6 +299,24 @@ return AbstractController.extend(FieldManagerMixin, {
         return state.count !== 0;
     },
     /**
+     * Helper function to display a warning that some fields have an invalid
+     * value. This is used when a save operation cannot be completed.
+     *
+     * @private
+     * @param {string[]} invalidFields - list of field names
+     */
+    _notifyInvalidFields: function (invalidFields) {
+        var record = this.model.get(this.handle, {raw: true});
+        var fields = record.fields;
+        var warnings = invalidFields.map(function (fieldName) {
+            var fieldStr = fields[fieldName].string;
+            return _.str.sprintf('<li>%s</li>', _.escape(fieldStr));
+        });
+        warnings.unshift('<ul>');
+        warnings.push('</ul>');
+        this.do_warn(_t("The following fields are invalid:"), warnings.join(''));
+    },
+    /**
      * Hook method, called when record(s) has been deleted.
      *
      * @see _deleteRecord
@@ -193,6 +326,67 @@ return AbstractController.extend(FieldManagerMixin, {
         this.update({});
     },
     /**
+     * Saves the record whose ID is given, if necessary. Automatically leaves
+     * edit mode for the given record, unless told otherwise.
+     *
+     * @param {string} [recordID] - default to main recordID
+     * @param {Object} [options]
+     * @param {boolean} [options.stayInEdit=false]
+     *        if true, leave the record in edit mode after save
+     * @param {boolean} [options.reload=true]
+     *        if true, reload the record after (real) save
+     * @param {boolean} [options.savePoint=false]
+     *        if true, the record will only be 'locally' saved: its changes
+     *        will move from the _changes key to the data key
+     * @returns {Deferred}
+     */
+    _saveRecord: function (recordID, options) {
+        recordID = recordID || this.handle;
+        options = _.defaults(options || {}, {
+            stayInEdit: false,
+            reload: true,
+            savePoint: false,
+        });
+
+        var def = $.Deferred();
+        // Check if the view is in a valid state for saving
+        // Note: it is the model's job to do nothing if there is nothing to save
+        if (this.canBeSaved(recordID)) {
+            def = this.model.save(recordID, { // Save then leave edit mode
+                reload: options.reload,
+                savePoint: options.savePoint,
+            });
+        } else {
+            def.reject(); // Cannot be saved, do nothing at all
+        }
+
+        if (options.stayInEdit) {
+            return def;
+        } else {
+            return def.then(this._setMode.bind(this, 'readonly', recordID));
+        }
+    },
+    /**
+     * Change the mode for the record associated to the given ID.
+     * If the given recordID is the view's main one, then the whole view mode is
+     * changed (@see BasicController.update).
+     *
+     * @private
+     * @param {string} mode - 'readonly' or 'edit'
+     * @param {string} [recordID]
+     */
+    _setMode: function (mode, recordID) {
+        recordID = recordID || this.handle;
+        // If trying to make a temporary record readonly, discard the record
+        if (mode === 'readonly' && this.model.isNew(recordID)) {
+            this._abandonRecord(recordID);
+            return;
+        }
+        if (recordID === this.handle) {
+            this.update({mode: mode}, {reload: false});
+        }
+    },
+    /**
      * Helper method, to get the current environment variables from the model
      * and notifies the component chain (by bubbling an event up)
      */
@@ -226,15 +420,17 @@ return AbstractController.extend(FieldManagerMixin, {
     //--------------------------------------------------------------------------
 
     /**
-     * We need to keep manually track of the isDirty property.  Note that it is
-     * not the same as the one from the basic model.  A new record from the
-     * basic model is considered dirty, but from the controller point of view,
-     * there was no user interaction and should not be dirty.
+     * Forces to save directly the changes if the controller is in readonly,
+     * because in that case the changes come from widgets that are editable even
+     * in readonly (e.g. Priority).
      *
      * @private
+     * @param {OdooEvent}
      */
-    _onFieldChanged: function () {
-        this.isDirty = true;
+    _onFieldChanged: function (ev) {
+        if (this.mode === 'readonly') {
+            ev.data.force_save = true;
+        }
         FieldManagerMixin._onFieldChanged.apply(this, arguments);
     },
     /**
@@ -261,7 +457,7 @@ return AbstractController.extend(FieldManagerMixin, {
             this.reload({fieldNames: data.fieldNames});
         }
     },
-
 });
 
+return BasicController;
 });