feat: restore dialog action #1702
Conversation
…ng granular opting out of individual features.
Completed Endpoint Removed comments
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive implementation for dialog restoration in the Dialogporten system. The changes span multiple components, including the domain, application, infrastructure, and web API layers. Key modifications include adding a new restoration endpoint, creating domain events for dialog restoration, updating entity handling mechanisms, and introducing more granular filter controls for unit of work operations. The implementation allows for restoring soft-deleted dialogs while preserving their original state and generating appropriate domain events. Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/Digdir.Domain.Dialogporten.Domain/Dialogs/Events/DialogRestoredDomainEvent.cs (1)
5-10: LGTM! Consider adding XML documentation.The domain event structure is well-designed with appropriate properties for tracking dialog restoration. Consider adding XML documentation to describe the purpose of each property, especially the relationship between
ProcessandPrecedingProcess.public sealed record DialogRestoredDomainEvent( + /// <summary> + /// The unique identifier of the restored dialog. + /// </summary> Guid DialogId, + /// <summary> + /// The service resource associated with the dialog. + /// </summary> string ServiceResource, + /// <summary> + /// The party involved in the dialog. + /// </summary> string Party, + /// <summary> + /// The current process identifier, if any. + /// </summary> string? Process, + /// <summary> + /// The preceding process identifier, if any. + /// </summary> string? PrecedingProcess) : DomainEvent, IProcessEvent;src/Digdir.Library.Entity.EntityFrameworkCore/IEntityOptions.cs (2)
3-47: Consider providing default values documentation.The interface is well-structured and documented. However, it would be beneficial to document the default behavior when these options are not explicitly set.
Add default value documentation to each property:
/// <summary> /// Gets a value indicating whether the soft deletable filter is enabled. + /// Default value: true /// </summary> bool EnableSoftDeletableFilter { get; }
6-47: Consider providing implementation guidance.The interface provides good flexibility for entity handling, but consider adding implementation examples or guidance in the interface documentation.
Add interface-level documentation:
/// <summary> /// Interface for configuring entity handling behavior options. +/// <example> +/// Typical implementation might look like: +/// <code> +/// public class EntityOptions : IEntityOptions +/// { +/// public bool EnableSoftDeletableFilter => true; +/// public bool EnableImmutableFilter => true; +/// // ... other properties +/// } +/// </code> +/// </example> /// </summary> public interface IEntityOptionstests/k6/tests/serviceowner/dialogRestore.js (1)
49-64: Add test for concurrent modification scenario.While the current test thoroughly verifies successful restoration, consider adding a test case for concurrent modification where an incorrect ETag is provided. This would verify the 412 Precondition Failed response.
describe('Restore with incorrect ETag', () => { let incorrectEtag = '00000000-0000-0000-0000-000000000000'; let r = restoreDialog(dialogId, incorrectEtag); expectStatusFor(r).to.equal(412); });src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/DialogDbContext.cs (1)
65-67: Add XML documentation explaining the IsModified state preservation.The logic for preserving the IsModified state is correct, but it would benefit from documentation explaining why this state needs to be preserved and its implications for concurrency control.
+ /// <summary> + /// Attempts to set the original revision while preserving the IsModified state. + /// This is crucial for maintaining proper concurrency control during operations + /// like restoration where we need to track both the original revision and any + /// modifications made during the current transaction. + /// </summary> internal bool TrySetOriginalRevision<TEntity>(
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/Digdir.Domain.Dialogporten.Application/Externals/IUnitOfWork.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogQuery.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Restore/RestoreDialogCommand.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/GetDialogQuery.cs(1 hunks)src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/DialogEntity.cs(2 hunks)src/Digdir.Domain.Dialogporten.Domain/Dialogs/Events/DialogRestoredDomainEvent.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/DialogDbContext.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/UnitOfWork.cs(4 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Restore/RestoreDialogEndpoint.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Restore/RestoreDialogEndpointSummary.cs(1 hunks)src/Digdir.Library.Entity.EntityFrameworkCore/EntityLibraryEfCoreExtensions.cs(2 hunks)src/Digdir.Library.Entity.EntityFrameworkCore/Features/Aggregate/AggregateExtensions.cs(2 hunks)src/Digdir.Library.Entity.EntityFrameworkCore/IEntityOptions.cs(1 hunks)tests/k6/tests/serviceowner/all-tests.js(2 hunks)tests/k6/tests/serviceowner/dialogRestore.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Restore/RestoreDialogEndpointSummary.cs
🔇 Additional comments (17)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Restore/RestoreDialogCommand.cs (1)
59-63: Verify the necessity of disabling multiple filtersLines 59-63 disable several entity framework filters before saving changes. Confirm that disabling
Updatable,SoftDeletable, and enabling concurrency checks are required for the restore operation and do not have unintended side effects on other entities or operations.src/Digdir.Domain.Dialogporten.Infrastructure/UnitOfWork.cs (2)
54-56: Review the impact of movingBeginTransactionAsyncThe
BeginTransactionAsyncmethod has been moved to a different location in the file (lines 54-56). Verify that this relocation does not affect the initialization order or any dependencies that rely on the method's position within the class.
113-116: Ensure proper usage ofHandleAuditableEntitiesIn lines 113-116,
HandleAuditableEntitiesis called with_saveChangesOptions. Confirm that_saveChangesOptionsis configured correctly and that the method handles all necessary entity types as intended.src/Digdir.Domain.Dialogporten.Application/Externals/IUnitOfWork.cs (1)
19-22: Breaking change: Verify all usages of WithoutAggregateSideEffects()The removal of
WithoutAggregateSideEffects()in favor of granular filter controls is a good architectural decision. However, this is a breaking change that requires verification of all existing usages.tests/k6/tests/serviceowner/all-tests.js (1)
12-12: LGTM! Test integration looks good.The dialogRestore test has been properly integrated into the test suite following the existing pattern.
Also applies to: 27-27
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Restore/RestoreDialogEndpoint.cs (3)
20-32: LGTM! Well-structured endpoint configuration.The endpoint follows RESTful conventions and properly documents response types. Authorization is correctly configured using the ServiceProvider policy.
34-50: LGTM! Robust request handling with proper error cases.The implementation correctly:
- Handles concurrency using ETags
- Returns appropriate status codes (204, 404, 412)
- Uses pattern matching for clean error handling
53-59: LGTM! Clean and focused request model.The request model correctly handles the dialog ID and optional ETag for concurrency control.
tests/k6/tests/serviceowner/dialogRestore.js (3)
14-23: LGTM! Well-structured test setup.The helper function and test variables are properly initialized. The restoreDialog function correctly handles the optional ETag parameter.
43-47: LGTM! Good idempotency test.The test correctly verifies that restoring a non-deleted dialog is idempotent by checking both the status code and unchanged ETag.
66-68: LGTM! Proper test cleanup.The cleanup ensures proper test isolation by purging the test dialog.
src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/DialogEntity.cs (2)
22-23: LGTM! Appropriate interface implementations.The entity correctly implements IEventPublisher and IAggregateRestoredHandler to support the restoration functionality.
88-90: Consider handling related entities during restoration.While the basic restoration event is correctly published, consider whether related entities (Transmissions, Content, SearchTags, etc.) need special handling during restoration. For example, you might need to:
- Restore related entities' states
- Validate relationships
- Update timestamps
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/GetDialogQuery.cs (1)
118-119: LGTM! Granular control over entity tracking.The replacement of
.WithoutAggregateSideEffects()with.DisableUpdatableFilter()and.DisableVersionableFilter()provides more precise control over which entity tracking features are disabled when saving changes.src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogQuery.cs (1)
129-130: LGTM! Consistent implementation of granular entity tracking.The changes align with the service owner implementation, maintaining consistency across the codebase in how entity tracking is controlled.
src/Digdir.Library.Entity.EntityFrameworkCore/Features/Aggregate/AggregateExtensions.cs (2)
18-18: LGTM! Enhanced flexibility with options parameter.The addition of
IEntityOptionsparameter allows for more configurable entity handling behavior.
27-57: LGTM! Well-structured conditional feature handling.The implementation properly segregates different entity features:
- Aggregate operations (create, update, delete, restore)
- Updatable entity handling
- Versionable entity handling
Each feature can now be independently controlled through the options parameter, improving modularity and maintainability.
...porten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Restore/RestoreDialogCommand.cs
Show resolved
Hide resolved
...porten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Restore/RestoreDialogCommand.cs
Show resolved
Hide resolved
src/Digdir.Library.Entity.EntityFrameworkCore/EntityLibraryEfCoreExtensions.cs
Show resolved
Hide resolved
src/Digdir.Library.Entity.EntityFrameworkCore/EntityLibraryEfCoreExtensions.cs
Show resolved
Hide resolved
|
Caution No docstrings were generated. |
| // Although the entity framework supports all the options, | ||
| // enabling some of them ia an incredibly bad idea. Therefore, | ||
| // only some of them have setters until the day we actually | ||
| // have a use case for them. |
There was a problem hiding this comment.
| // Although the entity framework supports all the options, | |
| // enabling some of them ia an incredibly bad idea. Therefore, | |
| // only some of them have setters until the day we actually | |
| // have a use case for them. | |
| // Although the Entity Framework supports all the options, | |
| // enabling some of them is an incredibly bad idea. Therefore, | |
| // only some of them have setters until the day we actually | |
| // have a use case for them. |
Description
Created new endpoint for restoration of soft deleted dialogs
Modified enitity librarty enabling granular opting out of individual features
Related Issue(s)
#1543
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)