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] im_livechat, web: safely open image with placeholder #32664

Open
wants to merge 3 commits into
base: saas-12.3
from

Conversation

Projects
None yet
4 participants
@alexkuhn
Copy link
Contributor

alexkuhn commented Apr 12, 2019

Task-ID 1924666

@alexkuhn alexkuhn changed the title ] [FIX] im_livechat, web: safely open image with placeholder Apr 12, 2019

@robodoo robodoo added the seen 🙂 label Apr 12, 2019

@alexkuhn alexkuhn requested a review from odony Apr 12, 2019

@C3POdoo C3POdoo added the RD label Apr 12, 2019

@robodoo robodoo added the CI 🤖 label Apr 12, 2019

@seb-odoo
Copy link
Contributor

seb-odoo left a comment

I tested the PR and this fixes the issue.

I'm not really sure about the code though. I made my comments as if we are doing it this way but I'm not sure it is the best one.

@odony I'm not 100% convinced about the need to duplicate the method, I feel like we are adding more complexity which can ultimately lead to making more (security) mistakes.

I do like the idea of having one method for handling images though, especially since the code it would contain has also been copied in other places such as documents and website_profile where by the way they also define (yet another) way to get a placeholder for a user.

Maybe just fix the placeholder method here as it was done, and do a proper refactoring in master if necessary?

@@ -1004,6 +1003,26 @@ def content_common(self, xmlid=None, model='ir.attachment', id=None, field='data
response.set_cookie('fileToken', token)
return response

@http.route(['/web/image',
'/web/user_image/<string:xmlid>',

This comment has been minimized.

Copy link
@seb-odoo

seb-odoo Apr 15, 2019

Contributor

I feel like all of those parameters are overkill, it could be a specific route since you know you'll only ever need the model res.partner and only give an id and a field for now.

Then I would replace user by partner in the name since this is what it really is.

If we really want to keep all of those variants here too, we need to find a way to not have to copy/paste them.

'/web/user_image/<int:id>-<string:unique>/<int:width>x<int:height>',
'/web/user_image/<int:id>-<string:unique>/<int:width>x<int:height>/<string:filename>'], type='http', auth="public")
def content_image_user(self, **kw):
return self.content_image_core(**kw, placeholder='user_placeholder.jpg')

This comment has been minimized.

Copy link
@seb-odoo

seb-odoo Apr 15, 2019

Contributor

It is not a big deal, but this will crash if kw contains placeholder if you don't pop it because you will pass it twice, also I would put **kw as last parameter, and actually name it **kwargs on new methods.

@@ -1021,10 +1040,13 @@ def content_common(self, xmlid=None, model='ir.attachment', id=None, field='data
'/web/image/<int:id>-<string:unique>/<string:filename>',
'/web/image/<int:id>-<string:unique>/<int:width>x<int:height>',
'/web/image/<int:id>-<string:unique>/<int:width>x<int:height>/<string:filename>'], type='http', auth="public")
def content_image(self, xmlid=None, model='ir.attachment', id=None, field='datas',
def content_image(self, **kw):
return self.content_image_core(**kw, placeholder='placeholder.png')

This comment has been minimized.

Copy link
@seb-odoo

seb-odoo Apr 15, 2019

Contributor

I would put the default for placeholder in content_image_core (just like it was in the original code) and here do a kw.pop('placeholder'). This will also prevent the generic url from crashing when given a placeholder, since this is supposedly in stable. Same remark for the other route.

def content_image(self, **kw):
return self.content_image_core(**kw, placeholder='placeholder.png')

def content_image_core(self, placeholder, xmlid=None, model='ir.attachment', id=None, field='datas',

This comment has been minimized.

Copy link
@seb-odoo

seb-odoo Apr 15, 2019

Contributor

If we really want this method, I would prefix it with _.

Also I would take this opportunity to write a proper docstring for it.

@@ -1021,10 +1040,13 @@ def content_common(self, xmlid=None, model='ir.attachment', id=None, field='data
'/web/image/<int:id>-<string:unique>/<string:filename>',
'/web/image/<int:id>-<string:unique>/<int:width>x<int:height>',
'/web/image/<int:id>-<string:unique>/<int:width>x<int:height>/<string:filename>'], type='http', auth="public")
def content_image(self, xmlid=None, model='ir.attachment', id=None, field='datas',
def content_image(self, **kw):

This comment has been minimized.

Copy link
@seb-odoo

seb-odoo Apr 15, 2019

Contributor

If we consider it a change in stable version, you also need to define *args in case someone called the method without specifying the keys.

alexkuhn added some commits Apr 12, 2019

[IMP] im_livechat, web: new controller 'web/user_image'
This controller is almost similar to `web/image` but uses
custom placeholder 'user_placeholder.jpg'.

Task-ID 1924666

@alexkuhn alexkuhn force-pushed the odoo-dev:saas-12.3-fixes-aku branch from 16008af to 9076908 Apr 15, 2019

@robodoo robodoo removed the CI 🤖 label Apr 15, 2019

@alexkuhn alexkuhn force-pushed the odoo-dev:saas-12.3-fixes-aku branch from 9076908 to 7a6d3ce Apr 15, 2019

@robodoo robodoo added the CI 🤖 label Apr 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.