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

feat: upload image to aws s3 #200

Closed
wants to merge 1 commit into from

Conversation

CleverFool77
Copy link
Contributor

Description

  • Save User images on Amazon S3
  • Save object URL of image in aws s3 on UserModel photo_url field
  • Return photo_url on new api endpoint user/profile_picture

Fixes #152

Server will be getting an encoded string of image as input,The image will be decoded and uploaded on aws s3. Then aws s3 url of the image will be saved in the db field of user's photo_url. A new api endpoint to return the user's profile picture is created.
The testing has been done in my own aws s3 bucket. Three environment variables AWS_ACCESS_KEY_ID , AWS_SECRET_ACCESS_KEY and AWS_PROFILE_BUCKET are created. The username is taken as image name to avoid multiple images of user.
I've extracted the image extension from the encoded string input (uploaded from frontend).

It's assumed that the frontend encodes the image in base64 encoding.

Type of Change:

Delete irrelevant options.

  • Code
  • Quality Assurance
  • User Interface
  • Outreach
  • Documentation

Code/Quality Assurance Only

  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

This has been tested using postman.

Checklist:

Delete irrelevant options.

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
  • Any dependent changes have been merged
  • Update Postman API at /docs folder
  • Update Swagger documentation and the exported file at /docs folder
  • Update requirements.txt

Code/Quality Assurance Only

  • My changes generate no new warnings
  • My PR currently breaks something (fix or feature that would cause existing functionality to not work as expected)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been published in downstream modules

@sys-bot
Copy link

sys-bot commented Feb 21, 2019

Please send PRs only to approved issues.

@sys-bot sys-bot closed this Feb 21, 2019
@CleverFool77
Copy link
Contributor Author

@isabelcosta @ramitsawhney27 I've opened a Pr reagrding the upload of image in amazon s3 for the issue #152

app/api/dao/user.py Outdated Show resolved Hide resolved
app/api/dao/user.py Outdated Show resolved Hide resolved
app/api/dao/user.py Outdated Show resolved Hide resolved
@CleverFool77 CleverFool77 force-pushed the aws_s3_upload branch 2 times, most recently from e1ac1b6 to ca69d2d Compare March 3, 2019 08:11
@CleverFool77
Copy link
Contributor Author

@ramitsawhney27 @agarwalrounak I've updated the PR.

app/api/dao/user.py Outdated Show resolved Hide resolved
@CleverFool77 CleverFool77 force-pushed the aws_s3_upload branch 2 times, most recently from b9811f8 to 707e87a Compare March 4, 2019 16:58
@CleverFool77
Copy link
Contributor Author

@ramitsawhney27 I've updated the PR with the changes and removed the merge conflict.

@isabelcosta
Copy link
Member

@CleverFool77 uploading an image is testable on Postman? I have to try this. Thank you so much for the work you're doing here.

@CleverFool77
Copy link
Contributor Author

CleverFool77 commented Mar 6, 2019

@isabelcosta Yes we can test it on Postman.

  • In the code, we have to setup the environment variable which include the name of bucket and other aws details.
  • As the input image in encoded string , so in update user profile endpoint. we put the encoded string in base64 of image as input.
  • The further process will be according to code - Uploading decoded image to aws s3, storing the image url from aws s3 to database.
  • The link of user's profile image url in amazon aws s3 is returned in Profile picture endpoint.

ramitsawhney27
ramitsawhney27 previously approved these changes Mar 11, 2019
Copy link
Member

@isabelcosta isabelcosta left a comment

Choose a reason for hiding this comment

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

@CleverFool77 I left two comments. I haven't got the time to test this feature yet. Once I do, I'll give you a complete review

app/api/dao/user.py Outdated Show resolved Hide resolved
app/api/dao/user.py Outdated Show resolved Hide resolved
@isabelcosta
Copy link
Member

@m-murad we have to setup these (e.g.: AWS_ACCESS_KEY and etc) on TravisCI right?
We need @mayburgos help with the credentials as well.

Copy link
Member

@isabelcosta isabelcosta left a comment

Choose a reason for hiding this comment

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

@CleverFool77 I'm ok with this after you make the changes I requested ;)

@CleverFool77
Copy link
Contributor Author

Hi @isabelcosta I've updated the PR as requested.
Thanks :)

ramitsawhney27
ramitsawhney27 previously approved these changes Mar 21, 2019
Copy link
Member

@isabelcosta isabelcosta left a comment

Choose a reason for hiding this comment

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

@CleverFool77 left a comment for a typo. It's almost done ;)

if 'photo_url' in data and data['photo_url']:
image_type = data['photo_url'].split(';')[0].split('/')[1]
image_name = ".".join((username, image_type))
photo_url = aws_upload.upload_to_aws(data['photo_url'], application.congif["AWS_PROFILE_BUCKET"], image_name)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
photo_url = aws_upload.upload_to_aws(data['photo_url'], application.congif["AWS_PROFILE_BUCKET"], image_name)
photo_url = aws_upload.upload_to_aws(data['photo_url'], application.config["AWS_PROFILE_BUCKET"], image_name)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated .

Copy link
Member

Choose a reason for hiding this comment

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

@CleverFool77 this doesn't appear to be changed. Maybe it happened something when you squashed the commits. Can you please change that.

Suggested change
photo_url = aws_upload.upload_to_aws(data['photo_url'], application.congif["AWS_PROFILE_BUCKET"], image_name)
photo_url = aws_upload.upload_to_aws(data['photo_url'], application.config["AWS_PROFILE_BUCKET"], image_name)

config.py Show resolved Hide resolved
@CleverFool77
Copy link
Contributor Author

Hi @isabelcosta
I missed out the mention.
I've updated the PR.
Thanks for the patience !!

ramitsawhney27
ramitsawhney27 previously approved these changes Apr 14, 2019
@isabelcosta
Copy link
Member

@CleverFool77 thank you so much for your contribution and patience! Well done! 🎉

I'm not sure when I will merge this because I haven't figure out yet if it's better to set up the environments to the AWS Systers uses, or not. I'm also analyzing the volatile database issue and where data will be stored, and all of this thinking about the development environment. Your work here is done, just wanted to let you know where I'm at.

There's also the possibility of merging this without the environment variables set, and all run ok. I have to test this, yet. Once I do, if all goes well I can merge right away.

cc @ramitsawhney27

Copy link
Member

@isabelcosta isabelcosta left a comment

Choose a reason for hiding this comment

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

@CleverFool77 left some comments because of something I noticed on the Travis CI current configuration.
Let me know if you need any clarification.

config.py Outdated
@@ -14,6 +14,11 @@ class BaseConfig(object):
JWT_ACCESS_TOKEN_EXPIRES = timedelta(weeks=1)
JWT_REFRESH_TOKEN_EXPIRES = timedelta(weeks=4)

# Aws details
AWS_ACCESS_KEY_ID = os.getenv('AWS_ACCESS_KEY_ID')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
AWS_ACCESS_KEY_ID = os.getenv('AWS_ACCESS_KEY_ID')
AWS_ACCESS_KEY_ID = os.getenv('AWS_ACCESS_ID')

Can you please change the expected environment variable to AWS_ACCESS_ID. I'm saying this because during the Community Open Session with @mayburgos and @m-murad that Travis CI has that variable like AWS_ACCESS_ID instead of AWS_ACCESS_KEY_ID.

if 'photo_url' in data and data['photo_url']:
image_type = data['photo_url'].split(';')[0].split('/')[1]
image_name = ".".join((username, image_type))
photo_url = aws_upload.upload_to_aws(data['photo_url'], application.congif["AWS_PROFILE_BUCKET"], image_name)
Copy link
Member

Choose a reason for hiding this comment

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

@CleverFool77 this doesn't appear to be changed. Maybe it happened something when you squashed the commits. Can you please change that.

Suggested change
photo_url = aws_upload.upload_to_aws(data['photo_url'], application.congif["AWS_PROFILE_BUCKET"], image_name)
photo_url = aws_upload.upload_to_aws(data['photo_url'], application.config["AWS_PROFILE_BUCKET"], image_name)

@CleverFool77
Copy link
Contributor Author

Thanks @isabelcosta for the review.
I'll update the PR soon.

@CleverFool77
Copy link
Contributor Author

Hii @isabelcosta !!
I have updated the PR as requested.

Thanks !!!

@vj-codes
Copy link
Member

vj-codes commented Oct 7, 2020

@CleverFool77 hey can you please solve the merge conflicts ? Thanks!

@vj-codes
Copy link
Member

@CleverFool77 closing the PR due to inactivity , thank you for your contribution :)

@vj-codes vj-codes closed this Feb 20, 2021
@isabelcosta
Copy link
Member

@vj-codes thank you for closing this. At the moment we don't have access to AWS S3 so this wouldn't be merged so soon. We'll reopen if the situation changes.
Thank you for your contribution @CleverFool77 , I hope you learned something new with this PR, I sure did :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upload photos to Amazon S3
6 participants