From 6e45c0fd2273ffce73013c6a409a5fb77799a32b Mon Sep 17 00:00:00 2001 From: Pierre Rousseau Date: Fri, 24 Feb 2023 13:00:05 +0000 Subject: [PATCH] [FIX] spreadsheet: preload list record limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Steps to reproduce: - Insert a list view in a spreadsheet - Cut & Paste the entire list to a new sheet - On the first sheet, insert a formula, `=SUM()` which sum many records of the list - Click on `Clear history` in the file menu (to force a snapshot and a reload) => There are one RPC by number of records in the SUM function, with the limit increased by 1 each time. The record limit (the bigger record index) is computed during the evaluation. With a normal case, the evaluation goes through all the cells of the viewport, collect the biggest record index, and trigger the rpc after that. However, in our usecase, the evaluation is not passed on the second sheet (the one which contains the list view), so the limit is not computed. When we evaluate the arguments of the `SUM` function, we evaluate each argument one by one. So we evaluate a `ODOO.LIST` formula, the limit is lower than the previous limit, so the function returns an error. But when evaluating the arguments of a function, we stop the evaluation as soon as we find an error. So the evaluation of the `SUM` function is stopped, and the limit is not entirely computed. So the first RPC will have a limit 1, then the evaluation is re-run, a second RPC is triggered with a limit 2, and so on. This commit fixes the issue by preloading the limit during the import of the data and during an UPDATE_CELL. opw-3165458 closes odoo/odoo#113789 X-original-commit: 1c5a05198542b3a40c97443b2a9ecf5fb32eb5f2 Signed-off-by: Lucas Lefèvre (lul) Signed-off-by: Pierre Rousseau (pro) --- .../static/src/list/list_data_source.js | 20 ++++++--- .../src/list/plugins/list_core_plugin.js | 43 +++++++++++++++++++ .../static/tests/lists/list_plugin_test.js | 42 +++++++++++++++--- 3 files changed, 95 insertions(+), 10 deletions(-) diff --git a/addons/spreadsheet/static/src/list/list_data_source.js b/addons/spreadsheet/static/src/list/list_data_source.js index e41cc567baf..9a1c6036766 100644 --- a/addons/spreadsheet/static/src/list/list_data_source.js +++ b/addons/spreadsheet/static/src/list/list_data_source.js @@ -35,13 +35,22 @@ export default class ListDataSource extends OdooViewsDataSource { */ constructor(services, params) { super(services, params); - this.limit = params.limit; + this.maxPosition = params.limit; + this.maxPositionFetched = 0; this.data = []; } + /** + * Increase the max position of the list + * @param {number} position + */ + increaseMaxPosition(position) { + this.maxPosition = Math.max(this.maxPosition, position); + } + async _load() { await super._load(); - if (this.limit === 0) { + if (this.maxPosition === 0) { this.data = []; return; } @@ -52,10 +61,11 @@ export default class ListDataSource extends OdooViewsDataSource { this._getFieldsToFetch(), { order: orderByToString(orderBy), - limit: this.limit, + limit: this.maxPosition, context, } ); + this.maxPositionFetched = this.maxPosition; } /** @@ -99,8 +109,8 @@ export default class ListDataSource extends OdooViewsDataSource { */ getListCellValue(position, fieldName) { this._assertDataIsLoaded(); - if (position >= this.limit) { - this.limit = position + 1; + if (position >= this.maxPositionFetched) { + this.increaseMaxPosition(position + 1); // A reload is needed because the asked position is not already loaded. this._triggerFetching(); throw new LoadingDataError(); diff --git a/addons/spreadsheet/static/src/list/plugins/list_core_plugin.js b/addons/spreadsheet/static/src/list/plugins/list_core_plugin.js index 685134bf975..d862af5aaf2 100644 --- a/addons/spreadsheet/static/src/list/plugins/list_core_plugin.js +++ b/addons/spreadsheet/static/src/list/plugins/list_core_plugin.js @@ -9,6 +9,7 @@ import { _t } from "@web/core/l10n/translation"; import { globalFiltersFieldMatchers } from "@spreadsheet/global_filters/plugins/global_filters_core_plugin"; import { sprintf } from "@web/core/utils/strings"; import { checkFilterFieldMatching } from "@spreadsheet/global_filters/helpers"; +import { getFirstListFunction, getNumberOfListFormulas } from "../list_helpers"; /** * @typedef {Object} ListDefinition @@ -143,9 +144,51 @@ export default class ListCorePlugin extends CorePlugin { case "REMOVE_GLOBAL_FILTER": this._onFilterDeletion(cmd.id); break; + + case "START": + for (const sheetId of this.getters.getSheetIds()) { + const cells = this.getters.getCells(sheetId); + for (const cell of Object.values(cells)) { + if (cell.isFormula) { + this._addListPositionToDataSource(cell.content); + } + } + } + break; + case "UPDATE_CELL": + if (cmd.content) { + this._addListPositionToDataSource(cmd.content); + } + break; } } + /** + * Extract the position of the records asked in the given formula and + * increase the max position of the corresponding data source. + * + * @param {string} content Odoo list formula + */ + _addListPositionToDataSource(content) { + if (getNumberOfListFormulas(content) !== 1) { + return; + } + const { functionName, args } = getFirstListFunction(content); + if (functionName !== "ODOO.LIST") { + return; + } + const [listId, positionArg] = args.map((arg) => arg.value.toString()); + if (!(listId in this.lists)) { + return; + } + const position = parseInt(positionArg, 10); + if (isNaN(position)) { + return; + } + const dataSourceId = this.lists[listId].dataSourceId; + this.dataSources.get(dataSourceId).increaseMaxPosition(position); + } + // ------------------------------------------------------------------------- // Getters // ------------------------------------------------------------------------- diff --git a/addons/spreadsheet/static/tests/lists/list_plugin_test.js b/addons/spreadsheet/static/tests/lists/list_plugin_test.js index b6329bdbda2..b5fb44cdba7 100644 --- a/addons/spreadsheet/static/tests/lists/list_plugin_test.js +++ b/addons/spreadsheet/static/tests/lists/list_plugin_test.js @@ -137,7 +137,7 @@ QUnit.module("spreadsheet > list plugin", {}, () => { const { model } = await createSpreadsheetWithList({ linesNumber: 2 }); const [listId] = model.getters.getListIds(); const dataSource = model.getters.getListDataSource(listId); - assert.strictEqual(dataSource.limit, 2); + assert.strictEqual(dataSource.maxPosition, 2); }); QUnit.test("can select a List from cell formula within a formula", async function (assert) { @@ -246,13 +246,9 @@ QUnit.module("spreadsheet > list plugin", {}, () => { model.dispatch("ACTIVATE_SHEET", { sheetIdFrom: "sheet1", sheetIdTo: "sheet2" }); /* * Ask a first time the value => It will trigger a loading of the data source. - * As the list index limit is to 0, there is loading. */ assert.equal(getCellValue(model, "A1"), "Loading..."); await nextTick(); - // Ask a second time the value => The list index limit is raised => reload - assert.equal(getCellValue(model, "A1"), "Loading..."); - await nextTick(); assert.equal(getCellValue(model, "A1"), 12); assert.verifySteps(["partner/fields_get", "partner/search_read"]); }); @@ -508,6 +504,42 @@ QUnit.module("spreadsheet > list plugin", {}, () => { }); }); + QUnit.test( + "List record limit is computed during the import and UPDATE_CELL", + async function (assert) { + const spreadsheetData = { + sheets: [ + { + id: "sheet1", + cells: { + A1: { content: `=ODOO.LIST("1", "1", "foo")` }, + }, + }, + ], + lists: { + 1: { + id: 1, + columns: ["foo", "contact_name"], + domain: [], + model: "partner", + orderBy: [], + context: {}, + }, + }, + }; + const model = await createModelWithDataSource({ spreadsheetData }); + const ds = model.getters.getListDataSource("1"); + assert.strictEqual(ds.maxPosition, 1); + assert.strictEqual(ds.maxPositionFetched, 0); + setCellContent(model, "A1", `=ODOO.LIST("1", "42", "foo", 2)`); + assert.strictEqual(ds.maxPosition, 42); + assert.strictEqual(ds.maxPositionFetched, 0); + await waitForDataSourcesLoaded(model); + assert.strictEqual(ds.maxPosition, 42); + assert.strictEqual(ds.maxPositionFetched, 42); + } + ); + QUnit.test( "Load list spreadsheet with models that cannot be accessed", async function (assert) {