-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[10.0] [IMP] base_multi_image: improve uninstall hook #1286
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
[10.0] [IMP] base_multi_image: improve uninstall hook #1286
Conversation
| Model = env[model] | ||
| Field = field and Model._fields[field] | ||
| FieldMedium = field_medium and Model._fields[field_medium] | ||
| FieldSmall = field_small and Model._fields[field_small] |
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.
one of these 3 can fail with KeyError easily
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.
Hi @yajo , if a KeyError is raised it means that the uninstall hook hasn't been used correctly by the module that depends on it. It would be a programming error.
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.
In such case, the arguments shouldn't be optional
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.
If the new parameters would be required then it will break existing modules using the hook.
They should be optional because the introduced behavior isn't the desired in some cases as product variants in OCA/product-attribute#359
|
Wasn't it supposed that this addon lets the original image in place, stored in database? |
|
@yajo , no, it moves the main of multiple images to the original field, stored in a database column or as an attachment depending of the field declaration. |
|
Hi @yajo , Is there something more to change? Could you change you review? Thanks! |
yajo
left a comment
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.
Yikes, the code is very hard to read 😕
However it seems to do the trick.
pedrobaeza
left a comment
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.
Indeed, this is very convoluted, but it's what Odoo forces us to do for preserving this. Thanks for the hard work
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
Syncing from upstream OCA/server-tools (14.0)
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
improve uninstall hook to move images from multi to single mode
Hi,
Improve uninstall hook to move images from multi to single mode.
Thanks!