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 picture fetching as an attribute #294

Merged
merged 1 commit into from
Jan 23, 2018

Conversation

jntullo
Copy link

@jntullo jntullo commented Jan 19, 2018

Fetching a picture as an attribute is causing the same error that we saw #292, where it returns:

"kind": "internal_server_error",
"message": "\"\\x89\" from ASCII-8BIT to UTF-8",
"klass": "Encoding::UndefinedConversionError"

due to the changes in picture content.

This change creates a picture mixin to both format the way a picture object will now be returned, as well as provide a fetch method for collections with picture attributes.

Note, that I only added the module to collections that we were testing picture attribute selection on. We may need to add it to more in the future, or consider other options such as a picture serializer.

@miq-bot add_label bug, gaprindashvili/yes
@miq-bot assign @abellotti

@jntullo
Copy link
Author

jntullo commented Jan 19, 2018

cc: @himdel

…ent collections. This will allow for formatting of picture responses to not return the picture content and will return the picture image_href by default
"MS4wLyI+CiAgICAgICAgIDx0aWZmOk9yaWVudGF0aW9uPjE8L3RpZmY6T3Jp"\
"ZW50YXRpb24+CiAgICAgIDwvcmRmOkRlc2NyaXB0aW9uPgogICA8L3JkZjpS"\
"REY+CjwveDp4bXBtZXRhPgpMwidZAAAADUlEQVQIHWNgYGCwBQAAQgA+3N0+"\
"xQAAAABJRU5ErkJggg=="
Copy link
Member

@Fryguy Fryguy Jan 19, 2018

Choose a reason for hiding this comment

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

Now you know I HAVE to see what this picture is cc @bdunne

Copy link
Author

Choose a reason for hiding this comment

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

@Fryguy let me know, I added this in a while ago and forgot 😆 maybe a heart?

Copy link
Member

Choose a reason for hiding this comment

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

awww..just a blank square :/

Copy link
Contributor

Choose a reason for hiding this comment

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

I *think* IIRC this is a single pixel - maybe the smallest possible image?

@jntullo jntullo force-pushed the picture_fetch_method branch from 46e8fdc to 026a716 Compare January 19, 2018 19:01
@miq-bot
Copy link
Member

miq-bot commented Jan 19, 2018

Checked commit jntullo@026a716 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
8 files checked, 0 offenses detected
Everything looks fine. 🏆

@himdel
Copy link
Contributor

himdel commented Jan 19, 2018

Thanks, confirming this fixes my /api/service_templates?expand=resources&attributes=picture issue :)

Heads up: SUI's Service Catalog section isn't accessible without this fix (assuming any pictures I guess), so possibly this should be a blocker.

@jntullo
Copy link
Author

jntullo commented Jan 19, 2018

@miq-bot add_label blocker

@abellotti
Copy link
Member

LGTM!! Thanks @jntullo for fixing this ...

@abellotti
Copy link
Member

ping @Fryguy @imtayadeway please review/ ✅ when you get a chance. Thanks!!

Copy link
Contributor

@imtayadeway imtayadeway left a comment

Choose a reason for hiding this comment

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

I'm good with this. The only concern I have is the duplicated knowledge of which things can have a picture - if that changes on the backend will someone remember to update this too? I'm wondering if the mixin should be included in the base controller?

format_picture_response(resource.picture)
end

def format_picture_response(picture)
Copy link
Contributor

Choose a reason for hiding this comment

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

I need to get that serializers PR finished ;)

Copy link
Author

Choose a reason for hiding this comment

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

@imtayadeway yes please! 😛

@jntullo
Copy link
Author

jntullo commented Jan 20, 2018

@imtayadeway Yeah, that was a concern I had as well. Even if we include the module in the base controller, it is still looking for methods with the resource name, ie fetch_service_picture, which will require a change. I think by including the module in the individual controllers, it is a bit more clear for now about where things are coming from. I was going to refactor this to work for any resource that may have a picture, but felt this was a bit out of scope of the fix for now (but I do want to get back to refactoring it)

@abellotti
Copy link
Member

Looks like this is good to go. Thanks @jntullo for fixing this 👍

@abellotti abellotti added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 23, 2018
@abellotti abellotti merged commit 99a3b63 into ManageIQ:master Jan 23, 2018
simaishi pushed a commit that referenced this pull request Jan 23, 2018
Fix picture fetching as an attribute
(cherry picked from commit 99a3b63)
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit a5dfea6a4fc39fdaa86f9b4927e5adb1527cacd0
Author: Alberto Bellotti <[email protected]>
Date:   Tue Jan 23 09:13:46 2018 -0500

    Merge pull request #294 from jntullo/picture_fetch_method
    
    Fix picture fetching as an attribute
    (cherry picked from commit 99a3b6303e6674840b9114988dfeb7d59bc4731a)

@jntullo jntullo deleted the picture_fetch_method branch February 14, 2018 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants