[FIX] mail: ignore undefined in JS fields create/write

Passing a field containing `undefined` should lead to the same result as not
passing the field.

Fields that relied on `undefined` as a value have been changed to use the newly
introduced `clear` command, that resets a field to its default value (often
that is equal to `undefined`).

task-2336491

closes odoo/odoo#58314

X-original-commit: b18824485483564d2ab80f41914a3548aa528ae0
Signed-off-by: Alexandre Kühn (aku) <aku@odoo.com>

addons/mail/static/src/components/discuss/tests/discuss_pinned_tests.js

@@ -81,7 +81,11 @@ QUnit.test('sidebar: pinned channel 3: open pinned channel and unpin it', async
 
     // channel that is expected to be found in the sidebar
     // with a random unique id that will be referenced in the test
-    this.data['mail.channel'].records.push({ id: 20 });
+    this.data['mail.channel'].records.push({
+        id: 20,
+        is_minimized: true,
+        state: 'open',
+    });
     await this.start({
         async mockRPC(route, args) {
             if (args.method === 'execute_command') {

addons/mail/static/src/components/discuss/tests/discuss_tests.js

@@ -1834,7 +1834,7 @@ QUnit.test('new messages separator [REQUIRE FOCUS]', async function (assert) {
 });
 
 QUnit.test('restore thread scroll position', async function (assert) {
-    assert.expect(5);
+    assert.expect(6);
     // channels expected to be rendered, with random unique id that will be referenced in the test
     this.data['mail.channel'].records.push({ id: 11 }, { id: 12 });
     for (let i = 1; i <= 25; i++) {
@@ -1872,6 +1872,15 @@ QUnit.test('restore thread scroll position', async function (assert) {
         25,
         "should have 25 messages in channel 11"
     );
+    const initialMessageList = document.querySelector(`
+        .o_Discuss_thread
+        .o_ThreadView_messageList
+    `);
+    assert.strictEqual(
+        initialMessageList.scrollTop + initialMessageList.clientHeight,
+        initialMessageList.scrollHeight,
+        "should have scrolled to bottom of channel 11 initially"
+    );
 
     await this.afterEvent({
         eventName: 'o-component-message-list-scrolled',

addons/mail/static/src/model/model_field.js

@@ -1,6 +1,8 @@
 odoo.define('mail/static/src/model/model_field.js', function (require) {
 'use strict';
 
+const { FieldCommand } = require('mail/static/src/model/model_field_command.js');
+
 /**
  * Class whose instances represent field on a model.
  * These field definitions are generated from declared fields in static prop
@@ -215,6 +217,30 @@ class ModelField {
         return ModelField._relation(modelName, Object.assign({}, options, { relationType: 'one2one' }));
     }
 
+    /**
+     * Clears the value of this field on the given record. It consists of
+     * setting this to its default value. In particular, using `clear` is the
+     * only way to write `undefined` on a field, as long as `undefined` is its
+     * default value. Relational fields are always unlinked before the default
+     * is applied.
+     *
+     * @param {mail.model} record
+     * @param {options} [options]
+     * @returns {boolean} whether the value changed for the current field
+     */
+    clear(record, options) {
+        let hasChanged = false;
+        if (this.fieldType === 'relation') {
+            if (this.parseAndExecuteCommands(record, [['unlink-all']], options)) {
+                hasChanged = true;
+            }
+        }
+        if (this.parseAndExecuteCommands(record, this.default, options)) {
+            hasChanged = true;
+        }
+        return hasChanged;
+    }
+
     /**
      * Combine current field definition with provided field definition and
      * return the combined field definition. Useful to track list of hashes of
@@ -303,6 +329,33 @@ class ModelField {
         throw new Error(`cannot get field with unsupported type ${this.fieldType}.`);
     }
 
+    /**
+     * Parses newVal for command(s) and executes them.
+     *
+     * @param {mail.model} record
+     * @param {any} newVal
+     * @param {Object} [options]
+     * @returns {boolean} whether the value changed for the current field
+     */
+    parseAndExecuteCommands(record, newVal, options) {
+        if (newVal instanceof FieldCommand) {
+            // single command given
+            return newVal.execute(this, record, options);
+        }
+        if (typeof newVal instanceof Array && newVal[0] instanceof FieldCommand) {
+            // multi command given
+            let hasChanged = false;
+            for (const command of newVal) {
+                if (command.execute(this, record, options)) {
+                    hasChanged = true;
+                }
+            }
+            return hasChanged;
+        }
+        // not a command
+        return this.set(record, newVal, options);
+    }
+
     /**
      * Get the raw value associated to this field. For relations, this means
      * the local id or list of local ids of records in this relational field.

addons/mail/static/src/model/model_field_command.js

@@ -0,0 +1,45 @@
+odoo.define('mail/static/src/model/model_field_command.js', function (require) {
+'use strict';
+
+/**
+ * Allows field update to detect if the value it received is a command to
+ * execute (in which was it will be an instance of this class) or an actual
+ * value to set (in all other cases).
+ */
+class FieldCommand {
+    /**
+     * @constructor
+     * @param {function} func function to call when executing this command
+     */
+    constructor(func) {
+        this.func = func;
+    }
+
+    /**
+     * @param {ModelField} field
+     * @param {mail.model} record
+     * @param {options} [options]
+     * @returns {boolean} whether the value changed for the current field
+     */
+    execute(field, record, options) {
+        return this.func(field, record, options);
+    }
+}
+
+/**
+ * Returns a clear command to give to the model manager at create/update.
+ */
+function clear() {
+    return new FieldCommand((field, record, options) =>
+        field.clear(record, options)
+    );
+}
+
+return {
+    // class
+    FieldCommand,
+    // shortcuts
+    clear,
+};
+
+});

addons/mail/static/src/model/model_manager.js

@@ -594,7 +594,8 @@ class ModelManager {
              */
             const data2 = {};
             for (const field of Model.__fieldList) {
-                if (field.fieldName in data) {
+                // `undefined` should have the same effect as not passing the field
+                if (data[field.fieldName] !== undefined) {
                     data2[field.fieldName] = data[field.fieldName];
                 } else {
                     data2[field.fieldName] = field.default;
@@ -630,7 +631,7 @@ class ModelManager {
         for (const field of Model.__fieldList) {
             if (field.fieldType === 'relation') {
                 // ensure inverses are properly unlinked
-                field.set(record, [['unlink-all']]);
+                field.parseAndExecuteCommands(record, [['unlink-all']]);
             }
         }
         this._hasAnyChangeDuringCycle = true;
@@ -1066,12 +1067,16 @@ class ModelManager {
         const Model = record.constructor;
         let hasChanged = false;
         for (const fieldName of Object.keys(data)) {
+            if (data[fieldName] === undefined) {
+                // `undefined` should have the same effect as not passing the field
+                continue;
+            }
             const field = Model.__fieldMap[fieldName];
             if (!field) {
                 throw new Error(`Cannot create/update record with data unrelated to a field. (model: "${Model.modelName}", non-field attempted update: "${fieldName}")`);
             }
             const newVal = data[fieldName];
-            if (!field.set(record, newVal, options)) {
+            if (!field.parseAndExecuteCommands(record, newVal, options)) {
                 continue;
             }
             hasChanged = true;

addons/mail/static/src/models/activity/activity.js

@@ -3,6 +3,7 @@ odoo.define('mail/static/src/models/activity/activity/js', function (require) {
 
 const { registerNewModel } = require('mail/static/src/model/model_core.js');
 const { attr, many2many, many2one } = require('mail/static/src/model/model_field.js');
+const { clear } = require('mail/static/src/model/model_field_command.js');
 
 function factory(dependencies) {
 
@@ -245,7 +246,7 @@ function factory(dependencies) {
          */
         _computeNote() {
             if (this.note === '<p><br></p>') {
-                return undefined;
+                return clear();
             }
             return this.note;
         }

addons/mail/static/src/models/attachment/attachment.js

@@ -3,6 +3,7 @@ odoo.define('mail/static/src/models/attachment/attachment.js', function (require
 
 const { registerNewModel } = require('mail/static/src/model/model_core.js');
 const { attr, many2many, many2one } = require('mail/static/src/model/model_field.js');
+const { clear } = require('mail/static/src/model/model_field_command.js');
 
 function factory(dependencies) {
 
@@ -142,7 +143,7 @@ function factory(dependencies) {
 
         /**
          * @private
-         * @returns {string}
+         * @returns {string|undefined}
          */
         _computeDefaultSource() {
             if (this.fileType === 'image') {
@@ -169,15 +170,19 @@ function factory(dependencies) {
             if (this.fileType === 'video') {
                 return `/web/image/${this.id}?model=ir.attachment`;
             }
-            return undefined;
+            return clear();
         }
 
         /**
          * @private
-         * @returns {string}
+         * @returns {string|undefined}
          */
         _computeDisplayName() {
-            return this.name || this.filename;
+            const displayName = this.name || this.filename;
+            if (displayName) {
+                return displayName;
+            }
+            return clear();
         }
 
         /**
@@ -185,7 +190,11 @@ function factory(dependencies) {
          * @returns {string|undefined}
          */
         _computeExtension() {
-            return this.filename && this.filename.split('.').pop();
+            const extension = this.filename && this.filename.split('.').pop();
+            if (extension) {
+                return extension;
+            }
+            return clear();
         }
 
         /**
@@ -194,15 +203,15 @@ function factory(dependencies) {
          */
         _computeFileType() {
             if (this.type === 'url' && !this.url) {
-                return undefined;
+                return clear();
             } else if (!this.mimetype) {
-                return undefined;
+                return clear();
             }
             const match = this.type === 'url'
                 ? this.url.match('(youtu|.png|.jpg|.gif)')
                 : this.mimetype.match('(image|video|application/pdf|text)');
             if (!match) {
-                return undefined;
+                return clear();
             }
             if (match[1].match('(.png|.jpg|.gif)')) {
                 return 'image';

addons/mail/static/src/models/chat_window/chat_window.js

@@ -3,6 +3,7 @@ odoo.define('mail/static/src/models/chat_window/chat_window.js', function (requi
 
 const { registerNewModel } = require('mail/static/src/model/model_core.js');
 const { attr, many2one, one2many, one2one } = require('mail/static/src/model/model_field.js');
+const { clear } = require('mail/static/src/model/model_field_command.js');
 
 function factory(dependencies) {
 
@@ -188,14 +189,6 @@ function factory(dependencies) {
             return this.isFolded;
         }
 
-        /**
-         * @private
-         * @returns {boolean}
-         */
-        _computeIsVisible() {
-            return this.manager.allOrderedVisible.includes(this);
-        }
-
         /**
          * @private
          * @returns {boolean}
@@ -224,12 +217,12 @@ function factory(dependencies) {
          */
         _computeVisibleIndex() {
             if (!this.manager) {
-                return undefined;
+                return clear();
             }
             const visible = this.manager.visual.visible;
             const index = visible.findIndex(visible => visible.chatWindowLocalId === this.localId);
             if (index === -1) {
-                return undefined;
+                return clear();
             }
             return index;
         }
@@ -383,17 +376,7 @@ function factory(dependencies) {
             default: false,
         }),
         /**
-         * States whether `this` is visible or not.
-         */
-        isVisible: attr({
-            compute: '_computeIsVisible',
-            dependencies: [
-                'managerAllOrderedVisible',
-            ],
-            default: false,
-        }),
-        /**
-         * Whether this chat window is visible or not. Should be considered
+         * States whether `this` is visible or not. Should be considered
          * read-only. Setting this value manually will not make it visible.
          * @see `makeVisible`
          */

addons/mail/static/src/models/discuss/discuss.js

@@ -3,6 +3,7 @@ odoo.define('mail/static/src/models/discuss.discuss.js', function (require) {
 
 const { registerNewModel } = require('mail/static/src/model/model_core.js');
 const { attr, many2one, one2many, one2one } = require('mail/static/src/model/model_field.js');
+const { clear } = require('mail/static/src/model/model_field_command.js');
 
 function factory(dependencies) {
 
@@ -235,7 +236,7 @@ function factory(dependencies) {
          */
         _computeActiveId() {
             if (!this.thread) {
-                return undefined;
+                return clear();
             }
             return this.threadToActiveId(this.thread);
         }

addons/mail/static/src/models/message/message.js

@@ -387,7 +387,7 @@ function factory(dependencies) {
          * @returns {boolean}
          */
         _computeIsCurrentPartnerAuthor() {
-            return (
+            return !!(
                 this.author &&
                 this.messagingCurrentPartner &&
                 this.messagingCurrentPartner === this.author

addons/mail/static/src/models/thread/thread.js

@@ -3,6 +3,7 @@ odoo.define('mail/static/src/models/thread/thread.js', function (require) {
 
 const { registerNewModel } = require('mail/static/src/model/model_core.js');
 const { attr, many2many, many2one, one2many, one2one } = require('mail/static/src/model/model_field.js');
+const { clear } = require('mail/static/src/model/model_field_command.js');
 const throttle = require('mail/static/src/utils/throttle/throttle.js');
 const Timer = require('mail/static/src/utils/timer/timer.js');
 const mailUtils = require('mail.utils');
@@ -1235,7 +1236,7 @@ function factory(dependencies) {
             if (
                 this.serverFoldState === this.pendingFoldState
             ) {
-                this.update({ pendingFoldState: undefined });
+                this.update({ pendingFoldState: clear() });
             }
             if (
                 this.isPendingPinned !== undefined &&
@@ -1246,7 +1247,7 @@ function factory(dependencies) {
             if (
                 this.isServerPinned === this.isPendingPinned
             ) {
-                this.update({ isPendingPinned: undefined });
+                this.update({ isPendingPinned: clear() });
             }
 
             // TODO FIXME prevent to open/close a channel on mobile when you

addons/mail/static/src/models/thread_view/thread_view.js

@@ -4,6 +4,7 @@ odoo.define('mail/static/src/models/thread_view/thread_view.js', function (requi
 const { registerNewModel } = require('mail/static/src/model/model_core.js');
 const { RecordDeletedError } = require('mail/static/src/model/model_errors.js');
 const { attr, many2many, many2one, one2one } = require('mail/static/src/model/model_field.js');
+const { clear } = require('mail/static/src/model/model_field_command.js');
 
 function factory(dependencies) {
 
@@ -77,9 +78,13 @@ function factory(dependencies) {
          */
         _computeThreadCacheInitialScrollPosition() {
             if (!this.threadCache) {
-                return;
+                return clear();
             }
-            return this.threadCacheInitialScrollPositions[this.threadCache.localId];
+            const threadCacheInitialScrollPosition = this.threadCacheInitialScrollPositions[this.threadCache.localId];
+            if (threadCacheInitialScrollPosition !== undefined) {
+                return threadCacheInitialScrollPosition;
+            }
+            return clear();
         }
 
         /**
@@ -102,7 +107,6 @@ function factory(dependencies) {
                     }
                 });
             }
-            return true;
         }
 
         /**

addons/mail/views/assets.xml

@@ -120,6 +120,7 @@
                 <script type="text/javascript" src="/mail/static/src/model/model_core.js"></script>
                 <script type="text/javascript" src="/mail/static/src/model/model_errors.js"></script>
                 <script type="text/javascript" src="/mail/static/src/model/model_field.js"></script>
+                <script type="text/javascript" src="/mail/static/src/model/model_field_command.js"></script>
                 <script type="text/javascript" src="/mail/static/src/model/model_manager.js"></script>
                 <script type="text/javascript" src="/mail/static/src/models/activity/activity.js"></script>
                 <script type="text/javascript" src="/mail/static/src/models/activity_type/activity_type.js"></script>