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

[FIX] web_editor: upload CORS images by URL #160085

Conversation

AH-Yussef
Copy link
Contributor

@AH-Yussef AH-Yussef commented Apr 2, 2024

Steps to reproduce:

  • Install E-commerce app
  • Go to Website > Shop, and open a product
  • Click on edit on the upper right corner.
  • Double click the product image to change it. A popup is shown.
  • In the popup, click on Add URL and place this URL 1
  • Click Save. The image disappears!

Investigation:

  • The issue was introduced here 2
  • As URL now starts with /web/image/ which will be later used here 3
  • The condition to enter this line is url_object.path.startswith('/web/image')
  • item[field] is used which equals to attachment[datas]
  • But actually datas is empty as the image upload fails because of CORS
  • add_url is called 4 saving attachment without datas but with type URL

Fix

  • The commit solves the issue by fetching the remote data from the server if the url is a remote redirection, which still happens when the image data cannot be fetched by the client browser at upload time.

opw-3746245

@AH-Yussef AH-Yussef self-assigned this Apr 2, 2024
@robodoo
Copy link
Contributor

robodoo commented Apr 2, 2024

@C3POdoo C3POdoo added the OE the report is linked to a support ticket (opw-...) label Apr 2, 2024
@AH-Yussef AH-Yussef changed the title [FIX] web_editor: upload iamges by URL [FIX] web_editor: upload images by URL Apr 2, 2024
@AH-Yussef AH-Yussef force-pushed the 17.0-opw-3746245-upload_images_by_URL-alhy branch from 4145f37 to d01ed5e Compare April 2, 2024 09:45
@bso-odoo
Copy link
Contributor

bso-odoo commented Apr 2, 2024

The fix looks ok to fallback to the old behavior. 👍
But I was wondering if it would not make sense to detect the scenario upon attachment upload and do this fetch from server only at that time - instead of at every read access ? @rdeodoo

@rdeodoo rdeodoo marked this pull request as ready for review April 11, 2024 15:15
@C3POdoo C3POdoo requested a review from a team April 11, 2024 15:16
@rdeodoo
Copy link
Contributor

rdeodoo commented Apr 11, 2024

Indeed.. I think we did not do that because we did not want the server to contact another random server that any of our client can ask us to.
But this load_local_url is already doing it so.. We should definitely fetch the b64 from there at save if we can't do that through the client side.

About the steps to reproduce, note that the image you provided doesn't work on my side (it doesn't let me go through the media dialog). I still reproduced it using any other cors protected image tho.

@robodoo delegate=@bso-odoo

@AH-Yussef AH-Yussef force-pushed the 17.0-opw-3746245-upload_images_by_URL-alhy branch from d01ed5e to def179c Compare April 16, 2024 00:09
@bso-odoo
Copy link
Contributor

The fix looks ok to fallback to the old behavior. 👍 But I was wondering if it would not make sense to detect the scenario upon attachment upload and do this fetch from server only at that time - instead of at every read access ?

After re-testing, I notice I misunderstood this during my first review. This is actually happening during record save - therefore at upload, which is good news.

@AH-Yussef AH-Yussef force-pushed the 17.0-opw-3746245-upload_images_by_URL-alhy branch from def179c to 12dd433 Compare April 16, 2024 08:57
@AH-Yussef AH-Yussef changed the title [FIX] web_editor: upload images by URL [FIX] web_editor: upload CORS images by URL Apr 16, 2024
@AH-Yussef AH-Yussef force-pushed the 17.0-opw-3746245-upload_images_by_URL-alhy branch 2 times, most recently from d8a5da4 to dcd38d1 Compare April 17, 2024 07:39
- Install E-commerce app
- Go to Website > Shop, and open a product
- Click on edit on the upper right corner.
- Double click the product image to change it. A popup is shown.
- In the popup, click on Add URL and place this URL [1]
- Click Save. The image disappears!

- The issue was introduced here [2]
- As URL now starts with `/web/image/` which will be later used here [3]
- The condition to enter this line is `url_object.path.startswith('/web/image')`
- `item[field]` is used which equals to `attachment[datas]`
- But actually `datas` is empty as the image upload fails because of CORS
- `add_url` is called [4] saving attachment without `datas` but with type URL

- The commit solves the issue by fetching the remote data from the server
if the url is a remote redirection, which still happens when the image
data cannot be fetched by the client browser at upload time.

