From 416e05b210cf8f4e209fef044200e7077e550c34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20K=C3=BChn?= Date: Thu, 19 Oct 2023 17:29:12 +0200 Subject: [PATCH] [REF] mail:tests: add/delete on record fields with inverse Some test coverage on model with inverses, especially on deletion that are not frequently functionally covered by tests. To make test coverage on very specific on models and relational fields with inverses, store service has been refactored so we can test the store without Persona/Thread/Message models and all related services and behaviours. Part-of: odoo/odoo#138760 --- addons/mail/static/src/core/common/record.js | 3 +- .../static/src/core/common/store_service.js | 470 +++++++++--------- addons/mail/static/tests/core/record_tests.js | 79 +++ .../mail/static/tests/helpers/test_utils.js | 2 +- 4 files changed, 319 insertions(+), 235 deletions(-) create mode 100644 addons/mail/static/tests/core/record_tests.js diff --git a/addons/mail/static/src/core/common/record.js b/addons/mail/static/src/core/common/record.js index 2b057ccdfe4..77a0e360b67 100644 --- a/addons/mail/static/src/core/common/record.js +++ b/addons/mail/static/src/core/common/record.js @@ -380,7 +380,8 @@ export class RecordList extends Array { last, (r) => { if (r.notEq(this[0])) { - const old = this.__list__.pop(); + const old = this.at(-1); + this.__list__.pop(); if (old) { this.__deleteInverse__(old); } diff --git a/addons/mail/static/src/core/common/store_service.js b/addons/mail/static/src/core/common/store_service.js index 22596a22cb9..fc20ffbb8c9 100644 --- a/addons/mail/static/src/core/common/store_service.js +++ b/addons/mail/static/src/core/common/store_service.js @@ -8,7 +8,21 @@ import { registry } from "@web/core/registry"; import { debounce } from "@web/core/utils/timing"; import { modelRegistry, Record, RecordInverses, RecordList } from "./record"; -export class Store extends Record { +export class BaseStore extends Record { + /** + * @param {string} localId + * @returns {Record} + */ + get(localId) { + if (typeof localId !== "string") { + return undefined; + } + const modelName = Record.modelFromLocalId(localId); + return this[modelName].records[localId]; + } +} + +export class Store extends BaseStore { /** @returns {import("models").Store} */ static insert() { return super.insert(); @@ -91,18 +105,6 @@ export class Store extends Record { this.updateBusSubscription = debounce(this.updateBusSubscription, 0); // Wait for thread fully inserted. } - /** - * @param {string} localId - * @returns {Record} - */ - get(localId) { - if (typeof localId !== "string") { - return undefined; - } - const modelName = Record.modelFromLocalId(localId); - return this[modelName].records[localId]; - } - updateBusSubscription() { const channelIds = []; const ids = Object.keys(this.Thread.records).sort(); // Ensure channels processed in same order. @@ -121,6 +123,227 @@ export class Store extends Record { } Store.register(); +export function makeStore(env) { + const res = { + // fake store for now, until it becomes a model + /** @type {Store} */ + store: { + env, + get: (...args) => Store.prototype.get.call(this, ...args), + }, + }; + const Models = {}; + for (const [name, _OgClass] of modelRegistry.getEntries()) { + /** @type {typeof Record} */ + const OgClass = _OgClass; + if (res.store[name]) { + throw new Error(`There must be no duplicated Model Names (duplicate found: ${name})`); + } + // classes cannot be made reactive because they are functions and they are not supported. + // work-around: make an object whose prototype is the class, so that static props become + // instance props. + const Model = Object.assign(Object.create(OgClass), { env, store: res.store }); + // Produce another class with changed prototype, so that there are automatic get/set on relational fields + const Class = { + [OgClass.name]: class extends OgClass { + constructor() { + super(); + const proxy = new Proxy(this, { + /** @param {Record} receiver */ + get(target, name, receiver) { + if (name !== "__rels__" && receiver.__rels__.has(name)) { + const l1 = receiver.__rels__.get(name); + if (RecordList.isMany(l1)) { + return l1; + } + return l1[0]; + } + return Reflect.get(target, name, receiver); + }, + deleteProperty(target, name) { + if (name !== "__rels__" && target.__rels__.has(name)) { + const r1 = target; + const l1 = r1.__rels__.get(name); + l1.clear(); + return true; + } + const ret = Reflect.deleteProperty(target, name); + return ret; + }, + /** @param {Record} receiver */ + set(target, name, val, receiver) { + if (!receiver.__rels__.has(name)) { + Reflect.set(target, name, val, receiver); + return true; + } + /** @type {RecordList} */ + const r1 = receiver; + const l1 = receiver.__rels__.get(name); + if (RecordList.isMany(l1)) { + // [Record.many] = + if (Record.isCommand(val)) { + for (const [cmd, cmdData] of val) { + if (Array.isArray(cmdData)) { + for (const item of cmdData) { + if (cmd === "ADD") { + l1.add(item); + } else if (cmd === "ADD.noinv") { + l1.__addNoinv(item); + } else if (cmd === "DELETE.noinv") { + l1.__deleteNoinv(item); + } else { + l1.delete(item); + } + } + } else { + if (cmd === "ADD") { + l1.add(cmdData); + } else if (cmd === "ADD.noinv") { + l1.__addNoinv(cmdData); + } else if (cmd === "DELETE.noinv") { + l1.__deleteNoinv(cmdData); + } else { + l1.delete(cmdData); + } + } + } + return true; + } + if ([null, false, undefined].includes(val)) { + l1.clear(); + return true; + } + if (!Array.isArray(val)) { + val = [val]; + } + /** @type {Record[]|Set|RecordList} */ + const collection = Record.isRecord(val) ? [val] : val; + const oldRecords = l1.slice(); + for (const r2 of oldRecords) { + r2.__invs__.delete(r1.localId, name); + } + // l1 and collection could be same record list, + // save before clear to not push mutated recordlist that is empty + const col = [...collection]; + l1.clear(); + l1.push(...col); + } else { + // [Record.one] = + if (Record.isCommand(val)) { + const [cmd, cmdData] = val.at(-1); + if (cmd === "ADD") { + l1.add(cmdData); + } else if (cmd === "ADD.noinv") { + l1.__addNoinv(cmdData); + } else if (cmd === "DELETE.noinv") { + l1.__deleteNoinv(cmdData); + } else { + l1.delete(cmdData); + } + return true; + } + if ([null, false, undefined].includes(val)) { + delete receiver[name]; + return true; + } + l1.add(val); + } + return true; + }, + }); + if (this instanceof Store) { + res.store = proxy; + } + for (const name of Model.__rels__.keys()) { + // Relational fields contain symbols for detection in original class. + // This constructor is called on genuine records: + // - 'one' fields => undefined + // - 'many' fields => RecordList + // this[name]?.[0] is ONE_SYM or MANY_SYM + const newVal = new RecordList(this[name]?.[0]); + if (this instanceof Store) { + newVal.__store__ = proxy; + } else { + newVal.__store__ = res.store; + } + newVal.name = name; + newVal.owner = proxy; + this.__rels__.set(name, newVal); + this.__invs__ = new RecordInverses(); + this[name] = newVal; + } + for (const [name, fn] of Model.__computes__.entries()) { + let boundFn; + const proxy2 = reactive(proxy, () => boundFn()); + boundFn = () => { + if (Record.__atomic__ > 0) { + Record.__atomics__.set( + `compute.${this.localId}.${name}`, + () => (proxy[name] = fn.call(proxy2)) + ); + } else { + proxy[name] = fn.call(proxy2); + } + }; + this.__computes__.set(name, boundFn); + } + return proxy; + } + }, + }[OgClass.name]; + Object.assign(Model, { + Class, + records: JSON.parse(JSON.stringify(OgClass.records)), + __rels__: new Map(), + __computes__: new Map(), + }); + Models[name] = Model; + res.store[name] = Model; + // Detect relational fields with a dummy record and setup getter/setters on them + const obj = new OgClass(); + for (const [name, val] of Object.entries(obj)) { + const SYM = val?.[0]; + if (![Record.one()[0], Record.many()[0]].includes(SYM)) { + continue; + } + Model.__rels__.set(name, { [SYM]: true, ...val[1] }); + if (val[1].compute) { + Model.__computes__.set(name, val[1].compute); + } + } + } + // Sync inverse fields + for (const Model of Object.values(Models)) { + for (const [name, { targetModel, inverse }] of Model.__rels__.entries()) { + if (targetModel && !Models[targetModel]) { + throw new Error(`No target model ${targetModel} exists`); + } + if (inverse) { + const rel2 = Models[targetModel].__rels__.get(inverse); + if (rel2.targetModel && rel2.targetModel !== Model.name) { + throw new Error( + `Fields ${Models[targetModel].name}.${inverse} has wrong targetModel. Expected: "${Model.name}" Actual: "${rel2.targetModel}"` + ); + } + if (rel2.inverse && rel2.inverse !== name) { + throw new Error( + `Fields ${Models[targetModel].name}.${inverse} has wrong inverse. Expected: "${name}" Actual: "${rel2.inverse}"` + ); + } + Object.assign(rel2, { targetModel: Model.name, inverse: name }); + } + } + } + // Make true store (as a model) + res.store = reactive(res.store.Store.insert()); + res.store.env = env; + for (const Model of Object.values(Models)) { + Model.store = res.store; + res.store[Model.name] = Model; + } + return res.store; +} + export const storeService = { dependencies: ["bus_service", "ui"], /** @@ -128,226 +351,7 @@ export const storeService = { * @param {Partial} services */ start(env, services) { - const res = { - // fake store for now, until it becomes a model - /** @type {Store} */ - store: { - env, - get: (...args) => Store.prototype.get.call(this, ...args), - }, - }; - const Models = {}; - for (const [name, _OgClass] of modelRegistry.getEntries()) { - /** @type {typeof Record} */ - const OgClass = _OgClass; - if (res.store[name]) { - throw new Error( - `There must be no duplicated Model Names (duplicate found: ${name})` - ); - } - // classes cannot be made reactive because they are functions and they are not supported. - // work-around: make an object whose prototype is the class, so that static props become - // instance props. - const Model = Object.assign(Object.create(OgClass), { env, store: res.store }); - // Produce another class with changed prototype, so that there are automatic get/set on relational fields - const Class = { - [OgClass.name]: class extends OgClass { - constructor() { - super(); - const proxy = new Proxy(this, { - /** @param {Record} receiver */ - get(target, name, receiver) { - if (name !== "__rels__" && receiver.__rels__.has(name)) { - const l1 = receiver.__rels__.get(name); - if (RecordList.isMany(l1)) { - return l1; - } - return l1[0]; - } - return Reflect.get(target, name, receiver); - }, - deleteProperty(target, name) { - if (name !== "__rels__" && target.__rels__.has(name)) { - const r1 = target; - const l1 = r1.__rels__.get(name); - l1.clear(); - return true; - } - const ret = Reflect.deleteProperty(target, name); - return ret; - }, - /** @param {Record} receiver */ - set(target, name, val, receiver) { - if (!receiver.__rels__.has(name)) { - Reflect.set(target, name, val, receiver); - return true; - } - /** @type {RecordList} */ - const r1 = receiver; - const l1 = receiver.__rels__.get(name); - if (RecordList.isMany(l1)) { - // [Record.many] = - if (Record.isCommand(val)) { - for (const [cmd, cmdData] of val) { - if (Array.isArray(cmdData)) { - for (const item of cmdData) { - if (cmd === "ADD") { - l1.add(item); - } else if (cmd === "ADD.noinv") { - l1.__addNoinv(item); - } else if (cmd === "DELETE.noinv") { - l1.__deleteNoinv(item); - } else { - l1.delete(item); - } - } - } else { - if (cmd === "ADD") { - l1.add(cmdData); - } else if (cmd === "ADD.noinv") { - l1.__addNoinv(cmdData); - } else if (cmd === "DELETE.noinv") { - l1.__deleteNoinv(cmdData); - } else { - l1.delete(cmdData); - } - } - } - return true; - } - if ([null, false, undefined].includes(val)) { - l1.clear(); - return true; - } - if (!Array.isArray(val)) { - val = [val]; - } - /** @type {Record[]|Set|RecordList} */ - const collection = Record.isRecord(val) ? [val] : val; - const oldRecords = l1.slice(); - for (const r2 of oldRecords) { - r2.__invs__.delete(r1.localId, name); - } - // l1 and collection could be same record list, - // save before clear to not push mutated recordlist that is empty - const col = [...collection]; - l1.clear(); - l1.push(...col); - } else { - // [Record.one] = - if (Record.isCommand(val)) { - const [cmd, cmdData] = val.at(-1); - if (cmd === "ADD") { - l1.add(cmdData); - } else if (cmd === "ADD.noinv") { - l1.__addNoinv(cmdData); - } else if (cmd === "DELETE.noinv") { - l1.__deleteNoinv(cmdData); - } else { - l1.delete(cmdData); - } - return true; - } - if ([null, false, undefined].includes(val)) { - delete receiver[name]; - return true; - } - l1.add(val); - } - return true; - }, - }); - if (this instanceof Store) { - res.store = proxy; - } - for (const name of Model.__rels__.keys()) { - // Relational fields contain symbols for detection in original class. - // This constructor is called on genuine records: - // - 'one' fields => undefined - // - 'many' fields => RecordList - // this[name]?.[0] is ONE_SYM or MANY_SYM - const newVal = new RecordList(this[name]?.[0]); - if (this instanceof Store) { - newVal.__store__ = proxy; - } else { - newVal.__store__ = res.store; - } - newVal.name = name; - newVal.owner = proxy; - this.__rels__.set(name, newVal); - this.__invs__ = new RecordInverses(); - this[name] = newVal; - } - for (const [name, fn] of Model.__computes__.entries()) { - let boundFn; - const proxy2 = reactive(proxy, () => boundFn()); - boundFn = () => { - if (Record.__atomic__ > 0) { - Record.__atomics__.set( - `compute.${this.localId}.${name}`, - () => (proxy[name] = fn.call(proxy2)) - ); - } else { - proxy[name] = fn.call(proxy2); - } - }; - this.__computes__.set(name, boundFn); - } - return proxy; - } - }, - }[OgClass.name]; - Object.assign(Model, { - Class, - records: JSON.parse(JSON.stringify(OgClass.records)), - __rels__: new Map(), - __computes__: new Map(), - }); - Models[name] = Model; - res.store[name] = Model; - // Detect relational fields with a dummy record and setup getter/setters on them - const obj = new OgClass(); - for (const [name, val] of Object.entries(obj)) { - const SYM = val?.[0]; - if (![Record.one()[0], Record.many()[0]].includes(SYM)) { - continue; - } - Model.__rels__.set(name, { [SYM]: true, ...val[1] }); - if (val[1].compute) { - Model.__computes__.set(name, val[1].compute); - } - } - } - // Sync inverse fields - for (const Model of Object.values(Models)) { - for (const [name, { targetModel, inverse }] of Model.__rels__.entries()) { - if (targetModel && !Models[targetModel]) { - throw new Error(`No target model ${targetModel} exists`); - } - if (inverse) { - const rel2 = Models[targetModel].__rels__.get(inverse); - if (rel2.targetModel && rel2.targetModel !== Model.name) { - throw new Error( - `Fields ${Models[targetModel].name}.${inverse} has wrong targetModel. Expected: "${Model.name}" Actual: "${rel2.targetModel}"` - ); - } - if (rel2.inverse && rel2.inverse !== name) { - throw new Error( - `Fields ${Models[targetModel].name}.${inverse} has wrong inverse. Expected: "${name}" Actual: "${rel2.inverse}"` - ); - } - Object.assign(rel2, { targetModel: Model.name, inverse: name }); - } - } - } - // Make true store (as a model) - res.store = reactive(res.store.Store.insert()); - res.store.env = env; - for (const Model of Object.values(Models)) { - Model.store = res.store; - res.store[Model.name] = Model; - } - const store = res.store; + const store = makeStore(env); store.discuss = {}; store.discuss.activeTab = env.services.ui.isSmall ? "mailbox" : "all"; onChange(store.Thread, "records", () => store.updateBusSubscription()); diff --git a/addons/mail/static/tests/core/record_tests.js b/addons/mail/static/tests/core/record_tests.js new file mode 100644 index 00000000000..86b32a5145e --- /dev/null +++ b/addons/mail/static/tests/core/record_tests.js @@ -0,0 +1,79 @@ +/* @odoo-module */ + +import { Record, modelRegistry } from "@mail/core/common/record"; +import { BaseStore, makeStore } from "@mail/core/common/store_service"; + +import { registry } from "@web/core/registry"; +import { clearRegistryWithCleanup, makeTestEnv } from "@web/../tests/helpers/mock_env"; + +const serviceRegistry = registry.category("services"); + +let start; +QUnit.module("record", { + beforeEach() { + serviceRegistry.add("store", { start: (env) => makeStore(env) }); + clearRegistryWithCleanup(modelRegistry); + Record.register(); + ({ Store: class extends BaseStore {} }).Store.register(); + start = async () => await makeTestEnv(); + }, +}); + +QUnit.test("Assign & Delete on fields with inverses", async (assert) => { + (class A extends Record { + static id = "id"; + id; + b = Record.one("B", { inverse: "a" }); + c = Record.one("C", { inverse: "aa" }); + dd = Record.many("D", { inverse: "aa" }); + }).register(); + (class B extends Record { + static id = "id"; + id; + a = Record.one("A"); + }).register(); + (class C extends Record { + static id = "id"; + id; + aa = Record.many("A"); + }).register(); + (class D extends Record { + static id = "id"; + id; + aa = Record.many("A"); + }).register(); + const env = await start(); + const a = env.services.store.A.insert("a"); + const b = env.services.store.B.insert("b"); + const c1 = env.services.store.C.insert("c1"); + const c2 = env.services.store.C.insert("c2"); + const d1 = env.services.store.D.insert("d1"); + const d2 = env.services.store.D.insert("d2"); + // Assign on fields should adapt inverses + Object.assign(a, { b, c: [["ADD", c1]], dd: [d1, d2] }); + assert.ok(a.b.eq(b)); + assert.ok(b.a.eq(a)); + assert.ok(a.c.eq(c1)); + assert.ok(a.in(c1.aa)); + assert.ok(d1.in(a.dd)); + assert.ok(d2.in(a.dd)); + assert.ok(a.in(d1.aa)); + assert.ok(a.in(d2.aa)); + // add() should adapt inverses + c2.aa.add(a); + assert.ok(a.in(c2.aa)); + assert.ok(a.c.eq(c2)); + // delete should adapt inverses + c2.aa.delete(a); + assert.notOk(a.in(c2.aa)); + assert.notOk(a.c); + // Deletion removes all relations + a.delete(); + assert.notOk(a.b); + assert.notOk(b.a); + assert.notOk(a.in(c1.aa)); + assert.notOk(d1.in(a.dd)); + assert.notOk(d2.in(a.dd)); + assert.notOk(a.in(d1.aa)); + assert.notOk(a.in(d2.aa)); +}); diff --git a/addons/mail/static/tests/helpers/test_utils.js b/addons/mail/static/tests/helpers/test_utils.js index 7c1768bfc4c..14d87216347 100644 --- a/addons/mail/static/tests/helpers/test_utils.js +++ b/addons/mail/static/tests/helpers/test_utils.js @@ -25,7 +25,7 @@ import { doAction, getActionManagerServerData } from "@web/../tests/webclient/he // load emoji data and lamejs once, when the test suite starts. QUnit.begin(loadEmoji); QUnit.begin(loadLamejs); -registryNamesToCloneWithCleanup.push("mock_server_callbacks"); +registryNamesToCloneWithCleanup.push("mock_server_callbacks", "discuss.model"); //------------------------------------------------------------------------------ // Public: test lifecycle