[8.0][base_multi_image][product_multi_image] Multi images for products split in abstract and concrete models #112

yajo · 2015-11-19T15:12:25Z

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.

pedrobaeza · 2015-11-19T15:16:06Z

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.

pedrobaeza · 2015-11-19T15:18:14Z

multi_image_base/README.rst

+
+To develop a module based on this one:
+
+* See module ``multi_image_product`` as an example.


product_multi_image: always first the name of the "area".

product_image_multi?

Move most code to base module, move to api.multi, fix some bugs.

yajo · 2016-02-01T17:36:41Z

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:

Create a product.
Go to images tab.
Add one.
That image gets set as the product's image.

Now the reverse way does not work:

Create a product.
Set it an image.
No multi_image record is created, but the product gets the image!

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.

yajo · 2016-02-03T15:31:30Z

Finally it works (I hope). Ready to review!

rafaelbn · 2016-02-03T17:50:38Z

Travis fails

yajo · 2016-02-15T16:24:12Z

The name is alright because odoo names for modules are <section>_<functionality>. In this case base indicates the functionality of the multi_image section, not the other way around.

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.

pedrobaeza · 2016-02-15T16:27:18Z

@yajo, you can say what you want, but the name is not correct. All is prefixed with base, not suffixed. If not, I challenge you to show me one 8.0 standard module with the base word at the end. Please stop debating all the reviews you receive and accept humbly the suggestions made by the reviewers.

yajo · 2016-02-15T16:32:37Z

Hmm you are right, I did not know that exception. About the website dependency it's about the usage of assets_frontend view.

dreispt · 2016-02-16T09:20:36Z

The multi_image module is a generic feature, not specifically linked to products.
So I insist it should go in OCA/server-tools.

dreispt · 2016-02-16T09:22:05Z

The website dependency is not needed then: you can add the assets to web instead of website.

yajo · 2016-02-17T17:30:58Z

@dreispt

The multi_image module is a generic feature, not specifically linked to products.
So I insist it should go in OCA/server-tools.

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.

The website dependency is not needed then: you can add the assets to web instead of website.

I'm afraid the assets_frontend view is defined in website. Anyway, it's not too common to want to have an image gallery for something you don't want to show up in your website, don't you think? This is another case of perfect vs practical. I think a module that just adds that template would be an overkill.

pedrobaeza · 2016-02-17T17:50:29Z

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.

yajo · 2016-02-19T15:53:24Z

Do you want 2 modules then?

dreispt · 2016-02-19T16:01:06Z

@yajo you mean 3?

yajo · 2016-02-19T16:04:08Z

yes, sorry.

dreispt · 2016-02-19T16:05:00Z

Can't you just replace the website dependency by a web one?

yajo · 2016-02-19T16:07:14Z

base_multi_image/views/assets.xml

+<openerp>
+<data>
+
+<template id="assets_frontend" inherit_id="website.assets_frontend">


@dreispt Here's why I need website (or split module).

dreispt · 2016-02-19T16:09:11Z

@yajo check https://github.com/OCA/web/blob/9.0/help_popup/views/popup_help_view.xml

yajo · 2016-02-19T16:25:13Z

base_multi_image/views/owner_templates.xml

+<data>
+
+<!-- Call this template in your owner's website page to get a carousel.
+     It needs a `multi_image_owner` variable set. -->


@dreispt These templates are to reuse in frontend website, not backend, so assets must go in frontend.

See usage example in https://github.com/OCA/event/pull/33/files#diff-068a0c4a89bc9582a1c4cf77d01be469R32

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.

dreispt · 2016-02-19T17:21:52Z

I don't understand we we need frontend features in a backend module. Confusing...
Front end features should go in their own modules and use a prefix website_

rafaelbn · 2016-02-23T19:11:08Z

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.

yajo · 2016-02-24T15:12:19Z

Well after confusions have been cleared, let me split this into 3 modules and PR. Thanks everyone!

yajo · 2016-02-24T17:53:18Z

Thanks everyone! 😉 This is now properly split. Please review: OCA/server-tools#374, OCA/website#171, #135, OCA/event#31, OCA/event#33.

dreispt · 2016-02-25T00:08:12Z

@yajo Shoudn't there be a PR for OCA/product-attribute also?

yajo · 2016-02-25T08:59:51Z

True, I forgot to say it: #135

pedrobaeza added the work in progress label Nov 19, 2015

pedrobaeza reviewed Nov 19, 2015
View reviewed changes

lukebranch mentioned this pull request Nov 20, 2015

Are you interested to contribute in the Odoo Community Association? OdooCommunityWidgets/website_navigation_megamenu#3

Closed

pedrobaeza and others added 11 commits February 1, 2016 16:11

[MIG] product_multi_image: Module totally redone from old product_images

1d0f3b1

[FIX] product_multi_image: PEP8

92ee1de

Split model in abstract & concrete.

a0a7ee8

