[8.0][base_multi_image][product_multi_image] Multi images for products split in abstract and concrete models#112
Conversation
|
The name should be base_multi_image and I'm not sure if the repo target should be this or server-tools, and let product_multi_image for here. Anway, this is a WIP and we can let here the discussion. |
multi_image_base/README.rst
Outdated
There was a problem hiding this comment.
product_multi_image: always first the name of the "area".
|
I refactored most code to make it easier to inherit, and moving as much as possible to the base module, following the method learned with OCA/server-tools#313. It fixed some bugs by the way, but there is one remaining that I'd thank help:
Now the reverse way does not work:
|
It takes care of moving images from the main model to the gallery submodel, making the first image in the set always the main one. It's implemented in :meth:`~.multi_image_base.hooks.post_init_hook_for_submodules` for better reusability across submodules.
|
Finally it works (I hope). Ready to review! |
|
Travis fails |
|
The name is alright because odoo names for modules are Also I kept both modules in 1 PR because one does nothing without the another. I understand the base one should go to server-tools, but please review this and when I have enough 👍 I will split it before merging, so you can test now in runbot and so. |
|
@yajo, you can say what you want, but the name is not correct. All is prefixed with |
|
Hmm you are right, I did not know that exception. About the website dependency it's about the usage of |
yajo
changed the title
|
The multi_image module is a generic feature, not specifically linked to products. |
|
The |
I agree. But could you (and others) please test and conditionally approve this before I split? It's just because one module without the other don't make too much sense and will break bots.
I'm afraid the |
|
I don't agree about the website part, @yajo. You can have multiple images because you synchronize Prestashop/Magento products, and you only want to maintain them. For that, you don't need website. |
|
Do you want 2 modules then? |
|
@yajo you mean 3? |
|
yes, sorry. |
|
Can't you just replace the |
| <openerp> | ||
| <data> | ||
|
|
||
| <template id="assets_frontend" inherit_id="website.assets_frontend"> |
There was a problem hiding this comment.
@dreispt Here's why I need website (or split module).
| <data> | ||
|
|
||
| <!-- Call this template in your owner's website page to get a carousel. | ||
| It needs a `multi_image_owner` variable set. --> |
There was a problem hiding this comment.
@dreispt These templates are to reuse in frontend website, not backend, so assets must go in frontend.
There was a problem hiding this comment.
There was a problem hiding this comment.
Since this is a base module, it provides tools to reuse in submodules, like these templates. These templates are useless without the attached CSS, and you cannot attach it without the website module. I hope you understand the dependency is required, so either I split the module or it stays there.
|
I don't understand we we need frontend features in a backend module. Confusing... |
|
Hi @yajo , please which is the state of this PR, please could you give a resume? Thanks @dreispt we don't need here front-end features, this module is done to provide base functionalities to future modules as product_multi_image, event_multi_image, .... always in back-end. Of course when we will have such modules as event_multi_image (which depend on this one) we will do website_event_multi_image to show this multi images in front-end. Note: like this module OCA has more base_* modules like https://github.com/OCA/partner-contact/tree/8.0/base_contact, .... I think idea is clear. |
|
Well after confusions have been cleared, let me split this into 3 modules and PR. Thanks everyone! |
|
Thanks everyone! 😉 This is now properly split. Please review: OCA/server-tools#374, OCA/website#171, #135, OCA/event#31, OCA/event#33. |
|
@yajo Shoudn't there be a PR for OCA/product-attribute also? |
|
True, I forgot to say it: #135 |
7 participants
According to OdooCommunityWidgets/website_multi_image#34 (comment), I'm pushing this PR that continues job from #57 to discuss about design, but it is a WIP right now.