-
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
Dka36/EmployeePhotoS3 #558
base: master
Are you sure you want to change the base?
Conversation
…ing the old tables and adding Pl to it.
[diff-counting] Significant lines: 421. |
if (phoneNumber !== undefined) { | ||
const fmtPhone = formatPhone(phoneNumber); | ||
} else { | ||
const fmtPhone = ''; |
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 dont think this else statement is needed since you set fmtPhone to '' already on line 36
also, nit: try to keep all the logic for formatPhone within the helper function, ie. do the check for if the phone number is undefined within formatPhone itself, so that if the helper is used in the future, you don't have to repeat this logic
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.
Thanks, I'll fix it here. I left it in when debugging some error regarding workingContext sending duplicated request.
} else { | ||
console.log('invalid file'); | ||
setErrorMessage(`Images must be under ${IMAGE_SIZE_LIMIT / 1000} KB`); |
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.
Hey Dwain, I checked out this branch and was having some issues uploading an image at first, so I checked the logs and got "Images must be under 50 KB," which seems to have come from here. After choosing a smaller image (20 KB) I was able to upload an image but the first one I tried was 116 KB, which is a size I think users should also be able to use to upload, so either here or in a future PR, maybe try either increasing the IMAGE_SIZE_LIMIT
or investigating why the if statement would be returning false here.
Otherwise, good work on figuring out the backend and getting the uploading images to work!!
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.
Hello, the img size limit was set prior, in server/app.ts its set to 5000kb and anything above it throws a payload size error. I can ask Desmond about changing that directly. Otherwise I'll look into other ways to get past it when i add the optimizations. Also thank you!
Implemented the ability to add employee photos using a newly created AWS-S3 bucket.
Revamped existing upload functions and added checks for existing API.
API Documentation for Image Upload Endpoint
POST
/
Description:
This endpoint allows users to upload images associated with Drivers or Admins and stores the images in an Amazon S3 bucket. It then updates the respective database record with the uploaded image's URL.
Request Headers:
Authorization
Request Body:
The request body should be sent in JSON format with the following fields:
id
tableName
Drivers
orAdmins
.fileBuffer
Response:
Success (200):
If the image is successfully uploaded and the database is updated:
Error Responses:
id
{ "err": "Invalid ID: empty or missing" }
fileBuffer
{ "err": "Invalid file buffer: empty or missing" }
tableName
{ "err": "Invalid table name: <tableName>" }
{ "err": "Error message from S3" }
{ "err": "Unexpected server error occurred" }
Validation Rules:
Drivers
orAdmins
.Implementation Details:
carriage-images
in theus-east-2
region.<tableName>/<id>
.public-read
).fileBuffer
is decoded from Base64 before being sent to S3.Usage Notes
photoLink
is saved in the database for the given Admin or Driver.User
role can access this endpoint. The uploaded image is only allowed forDrivers
andAdmins
.WIP/next iteration
Weird availability parse error (unrelated to this api I think)
Better cache (unsure if needed)