From 817ce7ae5a2af20bc4f84d98e2c99538a081c1af Mon Sep 17 00:00:00 2001
From: Priyanka Kakadiya <pka@odoo.com>
Date: Wed, 26 Aug 2020 07:38:43 +0000
Subject: [PATCH] [FIX] web: hide action button when no action

Before this commit: the action menu registry items were rendered
regardless of their inner state. The problem is that these item
components can choose not to render themselves, leading to a blank
dropdown menu if no other item was present.

To fix this, the "getProps" function of the registry has been altered to
act as validation: the item will not be rendered if the return value is
falsy. The function has also been made asynchronous to allow RPC calls.

This solution is still flawed and hides a bad design choice which is to
register components and not items or item getters; this API is too
complex for its use case(s) and adds pointless depth to the DOM. The
shape of the registry should be simplified in an improvement in master.

closes odoo/odoo#59259

Task: 2325587
X-original-commit: 45c7d881a5d130f6b3222749c8c87b6c6b45e910
Signed-off-by: Aaron Bohy (aab) <aab@odoo.com>
Co-authored-by: Julien Mougenot <jum@odoo.com>
Co-authored-by: Mohammed Shekha <msh@odoo.com>
---
 addons/google_drive/static/src/js/gdrive.js   |  71 ++++++------
 addons/google_drive/static/src/xml/gdrive.xml |   2 +-
 .../google_drive/static/tests/gdrive_test.js  |  35 +++++-
 .../static/src/js/components/action_menus.js  | 102 ++++++++++--------
 .../tests/components/action_menus_tests.js    |  20 ++++
 .../static/tests/helpers/test_utils_create.js |   9 ++
 6 files changed, 157 insertions(+), 82 deletions(-)

diff --git a/addons/google_drive/static/src/js/gdrive.js b/addons/google_drive/static/src/js/gdrive.js
index 03d8e798229..d6090aa9d37 100644
--- a/addons/google_drive/static/src/js/gdrive.js
+++ b/addons/google_drive/static/src/js/gdrive.js
@@ -4,6 +4,36 @@ odoo.define('google_drive.ActionMenus', function (require) {
     const ActionMenus = require('web.ActionMenus');
     const DropdownMenuItem = require('web.DropdownMenuItem');
 
+    /**
+     * Fetches the google drive action menu item props. To do so this function
+     * is given its parent props and env, as well as the RPC function bound to
+     * the parent context.
+     * Note that we use the bound RPC to benefit from its added behaviour (see
+     * web/component_extension).
+     * @param {Object} props
+     * @param {number[]} props.activeIds
+     * @param {Object} props.context
+     * @param {Object} env
+     * @param {Object} env.action The current action
+     * @param {Object} env.view The current view
+     * @param {Function} rpc Bound to the ActionMenus context
+     * @returns {Object | boolean} item props or false
+     */
+    async function googleDrivePropsGetter(props, env, rpc) {
+        const [activeId] = props.activeIds;
+        const { context } = props;
+        if (env.view.type !== "form" || !activeId) {
+            return false;
+        }
+        const items = await rpc({
+            args: [env.action.res_model, activeId],
+            context,
+            method: 'get_google_drive_config',
+            model: 'google.drive.config',
+        });
+        return Boolean(items.length) && { activeId, context, items };
+    }
+
     /**
      * Google drive menu
      *
@@ -15,32 +45,6 @@ odoo.define('google_drive.ActionMenus', function (require) {
      */
     class GoogleDriveMenu extends DropdownMenuItem {
 
-        async willStart() {
-            if (this.env.view.type === "form" && this.props.activeIds[0]) {
-                this.gdriveItems = await this._getGoogleDocItems();
-            } else {
-                this.gdriveItems = [];
-            }
-        }
-
-        //---------------------------------------------------------------------
-        // Private
-        //---------------------------------------------------------------------
-
-        /**
-         * @private
-         * @returns {Promise<Object[]>}
-         */
-        async _getGoogleDocItems() {
-            const items = await this.rpc({
-                args: [this.env.action.res_model, this.props.activeIds[0]],
-                context: this.props.context,
-                method: 'get_google_drive_config',
-                model: 'google.drive.config',
-            });
-            return items;
-        }
-
         //---------------------------------------------------------------------
         // Handlers
         //---------------------------------------------------------------------
@@ -51,7 +55,7 @@ odoo.define('google_drive.ActionMenus', function (require) {
          * @returns {Promise}
          */
         async _onGoogleDocItemClick(itemId) {
-            const resID = this.props.activeIds[0];
+            const resID = this.props.activeId;
             const domain = [['id', '=', itemId]];
             const fields = ['google_drive_resource_id', 'google_drive_client_id'];
             const configs = await this.rpc({
@@ -71,19 +75,18 @@ odoo.define('google_drive.ActionMenus', function (require) {
         }
     }
     GoogleDriveMenu.props = {
-        activeIds: Array,
+        activeId: Number,
         context: Object,
+        items: {
+            type: Array,
+            element: Object,
+        },
     };
     GoogleDriveMenu.template = 'GoogleDriveMenu';
 
     ActionMenus.registry.add('google-drive-menu', {
         Component: GoogleDriveMenu,
-        getProps(parentProps) {
-            return {
-                activeIds: parentProps.activeIds,
-                context: parentProps.context,
-            };
-        },
+        getProps: googleDrivePropsGetter,
     });
 
     return GoogleDriveMenu;
diff --git a/addons/google_drive/static/src/xml/gdrive.xml b/addons/google_drive/static/src/xml/gdrive.xml
index 48ce57494c9..d12b3459516 100644
--- a/addons/google_drive/static/src/xml/gdrive.xml
+++ b/addons/google_drive/static/src/xml/gdrive.xml
@@ -2,7 +2,7 @@
     <t t-name="GoogleDriveMenu" owl="1">
         <li>
             <ul class="o_embed_menu">
-                <li t-foreach="gdriveItems" t-as="gdriveItem" t-key="gdriveItem.id"
+                <li t-foreach="props.items" t-as="gdriveItem" t-key="gdriveItem.id"
                     role="menuitem"
                     class="o_menu_item oe_share_gdoc_item dropdown-item"
                     >
diff --git a/addons/google_drive/static/tests/gdrive_test.js b/addons/google_drive/static/tests/gdrive_test.js
index b04ebf27c26..3c684f8d614 100644
--- a/addons/google_drive/static/tests/gdrive_test.js
+++ b/addons/google_drive/static/tests/gdrive_test.js
@@ -72,10 +72,35 @@ odoo.define('google_drive.gdrive_integration', function (require) {
             form.destroy();
         });
 
+        QUnit.test("no google drive data", async function (assert) {
+            assert.expect(1);
+
+            const form = await testUtils.createView({
+                actionMenusRegistry: true,
+                arch:
+                    `<form string="Partners">
+                        <field name="display_name"/>
+                    </form>`,
+                data: this.data,
+                model: 'partner',
+                res_id: 1,
+                View: FormView,
+                viewOptions: {
+                    hasActionMenus: true,
+                    ids: [1, 2],
+                    index: 0,
+                },
+            });
+
+            assert.containsNone(form, ".o_cp_action_menus .o_embed_menu");
+
+            form.destroy();
+        });
+
         QUnit.test('click on the google drive attachments after switching records', async function (assert) {
             assert.expect(4);
 
-            let currentID;
+            let currentRecordId = 1;
             const form = await testUtils.createView({
                 actionMenusRegistry: true,
                 arch:
@@ -86,7 +111,7 @@ odoo.define('google_drive.gdrive_integration', function (require) {
                 async mockRPC(route, args) {
                     switch (route) {
                         case '/web/dataset/call_kw/google.drive.config/get_google_drive_config':
-                            assert.deepEqual(args.args, ['partner', currentID],
+                            assert.deepEqual(args.args, ['partner', currentRecordId],
                                 'The route to get google drive config should have been called');
                             return [{
                                 id: 27,
@@ -99,7 +124,7 @@ odoo.define('google_drive.gdrive_integration', function (require) {
                                 id: 1,
                             }];
                         case '/web/dataset/call_kw/google.drive.config/get_google_drive_url':
-                            assert.deepEqual(args.args, [27, currentID, 'T1000'],
+                            assert.deepEqual(args.args, [27, currentRecordId, 'T1000'],
                                 'The route to get the Google url should have been called');
                             return; // do not return anything or it will open a new tab.
                     }
@@ -113,13 +138,13 @@ odoo.define('google_drive.gdrive_integration', function (require) {
                     index: 0,
                 },
             });
-            currentID = 1;
+
             await cpHelpers.toggleActionMenu(form);
             await cpHelpers.toggleMenuItem(form, "Cyberdyne Systems");
 
+            currentRecordId = 2;
             await cpHelpers.pagerNext(form);
 
-            currentID = 2;
             await cpHelpers.toggleActionMenu(form);
             await cpHelpers.toggleMenuItem(form, "Cyberdyne Systems");
 
diff --git a/addons/web/static/src/js/components/action_menus.js b/addons/web/static/src/js/components/action_menus.js
index 4427ac5c948..ed95e6663fa 100644
--- a/addons/web/static/src/js/components/action_menus.js
+++ b/addons/web/static/src/js/components/action_menus.js
@@ -7,6 +7,8 @@ odoo.define('web.ActionMenus', function (require) {
 
     const { Component } = owl;
 
+    let registryActionId = 1;
+
     /**
      * Action menus (or Action/Print bar, previously called 'Sidebar')
      *
@@ -21,6 +23,16 @@ odoo.define('web.ActionMenus', function (require) {
      */
     class ActionMenus extends Component {
 
+        async willStart() {
+            this.actionItems = await this._setActionItems(this.props);
+            this.printItems = await this._setPrintItems(this.props);
+        }
+
+        async willUpdateProps(nextProps) {
+            this.actionItems = await this._setActionItems(nextProps);
+            this.printItems = await this._setPrintItems(nextProps);
+        }
+
         mounted() {
             this._addTooltips();
         }
@@ -29,52 +41,12 @@ odoo.define('web.ActionMenus', function (require) {
             this._addTooltips();
         }
 
-        //---------------------------------------------------------------------
-        // Getters
-        //---------------------------------------------------------------------
-
-        /**
-         * @returns {Object[]}
-         */
-        get actionItems() {
-            // Callback based actions
-            const callbackActions = (this.props.items.other || []).map(
-                action => Object.assign({ key: `action-${action.description}` }, action)
-            );
-            // Action based actions
-            const actionActions = this.props.items.action || [];
-            const relateActions = this.props.items.relate || [];
-            const formattedActions = [...actionActions, ...relateActions].map(
-                action => ({ action, description: action.name, key: action.id })
-            );
-            // ActionMenus action registry components
-            const registryActions = this.constructor.registry.values().map(
-                ({ Component, getProps }, index) => ({
-                    key: `registry-action-${index}`,
-                    Component,
-                    props: getProps(this.props),
-                })
-            );
-            return [...callbackActions, ...formattedActions, ...registryActions];
-        }
-
-        /**
-         * @returns {Object[]}
-         */
-        get printItems() {
-            const printActions = this.props.items.print || [];
-            const printItems = printActions.map(
-                action => ({ action, description: action.name, key: action.id })
-            );
-            return printItems;
-        }
-
         //---------------------------------------------------------------------
         // Private
         //---------------------------------------------------------------------
 
         /**
-         * Add teh tooltips to the items
+         * Add the tooltips to the items
          * @private
          */
         _addTooltips() {
@@ -83,6 +55,52 @@ odoo.define('web.ActionMenus', function (require) {
             });
         }
 
+        /**
+         * @private
+         * @param {Object} props
+         * @returns {Promise<Object[]>}
+         */
+        async _setActionItems(props) {
+            // Callback based actions
+            const callbackActions = (props.items.other || []).map(
+                action => Object.assign({ key: `action-${action.description}` }, action)
+            );
+            // Action based actions
+            const actionActions = props.items.action || [];
+            const relateActions = props.items.relate || [];
+            const formattedActions = [...actionActions, ...relateActions].map(
+                action => ({ action, description: action.name, key: action.id })
+            );
+            // ActionMenus action registry components
+            const registryActions = [];
+            const rpc = this.rpc.bind(this);
+            for (const { Component, getProps } of this.constructor.registry.values()) {
+                const itemProps = await getProps(props, this.env, rpc);
+                if (itemProps) {
+                    registryActions.push({
+                        Component,
+                        key: `registry-action-${registryActionId++}`,
+                        props: itemProps,
+                    });
+                }
+            }
+
+            return [...callbackActions, ...formattedActions, ...registryActions];
+        }
+
+        /**
+         * @private
+         * @param {Object} props
+         * @returns {Promise<Object[]>}
+         */
+        async _setPrintItems(props) {
+            const printActions = props.items.print || [];
+            const printItems = printActions.map(
+                action => ({ action, description: action.name, key: action.id })
+            );
+            return printItems;
+        }
+
         //---------------------------------------------------------------------
         // Handlers
         //---------------------------------------------------------------------
@@ -160,7 +178,7 @@ odoo.define('web.ActionMenus', function (require) {
 
     ActionMenus.components = { DropdownMenu };
     ActionMenus.props = {
-        activeIds: { type: Array, element: [Number | String] }, // virtual IDs are strings.
+        activeIds: { type: Array, element: [Number, String] }, // virtual IDs are strings.
         context: Object,
         domain: { type: Array, optional: 1 },
         isDomainSelected: { type: Boolean, optional: 1 },
diff --git a/addons/web/static/tests/components/action_menus_tests.js b/addons/web/static/tests/components/action_menus_tests.js
index e17ae95ba78..d1df0d59e96 100644
--- a/addons/web/static/tests/components/action_menus_tests.js
+++ b/addons/web/static/tests/components/action_menus_tests.js
@@ -5,6 +5,7 @@ odoo.define('web.action_menus_tests', function (require) {
     const Registry = require('web.Registry');
     const testUtils = require('web.test_utils');
 
+    const { Component } = owl;
     const cpHelpers = testUtils.controlPanel;
     const { createComponent } = testUtils;
 
@@ -86,6 +87,25 @@ odoo.define('web.action_menus_tests', function (require) {
             actionMenus.destroy();
         });
 
+        QUnit.test("empty action menus", async function (assert) {
+            assert.expect(1);
+
+            ActionMenus.registry.add("test", { Component, getProps: () => false });
+            this.props.items = {};
+
+            const actionMenus = await createComponent(ActionMenus, {
+                env: {
+                    action: this.action,
+                    view: this.view,
+                },
+                props: this.props,
+            });
+
+            assert.containsNone(actionMenus, ".o_cp_action_menus > *");
+
+            actionMenus.destroy();
+        });
+
         QUnit.test('execute action', async function (assert) {
             assert.expect(4);
 
diff --git a/addons/web/static/tests/helpers/test_utils_create.js b/addons/web/static/tests/helpers/test_utils_create.js
index 86fc01b5a4c..908ac97aa62 100644
--- a/addons/web/static/tests/helpers/test_utils_create.js
+++ b/addons/web/static/tests/helpers/test_utils_create.js
@@ -71,10 +71,19 @@ odoo.define('web.test_utils_create', function (require) {
         const userContext = params.context && params.context.user_context || {};
         const actionManager = new ActionManager(widget, userContext);
 
+        // Override the ActionMenus registry unless told otherwise.
+        let actionMenusRegistry = ActionMenus.registry;
+        if (params.actionMenusRegistry !== true) {
+            ActionMenus.registry = new Registry();
+        }
+
         const originalDestroy = ActionManager.prototype.destroy;
         actionManager.destroy = function () {
             actionManager.destroy = originalDestroy;
             widget.destroy();
+            if (params.actionMenusRegistry !== true) {
+                ActionMenus.registry = actionMenusRegistry;
+            }
         };
         const fragment = document.createDocumentFragment();
         await actionManager.appendTo(fragment);