Conversation
Co-authored-by: Copilot <copilot@github.com>
…ception middleware Co-authored-by: Copilot <copilot@github.com>
…dlerTests with notification fanout service
📝 WalkthroughWalkthroughCác thay đổi bao gồm việc chuyển hướng một dự án kiểm thử từ .NET 10.0 sang 8.0 kèm cập nhật gói, thêm một lớp tiện ích ghi nhật ký tĩnh mới, tạo mock cho các dịch vụ thông báo trong các bài kiểm thử, tổ chức lại không gian tên middleware, hủy bỏ bỏ qua thư mục nhật ký trong gitignore và vô hiệu hóa bộ nhớ đệm SDK trong quy trình CI. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 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)
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: 3
🧹 Nitpick comments (1)
backend_system/ContentService/ContentService.Tests/Features/Articles/Management/ApproveArticleHandlerTests.cs (1)
90-94: Nên thêm test cho nhánh “article đã publish trước đó” để khóa chặt logic fanout một lần.Hiện đã cover “tồn tại & publish lần đầu” và “không tồn tại”, nhưng chưa chốt hành vi khi
IsPublished = truengay từ đầu (theo handler thì fanout phải không gọi).🧪 Test bổ sung đề xuất
+ [Fact] + public async Task Handle_ShouldNotFanout_WhenArticleAlreadyPublished() + { + // Arrange + var articleId = Guid.NewGuid(); + var article = new Article + { + Id = articleId, + Title = "Already published", + Slug = "already-published", + Content = "content", + IsPublished = true, + PublishedAt = DateTime.UtcNow.AddDays(-1), + }; + + await _context.Articles.AddAsync(article); + await _context.SaveChangesAsync(); + + var command = new ApproveArticleCommand(articleId); + + // Act + var result = await _handler.Handle(command, CancellationToken.None); + + // Assert + result.Should().BeTrue(); + _fanoutMock.Verify( + x => x.NotifyFollowersOnFirstPublishAsync( + It.IsAny<Guid>(), + It.IsAny<CancellationToken>() + ), + Times.Never + ); + }Also applies to: 107-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend_system/ContentService/ContentService.Tests/Features/Articles/Management/ApproveArticleHandlerTests.cs` around lines 90 - 94, Add a new unit test in ApproveArticleHandlerTests that arranges an Article with IsPublished = true (simulate existing published article) and the same articleId used in other tests, invokes ApproveArticleHandler.HandleAsync (or the test helper used), and asserts that _fanoutMock.NotifyFollowersOnFirstPublishAsync is never called (Times.Never) while preserving other expectations; locate the existing tests around the _fanoutMock.Verify call and mirror their setup but set IsPublished true to assert the fanout branch is locked down.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 35-36: Hiện .gitignore đang bỏ dấu un-ignore cho toàn bộ thư mục
Logs (dòng với "!.../Logs/"), điều này có thể khiến các file log được track
nhầm; hãy thay rule un-ignore thư mục bằng một rule chỉ un-ignore đúng file mã
nguồn cần thiết (ví dụ chỉ un-ignore LogExceptions.cs) và xóa/không dùng rule
"!.../Logs/" để đảm bảo chỉ file LogExceptions.cs được theo dõi; cập nhật entry
liên quan (the one that currently negates Logs/) và giữ chỉ the specific
negation for the LogExceptions.cs filename.
In
`@backend_system/ContentService/ContentService.Tests/ContentService.Tests.csproj`:
- Around line 13-14: The project uses floating package versions ("8.0.*") which
can cause restore drift; update the PackageReference entries for
Microsoft.AspNetCore.Mvc.Testing and Microsoft.EntityFrameworkCore.InMemory in
ContentService.Tests.csproj to a fixed version (e.g., change "8.0.*" to "8.0.0"
to match IdentityService.Tests.csproj) or alternatively enable lock-file based
restores by adding
<RestorePackagesWithLockFile>true</RestorePackagesWithLockFile> to the
PropertyGroup so a packages.lock.json is produced; pick one approach and apply
it consistently for the two PackageReference items in this csproj.
In `@backend_system/SharedLibrary/Library/Logs/LogExceptions.cs`:
- Around line 6-9: The LogException method currently uses Debug.WriteLine and is
marked with [Conditional("DEBUG")] so it is stripped in Release; remove the
conditional attribute and replace the Debug.WriteLine call in
LogExceptions.LogException(Exception ex) with an always-on logging channel such
as System.Diagnostics.Trace.TraceError(ex.ToString()) (or another
production-capable sink) so the fallback logger emits errors in production;
ensure the method no longer depends on the DEBUG symbol and preserves the
exception.ToString() details when writing the log.
---
Nitpick comments:
In
`@backend_system/ContentService/ContentService.Tests/Features/Articles/Management/ApproveArticleHandlerTests.cs`:
- Around line 90-94: Add a new unit test in ApproveArticleHandlerTests that
arranges an Article with IsPublished = true (simulate existing published
article) and the same articleId used in other tests, invokes
ApproveArticleHandler.HandleAsync (or the test helper used), and asserts that
_fanoutMock.NotifyFollowersOnFirstPublishAsync is never called (Times.Never)
while preserving other expectations; locate the existing tests around the
_fanoutMock.Verify call and mirror their setup but set IsPublished true to
assert the fanout branch is locked down.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 39558a86-4d1c-4e64-826d-b320ecafdc8c
📒 Files selected for processing (6)
.github/workflows/ci-backend.yml.gitignorebackend_system/ContentService/ContentService.Tests/ContentService.Tests.csprojbackend_system/ContentService/ContentService.Tests/Features/Articles/Management/ApproveArticleHandlerTests.csbackend_system/SharedLibrary/Library/Logs/LogExceptions.csbackend_system/SharedLibrary/Library/Middleware/GlobalExceptions.cs
💤 Files with no reviewable changes (1)
- .github/workflows/ci-backend.yml
Comment on lines
+35
to
+36
| !backend_system/SharedLibrary/Library/Logs/ | ||
| !backend_system/SharedLibrary/Library/Logs/LogExceptions.cs |
There was a problem hiding this comment.
Giới hạn phạm vi un-ignore để tránh commit nhầm file log.
Ở Line 35, việc mở toàn bộ .../Logs/ có thể khiến các file log phát sinh trong thư mục này được track ngoài ý muốn. Nên chỉ mở đúng file mã nguồn cần thiết.
Đề xuất chỉnh rule an toàn hơn
!backend_system/SharedLibrary/Library/Logs/
+backend_system/SharedLibrary/Library/Logs/*
!backend_system/SharedLibrary/Library/Logs/LogExceptions.cs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 35 - 36, Hiện .gitignore đang bỏ dấu un-ignore cho
toàn bộ thư mục Logs (dòng với "!.../Logs/"), điều này có thể khiến các file log
được track nhầm; hãy thay rule un-ignore thư mục bằng một rule chỉ un-ignore
đúng file mã nguồn cần thiết (ví dụ chỉ un-ignore LogExceptions.cs) và xóa/không
dùng rule "!.../Logs/" để đảm bảo chỉ file LogExceptions.cs được theo dõi; cập
nhật entry liên quan (the one that currently negates Logs/) và giữ chỉ the
specific negation for the LogExceptions.cs filename.
Comment on lines
+13
to
+14
| <PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" Version="8.0.*" /> | ||
| <PackageReference Include="Microsoft.EntityFrameworkCore.InMemory" Version="8.0.*" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Tìm cấu hình quản lý version/lock
fd -HI 'Directory.Packages.props|packages.lock.json|global.json|NuGet.Config'
# Kiểm tra các csproj có bật lock mode và đang dùng floating version ở đâu
rg -n --glob '*.csproj' \
'RestorePackagesWithLockFile|RestoreLockedMode|ManagePackageVersionsCentrally|PackageReference Include="Microsoft.AspNetCore.Mvc.Testing"|PackageReference Include="Microsoft.EntityFrameworkCore.InMemory"|Version="[^"]*\*"'Repository: DoawnLet/BloggingPlatform
Length of output: 2256
Khóa version cụ thể để tránh restore drift trên CI.
File này sử dụng floating version 8.0.*, nhưng IdentityService.Tests.csproj lại sử dụng version cố định 8.0.0. Floating version có thể gây restore drift—kết quả restore thay đổi theo thời điểm, khiến test khó tái lập.
Đồng bộ cách quản lý version bằng một trong hai cách:
- Khóa version cụ thể: Thay
8.0.*thành8.0.0(đồng bộ vớiIdentityService.Tests.csproj) - Bật lock file: Thêm
<RestorePackagesWithLockFile>true</RestorePackagesWithLockFile>vàoPropertyGroupvà tạopackages.lock.json
Hiện tại không có cơ chế lock hoặc centralized package management trong repo.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend_system/ContentService/ContentService.Tests/ContentService.Tests.csproj`
around lines 13 - 14, The project uses floating package versions ("8.0.*") which
can cause restore drift; update the PackageReference entries for
Microsoft.AspNetCore.Mvc.Testing and Microsoft.EntityFrameworkCore.InMemory in
ContentService.Tests.csproj to a fixed version (e.g., change "8.0.*" to "8.0.0"
to match IdentityService.Tests.csproj) or alternatively enable lock-file based
restores by adding
<RestorePackagesWithLockFile>true</RestorePackagesWithLockFile> to the
PropertyGroup so a packages.lock.json is produced; pick one approach and apply
it consistently for the two PackageReference items in this csproj.
Comment on lines
+6
to
+9
| public static void LogException(Exception ex) | ||
| { | ||
| // Prefer structured logging in DI; this preserves call sites without a logger instance. | ||
| System.Diagnostics.Debug.WriteLine(ex); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Trong .NET, System.Diagnostics.Debug.WriteLine có bị loại bỏ khi build không define DEBUG (ví dụ cấu hình Release) không?
💡 Result:
Có, System.Diagnostics.Debug.WriteLine bị loại bỏ (không được biên dịch vào mã) khi build không define DEBUG, ví dụ cấu hình Release mặc định.
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.debug?view=net-9.0
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.debug?view=net-10.0
- 3: https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.debug.writeline?view=net-9.0
- 4: https://stackoverflow.com/questions/1600985/system-diagnostics-debug-writeline-in-production-code
- 5: https://stackoverflow.com/questions/5419534/debug-writeline-in-release-build
- 6: https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.debug.writeline?view=net-10.0
Debug.WriteLine ở Line 9 không hoạt động trong bản Release.
Phương pháp này được trang bị [Conditional("DEBUG")] nên sẽ bị loại bỏ hoàn toàn khi biên dịch mà không định nghĩa DEBUG symbol. Fallback logger sẽ không ghi bất kỳ log nào ở production.
Đề xuất thay bằng kênh ghi luôn hoạt động
public static void LogException(Exception ex)
{
- // Prefer structured logging in DI; this preserves call sites without a logger instance.
- System.Diagnostics.Debug.WriteLine(ex);
+ if (ex is null) return;
+ // Fallback tối thiểu, hoạt động cả khi không có DEBUG symbol.
+ Console.Error.WriteLine(ex);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend_system/SharedLibrary/Library/Logs/LogExceptions.cs` around lines 6 -
9, The LogException method currently uses Debug.WriteLine and is marked with
[Conditional("DEBUG")] so it is stripped in Release; remove the conditional
attribute and replace the Debug.WriteLine call in
LogExceptions.LogException(Exception ex) with an always-on logging channel such
as System.Diagnostics.Trace.TraceError(ex.ToString()) (or another
production-capable sink) so the fallback logger emits errors in production;
ensure the method no longer depends on the DEBUG symbol and preserves the
exception.ToString() details when writing the log.
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
1 participant
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
Ghi chú phát hành
Tests
Chores