Feat/fight rabbit merge#82
Merged
Merged
Conversation
- Updated service response handling in CompilerEndpoints, CourseEndpoints, EnrollmentEndpoints, ExamEndpoints, ExamParticipationEndpoints, ExamRoomEndpoints, HealthApi, LogBridgeEndpoints, MentorEndpoints, QuizletEndpoints, StudentSubmissionEndpoints, StudentTestCaseEndpoints, TeacherEndpoints, TestCaseEndpoints, and TestRoomEndpoints to enhance clarity and maintainability. - Removed unnecessary using directives and organized namespaces across various files. - Simplified response status code handling in EndpointExtensions and OpenApiConventionExtensions. - Introduced global usings for common namespaces in Presentation, Domain, and Persistence layers to reduce boilerplate code.
…pied-lms into feat/fix-course-authen
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR transitions the LMS from a teacher-centric to mentor-centric role model, modernizes course identifiers from ChangesRole System and Entity Refactoring: Teacher → Mentor
Course Identifier Modernization: int → Guid
Background Email Processing Infrastructure
User DTO Consolidation and Query Optimization
Question and Quiz Feature Additions
API Endpoint Refactoring and Result Mapping
Sequence Diagram(s)sequenceDiagram
participant User as User/Admin
participant Endpoint as Course Mentor<br/>Endpoint
participant Handler as AssignMentors<br/>Handler
participant Repo as IUnitOfWork<br/>Repository
participant Queue as Email<br/>Queue
participant Service as Email<br/>Background Service
User->>Endpoint: POST /courses/{id}/mentors<br/>{mentors: [id1, id2]}
Endpoint->>Endpoint: Validate non-null,<br/>no duplicates
Endpoint->>Handler: AssignMentorsCommand
Handler->>Repo: Load course by ID
Handler->>Repo: Fetch mentor users by IDs
Handler->>Handler: Validate all IDs exist
Handler->>Handler: Validate all have Mentor role
Handler->>Repo: Clear/repopulate course.Mentors
Handler->>Repo: CommitAsync()
Handler->>Queue: EnqueueEmailAsync (per mentor)
Handler->>Endpoint: ServiceResponse<string> success
Endpoint->>User: 200 OK
Queue->>Service: Dequeue EmailJob
Service->>Service: Retry loop (up to MaxRetries)
Service->>Service: Create DI scope
Service->>Service: Resolve IEmailService
Service->>Service: SendCourseAssignmentAsync
Service->>Queue: Success / Retry with delay
sequenceDiagram
participant User as User
participant Endpoint as Question<br/>Endpoint
participant Handler as CheckAnswer<br/>Handler
participant Repo as IUnitOfWork<br/>Repository
participant Logic as Answer<br/>Evaluation
User->>Endpoint: POST /api/questions/check<br/>{questionId, selectedOptions}
Endpoint->>Handler: CheckQuestionAnswerCommand
Handler->>Repo: Load question with answers
Handler->>Handler: Return NOT_FOUND if missing
Handler->>Logic: Compute set equality:<br/>selectedOptions vs correct answers
Logic->>Logic: IsCorrect = exact match?
Handler->>Handler: Build CheckAnswerResponse<br/>(isCorrect, correctAnswers, explanation)
Handler->>Endpoint: ServiceResponse<CheckAnswerResponse>
Endpoint->>User: 200 OK (with evaluation result)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Critical Issues & Observations1. Incomplete Role Constant Replacement (SERIOUS)The
2. Null Reference Risk in Email Enqueue (MEDIUM)The if (!string.IsNullOrEmpty(mentor.Email))
await backgroundEmailQueue.EnqueueEmailAsync(new EmailJob { Email = mentor.Email, ... });The 3. Duplicate Mentor Validation Logic (LOW)The 4. Profile Picture Resolution Inconsistency (MEDIUM)The
Recommendation: Standardize profile picture error handling across all user query handlers with consistent logging and fallback behavior. 5. Course Authorization Tightened Without Migration Path (HIGH)The 6. N+1 Query Optimization Incomplete (MEDIUM)While 7. Database Seeding Role Assignment Logic (MEDIUM)The if (!await userManager.IsInRoleAsync(mentor, mentorRole.Name))
await userManager.AddToRoleAsync(mentor, mentorRole.Name);
else
_logger.LogInformation($"User {mentor.Id} is already a Mentor.");This prevents exceptions from being re-thrown if role assignment fails for other reasons (e.g., database errors). Verify that all exceptions on role assignment should be caught and logged rather than propagated. 8. Question/Quiz Type Safety (LOW)The Recommendation: Address issues #1 (role constant uniformity), #2 (email validation), #5 (authorization breaking change), and #7 (exception handling on role assignment) before merging. Issues #3, #4, and #6 are architectural refinements suitable for follow-up PRs. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
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.Application/UserCases/Queries/Course/GetCourseByIdHandler.cs (1)
100-108:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve mentor profile picture URLs before returning DTOs.
Line 107 passes
t.ProfilePictureUrlthrough directly. This can leak storage keys/relative paths instead of consumable URLs and will produce inconsistent API output.Suggested fix
- var mentorDtos = course.Mentors.Select(t => new CourseMentorDto( - t.Id, - t.FirstName ?? string.Empty, - t.LastName ?? string.Empty, - t.Email ?? string.Empty, - t.Bio, - t.ProfilePictureUrl - )).ToList(); + var mentorDtos = new List<CourseMentorDto>(); + foreach (var t in course.Mentors) + { + var profilePictureUrl = string.IsNullOrWhiteSpace(t.ProfilePictureUrl) + ? null + : await fileStorageService.GetFileUrlAsync(t.ProfilePictureUrl); + + mentorDtos.Add(new CourseMentorDto( + t.Id, + t.FirstName ?? string.Empty, + t.LastName ?? string.Empty, + t.Email ?? string.Empty, + t.Bio, + profilePictureUrl + )); + }Also applies to: 120-120
🤖 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/Queries/Course/GetCourseByIdHandler.cs` around lines 100 - 108, The mentor mapping is returning raw storage keys via t.ProfilePictureUrl; update the mapping in GetCourseByIdHandler (the course.Mentors.Select(...) that constructs CourseMentorDto) to resolve each mentor's ProfilePictureUrl into a public/consumable URL before passing it to CourseMentorDto (e.g., call your existing storage/url helper or service method such as ResolveUrl/GetPublicUrl on t.ProfilePictureUrl). Apply the same change to the other mentor mapping site referenced (the second mapping around line 120) so all CourseMentorDto instances receive resolved URLs rather than raw keys/relative paths.
🤖 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/Course/AssignMentorsHandler.cs`:
- Around line 73-93: Assigning mentors currently commits DB changes before
calling backgroundEmailQueue.EnqueueEmailAsync in AssignMentorsHandler, which
can leave mentors assigned without notifications if enqueueing fails; fix by
either (A) moving the enqueue calls into the same transaction scope as the DB
updates (invoke EnqueueEmailAsync for each mentor before SaveChangesAsync /
transaction.Commit so enqueueing occurs inside the transaction), or (B)
implement compensating tracking: create and persist a PendingNotification entity
when assigning mentors (e.g., AddPendingNotification for each mentor) before
commit, then after commit attempt EnqueueEmailAsync and on success remove the
corresponding PendingNotification (or keep a retry worker to process
PendingNotification records until successful); reference
backgroundEmailQueue.EnqueueEmailAsync, AssignMentorsHandler, and the DB
save/transaction methods to locate where to apply the change.
- Around line 116-120: The check for a missing Mentor role in
AssignMentorsHandler (mentorRole == null) currently logs and returns 'users',
which misclassifies a system configuration error as a validation failure; change
this to fail fast: when mentorRole is null, log the error with context (e.g.,
"Mentor role not found in the system") and then either throw an appropriate
exception (e.g., InvalidOperationException or a custom
SystemConfigurationException) from Handle, or return an error result/response
directly from Handle indicating system misconfiguration; update callers/tests to
expect this failure instead of treating it as a user validation issue.
- Around line 63-68: The Clear()+foreach pattern on course.Mentors causes many
change notifications; in AssignMentorsHandler replace that block with a single
assignment to the collection (e.g., course.Mentors = mentors.ToList() or
course.Mentors = new List<Mentor>(mentors)) so the navigation property is
swapped in one operation; ensure the target property type matches
(ICollection<T>/List<T>) and that your EF entity configuration supports direct
collection replacement before committing this change.
In
`@backend/Src/PIED_LMS.Application/UserCases/Commands/Course/CreateCourseHandler.cs`:
- Around line 49-58: The two identical catch blocks in CreateCourseHandler
(catching ArgumentException and InvalidOperationException) should be
consolidated into a single handler to avoid duplication; replace them with one
conditional catch that logs via logger.LogError(ex, "...") and returns the same
new ServiceResponse<Guid>(false, "Failed to upload thumbnail image. Please try
again.") — for example use a single catch (Exception ex) when (ex is
ArgumentException || ex is InvalidOperationException) { ... } so the identical
behavior around logging and returning is centralized.
In
`@backend/Src/PIED_LMS.Application/UserCases/Commands/Identity/UpdateProfileHandler.cs`:
- Around line 58-64: The handler currently logs result.Errors but returns only a
generic message; update UpdateProfileHandler to include the specific error
descriptions from result.Errors (the errors variable built from
result.Errors.Select(...)) in the ServiceResponse<string> returned on failure so
callers get actionable feedback (either by passing the errors collection into
ServiceResponse<string> if it has an errors/details field or by appending
string.Join(", ", errors) to the message); keep the existing
logger.LogWarning(user.Id, errors) behavior but ensure the returned
ServiceResponse contains the actual error descriptions instead of just "Failed
to update profile".
In
`@backend/Src/PIED_LMS.Application/UserCases/Commands/Question/CheckQuestionAnswerHandler.cs`:
- Around line 15-17: The query in CheckQuestionAnswerHandler retrieves a
question by ID without enforcing visibility/publication, which can expose hidden
or unpublished content; update the
unitOfWork.Repository<Domain.Entities.Question>().FindAll(...) predicate to
include the same visibility/publication gates used by your random-question
retrieval (add the published/visible conditions—e.g., the same q.IsVisible &&
q.IsPublished or q.Visible && q.Published predicate used there) so the handler
only loads questions that are public and visible when evaluating answers for
request.QuestionId.
- Around line 30-32: The code in CheckQuestionAnswerHandler computes isCorrect
by dereferencing request.SelectedOptions without a null guard; add a null check
for request.SelectedOptions before the isCorrect calculation (in the same
handler/method that declares isCorrect) and handle the null case by returning a
controlled client error (e.g., BadRequest/validation result) or treating it as
an empty selection per business rules; ensure references to
request.SelectedOptions and the isCorrect calculation are updated so the
comparison only runs when SelectedOptions is non-null.
In `@backend/Src/PIED_LMS.Application/UserCases/Queries/GetMeHandler.cs`:
- Around line 19-20: The failure branch in GetMeHandler returns new
ServiceResponse<UserDto>(false, "User not found") but doesn't mark the response
as NotFound; update this branch to set the not-found indicator on the
ServiceResponse (e.g., use the constructor overload or property to set
NotFound/IsNotFound = true) so API translators can return HTTP 404; modify the
return in GetMeHandler accordingly using ServiceResponse<UserDto>'s not-found
parameter or property.
In
`@backend/Src/PIED_LMS.Application/UserCases/Queries/Mentor/GetMentorsHandler.cs`:
- Around line 25-30: The handler GetMentorsHandler currently treats a missing
mentor role (result of _roleManager.FindByNameAsync(RoleConstants.Mentor)) as a
successful empty result; change this to return a failure ServiceResponse
(success = false) with an appropriate error message and no page payload (or
null) so configuration errors are surfaced; update the block handling mentorRole
== null to construct and return ServiceResponse<PagedResult<MentorDto>> with
success false and a descriptive error, and consider logging the missing role
before returning.
In
`@backend/Src/PIED_LMS.Application/UserCases/Queries/Submission/GetSubmissionByIdQueryHandler.cs`:
- Around line 41-46: The handler currently only rejects Students/Mentors when
they don’t own the submission but implicitly allows all other roles; change
GetSubmissionByIdQueryHandler to implement deny-by-default by replacing the two
separate checks with an explicit allow-list: if roleClaim ==
RoleConstants.Student then require submission.StudentId == currentUserId, if
roleClaim == RoleConstants.Mentor then require submission.Exam.CreatedBy ==
currentUserId, otherwise deny; if none of the allow conditions match return a
ServiceResponse<SubmissionDetailResponse>(false, "Forbidden", ...) so any
missing or unexpected roleClaim cannot read submissions.
In `@backend/Src/PIED_LMS.Contract/Services/Enrollment/Command.cs`:
- Line 11: The parameter declaration uses a fully qualified type
Microsoft.AspNetCore.Http.IFormFile for PaymentProof even though
Microsoft.AspNetCore.Http is already imported; change the parameter type to the
unqualified IFormFile (i.e., update the PaymentProof parameter in the Command
class/constructor signature) to remove the redundant namespace qualification
while leaving the using directive intact.
In `@backend/Src/PIED_LMS.Contract/Services/Enrollment/Request.cs`:
- Around line 6-17: The pagination params accept invalid (zero/negative) values;
add validation attributes to enforce minimum bounds by decorating PageIndex and
PageSize in both GetAvailableCoursesRequest and GetEnrollmentsRequest with a
Range attribute (e.g., [Range(1, int.MaxValue)]) and import
System.ComponentModel.DataAnnotations so the contract rejects values below 1;
ensure you apply the attribute to both occurrences of PageIndex and PageSize in
the two record definitions.
In `@backend/Src/PIED_LMS.Infrastructure/BackgroundTasks/BackgroundEmailQueue.cs`:
- Around line 10-17: The constructor in BackgroundEmailQueue currently hardcodes
capacity=100; make the queue capacity configurable by reading
BackgroundTaskSettings.EmailQueueCapacity (or an injected options/settings
instance) and using that value as the BoundedChannelOptions capacity with a
sensible fallback (e.g., existing default) if the config is missing or invalid;
update the BackgroundEmailQueue constructor signature to accept the
settings/options or IConfiguration, validate/parse
BackgroundTaskSettings.EmailQueueCapacity, and use it when creating
Channel.CreateBounded<EmailJob>(options) so capacity can be tuned via
configuration rather than a hardcoded literal.
In
`@backend/Src/PIED_LMS.Infrastructure/BackgroundTasks/EmailBackgroundService.cs`:
- Around line 52-63: The code unconditionally sets emailSent = true after
calling SendCourseAssignmentAsync and ignores its Task<bool> result; change it
to capture the bool returned by SendCourseAssignmentAsync (assign to emailSent
like in EnrollStudentsHandler), and if the result is false throw an
InvalidOperationException to trigger retry logic; also remove the misleading
comment that rationalizes treating non-throwing as success. Reference:
SendCourseAssignmentAsync, emailSent, EmailBackgroundService and the job.*
parameters used in the call.
In `@backend/Src/PIED_LMS.Presentation/APIs/AuthenticationEndpoints.cs`:
- Line 76: The AuthorizeAttribute usage in the RequireAuthorization call uses
unnecessary string interpolation; replace
$"{RoleConstants.Administrator},{RoleConstants.Mentor}" with a plain string
concatenation or, better, a reusable constant for the combined roles (e.g.,
create a Roles.AdminAndMentor constant) and use new AuthorizeAttribute { Roles =
"<combined-roles>" } in the RequireAuthorization call to remove the
interpolation while keeping the same values.
In `@backend/Src/PIED_LMS.Presentation/APIs/EnrollmentEndpoints.cs`:
- Around line 79-83: Refactor the repeated inline pagination default logic into
a reusable extension by adding a static class (e.g., PaginationExtensions) with
methods GetPageIndexOrDefault(this int) and GetPageSizeOrDefault(this int) that
return pageIndex > 0 ? pageIndex : 1 and pageSize > 0 ? pageSize : 10
respectively, then update EnrollmentEndpoints where queries are created (e.g.,
the call constructing
PIED_LMS.Contract.Services.Course.GetStudentAvailableCoursesQuery and the other
occurrences around lines 93-96) to call
request.PageIndex.GetPageIndexOrDefault() and
request.PageSize.GetPageSizeOrDefault() instead of the duplicated ternary
expressions.
In `@backend/Src/PIED_LMS.Presentation/APIs/QuestionEndpoints.cs`:
- Around line 41-49: The CheckAnswer endpoint does not accept or forward a
CancellationToken, so add a CancellationToken parameter to the method signature
(e.g., CancellationToken cancellationToken), accept it from the framework, and
propagate it when sending the MediatR command and converting the result: pass
cancellationToken into sender.Send(...) for the CheckQuestionAnswerCommand and
into result.ToActionResult(context, cancellationToken) (or the equivalent
overload) so the request can be cancelled if the client disconnects.
- Around line 35-39: GetRandomQuestion currently doesn't accept or propagate a
CancellationToken, so update the GetRandomQuestion method signature to accept a
CancellationToken (e.g., CancellationToken cancellationToken) and pass that
token into the Send call (sender.Send(new GetRandomQuestionQuery(),
cancellationToken)); also propagate the token into the result conversion if
available (e.g., result.ToActionResult(context, cancellationToken)) so the
request can be cancelled when the client disconnects. Ensure you update any
callers or route bindings that invoke GetRandomQuestion to supply the
cancellation token from the HttpContext or framework binding.
In `@backend/Src/PIED_LMS.Presentation/APIs/StudentSubmissionEndpoints.cs`:
- Around line 30-34: The MapGet endpoint for "/submissions/{id:guid}" currently
calls RequireAuthorization() which allows any authenticated user; restrict it at
the API boundary by specifying the allowed roles on the endpoint (e.g., require
Student, Mentor and Admin roles) instead of the generic call — update the
authorization on the MapGet("/submissions/{id:guid}", GetSubmissionById) chain
(the RequireAuthorization call) to require explicit roles or a dedicated policy
(keeping the existing GetSubmissionById handler checks as defense-in-depth).
In `@backend/Src/PIED_LMS.Presentation/Extensions/EndpointExtensions.cs`:
- Around line 9-15: The current branch inside the success path checks
result.Success and then returns Results.NotFound when result.Data is null (and
typeof(T).IsClass), which incorrectly maps a successful operation to HTTP 404;
update the success handling in the method that inspects result.Success so that
when result.Success == true you always return Results.Ok(result) (remove the
Results.NotFound branch), and ensure callers/handlers set result.Success = false
and appropriate error info when data is truly missing so that NotFound (or other
error responses) are produced only from non-success results.
- Around line 19-21: The NotFound detection currently relies on message text
(result.Message.Contains("not found")), which is fragile; update the check in
the EndpointExtensions code to remove the Message-based check and rely on
result.IsNotFound and a canonical ErrorCode check instead (e.g., inspect
result.ErrorCode for a NOT_FOUND marker using a null-safe comparison on
result.ErrorCode, such as result.ErrorCode is not null &&
result.ErrorCode.Contains("NOT_FOUND", StringComparison.OrdinalIgnoreCase)), so
only IsNotFound or ErrorCode determine 404 handling and any Message inspection
is eliminated.
---
Outside diff comments:
In
`@backend/Src/PIED_LMS.Application/UserCases/Queries/Course/GetCourseByIdHandler.cs`:
- Around line 100-108: The mentor mapping is returning raw storage keys via
t.ProfilePictureUrl; update the mapping in GetCourseByIdHandler (the
course.Mentors.Select(...) that constructs CourseMentorDto) to resolve each
mentor's ProfilePictureUrl into a public/consumable URL before passing it to
CourseMentorDto (e.g., call your existing storage/url helper or service method
such as ResolveUrl/GetPublicUrl on t.ProfilePictureUrl). Apply the same change
to the other mentor mapping site referenced (the second mapping around line 120)
so all CourseMentorDto instances receive resolved URLs rather than raw
keys/relative paths.
🪄 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: a42ccd50-dbce-4063-a4c5-49a0f65062dd
⛔ Files ignored due to path filters (7)
backend/Src/PIED_LMS.Persistence/Migrations/20260510085819_RenameTeacherToMentor.Designer.csis excluded by!**/*.Designer.cs,!**/Migrations/**and included bybackend/Src/**backend/Src/PIED_LMS.Persistence/Migrations/20260510085819_RenameTeacherToMentor.csis excluded by!**/Migrations/**and included bybackend/Src/**backend/Src/PIED_LMS.Persistence/Migrations/20260510091502_ChangeCourseFromIntegerToGuid.Designer.csis excluded by!**/*.Designer.cs,!**/Migrations/**and included bybackend/Src/**backend/Src/PIED_LMS.Persistence/Migrations/20260510091502_ChangeCourseFromIntegerToGuid.csis excluded by!**/Migrations/**and included bybackend/Src/**backend/Src/PIED_LMS.Persistence/Migrations/20260510102335_ChangeCourseIntegerToGUID.Designer.csis excluded by!**/*.Designer.cs,!**/Migrations/**and included bybackend/Src/**backend/Src/PIED_LMS.Persistence/Migrations/20260510102335_ChangeCourseIntegerToGUID.csis excluded by!**/Migrations/**and included bybackend/Src/**backend/Src/PIED_LMS.Persistence/Migrations/PiedLmsDbContextModelSnapshot.csis excluded by!**/Migrations/**and included bybackend/Src/**
📒 Files selected for processing (69)
backend/Src/PIED_LMS.Application/Options/CourseManagementSettings.csbackend/Src/PIED_LMS.Application/UserCases/Commands/Compiler/JudgeFromFileCommandHandler.csbackend/Src/PIED_LMS.Application/UserCases/Commands/Course/AssignMentorsHandler.csbackend/Src/PIED_LMS.Application/UserCases/Commands/Course/AssignTeachersHandler.csbackend/Src/PIED_LMS.Application/UserCases/Commands/Course/CreateCourseHandler.csbackend/Src/PIED_LMS.Application/UserCases/Commands/Identity/UpdateProfileHandler.csbackend/Src/PIED_LMS.Application/UserCases/Commands/Question/CheckQuestionAnswerHandler.csbackend/Src/PIED_LMS.Application/UserCases/Commands/Quiz/UpdateQuestionQuizHandler.csbackend/Src/PIED_LMS.Application/UserCases/Commands/Room/CreateRoomHandler.csbackend/Src/PIED_LMS.Application/UserCases/Queries/Course/GetCourseByIdHandler.csbackend/Src/PIED_LMS.Application/UserCases/Queries/Course/GetCoursesHandler.csbackend/Src/PIED_LMS.Application/UserCases/Queries/Course/GetStudentAvailableCoursesHandler.csbackend/Src/PIED_LMS.Application/UserCases/Queries/GetAllStudentsHandler.csbackend/Src/PIED_LMS.Application/UserCases/Queries/GetAllUsersHandler.csbackend/Src/PIED_LMS.Application/UserCases/Queries/GetMeHandler.csbackend/Src/PIED_LMS.Application/UserCases/Queries/GetUserByIdHandler.csbackend/Src/PIED_LMS.Application/UserCases/Queries/Mentor/GetAllMentorsSimpleHandler.csbackend/Src/PIED_LMS.Application/UserCases/Queries/Mentor/GetMentorByIdHandler.csbackend/Src/PIED_LMS.Application/UserCases/Queries/Mentor/GetMentorsHandler.csbackend/Src/PIED_LMS.Application/UserCases/Queries/Question/GetRandomQuestionHandler.csbackend/Src/PIED_LMS.Application/UserCases/Queries/Submission/GetSubmissionByIdQueryHandler.csbackend/Src/PIED_LMS.Application/UserCases/Queries/Teacher/GetAllTeachersSimpleHandler.csbackend/Src/PIED_LMS.Application/UserCases/Queries/Teacher/GetTeacherByIdHandler.csbackend/Src/PIED_LMS.Application/UserCases/Queries/Teacher/GetTeachersHandler.csbackend/Src/PIED_LMS.Contract/Abstractions/BackgroundTasks/IBackgroundEmailQueue.csbackend/Src/PIED_LMS.Contract/Services/Course/Command.csbackend/Src/PIED_LMS.Contract/Services/Course/Query.csbackend/Src/PIED_LMS.Contract/Services/Course/Request.csbackend/Src/PIED_LMS.Contract/Services/Course/Response.csbackend/Src/PIED_LMS.Contract/Services/Enrollment/Command.csbackend/Src/PIED_LMS.Contract/Services/Enrollment/Request.csbackend/Src/PIED_LMS.Contract/Services/Enrollment/Response.csbackend/Src/PIED_LMS.Contract/Services/ExamParticipation/Query.csbackend/Src/PIED_LMS.Contract/Services/ExamParticipation/Response.csbackend/Src/PIED_LMS.Contract/Services/Identity/Query.csbackend/Src/PIED_LMS.Contract/Services/Identity/Response.csbackend/Src/PIED_LMS.Contract/Services/Identity/Validators/UpdateProfileValidator.csbackend/Src/PIED_LMS.Contract/Services/Mentor/Query.csbackend/Src/PIED_LMS.Contract/Services/Mentor/Response.csbackend/Src/PIED_LMS.Contract/Services/Question/RandomQuestionDto.csbackend/Src/PIED_LMS.Contract/Services/Question/RandomQuestionQuery.csbackend/Src/PIED_LMS.Contract/Services/QuestionQuiz/Request.csbackend/Src/PIED_LMS.Contract/Services/Teacher/Query.csbackend/Src/PIED_LMS.Contract/Services/Teacher/Response.csbackend/Src/PIED_LMS.Domain/Abstractions/ICourseLockingService.csbackend/Src/PIED_LMS.Domain/Constants/RoleConstants.csbackend/Src/PIED_LMS.Domain/Entities/Course.csbackend/Src/PIED_LMS.Domain/Entities/Enrollment.csbackend/Src/PIED_LMS.Infrastructure/BackgroundTasks/BackgroundEmailQueue.csbackend/Src/PIED_LMS.Infrastructure/BackgroundTasks/EmailBackgroundService.csbackend/Src/PIED_LMS.Infrastructure/DbInitializer.csbackend/Src/PIED_LMS.Infrastructure/DependencyInjection.csbackend/Src/PIED_LMS.Infrastructure/Email/SmtpEmailService.csbackend/Src/PIED_LMS.Persistence/Configurations/CourseConfiguration.csbackend/Src/PIED_LMS.Persistence/Services/CourseLockingService.csbackend/Src/PIED_LMS.Presentation/APIs/AuthenticationEndpoints.csbackend/Src/PIED_LMS.Presentation/APIs/CourseEndpoints.csbackend/Src/PIED_LMS.Presentation/APIs/EnrollmentEndpoints.csbackend/Src/PIED_LMS.Presentation/APIs/ExamEndpoints.csbackend/Src/PIED_LMS.Presentation/APIs/ExamParticipationEndpoints.csbackend/Src/PIED_LMS.Presentation/APIs/ExamRoomEndpoints.csbackend/Src/PIED_LMS.Presentation/APIs/MentorEndpoints.csbackend/Src/PIED_LMS.Presentation/APIs/QuestionEndpoints.csbackend/Src/PIED_LMS.Presentation/APIs/QuizletEndpoints.csbackend/Src/PIED_LMS.Presentation/APIs/StudentSubmissionEndpoints.csbackend/Src/PIED_LMS.Presentation/APIs/TeacherEndpoints.csbackend/Src/PIED_LMS.Presentation/APIs/TestCaseEndpoints.csbackend/Src/PIED_LMS.Presentation/APIs/TestRoomEndpoints.csbackend/Src/PIED_LMS.Presentation/Extensions/EndpointExtensions.cs
💤 Files with no reviewable changes (8)
- backend/Src/PIED_LMS.Application/UserCases/Commands/Course/AssignTeachersHandler.cs
- backend/Src/PIED_LMS.Contract/Services/Teacher/Response.cs
- backend/Src/PIED_LMS.Application/UserCases/Queries/Teacher/GetTeachersHandler.cs
- backend/Src/PIED_LMS.Contract/Services/Teacher/Query.cs
- backend/Src/PIED_LMS.Domain/Constants/RoleConstants.cs
- backend/Src/PIED_LMS.Presentation/APIs/TeacherEndpoints.cs
- backend/Src/PIED_LMS.Application/UserCases/Queries/Teacher/GetTeacherByIdHandler.cs
- backend/Src/PIED_LMS.Application/UserCases/Queries/Teacher/GetAllTeachersSimpleHandler.cs
Comment on lines
+63
to
+68
| course.Mentors.Clear(); | ||
|
|
||
| foreach (var mentor in mentors) | ||
| { | ||
| course.Mentors.Add(mentor); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Collection modification pattern is inefficient.
Using Clear() followed by individual Add() calls triggers multiple collection modification notifications in Entity Framework. Consider replacing the collection directly if the EF configuration supports it.
♻️ More efficient collection replacement
- course.Mentors.Clear();
-
- foreach (var mentor in mentors)
- {
- course.Mentors.Add(mentor);
- }
+ course.Mentors.Clear();
+ foreach (var mentor in mentors)
+ course.Mentors.Add(mentor);Or if your EF configuration allows collection replacement:
- course.Mentors.Clear();
- foreach (var mentor in mentors)
- {
- course.Mentors.Add(mentor);
- }
+ course.Mentors = mentors;Verify your entity configuration supports direct collection assignment before applying the second option.
📝 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.
Suggested change
| course.Mentors.Clear(); | |
| foreach (var mentor in mentors) | |
| { | |
| course.Mentors.Add(mentor); | |
| } | |
| course.Mentors = mentors; |
🤖 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/Course/AssignMentorsHandler.cs`
around lines 63 - 68, The Clear()+foreach pattern on course.Mentors causes many
change notifications; in AssignMentorsHandler replace that block with a single
assignment to the collection (e.g., course.Mentors = mentors.ToList() or
course.Mentors = new List<Mentor>(mentors)) so the navigation property is
swapped in one operation; ensure the target property type matches
(ICollection<T>/List<T>) and that your EF entity configuration supports direct
collection replacement before committing this change.
Comment on lines
+73
to
+93
| foreach (var mentor in mentors) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(mentor.Email)) | ||
| { | ||
| logger.LogWarning( | ||
| "Skipping email notification for mentor {MentorId} - email is null or empty", | ||
| mentor.Id); | ||
| continue; | ||
| } | ||
|
|
||
| await backgroundEmailQueue.EnqueueEmailAsync(new EmailJob( | ||
| mentor.Email, | ||
| $"{mentor.FirstName} {mentor.LastName}", | ||
| course.Title, | ||
| course.StartDate, | ||
| course.EndDate, | ||
| _courseManagementSettings.GetCourseUrl(course.Id), | ||
| _courseManagementSettings.EmailRetryAttempts, | ||
| _courseManagementSettings.EmailRetryDelayMs | ||
| )); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Email enqueueing occurs after database commit.
The email notifications are enqueued after the database transaction commits. If enqueueing fails (e.g., queue service unavailable), mentors will be assigned without receiving notifications. This creates a consistency gap between assignment and notification.
Consider either:
- Moving email enqueueing before the commit and wrapping in a transaction scope, or
- Implementing compensating logic to track un-notified assignments for retry
🤖 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/Course/AssignMentorsHandler.cs`
around lines 73 - 93, Assigning mentors currently commits DB changes before
calling backgroundEmailQueue.EnqueueEmailAsync in AssignMentorsHandler, which
can leave mentors assigned without notifications if enqueueing fails; fix by
either (A) moving the enqueue calls into the same transaction scope as the DB
updates (invoke EnqueueEmailAsync for each mentor before SaveChangesAsync /
transaction.Commit so enqueueing occurs inside the transaction), or (B)
implement compensating tracking: create and persist a PendingNotification entity
when assigning mentors (e.g., AddPendingNotification for each mentor) before
commit, then after commit attempt EnqueueEmailAsync and on success remove the
corresponding PendingNotification (or keep a retry worker to process
PendingNotification records until successful); reference
backgroundEmailQueue.EnqueueEmailAsync, AssignMentorsHandler, and the DB
save/transaction methods to locate where to apply the change.
Comment on lines
+116
to
+120
| if (mentorRole == null) | ||
| { | ||
| logger.LogError("Mentor role not found in the system"); | ||
| return users; | ||
| } |
There was a problem hiding this comment.
Critical: Handle missing Mentor role as a system error, not validation failure.
If the Mentor role doesn't exist in the system, returning all users as invalid will produce the misleading error "One or more users do not have the Mentor role." This is a system configuration error, not a validation failure. The operation should fail fast with an appropriate error message indicating system misconfiguration.
🐛 Fail fast on missing Mentor role
if (mentorRole == null)
{
logger.LogError("Mentor role not found in the system");
- return users;
+ throw new InvalidOperationException("Mentor role is not configured in the system. Please contact system administrator.");
}Alternatively, if throwing is not desired, return an error response directly from the Handle method after detecting this condition.
📝 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.
Suggested change
| if (mentorRole == null) | |
| { | |
| logger.LogError("Mentor role not found in the system"); | |
| return users; | |
| } | |
| if (mentorRole == null) | |
| { | |
| logger.LogError("Mentor role not found in the system"); | |
| throw new InvalidOperationException("Mentor role is not configured in the system. Please contact system administrator."); | |
| } |
🤖 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/Course/AssignMentorsHandler.cs`
around lines 116 - 120, The check for a missing Mentor role in
AssignMentorsHandler (mentorRole == null) currently logs and returns 'users',
which misclassifies a system configuration error as a validation failure; change
this to fail fast: when mentorRole is null, log the error with context (e.g.,
"Mentor role not found in the system") and then either throw an appropriate
exception (e.g., InvalidOperationException or a custom
SystemConfigurationException) from Handle, or return an error result/response
directly from Handle indicating system misconfiguration; update callers/tests to
expect this failure instead of treating it as a user validation issue.
Comment on lines
49
to
58
| catch (ArgumentException ex) | ||
| { | ||
| logger.LogError(ex, "Failed to upload thumbnail to S3 for course creation"); | ||
| return new ServiceResponse<int>(false, "Failed to upload thumbnail image. Please try again."); | ||
| return new ServiceResponse<Guid>(false, "Failed to upload thumbnail image. Please try again."); | ||
| } | ||
| catch (InvalidOperationException ex) | ||
| { | ||
| logger.LogError(ex, "Failed to upload thumbnail to S3 for course creation"); | ||
| return new ServiceResponse<int>(false, "Failed to upload thumbnail image. Please try again."); | ||
| return new ServiceResponse<Guid>(false, "Failed to upload thumbnail image. Please try again."); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consolidate duplicate exception handling.
Both ArgumentException and InvalidOperationException catch blocks log and return identical error messages. This violates the DRY principle.
♻️ Consolidate exception handlers
- catch (ArgumentException ex)
- {
- logger.LogError(ex, "Failed to upload thumbnail to S3 for course creation");
- return new ServiceResponse<Guid>(false, "Failed to upload thumbnail image. Please try again.");
- }
- catch (InvalidOperationException ex)
- {
- logger.LogError(ex, "Failed to upload thumbnail to S3 for course creation");
- return new ServiceResponse<Guid>(false, "Failed to upload thumbnail image. Please try again.");
- }
+ catch (Exception ex) when (ex is ArgumentException or InvalidOperationException)
+ {
+ logger.LogError(ex, "Failed to upload thumbnail to S3 for course creation");
+ return new ServiceResponse<Guid>(false, "Failed to upload thumbnail image. Please try again.");
+ }🤖 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/Course/CreateCourseHandler.cs`
around lines 49 - 58, The two identical catch blocks in CreateCourseHandler
(catching ArgumentException and InvalidOperationException) should be
consolidated into a single handler to avoid duplication; replace them with one
conditional catch that logs via logger.LogError(ex, "...") and returns the same
new ServiceResponse<Guid>(false, "Failed to upload thumbnail image. Please try
again.") — for example use a single catch (Exception ex) when (ex is
ArgumentException || ex is InvalidOperationException) { ... } so the identical
behavior around logging and returning is centralized.
Comment on lines
+58
to
+64
| if (!result.Succeeded) | ||
| { | ||
| var errors = result.Errors.Select(e => e.Description).ToList(); | ||
| logger.LogWarning("Failed to update profile for user {UserId}: {Errors}", user.Id, | ||
| string.Join(", ", errors)); | ||
| return new ServiceResponse<string>(false, "Failed to update profile"); | ||
| } |
There was a problem hiding this comment.
Error details are not surfaced to the user.
The error descriptions are collected on line 60 and logged on line 62, but the response on line 63 returns only a generic "Failed to update profile" message. Users cannot determine what went wrong with their profile update (e.g., validation failures, username conflicts, etc.).
Return the specific error messages to help users correct their input.
💡 Include error details in the response
if (!result.Succeeded)
{
var errors = result.Errors.Select(e => e.Description).ToList();
logger.LogWarning("Failed to update profile for user {UserId}: {Errors}", user.Id,
string.Join(", ", errors));
- return new ServiceResponse<string>(false, "Failed to update profile");
+ return new ServiceResponse<string>(false, $"Failed to update profile: {string.Join("; ", errors)}");
}🤖 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 58 - 64, The handler currently logs result.Errors but returns only
a generic message; update UpdateProfileHandler to include the specific error
descriptions from result.Errors (the errors variable built from
result.Errors.Select(...)) in the ServiceResponse<string> returned on failure so
callers get actionable feedback (either by passing the errors collection into
ServiceResponse<string> if it has an errors/details field or by appending
string.Join(", ", errors) to the message); keep the existing
logger.LogWarning(user.Id, errors) behavior but ensure the returned
ServiceResponse contains the actual error descriptions instead of just "Failed
to update profile".
Comment on lines
+35
to
+39
| private static async Task<IResult> GetRandomQuestion(ISender sender, HttpContext context) | ||
| { | ||
| var result = await sender.Send(new GetRandomQuestionQuery()); | ||
| return result.ToActionResult(context); | ||
| } |
There was a problem hiding this comment.
Missing CancellationToken propagation.
Lines 35-39 do not accept or propagate a CancellationToken. This prevents proper request cancellation—if the client disconnects, the query continues executing, wasting resources.
🛡️ Proposed fix to add cancellation support
-private static async Task<IResult> GetRandomQuestion(ISender sender, HttpContext context)
+private static async Task<IResult> GetRandomQuestion(
+ ISender sender,
+ HttpContext context,
+ CancellationToken cancellationToken)
{
- var result = await sender.Send(new GetRandomQuestionQuery());
+ var result = await sender.Send(new GetRandomQuestionQuery(), cancellationToken);
return result.ToActionResult(context);
}🤖 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/APIs/QuestionEndpoints.cs` around lines 35
- 39, GetRandomQuestion currently doesn't accept or propagate a
CancellationToken, so update the GetRandomQuestion method signature to accept a
CancellationToken (e.g., CancellationToken cancellationToken) and pass that
token into the Send call (sender.Send(new GetRandomQuestionQuery(),
cancellationToken)); also propagate the token into the result conversion if
available (e.g., result.ToActionResult(context, cancellationToken)) so the
request can be cancelled when the client disconnects. Ensure you update any
callers or route bindings that invoke GetRandomQuestion to supply the
cancellation token from the HttpContext or framework binding.
Comment on lines
+41
to
+49
| private static async Task<IResult> CheckAnswer( | ||
| [FromBody] CheckAnswerRequest request, | ||
| ISender sender, | ||
| HttpContext context) | ||
| { | ||
| var command = new CheckQuestionAnswerCommand(request.QuestionId, request.SelectedOptions); | ||
| var result = await sender.Send(command); | ||
| return result.ToActionResult(context); | ||
| } |
There was a problem hiding this comment.
Missing CancellationToken propagation.
Lines 41-49 do not accept or propagate a CancellationToken. This prevents proper request cancellation, causing the command to execute even after the client disconnects.
🛡️ Proposed fix to add cancellation support
private static async Task<IResult> CheckAnswer(
[FromBody] CheckAnswerRequest request,
ISender sender,
- HttpContext context)
+ HttpContext context,
+ CancellationToken cancellationToken)
{
var command = new CheckQuestionAnswerCommand(request.QuestionId, request.SelectedOptions);
- var result = await sender.Send(command);
+ var result = await sender.Send(command, cancellationToken);
return result.ToActionResult(context);
}📝 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.
Suggested change
| private static async Task<IResult> CheckAnswer( | |
| [FromBody] CheckAnswerRequest request, | |
| ISender sender, | |
| HttpContext context) | |
| { | |
| var command = new CheckQuestionAnswerCommand(request.QuestionId, request.SelectedOptions); | |
| var result = await sender.Send(command); | |
| return result.ToActionResult(context); | |
| } | |
| private static async Task<IResult> CheckAnswer( | |
| [FromBody] CheckAnswerRequest request, | |
| ISender sender, | |
| HttpContext context, | |
| CancellationToken cancellationToken) | |
| { | |
| var command = new CheckQuestionAnswerCommand(request.QuestionId, request.SelectedOptions); | |
| var result = await sender.Send(command, cancellationToken); | |
| return result.ToActionResult(context); | |
| } |
🤖 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/APIs/QuestionEndpoints.cs` around lines 41
- 49, The CheckAnswer endpoint does not accept or forward a CancellationToken,
so add a CancellationToken parameter to the method signature (e.g.,
CancellationToken cancellationToken), accept it from the framework, and
propagate it when sending the MediatR command and converting the result: pass
cancellationToken into sender.Send(...) for the CheckQuestionAnswerCommand and
into result.ToActionResult(context, cancellationToken) (or the equivalent
overload) so the request can be cancelled if the client disconnects.
Comment on lines
+30
to
+34
| // Admins/Mentors could also use this if they have the ID | ||
| group.MapGet("/submissions/{id:guid}", GetSubmissionById) | ||
| .WithName("GetSubmissionById") | ||
| .RequireAuthorization() // general auth since both teacher and student can view | ||
| .WithServiceResponseOpenApi< | ||
| SubmissionDetailResponse>(ServiceResponseStatusProfile.OkOrBadRequestOrNotFound); | ||
| .RequireAuthorization() // general auth since both mentor and student can view | ||
| .WithServiceResponseOpenApi<SubmissionDetailResponse>(ServiceResponseStatusProfile.OkOrBadRequestOrNotFound); |
There was a problem hiding this comment.
Restrict endpoint roles at the API boundary.
Line 33 currently allows any authenticated role. That is an authorization scope regression for a sensitive resource-by-id endpoint. Enforce explicit roles here and keep handler checks as defense-in-depth.
Suggested fix
- group.MapGet("/submissions/{id:guid}", GetSubmissionById)
- .WithName("GetSubmissionById")
- .RequireAuthorization() // general auth since both mentor and student can view
- .WithServiceResponseOpenApi<SubmissionDetailResponse>(ServiceResponseStatusProfile.OkOrBadRequestOrNotFound);
+ group.MapGet("/submissions/{id:guid}", GetSubmissionById)
+ .WithName("GetSubmissionById")
+ .RequireAuthorization(policy => policy.RequireRole("Student", "Mentor", "Administrator"))
+ .WithServiceResponseOpenApi<SubmissionDetailResponse>(ServiceResponseStatusProfile.OkOrBadRequestOrNotFound);📝 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.
Suggested change
| // Admins/Mentors could also use this if they have the ID | |
| group.MapGet("/submissions/{id:guid}", GetSubmissionById) | |
| .WithName("GetSubmissionById") | |
| .RequireAuthorization() // general auth since both teacher and student can view | |
| .WithServiceResponseOpenApi< | |
| SubmissionDetailResponse>(ServiceResponseStatusProfile.OkOrBadRequestOrNotFound); | |
| .RequireAuthorization() // general auth since both mentor and student can view | |
| .WithServiceResponseOpenApi<SubmissionDetailResponse>(ServiceResponseStatusProfile.OkOrBadRequestOrNotFound); | |
| // Admins/Mentors could also use this if they have the ID | |
| group.MapGet("/submissions/{id:guid}", GetSubmissionById) | |
| .WithName("GetSubmissionById") | |
| .RequireAuthorization(policy => policy.RequireRole("Student", "Mentor", "Administrator")) | |
| .WithServiceResponseOpenApi<SubmissionDetailResponse>(ServiceResponseStatusProfile.OkOrBadRequestOrNotFound); |
🤖 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/APIs/StudentSubmissionEndpoints.cs` around
lines 30 - 34, The MapGet endpoint for "/submissions/{id:guid}" currently calls
RequireAuthorization() which allows any authenticated user; restrict it at the
API boundary by specifying the allowed roles on the endpoint (e.g., require
Student, Mentor and Admin roles) instead of the generic call — update the
authorization on the MapGet("/submissions/{id:guid}", GetSubmissionById) chain
(the RequireAuthorization call) to require explicit roles or a dedicated policy
(keeping the existing GetSubmissionById handler checks as defense-in-depth).
Comment on lines
+9
to
+15
| if (result.Success) | ||
| { | ||
| if (result.Data is null && typeof(T).IsClass) | ||
| return Results.NotFound(result); | ||
|
|
||
| return Results.Ok(result); | ||
| } |
There was a problem hiding this comment.
Success response with null data should not return NotFound.
Lines 11-12 return NotFound when result.Success == true but result.Data is null. This creates a semantic mismatch: a successful operation should not yield an HTTP 404 status.
This logic masks handler implementation errors. Handlers should explicitly set Success = false with an appropriate error code when data is not found, rather than relying on this fallback.
Returning 404 for a "successful" response degrades error signaling and creates inconsistent API behavior.
🤖 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 - 15, The current branch inside the success path checks result.Success
and then returns Results.NotFound when result.Data is null (and
typeof(T).IsClass), which incorrectly maps a successful operation to HTTP 404;
update the success handling in the method that inspects result.Success so that
when result.Success == true you always return Results.Ok(result) (remove the
Results.NotFound branch), and ensure callers/handlers set result.Success = false
and appropriate error info when data is truly missing so that NotFound (or other
error responses) are produced only from non-success results.
Comment on lines
+19
to
+21
| if (result.IsNotFound || | ||
| (result.ErrorCode is not null && result.ErrorCode.Contains("NOT_FOUND")) || | ||
| (result.Message is not null && result.Message.Contains("not found", StringComparison.OrdinalIgnoreCase))) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Message-based NotFound detection is fragile.
Line 21 checks if result.Message contains "not found" to determine HTTP 404 status. This creates implicit coupling between message text and HTTP status codes, which is unreliable.
Message content can vary and may legitimately contain "not found" in contexts where 404 is inappropriate. Use ErrorCode as the authoritative source for error classification.
🤖 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 19 - 21, The NotFound detection currently relies on message text
(result.Message.Contains("not found")), which is fragile; update the check in
the EndpointExtensions code to remove the Message-based check and rely on
result.IsNotFound and a canonical ErrorCode check instead (e.g., inspect
result.ErrorCode for a NOT_FOUND marker using a null-safe comparison on
result.ErrorCode, such as result.ErrorCode is not null &&
result.ErrorCode.Contains("NOT_FOUND", StringComparison.OrdinalIgnoreCase)), so
only IsNotFound or ErrorCode determine 404 handling and any Message inspection
is eliminated.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+30
to
+33
| catch (Exception ex) | ||
| { | ||
| logger.LogError(ex, "Unexpected error in Email Background Service loop."); | ||
| } |
Comment on lines
+72
to
+88
| catch (Exception ex) | ||
| { | ||
| if (attempt >= job.MaxRetryAttempts) | ||
| { | ||
| logger.LogError(ex, | ||
| "Failed to send course assignment email to {Email} after {Attempts} attempts", | ||
| job.Email, job.MaxRetryAttempts); | ||
| } | ||
| else | ||
| { | ||
| logger.LogWarning(ex, | ||
| "Failed to send course assignment email to {Email} on attempt {Attempt}. Retrying in {DelayMs}ms", | ||
| job.Email, attempt, job.RetryDelayMs); | ||
|
|
||
| await Task.Delay(job.RetryDelayMs, stoppingToken); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Refactor
Bug Fixes