-
Notifications
You must be signed in to change notification settings - Fork 4
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
API response should include a URL for each image of an item #178
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, I wrote some minor comments but you can merge
@@ -189,6 +189,15 @@ def enrich_item(item, db=None, collection_name=None): | |||
if pictures: | |||
main_image_id = None | |||
for image in pictures: | |||
# Add 'PictureUrl' to all images of an item | |||
image_id = image['PictureId'] | |||
if item.has_key('bagnowka'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to also check if the value is True, not only that it exists
can be done like this:
if item.get('bagnowka'):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a bagnowka key, the value is always "True".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but you can't know that it will always be true, in the future as well..
@@ -189,6 +189,15 @@ def enrich_item(item, db=None, collection_name=None): | |||
if pictures: | |||
main_image_id = None | |||
for image in pictures: | |||
# Add 'PictureUrl' to all images of an item | |||
image_id = image['PictureId'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are sure that image will always have 'PictureId'? if not, then it will raise an exception here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is what happened on test environment, worth to look into it - why there are images without PictureId
(See #177)
API adds a URL to every picture in
Pictures
(to all Items, in order to provide front all URLs without needing to generate them).Example for added
PictureUrl
in expected response: