From 180ddd6da2cf70830d590972ba01c2f21d03bf69 Mon Sep 17 00:00:00 2001 From: Pierre Rousseau Date: Wed, 14 Dec 2022 10:16:15 +0000 Subject: [PATCH] [FIX] spreadsheet: autoresize pivot table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before this revision, the pivot table was not autoresized when group bys contains a many2one or a many2many field as it required an extra request to get the name_get of the values. With this revision, the values of the relational fields are extracted from the pivot model in order to save an extra request. Note: this fix is not ideal as it introduce a dependency between the pivot model and the server data source. However, it is the best solution in a stable version. Task-id 2928601 closes odoo/odoo#108527 X-original-commit: d6a1aa63702132ffc59d2412c2a009912eba797a Signed-off-by: Lucas Lefèvre (lul) --- .../data_sources/display_name_repository.js | 200 ++++++++++++++++++ .../src/data_sources/labels_repository.js | 54 +++++ .../src/data_sources/metadata_repository.js | 43 ++-- .../static/src/pivot/pivot_model.js | 6 + .../tests/pivots/model/pivot_plugin_test.js | 12 +- 5 files changed, 281 insertions(+), 34 deletions(-) create mode 100644 addons/spreadsheet/static/src/data_sources/display_name_repository.js create mode 100644 addons/spreadsheet/static/src/data_sources/labels_repository.js diff --git a/addons/spreadsheet/static/src/data_sources/display_name_repository.js b/addons/spreadsheet/static/src/data_sources/display_name_repository.js new file mode 100644 index 00000000000..b8508f85851 --- /dev/null +++ b/addons/spreadsheet/static/src/data_sources/display_name_repository.js @@ -0,0 +1,200 @@ +/** @odoo-module */ + +import { Deferred } from "@web/core/utils/concurrency"; +import { LoadingDataError } from "../o_spreadsheet/errors"; +import BatchEndpoint, { Request } from "./server_data"; + +/** + * @typedef PendingDisplayName + * @property {"PENDING"} state + * @property {Deferred} deferred + * + * @typedef ErrorDisplayName + * @property {"ERROR"} state + * @property {Deferred} deferred + * @property {Error} error + * + * @typedef CompletedDisplayName + * @property {"COMPLETED"} state + * @property {Deferred} deferred + * @property {string|undefined} value + * + * @typedef {PendingDisplayName | ErrorDisplayName | CompletedDisplayName} DisplayNameResult + * + * @typedef {[number, string]} BatchedNameGetRPCResult + */ + +/** + * This class is responsible for fetching the display names of records. It + * caches the display names of records that have already been fetched. + * It also provides a way to wait for the display name of a record to be + * fetched. + */ +export class DisplayNameRepository { + /** + * + * @param {import("@web/core/orm_service").ORM} orm + * @param {Object} params + * @param {function} params.whenDataIsFetched Callback to call when the + * display name of a record is fetched. + */ + constructor(orm, { whenDataIsFetched }) { + this.dataFetchedCallback = whenDataIsFetched; + /** + * Contains the display names of records. It's organized in the following way: + * { + * "res.country": { + * 1: { + * "value": "Belgium", + * "deferred": Deferred<"Belgium">, + * }, + * } + */ + /** @type {Object.>}*/ + this._displayNames = {}; + this._orm = orm; + this._endpoints = {}; + } + + /** + * Get the display name of the given record. + * + * @param {string} model + * @param {number} id + * @returns {Promise} + */ + async getDisplayNameAsync(model, id) { + const displayNameResult = this._displayNames[model] && this._displayNames[model][id]; + if (!displayNameResult) { + return this._fetchDisplayName(model, id); + } + return displayNameResult.deferred; + } + + /** + * Set the display name of the given record. This will prevent the display name + * from being fetched in the background. + * + * @param {string} model + * @param {number} id + * @param {string} displayName + */ + setDisplayName(model, id, displayName) { + if (!this._displayNames[model]) { + this._displayNames[model] = {}; + } + this._displayNames[model][id] = { + state: "COMPLETED", + deferred: new Deferred().resolve(displayName), + value: displayName, + }; + } + + /** + * Get the display name of the given record. If the record does not exist, + * it will throw a LoadingDataError and fetch the display name in the background. + * + * @param {string} model + * @param {number} id + * @returns {string} + */ + getDisplayName(model, id) { + const displayNameResult = this._displayNames[model] && this._displayNames[model][id]; + if (!displayNameResult) { + // Catch the error to prevent the error from being thrown in the + // background. + this._fetchDisplayName(model, id).catch(() => {}); + throw new LoadingDataError(); + } + switch (displayNameResult.state) { + case "ERROR": + throw displayNameResult.error; + case "COMPLETED": + return displayNameResult.value; + default: + throw new LoadingDataError(); + } + } + + /** + * Get the batch endpoint for the given model. If it does not exist, it will + * be created. + * + * @param {string} model + * @returns {BatchEndpoint} + */ + _getEndpoint(model) { + if (!this._endpoints[model]) { + this._endpoints[model] = new BatchEndpoint(this._orm, model, "name_get", { + whenDataIsFetched: () => this.dataFetchedCallback(), + successCallback: this._assignResult.bind(this), + failureCallback: this._assignError.bind(this), + }); + } + return this._endpoints[model]; + } + + /** + * This method is called when the display name of a record is successfully + * fetched. It updates the cache and resolves the deferred of the record. + * + * @param {Request} request + * @param {BatchedNameGetRPCResult} result + * + * @private + */ + _assignResult(request, result) { + const deferred = this._displayNames[request.resModel][request.args[0]].deferred; + deferred.resolve(result && result[1]); + this._displayNames[request.resModel][request.args[0]] = { + state: "COMPLETED", + deferred, + value: result && result[1], + }; + } + + /** + * This method is called when the display name of a record could not be + * fetched. It updates the cache and rejects the deferred of the record. + * + * @param {Request} request + * @param {Error} error + * + * @private + */ + _assignError(request, error) { + const deferred = this._displayNames[request.resModel][request.args[0]].deferred; + deferred.reject(error); + this._displayNames[request.resModel][request.args[0]] = { + state: "ERROR", + deferred, + error, + }; + } + + /** + * This method is called when the display name of a record is not in the + * cache. It creates a deferred and fetches the display name in the + * background. + * + * @param {string} model + * @param {number} id + * + * @private + * @returns {Deferred} + */ + async _fetchDisplayName(model, id) { + const deferred = new Deferred(); + if (!this._displayNames[model]) { + this._displayNames[model] = {}; + } + this._displayNames[model][id] = { + state: "PENDING", + deferred, + }; + const endpoint = this._getEndpoint(model); + const request = new Request(model, "name_get", [id]); + endpoint.call(request); + return deferred; + } +} diff --git a/addons/spreadsheet/static/src/data_sources/labels_repository.js b/addons/spreadsheet/static/src/data_sources/labels_repository.js new file mode 100644 index 00000000000..f7b347c64bd --- /dev/null +++ b/addons/spreadsheet/static/src/data_sources/labels_repository.js @@ -0,0 +1,54 @@ +/** @odoo-module */ + +/** + * This class is responsible for keeping track of the labels of records. It + * caches the labels of records that have already been fetched. + * This class will not fetch the labels of records, it is the responsibility of + * the caller to fetch the labels and insert them in this repository. + */ +export class LabelsRepository { + constructor() { + /** + * Contains the labels of records. It's organized in the following way: + * { + * "crm.lead": { + * "city": { + * "bruxelles": "Bruxelles", + * } + * }, + * } + */ + this._labels = {}; + } + + /** + * Get the label of a record. + * @param {string} model technical name of the model + * @param {string} field name of the field + * @param {any} value value of the field + * + * @returns {string|undefined} label of the record + */ + getLabel(model, field, value) { + return ( + this._labels[model] && this._labels[model][field] && this._labels[model][field][value] + ); + } + + /** + * Set the label of a record. + * @param {string} model + * @param {string} field + * @param {string|number} value + * @param {string|undefined} label + */ + setLabel(model, field, value, label) { + if (!this._labels[model]) { + this._labels[model] = {}; + } + if (!this._labels[model][field]) { + this._labels[model][field] = {}; + } + this._labels[model][field][value] = label; + } +} diff --git a/addons/spreadsheet/static/src/data_sources/metadata_repository.js b/addons/spreadsheet/static/src/data_sources/metadata_repository.js index ee4f2ee3b5c..150862292a4 100644 --- a/addons/spreadsheet/static/src/data_sources/metadata_repository.js +++ b/addons/spreadsheet/static/src/data_sources/metadata_repository.js @@ -5,6 +5,8 @@ import { sprintf } from "@web/core/utils/strings"; import { ServerData } from "../data_sources/server_data"; import { LoadingDataError } from "../o_spreadsheet/errors"; +import { DisplayNameRepository } from "./display_name_repository"; +import { LabelsRepository } from "./labels_repository"; const { EventBus } = owl; @@ -28,7 +30,7 @@ const { EventBus } = owl; * for different entities that are defined on the same model. * * Implementation note: - * For the labels, when someone is asking for a label which is not loaded yet, + * For the labels, when someone is asking for a display name which is not loaded yet, * the proxy returns directly (undefined) and a request for a name_get will * be triggered. All the requests created are batched and send, with only one * request per model, after a clock cycle. @@ -38,21 +40,16 @@ export class MetadataRepository extends EventBus { constructor(orm) { super(); this.orm = orm.silent; - /** - * Contains the labels of records. It's organized in the following way: - * { - * "crm.lead": { - * "city": { - * "bruxelles": "Bruxelles", - * } - * }, - * } - */ - this._labels = {}; this.serverData = new ServerData(this.orm, { whenDataIsFetched: () => this.trigger("labels-fetched"), }); + + this.labelsRepository = new LabelsRepository(); + + this.displayNameRepository = new DisplayNameRepository(this.orm, { + whenDataIsFetched: () => this.trigger("labels-fetched"), + }); } /** @@ -85,13 +82,7 @@ export class MetadataRepository extends EventBus { * @param {string} label */ registerLabel(model, field, value, label) { - if (!this._labels[model]) { - this._labels[model] = {}; - } - if (!this._labels[model][field]) { - this._labels[model][field] = {}; - } - this._labels[model][field][value] = label; + this.labelsRepository.setLabel(model, field, value, label); } /** @@ -103,9 +94,14 @@ export class MetadataRepository extends EventBus { * @returns {string} */ getLabel(model, field, value) { - return ( - this._labels[model] && this._labels[model][field] && this._labels[model][field][value] - ); + return this.labelsRepository.getLabel(model, field, value); + } + + /** + * Save the result of a name_get request in the cache + */ + setDisplayName(model, id, result) { + this.displayNameRepository.setDisplayName(model, id, result); } /** @@ -119,8 +115,7 @@ export class MetadataRepository extends EventBus { */ getRecordDisplayName(model, id) { try { - const result = this.serverData.batch.get(model, "name_get", id); - return result[1]; + return this.displayNameRepository.getDisplayName(model, id); } catch (e) { if (e instanceof LoadingDataError) { throw e; diff --git a/addons/spreadsheet/static/src/pivot/pivot_model.js b/addons/spreadsheet/static/src/pivot/pivot_model.js index 512ffdf4189..b3f14077227 100644 --- a/addons/spreadsheet/static/src/pivot/pivot_model.js +++ b/addons/spreadsheet/static/src/pivot/pivot_model.js @@ -414,6 +414,12 @@ export class SpreadsheetPivotModel extends PivotModel { group.values[i], group.labels[i] ); + } else { + metadataRepository.setDisplayName( + field.relation, + group.values[i], + group.labels[i] + ); } } } diff --git a/addons/spreadsheet/static/tests/pivots/model/pivot_plugin_test.js b/addons/spreadsheet/static/tests/pivots/model/pivot_plugin_test.js index 0659bae1260..a9013fb7f08 100644 --- a/addons/spreadsheet/static/tests/pivots/model/pivot_plugin_test.js +++ b/addons/spreadsheet/static/tests/pivots/model/pivot_plugin_test.js @@ -396,7 +396,6 @@ QUnit.module("spreadsheet > pivot plugin", {}, () => { QUnit.test("display loading while data is not fully available", async function (assert) { const metadataPromise = makeDeferred(); const dataPromise = makeDeferred(); - const namePromise = makeDeferred(); const spreadsheetData = { sheets: [ { @@ -437,8 +436,7 @@ QUnit.module("spreadsheet > pivot plugin", {}, () => { await dataPromise; } if (model === "product" && method === "name_get") { - assert.step(`${model}/${method}`); - await namePromise; + assert.ok(false, "should not be called because data is put in cache"); } return result; }, @@ -456,15 +454,9 @@ QUnit.module("spreadsheet > pivot plugin", {}, () => { await nextTick(); setCellContent(model, "A10", "2"); assert.strictEqual(getCellValue(model, "A1"), "Probability"); - assert.strictEqual(getCellValue(model, "A2"), "Loading..."); - assert.strictEqual(getCellValue(model, "A3"), 131); - namePromise.resolve(); - await nextTick(); - setCellContent(model, "A10", "3"); - assert.strictEqual(getCellValue(model, "A1"), "Probability"); assert.strictEqual(getCellValue(model, "A2"), "xphone"); assert.strictEqual(getCellValue(model, "A3"), 131); - assert.verifySteps(["partner/fields_get", "partner/read_group", "product/name_get"]); + assert.verifySteps(["partner/fields_get", "partner/read_group"]); }); QUnit.test("relational PIVOT.HEADER with missing id", async function (assert) {