Skip to content
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

[8.0][base_multi_image] Base module for models with multiple images (a gallery) #374

Merged
merged 11 commits into from
Mar 9, 2016
Merged

Conversation

yajo
Copy link
Member

@yajo yajo commented Feb 24, 2016

Following what was discussed in OCA/product-attribute#112, this is the base part of the module stack, without website dependencies.

This module allows other submodules to implement multiple images in very few steps.

@rafaelbn

@rafaelbn
Copy link
Member

Thanks
cc @pedrobaeza @antespi @sergio-incaser @carlos-incaser

@carlosdauden
Copy link
Contributor

It's easy to refresh the image to make it look when selected?
For the rest ok 👍

@rafaelbn
Copy link
Member

rafaelbn commented Mar 2, 2016

Please @pedrobaeza @antespi @dreispt could you take a look here?


class MyOwner(models.Model):
_name = "mymodule.name"
_inherit = "base_multi_image.owner"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most common example will inherit from an existing model and this abstract one, so I think it's better to put it here:

class ...:
    _name = "name"
    _inherit = ["base_multi_image.owner", "base_model"]

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@andhit-r
Copy link
Member

andhit-r commented Mar 3, 2016

LGTM 👍

* Sharoon Thomas
* Pedro M. Baeza <pedro.baeza@serviciosbaeza.com>
* Rafael Blasco <rafabn@antiun.com>
* Jairo Llopis <yajo.sk8@gmail.com>
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No one from Akretion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Akretion has nothing to do with this. Why do you say that?

@pedrobaeza
Copy link
Member

I'm thinking that is interesting to have a controller that allows to retrieve these images via a URL, so that you can link them in any other site.

return False

@api.multi
def _get_image_from_url(self):
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider using api cache here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was desirable, but for now, there's no cache mechanism. Do you know any quick mechanism?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@dreispt
Copy link
Sponsor Member

dreispt commented Mar 3, 2016

Nice feature, smart implementation, well done!
I made a few comments on the design an implementation.

@dreispt dreispt added this to the 8.0 milestone Mar 3, 2016
@yajo
Copy link
Member Author

yajo commented Mar 3, 2016

All should be fixed.

@dreispt
Copy link
Sponsor Member

dreispt commented Mar 3, 2016

@yajo following discussion above, it would be nice to add to the README Roadmap, the idea of adding a "attachment" storage mode. Maybe someone contributes that in the future.

@dreispt
Copy link
Sponsor Member

dreispt commented Mar 3, 2016

I'm thinking that is interesting to have a controller that allows to retrieve these images via a URL, so that you can link them in any other site.

@pedrobaeza I think that should go on the website companion module. I think @yajo also isolated that in a module.

@pedrobaeza
Copy link
Member

@dreispt, I thought about putting on website module, but it's something similar to report, that has a controller for getting reports even without website installed.

@antespi
Copy link

antespi commented Mar 3, 2016

LGTM 👍
Thanks @yajo

@dreispt
Copy link
Sponsor Member

dreispt commented Mar 3, 2016

@pedrobaeza OK. My concern in only to avoid pushing the website dependency. That could be added now ot put in the roadmap.

@pedrobaeza
Copy link
Member

@dreispt, you don't need website for making a controller. See the report example.

@rafaelbn
Copy link
Member

rafaelbn commented Mar 3, 2016

Thanks! 👍 the first piece

@pedrobaeza
Copy link
Member

@yajo, can you add the missing features commented to the Known issues section?

@rafaelbn
Copy link
Member

rafaelbn commented Mar 8, 2016

Which known issues @pedrobaeza ?

@pedrobaeza
Copy link
Member

It has been discussed all over the PR: store as attachment and so on

@yajo
Copy link
Member Author

yajo commented Mar 9, 2016

Yes, hold on.

@yajo
Copy link
Member Author

yajo commented Mar 9, 2016

There you have the known issues.

About the controller, Odoo already includes website.image_url that makes the dirty job for you, so I see no need for now.

This should be ready to merge then.

pedrobaeza added a commit that referenced this pull request Mar 9, 2016
[8.0][base_multi_image] Base module for models with multiple images (a gallery)
@pedrobaeza pedrobaeza merged commit 542a7e8 into OCA:8.0 Mar 9, 2016
@yajo yajo deleted the multi_image branch March 9, 2016 16:29
@yajo
Copy link
Member Author

yajo commented Mar 9, 2016

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants