-
Notifications
You must be signed in to change notification settings - Fork 2
Feat/issue55 profile photo #392
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
Conversation
…leaking of s3 secrets
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
… update s3 module to use aws-sdk v3 syntax, remove unused file s3.ts
|
I think just add some random string as values for your new env variables in |
Thank you for the reply. I added fake secrets in .env.test and the checks are passed. I'll provide the real secrets separately for you to test on your local device. |
|
One small thing, I think we should disable "save" button when we are saving. |
swh00tw
left a comment
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. But I think the naming (files, functions, API endpoints) could be better. I think "s3" should not be part of the names, it sounds strongly coupled with AWS S3. What if in the future we decide to switch to another image storage service? we will need to rename everything. I think calling it photo.service.ts, photo.controller.ts, ... could be a bit better (you can come up with better name if you have).
And API is not really RESTful as well. Maybe change
GET /s3urltoGET /photo/uploadLinksince we are getting the url of the upload linkDELETE /s3urltoDELETE /photo.
And I would recommend you document the API (request body, return, params) in recnet-api-model
| @@ -315,7 +402,7 @@ export function ProfileEditForm() { | |||
| type="submit" | |||
| disabled={!formState.isValid} | |||
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 think we should disable when isUploading is true as well
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.
Will make this change
joannechen1223
left a comment
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.
Left some comments. Please take a look, thank you!
Btw, as the flow of uploading photos is a bit complicated, it would be great if we could have a diagram to showcase how it works from end to end.
apps/recnet-api/.env.test
Outdated
|
|
||
| # AWS S3 | ||
| export AWS_BUCKET_NAME="profile-photo-storage-dev" | ||
| export AWS_ACCESS_KEY_ID="ABCD012FAKETESTSTRING" | ||
| export AWS_SECRET_ACCESS_KEY="ABCDEFGHKB1BS3YTHISISAfakeTESTSECRET/pk" | ||
| export AWS_BUCKET_REGION="us-east-2" No newline at end of file |
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.
nit
AWS_BUCKET_NAME=test_aws_bucket_name
AWS_ACCESS_KEY_ID=test_aws_access_key_id
...
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.
will make these changes.
I can make the diagram. Where do you want me to attach the diagram? In this PR or development handbook?
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.
For the diagram, I think it would be great if you could create a page in Notion under Design Docs and fill in more details in the document.
| SLACK_TOKEN_ENCRYPTION_KEY=test_token_encryption_key No newline at end of file | ||
| SLACK_TOKEN_ENCRYPTION_KEY=test_token_encryption_key | ||
|
|
||
| # AWS S3 |
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.
Add these env var to env.sample
| this.s3Region = this.s3Config.s3Region; | ||
| this.s3BucketName = this.s3Config.bucketName; | ||
| this.accessKeyId = this.s3Config.accessKeyId; | ||
| this.secretAccessKey = this.s3Config.secretAccessKey; | ||
| this.s3 = new S3Client({ | ||
| region: this.s3Region, | ||
| credentials: { | ||
| accessKeyId: this.accessKeyId, | ||
| secretAccessKey: this.secretAccessKey, | ||
| }, | ||
| }); |
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.
nit: We might not need to extract this from config again.
this.s3 = new s3Client({
region: this.s3Config.s3Region,
credentials: {
accessKeyId: this.s3Config.accessKeyId,
secretAccessKey: this.s3Config.secretAccessKey,
}
})| const timestamp = new Date() | ||
| .toLocaleString("en-US", { | ||
| year: "numeric", | ||
| month: "2-digit", | ||
| day: "2-digit", | ||
| hour: "2-digit", | ||
| minute: "2-digit", | ||
| second: "2-digit", | ||
| hour12: false, | ||
| }) | ||
| .replace(/[/,: ]/g, "-"); |
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.
Could you provide an example in the comment of this date format?
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. If the current date and time is February 21, 2025, 10:30:45 AM, the timestamp would be:
2025-02-21-10-30-45
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.
Will also add the example to the code.
| } catch (error: unknown) { | ||
| const errorMessage = | ||
| error instanceof Error ? error.message : "Unknown error"; | ||
| throw new Error(`Failed to delete S3 object: ${errorMessage}`); |
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.
Please define your error in recnet.error.const.ts and throw RecnetError to keep the error response consistent.
|
nit: Next time, kindly ensure that the screenshots do not contain Chinese text to maintain consistency and be inclusive to future maintainers. |
Added AWS S3 config to .env.sample Changed AWS config and secret names in .env.test Added example time stamp to s3.service.ts Defined RecnetError for S3 upload and delete errors and throw RecnetError in s3.service.ts
…edesign API endpoints to meet RESTful requirements Rename s3.service.ts to photo-storage.service.ts Rename s3.controller.ts to photo-storage.controller.ts Rename s3.module.ts to photo-storage.module.ts Updated app.module.ts to import the PhotoStorageModule correctly Modified getUploadUrl router to generateUploadUrl and use the POST method instead, because this request results in the creation of a new resource -the S3 URL which will be used later to upload a new photo. Hence POST is the appropriate method. Updated the corresponding method generateS3UploadUrl in the backend photo-storage.service.ts and photo-storage.controller.ts, update to POST method.
…oading large file
… document the request and response schema for photo-storage API endpoints
joannechen1223
left a comment
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
Please remember to add your env vars to AWS before deployment
Reverts #392 I accidentally merged this PR to master instead of dev.
## Description This PR adds a new edit profile photo feature. Prior to this, RecNet displays a user's Google account profile photo and does not allow changes. This feature allows any RecNet user to upload a profile photo, which is stored in an AWS S3 bucket and make updates to the user profile information in the database. ## Related Issue * See [PR#392](#392) for previous comments and requested changes. Reopening PR for the same issue to merge to `dev` before `master`. ## Frontend Changes * Modified `/apps/recnet/src/components/setting/profile/ProfileEditForm.tsx` adding new form element and function: * Added profile photo `<input>` element with a photo preview image to accept and display the selected photo. * Added `handleUploadS3` function to get a secure uploadUrl from the backend, put objects to the S3 bucket, and update the form data. * Modified the `onSubmit` function to call the `handleUploadS3` function when a user saves the form. * Updated the `ProfileEditSchema`, `useForm` hook to include the photoUrl field. * Added a `generateUploadUrlMutation` hook to send a request to the backend to get a secure S3 upload URL. * Added new state hooks `[isUploading, setIsUploading]`, `[selectedFile, setSelectedFile]`, `[photoPreviewUrl, setPhotoPreviewUrl]` , `[fileError, setFileError]` to manage the selected photo and upload status. * Created a new router `generateS3UploadUrl` in the `/apps/recnet/src/server/routers/user.ts` * Modified the router `updateUser` in `/apps/recnet/src/server/routers/user.ts` to fetch the original photoUrl and delete the corresponding S3 file if a new profile photo is uploaded. ## Backend Changes * Added a new folder `photo-storage` under `/apps/recnet-api/src/modules` * Created a `photo-storage.controler.ts` to handle requests to the `photo-storage` endpoint. * Created a `photo-storage.service.ts`. Included a `generateS3UploadUrl` method and a `deleteS3Object` method. * Created a `photo-storage.module.ts` to package and export the s3 class. * Modified `common.config.ts` under `/apps/recnet-api/src/config` to register AWS S3 configuration. * Updated `env.schema.ts` under `/apps/recnet-api/src/config` to include the s3 environment variable data types. * Updated `app.module.ts` under `/apps/recnet-api/src` to include and import the `PhotoStorageModule`. ## Other Changes * Added `photo-storage.ts` under `/libs/recnet-api-model/src/lib/api` to document the new APIs related to `photo-storage`. * Updated `package.json` to include `aws-sdk/client-s3` and `aws-sdk/s3-request-presigner` . * Install aws-sdk: `@aws-sdk/client-s3 @aws-sdk/s3-request-presigner`. Make sure to use aws-sdk v3. ## How To Test 1. Log in to your account. 2. Navigate to the Profile page. 3. Click on the "Settings" button and open the first tab "Edit profile". 4. Click on "Choose file" to choose a file and attach to the form. 5. Click on the "Save" button and wait for the upload to complete. 6. Check your profile photo and it should be updated to the new photo you uploaded. ## Screenshots (if appropriate): <img width="863" alt="1_profile-photo-before" src="https://github.com/user-attachments/assets/608bfba6-883f-4fbd-a50f-329434d920f3" /> <img width="783" alt="2_edit-photo" src="https://github.com/user-attachments/assets/38ed449d-d572-4259-b7f6-fe42a4a44664" /> <img width="799" alt="3_photo-attached" src="https://github.com/user-attachments/assets/94b94372-5dfa-4c7d-9dba-5586850799f0" /> <img width="739" alt="4_uploading" src="https://github.com/user-attachments/assets/8c3bc13c-e06b-40d9-b254-5648e78bfd8f" /> <img width="851" alt="5_photo-updated" src="https://github.com/user-attachments/assets/a8267cfa-dd82-45ba-a6d6-11908eed4dcd" /> ## TODO - Add AWS S3 configurations in the `.env` file under `/apps/recnet-api` for the dev and prod environment for deployment
## RecNet auto-release action This is an auto-generated PR by recnet-release-action 🤖 Please make sure to test your changes in staging before merging. ## Related Issues - #392 - #371 ## Related PRs - #394 - #391 ## Staging links recnet-web: [https://vercel.live/link/recnet-git-dev-recnet-542617e7.vercel.app](https://vercel.live/link/recnet-git-dev-recnet-542617e7.vercel.app) recnet-api: [https://dev-api.recnet.io/api](https://dev-api.recnet.io/api)
## RecNet auto-release action This is an auto-generated PR by recnet-release-action 🤖 Please make sure to test your changes in staging before merging. Recnet-api version bump for deployment. Related to PR#390. ## Related Issues - #379 - #55 - #371 ## Related PRs - #382 - #391 - #392 - #394 - #390 ## Staging links recnet-web: [https://vercel.live/link/recnet-git-dev-recnet-542617e7.vercel.app](https://vercel.live/link/recnet-git-dev-recnet-542617e7.vercel.app) recnet-api: [https://dev-api.recnet.io/api](https://dev-api.recnet.io/api)






Description
This PR adds a new edit profile photo feature. Prior to this, RecNet displays a user's Google account profile photo and does not allow changes. This feature allows any RecNet user to upload a profile photo, which is stored in an AWS S3 bucket and make updates to the user profile information in the database.
Frontend Changes
/apps/recnet/src/components/setting/profile/ProfileEditForm.tsxadding new form element and function:<input>element with a photo preview image to accept and display the selected photo.handleUploadS3function to get a secure uploadUrl from the backend, put objects to the S3 bucket, and update the form data.onSubmitfunction to call thehandleUploadS3function when a user saves the form.ProfileEditSchema,useFormhook to include the photoUrl field.getUploadUrlMutationhook to send a request to the backend to get a secure S3 upload URL.[isUploading, setIsUploading],[selectedFile, setSelectedFile],[photoPreviewUrl, setPhotoPreviewUrl]to manage the selected photo and upload status.getS3UploadUrlin the/apps/recnet/src/server/routers/user.tsupdateUserin/apps/recnet/src/server/routers/user.tsto fetch the original photoUrl and delete the corresponding S3 file if a new profile photo is uploaded.Backend Changes
s3under/apps/recnet-api/src/moduless3.controler.tsto handle requests to thes3urlendpoint.s3.service.ts. Included agetS3UploadUrlmethod and adeleteS3Objectmethod.s3.module.tsto package and export the s3 class.common.config.tsunder/apps/recnet-api/src/configto register AWS S3 configuration.env.schema.tsunder /apps/recnet-api/src/config` to include the s3 environment variable data types.Other Changes
app.module.tsunder/apps/recnet-api/srcto import and include theS3Module.package.jsonto includeaws-sdk.@aws-sdk/client-s3 @aws-sdk/s3-request-presigner. Make sure to use aws-sdk v3.How to Test
Screenshots (if appropriate):
TODO
.envfile under/apps/recnet-apifor the dev and prod environment for deployment