pengyuthon/odoo_source
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[FIX] web: fix concurrency issue in pivot

Browse Source 
Let's assume the following scenario in a pivot view
 - toggle a filter on a slow network s.t. it takes a while to load
 - while it's loading, toggle a measure

Before this commit, the view was kind of locked after those two
steps, i.e. toggling measures, expanding groups, flipping axis did
not work anymore (it could be unlocked by interacting again with the
search view though). The reason is that the new filter triggered a
re-rendering of the withSearch component (the parent of the pivot
view), thus calling willUpdateProps on the pivot view, which reloads
the data. Toggling a measure cancels the current load operation
(via keepLast), and the promise returned by willUpdateProps is thus
pending forever. From that point, only another re-rendering from the
parent (withSearch) could unlock the situation by cancelling the
previous rendering.

X-original-commit: b9278e07b9ff01e608ccba6f4602eb9069213a46
Part-of: odoo/odoo#76576
This commit is contained in:
Aaron Bohy
2021-09-15 15:22:23 +00:00
parent 932de2fc0f
commit a9bde08f98
3 changed files with 146 additions and 93 deletions
Download Patch File Download Diff File Expand all files Collapse all files
addons/web/static/src/views/helpers/model.js
+3 -3
View File
@@ -26,12 +26,12 @@ export class Model extends EventBus {
* @param {Object} params
* @param {Object} services
*/
setup(params, services) {}
setup(/* params, services */) {}
/**
* @param {SearchParams} searchParams
*/
load(searchParams) {}
load(/* searchParams */) {}
notify() {
this.trigger("update");
@@ -78,7 +78,7 @@ export function useModel(ModelClass, params, options = {}) {
onWillUpdateProps((nextProps) => {
const searchParams = getSearchParams(nextProps);
searchParams.useSampleModel = false; // not sure it is good --> we'll know when implementing dashboard
searchParams.useSampleModel = false;
return model.load(searchParams);
});
return model;
addons/web/static/src/views/pivot/pivot_model.js
+97 -90
View File
@@ -281,7 +281,6 @@ import { computeReportMeasures, processMeasure } from "@web/views/helpers/utils"
/**
* @typedef Meta
* @property {Object} meta
* @property {string[]} activeMeasures
* @property {string[]} colGroupBys
* @property {boolean} disableLinking
@@ -296,6 +295,8 @@ import { computeReportMeasures, processMeasure } from "@web/views/helpers/utils"
* @property {string[]} expandedRowGroupBys
* @property {string[]} expandedColGroupBys
* @property {Object} sortedColumn
* @property {Array[]} domains
* @property {string[]} origins
*/
/**
@@ -316,7 +317,6 @@ import { computeReportMeasures, processMeasure } from "@web/views/helpers/utils"
* @typedef Config
* @property {Meta} meta
* @property {Data} data
* @property {SearchParams} searchParams
*/
export class PivotModel extends Model {
@@ -360,8 +360,13 @@ export class PivotModel extends Model {
expandedColGroupBys: params.meta.expandedColGroupBys || [],
sortedColumn,
});
this.meta = this._buildMeta(meta);
this.searchParams = {
context: {},
domain: [],
domains: [],
groupBy: [],
};
this.data = params.data || {
colGroupTree: null,
rowGroupTree: null,
@@ -370,16 +375,12 @@ export class PivotModel extends Model {
counts: {},
numbering: {},
};
this.meta = this._buildMeta(meta);
this.searchParams = {
context: {},
domain: [],
groupBy: [],
domains: null, // not the same as searchParams.domains from cp
origins: null, // generated from searchParams.domains from cp
};
this.reload = false; // used to discriminate between the first load and subsequent reloads
// loadProm ensures that loadData always returns a promise that resolves when
// the final data is loaded (in case several load requests arrive consecutively)
this.loadProm = null;
this.nextActiveMeasures = null; // allows to toggle several measures consecutively
}
@@ -421,7 +422,7 @@ export class PivotModel extends Model {
} else {
meta.expandedColGroupBys.push(groupBy);
}
const config = { meta, data: this.data, searchParams: this.searchParams };
const config = { meta, data: this.data };
await this._expandGroup(groupId, type, config);
this.meta = meta;
this.notify();
@@ -493,8 +494,8 @@ export class PivotModel extends Model {
* This is the easiest way to expand all the groups that are not expanded
*/
async expandAll() {
const config = { meta: this.meta, data: this.data, searchParams: this.searchParams };
this.data = await this._loadData(config);
const config = { meta: this.meta, data: this.data };
await this._loadData(config, false);
this.notify();
}
/**
@@ -504,7 +505,11 @@ export class PivotModel extends Model {
* @param {'row'|'col'} type
*/
async expandGroup(groupId, type) {
const config = { meta: this.meta, data: this.data, searchParams: this.searchParams };
if (this.loadProm) {
return; // we are currently reloaded the table
}
const config = { meta: this.meta, data: this.data };
await this._expandGroup(groupId, type, config);
this.notify();
}
@@ -516,7 +521,7 @@ export class PivotModel extends Model {
*/
exportData() {
const measureCount = this.meta.activeMeasures.length;
const originCount = this.searchParams.origins.length;
const originCount = this.meta.origins.length;
const table = this.getTable();
@@ -633,7 +638,7 @@ export class PivotModel extends Model {
* @returns {Array[]}
*/
getGroupDomain(group) {
const config = { meta: this.meta, data: this.data, searchParams: this.searchParams };
const config = { meta: this.meta, data: this.data };
return this._getGroupDomain(group, config);
}
/**
@@ -661,8 +666,7 @@ export class PivotModel extends Model {
* @returns {boolean} true iff there's no data in the table
*/
hasData() {
const config = { meta: this.meta, data: this.data, searchParams: this.searchParams };
return this._hasData(config);
return this._hasData(this.data);
}
/**
* @override
@@ -670,7 +674,7 @@ export class PivotModel extends Model {
*/
async load(searchParams) {
this.orm2Use = this.realORM;
searchParams = JSON.parse(JSON.stringify(searchParams)); //This to prevent errors in the Dashboard
this.searchParams = JSON.parse(JSON.stringify(searchParams));
const activeMeasures =
processMeasure(searchParams.context.pivot_measures) || this.meta.activeMeasures;
@@ -687,10 +691,6 @@ export class PivotModel extends Model {
}
meta.colGroupBys = searchParams.context.pivot_column_groupby || this.meta.colGroupBys;
const { domains, origins } = this._computeDerivedParams(searchParams);
searchParams.domains = domains;
searchParams.origins = origins;
if (JSON.stringify(meta.rowGroupBys) !== JSON.stringify(this.meta.rowGroupBys)) {
meta.expandedRowGroupBys = [];
}
@@ -704,20 +704,8 @@ export class PivotModel extends Model {
meta.activeMeasures,
meta.additionalMeasures
);
const config = { meta, data: this.data, searchParams };
const data = await this._loadData(config);
const oldConfig = { meta: this.meta, data: this.data, searchParams: this.searchParams };
if (this._hasData(oldConfig)) {
if (symmetricalDifference(meta.rowGroupBys, this.meta.rowGroupBys).length === 0) {
this._pruneTree(data.rowGroupTree, this.data.rowGroupTree);
}
if (symmetricalDifference(meta.colGroupBys, this.meta.colGroupBys).length === 0) {
this._pruneTree(data.colGroupTree, this.data.colGroupTree);
}
}
this.data = data;
this.meta = meta;
this.searchParams = searchParams;
const config = { meta, data: this.data };
return this._loadData(config);
}
/**
* Sort the rows, depending on the values of a given column. This is an
@@ -729,7 +717,7 @@ export class PivotModel extends Model {
sortRows(sortedColumn) {
this._cancelPreviousOperation();
const config = { meta: this.meta, data: this.data, searchParams: this.searchParams };
const config = { meta: this.meta, data: this.data };
this._sortRows(sortedColumn, config);
this.notify();
@@ -752,13 +740,13 @@ export class PivotModel extends Model {
// to wait in case there is a pending load)
meta.activeMeasures.splice(index, 1);
await Promise.resolve(this.loadProm);
this.meta = meta;
} else {
meta.activeMeasures.push(fieldName);
const config = { meta, data: this.data, searchParams: this.searchParams };
this.data = await this._loadData(config);
let config = { meta, data: this.data };
await this._loadData(config);
}
this.nextActiveMeasures = null;
this.meta = meta;
this.notify();
}
@@ -810,6 +798,14 @@ export class PivotModel extends Model {
meta.customGroupBys = new Map([...meta.customGroupBys]);
// shallow copy sortedColumn because we never modify groupId in place
meta.sortedColumn = meta.sortedColumn ? { ...meta.sortedColumn } : null;
if (this.searchParams.comparison) {
const domains = this.searchParams.comparison.domains.slice().reverse();
meta.domains = domains.map((d) => d.arrayRepr);
meta.origins = domains.map((d) => d.description);
} else {
meta.domains = [this.searchParams.domain];
meta.origins = [""];
}
Object.defineProperty(meta, "fullColGroupBys", {
get() {
return meta.colGroupBys.concat(meta.expandedColGroupBys);
@@ -961,7 +957,7 @@ export class PivotModel extends Model {
measureSpecs,
groupBy,
options,
config.searchParams.context
this.searchParams.context
);
return {
group: group,
@@ -1016,7 +1012,7 @@ export class PivotModel extends Model {
* @returns {Array}
*/
_getMeasurements(group, config) {
const { meta, searchParams } = config;
const { meta } = config;
return meta.activeMeasures.reduce((measurements, measureName) => {
var measurement = group[measureName];
if (measurement instanceof Array) {
@@ -1026,7 +1022,7 @@ export class PivotModel extends Model {
if (meta.measures[measureName].type === "boolean" && measurement instanceof Boolean) {
measurement = measurement ? 1 : 0;
}
if (searchParams.origins.length > 1 && !measurement) {
if (meta.origins.length > 1 && !measurement) {
measurement = 0;
}
measurements[measureName] = measurement;
@@ -1051,7 +1047,7 @@ export class PivotModel extends Model {
height: 1,
measure: measureName,
title: this.meta.measures[measureName].string,
width: 2 * this.searchParams.origins.length - 1,
width: 2 * this.meta.origins.length - 1,
};
if (
sortedColumn.measure === measureName &&
@@ -1135,7 +1131,7 @@ export class PivotModel extends Model {
const isSortedByOrigin = isSorted && !sortedColumn.originIndexes[1];
const isSortedByVariation = isSorted && sortedColumn.originIndexes[1];
this.searchParams.origins.forEach((origin, originIndex) => {
this.meta.origins.forEach((origin, originIndex) => {
const originCell = {
groupId: groupId,
height: 1,
@@ -1181,7 +1177,7 @@ export class PivotModel extends Model {
const colGroupBys = this.meta.fullColGroupBys;
const height = colGroupBys.length + 1;
const measureCount = this.meta.activeMeasures.length;
const originCount = this.searchParams.origins.length;
const originCount = this.meta.origins.length;
const leafCounts = this._getLeafCounts(this.data.colGroupTree);
let headers = [];
const measureColumns = []; // used to generate the measure cells
@@ -1330,11 +1326,10 @@ export class PivotModel extends Model {
}
/**
* @private
* @param {Config} config
* @param {Data} data
* @returns {boolean} true iff there's no data in the table
*/
_hasData(config) {
const { data } = config;
_hasData(data) {
return (data.counts[JSON.stringify([[], []])] || []).some((count) => {
return count > 0;
});
@@ -1350,9 +1345,18 @@ export class PivotModel extends Model {
* @private
* @param {Config} config
*/
async _loadData(config) {
async _loadData(config, prune = true) {
if (!this.loadProm) {
this.loadProm = new Promise((resolve) => {
this.resolveLoadProm = () => {
this.loadProm = null;
resolve();
};
});
}
config.data = {}; // data will be completely recomputed
const { data, meta, searchParams } = config;
const { data, meta } = config;
data.rowGroupTree = { root: { labels: [], values: [] }, directSubTrees: new Map() };
data.colGroupTree = { root: { labels: [], values: [] }, directSubTrees: new Map() };
data.measurements = {};
@@ -1360,24 +1364,46 @@ export class PivotModel extends Model {
data.groupDomains = {};
data.numbering = {};
const key = JSON.stringify([[], []]);
data.groupDomains[key] = searchParams.domains.slice(0);
data.groupDomains[key] = meta.domains.slice(0);
const group = { rowValues: [], colValues: [] };
const leftDivisors = sections(meta.fullRowGroupBys);
const rightDivisors = sections(meta.fullColGroupBys);
const divisors = cartesian(leftDivisors, rightDivisors);
await this._subdivideGroup(group, divisors.slice(0, 1), config);
await this._subdivideGroup(group, divisors.slice(1), config);
this._subdivideGroup(group, divisors.slice(0, 1), config)
.then(() => {
return this._subdivideGroup(group, divisors.slice(1), config);
})
.then(async () => {
if (meta.useSampleModel && !this._hasData(config.data)) {
const fakeORM = buildSampleORM(meta.resModel, meta.fields, this.user);
this.orm2Use = fakeORM;
await this._loadData(config);
} else if (this.orm2Use.rpc.name !== "fakeRPC") {
meta.useSampleModel = false;
}
if (meta.useSampleModel && !this._hasData(config)) {
const fakeORM = buildSampleORM(meta.resModel, meta.fields, this.user);
this.orm2Use = fakeORM;
return this._loadData(config);
} else if (this.orm2Use.rpc.name !== "fakeRPC") {
meta.useSampleModel = false;
}
return config.data;
// keep folded groups folded after the reload if the structure of the table is the same
if (prune && this._hasData(data) && this._hasData(this.data)) {
if (
symmetricalDifference(meta.rowGroupBys, this.meta.rowGroupBys).length === 0
) {
this._pruneTree(data.rowGroupTree, this.data.rowGroupTree);
}
if (
symmetricalDifference(meta.colGroupBys, this.meta.colGroupBys).length === 0
) {
this._pruneTree(data.colGroupTree, this.data.colGroupTree);
}
}
this.data = config.data;
this.meta = config.meta;
this.resolveLoadProm();
});
return this.loadProm;
}
/**
* Extract the information in the read_group results (groupSubdivisions)
@@ -1392,7 +1418,7 @@ export class PivotModel extends Model {
* @param {Config} config
*/
_prepareData(group, groupSubdivisions, config) {
const { data, meta, searchParams } = config;
const { data, meta } = config;
const groupRowValues = group.rowValues;
let groupRowLabels = [];
let rowSubTree = data.rowGroupTree;
@@ -1438,21 +1464,21 @@ export class PivotModel extends Model {
const originIndex = groupSubdivision.group.originIndex;
if (!(key in data.measurements)) {
data.measurements[key] = searchParams.origins.map(() => {
data.measurements[key] = meta.origins.map(() => {
return this._getMeasurements({}, config);
});
}
data.measurements[key][originIndex] = this._getMeasurements(subGroup, config);
if (!(key in data.counts)) {
data.counts[key] = searchParams.origins.map(function () {
data.counts[key] = meta.origins.map(function () {
return 0;
});
}
data.counts[key][originIndex] = subGroup.__count;
if (!(key in data.groupDomains)) {
data.groupDomains[key] = searchParams.origins.map(function () {
data.groupDomains[key] = meta.origins.map(function () {
return Domain.FALSE.toList();
});
}
@@ -1469,25 +1495,6 @@ export class PivotModel extends Model {
this._sortRows(meta.sortedColumn, config);
}
}
/**
* @todo review comment
* Determine this.searchParams.domains and this.searchParams.origins from
* this.searchParams.domains.
*
* @private
* @param {SearchParams} searchParams
*/
_computeDerivedParams(searchParams) {
const { comparison, domain } = searchParams;
if (comparison) {
const domains = searchParams.comparison.domains.slice().reverse();
return {
domains: domains.map((d) => d.arrayRepr),
origins: domains.map((d) => d.description),
};
}
return { domains: [domain], origins: [""] };
}
/**
* Make any group in tree a leaf if it was a leaf in oldTree.
*
@@ -1561,10 +1568,10 @@ export class PivotModel extends Model {
* @param {Config} config
*/
async _subdivideGroup(group, divisors, config) {
const { data, searchParams } = config;
const { data, meta } = config;
const key = JSON.stringify([group.rowValues, group.colValues]);
const proms = searchParams.origins.reduce((acc, origin, originIndex) => {
const proms = meta.origins.reduce((acc, origin, originIndex) => {
// if no information on group content is available, we fetch data.
// if group is known to be empty for the given origin,
// we don't need to fetch data for that origin.
addons/web/static/tests/views/pivot_view_tests.js
+46
View File
@@ -4882,6 +4882,52 @@ QUnit.module("Views", (hooks) => {
assert.strictEqual(getCurrentValues(pivot), ["2", "4", "1", "1", "1", "3"].join(","));
});
QUnit.test(
"concurrent reloads: add a filter, and directly toggle a measure",
async function (assert) {
let def;
const pivot = await makeView({
type: "pivot",
resModel: "partner",
serverData,
arch: `
<pivot>
<field name="foo" type="measure"/>
<field name="product_id" type="row"/>
</pivot>`,
searchViewArch: `
<search>
<filter name="my_filter" string="My Filter" domain="[('product_id', '=', 37)]"/>
</search>`,
mockRPC(route, args) {
if (args.method === "read_group") {
return Promise.resolve(def);
}
},
});
assert.strictEqual(getCurrentValues(pivot), ["32", "12", "20"].join(","));
// Set a domain (this reload is delayed)
def = makeDeferred();
await toggleFilterMenu(pivot);
await toggleMenuItem(pivot, "My Filter");
assert.strictEqual(getCurrentValues(pivot), ["32", "12", "20"].join(","));
// Toggle a measure
await toggleMenu(pivot, "Measures");
await toggleMenuItem(pivot, "Count");
assert.strictEqual(getCurrentValues(pivot), ["32", "12", "20"].join(","));
def.resolve();
await nextTick();
assert.strictEqual(getCurrentValues(pivot), ["12", "1", "12", "1"].join(","));
}
);
QUnit.test(
"if no measure is set in arch, 'Count' is used as measure initially",
async function (assert) {