feat(course): refactor curriculum and insight to use Clean Architectu…#87
Merged
Conversation
…re and DDD - Introduce CurriculumSection Value Object with domain validations - Migrate Course Curriculum from string to List<CurriculumSection> - Use EF Core HasConversion for Curriculum persistence - Update Command/Query Handlers and DTOs to use strongly typed collections - Remove deprecated CourseContentHelper
|
Warning Review limit reached
More reviews will be available in 7 minutes and 34 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR refactors course curriculum and insight management from a utility-based JSON serialization pattern to a domain-driven approach. ChangesCourse Curriculum Domain Refactoring
Possibly Related PRs
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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.Domain/Entities/Course.cs (1)
24-40:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftStop exposing the aggregate’s curriculum as a mutable
List<>.Lines 24-40 still allow callers to mutate
Course.Curriculumdirectly or mutate the same list instance afterSetCurriculumreturns, which bypasses the new domain validation entirely. Store a copy and expose a read-only collection/backing field instead.🤖 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.Domain/Entities/Course.cs` around lines 24 - 40, Course.Curriculum is exposed as a mutable List allowing external mutation and bypassing domain validation; change it to expose a read-only collection (IReadOnlyList<CurriculumSection> or ReadOnlyCollection<CurriculumSection>) backed by a private List<CurriculumSection> field (e.g., _curriculum), and update SetCurriculum to assign a defensive copy (new List<CurriculumSection>(curriculum ?? Enumerable.Empty<CurriculumSection>())) to that private field; have the public Curriculum getter return the read-only view (e.g., _curriculum.AsReadOnly() or the IReadOnlyList) so callers cannot mutate the aggregate or the original list after SetCurriculum returns.
🤖 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/CreateCourseHandler.cs`:
- Around line 83-97: CreateCourseHandler currently calls SaveFileAsync before
validating CurriculumSection and Insight, so if course.SetCurriculum or
course.SetInsight throws (ArgumentException) an uploaded thumbnail is orphaned;
move the validation logic so you construct/validate the domainCurriculum (using
Domain.Entities.CurriculumSection) and validate request.Insight (or call
SetCurriculum/SetInsight on a temp Course object) before invoking SaveFileAsync,
or, if you prefer to keep upload first, catch exceptions after
SetCurriculum/SetInsight and delete the uploaded file by calling the same
storage delete routine used elsewhere; update the try/catch around SetCurriculum
and SetInsight to ensure uploaded file is removed on failure and retain the
logger.LogWarning/ServiceResponse behavior.
In
`@backend/Src/PIED_LMS.Application/UserCases/Commands/Course/UpdateCourseHandler.cs`:
- Around line 131-137: The handler currently calls course.SetCurriculum and
course.SetInsight after performing thumbnail side effects, which can delete the
old thumbnail and upload a new one before validation fails; move all domain
validation (invoke SetCurriculum, SetInsight and any other domain setters that
can throw, e.g., Validate... on the Course aggregate) to run before any
thumbnail Delete/Upload operations so that ArgumentException is raised prior to
making storage changes, and only if validation succeeds proceed to call the
thumbnail methods (e.g., DeleteThumbnail/UploadThumbnail) and then
persist/commit the changes.
- Around line 121-130: The handler currently treats a null
UpdateCourseCommand.Curriculum as an instruction to clear the course curriculum;
change UpdateCourseHandler so that when request.Curriculum is null it does
nothing (preserve existing curriculum), and only call course.SetCurriculum(...)
when request.Curriculum is non-null — mapping items to
Domain.Entities.CurriculumSection for a non-empty list, and explicitly clearing
(e.g. course.SetCurriculum(null) or an agreed “clear” value) only when
request.Curriculum is an empty list; update the branch around request.Curriculum
and calls to course.SetCurriculum accordingly.
In `@backend/Src/PIED_LMS.Domain/Entities/CurriculumSection.cs`:
- Around line 7-25: The Content property currently exposes a mutable
List<string> and the constructor stores the caller's list reference directly;
change CurriculumSection.Content to expose an immutable/read-only type (e.g.,
IReadOnlyList<string> or ReadOnlyCollection<string>) and in the
CurriculumSection(string title, string summary, List<string> content)
constructor make a defensive copy of the incoming list (new
List<string>(content)) and assign a read-only wrapper (e.g., AsReadOnly or cast
to IReadOnlyList) so consumers cannot mutate the internal collection after
construction while preserving EF compatibility via the private parameterless
constructor.
In `@backend/Src/PIED_LMS.Persistence/Configurations/CourseConfiguration.cs`:
- Line 1: Course.Curriculum is mapped via JSON conversion but lacks a
ValueComparer, so EF Core will track snapshots by reference and miss in-place
edits; in CourseConfiguration add a ValueComparer for the converted type (e.g.,
ValueComparer<List<CurriculumSection>>) and attach it to the property metadata
(via .Metadata.SetValueComparer(...) or the fluent API). Implement the comparer
to compare value equality (either by comparing serialized JSON strings or deep
sequence equality on CurriculumSection items) and to produce snapshots by
cloning (e.g., via JsonSerializer serialize/deserialize) so mutations of
CurriculumSection.Content are detected; update the CourseConfiguration mapping
for the Curriculum property to use this comparer alongside the existing
HasConversion/JsonSerializer usage.
---
Outside diff comments:
In `@backend/Src/PIED_LMS.Domain/Entities/Course.cs`:
- Around line 24-40: Course.Curriculum is exposed as a mutable List allowing
external mutation and bypassing domain validation; change it to expose a
read-only collection (IReadOnlyList<CurriculumSection> or
ReadOnlyCollection<CurriculumSection>) backed by a private
List<CurriculumSection> field (e.g., _curriculum), and update SetCurriculum to
assign a defensive copy (new List<CurriculumSection>(curriculum ??
Enumerable.Empty<CurriculumSection>())) to that private field; have the public
Curriculum getter return the read-only view (e.g., _curriculum.AsReadOnly() or
the IReadOnlyList) so callers cannot mutate the aggregate or the original list
after SetCurriculum returns.
🪄 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: 792bdc10-0b6c-4eed-8c9f-505fe0255a2d
📒 Files selected for processing (11)
backend/Src/PIED_LMS.Application/UserCases/Commands/Course/CreateCourseHandler.csbackend/Src/PIED_LMS.Application/UserCases/Commands/Course/UpdateCourseHandler.csbackend/Src/PIED_LMS.Application/UserCases/CourseContentHelper.csbackend/Src/PIED_LMS.Application/UserCases/Queries/Course/GetCourseByIdHandler.csbackend/Src/PIED_LMS.Application/UserCases/Queries/Course/GetCourseCurriculumHandler.csbackend/Src/PIED_LMS.Application/UserCases/Queries/Course/GetCoursesHandler.csbackend/Src/PIED_LMS.Contract/Services/Course/Command.csbackend/Src/PIED_LMS.Contract/Services/Course/Request.csbackend/Src/PIED_LMS.Domain/Entities/Course.csbackend/Src/PIED_LMS.Domain/Entities/CurriculumSection.csbackend/Src/PIED_LMS.Persistence/Configurations/CourseConfiguration.cs
💤 Files with no reviewable changes (1)
- backend/Src/PIED_LMS.Application/UserCases/CourseContentHelper.cs
| @@ -1,3 +1,4 @@ | |||
| using System.Text.Json; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'Property\(c => c\.Curriculum\)|HasConversion|SetValueComparer|ValueComparer<|public List<CurriculumSection> Curriculum|List<string> Content' backend/SrcRepository: PiedTeam/pied-lms
Length of output: 6280
Add a ValueComparer for the JSON-converted Course.Curriculum collection (missing today).
CourseConfiguration maps Course.Curriculum as a mutable List<CurriculumSection> via HasConversion(...JsonSerializer...), but there is no ValueComparer/.Metadata.SetValueComparer(...) configured (no such configuration exists in backend/Src). EF Core will then track snapshots by reference, so in-place edits can be missed—including edits to CurriculumSection.Content, since that list is serialized inside the Course.Curriculum JSON payload. (https://learn.microsoft.com/en-us/ef/core/modeling/value-comparers)
Proposed fix
+using Microsoft.EntityFrameworkCore.ChangeTracking;
using System.Text.Json;
using PIED_LMS.Domain.Entities;
@@
- var jsonOptions = new JsonSerializerOptions { PropertyNameCaseInsensitive = true };
+ var jsonOptions = new JsonSerializerOptions { PropertyNameCaseInsensitive = true };
+ var curriculumComparer = new ValueComparer<List<CurriculumSection>>(
+ (left, right) =>
+ JsonSerializer.Serialize(left ?? new List<CurriculumSection>(), jsonOptions) ==
+ JsonSerializer.Serialize(right ?? new List<CurriculumSection>(), jsonOptions),
+ value =>
+ JsonSerializer.Serialize(value ?? new List<CurriculumSection>(), jsonOptions).GetHashCode(),
+ value =>
+ JsonSerializer.Deserialize<List<CurriculumSection>>(
+ JsonSerializer.Serialize(value ?? new List<CurriculumSection>(), jsonOptions),
+ jsonOptions) ?? new List<CurriculumSection>());
+
builder.Property(c => c.Curriculum)
.HasConversion(
v => JsonSerializer.Serialize(v, jsonOptions),
v => string.IsNullOrWhiteSpace(v)
? new List<CurriculumSection>()
: JsonSerializer.Deserialize<List<CurriculumSection>>(v, jsonOptions) ?? new List<CurriculumSection>()
- );
+ )
+ .Metadata.SetValueComparer(curriculumComparer);🤖 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.Persistence/Configurations/CourseConfiguration.cs` at
line 1, Course.Curriculum is mapped via JSON conversion but lacks a
ValueComparer, so EF Core will track snapshots by reference and miss in-place
edits; in CourseConfiguration add a ValueComparer for the converted type (e.g.,
ValueComparer<List<CurriculumSection>>) and attach it to the property metadata
(via .Metadata.SetValueComparer(...) or the fluent API). Implement the comparer
to compare value equality (either by comparing serialized JSON strings or deep
sequence equality on CurriculumSection items) and to produce snapshots by
cloning (e.g., via JsonSerializer serialize/deserialize) so mutations of
CurriculumSection.Content are detected; update the CourseConfiguration mapping
for the Curriculum property to use this comparer alongside the existing
HasConversion/JsonSerializer usage.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+104
to
+107
| catch (Exception deleteEx) | ||
| { | ||
| logger.LogError(deleteEx, "Failed to delete orphaned thumbnail {ThumbnailPath}", thumbnailPath); | ||
| } |
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.
…re and DDD
Introduce CurriculumSection Value Object with domain validations
Migrate Course Curriculum from string to List
Use EF Core HasConversion for Curriculum persistence
Update Command/Query Handlers and DTOs to use strongly typed collections
Remove deprecated CourseContentHelper
Summary by CodeRabbit
New Features
Bug Fixes
Refactor