-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FIX] model: prevent UI plugins to refuse core commands #2120
[FIX] model: prevent UI plugins to refuse core commands #2120
Conversation
Currently it's possible for UI plugins to handle and refuse core commands in the allowDispatch. This is a problem because this could lead to de-synchronized states in multi-user. This commit fixes this by making sure that the core commands don't go through the allowDispatch of the UI plugins. Odoo task 3209463
@@ -41,7 +40,7 @@ export class FilterEvaluationPlugin extends UIPlugin { | |||
hiddenRows: Set<number> = new Set(); | |||
isEvaluationDirty = false; | |||
|
|||
allowDispatch(cmd: Command) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note : I changed the allowDispatch interface of the current local plugins, but I couldn't find a way to type UI plugins correctly to prevent them from handling core commands. Someone could create a UI plugin w/ allowDispatch(cmd : LocalCommand | CoreCommand)
and ts wouldn't complain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LucasLefevre any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't prevent that.
Nothing prevents a subclass to have a more general API than its parent, as long as it implements the parent's interface.
class A {
coucou(a: number) {
return true
}
}
class B extends A {
coucou(n: number | string) {
return true
}
}
Let's say I have a: A;
but a
is actually an instance of B
under the hood, but I don't know it. It's totally fine, I don't care that a
can actually handle/do more stuff, I just use it as an A
and will give it only numbers
However, if it doesn't have the same API than the parent, then it does work.
class B2 extends A {
// error: Type 'number' is not assignable to type 'string'
coucou(n: string) {
return true
}
}
Here since LocalCommand
(parent API) is assignable to LocalCommand | CoreCommand
(child class), everything is fine.
cc @anhe-odoo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
robodoo r+ 👍
Currently it's possible for UI plugins to handle and refuse core commands in the allowDispatch. This is a problem because this could lead to de-synchronized states in multi-user. This commit fixes this by making sure that the core commands don't go through the allowDispatch of the UI plugins. Odoo task 3209463 closes #2120 Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
Description
Currently it's possible for UI plugins to handle and refuse core commands in the allowDispatch. This is a problem because this could lead to de-synchronized states in multi-user.
This commit fixes this by making sure that the core commands don't go through the allowDispatch of the UI plugins.
Odoo task ID : 3209463
review checklist