Conversation
Dao-Ho
left a comment
Dao-Ho
left a comment
There was a problem hiding this comment.
Just two very quick changes, should be good to be merged otherwise!
backend/cmd/server/main.go
Outdated
|
|
||
| "github.com/generate/selfserve/config" | ||
| _ "github.com/generate/selfserve/docs" | ||
| //_ "github.com/generate/selfserve/docs" |
There was a problem hiding this comment.
I believe you still need to uncomment this
| }, nil | ||
| } | ||
|
|
||
| func (s *Storage) GeneratePresignedURL(ctx context.Context, key string, expiration time.Duration) (string, error) { |
There was a problem hiding this comment.
It's not really clear what differentiates this from GeneratePresignedGetURL when reading the name. I had to drill down to this logic to find what it's really doing. I think we can probably rename this to be more explicit (also rename the parent function that calls this to ensure it's consistent)
| func (s *Storage) GeneratePresignedURL(ctx context.Context, key string, expiration time.Duration) (string, error) { | |
| func (s *Storage) GeneratePresignedUploadURL(ctx context.Context, key string, expiration time.Duration) (string, error) { |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #113 +/- ##
==========================================
- Coverage 20.37% 9.33% -11.04%
==========================================
Files 39 115 +76
Lines 1453 4037 +2584
Branches 0 24 +24
==========================================
+ Hits 296 377 +81
- Misses 1153 3654 +2501
- Partials 4 6 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
backend/internal/handler/s3.go
Outdated
| // Supported formats: jpg, jpeg, png, webp | ||
| ext := c.Query("ext", "jpg") | ||
| allowedExts := map[string]bool{"jpg": true, "jpeg": true, "png": true, "webp": true} | ||
| allowedExts := map[string]bool{"jpg": true, "jpeg": true, "png": true, "webp": true} |
There was a problem hiding this comment.
this should be a const somewhere, it's a config that doesn't feel right to be embedded into the logic, can we just make this a const?
backend/internal/handler/users.go
Outdated
There was a problem hiding this comment.
what was wrong with BindAndValidate? validateCreateUser was an old pattern before @zaydaanjahangir implemented our validation system. Was this intentional?
There was a problem hiding this comment.
I don't think we have clerk_id anymore, clerk id is just our id now, is it throwing for you? Make sure your code is up to date
There was a problem hiding this comment.
same concept as above, should it be clerk id here?
| if err != nil { | ||
| return "", err | ||
| return "", fmt.Errorf("failed to generate presigned get URL with key %s: %w", key, err) | ||
| } |
There was a problem hiding this comment.
nit so NOT necessary but since we're throwing a lot of custom errors for s3, feels like custom errors for them could be good here, not needed tho
Dao-Ho
left a comment
Dao-Ho
left a comment
There was a problem hiding this comment.
one removal and it's good to go
|
LGTM ✅ |
3 participants
Description
Adds backend endpoints for user profile picture management with S3 integration. Enables uploading, viewing, and deleting profile pictures.
Why:
How it works:
Type of Change
Related Issue(s)
Closes #
Related to #51
What Changed?
-Added user profile picture endpoints GET, PUT, and DELETE (/users/{userId}/profile-picture) endpoints for retrieving, updating, and deleting user profile pictures with S3 integration
-Added the S3 presigned URL for access to objects in the bucket (building upon what was in the s3-setup branch which was only a presign URL to put objects in the bucket)
-Implemented repository methods: UpdateProfilePicture, GetKey, and DeleteProfilePicture to manage profile picture S3 keys in the database
Testing & Validation
How this was tested
Screenshots/Recordings
This show the profile picture being stored and removed from the db based on interactions with the frontend
Unfinished Work & Known Issues
[] None, this PR is complete and production-ready
The following items are intentionally deferred:
Comments left on the first PR will apply to some of the files in this ex) s3storage.go still needs to be changed based on the comments left on the s3-setup PR
-There may be overlapping S3-related changes across 3 related PRs. I reviewed branch contents and believe everything is in the correct branch, but some S3 changes may appear in multiple PRs due to the branch mix-up.
Notes & Nuances
Pre-Merge Checklist
Code Quality
Testing & CI
Documentation
Reviewer Notes