delete dialog when publish goes wrong #682
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 WalkthroughWalkthroughThis pull request introduces a new test method in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This reverts commit eada3ec.
|
Added test and tested manually locally. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Test/Altinn.Correspondence.Tests/TestingFeature/DialogportenTests.cs (1)
102-145: Enhance test robustness and clarity.While the test verifies the dialog purge behavior, consider these improvements:
- Add explicit setup to trigger the publish failure (e.g., mock a service to throw an exception).
- Verify the correspondence status after failure.
- Add test data cleanup.
Here's a suggested implementation:
[Fact] public async Task FailedPublish_DialogIsPurged() { // Arrange var hangfireBackgroundJobClient = new Mock<IBackgroundJobClient>(); var contactReservationRegistry = new Mock<IContactReservationRegistryService>(); + var publishService = new Mock<IPublishService>(); + publishService.Setup(x => x.PublishCorrespondence(It.IsAny<Guid>(), It.IsAny<CancellationToken>())) + .ThrowsAsync(new Exception("Simulated publish failure")); contactReservationRegistry.Setup(contactReservationRegistry => contactReservationRegistry.IsPersonReserved(It.IsAny<string>())).ReturnsAsync(true); contactReservationRegistry.Setup(contactReservationRegistry => contactReservationRegistry.GetReservedRecipients(It.IsAny<List<string>>())).ReturnsAsync(new List<string>()); var testFactory = new UnitWebApplicationFactory((IServiceCollection services) => { services.AddSingleton(hangfireBackgroundJobClient.Object); services.AddSingleton(contactReservationRegistry.Object); + services.AddSingleton(publishService.Object); }); var correspondence = new CorrespondenceBuilder().CreateCorrespondence().Build(); var testClient = testFactory.CreateSenderClient(); + Guid? correspondenceId = null; try { // Act using var scope = testFactory.Services.CreateScope(); var correspondenceRepository = scope.ServiceProvider.GetRequiredService<ICorrespondenceRepository>(); var initializedCorrespondence = await correspondenceRepository.CreateCorrespondence(new Core.Models.Entities.CorrespondenceEntity() { Created = DateTimeOffset.UtcNow, Recipient = correspondence.Recipients[0], RequestedPublishTime = DateTimeOffset.UtcNow, ResourceId = correspondence.Correspondence.ResourceId, Sender = correspondence.Correspondence.Sender, SendersReference = correspondence.Correspondence.SendersReference, Statuses = new List<Core.Models.Entities.CorrespondenceStatusEntity>() { new Core.Models.Entities.CorrespondenceStatusEntity() { Status = Core.Models.Enums.CorrespondenceStatus.Initialized, StatusChanged = DateTimeOffset.UtcNow } } }, CancellationToken.None); correspondenceId = initializedCorrespondence.Id; Assert.NotNull(correspondenceId); var handler = scope.ServiceProvider.GetRequiredService<PublishCorrespondenceHandler>(); await handler.Process(correspondenceId.Value, null, CancellationToken.None); // Assert Assert.True(hangfireBackgroundJobClient.Invocations.Any(invocation => invocation.Arguments[0].ToString() == "IDialogportenService.PurgeCorrespondenceDialog")); + var updatedCorrespondence = await correspondenceRepository.GetCorrespondenceById(correspondenceId.Value, false, false, CancellationToken.None); + Assert.Equal(Core.Models.Enums.CorrespondenceStatus.Failed, updatedCorrespondence.Statuses.OrderByDescending(s => s.StatusChanged).First().Status); } finally { + if (correspondenceId.HasValue) { + using var scope = testFactory.Services.CreateScope(); + var correspondenceRepository = scope.ServiceProvider.GetRequiredService<ICorrespondenceRepository>(); + await correspondenceRepository.DeleteCorrespondence(correspondenceId.Value, CancellationToken.None); + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Test/Altinn.Correspondence.Tests/TestingFeature/DialogportenTests.cs(2 hunks)src/Altinn.Correspondence.Application/PublishCorrespondence/PublishCorrespondenceHandler.cs(2 hunks)src/Altinn.Correspondence.Core/Services/IDialogportenService.cs(1 hunks)src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenDevService.cs(1 hunks)src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Test application
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs
[failure] 75-75:
The name '_logger' does not exist in the current context
[failure] 75-75:
The name '_logger' does not exist in the current context
🪛 GitHub Actions: Test application
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs
[error] 75-75: The name '_logger' does not exist in the current context.
🔇 Additional comments (5)
src/Altinn.Correspondence.Core/Services/IDialogportenService.cs (1)
10-11: LGTM! Clean interface extension.The new
PurgeCorrespondenceDialogmethod follows the existing interface patterns and naming conventions.src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenDevService.cs (1)
17-21: LGTM! Appropriate dev implementation.The implementation follows the pattern of other methods in the development service, using
Task.CompletedTaskfor non-production scenarios.src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs (1)
63-84: LGTM! Robust implementation with proper error handling.The implementation follows the established patterns:
- Validates correspondence existence
- Checks for dialog ID
- Handles HTTP response errors
🧰 Tools
🪛 GitHub Check: Test application
[failure] 75-75:
The name '_logger' does not exist in the current context
[failure] 75-75:
The name '_logger' does not exist in the current context🪛 GitHub Actions: Test application
[error] 75-75: The name '_logger' does not exist in the current context.
src/Altinn.Correspondence.Application/PublishCorrespondence/PublishCorrespondenceHandler.cs (1)
25-25: LGTM! Well-integrated error handling.The changes properly integrate dialog purging:
- Correctly injected IDialogportenService
- Appropriately scheduled dialog purge in the error path using Hangfire
- Consistent with existing background job patterns
Also applies to: 86-86
Test/Altinn.Correspondence.Tests/TestingFeature/DialogportenTests.cs (1)
2-3: LGTM!The added using directives are necessary for the new test and follow the existing organization pattern.
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Verification
Documentation
Summary by CodeRabbit
New Features
Tests