Some code cleanup and simplification.

ce963eb

Update contributors.

1196bd9

Fix some errors in code.

5360f21

Move field upper, so readers read the comment before.

5293c5d

New-style reduced copyright header.

bc9cd76

Improved manifest and README for base module.

ce68548

Spell checking.

98485b4

Refactor

d1363cf

Move most code to base module, move to api.multi, fix some bugs.

yajo added 2 commits February 2, 2016 17:25

Working version.

2766472

Add post_init_hook.

e0d84c1

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.

yajo mentioned this pull request Feb 3, 2016

[8.0][ADD][WIP][event_multi_image] Image gallery for events. OCA/event#31

Merged

1 task

yajo added 2 commits February 4, 2016 16:06

Add images and license key.

0c6fefa

Provide base template for website carousel galleries.

e2dfbcf

yajo mentioned this pull request Feb 4, 2016

[8.0][ADD][website_event_multi_image] Public image gallery in events OCA/event#33

Closed

dreispt added this to the 8.0 milestone Feb 13, 2016

yajo changed the title ~~[8.0][multi_image_base][product_multi_image] Multi images for products split in abstract and concrete models~~ [8.0][base_multi_image][product_multi_image] Multi images for products split in abstract and concrete models Feb 15, 2016

Rename to base_multi_image.

8fe8801

max3903 added work in progress and removed needs review labels Feb 15, 2016

yajo mentioned this pull request Feb 16, 2016

Improve module naming docs. OCA/maintainer-tools#178

Merged

yajo reviewed Feb 19, 2016
View reviewed changes

yajo closed this Feb 24, 2016

yvaucher removed the work in progress label Feb 24, 2016

rafaelbn mentioned this pull request Feb 24, 2016

Porting website_event_photo to OCA/website Antiun/antiun-odoo-addons#30

Open

yajo mentioned this pull request Feb 24, 2016

[8.0][base_multi_image] Base module for models with multiple images (a gallery) OCA/server-tools#374

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[8.0][base_multi_image][product_multi_image] Multi images for products split in abstract and concrete models #112

[8.0][base_multi_image][product_multi_image] Multi images for products split in abstract and concrete models #112

yajo commented Nov 19, 2015

pedrobaeza commented Nov 19, 2015

pedrobaeza Nov 19, 2015

elicoidal Nov 20, 2015

yajo commented Feb 1, 2016

yajo commented Feb 3, 2016

rafaelbn commented Feb 3, 2016

yajo commented Feb 15, 2016

pedrobaeza commented Feb 15, 2016

yajo commented Feb 15, 2016

dreispt commented Feb 16, 2016

dreispt commented Feb 16, 2016

yajo commented Feb 17, 2016

pedrobaeza commented Feb 17, 2016

yajo commented Feb 19, 2016

dreispt commented Feb 19, 2016

yajo commented Feb 19, 2016

dreispt commented Feb 19, 2016

yajo Feb 19, 2016

dreispt commented Feb 19, 2016

yajo Feb 19, 2016

yajo Feb 19, 2016

yajo Feb 19, 2016

dreispt commented Feb 19, 2016

rafaelbn commented Feb 23, 2016

yajo commented Feb 24, 2016

yajo commented Feb 24, 2016

dreispt commented Feb 25, 2016

yajo commented Feb 25, 2016


		To develop a module based on this one:

		* See module ``multi_image_product`` as an example.

[8.0][base_multi_image][product_multi_image] Multi images for products split in abstract and concrete models #112

[8.0][base_multi_image][product_multi_image] Multi images for products split in abstract and concrete models #112

Conversation

yajo commented Nov 19, 2015

pedrobaeza commented Nov 19, 2015

pedrobaeza Nov 19, 2015

Choose a reason for hiding this comment

elicoidal Nov 20, 2015

Choose a reason for hiding this comment

yajo commented Feb 1, 2016

yajo commented Feb 3, 2016

rafaelbn commented Feb 3, 2016

yajo commented Feb 15, 2016

pedrobaeza commented Feb 15, 2016

yajo commented Feb 15, 2016

dreispt commented Feb 16, 2016

dreispt commented Feb 16, 2016

yajo commented Feb 17, 2016

pedrobaeza commented Feb 17, 2016

yajo commented Feb 19, 2016

dreispt commented Feb 19, 2016

yajo commented Feb 19, 2016

dreispt commented Feb 19, 2016

yajo Feb 19, 2016

Choose a reason for hiding this comment

dreispt commented Feb 19, 2016

yajo Feb 19, 2016

Choose a reason for hiding this comment

yajo Feb 19, 2016

Choose a reason for hiding this comment

yajo Feb 19, 2016

Choose a reason for hiding this comment

dreispt commented Feb 19, 2016

rafaelbn commented Feb 23, 2016

yajo commented Feb 24, 2016

yajo commented Feb 24, 2016

dreispt commented Feb 25, 2016

yajo commented Feb 25, 2016