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

[16.0][mig] product_multi_image #1707

Open
wants to merge 24 commits into
base: 16.0
Choose a base branch
from

Conversation

IJOL
Copy link

@IJOL IJOL commented Aug 9, 2024

No description provided.

@IJOL
Copy link
Author

IJOL commented Aug 9, 2024

Waiting for a merge in server-tools

@IJOL IJOL force-pushed the 16.0-mig-product_multi_image branch 2 times, most recently from 05b9819 to b0ab882 Compare August 9, 2024 14:00
@IJOL IJOL force-pushed the 16.0-mig-product_multi_image branch from f884ced to e4b0c54 Compare August 19, 2024 14:43
@thomaspaulb
Copy link

@IJOL Can you rebase to see what this now does

@IJOL IJOL force-pushed the 16.0-mig-product_multi_image branch 4 times, most recently from d3286f3 to 57044a6 Compare September 11, 2024 15:49
@IJOL
Copy link
Author

IJOL commented Sep 11, 2024

Green Green, needs review thought

@IJOL IJOL force-pushed the 16.0-mig-product_multi_image branch 4 times, most recently from 71234e0 to 2373ea9 Compare September 19, 2024 14:33
@AurelijaNorvaisaite
Copy link

@jacekmichalski @rousseldenis @thomaspaulb maybe you could review the code?

Copy link
Contributor

@bosd bosd left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this. Please remove the new introduced comments in the migration commit.

product_multi_image/__manifest__.py Outdated Show resolved Hide resolved
@IJOL IJOL force-pushed the 16.0-mig-product_multi_image branch from 7d33f46 to 6317125 Compare September 30, 2024 13:32
@IJOL
Copy link
Author

IJOL commented Sep 30, 2024

Thanks for your work on this. Please remove the new introduced comments in the migration commit.

I dont think I fully understand this remark..

@IJOL IJOL force-pushed the 16.0-mig-product_multi_image branch from 6317125 to 91a12df Compare September 30, 2024 13:44
Copy link
Contributor

@bosd bosd left a comment

Choose a reason for hiding this comment

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

To clarify this is what I was pointing at.

product_multi_image/tests/test_product_multi_image.py Outdated Show resolved Hide resolved
product_multi_image/tests/test_product_multi_image.py Outdated Show resolved Hide resolved
product_multi_image/tests/test_product_multi_image.py Outdated Show resolved Hide resolved
@IJOL IJOL force-pushed the 16.0-mig-product_multi_image branch from 91a12df to 2cc4f89 Compare September 30, 2024 13:47
@IJOL IJOL force-pushed the 16.0-mig-product_multi_image branch from a9e1dc3 to 12e3233 Compare September 30, 2024 13:55
@yvaucher
Copy link
Member

/ocabot migration product_multi_image

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Sep 30, 2024
@OCA-git-bot OCA-git-bot mentioned this pull request Sep 30, 2024
60 tasks
Copy link
Member

@yvaucher yvaucher left a comment

Choose a reason for hiding this comment

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

LGTM

Code review

@psugne
Copy link

psugne commented Oct 10, 2024

@bosd @yvaucher How is going here? Any timeline when changes will be merged to 16.0 version?

Copy link
Contributor

@bosd bosd left a comment

Choose a reason for hiding this comment

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

Thanks for all your work on this module. 👍
I have found some warnings in the log to call deprecated flush method.Added code suggestions to fix it.
I noticed there is some leftover from previous migration.

I have found some things that didn't work during functional tests.

On product variants incorrect values are shown in the variants section.

To reproduce....
Add an image from the product variant.Go to a product variant, 
On Images tab click ADD.
In the screen that opens there is a "Visible in these Variants" section.
When clicking on the dropdown.
You can select another totally unrelated product (variant).
Which is actually quite confusing.
It should be restricted automatically to the variant you are editing.
But when you click save.
It actually does restrict the image to the variant you are editing.

First image is not saved when creating a new product.

To reproduce....
Create a new product.
Add one image, click save and new,
Add the second product image.
Hit the save button (The cloud symbol) Then you will notice the content of the first image is deleted.

Image is not saved on a newly created product

To reproduce....
Create a new product.
Go to images tab. Click add. select a image.Click "Save & Close".
You will see a preview of the image on the images tab.
Once you click the save button (The cloud symbol).
The image preview is deleted.

Image is deleted when changing the sequence.

To reproduce....
Create or select an existing product.
click on save button (The cloud icon).
Click the add button to add a image.
Upload an image click save and close.
Back in the product click on save button (The cloud icon).
Upload another image click save and close.
Back in the product click on save button (The cloud icon).
Click on the image you last uploaded to edit it.
Change sequence from 10 to a lower number.. e.g. 9
Click save
Now it is ok. Earlier my test was failling. Likely when not pressing the cloud icon.

It is possible to add an empty image

Rather more a "feature then a big bug"
To reproduce....
Create or select an existing product.
On Images tab click ADD.
Fill nothing in... Leave all empty. Click on save button.You have just added an empty picture to your product.

Adding an image from url results in an error.

Use the url field when adding an image.

``` Traceback (most recent call last):   File "/opt/odoo/odoo/http.py", line 1651, in _serve_db     return service_model.retrying(self._serve_ir_http, self.env)   File "/opt/odoo/odoo/service/model.py", line 133, in retrying     result = func()   File "/opt/odoo/odoo/http.py", line 1678, in _serve_ir_http     response = self.dispatcher.dispatch(rule.endpoint, args)   File "/opt/odoo/odoo/http.py", line 1882, in dispatch     result = self.request.registry['ir.http']._dispatch(endpoint)   File "/opt/odoo/addons/website/models/ir_http.py", line 237, in _dispatch     response = super()._dispatch(endpoint)   File "/opt/odoo/odoo/addons/base/models/ir_http.py", line 154, in _dispatch     result = endpoint(**request.params)   File "/opt/odoo/odoo/http.py", line 734, in route_wrapper     result = endpoint(self, *args, **params_ok)   File "/opt/odoo/addons/web/controllers/dataset.py", line 42, in call_kw     return self._call_kw(model, method, args, kwargs)   File "/opt/odoo/addons/web/controllers/dataset.py", line 33, in _call_kw     return call_kw(request.env[model], method, args, kwargs)   File "/opt/odoo/odoo/api.py", line 468, in call_kw     result = _call_kw_multi(method, model, args, kwargs)   File "/opt/odoo/odoo/api.py", line 453, in _call_kw_multi     result = method(recs, *args, **kwargs)   File "/opt/odoo/odoo/models.py", line 6614, in onchange     record._onchange_eval(name, field_onchange[name], result)   File "/opt/odoo/odoo/models.py", line 6325, in _onchange_eval     method_res = method(self)   File "/opt/odoo-venv/lib/python3.10/site-packages/odoo/addons/base_multi_image/models/image.py", line 91, in _onchange_load_from     self.name, self.extension = os.path.splitext(filename) AttributeError: 'base_multi_image.image' object has no attribute 'extension'

The above server error caused the following client error:
null

</details>

These 2 commits should be squashed:
![image](https://github.com/user-attachments/assets/4cfd9984-61dd-4dae-bdaa-c0975dde8dda)

{"image_1920": self.grey_image},
)
]
self.product_template.refresh()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.product_template.refresh()
self.product_template.invalidate_cache()

def test_edit_image_variant(self):
text = "Test name changed"
self.product_1.image_ids[0].name = text
self.product_template.refresh()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.product_template.refresh()
self.product_template.invalidate_cache()

def test_remove_image_all_variants(self):
self.product_1.image_ids = [(3, self.product_1.image_ids[0].id)]
self.product_2.image_ids = [(3, self.product_2.image_ids[0].id)]
self.product_template.refresh()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.product_template.refresh()
self.product_template.invalidate_cache()


def test_remove_image_variant(self):
self.product_1.image_ids = [(3, self.product_1.image_ids[0].id)]
self.product_template.refresh()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.product_template.refresh()
self.product_template.invalidate_cache()

Comment on lines +1 to +3
* When you change the image on the product variant, the preview image of the
*Images* tab doesn't get refreshed until you refresh the browser, or if you
go to its template, but the image has been actually saved!
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* When you change the image on the product variant, the preview image of the
*Images* tab doesn't get refreshed until you refresh the browser, or if you
go to its template, but the image has been actually saved!

This actually has been fixed. So can be removed from the roadmap.

#. If you add an image here, the image is actually added to the product
template, and restricted to this variant.
#. When editing an existing image, the image is changed generally for all
the variants where is enabled, not only for this variant.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
the variants where is enabled, not only for this variant.
the variants where it is enabled, not only for this variant.

the variants where is enabled, not only for this variant.
#. When removing an image from this form, if the image is only in this variant,
the image is removed. Otherwise, the image gets restricted to the rest of
the variants where is available.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
the variants where is available.
the variants where it is used.

Comment on lines +24 to +26
.. image:: https://odoo-community.org/website/image/ir.attachment/5784_f2813bd/datas
:alt: Try me on Runbot
:target: https://runbot.odoo-community.org/runbot/135/9.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.. image:: https://odoo-community.org/website/image/ir.attachment/5784_f2813bd/datas
:alt: Try me on Runbot
:target: https://runbot.odoo-community.org/runbot/135/9.0

Runbot 9 is no longer there. Better remove this section.

image.product_variant_count,
1,
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add an test to import image from url. As it is failling with an error currently.

Choose a reason for hiding this comment

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

Maybe add an test to import image from url. As it is failling with an error currently.

@bosd I think this issue has been fixed in this PR - OCA/server-tools#3168

product_multi_image/models/product_product.py Outdated Show resolved Hide resolved
Co-authored-by: bosd <c5e2fd43-d292-4c90-9d1f-74ff3436329a@anonaddy.me>
@AurelijaNorvaisaite
Copy link

@IJOL How is going here?

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.