fix: Use requests to fetch event and speaker images #6639
Conversation
|
@iamareebjamal The test fails and gives Corrupt/Invalid images for few For now, the image resizing is working with local images as well as postman |
Log Also, tried |
|
That's because It's simply |
Codecov Report
@@ Coverage Diff @@
## development #6639 +/- ##
===============================================
+ Coverage 65.35% 65.35% +<.01%
===============================================
Files 298 298
Lines 15239 15240 +1
===============================================
+ Hits 9959 9960 +1
Misses 5280 5280
Continue to review full report at Codecov.
|
app/api/helpers/files.py
Outdated
|
|
||
| import PIL | ||
| from PIL import Image | ||
| from PIL import Image, ImageFile |
There was a problem hiding this comment.
'PIL.ImageFile' imported but unused
iamareebjamal
changed the title
app/api/helpers/files.py
Outdated
| @@ -77,7 +78,7 @@ def create_save_resized_image(image_file, basewidth=None, maintain_aspect=None, | |||
| if not image_file: | |||
| return None | |||
| filename = '{filename}.{ext}'.format(filename=get_file_name(), ext=ext) | |||
| data = urllib.request.urlopen(image_file).read() | |||
| data = requests.get(image_file, stream=True).content | |||
There was a problem hiding this comment.
Was previously using raw, have updated.
|
dredd tests are broken. Fix them |
|
@iamareebjamal Check now! |
|
Please don't just skip the transactions, just delete the entire dredd tests as well. Why test anything, right? |
|
Sorry, I didn't mean that. Let me dive into dredd, I m new to it. I dont' know what is causing the response to be 500, when the actual response from postman is 201. Let me troubleshoot it again! I will take time and fix it. Thanks |
|
It's simple example.com/example.png does not exist |
|
|
@iamareebjamal What shall be done for this issue I can understand that basewidth is becoming none, but when we are calling the function we are passing the basewidth for every image size, is it somwhere we are passing none to the function means image basewidth is coming to be None? |
|
That, you have to debug as to why it is becoming None |
|
@iamareebjamal Okay debugging, taking time for it. |
codedsun
force-pushed
the
image-fix
branch
3 times, most recently
from
00dc952 to
06b0dfc
Compare
app/api/helpers/files.py
Outdated
| @@ -77,6 +78,7 @@ def create_save_resized_image(image_file, basewidth=None, maintain_aspect=None, | |||
| if not image_file: | |||
| return None | |||
| filename = '{filename}.{ext}'.format(filename=get_file_name(), ext=ext) | |||
| # data = requests.get(image_file).content | |||
There was a problem hiding this comment.
Why? Debug things locally. Don't push intermediate changes
|
@iamareebjamal I logged locally all the basewidth sizes, they are coming to be appropriate sizes and working fine for both the API's. I will take more time to debug, I don't know what is getting wrong. |
|
@iamareebjamal Pheew!!!. This has passed all the test, please check. |
app/api/helpers/files.py
Outdated
| icon_width=75, icon_height=30, icon_aspect=True, | ||
| icon_quality=80, thumbnail_width=500, thumbnail_height=200, | ||
| thumbnail_aspect=True, thumbnail_quality=80, logo_width=500, | ||
| logo_height=200) |
There was a problem hiding this comment.
No need for condition, add all values unconditionally
There was a problem hiding this comment.
The values are different for speaker-type and event-type
There was a problem hiding this comment.
No, they are mutually exclusive
docs/api/blueprint/user/users.apib
Outdated
| @@ -213,7 +213,7 @@ Create a new user using an email, password and an optional name. | |||
| "twitter-url": "http://twitter.com/twitter", | |||
| "instagram-url": "http://instagram.com/instagram", | |||
| "google-plus-url": "http://plus.google.com/plus.google", | |||
| "original-image-url": "https://cdn.pixabay.com/photo/2013/11/23/16/25/birds-216412_1280.jpg" | |||
docs/api/blueprint/user/users.apib
Outdated
| "icon-image-url": "http://example.com/example.png" | ||
| "thumbnail-image-url": "https://image.flaticon.com/icons/png/64/126/126486.png", | ||
| "small-image-url": "https://image.flaticon.com/icons/png/64/126/126486.png", | ||
| "icon-image-url": "https://image.flaticon.com/icons/png/64/126/126486.png" |
There was a problem hiding this comment.
These are read only and should be removed
There was a problem hiding this comment.
remove these fields or revert the change? Removing from the documentation response will be like the documentation is incomplete? Your call, tell me what
There was a problem hiding this comment.
No, they are discarded. So, the documentation will be complete
There was a problem hiding this comment.
@iamareebjamal Test are failing because the request becomes incomplete
There was a problem hiding this comment.
No, they are failing because it's invalid JSON
There was a problem hiding this comment.
reverting this change to the previous one.
There was a problem hiding this comment.
You can't have trailing commas in JSON
codedsun
force-pushed
the
image-fix
branch
2 times, most recently
from
e25eca1 to
dbd71c0
Compare
docs/api/blueprint/user/users.apib
Outdated
| @@ -559,9 +559,6 @@ Authorized user should be same as user in request body or must be admin. | |||
| "twitter-url": "http://twitter.com/twitter", | |||
| "instagram-url": "http://instagram.com/instagram", | |||
| "google-plus-url": "http://plus.google.com/plus.google", | |||
There was a problem hiding this comment.
JSON can't have trailing commas
There was a problem hiding this comment.
That was a blunder, apologies.
|
Clones added
============
- app/api/helpers/files.py 1
See the complete overview on Codacy |
|
Check now! |
|
Great work. Now, work on creating a management task which resizes the images of events and speakers which were not done previously |
iamareebjamal
changed the title
|
This wasn't great as I used to do, it had lot of blunders from my side. Thnkyou for the help and efforts 👍 May be my bad health made it more worse but yes I did even in this case ^_^
New Issue for this? |
|
No, follow up to this PR as it was a part of this issue only |
|
Can you give me any example of management-task written in the repo or tell me what it's similar to as in java-servers.
Okay, will send a new PR with the same issue tag. |
|
manage.py |
|
@iamareebjamal I have created the management command but directly invoking the function of create_resized image, the urllib gives connection refused error. Do I need to invoke the same celery task? |
You replaced urllib with requests yourself |
|
@iamareebjamal Yes, sorry I fixed that. My server was not running hence the error was coming up. Also do i need to create seperate management commands for events and speakers or one? Thanks |
Yes and facing and fixing the dredd tests was fantastic tasks :P |
|
One |
Fixes #6618
Short description of what this resolves:
Resolves image-resizing task which was throwing error 403
Checklist
developmentbranch.