[WIP] [MIG] product_multi_image: Module totally redone from old product_images #57
Conversation
| @api.one | ||
| def unlink(self): | ||
| if self.path: | ||
| os.path.isfile(self.path) and os.remove(self.path) |
There was a problem hiding this comment.
It is unlikely to happen but there is a small chance that 2 calls are done at the same time, one call will raise an OSError. I think the line could be replaced by the following because we do not mind if it does no longer exist.
try:
os.remove(self.path)
except OSError:
pass
There was a problem hiding this comment.
Also, I think the file should be deleter after the call to super, so if something raise in super, the file won't be deleted.
There was a problem hiding this comment.
You're right, thanks.
pedrobaeza
force-pushed
the
8.0-product_images
branch
from
b5bcd11
to
8998fe6
Compare
pedrobaeza
force-pushed
the
8.0-product_images
branch
from
8998fe6
to
fb8a91c
Compare
|
@sebastienbeau, take a look, you have done job on this matter |
|
Hi, I don't understand the problem with runbot. |
|
I forced a rebuild, let's see what happens. |
|
Well, seems to be an error due to the module Maybe it never gets called? I see the module partner_firstname uses |
Those part dispappeared from the PR and was present in the OCA repo. This code reintegrate the missing part OCA#57 (comment)
|
Hi @LeartS Thanks for your report. Thanks |
|
@flotho I've just noticed that this PR is older than that code, @pedrobaeza should just rebase this branch and it should fix itself. |
|
@LeartS |
pedrobaeza
force-pushed
the
8.0-product_images
branch
from
b48ad34
to
f5dc106
Compare
|
Branch rebased. Let's see CIs' result. |
|
Hi, I don't understand the reason of the runbot built failure. It talks about mrp, yet there is no mrp in this PR. |
|
Any chance to have some odoo core updates and rerun this one? |
Those part dispappeared from the PR and was present in the OCA repo. This code reintegrate the missing part OCA#57 (comment)
|
cc @yajo @sergio-incaser @carlos-incaser |
|
|
||
| This module implements the possibility to have multiple images for a product, | ||
| that it's to say, an image gallery. | ||
|
|
There was a problem hiding this comment.
This is missing some important parts such as usage instructions, try on runbot, report issues, etc.
| self.name = self._make_pretty(self.name) | ||
|
|
||
| @api.onchange('path') | ||
| def onchange_path(self): |
There was a problem hiding this comment.
Please prefix with _ following guidelines.
|
Full code review done for now, with some remarks as you see. You did 828 additions and 448 deletions, some tests would be appreciated. Also please fix runbot to start functional review. Thanks! |
|
@yajo, that was done a lot of time ago. My skills have improved since then 😉 I'll check your comments later |
| def _check_url(self): | ||
| if self.type == 'url' and not self.url: | ||
| raise exceptions.ValidationError( | ||
| 'You must provide a URL for the product image.') |
|
I'm thinking of another approach, let's see what you think. What about putting all the gallery-related stuff in an abstract model in a separate module, and then inherit it in products and add the views? I'm sure with time more and more models will benefit of having a gallery (events, blog...). This way we'd reuse the abstract one and repeat less code. |
|
Yeah, it's a very good idea. We need them 2 modules: base_multi_image, with all the stuff that handles the possible sources, cache... and then inherit in product_multi_image, event_multi_image... Are you going to do it? |
|
Let's ask @rafaelbn. |
|
I agree, let's go. Please, talking about images they must be stored outside data base. How to solve this? |
|
For now, there's 3 possibilities:
We can add a fourth option that is "stored as a attachment". |
|
I'm forking this to develop. Close for now if you wish. |
|
OK, eager to see the new approach |
This is still a work in progress.
Refactorized:
New planned features: