[FIX] web: add view id in optional fields local storage key

Since the renderer does not know the view id, we moved the local
storage key computation up to the basic controller, which makes
more sense since the renderer is supposed to be inert.

The _processColumns function had to be moved in willStart however,
since it now needs the controller to respond to the trigger_up
and the controller is not present during the init of the renderer.

closes odoo/odoo#34051

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

addons/web/static/src/js/fields/relational_fields.js

@@ -856,6 +856,8 @@ var FieldX2Many = AbstractField.extend({
         toggle_column_order: '_onToggleColumnOrder',
         activate_next_widget: '_onActiveNextWidget',
         navigation_move: '_onNavigationMove',
+        save_optional_fields: '_onSaveOrLoadOptionalFields',
+        load_optional_fields: '_onSaveOrLoadOptionalFields',
     }),
 
     // We need to trigger the reset on every changes to be aware of the parent changes
@@ -1387,6 +1389,18 @@ var FieldX2Many = AbstractField.extend({
             });
         });
     },
+    /**
+     * Add necessary key parts for the basic controller to compute the local
+     * storage key. The event will be properly handled by the basic controller.
+     *
+     * @param {OdooEvent} ev
+     * @private
+     */
+    _onSaveOrLoadOptionalFields: function (ev) {
+        ev.data.keyParts.relationalField = this.name;
+        ev.data.keyParts.subViewId = this.view.view_id;
+        ev.data.keyParts.subViewType = this.view.type;
+    },
     /**
      * Forces a resequencing of the records.
      *

addons/web/static/src/js/views/basic/basic_controller.js

@@ -21,6 +21,8 @@ var BasicController = AbstractController.extend(FieldManagerMixin, {
         reload: '_onReload',
         resequence_records: '_onResequenceRecords',
         set_dirty: '_onSetDirty',
+        load_optional_fields: '_onLoadOptionalFields',
+        save_optional_fields: '_onSaveOptionalFields',
         sidebar_data_asked: '_onSidebarDataAsked',
         translate: '_onTranslate',
     }),
@@ -43,6 +45,7 @@ var BasicController = AbstractController.extend(FieldManagerMixin, {
         // operations to complete before checking if there are changes to
         // discard when discardChanges is called
         this.savingDef = Promise.resolve();
+        this.viewId = params.viewId;
     },
     /**
      * @override
@@ -418,6 +421,49 @@ var BasicController = AbstractController.extend(FieldManagerMixin, {
             resIds: env.ids,
         });
     },
+    /**
+     * Compute the optional fields local storage key using the given parts.
+     *
+     * @param {Object} keyParts
+     * @param {string} keyParts.viewType view type
+     * @param {string} [keyParts.relationalField] name of the field with subview
+     * @param {integer} [keyParts.subViewId] subview id
+     * @param {string} [keyParts.subViewType] type of the subview
+     * @param {Object} keyParts.fields fields
+     * @param {string} keyParts.fields.name field name
+     * @param {string} keyParts.fields.type field type
+     * @returns {string} local storage key for optional fields in this view
+     * @private
+     */
+    _getOptionalFieldsLocalStorageKey: function (keyParts) {
+        keyParts.model = this.modelName;
+        keyParts.viewType = this.viewType;
+        keyParts.viewId = this.viewId;
+
+        var parts = [
+            'model',
+            'viewType',
+            'viewId',
+            'relationalField',
+            'subViewType',
+            'subViewId',
+        ];
+
+        var viewIdentifier = parts.reduce(function (identifier, partName) {
+            if (partName in keyParts) {
+                return identifier + ',' + keyParts[partName];
+            }
+            return identifier;
+        }, 'optional_fields');
+
+        viewIdentifier =
+            keyParts.fields.sort(this._nameSortComparer)
+                           .reduce(function (identifier, field) {
+                                return identifier + ',' + field.name;
+                            }, viewIdentifier);
+
+        return viewIdentifier;
+    },
     /**
      * Return the params (current_min, limit and size) to pass to the pager,
      * according to the current state.
@@ -457,6 +503,16 @@ var BasicController = AbstractController.extend(FieldManagerMixin, {
     _isPagerVisible: function () {
         return true;
     },
+    /**
+     *  Sort function used to sort the fields by names, to compute the optional fields keys
+     *
+     *  @param {Object} left
+     *  @param {Object} right
+     *  @private
+      */
+    _nameSortComparer: function(left, right) {
+        return left.name < right.name ? -1 : 1;
+    },
     /**
      * Helper function to display a warning that some fields have an invalid
      * value. This is used when a save operation cannot be completed.
@@ -675,6 +731,39 @@ var BasicController = AbstractController.extend(FieldManagerMixin, {
             },
         });
     },
+    /**
+     * Load the optional columns settings in local storage for this view
+     *
+     * @param {OdooEvent} ev
+     * @param {Object} ev.data.keyParts see _getLocalStorageKey
+     * @param {function} ev.data.callback function to call with the result
+     * @private
+     */
+    _onLoadOptionalFields: function (ev) {
+        var res = this.call(
+            'local_storage',
+            'getItem',
+            this._getOptionalFieldsLocalStorageKey(ev.data.keyParts)
+        );
+        ev.data.callback(res);
+    },
+    /**
+     * Save the optional columns settings in local storage for this view
+     *
+     * @param {OdooEvent} ev
+     * @param {Object} ev.data.keyParts see _getLocalStorageKey
+     * @param {Array<string>} ev.data.optionalColumnsEnabled list of optional
+     *   field names that have been enabled
+     * @private
+     */
+    _onSaveOptionalFields: function (ev) {
+        this.call(
+            'local_storage',
+            'setItem',
+            this._getOptionalFieldsLocalStorageKey(ev.data.keyParts),
+            ev.data.optionalColumnsEnabled
+        );
+    },
     /**
      * @private
      * @param {OdooEvent} ev

addons/web/static/src/js/views/basic/basic_view.js

@@ -48,6 +48,7 @@ var BasicView = AbstractView.extend({
         this.controllerParams.archiveEnabled = 'active' in this.fields;
         this.controllerParams.hasButtons =
                 'action_buttons' in params ? params.action_buttons : true;
+        this.controllerParams.viewId = viewInfo.view_id;
 
         this.loadParams.fieldsInfo = this.fieldsInfo;
         this.loadParams.fields = this.fields;

addons/web/static/src/js/views/list/list_renderer.js

@@ -69,7 +69,16 @@ var ListRenderer = BasicRenderer.extend({
         this.editable = params.editable;
         this.isGrouped = this.state.groupedBy.length > 0;
         this.groupbys = params.groupbys;
+    },
+    /**
+     * Compute columns visilibity. This can't be done earlier as we need the
+     * controller to respond to the load_optional_fields call of processColumns.
+     *
+     * @override
+     */
+    willStart: function() {
         this._processColumns(this.columnInvisibleFields || {});
+        return this._super.apply(this, arguments);
     },
 
     //--------------------------------------------------------------------------
@@ -240,14 +249,13 @@ var ListRenderer = BasicRenderer.extend({
      * @private
      * @returns {string}
      */
-    _getOptionalColumnsStorageKey: function () {
+    _getOptionalColumnsStorageKeyParts: function () {
         var self = this;
-        var fields = [];
-        _.each(this.state.fieldsInfo[this.viewType], function (field, name) {
-            fields.push(name + ":" + self.state.fields[name].type);
-        });
-        fields.sort();
-        return "list_optional_fields," + this.state.model + "," + this.viewType + "," + fields.join(',');
+        return {
+            fields: _.map(this.state.fieldsInfo[this.viewType], function (_, fieldName) {
+                return {name: fieldName, type: self.state.fields[fieldName].type};
+            }),
+        }
     },
     /**
      * Removes the columns which should be invisible.
@@ -260,8 +268,13 @@ var ListRenderer = BasicRenderer.extend({
         this.columns = [];
         this.optionalColumns = [];
         this.optionalColumnsEnabled = [];
-        var localStorageKey = this._getOptionalColumnsStorageKey();
-        var storedOptionalColumns = this.call('local_storage', 'getItem', localStorageKey);
+        var storedOptionalColumns;
+        this.trigger_up('load_optional_fields', {
+            keyParts: this._getOptionalColumnsStorageKeyParts(),
+            callback: function(res) {
+                storedOptionalColumns = res;
+            },
+        });
         _.each(this.arch.children, function (c) {
             if (c.tag !== 'control' && c.tag !== 'groupby') {
                 var reject = c.attrs.modifiers.column_invisible;
@@ -1030,7 +1043,10 @@ var ListRenderer = BasicRenderer.extend({
         } else {
             this.optionalColumnsEnabled.push(input.name);
         }
-        this.call('local_storage', 'setItem', this._getOptionalColumnsStorageKey(), this.optionalColumnsEnabled);
+        this.trigger_up('save_optional_fields', {
+            keyParts: this._getOptionalColumnsStorageKeyParts(),
+            optionalColumnsEnabled: this.optionalColumnsEnabled,
+        });
         this._processColumns(this.columnInvisibleFields || {});
         this._renderView().then(function() {
             self._onToggleOptionalColumnDropdown(ev);

addons/web/static/tests/views/form_tests.js

@@ -1,6 +1,7 @@
 odoo.define('web.form_tests', function (require) {
 "use strict";
 
+var AbstractStorageService = require('web.AbstractStorageService');
 var BasicModel = require('web.BasicModel');
 var concurrency = require('web.concurrency');
 var core = require('web.core');
@@ -9,6 +10,7 @@ var FormView = require('web.FormView');
 var mixins = require('web.mixins');
 var NotificationService = require('web.NotificationService');
 var pyUtils = require('web.py_utils');
+var RamStorage = require('web.RamStorage');
 var testUtils = require('web.test_utils');
 var widgetRegistry = require('web.widget_registry');
 var Widget = require('web.Widget');
@@ -7924,6 +7926,161 @@ QUnit.module('Views', {
         form.destroy();
     });
 
+    QUnit.test('form view with inline tree view with optional fields and local storage mock', async function (assert) {
+        assert.expect(12);
+
+        var Storage = RamStorage.extend({
+            getItem: function (key) {
+                assert.step('getItem ' + key);
+                return this._super.apply(this, arguments);
+            },
+            setItem: function (key, value) {
+                assert.step('setItem ' + key + ' to ' + value);
+                return this._super.apply(this, arguments);
+            },
+        });
+
+        var RamStorageService = AbstractStorageService.extend({
+            storage: new Storage(),
+        });
+
+        var form = await createView({
+            View: FormView,
+            model: 'partner',
+            data: this.data,
+            arch: '<form string="Partners">' +
+                    '<field name="qux"/>' +
+                    '<field name="p">' +
+                        '<tree>' +
+                            '<field name="foo"/>' +
+                            '<field name="bar" optional="hide"/>' +
+                        '</tree>' +
+                    '</field>' +
+                '</form>',
+            services: {
+                local_storage: RamStorageService,
+            },
+            view_id: 27,
+        });
+
+        var localStorageKey = 'optional_fields,partner,form,27,p,list,undefined,bar,foo';
+
+        assert.verifySteps(['getItem ' + localStorageKey]);
+
+        assert.containsN(form, 'th', 2,
+            "should have 2 th, 1 for selector, 1 for foo column");
+
+        assert.ok(form.$('th:contains(Foo)').is(':visible'),
+            "should have a visible foo field");
+
+        assert.notOk(form.$('th:contains(Bar)').is(':visible'),
+            "should not have a visible bar field");
+
+        // optional fields
+        await testUtils.dom.click(form.$('table .o_optional_columns_dropdown_toggle'));
+        assert.containsN(form, 'div.o_optional_columns div.dropdown-item', 1,
+            "dropdown have 1 optional field");
+
+        // enable optional field
+        await testUtils.dom.click(form.$('div.o_optional_columns div.dropdown-item input'));
+
+        assert.verifySteps([
+            'setItem ' + localStorageKey + ' to ["bar"]',
+            'getItem ' + localStorageKey,
+        ]);
+
+        assert.containsN(form, 'th', 3,
+            "should have 3 th, 1 for selector, 2 for columns");
+
+        assert.ok(form.$('th:contains(Foo)').is(':visible'),
+            "should have a visible foo field");
+
+        assert.ok(form.$('th:contains(Bar)').is(':visible'),
+            "should have a visible bar field");
+
+        form.destroy();
+    });
+
+    QUnit.test('form view with tree_view_ref with optional fields and local storage mock', async function (assert) {
+        assert.expect(12);
+
+        var Storage = RamStorage.extend({
+            getItem: function (key) {
+                assert.step('getItem ' + key);
+                return this._super.apply(this, arguments);
+            },
+            setItem: function (key, value) {
+                assert.step('setItem ' + key + ' to ' + value);
+                return this._super.apply(this, arguments);
+            },
+        });
+
+        var RamStorageService = AbstractStorageService.extend({
+            storage: new Storage(),
+        });
+
+        var form = await createView({
+            View: FormView,
+            model: 'partner',
+            data: this.data,
+            arch: '<form string="Partners">' +
+                    '<field name="qux"/>' +
+                    '<field name="p" context="{\'tree_view_ref\': \'34\'}"/>' +
+                '</form>',
+            archs: {
+                "partner,nope_not_this_one,list": '<tree>' +
+                    '<field name="foo"/>' +
+                    '<field name="bar"/>' +
+                    '</tree>',
+                "partner,34,list": '<tree>' +
+                        '<field name="foo" optional="hide"/>' +
+                        '<field name="bar"/>' +
+                    '</tree>',
+            },
+            services: {
+                local_storage: RamStorageService,
+            },
+            view_id: 27,
+        });
+
+        var localStorageKey = 'optional_fields,partner,form,27,p,list,34,bar,foo';
+
+        assert.verifySteps(['getItem ' + localStorageKey]);
+
+        assert.containsN(form, 'th', 2,
+            "should have 2 th, 1 for selector, 1 for foo column");
+
+        assert.notOk(form.$('th:contains(Foo)').is(':visible'),
+            "should have a visible foo field");
+
+        assert.ok(form.$('th:contains(Bar)').is(':visible'),
+            "should not have a visible bar field");
+
+        // optional fields
+        await testUtils.dom.click(form.$('table .o_optional_columns_dropdown_toggle'));
+        assert.containsN(form, 'div.o_optional_columns div.dropdown-item', 1,
+            "dropdown have 1 optional field");
+
+        // enable optional field
+        await testUtils.dom.click(form.$('div.o_optional_columns div.dropdown-item input'));
+
+        assert.verifySteps([
+            'setItem ' + localStorageKey + ' to ["foo"]',
+            'getItem ' + localStorageKey,
+        ]);
+
+        assert.containsN(form, 'th', 3,
+            "should have 3 th, 1 for selector, 2 for columns");
+
+        assert.ok(form.$('th:contains(Foo)').is(':visible'),
+            "should have a visible foo field");
+
+        assert.ok(form.$('th:contains(Bar)').is(':visible'),
+            "should have a visible bar field");
+
+        form.destroy();
+    });
+
     QUnit.module('FormViewTABMainButtons');
 
     QUnit.test('using tab in an empty required string field should not move to the next field', async function(assert) {

addons/web/static/tests/views/list_tests.js

@@ -6397,7 +6397,7 @@ QUnit.module('Views', {
         list.destroy();
     });
 
-    QUnit.test('optinal fields do not disappear even after listview reload', async function (assert) {
+    QUnit.test('optional fields do not disappear even after listview reload', async function (assert) {
         assert.expect(7);
 
         var RamStorageService = AbstractStorageService.extend({
@@ -6596,9 +6596,12 @@ QUnit.module('Views', {
             services: {
                 local_storage: RamStorageService,
             },
+            view_id: 42,
         });
 
-        assert.verifySteps(['getItem list_optional_fields,foo,list,foo:char,m2o:many2one,reference:reference']);
+        var localStorageKey = 'optional_fields,foo,list,42,foo,m2o,reference';
+
+        assert.verifySteps(['getItem ' + localStorageKey]);
 
         assert.containsN(list, 'th', 3,
             "should have 3 th, 1 for selector, 2 for columns");
@@ -6619,8 +6622,8 @@ QUnit.module('Views', {
         await testUtils.dom.click(list.$('div.o_optional_columns div.dropdown-item:eq(1) input'));
 
         assert.verifySteps([
-            'setItem list_optional_fields,foo,list,foo:char,m2o:many2one,reference:reference to ["m2o","reference"]',
-            'getItem list_optional_fields,foo,list,foo:char,m2o:many2one,reference:reference',
+            'setItem ' + localStorageKey + ' to ["m2o","reference"]',
+            'getItem ' + localStorageKey,
         ]);
 
         // 4 th (1 for checkbox, 3 for columns)