fix(perf): Add CreateTransmissionCommand, refactor common transmission plumbing #3162
Conversation
…o common services
📝 WalkthroughWalkthroughAdds a CreateTransmission feature (command, validator, handler, data loader), a transmission appender and hierarchy validator with repository and DI wiring, updates create/update endpoint/handlers and tests, adjusts project InternalsVisibleTo, and edits AGENTS.md guidance and code-style notes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
elsand
changed the title
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
AGENTS.md (1)
44-47: Contradictory guidance on lines 45-46.Lines 45 and 46 provide conflicting advice:
- Line 45: "Prefer switch expressions over statement-based
switch."- Line 46: "Prefer
if/elsechains over statement-basedswitch."This creates ambiguity. Based on the coding guidelines, line 46 should likely read "Prefer
ifwith early returns overif/elsechains" instead.- Prefer pattern matching (`switch` expressions, property patterns) over `if/else` chains. - Prefer switch expressions over statement-based `switch`. -- Prefer `if/else` chains over statement-based `switch`. +- Prefer `if` with early returns over `if/else` chains. - Prefer records or record structs for immutable data models.
🧹 Nitpick comments (8)
src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Extensions/TransmissionEntityExtensions.cs (1)
10-28: Consider LINQ alternative for consistency with coding guidelines.Per guidelines, LINQ is preferred over manual loops for transformations. A single-enumeration LINQ approach:
- internal static (int FromParty, int FromServiceOwner) GetTransmissionCounts(this IEnumerable<DialogTransmission> transmissions) - { - var fromParty = 0; - var fromServiceOwner = 0; - - foreach (var transmission in transmissions) - { - if (IsEndUserTransmissionType(transmission.TypeId)) - { - fromParty++; - } - else - { - fromServiceOwner++; - } - } - - return (fromParty, fromServiceOwner); - } + internal static (int FromParty, int FromServiceOwner) GetTransmissionCounts(this IEnumerable<DialogTransmission> transmissions) + { + var list = transmissions as IReadOnlyCollection<DialogTransmission> ?? transmissions.ToList(); + var fromParty = list.Count(x => IsEndUserTransmissionType(x.TypeId)); + return (fromParty, list.Count - fromParty); + }That said, your current implementation is efficient with a single O(n) pass. If performance is critical here, the imperative approach is acceptable.
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs (1)
56-92: Consider consolidating near-duplicate tests.Both
CreateDialogCommand_Should_Return_Forbidden_When_Scope_Is_MissingandCreateDialogCommand_Should_Return_Forbidden_When_User_Is_Not_Ownerhave identical arrange/act/assert logic. The test names suggest different scenarios, but both returnnew Forbidden()fromAuthorizeServiceResourcesand assertresult.IsT3.If they're meant to test distinct scenarios, consider differentiating the setup or assertions. Otherwise, one test may be redundant.
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateDialogTransmissionEndpoint.cs (1)
55-57: Consider defensive check for empty collection.
First()will throw ifTransmissionIdsis unexpectedly empty. Given this endpoint always sends exactly one transmission, the success path should guarantee one ID is returned, but consider usingSingle()to make the "exactly one" expectation explicit and catch bugs if the contract changes.- var transmissionId = success.TransmissionIds.First(); + var transmissionId = success.TransmissionIds.Single();src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/DialogTransmissionAppender.cs (1)
63-76: Error message lacks transmission/attachment index context.When validation fails, the error path doesn't indicate which transmission or attachment has the invalid
ExpiresAt. Consider including indices in the property path to help identify the specific offending item.- private void ValidateTimeFields(DialogTransmissionAttachment attachment) + private void ValidateTimeFields(DialogTransmissionAttachment attachment, int transmissionIndex, int attachmentIndex) { if (!_db.MustWhenAdded(attachment, propertyExpression: x => x.ExpiresAt, predicate: x => x > _clock.UtcNowOffset || x == null)) { var idString = attachment.Id == Guid.Empty ? string.Empty : $" (Id: {attachment.Id})"; - _domainContext.AddError($"{nameof(DialogEntity.Transmissions)}." + + _domainContext.AddError($"{nameof(DialogEntity.Transmissions)}[{transmissionIndex}]." + - $"{nameof(DialogTransmission.Attachments)}." + + $"{nameof(DialogTransmission.Attachments)}[{attachmentIndex}]." + $"{nameof(DialogTransmissionAttachment.ExpiresAt)}", $"Must be in future or null, got '{attachment.ExpiresAt}'.{idString}"); } }Update the call site in
Appendto pass indices:- foreach (var attachment in newTransmissions.SelectMany(x => x.Attachments)) + foreach (var (transmission, transmissionIndex) in newTransmissions.Select((t, i) => (t, i))) + foreach (var (attachment, attachmentIndex) in transmission.Attachments.Select((a, i) => (a, i))) { - ValidateTimeFields(attachment); + ValidateTimeFields(attachment, transmissionIndex, attachmentIndex); }src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cs (2)
29-35: Consider usingArgumentNullException.ThrowIfNull()for public API guards.Per coding guidelines, prefer
ArgumentNullException.ThrowIfNull()for public/internal API guards. This is more concise and consistent with modern .NET patterns.public TransmissionHierarchyValidator( IDialogDbContext dbContext, IDomainContext domainContext) { - _dbContext = dbContext ?? throw new ArgumentNullException(nameof(dbContext)); - _domainContext = domainContext ?? throw new ArgumentNullException(nameof(domainContext)); + ArgumentNullException.ThrowIfNull(dbContext); + ArgumentNullException.ThrowIfNull(domainContext); + _dbContext = dbContext; + _domainContext = domainContext; }
148-151: Prefer collection expression for empty list.Per coding guidelines, prefer collection expressions (
[]) instead ofnew List<T>().if (parentIds.Count == 0) { - return Task.FromResult(new List<TransmissionHierarchyNode>()); + return Task.FromResult<List<TransmissionHierarchyNode>>([]); }src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs (2)
21-21: Type alias references DTO from another command namespace.The
TransmissionDtois aliased fromUpdatenamespace. Consider whether a shared DTO type in theCommonnamespace would be more appropriate if this DTO is reused across multiple commands (Create, Update, CreateTransmission).
146-159: DuplicatedAddSystemLabelmethod.This method is identical to the one in
UpdateDialogCommandHandler. Consider extracting it to a shared helper or service in theCommonnamespace to reduce duplication across command handlers.#!/bin/bash # Find all occurrences of AddSystemLabel to assess duplication scope rg -n "AddSystemLabel" --type cs src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
AGENTS.md(1 hunks)src/Digdir.Domain.Dialogporten.Application/ApplicationExtensions.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Digdir.Domain.Dialogporten.Application.csproj(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Extensions/TransmissionEntityExtensions.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs(5 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommandValidator.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionDataLoader.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs(7 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogDataLoader.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/DialogTransmissionAppender.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateDialogTransmissionEndpoint.cs(2 hunks)tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.cs: All code must compile withTreatWarningsAsErrors=trueand pass .NET analyzers
Use file-scoped namespaces withusingdirectives outside the namespace
Use PascalCase for classes and methods, camelCase for variables and parameters
Prefer expression bodies for single-line members and usevarwhen the type is apparent
Enable nullable reference types and keep entities immutable; use OneOf for union returns when applicable
Prefer LINQ over manual loops for transformations, filtering, grouping, and projections
PreferFirstOrDefault()/SingleOrDefault()with explicit null handling over index-based access
UseSelect,Where,GroupBy,ToDictionary,ToLookupinstead of building collections imperatively
Avoid materializing collections early withToListorToArrayunless required for correctness or performance
Prefer immutable LINQ pipelines over mutating existing collections
Target latest language features available in the solution (.NET 9 / C# 13); prefer pattern matching, switch expressions, records, collection expressions, target-typednew(), andwithexpressions
Prefer pattern matching (switchexpressions, property patterns) overif/elsechains
Prefer switch expressions over statement-basedswitch
Preferifwith early returns overif/elsechains
Prefer records or record structs for immutable data models
Use collection expressions ([]) instead ofnew List<T>()ornew[] { }where applicable
Prefer target-typednew()when the type is obvious
Preferwithexpressions for non-destructive mutation
Do not suppress nullable warnings (!) unless strictly justified
Prefer pattern matching (is not null) over!= nullfor null checking
Prefer early returns over nested null checks
UseArgumentNullException.ThrowIfNull()for public API guards
Files:
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cssrc/Digdir.Domain.Dialogporten.Application/ApplicationExtensions.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Extensions/TransmissionEntityExtensions.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/DialogTransmissionAppender.cssrc/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateDialogTransmissionEndpoint.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommandValidator.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogDataLoader.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionDataLoader.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs
**/*.{cs,json,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Use 4-space indentation for C# and 2-space indentation for JSON/YAML with LF line endings
Files:
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cssrc/Digdir.Domain.Dialogporten.Application/ApplicationExtensions.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Extensions/TransmissionEntityExtensions.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/DialogTransmissionAppender.cssrc/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateDialogTransmissionEndpoint.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommandValidator.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogDataLoader.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionDataLoader.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs
src/**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.cs: Organize code by feature folders following Clean Architecture with Domain/Application/Infrastructure/API layers
Apply CQRS, domain events, repository pattern, and use rich domain result objects
Avoid throwing exceptions for domain flow; return domain-specific result objects instead
Files:
src/Digdir.Domain.Dialogporten.Application/ApplicationExtensions.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Extensions/TransmissionEntityExtensions.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/DialogTransmissionAppender.cssrc/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateDialogTransmissionEndpoint.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommandValidator.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogDataLoader.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionDataLoader.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs
🧠 Learnings (10)
📚 Learning: 2025-12-15T13:34:26.796Z
Learnt from: CR
Repo: Altinn/dialogporten PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-15T13:34:26.796Z
Learning: Run tests with `dotnet test Digdir.Domain.Dialogporten.sln` after making changes; skip integration tests in non-Docker environments with `--filter 'FullyQualifiedName!~Integration'`
Applied to files:
AGENTS.mdsrc/Digdir.Domain.Dialogporten.Application/Digdir.Domain.Dialogporten.Application.csprojtests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cssrc/Digdir.Domain.Dialogporten.Application/ApplicationExtensions.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs
📚 Learning: 2025-12-15T13:34:26.796Z
Learnt from: CR
Repo: Altinn/dialogporten PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-15T13:34:26.796Z
Learning: Build with `dotnet build Digdir.Domain.Dialogporten.sln`
Applied to files:
AGENTS.mdsrc/Digdir.Domain.Dialogporten.Application/Digdir.Domain.Dialogporten.Application.csprojtests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cssrc/Digdir.Domain.Dialogporten.Application/ApplicationExtensions.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs
📚 Learning: 2025-12-15T13:34:26.796Z
Learnt from: CR
Repo: Altinn/dialogporten PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-15T13:34:26.796Z
Learning: Applies to **/*.Tests.cs : Tests use xUnit with the fixture pattern and Verify for snapshot tests
Applied to files:
AGENTS.md
📚 Learning: 2025-10-09T08:50:14.740Z
Learnt from: oskogstad
Repo: Altinn/dialogporten PR: 2841
File: src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20251009083408_AddDialogSearch.cs:9-10
Timestamp: 2025-10-09T08:50:14.740Z
Learning: Files in `src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/*/**` are auto-generated Entity Framework Core migrations and should not be reviewed for formatting or coding style issues.
Applied to files:
AGENTS.mdsrc/Digdir.Domain.Dialogporten.Application/Digdir.Domain.Dialogporten.Application.csprojsrc/Digdir.Domain.Dialogporten.Application/ApplicationExtensions.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogDataLoader.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionDataLoader.cs
📚 Learning: 2025-12-15T13:34:26.796Z
Learnt from: CR
Repo: Altinn/dialogporten PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-15T13:34:26.796Z
Learning: Applies to **/*.cs : All code must compile with `TreatWarningsAsErrors=true` and pass .NET analyzers
Applied to files:
AGENTS.md
📚 Learning: 2025-10-09T10:20:37.692Z
Learnt from: oskogstad
Repo: Altinn/dialogporten PR: 2844
File: src/Digdir.Domain.Dialogporten.GraphQL/Program.cs:104-104
Timestamp: 2025-10-09T10:20:37.692Z
Learning: For the Altinn/dialogporten repository, do not flag compilation errors such as missing NuGet package references, as the team has build pipelines that catch these issues automatically.
Applied to files:
AGENTS.md
📚 Learning: 2025-12-15T13:34:26.796Z
Learnt from: CR
Repo: Altinn/dialogporten PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-15T13:34:26.796Z
Learning: Applies to **/*.cs : Use file-scoped namespaces with `using` directives outside the namespace
Applied to files:
AGENTS.md
📚 Learning: 2025-10-15T17:12:11.560Z
Learnt from: knuhau
Repo: Altinn/dialogporten PR: 2872
File: src/Digdir.Domain.Dialogporten.Janitor/CostManagementAggregation/MetricsAggregationOptions.cs:15-23
Timestamp: 2025-10-15T17:12:11.560Z
Learning: In the Dialogporten Janitor application (src/Digdir.Domain.Dialogporten.Janitor), prefer runtime validation over startup validation (e.g., [Required] attributes) for configuration values that are only needed by specific commands. This prevents the application from failing to start when running other commands that don't require those configuration values.
Applied to files:
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cssrc/Digdir.Domain.Dialogporten.Application/ApplicationExtensions.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommandValidator.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs
📚 Learning: 2025-12-12T13:51:39.643Z
Learnt from: elsand
Repo: Altinn/dialogporten PR: 3132
File: src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/NotificationCondition/NotificationConditionQuery.cs:40-55
Timestamp: 2025-12-12T13:51:39.643Z
Learning: In src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/NotificationCondition/NotificationConditionQuery.cs, the soft-deletion status of dialogs is intentionally not checked. The EntityDeleted case in the NotificationConditionResult union is unreachable by design, as whether a dialog is soft-deleted is not relevant for determining whether a notification should be sent.
Applied to files:
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cssrc/Digdir.Domain.Dialogporten.Application/ApplicationExtensions.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogDataLoader.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs
📚 Learning: 2025-10-06T08:24:02.136Z
Learnt from: MagnusSandgren
Repo: Altinn/dialogporten PR: 2818
File: src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/FusionCache/FeatureMetricServiceResourceCache.cs:12-23
Timestamp: 2025-10-06T08:24:02.136Z
Learning: Before raising concerns about captive dependencies or DI lifetime mismatches in the Altinn/dialogporten repository, verify the actual service registration lifetime by checking the DI configuration files (e.g., InfrastructureExtensions.cs).
Applied to files:
src/Digdir.Domain.Dialogporten.Application/ApplicationExtensions.cs
🧬 Code graph analysis (7)
src/Digdir.Domain.Dialogporten.Application/ApplicationExtensions.cs (2)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/DialogTransmissionAppender.cs (2)
DialogTransmissionAppender(20-77)DialogTransmissionAppender(26-34)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cs (2)
TransmissionHierarchyValidator(21-162)TransmissionHierarchyValidator(29-35)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cs (2)
src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/DialogEntity.cs (1)
DialogEntity(18-206)src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/Transmissions/DialogTransmission.cs (1)
DialogTransmission(14-67)
src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Extensions/TransmissionEntityExtensions.cs (1)
src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/Transmissions/DialogTransmission.cs (1)
DialogTransmission(14-67)
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateDialogTransmissionEndpoint.cs (2)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs (1)
CreateTransmissionCommand(25-33)src/Digdir.Domain.Dialogporten.Application/Common/Context/IApplicationContext.cs (1)
IsSilentUpdate(13-15)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs (2)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cs (2)
ValidateWholeAggregate(12-12)ValidateWholeAggregate(37-45)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs (1)
CreateDialogDto(14-162)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionDataLoader.cs (4)
src/Digdir.Domain.Dialogporten.Application/Common/Behaviours/DataLoader/IDataLoader.cs (1)
TypedDataLoader(64-86)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs (2)
CreateTransmissionCommand(25-33)Task(74-144)src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/DialogEntity.cs (1)
DialogEntity(18-206)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogDataLoader.cs (1)
Task(11-12)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs (5)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs (1)
AddSystemLabel(171-184)src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/DialogEntity.cs (1)
DialogEntity(18-206)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogDto.cs (1)
UpdateDialogDto(13-107)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241002134750_AddSystemLabel.cs (1)
AddSystemLabel(11-159)src/Digdir.Domain.Dialogporten.Domain/DialogEndUserContexts/Entities/SystemLabel.cs (2)
SystemLabel(6-32)SystemLabel(21-21)
🔇 Additional comments (19)
src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Extensions/TransmissionEntityExtensions.cs (2)
7-8: LGTM!Good refactor to accept
IEnumerable<DialogTransmission>for flexibility and use of the extracted helper method for reusability.
30-31: LGTM!Clean helper using pattern matching with
is ... or ...syntax.src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogDataLoader.cs (1)
7-8: LGTM!Good documentation of the future optimization opportunity. The TODO clearly explains the intent for follow-up work.
src/Digdir.Domain.Dialogporten.Application/ApplicationExtensions.cs (1)
76-77: LGTM!Transient registrations follow the existing pattern. Both
DialogTransmissionAppenderandTransmissionHierarchyValidatordepend on scoped services (IDialogDbContext,IDomainContext), which is safe when resolved within a request scope.tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs (1)
34-34: LGTM!Mock correctly added for the new
ITransmissionHierarchyValidatordependency. Since these tests verify the authorization failure path, the validator doesn't need explicit setup.AGENTS.md (1)
15-22: LGTM!Clear and helpful guidance for AI agents and developers about build requirements, integration test dependencies, and spec synchronization.
src/Digdir.Domain.Dialogporten.Application/Digdir.Domain.Dialogporten.Application.csproj (1)
32-32: LGTM!Standard addition to enable mocking frameworks (Moq/NSubstitute via Castle.DynamicProxy) to create proxies for internal types like
ITransmissionHierarchyValidatorandIDialogTransmissionAppenderin unit tests.src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateDialogTransmissionEndpoint.cs (1)
41-47: LGTM!Clean refactor to use the new command-based flow. The collection expression
[req]is idiomatic for single-element lists.src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommandValidator.cs (1)
6-18: LGTM!Clean validator implementation that reuses the existing
TransmissionDtovalidator viaRuleForEach. The validation rules appropriately ensure required fields are present before delegating to per-item validation.src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionDataLoader.cs (2)
22-29: LGTM on admin/org scoping logic.The conditional org lookup only when non-admin is an efficient optimization. The
WhereIfpattern cleanly applies the org filter conditionally.
31-37: No changes needed. The soft-delete handling is already implemented correctly inCreateTransmissionCommand.Handle()at lines 82–85, which explicitly checksif (dialog.Deleted)and returnsEntityDeletedbefore processing. TheIgnoreQueryFilters()in the data loader is intentional—it allows the command handler to make explicit domain-level decisions about deleted dialogs rather than relying solely on EF Core's query filter.src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs (2)
49-49: LGTM!Clean integration of
ITransmissionHierarchyValidatorvia constructor injection, following the established DI patterns in this handler.Also applies to: 59-60, 69-69
119-119: LGTM!Good placement of validation after
HasUnopenedContentdetermination but before persisting. Centralizes transmission hierarchy validation for the create flow.src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cs (2)
47-98: LGTM!The incremental validation logic correctly builds an in-memory graph by:
- Adding all new transmission nodes
- Loading ancestor chains for parents that reference existing transmissions
- Loading existing children to properly validate width constraints
The approach efficiently validates hierarchy constraints without loading the full transmission graph.
100-141: LGTM!The BFS traversal with batching (128 nodes per query) is a good approach to avoid excessive database round-trips while preventing overly large
INclauses. The deduplication via dictionary prevents redundant queries for already-loaded nodes.src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs (3)
57-58: LGTM!The new dependencies
IDialogTransmissionAppenderandITransmissionHierarchyValidatorare properly injected and initialized, following the existing conventions in this handler.
130-137: Potential duplicate hierarchy validation.The
AppendTransmissionmethod at line 130 internally calls_transmissionHierarchyValidator.ValidateNewTransmissionsAsync()(lines 341-344), which validates the new transmissions against the hierarchy constraints. Then lines 132-137 immediately validate the wholedialog.Transmissionscollection again.Given that
ValidateNewTransmissionsAsyncalready loads and validates the entire combined hierarchy, this second full-aggregate validation appears redundant. In contrast,CreateTransmissionCommandonly callsValidateNewTransmissionsAsynconce without a follow-up full collection validation. Consider whether the inlineValidateReferenceHierarchycall (lines 132-137) can be removed.
305-353: Unopened content flag is already updated correctly in the update flow.The
HasUnopenedContentflag is recalculated at line 182 viaDialogUnopenedContent.HasUnopenedContent(dialog, serviceResourceInformation)right beforeSaveChangesAsync(). This comprehensive approach checks all transmissions in the dialog; a newly appended service-owner transmission with no activities will correctly result inHasUnopenedContent = true. While the pattern differs fromCreateTransmissionCommand(which uses a simple conditional), the update flow's design is more robust for scenarios where dialog state changes multiple times.Likely an incorrect or invalid review comment.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs (1)
74-144: LGTM!The handler flow is well-structured:
- Pre-checks (entity existence, deletion, frozen state)
- ID normalization for UUIDv7
- DTO mapping with dialog association
- Hierarchy validation before persistence
- Append with conditional label/flag updates
- Authorization with proper error handling
- Final persistence with concurrency control
The pattern is consistent with
UpdateDialogCommandand properly handles all result types.
...ogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs
Outdated
Show resolved
Hide resolved
...on/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs
Outdated
Show resolved
Hide resolved
...alogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/DialogTransmissionAppender.cs
Outdated
Show resolved
Hide resolved
elsand
changed the title
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/DialogTransmissionAppender.cs (1)
26-34: Consider usingArgumentNullException.ThrowIfNull()for more concise null guards.The current manual null checks are correct, but the more modern pattern would be slightly more concise.
Apply this diff:
public DialogTransmissionAppender( IDialogDbContext db, IClock clock, IDomainContext domainContext) { - _db = db ?? throw new ArgumentNullException(nameof(db)); - _clock = clock ?? throw new ArgumentNullException(nameof(clock)); - _domainContext = domainContext ?? throw new ArgumentNullException(nameof(domainContext)); + ArgumentNullException.ThrowIfNull(db); + ArgumentNullException.ThrowIfNull(clock); + ArgumentNullException.ThrowIfNull(domainContext); + _db = db; + _clock = clock; + _domainContext = domainContext; }src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs (1)
146-159: Consider extractingAddSystemLabelto a shared helper.This method is duplicated in both
CreateTransmissionCommandHandlerandCreateDialogCommandHandler(per the relevant code snippets). Extracting it to a shared location would reduce duplication and improve maintainability.For example, add a static helper class:
internal static class DialogSystemLabelHelper { public static void AddSystemLabel( DialogEntity dialog, SystemLabel.Values labelToAdd, IUser user, IDomainContext domainContext) { if (!user.GetPrincipal().TryGetConsumerOrgNumber(out var organizationNumber)) { domainContext.AddError(new DomainFailure(nameof(organizationNumber), "Cannot find organization number for current user.")); return; } dialog.EndUserContext.UpdateSystemLabels( addLabels: [labelToAdd], removeLabels: [], $"{NorwegianOrganizationIdentifier.PrefixWithSeparator}{organizationNumber}", ActorType.Values.ServiceOwner); } }Then both command handlers can call
DialogSystemLabelHelper.AddSystemLabel(dialog, labelToAdd, _user, _domainContext).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs(5 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/DialogTransmissionAppender.cs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.cs: All code must compile withTreatWarningsAsErrors=trueand pass .NET analyzers
Use file-scoped namespaces withusingdirectives outside the namespace
Use PascalCase for classes and methods, camelCase for variables and parameters
Prefer expression bodies for single-line members and usevarwhen the type is apparent
Enable nullable reference types and keep entities immutable; use OneOf for union returns when applicable
Prefer LINQ over manual loops for transformations, filtering, grouping, and projections
PreferFirstOrDefault()/SingleOrDefault()with explicit null handling over index-based access
UseSelect,Where,GroupBy,ToDictionary,ToLookupinstead of building collections imperatively
Avoid materializing collections early withToListorToArrayunless required for correctness or performance
Prefer immutable LINQ pipelines over mutating existing collections
Target latest language features available in the solution (.NET 9 / C# 13); prefer pattern matching, switch expressions, records, collection expressions, target-typednew(), andwithexpressions
Prefer pattern matching (switchexpressions, property patterns) overif/elsechains
Prefer switch expressions over statement-basedswitch
Preferifwith early returns overif/elsechains
Prefer records or record structs for immutable data models
Use collection expressions ([]) instead ofnew List<T>()ornew[] { }where applicable
Prefer target-typednew()when the type is obvious
Preferwithexpressions for non-destructive mutation
Do not suppress nullable warnings (!) unless strictly justified
Prefer pattern matching (is not null) over!= nullfor null checking
Prefer early returns over nested null checks
UseArgumentNullException.ThrowIfNull()for public API guards
Files:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/DialogTransmissionAppender.cs
**/*.{cs,json,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Use 4-space indentation for C# and 2-space indentation for JSON/YAML with LF line endings
Files:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/DialogTransmissionAppender.cs
src/**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.cs: Organize code by feature folders following Clean Architecture with Domain/Application/Infrastructure/API layers
Apply CQRS, domain events, repository pattern, and use rich domain result objects
Avoid throwing exceptions for domain flow; return domain-specific result objects instead
Files:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/DialogTransmissionAppender.cs
🧠 Learnings (5)
📚 Learning: 2025-10-15T17:12:11.560Z
Learnt from: knuhau
Repo: Altinn/dialogporten PR: 2872
File: src/Digdir.Domain.Dialogporten.Janitor/CostManagementAggregation/MetricsAggregationOptions.cs:15-23
Timestamp: 2025-10-15T17:12:11.560Z
Learning: In the Dialogporten Janitor application (src/Digdir.Domain.Dialogporten.Janitor), prefer runtime validation over startup validation (e.g., [Required] attributes) for configuration values that are only needed by specific commands. This prevents the application from failing to start when running other commands that don't require those configuration values.
Applied to files:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs
📚 Learning: 2025-12-12T13:51:39.643Z
Learnt from: elsand
Repo: Altinn/dialogporten PR: 3132
File: src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/NotificationCondition/NotificationConditionQuery.cs:40-55
Timestamp: 2025-12-12T13:51:39.643Z
Learning: In src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/NotificationCondition/NotificationConditionQuery.cs, the soft-deletion status of dialogs is intentionally not checked. The EntityDeleted case in the NotificationConditionResult union is unreachable by design, as whether a dialog is soft-deleted is not relevant for determining whether a notification should be sent.
Applied to files:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/DialogTransmissionAppender.cs
📚 Learning: 2025-12-15T13:34:26.796Z
Learnt from: CR
Repo: Altinn/dialogporten PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-15T13:34:26.796Z
Learning: Build with `dotnet build Digdir.Domain.Dialogporten.sln`
Applied to files:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs
📚 Learning: 2025-12-15T13:34:26.796Z
Learnt from: CR
Repo: Altinn/dialogporten PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-15T13:34:26.796Z
Learning: Run tests with `dotnet test Digdir.Domain.Dialogporten.sln` after making changes; skip integration tests in non-Docker environments with `--filter 'FullyQualifiedName!~Integration'`
Applied to files:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs
📚 Learning: 2025-10-09T08:50:14.740Z
Learnt from: oskogstad
Repo: Altinn/dialogporten PR: 2841
File: src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20251009083408_AddDialogSearch.cs:9-10
Timestamp: 2025-10-09T08:50:14.740Z
Learning: Files in `src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/*/**` are auto-generated Entity Framework Core migrations and should not be reviewed for formatting or coding style issues.
Applied to files:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/DialogTransmissionAppender.cs
🧬 Code graph analysis (3)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs (2)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cs (2)
ValidateWholeAggregate(12-12)ValidateWholeAggregate(37-45)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs (1)
CreateDialogDto(14-162)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs (5)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs (3)
Task(72-137)Task(139-152)AddSystemLabel(171-184)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionDataLoader.cs (3)
Task(22-38)CreateTransmissionDataLoader(11-39)CreateTransmissionDataLoader(16-20)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cs (4)
Task(14-18)Task(47-98)Task(100-141)Task(143-159)src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/DialogEntity.cs (1)
DialogEntity(18-206)src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/Transmissions/DialogTransmission.cs (1)
DialogTransmission(14-67)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/DialogTransmissionAppender.cs (2)
src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/DialogEntity.cs (1)
DialogEntity(18-206)src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/Transmissions/DialogTransmission.cs (2)
DialogTransmission(14-67)DialogTransmissionAttachment(75-79)
🪛 GitHub Check: build / build-and-test
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs
[failure] 162-162:
Type or namespace definition, or end-of-file expected
[failure] 162-162:
Type or namespace definition, or end-of-file expected
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Dry run deploy apps / Deploy job reindex-dialogsearch-job to test
- GitHub Check: Dry run deploy apps / Deploy job sync-subject-resource-mappings-job to test
- GitHub Check: Dry run deploy apps / Deploy job sync-resource-policy-information-job to test
- GitHub Check: Dry run deploy apps / Deploy job aggregate-cost-metrics-job to test
🔇 Additional comments (11)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs (3)
14-14: LGTM: Dependency injection wiring follows conventions.The new
ITransmissionHierarchyValidatordependency is properly wired through the constructor following standard DI patterns, naming conventions, and null-check practices consistent with the rest of the handler.Also applies to: 49-49, 59-60, 69-69
119-119: LGTM: Centralized validation improves maintainability.Replacing inline validation with
ITransmissionHierarchyValidator.ValidateWholeAggregatecentralizes the transmission hierarchy checks, making the validation logic reusable across CreateDialogCommand, UpdateDialogCommand, and CreateTransmissionCommand as described in the PR objectives.
121-123: LGTM: Checked casts address the previous overflow concern.The code now uses
checkedcasts when assigningFromPartyTransmissionsCountandFromServiceOwnerTransmissionsCount, which properly addresses the overflow concern raised in the previous review. If the transmission count exceedsshort.MaxValue(32,767), anOverflowExceptionwill be thrown, which is appropriate since such a scenario indicates a data integrity issue rather than normal domain flow.src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/DialogTransmissionAppender.cs (4)
7-7: Consider using file-scoped namespace declaration.Per the coding guidelines, prefer file-scoped namespaces with a semicolon instead of block-scoped namespaces.
Apply this diff:
-namespace Digdir.Domain.Dialogporten.Application.Features.V1.ServiceOwner.Dialogs.Common; +namespace Digdir.Domain.Dialogporten.Application.Features.V1.ServiceOwner.Dialogs.Common;Wait, looking at the code more carefully, if line 7 shows:
namespace Digdir.Domain.Dialogporten.Application.Features.V1.ServiceOwner.Dialogs.Common;Then it's already file-scoped (ends with semicolon). But if the actual line is without semicolon, then it needs to be changed. Based on the annotations showing line 7~ followed by line 8~ with no closing brace visible before the interface definition on line 9~, I believe this is already file-scoped. Let me skip this comment.
9-18: LGTM!The interface and result record design is clean and focused. Using
IReadOnlyCollectionfor the parameter appropriately signals that the collection won't be modified during validation.
36-61: LGTM!The
Appendmethod implementation is well-structured:
- Early return for empty collections avoids unnecessary work
- Checked arithmetic prevents silent overflow (addressing the previously raised concern)
- Proper EF Core tracking by adding to both the navigation property and DbSet
- Efficient attachment validation using
SelectMany- Clear return value indicating transmission types added
63-76: LGTM!The
ValidateTimeFieldsmethod properly validates attachment expiration times using domain error reporting. The structured key path and optional ID in the error message provide good debugging context.src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs (4)
1-23: LGTM!File-scoped namespace declaration follows the coding guidelines. The using alias on line 21 appropriately reuses the
TransmissionDtofrom the Update command namespace.
25-38: LGTM!The command and result type design follows CQRS patterns and coding guidelines. Using
OneOffor the result type provides type-safe domain-specific outcomes, and the success record appropriately includes both the dialog revision and newly created transmission IDs.
52-72: Constructor follows established patterns.The constructor properly injects all necessary dependencies. While the manual null checks are verbose, they're consistent with the codebase pattern. (See earlier comment on
DialogTransmissionAppenderfor a more modern alternative.)
74-144: LGTM!The
Handlemethod implements a well-structured command flow:
- Proper validation sequence (existence, deletion status, frozen state, hierarchy)
- ID normalization ensures consistent UUID version 7 usage
- Delegation to shared components (
ITransmissionHierarchyValidator,IDialogTransmissionAppender) promotes reusability- Appropriate state updates based on transmission type (labels,
HasUnopenedContent)- Authorization with domain context rollback on failure
- Concurrency control via
IfMatchDialogRevision
...on/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs (1)
146-159: Consider extracting shared AddSystemLabel logic.This method is identical to
AddSystemLabelinUpdateDialogCommand. While the duplication is small (3 lines of core logic), extracting it to a shared helper would improve maintainability if system label logic evolves.Potential approaches:
- Extract to a static utility method in a shared class (e.g.,
DialogSystemLabelHelper.AddSystemLabel)- Create a shared base class for command handlers that need this functionality
- Accept the duplication given the small footprint
Given the limited scope, this refactor can be deferred.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.cs: All code must compile withTreatWarningsAsErrors=trueand pass .NET analyzers
Use file-scoped namespaces withusingdirectives outside the namespace
Use PascalCase for classes and methods, camelCase for variables and parameters
Prefer expression bodies for single-line members and usevarwhen the type is apparent
Enable nullable reference types and keep entities immutable; use OneOf for union returns when applicable
Prefer LINQ over manual loops for transformations, filtering, grouping, and projections
PreferFirstOrDefault()/SingleOrDefault()with explicit null handling over index-based access
UseSelect,Where,GroupBy,ToDictionary,ToLookupinstead of building collections imperatively
Avoid materializing collections early withToListorToArrayunless required for correctness or performance
Prefer immutable LINQ pipelines over mutating existing collections
Target latest language features available in the solution (.NET 9 / C# 13); prefer pattern matching, switch expressions, records, collection expressions, target-typednew(), andwithexpressions
Prefer pattern matching (switchexpressions, property patterns) overif/elsechains
Prefer switch expressions over statement-basedswitch
Preferifwith early returns overif/elsechains
Prefer records or record structs for immutable data models
Use collection expressions ([]) instead ofnew List<T>()ornew[] { }where applicable
Prefer target-typednew()when the type is obvious
Preferwithexpressions for non-destructive mutation
Do not suppress nullable warnings (!) unless strictly justified
Prefer pattern matching (is not null) over!= nullfor null checking
Prefer early returns over nested null checks
UseArgumentNullException.ThrowIfNull()for public API guards
Files:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs
**/*.{cs,json,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Use 4-space indentation for C# and 2-space indentation for JSON/YAML with LF line endings
Files:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs
src/**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.cs: Organize code by feature folders following Clean Architecture with Domain/Application/Infrastructure/API layers
Apply CQRS, domain events, repository pattern, and use rich domain result objects
Avoid throwing exceptions for domain flow; return domain-specific result objects instead
Files:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs
🧠 Learnings (1)
📚 Learning: 2025-10-09T08:50:14.740Z
Learnt from: oskogstad
Repo: Altinn/dialogporten PR: 2841
File: src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20251009083408_AddDialogSearch.cs:9-10
Timestamp: 2025-10-09T08:50:14.740Z
Learning: Files in `src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/*/**` are auto-generated Entity Framework Core migrations and should not be reviewed for formatting or coding style issues.
Applied to files:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs
🧬 Code graph analysis (1)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs (6)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs (3)
Task(72-137)Task(139-152)AddSystemLabel(171-184)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionDataLoader.cs (3)
Task(22-38)CreateTransmissionDataLoader(11-39)CreateTransmissionDataLoader(16-20)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cs (4)
Task(14-18)Task(47-98)Task(100-141)Task(143-159)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs (3)
Task(88-197)Task(305-353)AddSystemLabel(199-212)src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/DialogUnopenedContent.cs (1)
HasUnopenedContent(11-27)src/Digdir.Domain.Dialogporten.Domain/DialogEndUserContexts/Entities/DialogEndUserContext.cs (1)
UpdateSystemLabels(26-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build / build-and-test
🔇 Additional comments (3)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs (3)
25-38: LGTM! Clean command and result type definitions.The command structure follows CQRS patterns correctly, uses collection expressions, and implements the appropriate interfaces for silent updates and feature metrics. The OneOf-based result type properly encapsulates all possible outcomes without throwing exceptions for domain flow control.
40-72: LGTM! Proper dependency injection with validation.All dependencies are validated with explicit null checks. The handler correctly injects the new abstractions (IDialogTransmissionAppender, ITransmissionHierarchyValidator) introduced in this PR.
74-144: LGTM! Well-structured handler with correct performance optimization.The implementation correctly:
- Uses the preloaded data pattern to achieve O(1) transmission adds (key performance goal from PR objectives)
- Follows early-return pattern for error conditions
- Validates transmission hierarchy before appending
- Uses the new DialogTransmissionAppender abstraction to centralize counter updates
- Conditionally applies system labels based on transmission sender type
- Handles authorization with proper domain context rollback on failure
- Manages concurrency control through IfMatchDialogRevision
The flow is clean, error handling returns domain result types without exceptions, and the separation of concerns (validator, appender) improves maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs (1)
49-49: Dependency injection wiring is correct.The validator is properly registered as a readonly field, constructor parameter, and initialized with null checking. The pattern is consistent with other dependencies in this handler.
Optional: The coding guidelines suggest using
ArgumentNullException.ThrowIfNull()for parameter validation. You could refactor all constructor guards to use this more concise pattern:-_transmissionHierarchyValidator = transmissionHierarchyValidator ?? throw new ArgumentNullException(nameof(transmissionHierarchyValidator)); +ArgumentNullException.ThrowIfNull(transmissionHierarchyValidator); +_transmissionHierarchyValidator = transmissionHierarchyValidator;However, maintaining consistency with the existing pattern in this file is also reasonable.
Also applies to: 59-60, 69-69
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cs (2)
20-34: Class initialization is solid.The constants
MaxHierarchyDepthandMaxHierarchyWidth(both 20) provide clear configuration for hierarchy limits. Constructor dependencies are properly guarded.Optional: Consider using
ArgumentNullException.ThrowIfNull()for consistency with coding guidelines:public TransmissionHierarchyValidator( IDialogDbContext dbContext, IDomainContext domainContext) { - _dbContext = dbContext ?? throw new ArgumentNullException(nameof(dbContext)); - _domainContext = domainContext ?? throw new ArgumentNullException(nameof(domainContext)); + ArgumentNullException.ThrowIfNull(dbContext); + ArgumentNullException.ThrowIfNull(domainContext); + _dbContext = dbContext; + _domainContext = domainContext; }
98-139: Efficient breadth-first hierarchy loader with appropriate batching.The method implements a breadth-first traversal of the parent chain, using batching (up to 128 items per query) to avoid N+1 query issues. The logic correctly:
- Skips already-loaded nodes (line 112)
- Fetches batches via EF Core (lines 124-127)
- Enqueues newly discovered parents for subsequent loading (line 133)
Optional: Consider extracting the magic number 128 to a named constant for clarity:
+private const int HierarchyLoadBatchSize = 128; + private async Task<List<TransmissionHierarchyNode>> LoadHierarchyNodes( Guid dialogId, IReadOnlyCollection<Guid> startIds, CancellationToken cancellationToken) { var nodes = new Dictionary<Guid, TransmissionHierarchyNode>(); var pending = new Queue<Guid>(startIds); while (pending.Count > 0) { var batch = new HashSet<Guid>(); - while (pending.Count > 0 && batch.Count < 128) + while (pending.Count > 0 && batch.Count < HierarchyLoadBatchSize) {src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs (1)
84-85: Consider modernizing null checks (optional).While the current pattern is consistent with the existing codebase, the coding guidelines prefer
ArgumentNullException.ThrowIfNull()for newer code. Since this is new code in C# 13, you could modernize:-_dialogTransmissionAppender = dialogTransmissionAppender ?? throw new ArgumentNullException(nameof(dialogTransmissionAppender)); -_transmissionHierarchyValidator = transmissionHierarchyValidator ?? throw new ArgumentNullException(nameof(transmissionHierarchyValidator)); +ArgumentNullException.ThrowIfNull(dialogTransmissionAppender); +ArgumentNullException.ThrowIfNull(transmissionHierarchyValidator); +_dialogTransmissionAppender = dialogTransmissionAppender; +_transmissionHierarchyValidator = transmissionHierarchyValidator;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs(5 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs(7 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.cs: All code must compile withTreatWarningsAsErrors=trueand pass .NET analyzers
Use file-scoped namespaces withusingdirectives outside the namespace
Use PascalCase for classes and methods, camelCase for variables and parameters
Prefer expression bodies for single-line members and usevarwhen the type is apparent
Enable nullable reference types and keep entities immutable; use OneOf for union returns when applicable
Prefer LINQ over manual loops for transformations, filtering, grouping, and projections
PreferFirstOrDefault()/SingleOrDefault()with explicit null handling over index-based access
UseSelect,Where,GroupBy,ToDictionary,ToLookupinstead of building collections imperatively
Avoid materializing collections early withToListorToArrayunless required for correctness or performance
Prefer immutable LINQ pipelines over mutating existing collections
Target latest language features available in the solution (.NET 9 / C# 13); prefer pattern matching, switch expressions, records, collection expressions, target-typednew(), andwithexpressions
Prefer pattern matching (switchexpressions, property patterns) overif/elsechains
Prefer switch expressions over statement-basedswitch
Preferifwith early returns overif/elsechains
Prefer records or record structs for immutable data models
Use collection expressions ([]) instead ofnew List<T>()ornew[] { }where applicable
Prefer target-typednew()when the type is obvious
Preferwithexpressions for non-destructive mutation
Do not suppress nullable warnings (!) unless strictly justified
Prefer pattern matching (is not null) over!= nullfor null checking
Prefer early returns over nested null checks
UseArgumentNullException.ThrowIfNull()for public API guards
Files:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs
**/*.{cs,json,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Use 4-space indentation for C# and 2-space indentation for JSON/YAML with LF line endings
Files:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs
src/**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.cs: Organize code by feature folders following Clean Architecture with Domain/Application/Infrastructure/API layers
Apply CQRS, domain events, repository pattern, and use rich domain result objects
Avoid throwing exceptions for domain flow; return domain-specific result objects instead
Files:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs
🧠 Learnings (4)
📚 Learning: 2025-10-15T17:12:11.560Z
Learnt from: knuhau
Repo: Altinn/dialogporten PR: 2872
File: src/Digdir.Domain.Dialogporten.Janitor/CostManagementAggregation/MetricsAggregationOptions.cs:15-23
Timestamp: 2025-10-15T17:12:11.560Z
Learning: In the Dialogporten Janitor application (src/Digdir.Domain.Dialogporten.Janitor), prefer runtime validation over startup validation (e.g., [Required] attributes) for configuration values that are only needed by specific commands. This prevents the application from failing to start when running other commands that don't require those configuration values.
Applied to files:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs
📚 Learning: 2025-12-12T13:51:39.643Z
Learnt from: elsand
Repo: Altinn/dialogporten PR: 3132
File: src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/NotificationCondition/NotificationConditionQuery.cs:40-55
Timestamp: 2025-12-12T13:51:39.643Z
Learning: In src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/NotificationCondition/NotificationConditionQuery.cs, the soft-deletion status of dialogs is intentionally not checked. The EntityDeleted case in the NotificationConditionResult union is unreachable by design, as whether a dialog is soft-deleted is not relevant for determining whether a notification should be sent.
Applied to files:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs
📚 Learning: 2025-12-15T13:34:26.796Z
Learnt from: CR
Repo: Altinn/dialogporten PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-15T13:34:26.796Z
Learning: Build with `dotnet build Digdir.Domain.Dialogporten.sln`
Applied to files:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs
📚 Learning: 2025-12-15T13:34:26.796Z
Learnt from: CR
Repo: Altinn/dialogporten PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-15T13:34:26.796Z
Learning: Run tests with `dotnet test Digdir.Domain.Dialogporten.sln` after making changes; skip integration tests in non-Docker environments with `--filter 'FullyQualifiedName!~Integration'`
Applied to files:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs
🔇 Additional comments (16)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateTransmissionCommand.cs (5)
25-33: LGTM! Clean command structure.The command follows CQRS patterns correctly, uses collection expressions for initialization, and implements the required interfaces with appropriate explicit interface member for
DialogId.
35-38: LGTM! Proper use of OneOf for result types.The result type correctly uses OneOf for union returns with comprehensive error cases, and the success record is appropriately immutable.
40-72: LGTM! Well-structured handler with proper dependency injection.The handler is correctly scoped as internal sealed, injects all required dependencies, and validates them appropriately. The dependency list reflects the comprehensive workflow involving data loading, validation, appending, authorization, and persistence.
74-143: LGTM! Excellent implementation of the command handler workflow.The Handle method demonstrates a clean, well-structured flow:
- Early returns for validation failures (EntityNotFound, EntityDeleted, Frozen)
- Proper use of preloaded data for performance (O(1) transmission adds)
- Async hierarchy validation before appending
- Correct delegation to
DialogTransmissionAppenderfor counter updates and attachment validation- Appropriate system label management based on transmission type
- Authorization with rollback via
_domainContext.Pop()on Forbidden- Concurrency-safe persistence with revision checking
The implementation adheres to CQRS, domain-driven design, and the coding guidelines—returning domain result objects instead of throwing exceptions for flow control.
145-158: LGTM! Proper domain error handling.The helper method correctly handles the missing organization number case by adding a domain error and returning early, rather than throwing an exception. This aligns with the domain-driven design principle of using domain result objects for flow control.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs (3)
14-14: LGTM! Using directive properly supports the new validator dependency.The addition of the
Dialogs.Commonnamespace is necessary for theITransmissionHierarchyValidatorinterface used below.
119-119: Excellent refactor to use centralized validation.Delegating transmission hierarchy validation to
ITransmissionHierarchyValidatorimproves separation of concerns and enables reuse across create/update/transmission commands. The placement after UUID assignment (lines 113-116) is correct.
121-123: Checked cast properly addresses overflow protection.The use of
checkedcasts ensures that any overflow when converting counts toshortwill throw an exception rather than silently wrapping. This addresses the overflow concern raised in previous review feedback.src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cs (5)
10-18: Well-defined interface with clear responsibilities.The interface cleanly separates whole-aggregate validation (for create/update with full graph) from incremental validation (for adding transmissions to existing dialogs). Internal visibility is appropriate for this application-layer component.
36-44: Straightforward delegation to the hierarchy validation extension.The method correctly delegates to
ValidateReferenceHierarchy, passing the transmission collection with appropriate selectors for ID and parent reference. Errors are properly routed to the domain context.
46-96: Well-structured incremental validation with efficient hierarchy loading.The method correctly:
- Builds an in-memory representation of new transmissions
- Identifies which parents already exist in the database
- Loads the necessary existing hierarchy nodes (parents and their children) in batches
- Validates the combined hierarchy against depth/width constraints
The use of
HashSetfor ID lookups andTryAddfor deduplication ensures efficiency. The separation of new vs. existing nodes prevents unnecessary database queries.
141-157: Clean query for loading existing child transmissions.The method efficiently loads child nodes for the given parent IDs, with an early return optimization for empty input. The query correctly filters by dialog ID and checks for matching parent references.
159-159: Excellent use of a record for the internal hierarchy node representation.The
TransmissionHierarchyNoderecord provides a lightweight, immutable structure for building the in-memory hierarchy during validation. Thesealedmodifier and private scope are appropriate for an internal implementation detail.src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs (3)
14-14: LGTM! Dependency injection properly implemented.The new services (
IDialogTransmissionAppenderandITransmissionHierarchyValidator) are correctly injected following the existing constructor pattern, with appropriate null checks and field assignments.Also applies to: 57-58, 70-72, 84-85
130-130: LGTM! Async signature change correctly implemented.The method signature change to
async Taskand the correspondingawaitat the call site are properly implemented to support the new asynchronous validation flow.Also applies to: 305-305
341-351: Verify potential redundancy with existing validation.The new validation and append flow follows the existing pattern of adding domain errors to the context. However, the relationship between the new
ITransmissionHierarchyValidator.ValidateNewTransmissionsAsync()call (lines 341-344) and the legacyValidateReferenceHierarchy()call (lines 132-137) needs clarification:
- Per the PR summary,
ITransmissionHierarchyValidatorencapsulates "both full-aggregate and incremental transmission-hierarchy checks"- The new validator validates new transmissions incrementally
- The legacy validator runs on the full
dialog.Transmissionscollection after appendingConfirm whether
ValidateReferenceHierarchyis still necessary or fully replaced by the new validator to avoid redundant checks.
| @@ -0,0 +1,19 @@ | |||
| using FluentValidation; | |||
| using TransmissionDto = Digdir.Domain.Dialogporten.Application.Features.V1.ServiceOwner.Dialogs.Commands.Update.TransmissionDto; | |||
There was a problem hiding this comment.
Nå tar denne nye vertikalen avhengighet på en annen som skal være isolert.
There was a problem hiding this comment.
Tror strengt tatt ikke du trenger en dataloader her ettersom det kun er et sted som bryr seg om disse dataene. Så dette kan legges rett inn i CreateTransmissionCommand.Handle
|
|
||
| namespace Digdir.Domain.Dialogporten.Application.Features.V1.ServiceOwner.Dialogs.Commands.Update; | ||
|
|
||
| // TODO: The hierarchy validator can now fetch only the referenced transmissions, |
There was a problem hiding this comment.
Er dette noe som skal gjøres senere?
src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cs (1)
27-33: Consider usingArgumentNullException.ThrowIfNull()for consistency with modern C# patterns.While the current null checks are correct, C# 13 provides the more concise
ArgumentNullException.ThrowIfNull()helper. Although the coding guideline specifically mentions "public API guards," adopting this pattern for internal APIs improves consistency across the codebase.🔎 Proposed refactor
public TransmissionHierarchyValidator( ITransmissionHierarchyRepository hierarchyRepository, IDomainContext domainContext) { - _hierarchyRepository = hierarchyRepository ?? throw new ArgumentNullException(nameof(hierarchyRepository)); - _domainContext = domainContext ?? throw new ArgumentNullException(nameof(domainContext)); + ArgumentNullException.ThrowIfNull(hierarchyRepository); + ArgumentNullException.ThrowIfNull(domainContext); + _hierarchyRepository = hierarchyRepository; + _domainContext = domainContext; }src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateDialogTransmissionEndpoint.cs (2)
18-21: PreferArgumentNullException.ThrowIfNull()per coding guidelines.The coding guidelines require using
ArgumentNullException.ThrowIfNull()for public API guards. This is terser and aligns with modern .NET conventions.🔎 Proposed refactor
public CreateDialogTransmissionEndpoint(ISender sender) { - _sender = sender ?? throw new ArgumentNullException(nameof(sender)); + ArgumentNullException.ThrowIfNull(sender); + _sender = sender; }
54-54: ConsiderSingle()for semantic clarity.Since the command creates exactly one transmission (line 45), using
Single()instead ofFirst()would better express the expectation of exactly one ID in the result.🔎 Proposed refactor
-var transmissionId = success.TransmissionIds.First(); +var transmissionId = success.TransmissionIds.Single();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Digdir.Domain.Dialogporten.Application/Externals/ITransmissionHierarchyRepository.cssrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cssrc/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cssrc/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Repositories/TransmissionHierarchyRepository.cssrc/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateDialogTransmissionEndpoint.cstests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Common/DialogApplication.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Common/DialogApplication.cs
- src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Repositories/TransmissionHierarchyRepository.cs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.cs: All code must compile withTreatWarningsAsErrors=trueand pass .NET analyzers
Use file-scoped namespaces withusingdirectives outside the namespace
Use PascalCase for classes and methods, camelCase for variables and parameters
Prefer expression bodies for single-line members and usevarwhen the type is apparent
Enable nullable reference types and keep entities immutable; use OneOf for union returns when applicable
Prefer LINQ over manual loops for transformations, filtering, grouping, and projections
PreferFirstOrDefault()/SingleOrDefault()with explicit null handling over index-based access
UseSelect,Where,GroupBy,ToDictionary,ToLookupinstead of building collections imperatively
Avoid materializing collections early withToListorToArrayunless required for correctness or performance
Prefer immutable LINQ pipelines over mutating existing collections
Target latest language features available in the solution (.NET 9 / C# 13); prefer pattern matching, switch expressions, records, collection expressions, target-typednew(), andwithexpressions
Prefer pattern matching (switchexpressions, property patterns) overif/elsechains
Prefer switch expressions over statement-basedswitch
Preferifwith early returns overif/elsechains
Prefer records or record structs for immutable data models
Use collection expressions ([]) instead ofnew List<T>()ornew[] { }where applicable
Prefer target-typednew()when the type is obvious
Preferwithexpressions for non-destructive mutation
Do not suppress nullable warnings (!) unless strictly justified
Prefer pattern matching (is not null) over!= nullfor null checking
Prefer early returns over nested null checks
UseArgumentNullException.ThrowIfNull()for public API guards
Files:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cssrc/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateDialogTransmissionEndpoint.cssrc/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cssrc/Digdir.Domain.Dialogporten.Application/Externals/ITransmissionHierarchyRepository.cs
**/*.{cs,json,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Use 4-space indentation for C# and 2-space indentation for JSON/YAML with LF line endings
Files:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cssrc/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateDialogTransmissionEndpoint.cssrc/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cssrc/Digdir.Domain.Dialogporten.Application/Externals/ITransmissionHierarchyRepository.cs
src/**/*.cs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.cs: Organize code by feature folders following Clean Architecture with Domain/Application/Infrastructure/API layers
Apply CQRS, domain events, repository pattern, and use rich domain result objects
Avoid throwing exceptions for domain flow; return domain-specific result objects instead
Files:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cssrc/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateDialogTransmissionEndpoint.cssrc/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cssrc/Digdir.Domain.Dialogporten.Application/Externals/ITransmissionHierarchyRepository.cs
🧠 Learnings (3)
📚 Learning: 2025-10-06T08:24:02.136Z
Learnt from: MagnusSandgren
Repo: Altinn/dialogporten PR: 2818
File: src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/FusionCache/FeatureMetricServiceResourceCache.cs:12-23
Timestamp: 2025-10-06T08:24:02.136Z
Learning: Before raising concerns about captive dependencies or DI lifetime mismatches in the Altinn/dialogporten repository, verify the actual service registration lifetime by checking the DI configuration files (e.g., InfrastructureExtensions.cs).
Applied to files:
src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs
📚 Learning: 2025-10-09T08:50:14.740Z
Learnt from: oskogstad
Repo: Altinn/dialogporten PR: 2841
File: src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20251009083408_AddDialogSearch.cs:9-10
Timestamp: 2025-10-09T08:50:14.740Z
Learning: Files in `src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/*/**` are auto-generated Entity Framework Core migrations and should not be reviewed for formatting or coding style issues.
Applied to files:
src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs
📚 Learning: 2025-12-15T13:34:26.796Z
Learnt from: CR
Repo: Altinn/dialogporten PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-15T13:34:26.796Z
Learning: Run tests with `dotnet test Digdir.Domain.Dialogporten.sln` after making changes; skip integration tests in non-Docker environments with `--filter 'FullyQualifiedName!~Integration'`
Applied to files:
src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs
🧬 Code graph analysis (1)
src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs (1)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Repositories/TransmissionHierarchyRepository.cs (1)
TransmissionHierarchyRepository(9-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Dry run deploy apps / Deploy job sync-resource-policy-information-job to test
- GitHub Check: Dry run deploy apps / Deploy job aggregate-cost-metrics-job to test
- GitHub Check: Dry run deploy apps / Deploy job reindex-dialogsearch-job to test
- GitHub Check: Dry run deploy apps / Deploy job sync-subject-resource-mappings-job to test
- GitHub Check: build / build-and-test
🔇 Additional comments (5)
src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs (1)
114-114: LGTM!The transient registration for
ITransmissionHierarchyRepositoryis correctly placed alongside other repository registrations and uses the appropriate lifetime for a stateless repository.src/Digdir.Domain.Dialogporten.Application/Externals/ITransmissionHierarchyRepository.cs (1)
5-13: LGTM!The interface and record are well-designed:
IReadOnlyCollectionpromotes immutability for both input and output- The
TransmissionHierarchyNoderecord uses modern positional parameters- Nullable
ParentIdappropriately models root nodes in the hierarchy- Async signature with
CancellationTokenfollows best practicessrc/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Common/TransmissionHierarchyValidator.cs (1)
45-77: LGTM!The validation logic is well-structured:
- Early return for empty collections (line 50)
- Efficient filtering to identify only existing parent IDs that need to be fetched (lines 54-60)
- Appropriate use of collection expression
[]for empty collections (line 64, C# 13)- Correct combination of new and existing nodes before validation (lines 66-69)
- The
Distinct().Except()chain properly eliminates duplicate fetchesThe use of
x!.Valueon line 57 is justified by the precedingWhere(x => x.HasValue)filter.src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Commands/CreateTransmission/CreateDialogTransmissionEndpoint.cs (2)
38-65: Clean refactoring to command-based flow.The endpoint now follows a straightforward CQRS pattern: create command, dispatch, handle result. The collection expression on line 45 is idiomatic, error handling is comprehensive via Match, and the routing logic correctly passes both the query object and transmissionId to the GET endpoint.
68-77: Request structure looks good.The DTO reuses the Update command's TransmissionDto structure (line 10 alias), which is reasonable for this refactoring phase. The additional properties (DialogId, IfMatchDialogRevision, IsSilentUpdate) are correctly annotated with FastEndpoints attributes.
2 participants
Description
CreateTransmissionCommandwhich uses a custom preloader for O(1) adds of transmissionsITransmissionHierarchyValidatorto encapsulate both full-aggregate and incremental transmission hierarchy checks; registered it in DI and wired CreateDialogCommand, UpdateDialogCommand, and CreateTransmissionCommand to use it instead of bespoke logic.DialogTransmissionAppenderto centralize counter updates/attachment expiry validation, updatedTransmissionEntityExtensionswith count helpers, and adjusted command handlers/tests (including new InternalsVisibleTo entry) to accommodate the shared appender/validator.Similar refactoring for the
CreateDialogActivityEndpointwill be a separate PR. When this is done, we can make PUT/PATCH more performant by not hydrating the entire tranmissions/activity graph inUpdateDialogDataLoaderRelated Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)