[1]: https://lowendbox.com/wp-content/uploads/2022/09/odoo_logo_1200.png
[2]: odoo@943944d#diff-0897c80484208197b0aff67f2b06509e864c3976de1589e3cebad2788eef008fR49
[3]: https://github.com/odoo/odoo/blob/f72968561acec164697a7a9ee0965ec304854dd5/addons/web_editor/models/ir_qweb_fields.py#L473
[4]: https://github.com/odoo/odoo/blob/f72968561acec164697a7a9ee0965ec304854dd5/addons/web_editor/controllers/main.py#L268

opw-3746245
@bso-odoo bso-odoo force-pushed the 17.0-opw-3746245-upload_images_by_URL-alhy branch 2 times, most recently from 557e73b to bc74671 Compare April 18, 2024 13:48
@bso-odoo
Copy link
Contributor

@robodoo r+

robodoo pushed a commit that referenced this pull request Apr 19, 2024
- Install E-commerce app
- Go to Website > Shop, and open a product
- Click on edit on the upper right corner.
- Double click the product image to change it. A popup is shown.
- In the popup, click on Add URL and place this URL [1]
- Click Save. The image disappears!

- The issue was introduced here [2]
- As URL now starts with `/web/image/` which will be later used here [3]
- The condition to enter this line is `url_object.path.startswith('/web/image')`
- `item[field]` is used which equals to `attachment[datas]`
- But actually `datas` is empty as the image upload fails because of CORS
- `add_url` is called [4] saving attachment without `datas` but with type URL

- The commit solves the issue by fetching the remote data from the server
if the url is a remote redirection, which still happens when the image
data cannot be fetched by the client browser at upload time.

[1]: https://lowendbox.com/wp-content/uploads/2022/09/odoo_logo_1200.png
[2]: 943944d#diff-0897c80484208197b0aff67f2b06509e864c3976de1589e3cebad2788eef008fR49
[3]: https://github.com/odoo/odoo/blob/f72968561acec164697a7a9ee0965ec304854dd5/addons/web_editor/models/ir_qweb_fields.py#L473
[4]: https://github.com/odoo/odoo/blob/f72968561acec164697a7a9ee0965ec304854dd5/addons/web_editor/controllers/main.py#L268

opw-3746245

closes #160085

Signed-off-by: Benoit Socias (bso) <bso@odoo.com>
robodoo pushed a commit that referenced this pull request Apr 19, 2024
- Install E-commerce app
- Go to Website > Shop, and open a product
- Click on edit on the upper right corner.
- Double click the product image to change it. A popup is shown.
- In the popup, click on Add URL and place this URL [1]
- Click Save. The image disappears!

- The issue was introduced here [2]
- As URL now starts with `/web/image/` which will be later used here [3]
- The condition to enter this line is `url_object.path.startswith('/web/image')`
- `item[field]` is used which equals to `attachment[datas]`
- But actually `datas` is empty as the image upload fails because of CORS
- `add_url` is called [4] saving attachment without `datas` but with type URL

- The commit solves the issue by fetching the remote data from the server
if the url is a remote redirection, which still happens when the image
data cannot be fetched by the client browser at upload time.

[1]: https://lowendbox.com/wp-content/uploads/2022/09/odoo_logo_1200.png
[2]: 943944d#diff-0897c80484208197b0aff67f2b06509e864c3976de1589e3cebad2788eef008fR49
[3]: https://github.com/odoo/odoo/blob/f72968561acec164697a7a9ee0965ec304854dd5/addons/web_editor/models/ir_qweb_fields.py#L473
[4]: https://github.com/odoo/odoo/blob/f72968561acec164697a7a9ee0965ec304854dd5/addons/web_editor/controllers/main.py#L268

opw-3746245

closes #160085

Signed-off-by: Benoit Socias (bso) <bso@odoo.com>
robodoo pushed a commit that referenced this pull request Apr 19, 2024
- Install E-commerce app
- Go to Website > Shop, and open a product
- Click on edit on the upper right corner.
- Double click the product image to change it. A popup is shown.
- In the popup, click on Add URL and place this URL [1]
- Click Save. The image disappears!

- The issue was introduced here [2]
- As URL now starts with `/web/image/` which will be later used here [3]
- The condition to enter this line is `url_object.path.startswith('/web/image')`
- `item[field]` is used which equals to `attachment[datas]`
- But actually `datas` is empty as the image upload fails because of CORS
- `add_url` is called [4] saving attachment without `datas` but with type URL

- The commit solves the issue by fetching the remote data from the server
if the url is a remote redirection, which still happens when the image
data cannot be fetched by the client browser at upload time.

[1]: https://lowendbox.com/wp-content/uploads/2022/09/odoo_logo_1200.png
[2]: 943944d#diff-0897c80484208197b0aff67f2b06509e864c3976de1589e3cebad2788eef008fR49
[3]: https://github.com/odoo/odoo/blob/f72968561acec164697a7a9ee0965ec304854dd5/addons/web_editor/models/ir_qweb_fields.py#L473
[4]: https://github.com/odoo/odoo/blob/f72968561acec164697a7a9ee0965ec304854dd5/addons/web_editor/controllers/main.py#L268

opw-3746245

closes #160085

Signed-off-by: Benoit Socias (bso) <bso@odoo.com>
@robodoo robodoo closed this in 0810438 Apr 19, 2024
goffauxs pushed a commit to odoo-dev/odoo that referenced this pull request Apr 25, 2024
- Install E-commerce app
- Go to Website > Shop, and open a product
- Click on edit on the upper right corner.
- Double click the product image to change it. A popup is shown.
- In the popup, click on Add URL and place this URL [1]
- Click Save. The image disappears!

- The issue was introduced here [2]
- As URL now starts with `/web/image/` which will be later used here [3]
- The condition to enter this line is `url_object.path.startswith('/web/image')`
- `item[field]` is used which equals to `attachment[datas]`
- But actually `datas` is empty as the image upload fails because of CORS
- `add_url` is called [4] saving attachment without `datas` but with type URL

- The commit solves the issue by fetching the remote data from the server
if the url is a remote redirection, which still happens when the image
data cannot be fetched by the client browser at upload time.

[1]: https://lowendbox.com/wp-content/uploads/2022/09/odoo_logo_1200.png
[2]: odoo@943944d#diff-0897c80484208197b0aff67f2b06509e864c3976de1589e3cebad2788eef008fR49
[3]: https://github.com/odoo/odoo/blob/f72968561acec164697a7a9ee0965ec304854dd5/addons/web_editor/models/ir_qweb_fields.py#L473
[4]: https://github.com/odoo/odoo/blob/f72968561acec164697a7a9ee0965ec304854dd5/addons/web_editor/controllers/main.py#L268

opw-3746245

closes odoo#160085

Signed-off-by: Benoit Socias (bso) <bso@odoo.com>
willylohws pushed a commit to willylohws/odoo that referenced this pull request May 1, 2024
- Install E-commerce app
- Go to Website > Shop, and open a product
- Click on edit on the upper right corner.
- Double click the product image to change it. A popup is shown.
- In the popup, click on Add URL and place this URL [1]
- Click Save. The image disappears!

- The issue was introduced here [2]
- As URL now starts with `/web/image/` which will be later used here [3]
- The condition to enter this line is `url_object.path.startswith('/web/image')`
- `item[field]` is used which equals to `attachment[datas]`
- But actually `datas` is empty as the image upload fails because of CORS
- `add_url` is called [4] saving attachment without `datas` but with type URL

- The commit solves the issue by fetching the remote data from the server
if the url is a remote redirection, which still happens when the image
data cannot be fetched by the client browser at upload time.

[1]: https://lowendbox.com/wp-content/uploads/2022/09/odoo_logo_1200.png
[2]: odoo@943944d#diff-0897c80484208197b0aff67f2b06509e864c3976de1589e3cebad2788eef008fR49
[3]: https://github.com/odoo/odoo/blob/f72968561acec164697a7a9ee0965ec304854dd5/addons/web_editor/models/ir_qweb_fields.py#L473
[4]: https://github.com/odoo/odoo/blob/f72968561acec164697a7a9ee0965ec304854dd5/addons/web_editor/controllers/main.py#L268

opw-3746245

closes odoo#160085

Signed-off-by: Benoit Socias (bso) <bso@odoo.com>
@fw-bot fw-bot deleted the 17.0-opw-3746245-upload_images_by_URL-alhy branch May 3, 2024 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OE the report is linked to a support ticket (opw-...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants