-
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
#56 integrated SMCloudStore with Azure provider; cleaned Item form in frontend #58
Conversation
… properly upload image files; added new migrations for storing image urls
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 looks great! Please either address the three comments with either a bit more code, or new Issues.
// upload images to Azure and get their filenames | ||
const imageUrls = await Promise.all(imageFiles.map((file, index) => { | ||
const formattedDate = new Date().toISOString(); | ||
return uploadToAzure(file, `item-${formattedDate}-${newItem.id}.jpg`); |
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.
assuming .jpg is fine for now, but long-term it might bite you. maybe you want a function to handle detecting file types and generating your uploaded image file name. you could create an issue to do that. it might make a good-first-issue.
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.
Ah it makes sense. I blindly used .jpg but it could be .png image also. I will try to fix it or create a good first issue.
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.
Fixed this one by creating a function to find the file extension based on incoming file MIME type.
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.
starting with Azure makes sense based on what we discussed. do you have an issue created to add an option for all the supported platforms? it seems like it would be a straightforward update to this file, the donated item service, and package.json.
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.
writing a new connection for other cloud storage is easy but I have to create accounts for other cloud providers to get the connection keys, storage account etc which is time consuming. I will create an issue for that. What is the purpose of the multiple cloud providers? Is it for fail safe or to be able to switch to other providers in future?
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.
to switch to other providers in the future, especially if the solution is deployed by other users. solving for BWorks is our first use case, but in talking w/ Patrick my understanding is that his vision has always been something that anyone who does similar things could re-use the tool.
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.
It makes sense. I will work on that.
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.
i don't see any validation that you're only accepting images, indication what sort of images types might be accepted, or limits on file size. these aren't urgent concerns, but you'll probably want them at least in the client, and probably in the server, at some point. maybe just drop a new issue.
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.
It makes sense, Copied!!
…ix few tests to match the new donated item schema
…al-assist into 56-integrate-sm-cloud-store-into-backend
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.
lgtm; Thank you!
Fixes #56
Changes Made:
Reasons for Changes:
Implementation Details:
container-name/file-name
format, facilitating easier and more efficient retrieval.Screenshots
Files uploaded to Azure storage
Postgres storing list of filenames from azure storage