Feat/update update profile dto#84
Conversation
- Remove problematic logic in EndpointExtensions that returned 404 for successful responses with null data. - Update UpdateProfileHandler to return success message in the Data field.
📝 WalkthroughWalkthroughThe PR updates the user profile update endpoint to return a complete ChangesProfile Update Response Type
Sequence DiagramsequenceDiagram
participant Client
participant UpdateProfile as UpdateProfile Endpoint
participant UpdateProfileHandler
participant UserManager
participant FileStorageService
participant Response as ToActionResult
Client->>UpdateProfile: PUT /api/auth/profile
UpdateProfile->>UpdateProfileHandler: Handle(UpdateProfileCommand)
UpdateProfileHandler->>UserManager: FindByIdAsync(userId)
alt User not found
UpdateProfileHandler-->>UpdateProfile: ServiceResponse<UserDto> failure
else User found
UpdateProfileHandler->>UserManager: UpdateAsync(user)
alt Update failed
UpdateProfileHandler-->>UpdateProfile: ServiceResponse<UserDto> failure
else Update succeeded
UpdateProfileHandler->>UserManager: GetRolesAsync(user)
UpdateProfileHandler->>FileStorageService: GetFileUrlAsync(profilePictureUrl)
UpdateProfileHandler->>UpdateProfileHandler: Build UserDto with roles and picture URL
UpdateProfileHandler-->>UpdateProfile: ServiceResponse<UserDto> success with UserDto
end
end
UpdateProfile->>Response: ToActionResult<UserDto>(result)
Response-->>Client: 200 OK with UserDto
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| catch (Exception ex) | ||
| { | ||
| logger.LogWarning(ex, "Failed to resolve profile picture URL for user {UserId}. Key: {Key}", user.Id, | ||
| user.ProfilePictureUrl); | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/Src/PIED_LMS.Presentation/Extensions/EndpointExtensions.cs (1)
9-12:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMultiple handlers return successful responses with null data — verify clients handle 200 OK instead of 404.
Successful responses now unconditionally return
200 OKregardless of whetherresult.Datais null. This affects handlers that returnServiceResponse<string>(true, message)without providing data, since theDataparameter defaults to null. Found in:
- UpdateCourseHandler, DeleteCourseHandler
- UpdateQuestionQuizHandler, DeleteQuestionQuizHandler
- AssignMentorsHandler
Previously, these null data responses likely returned
404 Not Foundwhen clients expected them. Verify that clients consuming these endpoints do not depend on 404 responses. Update handlers to returnSuccess = falsewith appropriate error code if a 404 response is semantically required, rather than relying on null data detection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/Src/PIED_LMS.Presentation/Extensions/EndpointExtensions.cs` around lines 9 - 12, Handlers such as UpdateCourseHandler, DeleteCourseHandler, UpdateQuestionQuizHandler, DeleteQuestionQuizHandler, and AssignMentorsHandler are returning Results.Ok(result) even when result.Data is null, causing 200 OK where a 404 was expected; update those handlers to detect missing resources and either set ServiceResponse.Success = false with an appropriate error code/message or return Results.NotFound(...) (instead of Results.Ok) when result.Data == null so clients receive a 404, and ensure the code paths that construct ServiceResponse (e.g., ServiceResponse<string>(true, message)) are adjusted to reflect failure when no resource exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@backend/Src/PIED_LMS.Application/UserCases/Commands/Identity/UpdateProfileHandler.cs`:
- Around line 67-79: The response currently resolves the profile URL later and
can return null even after a successful upload; modify UpdateProfileHandler to
call fileStorageService.GetFileUrlAsync immediately after
fileStorageService.SaveFileAsync (use SaveFileAsync and GetFileUrlAsync) when a
new picture is uploaded, capture the resolved URL and use that resolved value
when constructing the UserDto (while still persisting the storage key in
user.ProfilePictureUrl), and if GetFileUrlAsync fails after SaveFileAsync log
the error and include the storage key as a fallback in the response so clients
are not misled by a null profilePicUrl.
- Line 88: The code passes roles.ToList() to the update call but GetRolesAsync
already returns an IList<string>, so remove the redundant allocation and pass
the existing roles collection directly; locate the roles variable (result of
UserManager.GetRolesAsync) in UpdateProfileHandler (Handle method) and replace
the roles.ToList() usage with roles.
---
Outside diff comments:
In `@backend/Src/PIED_LMS.Presentation/Extensions/EndpointExtensions.cs`:
- Around line 9-12: Handlers such as UpdateCourseHandler, DeleteCourseHandler,
UpdateQuestionQuizHandler, DeleteQuestionQuizHandler, and AssignMentorsHandler
are returning Results.Ok(result) even when result.Data is null, causing 200 OK
where a 404 was expected; update those handlers to detect missing resources and
either set ServiceResponse.Success = false with an appropriate error
code/message or return Results.NotFound(...) (instead of Results.Ok) when
result.Data == null so clients receive a 404, and ensure the code paths that
construct ServiceResponse (e.g., ServiceResponse<string>(true, message)) are
adjusted to reflect failure when no resource exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 09d00aa2-bf10-484e-982a-0f124a6f414b
📒 Files selected for processing (4)
backend/Src/PIED_LMS.Application/UserCases/Commands/Identity/UpdateProfileHandler.csbackend/Src/PIED_LMS.Contract/Services/Identity/Command.csbackend/Src/PIED_LMS.Presentation/APIs/AuthenticationEndpoints.csbackend/Src/PIED_LMS.Presentation/Extensions/EndpointExtensions.cs
| string? profilePicUrl = null; | ||
| if (!string.IsNullOrWhiteSpace(user.ProfilePictureUrl)) | ||
| { | ||
| try | ||
| { | ||
| profilePicUrl = await fileStorageService.GetFileUrlAsync(user.ProfilePictureUrl); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| logger.LogWarning(ex, "Failed to resolve profile picture URL for user {UserId}. Key: {Key}", user.Id, | ||
| user.ProfilePictureUrl); | ||
| } | ||
| } |
There was a problem hiding this comment.
Profile picture URL resolution failure returns incomplete data after successful upload.
If a new profile picture is uploaded successfully (lines 45-52) and stored in the database (line 57), but GetFileUrlAsync fails when building the response (line 72), the returned UserDto will have a null profilePicUrl even though the upload succeeded. This misleads the client into believing the upload failed when it actually succeeded.
Consider one of these approaches:
- Call
GetFileUrlAsyncimmediately afterSaveFileAsync(line 50) to validate the URL is retrievable before saving to the database. - Return the storage key instead of the resolved URL, letting clients resolve URLs on subsequent requests.
- Document that a null
profilePicUrlin the response doesn't necessarily mean upload failed, but this is poor UX.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend/Src/PIED_LMS.Application/UserCases/Commands/Identity/UpdateProfileHandler.cs`
around lines 67 - 79, The response currently resolves the profile URL later and
can return null even after a successful upload; modify UpdateProfileHandler to
call fileStorageService.GetFileUrlAsync immediately after
fileStorageService.SaveFileAsync (use SaveFileAsync and GetFileUrlAsync) when a
new picture is uploaded, capture the resolved URL and use that resolved value
when constructing the UserDto (while still persisting the storage key in
user.ProfilePictureUrl), and if GetFileUrlAsync fails after SaveFileAsync log
the error and include the storage key as a fallback in the response so clients
are not misled by a null profilePicUrl.
| user.LastName, | ||
| user.IsActive, | ||
| user.CreatedAt, | ||
| roles.ToList(), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Unnecessary list conversion.
GetRolesAsync already returns IList<string>. The ToList() call creates a redundant copy.
♻️ Proposed refactor
- roles.ToList(),
+ roles,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| roles.ToList(), | |
| roles, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend/Src/PIED_LMS.Application/UserCases/Commands/Identity/UpdateProfileHandler.cs`
at line 88, The code passes roles.ToList() to the update call but GetRolesAsync
already returns an IList<string>, so remove the redundant allocation and pass
the existing roles collection directly; locate the roles variable (result of
UserManager.GetRolesAsync) in UpdateProfileHandler (Handle method) and replace
the roles.ToList() usage with roles.
3 participants
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements