Notification System Implementation#20
Conversation
…ceId, Update Service methods to use new approach
…ent the type of notification
WalkthroughAdds a real-time notifications feature: new NotificationController, service, repository methods, DTOs, SignalR hub, and tests. Updates Program.cs to wire SignalR, CORS, JWT for hub, OpenAPI, and DI. Renames Notification model fields, adds Type/CreatedAt/UpdatedAt, and includes an EF migration/snapshot. Test project gains Moq and BLL reference. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Admin Client
participant API as API (NotificationController)
participant SVC as NotificationService
participant REPO as NotificationRepository
participant DB as Database
participant HUB as SignalR NotificationHub
participant Users as user_{userId} group
Admin->>API: POST /api/Notification (JWT)
API->>SVC: CreateNotification(userId, type, resourceId, message)
SVC->>REPO: Add Notification + GetActorUserByResourceId(...)
REPO->>DB: Insert notification, query actor
DB-->>REPO: Persisted entity + actor
SVC->>HUB: Clients.Group("user_{userId}").Send(ReceiveNotification, payload)
HUB-->>Users: ReceiveNotification(payload)
SVC-->>API: Completed
API-->>Admin: 200 OK
sequenceDiagram
autonumber
actor User as App Client
participant HUB as SignalR NotificationHub
participant Auth as JWT Bearer
User->>HUB: Connect /notificationHub?access_token=JWT
HUB->>Auth: Validate token (OnMessageReceived)
Auth-->>HUB: Principal with UserIdentifier
HUB->>HUB: OnConnectedAsync -> AddToGroup("user_{userId}")
HUB-->>User: Connected
User->>HUB: JoinUserGroup(userId) [optional]
HUB-->>User: Ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (41)
AskFm/AskFm.API/Properties/launchSettings.json (1)
17-18: Mirror the correct launchUrl for the HTTPS profile as well.Once you confirm the actual UI/doc endpoint, apply the same path here to avoid inconsistent behavior between profiles.
AskFm/AskFm.BLL/DTO/ActorDto.cs (2)
5-7: Fix nullable/required contract for DTO strings.With Nullable enabled, non-nullable strings must be initialized or marked required.
Apply this diff:
-public class ActorDto +public class ActorDto { public int Id { get; set; } - public string Username { get; set; } - public string AvatarPath { get; set; } + public required string Username { get; set; } + public string? AvatarPath { get; set; } }
3-8: Optional: prefer immutable DTOs for safer mapping/serialization.Consider a record with init-only setters.
-public class ActorDto +public record ActorDto { - public int Id { get; set; } - public required string Username { get; set; } - public string? AvatarPath { get; set; } + public int Id { get; init; } + public required string Username { get; init; } + public string? AvatarPath { get; init; } }AskFm/AskFm.BLL/DTO/PaginationDto.cs (1)
3-10: Optional: make pagination DTO immutable to avoid accidental mutation.Use a record with init-only properties.
-public class PaginationDto +public record PaginationDto { - public int CurrentPage { get; set; } - public int TotalPages { get; set; } - public int TotalCount { get; set; } - public bool HasNext { get; set; } - public bool HasPrevious { get; set; } + public int CurrentPage { get; init; } + public int TotalPages { get; init; } + public int TotalCount { get; init; } + public bool HasNext { get; init; } + public bool HasPrevious { get; init; } }AskFm/AskFm.DAL/ModelsConfigrations/NotificationConfigration.cs (1)
5-8: Nit: fix typos in names.Rename folder and class to “ModelsConfigurations/NotificationConfiguration” for consistency.
No functional change—purely readability.
AskFm/AskFm.DAL/Migrations/AppDbContextModelSnapshot.cs (2)
299-301: Set a default for IsRead at the database levelTo avoid accidental NULLs or needing to always send a value, add a default of false in the model configuration.
Example in NotificationConfiguration:
builder.Property(n => n.IsRead) .HasDefaultValue(false);
309-311: Add a supporting index for common notification queriesIf you list notifications by user and unread-first/newest-first, add a composite index on (UserId, IsRead, CreatedAt DESC).
Follow-up migration:
migrationBuilder.CreateIndex( name: "IX_Notifications_UserId_IsRead_CreatedAt", table: "Notifications", columns: new[] { "UserId", "IsRead", "CreatedAt" });AskFm/AskFm.DAL/UnitOfWork.cs (1)
12-12: Clarify Thread type usage to avoid System.Threading.Thread confusionUsing Models.Thread is fine; consider an alias for readability across the class.
Example:
using ThreadEntity = AskFm.DAL.Models.Thread; // ... private IRepository<ThreadEntity> _threads; // ... _threads = new Repository<ThreadEntity>(_context);Also applies to: 38-47, 43-44
AskFm/AskFm.BLL/DTO/NotificationDto.cs (3)
1-1: Remove self-referential usingThe using AskFm.BLL.DTO is unnecessary here and can be dropped.
-using AskFm.BLL.DTO;
16-16: Pagination per-item is awkward; return a paged envelope insteadCarrying PaginationDto on each NotificationDto duplicates metadata n times. Prefer an envelope like PagedResult with Items + Pagination.
Example:
public sealed class PagedResult<T> { public required IReadOnlyList<T> Items { get; init; } public required PaginationDto Pagination { get; init; } }And update service signatures to return PagedResult.
6-7: Consider using the enum type for Type in the DTOIf clients can handle enum names, expose NotificationStatus (serialized as string) to avoid stringly-typed bugs.
Example:
public NotificationStatus Type { get; set; }And configure System.Text.Json to serialize enums as strings.
AskFm/AskFm.DAL/Models/Notification.cs (2)
1-1: Remove unused importSystem.Runtime.InteropServices.JavaScript is unrelated in DAL.
-using System.Runtime.InteropServices.JavaScript;
15-15: Initialize non-nullable MessagePrevent nullability warnings and ensure EF treats it as required.
- public string Message { get; set; } + public string Message { get; set; } = default!;AskFm/AskFm.BLL/Services/INotificationService.cs (3)
8-9: Return a paged envelope, not a bare ListExpose totalCount/page metadata once via a PagedResult.
-Task<List<NotificationDto>> GetUserNotifications(int userId, int pageNumber = 1, int pageSize = 10); -Task<List<NotificationDto>> GetNotificationsByType(int userId, string category, int pageNumber = 1, int pageSize = 10); +Task<PagedResult<NotificationDto>> GetUserNotifications(int userId, int pageNumber = 1, int pageSize = 10, CancellationToken ct = default); +Task<PagedResult<NotificationDto>> GetNotificationsByType(int userId, string category, int pageNumber = 1, int pageSize = 10, CancellationToken ct = default);(Adjust implementations and controller accordingly.)
8-13: Add CancellationToken parametersStandard for async service interfaces; bubbles cancellation from HTTP requests.
-Task<string> MarkNotificationAsRead(int notificationId, int userId); -Task<string> MarkAllNotificationsAsRead(int userId); -Task CreateNotification(int userId, NotificationStatus type, int resourceId, string message); +Task<string> MarkNotificationAsRead(int notificationId, int userId, CancellationToken ct = default); +Task<string> MarkAllNotificationsAsRead(int userId, CancellationToken ct = default); +Task CreateNotification(int userId, NotificationStatus type, int resourceId, string message, CancellationToken ct = default);
12-12: Consider returning the created notification (or Id) from CreateNotificationImproves ergonomics for callers and tests.
-Task CreateNotification(int userId, NotificationStatus type, int resourceId, string message); +Task<NotificationDto> CreateNotification(int userId, NotificationStatus type, int resourceId, string message, CancellationToken ct = default);AskFm/AskFm.DAL/Interfaces/INotificationRepository.cs (1)
8-11: Add CancellationToken and document orderingRepository methods should accept CancellationToken and define sort order (e.g., CreatedAt DESC).
-Task<(IEnumerable<Notification> notifications, int totalCount)> GetAllNotifications(int userId, int pageNumber, int pageSize); -Task<(IEnumerable<Notification> notifications, int totalCount)> GetNotificationsByType(int userId, NotificationStatus status, int pageNumber, int pageSize); -Task<ApplicationUser?> GetActorUserByResourceId(int resourceId, NotificationStatus type); -Task<Notification?> GetUserNotificationById(int notificationId, int userId); +Task<(IEnumerable<Notification> notifications, int totalCount)> GetAllNotifications(int userId, int pageNumber, int pageSize, CancellationToken ct = default); +Task<(IEnumerable<Notification> notifications, int totalCount)> GetNotificationsByType(int userId, NotificationStatus status, int pageNumber, int pageSize, CancellationToken ct = default); +Task<ApplicationUser?> GetActorUserByResourceId(int resourceId, NotificationStatus type, CancellationToken ct = default); +Task<Notification?> GetUserNotificationById(int notificationId, int userId, CancellationToken ct = default);AskFm/AskFm.BLL/Services/NotificationService.cs (7)
23-31: Validate pagination inputs and guard against division by zero.pageSize can be zero/negative and pageNumber < 1, leading to exceptions or odd paging. Clamp/sanitize before querying.
public async Task<List<NotificationDto>> GetUserNotifications(int userId, int pageNumber = 1, int pageSize = 10) { - var (notifications, totalCount) = await _notificationRepository.GetAllNotifications(userId, pageNumber, pageSize); - - var totalPages = (int)Math.Ceiling((double)totalCount / pageSize); + pageNumber = Math.Max(1, pageNumber); + pageSize = pageSize <= 0 ? 10 : Math.Min(pageSize, 100); + var (notifications, totalCount) = await _notificationRepository.GetAllNotifications(userId, pageNumber, pageSize); + var totalPages = totalCount == 0 ? 0 : (int)Math.Ceiling((double)totalCount / pageSize);
31-58: Avoid N+1 + serial awaits for actor resolution.You await per-item actor lookups in a loop. Batch with Task.WhenAll to cut latency.
- var notificationDtos = new List<NotificationDto>(); - - foreach (var notification in notifications) - { - var actorUser = await _notificationRepository.GetActorUserByResourceId(notification.ResourceId, notification.Type); - notificationDtos.Add(new NotificationDto + var actorTasks = notifications.Select(n => _notificationRepository.GetActorUserByResourceId(n.ResourceId, n.Type)).ToList(); + var actors = await Task.WhenAll(actorTasks); + var notificationDtos = new List<NotificationDto>(actors.Length); + var i = 0; + foreach (var notification in notifications) + { + var actorUser = actors[i++]; + notificationDtos.Add(new NotificationDto { Id = notification.Id, UserId = notification.UserId, Type = notification.Type.ToString(), ResourceId = notification.ResourceId, Message = notification.Message, IsRead = notification.IsRead, CreatedAt = notification.CreatedAt, - Actor = actorUser == null ? null : new ActorDto + Actor = actorUser == null ? null : new ActorDto { - Id = actorUser.Id, - Username = actorUser?.UserName ?? "Unknown", - AvatarPath = actorUser?.AvatarPath ?? String.Empty + Id = actorUser.Id, + Username = actorUser.UserName ?? "Unknown", + AvatarPath = actorUser.AvatarPath ?? string.Empty }, Pagination = new PaginationDto { CurrentPage = pageNumber, TotalPages = totalPages, TotalCount = totalCount, HasNext = pageNumber < totalPages, HasPrevious = pageNumber > 1 } }); }Apply the same pattern to GetNotificationsByType.
65-66: Case-insensitive enum parse without ToUpper and null-safety.Using ToUpper() can throw on null and is unnecessary. Use the ignoreCase overload and trim input.
- if (!Enum.TryParse<NotificationStatus>(category.ToUpper(), out var notificationType)) + if (string.IsNullOrWhiteSpace(category) || + !Enum.TryParse<NotificationStatus>(category.Trim(), ignoreCase: true, out var notificationType)) throw new ArgumentException($"Invalid notification category: {category}");
68-71: Repeat pagination validation here too.- var (notifications, totalCount) = await _notificationRepository.GetNotificationsByType(userId, notificationType, pageNumber, pageSize); - - var totalPages = (int)Math.Ceiling((double)totalCount / pageSize); + pageNumber = Math.Max(1, pageNumber); + pageSize = pageSize <= 0 ? 10 : Math.Min(pageSize, 100); + var (notifications, totalCount) = await _notificationRepository.GetNotificationsByType(userId, notificationType, pageNumber, pageSize); + var totalPages = totalCount == 0 ? 0 : (int)Math.Ceiling((double)totalCount / pageSize);
86-91: Nit: remove redundant null-conditionals inside a non-null branch.Inside the actorUser != null branch, operator ?. is redundant. Also prefer string.Empty consistently.
- Username = actorUser?.UserName ?? "Unknown", - AvatarPath = actorUser?.AvatarPath ?? String.Empty + Username = actorUser.UserName ?? "Unknown", + AvatarPath = actorUser.AvatarPath ?? string.Empty
111-116: Update UpdatedAt when marking a notification as read.Keeps metadata accurate for clients relying on sorting/recency.
notification.IsRead = true; + notification.UpdatedAt = DateTime.UtcNow; _unitOfWork.Notifications.Update(notification); await _unitOfWork.SaveAsync();
120-129: Use a single bulk update for “mark all as read”.Current approach loads all rows and updates one-by-one. Prefer a DB-side update (EF Core ExecuteUpdateAsync) for scale.
- var unreadNotifications = await _unitOfWork.Notifications.FindAllAsync(n => n.UserId == userId && !n.IsRead); - - foreach (var notification in unreadNotifications) - { - notification.IsRead = true; - _unitOfWork.Notifications.Update(notification); - } - - await _unitOfWork.SaveAsync(); + await _unitOfWork.Notifications + .Where(n => n.UserId == userId && !n.IsRead) + .ExecuteUpdateAsync(s => s + .SetProperty(n => n.IsRead, true) + .SetProperty(n => n.UpdatedAt, DateTime.UtcNow)); return "All notifications marked as read";If your repository abstraction doesn’t expose IQueryable/ExecuteUpdateAsync, add a repository method to perform this bulk update.
AskFm/AskFm.API/Program.cs (2)
50-50: Remove duplicate AddControllers registration.Called at Line 27 already.
- builder.Services.AddControllers();
53-61: Validate JWT env vars and parsing to fail fast with clear errors.int.Parse on missing/invalid envs will throw unhelpful exceptions; also SigningKey/Issuer/Audience can be null.
- JwtOptions jwtOptions = new JwtOptions - { - Issuer = Environment.GetEnvironmentVariable("ISSUER"), - Audience = Environment.GetEnvironmentVariable("AUDIENCE"), - SigningKey = Environment.GetEnvironmentVariable("SIGNINGKEY"), - AccessExpiration = int.Parse(Environment.GetEnvironmentVariable("TOKEN_EXP")), - AccessRefreshTokenExpiration = int.Parse(Environment.GetEnvironmentVariable("REFRESH_TOKEN_EXP")), - }; - if (jwtOptions == null) - { - throw new Exception("jwtOptions is null"); - } + var issuer = Environment.GetEnvironmentVariable("ISSUER"); + var audience = Environment.GetEnvironmentVariable("AUDIENCE"); + var signingKey = Environment.GetEnvironmentVariable("SIGNINGKEY"); + if (!int.TryParse(Environment.GetEnvironmentVariable("TOKEN_EXP"), out var accessExp) || + !int.TryParse(Environment.GetEnvironmentVariable("REFRESH_TOKEN_EXP"), out var refreshExp) || + string.IsNullOrWhiteSpace(issuer) || string.IsNullOrWhiteSpace(audience) || string.IsNullOrWhiteSpace(signingKey)) + { + throw new InvalidOperationException("JWT configuration is invalid or missing required environment variables."); + } + JwtOptions jwtOptions = new JwtOptions + { + Issuer = issuer, + Audience = audience, + SigningKey = signingKey, + AccessExpiration = accessExp, + AccessRefreshTokenExpiration = refreshExp, + };AskFm/AskFm.DAL/Repositories/NotificationRepository.cs (2)
23-27: Prefer AsNoTracking() on read-only paged queries.These lists are not modified; skip change tracking for lower overhead.
- var notifications = await query + var notifications = await query + .AsNoTracking() .OrderByDescending(n => n.CreatedAt) .Skip((pageNumber - 1) * pageSize) .Take(pageSize) .ToListAsync();
38-42: Apply AsNoTracking() here as well.- var notifications = await query + var notifications = await query + .AsNoTracking() .OrderByDescending(n => n.CreatedAt) .Skip((pageNumber - 1) * pageSize) .Take(pageSize) .ToListAsync();AskFm/AskFm.API/Controllers/NotificationController.cs (4)
98-100: Return 201 Created with location/payload for POST.Use CreatedAtAction to align with REST and help clients.
- return Ok(new { message = "Notification created successfully" }); + return CreatedAtAction(nameof(GetUserNotifications), null, new { message = "Notification created successfully" });
67-70: Use 401 Unauthorized instead of 403 Forbid for invalid/missing user claim.Forbid implies authenticated-but-not-allowed; this is an authn failure.
- return Forbid(ex.Message); + return Unauthorized(ex.Message);
118-124: Add request validation attributes to prevent bad data reaching the service/DB.Message is required in the model; validate early. Also constrain lengths.
public class CreateNotificationRequest { - public int UserId { get; set; } - public NotificationStatus Type { get; set; } - public int ResourceId { get; set; } - public string Message { get; set; } + [System.ComponentModel.DataAnnotations.Range(1, int.MaxValue)] + public int UserId { get; set; } + public NotificationStatus Type { get; set; } + [System.ComponentModel.DataAnnotations.Range(1, int.MaxValue)] + public int ResourceId { get; set; } + [System.ComponentModel.DataAnnotations.Required, System.ComponentModel.DataAnnotations.StringLength(500)] + public string Message { get; set; } }
21-33: Avoid returning raw exception messages to clients.Prefer ProblemDetails with a generic message to prevent information disclosure.
Example:
catch (Exception) { return Problem(title: "Failed to fetch notifications.", statusCode: StatusCodes.Status500InternalServerError); }Also applies to: 77-90
AskFm/Tests/NotificationServiceTests.cs (9)
15-21: Capture the hub group for assertions.Add a field to capture the SignalR group name used by the service so tests can assert correct targeting.
private readonly Mock<IClientProxy> _mockClientProxy; private readonly Mock<IGroupManager> _mockGroups; private readonly NotificationService _notificationService; + private string _capturedGroup = string.Empty;
32-36: Assert the message is sent to the expected group (userId).Capture the group name during setup and later assert it equals userId.ToString().
- _mockClients.Setup(p => p.Group(It.IsAny<string>())).Returns(_mockClientProxy.Object); + _mockClients + .Setup(p => p.Group(It.IsAny<string>())) + .Callback<string>(g => _capturedGroup = g) + .Returns(_mockClientProxy.Object);
289-291: Return a completed Task from AddAsync to avoid awaiting null.If NotificationService awaits AddAsync, this setup must return a completed task.
- _unitOfWorkMock.Setup(p => p.Notifications.AddAsync(It.IsAny<Notification>())); + _unitOfWorkMock + .Setup(p => p.Notifications.AddAsync(It.IsAny<Notification>())) + .Returns(Task.CompletedTask);
295-301: Assert the notification is sent to the correct group.After CreateNotification, assert the captured group equals the target user’s group.
await _notificationService.CreateNotification(userId, type, resourceId, message); // Assert + Assert.Equal(userId.ToString(), _capturedGroup); _unitOfWorkMock.Verify(p => p.Notifications.AddAsync(It.IsAny<Notification>()), Times.Once); _unitOfWorkMock.Verify(p => p.SaveAsync(), Times.Once);
289-296: Strengthen AddAsync verification by validating the entity contents.Ensure the created Notification has expected fields before persisting.
- _unitOfWorkMock - .Setup(p => p.Notifications.AddAsync(It.IsAny<Notification>())) - .Returns(Task.CompletedTask); + _unitOfWorkMock + .Setup(p => p.Notifications.AddAsync(It.Is<Notification>(n => + n.UserId == userId && + n.Type == type && + n.ResourceId == resourceId && + n.Message == message && + !n.IsRead))) + .Returns(Task.CompletedTask);
208-216: Also assert UpdatedAt is advanced on mark-as-read.Capture the previous UpdatedAt and assert it increases after the operation.
- // Act + var prevUpdatedAt = notification.UpdatedAt; + // Act var result = await _notificationService.MarkNotificationAsRead(notificationId, userId); // Assert Assert.True(notification.IsRead); + Assert.True(notification.UpdatedAt > prevUpdatedAt); Assert.Equal("notification has been read", result);
181-184: Avoid brittle checks on exception messages.Asserting on the exception type is sufficient; message text can change.
- var ex = await Assert.ThrowsAsync<ArgumentException>(() => _notificationService.GetNotificationsByType(userId, category, pageNumber, pageSize)); - Assert.Contains("Invalid notification category", ex.Message); + await Assert.ThrowsAsync<ArgumentException>(() => + _notificationService.GetNotificationsByType(userId, category, pageNumber, pageSize));
79-88: Pagination embedded per item is awkward; prefer a paged wrapper.Consider returning a PagedResult with Items + Pagination instead of duplicating pagination on each item. Tests would then assert the wrapper’s Pagination.TotalCount and item fields separately.
Also applies to: 119-127, 161-170
234-277: Add a mixed-state test to ensure only unread notifications are updated.Current test uses all-unread inputs. Add a case with a pre-read item to verify it remains unchanged.
Example test to add:
[Fact] public async Task MarkAllNotificationsAsRead_MixedStates_OnlyUnreadAreUpdated() { int userId = 1; var notifications = new List<Notification> { new() { Id = 1, UserId = userId, IsRead = false, Type = NotificationStatus.QUESTION, ResourceId = 100, CreatedAt = DateTime.UtcNow, UpdatedAt = DateTime.UtcNow }, new() { Id = 2, UserId = userId, IsRead = true, Type = NotificationStatus.ANSWER, ResourceId = 200, CreatedAt = DateTime.UtcNow, UpdatedAt = DateTime.UtcNow } }; _unitOfWorkMock.Setup(p => p.Notifications.FindAllAsync(It.IsAny<System.Linq.Expressions.Expression<System.Func<Notification, bool>>>(), null)) .ReturnsAsync(notifications); _unitOfWorkMock.Setup(p => p.SaveAsync()).ReturnsAsync(1); var result = await _notificationService.MarkAllNotificationsAsRead(userId); Assert.Equal("All notifications marked as read", result); Assert.True(notifications[0].IsRead); Assert.True(notifications[1].IsRead); // service may idempotently set true; verify Update called only for previously unread: _unitOfWorkMock.Verify(p => p.Notifications.Update(It.Is<Notification>(n => n.Id == 1)), Times.Once); _unitOfWorkMock.Verify(p => p.Notifications.Update(It.Is<Notification>(n => n.Id == 2)), Times.Never); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
AskFm/AskFm.API/AskFm.API.csproj(1 hunks)AskFm/AskFm.API/Controllers/NotificationController.cs(1 hunks)AskFm/AskFm.API/Program.cs(6 hunks)AskFm/AskFm.API/Properties/launchSettings.json(2 hunks)AskFm/AskFm.BLL/AskFm.BLL.csproj(1 hunks)AskFm/AskFm.BLL/DTO/ActorDto.cs(1 hunks)AskFm/AskFm.BLL/DTO/NotificationDto.cs(1 hunks)AskFm/AskFm.BLL/DTO/PaginationDto.cs(1 hunks)AskFm/AskFm.BLL/Hub/NotificationHub.cs(1 hunks)AskFm/AskFm.BLL/Services/INotificationService.cs(1 hunks)AskFm/AskFm.BLL/Services/NotificationService.cs(1 hunks)AskFm/AskFm.DAL/AskFm.DAL.csproj(1 hunks)AskFm/AskFm.DAL/Interfaces/INotificationRepository.cs(1 hunks)AskFm/AskFm.DAL/Interfaces/IUnitOfWork.cs(1 hunks)AskFm/AskFm.DAL/Migrations/20250824204202_NotificationModelNaming.Designer.cs(1 hunks)AskFm/AskFm.DAL/Migrations/20250824204202_NotificationModelNaming.cs(1 hunks)AskFm/AskFm.DAL/Migrations/AppDbContextModelSnapshot.cs(1 hunks)AskFm/AskFm.DAL/Models/Notification.cs(1 hunks)AskFm/AskFm.DAL/ModelsConfigrations/NotificationConfigration.cs(1 hunks)AskFm/AskFm.DAL/Repositories/NotificationRepository.cs(1 hunks)AskFm/AskFm.DAL/UnitOfWork.cs(3 hunks)AskFm/Tests/NotificationServiceTests.cs(1 hunks)AskFm/Tests/Tests.csproj(2 hunks)
🔇 Additional comments (10)
AskFm/AskFm.API/Properties/launchSettings.json (1)
7-8: Verify launchUrl matches the actual OpenAPI/Swagger UI route.If Program.cs uses only
app.MapOpenApi()(no UI),/swaggerwill 404; use/openapi/v1.json. If Scalar UI is enabled, use/scalar. If Swashbuckle UI is enabled,/swaggeris correct. Please confirm and align.Example diffs depending on what’s configured:
- "launchUrl": "swagger", + "launchUrl": "openapi/v1.json",- "launchUrl": "swagger", + "launchUrl": "scalar",AskFm/Tests/Tests.csproj (2)
13-13: LGTM: Moq addition is appropriate for hub/service mocking.No issues spotted.
22-24: LGTM: BLL reference for integration-style unit tests.This aligns with testing NotificationService behavior.
AskFm/AskFm.DAL/Interfaces/IUnitOfWork.cs (1)
8-8: LGTM: Fully-qualifying Thread resolves ambiguity with System.Threading.Thread.This avoids name collisions and clarifies intent.
AskFm/AskFm.DAL/AskFm.DAL.csproj (1)
22-22: Blocker: remove Microsoft.AspNetCore.SignalR from DAL (legacy, incompatible with net9.0)Microsoft.AspNetCore.SignalR 1.2.0 targets legacy ASP.NET Core and must not be referenced from the DAL layer.
Location: AskFm/AskFm.DAL/AskFm.DAL.csproj:22
- <PackageReference Include="Microsoft.AspNetCore.SignalR" Version="1.2.0"/>Verify no other projects reference this package (run from repo root):
rg -n --glob '*.csproj' 'Microsoft.AspNetCore.SignalR' || trueAskFm/AskFm.BLL/Hub/NotificationHub.cs (2)
19-28: LGTM: auto-join on connectAuto-joining the caller to their own group based on UserIdentifier is correct.
30-38: LGTM: cleanup on disconnectLeaving the group on disconnect is correct and avoids stale memberships.
AskFm/AskFm.DAL/Migrations/20250824204202_NotificationModelNaming.cs (1)
23-29: Confirm default mapping for Notifications.Type (0 → ANSWER)AskFm/AskFm.DAL/Enums/NotificationStatus.cs defines ANSWER as the first enum member (implicitly 0). The migration AddColumn adds a non-null Notifications.Type with defaultValue: 0 (AskFm/AskFm.DAL/Migrations/20250824204202_NotificationModelNaming.cs). If ANSWER is not the intended default, either add an explicit Unknown = 0 to the enum or change the migration's defaultValue to the intended enum numeric value.
AskFm/Tests/NotificationServiceTests.cs (2)
41-46: Service instantiation and dependency wiring look solid.Mocks are composed correctly; constructor DI path is exercised.
297-301: Verify hub method and loosen CancellationToken matcherUse It.IsAny() when verifying the hub call and confirm whether the service invokes SendCoreAsync or SendAsync; update the test to verify the exact method.
- Suggested change (if service uses SendCoreAsync):
- _mockClientProxy.Verify(p => p.SendCoreAsync("ReceiveNotification", It.IsAny<object[]>(), default), Times.Once); + _mockClientProxy.Verify( + p => p.SendCoreAsync("ReceiveNotification", It.IsAny<object[]>(), It.IsAny<CancellationToken>()), + Times.Once);
- If the service calls SendAsync, verify SendAsync(...) with It.IsAny() instead.
- To locate the call in the codebase run: rg -nP --type=cs -C2 'ReceiveNotification|.?(SendCoreAsync|SendAsync)\s*('
| <PackageReference Include="Microsoft.EntityFrameworkCore.Proxies" Version="9.0.0"/> | ||
| <PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="9.0.0"/> | ||
| <PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="9.0.0"/> | ||
| <PackageReference Include="Microsoft.AspNetCore.SignalR" Version="1.2.0"/> |
There was a problem hiding this comment.
🧩 Analysis chain
Blocker: Remove obsolete SignalR package; rely on ASP.NET Core shared framework.
Web SDK already brings ASP.NET Core; the 1.2.0 package is incompatible with net9.0.
Apply this diff:
- <PackageReference Include="Microsoft.AspNetCore.SignalR" Version="1.2.0"/>Confirm Program.cs uses MapHub/AddSignalR without extra package refs:
🏁 Script executed:
#!/bin/bash
rg -nP 'AddSignalR|MapHub<|MapOpenApi|UseSwaggerUI' AskFm/AskFm.API/Program.cs -C2Length of output: 525
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="AskFm/AskFm.API/AskFm.API.csproj"
if [ ! -f "$file" ]; then
echo "MISSING: $file"
rg -nP 'Microsoft\.AspNetCore\.SignalR' -S || true
exit 0
fi
echo "=== $file ==="
nl -ba "$file" | sed -n '1,240p'
echo
echo "=== PackageReference lines matching SignalR ==="
rg -nP '<PackageReference\b[^>]*Microsoft\.AspNetCore\.SignalR' -S "$file" || true
echo
echo "=== Project SDK / TargetFramework ==="
rg -nP '<Project\s+Sdk=' -n "$file" || true
rg -nP '<TargetFramework>' -n "$file" || trueLength of output: 82
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="AskFm/AskFm.API/AskFm.API.csproj"
if [ ! -f "$file" ]; then
echo "MISSING: $file"
rg -nP 'Microsoft\.AspNetCore\.SignalR' -S || true
exit 0
fi
echo "=== $file ==="
sed -n '1,240p' "$file"
echo
echo "=== PackageReference lines matching SignalR ==="
rg -nP '<PackageReference\b[^>]*Include="Microsoft\.AspNetCore\.SignalR"[^>]*>' -n "$file" || true
echo
echo "=== All PackageReference lines ==="
rg -nP '<PackageReference\b' -n "$file" || true
echo
echo "=== Project SDK / TargetFramework ==="
rg -nP '<Project\s+Sdk=' -n "$file" || true
rg -nP '<TargetFramework>' -n "$file" || trueLength of output: 2614
Remove Microsoft.AspNetCore.SignalR (v1.2.0) PackageReference from AskFm.API.csproj
Remove the PackageReference — this project targets net9.0 using Microsoft.NET.Sdk.Web and the SignalR server APIs (AddSignalR / MapHub) are provided by the ASP.NET Core shared framework; the explicit Microsoft.AspNetCore.SignalR v1.2.0 NuGet is unnecessary and can cause assembly/version conflicts. (learn.microsoft.com)
Apply this diff:
- <PackageReference Include="Microsoft.AspNetCore.SignalR" Version="1.2.0"/>Location: AskFm/AskFm.API/AskFm.API.csproj (line 19). Program.cs already uses builder.Services.AddSignalR(...) (~line 67) and app.MapHub("/notificationHub") (~line 193) — keep those calls and remove only the explicit PackageReference.
📝 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.
| <PackageReference Include="Microsoft.AspNetCore.SignalR" Version="1.2.0"/> |
🤖 Prompt for AI Agents
In AskFm/AskFm.API/AskFm.API.csproj around line 19, remove the explicit
PackageReference to Microsoft.AspNetCore.SignalR Version="1.2.0" because the
project targets net9.0 and ASP.NET Core's shared framework already provides
SignalR server APIs; keep the existing calls to builder.Services.AddSignalR(...)
and app.MapHub<NotificationHub>("/notificationHub") in Program.cs unchanged and
only delete that single PackageReference line to avoid assembly/version
conflicts.
| // CORS Configuration for SignalR | ||
| builder.Services.AddCors(options => | ||
| { | ||
| options.AddPolicy("SignalRPolicy", policy => | ||
| { | ||
| // Option 1: Allow any origin (for development only) | ||
| policy.AllowAnyOrigin() | ||
| .AllowAnyMethod() | ||
| .AllowAnyHeader(); | ||
|
|
||
| // Option 2: Specific origins (uncomment and modify when you know frontend URLs) | ||
| // policy.WithOrigins( | ||
| // "http://localhost:3000", // React default | ||
| // "http://localhost:4200", // Angular default | ||
| // "http://localhost:8080", // Vue default | ||
| // "http://localhost:5173", // Vite default | ||
| // "https://yourdomain.com" // Production domain | ||
| // ) | ||
| // .AllowAnyMethod() | ||
| // .AllowAnyHeader() | ||
| // .AllowCredentials(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Don’t ship AllowAnyOrigin; gate CORS by environment and allow credentials for known origins.
As-is, any site can open a WebSocket to your hub in production. Tighten CORS.
- builder.Services.AddCors(options =>
- {
- options.AddPolicy("SignalRPolicy", policy =>
- {
- // Option 1: Allow any origin (for development only)
- policy.AllowAnyOrigin()
- .AllowAnyMethod()
- .AllowAnyHeader();
+ builder.Services.AddCors(options =>
+ {
+ options.AddPolicy("SignalRPolicy", policy =>
+ {
+ if (builder.Environment.IsDevelopment())
+ {
+ policy.AllowAnyOrigin()
+ .AllowAnyMethod()
+ .AllowAnyHeader();
+ }
+ else
+ {
+ var origins = (Environment.GetEnvironmentVariable("FRONTEND_ORIGINS") ?? string.Empty)
+ .Split(';', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries);
+ policy.WithOrigins(origins)
+ .AllowAnyMethod()
+ .AllowAnyHeader()
+ .AllowCredentials();
+ }📝 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.
| // CORS Configuration for SignalR | |
| builder.Services.AddCors(options => | |
| { | |
| options.AddPolicy("SignalRPolicy", policy => | |
| { | |
| // Option 1: Allow any origin (for development only) | |
| policy.AllowAnyOrigin() | |
| .AllowAnyMethod() | |
| .AllowAnyHeader(); | |
| // Option 2: Specific origins (uncomment and modify when you know frontend URLs) | |
| // policy.WithOrigins( | |
| // "http://localhost:3000", // React default | |
| // "http://localhost:4200", // Angular default | |
| // "http://localhost:8080", // Vue default | |
| // "http://localhost:5173", // Vite default | |
| // "https://yourdomain.com" // Production domain | |
| // ) | |
| // .AllowAnyMethod() | |
| // .AllowAnyHeader() | |
| // .AllowCredentials(); | |
| }); | |
| }); | |
| // CORS Configuration for SignalR | |
| builder.Services.AddCors(options => | |
| { | |
| options.AddPolicy("SignalRPolicy", policy => | |
| { | |
| if (builder.Environment.IsDevelopment()) | |
| { | |
| policy.AllowAnyOrigin() | |
| .AllowAnyMethod() | |
| .AllowAnyHeader(); | |
| } | |
| else | |
| { | |
| var origins = (Environment.GetEnvironmentVariable("FRONTEND_ORIGINS") ?? string.Empty) | |
| .Split(';', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); | |
| policy.WithOrigins(origins) | |
| .AllowAnyMethod() | |
| .AllowAnyHeader() | |
| .AllowCredentials(); | |
| } | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In AskFm/AskFm.API/Program.cs around lines 75 to 97, the CORS policy currently
uses AllowAnyOrigin which is unsafe for production; update the policy to gate
behavior by environment: in non-production (Development) allow permissive CORS
for local dev, but in Production read a configured list of allowed origins
(e.g., from appsettings or an environment variable) and call
policy.WithOrigins(...).AllowAnyMethod().AllowAnyHeader().AllowCredentials();
ensure you remove AllowAnyOrigin in production, apply the "SignalRPolicy" to the
SignalR hub endpoints, and validate the origins list is parsed and empty-check
guarded to avoid accidentally falling back to AllowAnyOrigin.
| <PackageReference Include="Microsoft.AspNetCore.SignalR" Version="1.2.0"/> | ||
| <PackageReference Include="AutoMapper" Version="15.0.1"/> | ||
| <PackageReference Include="AutoMapper.Extensions.Microsoft.DependencyInjection" Version="12.0.1"/> | ||
| </ItemGroup> |
There was a problem hiding this comment.
🧩 Analysis chain
Blocker: Replace obsolete SignalR package with FrameworkReference for ASP.NET Core.
- Microsoft.AspNetCore.SignalR 1.2.0 is not compatible with net9.0.
- For IHubContext/Hub types in a class library, use the shared framework Reference.
Apply this diff:
<ItemGroup>
- <PackageReference Include="Microsoft.AspNetCore.SignalR" Version="1.2.0"/>
<PackageReference Include="AutoMapper" Version="15.0.1"/>
<PackageReference Include="AutoMapper.Extensions.Microsoft.DependencyInjection" Version="12.0.1"/>
</ItemGroup>
+ <ItemGroup>
+ <FrameworkReference Include="Microsoft.AspNetCore.App" />
+ </ItemGroup>Check that BLL compiles without the package after adding the FrameworkReference.
🏁 Script executed:
#!/bin/bash
rg -n 'IHubContext|Hub<' -g 'AskFm/AskFm.BLL/**.cs'Length of output: 0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== AskFm.BLL.csproj (first 300 lines) ==="
if [ -f AskFm/AskFm.BLL/AskFm.BLL.csproj ]; then
sed -n '1,300p' AskFm/AskFm.BLL/AskFm.BLL.csproj || true
else
echo "File not found: AskFm/AskFm.BLL/AskFm.BLL.csproj"
fi
echo
echo "=== Search for Microsoft.AspNetCore.SignalR package/reference in repo ==="
rg -n "Microsoft.AspNetCore.SignalR" -S || true
echo
echo "=== Search for IHubContext usage in AskFm.BLL (.cs files) ==="
rg -n --type cs '\bIHubContext\b' AskFm/AskFm.BLL -S -C3 || true
echo
echo "=== Search for Hub usage in AskFm.BLL (.cs files) ==="
rg -n --type cs '\bHub\b' AskFm/AskFm.BLL -S -C3 || true
echo
echo "=== Search for FrameworkReference occurrences in repo ==="
rg -n "FrameworkReference" -S || true
echo
echo "=== Show TargetFramework in AskFm.BLL.csproj ==="
rg -n "<TargetFramework" AskFm/AskFm.BLL/AskFm.BLL.csproj || trueLength of output: 3823
Blocker: Replace obsolete SignalR PackageReference with FrameworkReference (Microsoft.AspNetCore.App)
AskFm.BLL targets net9.0 and contains server SignalR types (AskFm/AskFm.BLL/Hub/NotificationHub.cs and AskFm/AskFm.BLL/Services/NotificationService.cs). Server-side SignalR assemblies are provided by the ASP.NET Core shared framework — remove the Microsoft.AspNetCore.SignalR package reference and add a FrameworkReference to Microsoft.AspNetCore.App. (github.com)
<ItemGroup>
- <PackageReference Include="Microsoft.AspNetCore.SignalR" Version="1.2.0"/>
<PackageReference Include="AutoMapper" Version="15.0.1"/>
<PackageReference Include="AutoMapper.Extensions.Microsoft.DependencyInjection" Version="12.0.1"/>
</ItemGroup>
+ <ItemGroup>
+ <FrameworkReference Include="Microsoft.AspNetCore.App" />
+ </ItemGroup>- Remove Microsoft.AspNetCore.SignalR from other projects that reference it (AskFm/AskFm.API/AskFm.API.csproj, AskFm/AskFm.DAL/AskFm.DAL.csproj, tests) unless a package-targeted scenario drives a different decision.
- Verify: dotnet build AskFm/AskFm.BLL/AskFm.BLL.csproj (or dotnet build AskFm.sln) and fix any remaining references.
📝 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.
| <PackageReference Include="Microsoft.AspNetCore.SignalR" Version="1.2.0"/> | |
| <PackageReference Include="AutoMapper" Version="15.0.1"/> | |
| <PackageReference Include="AutoMapper.Extensions.Microsoft.DependencyInjection" Version="12.0.1"/> | |
| </ItemGroup> | |
| <PackageReference Include="AutoMapper" Version="15.0.1"/> | |
| <PackageReference Include="AutoMapper.Extensions.Microsoft.DependencyInjection" Version="12.0.1"/> | |
| </ItemGroup> | |
| <ItemGroup> | |
| <FrameworkReference Include="Microsoft.AspNetCore.App" /> | |
| </ItemGroup> |
🤖 Prompt for AI Agents
In AskFm/AskFm.BLL/AskFm.BLL.csproj around lines 12 to 15, the project
references the obsolete Microsoft.AspNetCore.SignalR package while targeting
net9.0 and using server SignalR types; remove the PackageReference for
Microsoft.AspNetCore.SignalR and add a FrameworkReference to
Microsoft.AspNetCore.App instead, then remove the same package reference from
other projects (AskFm/AskFm.API/AskFm.API.csproj,
AskFm/AskFm.DAL/AskFm.DAL.csproj and test projects) unless they have a specific
package-targeted need, update any using/assembly references if necessary, and
run dotnet build AskFm/AskFm.BLL/AskFm.BLL.csproj (or dotnet build AskFm.sln) to
resolve and fix any remaining reference or compile errors.
| </ItemGroup> | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Layering concern: JwtBearer belongs in API, not BLL.
Authentication hosting concerns should live in the web layer. Consider removing from BLL and adding to AskFm.API if needed.
- <ItemGroup>
- <PackageReference Include="Microsoft.AspNetCore.Authentication.JwtBearer" Version="9.0.0" />
- </ItemGroup>
+ <!-- Consider moving JwtBearer to AskFm.API -->Verify if any BLL files directly use JwtBearer types.
🏁 Script executed:
#!/bin/bash
rg -nP --type cs 'Microsoft\.AspNetCore\.Authentication\.JwtBearer' AskFm/AskFm.BLLLength of output: 0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Search for PackageReference to JwtBearer in repository (csproj files)"
rg -n --hidden -S "<PackageReference[^>]*Include=[\"']Microsoft\.AspNetCore\.Authentication\.JwtBearer[\"']" || true
echo
echo "2) Search for JwtBearer usages (AddJwtBearer, JwtBearerDefaults, JwtBearerOptions, using ...JwtBearer, JwtBearer types) in AskFm source"
rg -nP -S -C2 '(AddJwtBearer|JwtBearerDefaults|JwtBearerOptions|using\s+Microsoft\.AspNetCore\.Authentication\.JwtBearer|Microsoft\.AspNetCore\.Authentication\.JwtBearer|JwtBearer)' AskFm || true
echo
echo "3) Show AskFm/AskFm.BLL/AskFm.BLL.csproj (if present)"
if [ -f "AskFm/AskFm.BLL/AskFm.BLL.csproj" ]; then
sed -n '1,240p' AskFm/AskFm.BLL/AskFm.BLL.csproj
else
echo "AskFm/AskFm.BLL/AskFm.BLL.csproj not found"
fi
echo
echo "4) Show AskFm/AskFm.API csproj (if present) for comparison"
if [ -f "AskFm/AskFm.API/AskFm.API.csproj" ]; then
sed -n '1,240p' AskFm/AskFm.API/AskFm.API.csproj
else
echo "AskFm/AskFm.API/AskFm.API.csproj not found"
fiLength of output: 4144
Move JwtBearer package reference from AskFm.BLL to AskFm.API
AskFm/AskFm.BLL/AskFm.BLL.csproj contains the PackageReference (line 17) but JwtBearer types are only used in AskFm/AskFm.API/Program.cs — remove the reference from AskFm.BLL and add it to AskFm/AskFm.API.csproj.
- <ItemGroup>
- <PackageReference Include="Microsoft.AspNetCore.Authentication.JwtBearer" Version="9.0.0" />
- </ItemGroup>
+ <!-- Move JwtBearer to AskFm.API -->Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In AskFm/AskFm.BLL/AskFm.BLL.csproj around lines 17-18, there is a
PackageReference to Microsoft.AspNetCore.Authentication.JwtBearer which is
unused in the BLL project; remove that PackageReference from AskFm.BLL.csproj
and add the same PackageReference (matching Version="9.0.0") into
AskFm/AskFm.API/AskFm.API.csproj (or the API project file where Program.cs
lives) so the JwtBearer package is referenced only in the API project that
actually uses those types; update project files and run a restore/build to
verify no missing references.
| public class NotificationDto | ||
| { | ||
| public int Id { get; set; } | ||
| public string Type { get; set; } | ||
|
|
||
| public string Message { get; set; } | ||
|
|
||
| public bool IsRead { get; set; } | ||
| public DateTime CreatedAt { get; set; } | ||
|
|
||
| public int ResourceId { get; set; } | ||
| public int UserId { get; set; } | ||
| public ActorDto? Actor { get; set; } | ||
| public PaginationDto Pagination { get; set; } | ||
|
|
||
|
|
||
| } |
There was a problem hiding this comment.
Missing namespace; class is in the global namespace
This will break consumers expecting AskFm.BLL.DTO.NotificationDto.
Apply:
-public class NotificationDto
-{
+namespace AskFm.BLL.DTO;
+
+public class NotificationDto
+{Also initialize non-nullable strings to satisfy nullability:
- public string Type { get; set; }
+ public string Type { get; set; } = default!;
- public string Message { get; set; }
+ public string Message { get; set; } = default!;📝 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.
| public class NotificationDto | |
| { | |
| public int Id { get; set; } | |
| public string Type { get; set; } | |
| public string Message { get; set; } | |
| public bool IsRead { get; set; } | |
| public DateTime CreatedAt { get; set; } | |
| public int ResourceId { get; set; } | |
| public int UserId { get; set; } | |
| public ActorDto? Actor { get; set; } | |
| public PaginationDto Pagination { get; set; } | |
| } | |
| namespace AskFm.BLL.DTO; | |
| public class NotificationDto | |
| { | |
| public int Id { get; set; } | |
| public string Type { get; set; } = default!; | |
| public string Message { get; set; } = default!; | |
| public bool IsRead { get; set; } | |
| public DateTime CreatedAt { get; set; } | |
| public int ResourceId { get; set; } | |
| public int UserId { get; set; } | |
| public ActorDto? Actor { get; set; } | |
| public PaginationDto Pagination { get; set; } | |
| } |
🤖 Prompt for AI Agents
In AskFm/AskFm.BLL/DTO/NotificationDto.cs around lines 3 to 19, the class is
declared in the global namespace and non-nullable string properties are not
initialized; move the class into the AskFm.BLL.DTO namespace and initialize
non-nullable reference properties (at minimum Type and Message) with safe
defaults (e.g. empty strings). Also ensure any other non-nullable reference
properties (like Pagination) are either made nullable or given sensible default
instances so the DTO honors nullability annotations.
| b.Property<string>("Message") | ||
| .IsRequired() | ||
| .HasColumnType("NVARCHAR"); | ||
|
|
||
| b.Property<int>("ResourceId") |
There was a problem hiding this comment.
Invalid SQL type: NVARCHAR requires a length (e.g., nvarchar(500) or nvarchar(max)). Migration will fail.
Configure Message with a max length or nvarchar(max) in the model and re-scaffold the migration; don’t hand-edit the designer.
Proposed model configuration (Fluent API):
builder.Entity<Notification>()
.Property(n => n.Message)
.IsRequired()
.HasMaxLength(500); // or .HasColumnType("nvarchar(max)")Then re-generate the migration to emit nvarchar(500) or nvarchar(max).
🤖 Prompt for AI Agents
In AskFm/AskFm.DAL/Migrations/20250824204202_NotificationModelNaming.Designer.cs
around lines 275 to 279, the migration uses HasColumnType("NVARCHAR") which is
invalid because NVARCHAR requires a length; fix by updating the Notification
entity configuration in the model (not the designer) to specify a length or
nvarchar(max) (e.g., configure Message as IsRequired() and either
HasMaxLength(500) or HasColumnType("nvarchar(max)")), then
re-scaffold/re-generate the migration so the designer emits nvarchar(500) or
nvarchar(max) instead of the invalid NVARCHAR; do not hand-edit the designer
file.
| b.Property<string>("Message") | ||
| .IsRequired() | ||
| .HasColumnType("NVARCHAR"); | ||
|
|
There was a problem hiding this comment.
Invalid SQL type for Message: use nvarchar(max) or a bounded length
Column type is set to "NVARCHAR" without length, which is invalid on SQL Server. Prefer a bounded length or nvarchar(max) via model configuration so future migrations/snapshots are consistent.
Suggested fix in NotificationConfiguration (not this snapshot):
builder.Property(n => n.Message)
.IsRequired()
.HasMaxLength(1000); // lets EF emit nvarchar(1000)
// or explicitly:
// .HasColumnType("nvarchar(max)");🤖 Prompt for AI Agents
In AskFm/AskFm.DAL/Migrations/AppDbContextModelSnapshot.cs around lines 302 to
305, the snapshot shows
b.Property<string>("Message").IsRequired().HasColumnType("NVARCHAR"); which uses
an invalid/unspecified NVARCHAR SQL type; update the entity configuration
(NotificationConfiguration) instead of editing the snapshot: set a bounded
length via .HasMaxLength(1000) (or another appropriate length) so EF emits
nvarchar(1000), or explicitly set .HasColumnType("nvarchar(max)"); then
regenerate the migration/snapshot so the snapshot reflects the proper SQL type.
| builder.Property(n => n.Message) | ||
| .HasColumnType("NVARCHAR"); | ||
|
|
||
| builder.Property(n => n.isRead) | ||
| builder.Property(n => n.IsRead) | ||
| .IsRequired(); |
There was a problem hiding this comment.
🧩 Analysis chain
Blocker: NVARCHAR without length is invalid SQL Server type.
Specify length or use nvarchar(max); otherwise migrations will generate an invalid column type.
Apply this diff:
- builder.Property(n => n.Message)
- .HasColumnType("NVARCHAR");
+ builder.Property(n => n.Message)
+ .HasColumnType("nvarchar(max)");Optionally add an index for common queries:
+ builder.HasIndex(n => new { n.UserId, n.IsRead });Search for queries filtering by UserId/IsRead to justify the index:
🏁 Script executed:
#!/bin/bash
rg -nP --type cs 'Notifications?\.(Where|Any|Count)\(.*UserId|IsRead' -C2Length of output: 12751
Blocker: NVARCHAR without length — set Message to nvarchar(max).
File: AskFm/AskFm.DAL/ModelsConfigrations/NotificationConfigration.cs (lines 13–17)
- builder.Property(n => n.Message)
- .HasColumnType("NVARCHAR");
+ builder.Property(n => n.Message)
+ .HasColumnType("nvarchar(max)");Optional: add index for common queries (justified by FindAllAsync(n => n.UserId == userId && !n.IsRead) in AskFm/AskFm.BLL/Services/NotificationService.cs:120):
+ builder.HasIndex(n => new { n.UserId, n.IsRead });📝 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.
| builder.Property(n => n.Message) | |
| .HasColumnType("NVARCHAR"); | |
| builder.Property(n => n.isRead) | |
| builder.Property(n => n.IsRead) | |
| .IsRequired(); | |
| builder.Property(n => n.Message) | |
| .HasColumnType("nvarchar(max)"); | |
| builder.Property(n => n.IsRead) | |
| .IsRequired(); | |
| builder.HasIndex(n => new { n.UserId, n.IsRead }); |
🤖 Prompt for AI Agents
In AskFm/AskFm.DAL/ModelsConfigrations/NotificationConfigration.cs around lines
13-17, the Message property is configured as NVARCHAR without a length which
causes EF/Core to generate an invalid column type; change the configuration to
use nvarchar(max) explicitly (e.g. HasColumnType("NVARCHAR(MAX)")) so large
messages are supported and migrations are valid, and optionally add a DB index
on UserId and IsRead (composite index) to optimize queries like FindAllAsync(n
=> n.UserId == userId && !n.IsRead).
| public async Task<ApplicationUser?> GetActorUserByResourceId(int resourceId, NotificationStatus type) | ||
| { | ||
| if (type == NotificationStatus.FOLLOW) | ||
| { | ||
| var follow = await _unitOfWork.Follows.FindAsync(f => f.FollowedId == resourceId, new[] { "Follower" }); | ||
| return follow?.Follower; | ||
| } | ||
| else if (type == NotificationStatus.QUESTION) | ||
| { | ||
| var thread = await _unitOfWork.Threads.FindAsync(t => t.Id == resourceId, new[] { "Asker" }); | ||
| return thread?.Asker; | ||
| } | ||
| else if (type == NotificationStatus.ANSWER) | ||
| { | ||
| var thread = await _unitOfWork.Threads.FindAsync(t => t.Id == resourceId, new[] { "Asked" }); | ||
| return thread?.Asked; | ||
| } | ||
| else if (type == NotificationStatus.COMMENT_LIKE) | ||
| { | ||
| var commentLike = await _unitOfWork.CommentLikes.FindAsync(cl => cl.CommentId == resourceId, new[] { "User" }); | ||
| return commentLike?.User; | ||
| } | ||
| else if (type == NotificationStatus.QUESTION_LIKE) | ||
| { | ||
| var threadLike = await _unitOfWork.ThreadLikes.FindAsync(tl => tl.ThreadId == resourceId, new[] { "User" }); | ||
| return threadLike?.User; | ||
| } | ||
| else if (type == NotificationStatus.REPLAY) | ||
| { | ||
| var comment = await _unitOfWork.Comments.FindAsync(c => c.Id == resourceId, new[] { "User" }); | ||
| return comment?.User; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
Actor resolution is ambiguous for FOLLOW and LIKE events — wrong user may be returned.
Using only resourceId (e.g., CommentId/ThreadId/FollowedId) can match many rows; FindAsync will return an arbitrary record, not the actual actor. Persist ActorId on Notification and resolve by that, or include a unique composite key in resource metadata.
Proposed direction:
- Update Notification model to include ActorId (int, FK to ApplicationUser).
- Change CreateNotification(...) signature to accept actorId and persist it.
- Rewrite GetActorUserByResourceId(...) to GetActorUserAsync(int actorId) and drop resource lookups for FOLLOW/LIKE.
I can draft the model/migration/service/controller changes if you want.
🤖 Prompt for AI Agents
In AskFm/AskFm.DAL/Repositories/NotificationRepository.cs around lines 47 to 79,
the current GetActorUserByResourceId implementation resolves actor users by
querying resources (Follow/Thread/Comment/Like) using only a resourceId, which
can return arbitrary rows; change the design to use an explicit ActorId stored
on the Notification and stop inferring the actor from resource lookups: add an
ActorId (int FK) to the Notification model and migration, update any
CreateNotification signatures/usage to accept and persist actorId, and replace
GetActorUserByResourceId with a simple GetActorUserAsync(int actorId) that loads
and returns the ApplicationUser by that FK (remove FOLLOW/LIKE resource-based
queries).
| } | ||
| } | ||
|
|
||
| IRepository<Models.Thread> IUnitOfWork.Threads => throw new NotImplementedException(); |
There was a problem hiding this comment.
Do not throw from the interface implementation of Threads
This breaks consumers typed as IUnitOfWork and is a runtime landmine.
Apply one of the following:
-IRepository<Models.Thread> IUnitOfWork.Threads => throw new NotImplementedException();
+IRepository<Models.Thread> IUnitOfWork.Threads => Threads;Or simply remove the explicit implementation if the public property already satisfies the interface.
📝 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.
| IRepository<Models.Thread> IUnitOfWork.Threads => throw new NotImplementedException(); | |
| IRepository<Models.Thread> IUnitOfWork.Threads => Threads; |
🤖 Prompt for AI Agents
In AskFm/AskFm.DAL/UnitOfWork.cs around line 121 the explicit interface
implementation IRepository<Models.Thread> IUnitOfWork.Threads currently throws
NotImplementedException which causes a runtime failure for consumers using
IUnitOfWork; replace this throwing implementation with a proper getter that
returns the concrete/backing Threads repository (or delegate to the existing
public Threads property), or if the class already exposes a public Threads
property that matches the interface, remove the explicit implementation so the
public property satisfies IUnitOfWork; ensure the implementation returns the
repository instance and does not throw.
2 participants
Notification System Implementation
What's Implemented
This branch (
mahmoud-notif-patch) implements a complete notification system for the AskFm clone with the following features:Core Features
Notification Types Supported
ANSWER- When someone answers your questionQUESTION- When someone asks you a questionFOLLOW- When someone follows youCOMMENT_LIKE- When someone likes your commentQUESTION_LIKE- When someone likes your questionREPLAY- When someone replies to your commentTechnical Implementation
API Endpoints
Real-time Features
/notificationHubImplementation Phases
Phase 1: Foundation Setup
888cfb3- Add SignalR Packagead90d92- Add Notification Hub47f9e2b- Implement NotificationDtoPhase 2: Data Layer Implementation
fe6259a- Implement Notification Repository6c8330e- Fix notification model and repository for API compliance0f1a29c- Implement Notification ServicesPhase 3: Service Layer & API
e65c410- Fix NotificationDto and implement manual mapping7432403- Implement Notification Controller9125016- Implement notification hub065fdac- Add Dependency injectionPhase 4: Architecture Refinement
b2dca6d- Remove redundant actor fields and resolve actors via ResourceIdfda6755- Remove NotificationResponseType (frontend optimization)bf3cefd- Refactor to use UnitOfWork and Repository patternd7bd9cf- Update Naming in Notification model and apply migrations for it3e53d29- Enable SwaggerPhase 5: Testing & Service Integration
Testing Coverage
The notification system includes comprehensive unit tests covering:
Integration Notes
This notification system is fully integrated with:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores