Be explicit about SaveChanges - #2035
Closed
Ceredron wants to merge 27 commits into
Closed
Conversation
added 18 commits
June 19, 2026 12:12
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesThe PR centralizes EF Core save operations and PostgreSQL unique-violation handling. It adds shared idempotency APIs, updates application workflows and repositories, and expands tests for duplicate requests, deferred saves, retries, and persistence boundaries. Transactional idempotency and persistence
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
Ceredron
marked this pull request as ready for review
June 26, 2026 10:08
Ceredron
added
the
kind/chore
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
Test/Altinn.Correspondence.Tests/TestingFeature/DatabaseTransactionHelperTests.cs (1)
122-159: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winThis region is malformed and will not compile.
Two problems here from what looks like a botched merge:
ExecuteAsync_BackgroundJobClientException_RetriesAndSucceeds(Line 123) has no[Fact]attribute and still asserts a retry (attemptCount == 2), which contradicts the new no-retry behavior forBackgroundJobClientException. It is dead and misleading — remove it.ExecuteAsync_PostgreSqlDistributedLockException_DoesNotRetry(Lines 146-159) has a garbled body: it opens with a strayif (attemptCount == 1)and referencesattemptCount,expectedResult, andOperationthat are never declared in this scope, with mismatched braces. This does not compile.The intended
DoesNotRetrytest appears to be duplicated/correctly expressed at Lines 161-177 (despite its name). Delete the dead method and the malformed block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Test/Altinn.Correspondence.Tests/TestingFeature/DatabaseTransactionHelperTests.cs` around lines 122 - 159, The test region is malformed and contains a dead retry test plus a broken duplicate block; remove the obsolete ExecuteAsync_BackgroundJobClientException_RetriesAndSucceeds method since it conflicts with the no-retry behavior, and delete the corrupted ExecuteAsync_PostgreSqlDistributedLockException_DoesNotRetry fragment that references undeclared attemptCount/expectedResult/Operation and has mismatched braces. Keep the valid retry behavior covered only by the correctly formed test in DatabaseTransactionHelperTests, using the existing test method names to locate the cleanup points.src/Altinn.Correspondence.Application/MigrateToStorageProvider/MigrateToStorageProviderHandler.cs (1)
96-102: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSwallowed
SaveChangesAsyncfailure hides migration failures.
SaveChangesAsync(Line 97) is the sole persistence point for bothSetAttachmentSize(Line 90) andSetStorageProvider(Line 96), but thecatchat Line 99-102 only logs a warning and returns normally. A persistence failure here leaves the attachment silently unmigrated with no retry/alert signal. Consider surfacing the failure (rethrow or track failures) rather than swallowing it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Altinn.Correspondence.Application/MigrateToStorageProvider/MigrateToStorageProviderHandler.cs` around lines 96 - 102, The migration flow in MigrateToStorageProviderHandler currently swallows failures from dbContext.SaveChangesAsync after SetAttachmentSize and SetStorageProvider, which can leave attachments unmigrated without surfacing the error. Update the catch block in the attachment processing path to avoid returning success on persistence failure: either rethrow after logging or record the failure so the caller can detect it, and keep the logger.LogWarning for context using attachmentId. Ensure the SaveChangesAsync failure is propagated or otherwise tracked instead of being silently ignored.src/Altinn.Correspondence.Application/CleanupBulkFetchStatuses/CleanupBulkFetchStatusesHandler.cs (1)
76-94: 🩺 Stability & Availability | 🟡 MinorPersist-at-commit changes error semantics for duplicate-deletion loop
DeleteBulkFetchStatusonly stages removals (_context.CorrespondenceFetches.Remove) and never callsSaveChangesinternally. Consequently, the per-iterationtry/catchblock incrementingtotalErrorswill not capture database failures. Any persistence failure surfaces only at the batchdbContext.SaveChangesAsynccall outside that loop, causing the entire job to abort rather than tolerating partial failures.Update the error handling strategy to match the current all-or-nothing persistence model:
- Remove the
try/catchinside the loop and handle errors at the batch level, or- Revert to per-row commits if partial failure tolerance is required.
src/Altinn.Correspondence.Application/CleanupBulkFetchStatuses/CleanupBulkFetchStatusesHandler.cs:84-88
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Altinn.Correspondence.Application/CleanupBulkFetchStatuses/CleanupBulkFetchStatusesHandler.cs` around lines 76 - 94, The duplicate-deletion loop in CleanupBulkFetchStatusesHandler is catching errors too early because DeleteBulkFetchStatus only marks entities for removal and the actual database failure happens later in SaveChangesAsync. Update the error handling around the duplicates processing so it matches the all-or-nothing persistence model: either remove the per-iteration try/catch in CleanupBulkFetchStatusesHandler.Handle and let dbContext.SaveChangesAsync handle failures for the whole batch, or change DeleteBulkFetchStatus/call sites to commit per item if partial success is intended. Use the existing symbols DeleteBulkFetchStatus, Handle, and dbContext.SaveChangesAsync to keep the change aligned with the current flow.src/Altinn.Correspondence.Persistence/Repositories/AttachmentRepository.cs (1)
146-148: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPass the cancellation token to the attachment lookup.
Line 148 ignores the method’s
cancellationToken, so cancellation is not honored during the database read before the staged update.Proposed fix
- var attachment = await _context.Attachments.SingleOrDefaultAsync(a => a.Id == attachmentId); + var attachment = await _context.Attachments.SingleOrDefaultAsync(a => a.Id == attachmentId, cancellationToken);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Altinn.Correspondence.Persistence/Repositories/AttachmentRepository.cs` around lines 146 - 148, The attachment lookup in AttachmentRepository.SetStorageProvider is not honoring the method’s cancellationToken. Update the SingleOrDefaultAsync call on _context.Attachments to pass the cancellationToken so the database read can be cancelled consistently before the update continues.src/Altinn.Correspondence.Persistence/Repositories/ConfidentialReminderRepository.cs (1)
12-20: 🗄️ Data Integrity & Integration | 🟠 MajorCheck tracked reminders before adding another one to prevent duplicates in the ChangeTracker.
Although a unique database constraint exists on
CorrespondenceId, the current implementation fails to check theChangeTrackerwhenSaveChangesis deferred. CallingAddConfidentialRemindertwice for the same correspondence within the sameDbContextwill stage two entities, causing aDbUpdateExceptionupon save.Add a check against the
ChangeTrackerto ensure idempotency and avoid unnecessary database roundtrips or exceptions.Proposed fix
public async Task<Guid> AddConfidentialReminder(ConfidentialReminderEntity reminder, CancellationToken cancellationToken) { + var existingTracked = _context.ChangeTracker + .Entries<ConfidentialReminderEntity>() + .Where(e => e.State != EntityState.Deleted) + .Select(e => e.Entity) + .FirstOrDefault(r => r.CorrespondenceId == reminder.CorrespondenceId); + if (existingTracked != null) + { + return existingTracked.Id; + } + var existing = await _context.ConfidentialReminders .FirstOrDefaultAsync(r => r.CorrespondenceId == reminder.CorrespondenceId, cancellationToken); if (existing != null)Please add a focused test that calls
AddConfidentialRemindertwice in the sameDbContextbeforeSaveChangesAsyncand verifies only one reminder is staged and no exception occurs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Altinn.Correspondence.Persistence/Repositories/ConfidentialReminderRepository.cs` around lines 12 - 20, Update AddConfidentialReminder in ConfidentialReminderRepository to also inspect the current DbContext ChangeTracker for an already-tracked ConfidentialReminderEntity with the same CorrespondenceId before calling AddAsync. If a tracked reminder already exists, return its Id immediately so repeated calls in the same context stay idempotent and do not stage duplicates. Add a focused test around AddConfidentialReminder that invokes it twice on the same context before SaveChangesAsync and asserts only one reminder is tracked/staged and no exception is thrown.
🧹 Nitpick comments (4)
Test/Altinn.Correspondence.Tests/TestingHandler/UnreadConfidentialCorrespondenceReminderHandlerTests.cs (1)
44-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the new persistence boundary with a real staged entity.
Injecting a real context while keeping
IConfidentialReminderRepositorymocked makesSaveChangesAsynca no-op in these tests. Add at least one test using the real repository or a mock callback that attaches the reminder to this context, so the new explicit save is actually exercised.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Test/Altinn.Correspondence.Tests/TestingHandler/UnreadConfidentialCorrespondenceReminderHandlerTests.cs` around lines 44 - 45, The tests around UnreadConfidentialCorrespondenceReminderHandler currently use a real DbContext while keeping IConfidentialReminderRepository mocked, so the explicit SaveChangesAsync path is not actually exercised. Update the test setup in UnreadConfidentialCorrespondenceReminderHandlerTests to use a real repository backed by TestDbContextFactory.Create() or configure the mock to attach the reminder entity to that context before saving. Add at least one test that verifies the persistence boundary in UnreadConfidentialCorrespondenceReminderHandler, ensuring the real save behavior is covered.Test/Altinn.Correspondence.Tests/TestingHandler/PublishCorrespondenceHandlerTests.cs (1)
296-324: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert side effects are suppressed on deferred duplicates.
This test would still pass if Hangfire jobs were enqueued before the deferred unique violation is handled. Add a negative verification for background jobs so the duplicate-on-flush path proves it skips non-DB side effects too.
Proposed assertion
Assert.True(result.IsT0); _idempotencyKeyRepositoryMock.Verify( x => x.CreateAsync(It.IsAny<IdempotencyKeyEntity>(), It.IsAny<CancellationToken>()), Times.AtLeastOnce); +_backgroundJobClientMock.Verify( + x => x.Create(It.IsAny<Job>(), It.IsAny<IState>()), + Times.Never);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Test/Altinn.Correspondence.Tests/TestingHandler/PublishCorrespondenceHandlerTests.cs` around lines 296 - 324, The duplicate-on-flush test for PublishCorrespondenceHandler.Process only checks the result and idempotency write, so it can still pass even if non-DB side effects run; update Process_ShouldSkip_WhenUniqueViolationOnFlush to also verify the BackgroundJobClient mock is not used. Add a negative assertion against _backgroundJobClientMock for the job-enqueue path so this deferred unique violation case proves no Hangfire/background job side effects occur when the flush fails.Test/Altinn.Correspondence.Tests/TestingHandler/PurgeCorrespondenceHelperTests.cs (1)
131-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the staged purge idempotency key shape.
This test would be stronger if it verified the deterministic
Id,CorrespondenceId, andIdempotencyType.PurgeCorrespondence, not only thatCreateAsyncwas called.Suggested assertion tightening
_idempotencyKeyRepositoryMock.Verify( - x => x.CreateAsync(It.IsAny<IdempotencyKeyEntity>(), It.IsAny<CancellationToken>()), + x => x.CreateAsync( + It.Is<IdempotencyKeyEntity>(key => + key.Id == correspondenceId.CreateVersion5("PurgeCorrespondence") && + key.CorrespondenceId == correspondenceId && + key.IdempotencyType == IdempotencyType.PurgeCorrespondence), + It.IsAny<CancellationToken>()), Times.Once);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Test/Altinn.Correspondence.Tests/TestingHandler/PurgeCorrespondenceHelperTests.cs` around lines 131 - 133, The test currently only verifies that CreateAsync is called on _idempotencyKeyRepositoryMock, but it should also assert the staged IdempotencyKeyEntity contents. Update the assertion around PurgeCorrespondenceHelperTests to capture the argument passed to CreateAsync and verify the deterministic Id, CorrespondenceId, and IdempotencyType.PurgeCorrespondence so the test validates the exact idempotency key shape.src/Altinn.Correspondence.Application/Helpers/HangfireScheduleHelper.cs (1)
96-127: 🩺 Stability & Availability | 🔵 TrivialHangfire scheduling inside EF transaction creates a non-atomic dual-write window.
backgroundJobClient.Scheduleexecutes via Hangfire's own storage connection and does not enlist in the surrounding EF Core transaction. This creates a race condition:
- If the EF transaction commits successfully but the Hangfire schedule call fails, the job is never queued, and the state is consistent.
- If the EF transaction fails (rollback) after the Hangfire schedule succeeds, a dangling job is queued that will execute against invalid or missing data.
While
PublishCorrespondenceHandlerincludes a secondary idempotency check to mitigate double-processing, this does not prevent orphaned jobs from running unnecessarily or interacting with partially rolled-back state.Recommendations (choose one):
- Recommended: Move the
backgroundJobClient.Schedulecall outside theDatabaseTransactionHelper.ExecuteAsyncblock to ensure the job is only enqueued after the database changes are permanently committed.- Alternative: If atomicity is strictly required, implement a Transactional Outbox pattern (saving the job request to a table within the same transaction) or use
TransactionScopeif Hangfire shares the exact same database instance.Confirm if the current idempotency-only approach is the intended acceptance criteria for this trade-off.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Altinn.Correspondence.Application/Helpers/HangfireScheduleHelper.cs` around lines 96 - 127, The Hangfire enqueue in HangfireScheduleHelper.SchedulePublishCorrespondence is happening inside DatabaseTransactionHelper.ExecuteAsync, which creates a non-atomic EF/Hangfire dual-write window. Move backgroundJobClient.Schedule<PublishCorrespondenceHandler> out of the transaction block so it runs only after the database commit succeeds, and keep the existing idempotency staging and duplicate handling inside the transaction. If atomic enqueue semantics are required, replace this with a transactional outbox or another commit-safe handoff instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/DownloadCorrespondenceAttachmentHandler.cs`:
- Around line 110-116: The download idempotency key created in
DownloadCorrespondenceAttachmentHandler currently omits the required
IdempotencyType, so it won’t match the earlier lookup that filters for
IdempotencyType.DialogportenActivity. Update the IdempotencyKeyEntity
initialization in the handler to explicitly set IdempotencyType to
DialogportenActivity alongside the existing Id, CorrespondenceId, AttachmentId,
and StatusAction values so subsequent downloads reuse the same key.
In
`@src/Altinn.Correspondence.Application/ExpireAttachment/ExpireAttachmentHandler.cs`:
- Around line 91-108: Move the non-transactional work in ExpireAttachmentHandler
out of the database transaction: first persist the Expired
AttachmentStatusEntity through attachmentStatusRepository and commit that DB
change, then perform storageRepository.PurgeAttachment and
backgroundJobClient.Enqueue<IEventBus> only after the commit succeeds. Update
the ExpireAttachmentHandler flow so PurgeAttachment and event publishing are
executed in a post-commit step or outbox-style callback, preventing retries from
purging blobs or publishing duplicate AttachmentExpired events while the DB
state rolls back.
In
`@src/Altinn.Correspondence.Application/Helpers/CorrespondenceMigrationEventHelper.cs`:
- Around line 294-311: The purge flow in CorrespondenceMigrationEventHelper
currently performs attachment purge work before confirming the new purge
idempotency key is persisted. Move the TrySaveChangesAsync check to immediately
after idempotencyKeyRepository.CreateAsync, using the same purge-correspondence
duplicate message, so duplicate purge requests exit before
StoreDeleteEventAsCorrespondenceStatus, StoreDeleteEventForCorrespondence, and
purgeCorrespondenceHelper.CheckAndPurgeAttachments run.
- Around line 856-868: The duplicate-save handler in TrySaveChangesAsync leaves
failed Added entities tracked in dbContext after a DbUpdateException, so later
SaveChangesAsync calls keep retrying the same insert. Update the catch block for
ex.IsPostgresUniqueViolation() to clear the ChangeTracker on dbContext right
after logger.LogDebug and before returning false; if you need EF Core 6
compatibility, detach the tracked entries manually instead of relying on
ChangeTracker.Clear().
In
`@src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs`:
- Around line 72-82: The idempotent flow in InitializeCorrespondencesHandler
currently schedules Hangfire jobs/events inside InitializeCorrespondences before
DatabaseTransactionHelper.ExecuteAsync has committed the idempotency write, so a
duplicate request can still trigger external side effects for a rolled-back
transaction. Move the job/event enqueueing out of InitializeCorrespondences and
into a post-commit step, or ensure the correspondence and idempotency save is
fully committed before any external scheduling occurs; use the
InitializeCorrespondences and ExecuteAsync flow as the main points to adjust.
In
`@src/Altinn.Correspondence.Application/MigrateCorrespondence/MigrateCorrespondenceHandler.cs`:
- Around line 27-29: The constructor for MigrateCorrespondenceHandler has a
duplicate ApplicationDbContext dbContext parameter, which will not compile;
remove the redundant dbContext declaration from the handler signature and keep
the single intended dependency alongside CorrespondenceMigrationEventHelper.
After updating the constructor, adjust the corresponding
MigrateCorrespondenceHandlerTests wiring so it passes only one
ApplicationDbContext instance to match the revised signature.
In
`@src/Altinn.Correspondence.Application/MigrateCorrespondenceAttachment/MigrateAttachmentHandler.cs`:
- Around line 69-72: The duplicate handling in MigrateAttachmentHandler is too
broad because IsPostgresUniqueViolation currently treats any PostgreSQL 23505 as
an Altinn2AttachmentId duplicate. Update the catch filter in
MigrateAttachmentHandler to only map to MigrateAttachmentAttempt.Duplicate()
when the specific IX_Attachments_Altinn2AttachmentId constraint fails, and
extend DbUpdateExceptionExtensions.IsPostgresUniqueViolation to accept an
optional constraint name and match it against
PostgresException.ConstraintName/TableConstraintName before returning true.
In
`@src/Altinn.Correspondence.Application/PublishCorrespondence/PublishCorrespondenceHandler.cs`:
- Around line 117-127: The null correspondence failure path is unreachable
because PublishCorrespondenceHandler already dereferences correspondence! for
sender, recipient, and external references before this error branch. Move the
correspondence == null guard to immediately after the repository read in
PublishCorrespondenceHandler so the method exits before any dereference, and
keep the existing failure status/logging logic for that early return case.
- Around line 88-98: Move the publish idempotency check in
PublishCorrespondenceHandler before any downstream lookups so duplicate
publishes short-circuit immediately. In the PublishCorrespondenceHandler.Handle
flow, compute publishIdempotencyId from correspondenceId and call
DatabaseTransactionHelper.Idempotency.CheckAsync before any Altinn Register,
KRR, or role validation logic, then return duplicateCheck.DuplicateResult when
IsDuplicate is true. Keep the existing logger.LogInformation skip message, but
ensure all dependency calls happen only after the duplicate check passes.
- Around line 128-135: The side effects in PublishCorrespondenceHandler are
being queued inside the retrying transaction delegate, which can duplicate or
emit work for rolled-back attempts. Move the backgroundJobClient.Enqueue calls
and any event-bus publish/legacy notification scheduling out of the ExecuteAsync
transaction block, and instead capture the intended actions during processing
and execute them only after a successful commit or via an outbox. Apply the same
change to the Slack notification, Dialogporten purge, and any similar
enqueue/publish logic in PublishCorrespondenceHandler.
In
`@src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHelper.cs`:
- Around line 87-107: The purge idempotency reservation in
PurgeCorrespondenceHelper should be committed before any enqueueing or other
non-transactional side effects occur, because staging only inside
PurgeCorrespondence still allows concurrent requests to both pass
DatabaseTransactionHelper.Idempotency.CheckAsync and enqueue jobs. Update the
PurgeCorrespondence flow so the IdempotencyKeyEntity for purgeIdempotencyId is
flushed or otherwise persisted before Hangfire job scheduling, or move the
enqueue logic behind a committed boundary/outbox pattern; keep the
duplicate-check path in place but ensure the reservation is durable before side
effects.
In
`@src/Altinn.Correspondence.Application/SendNotificationOrder/SendNotificationOrderHandler.cs`:
- Around line 111-118: The main notification side effects in
SendNotificationOrderHandler should not run inside
DatabaseTransactionHelper.ExecuteAsync, because SendPublishedEvent and
ScheduleNotificationDeliveryCheck can still fire before the transaction commits.
Move those calls out of the ExecuteAsync callback in
SendNotificationOrderHandler, alongside the existing reminder scheduling change,
and keep only the database update work in UpdateDatabaseNotificationOrder within
the transaction.
In `@src/Altinn.Correspondence.Integrations/Azure/AzureResourceManagerService.cs`:
- Around line 137-138: The Azure deployment flow in AzureResourceManagerService
should not persist the database state only after provisioning, because a
SaveChangesAsync failure can cause retries to create a new storage account and
orphan the previous one. Update the deployment path around
InitializeStorageProvider and SaveChangesAsync to make the DB write idempotent
before retryable Azure work: either create and reuse a deterministic pending
storage-provider record before provisioning, or add cleanup/compensation for the
created resource before rethrowing. Ensure retries can locate the same pending
record instead of generating a new storage account name.
In `@src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs`:
- Line 35: The constructor signature for DialogportenService now requires an
ApplicationDbContext, but DialogportenServiceTests still uses the old argument
list, causing the mockPartyUrnHelper.Object to be passed into the dbContext
parameter. Update the remaining DialogportenService instantiation in
DialogportenServiceTests to supply a proper ApplicationDbContext mock or test
instance in the new slot, and keep the rest of the constructor arguments aligned
with the updated signature.
In
`@src/Altinn.Correspondence.Persistence/Repositories/CorrespondenceDeleteEventRepository.cs`:
- Around line 14-15: The unique-violation catch path in
DatabaseTransactionHelper.ExecuteAsync and callers such as
MigrateAttachmentHandler should clear the EF Core change tracker before
returning, because entities added via AddAsync can remain in the Added state
after a failed SaveChangesAsync and affect later operations in the same scope.
Update the relevant catch blocks to call dbContext.ChangeTracker.Clear() before
the early return, keeping the existing unique-violation handling intact.
In
`@Test/Altinn.Correspondence.Tests/TestingFeature/DatabaseTransactionHelperTests.cs`:
- Around line 161-177: The test name in DatabaseTransactionHelperTests is
misleading: ExecuteAsync_PostgreSqlDistributedLockException_RetriesAndSucceeds
actually verifies that DatabaseTransactionHelper.ExecuteAsync does not retry
because Operation always throws and the assertion checks attemptCount stays at
1. Rename this test to reflect the no-retry behavior, using the unique symbols
ExecuteAsync and PostgreSqlDistributedLockException, so it no longer conflicts
with the true retry-success test nearby.
In
`@Test/Altinn.Correspondence.Tests/TestingRepository/AttachmentRepositoryTests.cs`:
- Around line 186-190: The attachment repository test only checks the in-memory
and reloaded values, so it misses whether SetDataLocationUrl staged the change
before persistence. In AttachmentRepositoryTests, add an assertion on the
tracked Attachment entity state immediately after calling SetDataLocationUrl and
before SaveChangesAsync, using the existing attachment/context setup to verify
the update is pending rather than already persisted. Keep the existing post-save
ReloadAsync assertion so the test covers both deferred staging and final saved
value.
---
Outside diff comments:
In
`@src/Altinn.Correspondence.Application/CleanupBulkFetchStatuses/CleanupBulkFetchStatusesHandler.cs`:
- Around line 76-94: The duplicate-deletion loop in
CleanupBulkFetchStatusesHandler is catching errors too early because
DeleteBulkFetchStatus only marks entities for removal and the actual database
failure happens later in SaveChangesAsync. Update the error handling around the
duplicates processing so it matches the all-or-nothing persistence model: either
remove the per-iteration try/catch in CleanupBulkFetchStatusesHandler.Handle and
let dbContext.SaveChangesAsync handle failures for the whole batch, or change
DeleteBulkFetchStatus/call sites to commit per item if partial success is
intended. Use the existing symbols DeleteBulkFetchStatus, Handle, and
dbContext.SaveChangesAsync to keep the change aligned with the current flow.
In
`@src/Altinn.Correspondence.Application/MigrateToStorageProvider/MigrateToStorageProviderHandler.cs`:
- Around line 96-102: The migration flow in MigrateToStorageProviderHandler
currently swallows failures from dbContext.SaveChangesAsync after
SetAttachmentSize and SetStorageProvider, which can leave attachments unmigrated
without surfacing the error. Update the catch block in the attachment processing
path to avoid returning success on persistence failure: either rethrow after
logging or record the failure so the caller can detect it, and keep the
logger.LogWarning for context using attachmentId. Ensure the SaveChangesAsync
failure is propagated or otherwise tracked instead of being silently ignored.
In `@src/Altinn.Correspondence.Persistence/Repositories/AttachmentRepository.cs`:
- Around line 146-148: The attachment lookup in
AttachmentRepository.SetStorageProvider is not honoring the method’s
cancellationToken. Update the SingleOrDefaultAsync call on _context.Attachments
to pass the cancellationToken so the database read can be cancelled consistently
before the update continues.
In
`@src/Altinn.Correspondence.Persistence/Repositories/ConfidentialReminderRepository.cs`:
- Around line 12-20: Update AddConfidentialReminder in
ConfidentialReminderRepository to also inspect the current DbContext
ChangeTracker for an already-tracked ConfidentialReminderEntity with the same
CorrespondenceId before calling AddAsync. If a tracked reminder already exists,
return its Id immediately so repeated calls in the same context stay idempotent
and do not stage duplicates. Add a focused test around AddConfidentialReminder
that invokes it twice on the same context before SaveChangesAsync and asserts
only one reminder is tracked/staged and no exception is thrown.
In
`@Test/Altinn.Correspondence.Tests/TestingFeature/DatabaseTransactionHelperTests.cs`:
- Around line 122-159: The test region is malformed and contains a dead retry
test plus a broken duplicate block; remove the obsolete
ExecuteAsync_BackgroundJobClientException_RetriesAndSucceeds method since it
conflicts with the no-retry behavior, and delete the corrupted
ExecuteAsync_PostgreSqlDistributedLockException_DoesNotRetry fragment that
references undeclared attemptCount/expectedResult/Operation and has mismatched
braces. Keep the valid retry behavior covered only by the correctly formed test
in DatabaseTransactionHelperTests, using the existing test method names to
locate the cleanup points.
---
Nitpick comments:
In `@src/Altinn.Correspondence.Application/Helpers/HangfireScheduleHelper.cs`:
- Around line 96-127: The Hangfire enqueue in
HangfireScheduleHelper.SchedulePublishCorrespondence is happening inside
DatabaseTransactionHelper.ExecuteAsync, which creates a non-atomic EF/Hangfire
dual-write window. Move
backgroundJobClient.Schedule<PublishCorrespondenceHandler> out of the
transaction block so it runs only after the database commit succeeds, and keep
the existing idempotency staging and duplicate handling inside the transaction.
If atomic enqueue semantics are required, replace this with a transactional
outbox or another commit-safe handoff instead.
In
`@Test/Altinn.Correspondence.Tests/TestingHandler/PublishCorrespondenceHandlerTests.cs`:
- Around line 296-324: The duplicate-on-flush test for
PublishCorrespondenceHandler.Process only checks the result and idempotency
write, so it can still pass even if non-DB side effects run; update
Process_ShouldSkip_WhenUniqueViolationOnFlush to also verify the
BackgroundJobClient mock is not used. Add a negative assertion against
_backgroundJobClientMock for the job-enqueue path so this deferred unique
violation case proves no Hangfire/background job side effects occur when the
flush fails.
In
`@Test/Altinn.Correspondence.Tests/TestingHandler/PurgeCorrespondenceHelperTests.cs`:
- Around line 131-133: The test currently only verifies that CreateAsync is
called on _idempotencyKeyRepositoryMock, but it should also assert the staged
IdempotencyKeyEntity contents. Update the assertion around
PurgeCorrespondenceHelperTests to capture the argument passed to CreateAsync and
verify the deterministic Id, CorrespondenceId, and
IdempotencyType.PurgeCorrespondence so the test validates the exact idempotency
key shape.
In
`@Test/Altinn.Correspondence.Tests/TestingHandler/UnreadConfidentialCorrespondenceReminderHandlerTests.cs`:
- Around line 44-45: The tests around
UnreadConfidentialCorrespondenceReminderHandler currently use a real DbContext
while keeping IConfidentialReminderRepository mocked, so the explicit
SaveChangesAsync path is not actually exercised. Update the test setup in
UnreadConfidentialCorrespondenceReminderHandlerTests to use a real repository
backed by TestDbContextFactory.Create() or configure the mock to attach the
reminder entity to that context before saving. Add at least one test that
verifies the persistence boundary in
UnreadConfidentialCorrespondenceReminderHandler, ensuring the real save behavior
is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 410f7501-a659-4849-ae94-a7e1f812f84c
📒 Files selected for processing (55)
Test/Altinn.Correspondence.Tests/Dialogporten/DialogportenServiceTests.csTest/Altinn.Correspondence.Tests/Fixtures/UniqueViolationOnDeferredSaveDbContext.csTest/Altinn.Correspondence.Tests/Helpers/TestDbContextFactory.csTest/Altinn.Correspondence.Tests/TestingController/Correspondence/CorrespondenceInitializationTests.csTest/Altinn.Correspondence.Tests/TestingFeature/DatabaseTransactionHelperTests.csTest/Altinn.Correspondence.Tests/TestingHandler/CleanupBulkFetchStatusesHandlerTests.csTest/Altinn.Correspondence.Tests/TestingHandler/CreateNotificationOrderHandlerTests.csTest/Altinn.Correspondence.Tests/TestingHandler/ExpireAttachmentHandlerTests.csTest/Altinn.Correspondence.Tests/TestingHandler/HangfireScheduleHelperTests.csTest/Altinn.Correspondence.Tests/TestingHandler/InitializeCorrespondencesHandlerTests.csTest/Altinn.Correspondence.Tests/TestingHandler/MigrateCorrespondenceHandlerTests.csTest/Altinn.Correspondence.Tests/TestingHandler/PublishCorrespondenceHandlerTests.csTest/Altinn.Correspondence.Tests/TestingHandler/PurgeCorrespondenceHelperTests.csTest/Altinn.Correspondence.Tests/TestingHandler/SendNotificationOrderHandlerTests.csTest/Altinn.Correspondence.Tests/TestingHandler/UnreadConfidentialCorrespondenceReminderHandlerTests.csTest/Altinn.Correspondence.Tests/TestingRepository/AttachmentRepositoryTests.cssrc/Altinn.Correspondence.Application/CleanupBulkFetchStatuses/CleanupBulkFetchStatusesHandler.cssrc/Altinn.Correspondence.Application/CreateNotificationOrder/CreateNotificationOrderHandler.cssrc/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/DownloadCorrespondenceAttachmentHandler.cssrc/Altinn.Correspondence.Application/ExpireAttachment/ExpireAttachmentHandler.cssrc/Altinn.Correspondence.Application/Helpers/AttachmentHelper.cssrc/Altinn.Correspondence.Application/Helpers/CorrespondenceMigrationEventHelper.cssrc/Altinn.Correspondence.Application/Helpers/DatabaseTransactionHelper.cssrc/Altinn.Correspondence.Application/Helpers/HangfireScheduleHelper.cssrc/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondenceValidationHelper.cssrc/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cssrc/Altinn.Correspondence.Application/InitializeServiceOwner/InitializeServiceOwnerHandler.cssrc/Altinn.Correspondence.Application/MigrateCorrespondence/MigrateCorrespondenceHandler.cssrc/Altinn.Correspondence.Application/MigrateCorrespondenceAttachment/MigrateAttachmentHandler.cssrc/Altinn.Correspondence.Application/MigrateToStorageProvider/MigrateToStorageProviderHandler.cssrc/Altinn.Correspondence.Application/ProcessLegacyParty/ProcessLegacyPartyHandler.cssrc/Altinn.Correspondence.Application/PublishCorrespondence/PublishCorrespondenceHandler.cssrc/Altinn.Correspondence.Application/PurgeCorrespondence/LegacyPurgeCorrespondenceHandler.cssrc/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHandler.cssrc/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHelper.cssrc/Altinn.Correspondence.Application/PurgeDialogAndDeleteReminderForReadCorrespondences/PurgeDialogAndDeleteReminderForReadCorrespondencesHandler.cssrc/Altinn.Correspondence.Application/SendNotificationOrder/SendNotificationOrderHandler.cssrc/Altinn.Correspondence.Application/UnreadConfidentialCorrespondenceReminder/UnreadConfidentialCorrespondenceReminderHandler.cssrc/Altinn.Correspondence.Core/Models/Enums/IdempotencyType.cssrc/Altinn.Correspondence.Core/Repositories/IAttachmentRepository.cssrc/Altinn.Correspondence.Integrations/Azure/AzureResourceManagerService.cssrc/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cssrc/Altinn.Correspondence.Persistence/Helpers/CorrespondenceNpgsqlRetryingExecutionStrategy.cssrc/Altinn.Correspondence.Persistence/Helpers/DbUpdateExceptionExtensions.cssrc/Altinn.Correspondence.Persistence/Repositories/AttachmentRepository.cssrc/Altinn.Correspondence.Persistence/Repositories/AttachmentStatusRepository.cssrc/Altinn.Correspondence.Persistence/Repositories/ConfidentialReminderRepository.cssrc/Altinn.Correspondence.Persistence/Repositories/CorrespondenceDeleteEventRepository.cssrc/Altinn.Correspondence.Persistence/Repositories/CorrespondenceForwardingEventRepository.cssrc/Altinn.Correspondence.Persistence/Repositories/CorrespondenceNotificationRepository.cssrc/Altinn.Correspondence.Persistence/Repositories/CorrespondenceRepository.cssrc/Altinn.Correspondence.Persistence/Repositories/CorrespondenceStatusRepository.cssrc/Altinn.Correspondence.Persistence/Repositories/IdempotencyKeyRepository.cssrc/Altinn.Correspondence.Persistence/Repositories/LegacyPartyRepository.cssrc/Altinn.Correspondence.Persistence/Repositories/ServiceOwnerRepository.cs
💤 Files with no reviewable changes (2)
- src/Altinn.Correspondence.Persistence/Repositories/ServiceOwnerRepository.cs
- src/Altinn.Correspondence.Persistence/Repositories/CorrespondenceRepository.cs
| ILogger<DialogportenService> logger, | ||
| IIdempotencyKeyRepository _idempotencyKeyRepository, | ||
| IResourceRegistryService _resourceRegistryService, | ||
| ApplicationDbContext dbContext, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Update the remaining constructor call site.
DialogportenServiceTests.cs:185-194 still constructs DialogportenService without the new ApplicationDbContext argument, so that call now passes mockPartyUrnHelper.Object into the dbContext slot and will not compile.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs`
at line 35, The constructor signature for DialogportenService now requires an
ApplicationDbContext, but DialogportenServiceTests still uses the old argument
list, causing the mockPartyUrnHelper.Object to be passed into the dbContext
parameter. Update the remaining DialogportenService instantiation in
DialogportenServiceTests to supply a proper ApplicationDbContext mock or test
instance in the new slot, and keep the rest of the constructor arguments aligned
with the updated signature.
Comment on lines
+14
to
+15
| await _context.AddAsync(correspondenceDeleteEventEntity, cancellationToken); | ||
| return correspondenceDeleteEventEntity.Id; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect moved unique-violation handling for cleanup of failed EF Core entries.
# Expectation: duplicate handlers that catch IsPostgresUniqueViolation also detach/clear rejected Added entries.
rg -n -C 4 'IsPostgresUniqueViolation|OnUniqueViolation|EntityState\.Detached|ChangeTracker\.Clear|State\s*=\s*EntityState\.Detached' \
src/Altinn.Correspondence.Application \
src/Altinn.Correspondence.Persistence \
TestRepository: Altinn/altinn-correspondence
Length of output: 11944
Clear tracker on unique violation in higher-level handlers.
The catch blocks in DatabaseTransactionHelper.ExecuteAsync and callers like MigrateAttachmentHandler return on unique violation but do not clear the ChangeTracker. An entity added via AddAsync before the failing SaveChangesAsync remains tracked in the Added state, potentially poisoning the scoped context for subsequent operations. Add dbContext.ChangeTracker.Clear() in the catch block before returning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/Altinn.Correspondence.Persistence/Repositories/CorrespondenceDeleteEventRepository.cs`
around lines 14 - 15, The unique-violation catch path in
DatabaseTransactionHelper.ExecuteAsync and callers such as
MigrateAttachmentHandler should clear the EF Core change tracker before
returning, because entities added via AddAsync can remain in the Added state
after a failed SaveChangesAsync and affect later operations in the same scope.
Update the relevant catch blocks to call dbContext.ChangeTracker.Clear() before
the early return, keeping the existing unique-violation handling intact.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Test/Altinn.Correspondence.Tests/TestingFeature/DatabaseTransactionHelperTests.cs (1)
122-143: 🩺 Stability & Availability | 🟠 MajorRemove dead test
ExecuteAsync_BackgroundJobClientException_RetriesAndSucceeds: missing[Fact]and contradicts retry contractThe method lacks the
[Fact]attribute (line 122 is blank), preventing xUnit discovery. Even if enabled, it would fail becauseCorrespondenceNpgsqlRetryingExecutionStrategy.ShouldRetryOnexplicitly returnsfalseforBackgroundJobClientException. The operation will not retry, making the assertionAssert.Equal(2, attemptCount)impossible. This test duplicates the negative case correctly covered byExecuteAsync_BackgroundJobClientException_DoesNotRetryand must be removed.🧹 Suggested removal
- - public async Task ExecuteAsync_BackgroundJobClientException_RetriesAndSucceeds() - { - var expectedResult = Guid.NewGuid(); - var attemptCount = 0; - await using var dbContext = CreateDbContext(); - Task<Guid> Operation(CancellationToken ct) - { - attemptCount++; - - if (attemptCount == 1) - { - throw new BackgroundJobClientException("Hangfire job creation failed", new Exception("Inner exception")); - } - return Task.FromResult(expectedResult); - } - - var result = await DatabaseTransactionHelper.ExecuteAsync(dbContext, Operation); - - Assert.Equal(expectedResult, result); - Assert.Equal(2, attemptCount); - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Test/Altinn.Correspondence.Tests/TestingFeature/DatabaseTransactionHelperTests.cs` around lines 122 - 143, Remove the obsolete test method ExecuteAsync_BackgroundJobClientException_RetriesAndSucceeds from DatabaseTransactionHelperTests, since it is not discoverable without [Fact] and its retry expectation conflicts with CorrespondenceNpgsqlRetryingExecutionStrategy.ShouldRetryOn, which does not retry BackgroundJobClientException. Keep the existing negative coverage in ExecuteAsync_BackgroundJobClientException_DoesNotRetry and delete this duplicate so the test suite matches the actual retry contract.
♻️ Duplicate comments (1)
Test/Altinn.Correspondence.Tests/TestingFeature/DatabaseTransactionHelperTests.cs (1)
162-178: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRedundant test:
ExecuteAsync_PostgreSqlDistributedLockException_RetriesAndSucceedsnow duplicates..._DoesNotRetry.Its body and assertions (
attemptCount == 1, throwsPostgreSqlDistributedLockException) are identical to the new..._DoesNotRetrytest at Line 146 and no longer verify retry/success. Remove this redundant copy. (Previously flagged as a misnamed test.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Test/Altinn.Correspondence.Tests/TestingFeature/DatabaseTransactionHelperTests.cs` around lines 162 - 178, The test ExecuteAsync_PostgreSqlDistributedLockException_RetriesAndSucceeds is now redundant because it duplicates ExecuteAsync_PostgreSqlDistributedLockException_DoesNotRetry in both behavior and assertions. Remove this extra test from DatabaseTransactionHelperTests so only the non-retry case remains, and keep the existing ExecuteAsync/DatabaseTransactionHelper coverage focused on distinct behaviors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@Test/Altinn.Correspondence.Tests/TestingFeature/DatabaseTransactionHelperTests.cs`:
- Around line 122-143: Remove the obsolete test method
ExecuteAsync_BackgroundJobClientException_RetriesAndSucceeds from
DatabaseTransactionHelperTests, since it is not discoverable without [Fact] and
its retry expectation conflicts with
CorrespondenceNpgsqlRetryingExecutionStrategy.ShouldRetryOn, which does not
retry BackgroundJobClientException. Keep the existing negative coverage in
ExecuteAsync_BackgroundJobClientException_DoesNotRetry and delete this duplicate
so the test suite matches the actual retry contract.
---
Duplicate comments:
In
`@Test/Altinn.Correspondence.Tests/TestingFeature/DatabaseTransactionHelperTests.cs`:
- Around line 162-178: The test
ExecuteAsync_PostgreSqlDistributedLockException_RetriesAndSucceeds is now
redundant because it duplicates
ExecuteAsync_PostgreSqlDistributedLockException_DoesNotRetry in both behavior
and assertions. Remove this extra test from DatabaseTransactionHelperTests so
only the non-retry case remains, and keep the existing
ExecuteAsync/DatabaseTransactionHelper coverage focused on distinct behaviors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4bffd774-3171-47a8-9a75-9dcf1c41832c
📒 Files selected for processing (5)
Test/Altinn.Correspondence.Tests/Dialogporten/DialogportenServiceTests.csTest/Altinn.Correspondence.Tests/Helpers/AttachmentHelper.csTest/Altinn.Correspondence.Tests/TestingFeature/DatabaseTransactionHelperTests.csTest/Altinn.Correspondence.Tests/TestingHandler/MigrateCorrespondenceHandlerTests.cssrc/Altinn.Correspondence.Application/MigrateCorrespondence/MigrateCorrespondenceHandler.cs
💤 Files with no reviewable changes (2)
- src/Altinn.Correspondence.Application/MigrateCorrespondence/MigrateCorrespondenceHandler.cs
- Test/Altinn.Correspondence.Tests/TestingHandler/MigrateCorrespondenceHandlerTests.cs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Test/Altinn.Correspondence.Tests/Helpers/CustomWebApplicationFactory.cs (1)
50-50: 🚀 Performance & Scalability | 🔵 TrivialVerify Hangfire worker count and polling intervals against database pool size.
The test host configuration creates a high risk of database contention.
Test/Altinn.Correspondence.Tests/Helpers/CustomWebApplicationFactory.csline 50 setsMaximum Pool Size=50, yet line 82 starts 30 workers while line 68 removes queue backoff (TimeSpan.Zero) and line 81 sets the scheduler polling interval to 50ms.A single factory instance can consume 60% of the available connection pool with workers alone, leaving minimal headroom for other operations. This configuration is likely to cause flaky timeouts if the test suite runs in parallel.
Adjust
Test/Altinn.Correspondence.Tests/Helpers/CustomWebApplicationFactory.cs:
- Reduce
WorkerCountto5or10.- Increase
SchedulePollingIntervalto1000msor higher.- Set
QueuePollIntervalto at least500ms.Apply to lines 68-68 and 81-82.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Test/Altinn.Correspondence.Tests/Helpers/CustomWebApplicationFactory.cs` at line 50, The test host configuration in CustomWebApplicationFactory is oversubscribing the database pool and can cause flaky contention; reduce the Hangfire WorkerCount from the current high value to a small number such as 5 or 10, and slow down the polling settings by increasing the scheduler polling interval and QueuePollInterval to safer values. Update the Hangfire server setup in CustomWebApplicationFactory so the worker count and polling intervals are aligned with the DatabaseOptions:ConnectionString pool size and leave enough headroom for other test operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Test/Altinn.Correspondence.Tests/Helpers/CustomWebApplicationFactory.cs`:
- Line 50: The test host configuration in CustomWebApplicationFactory is
oversubscribing the database pool and can cause flaky contention; reduce the
Hangfire WorkerCount from the current high value to a small number such as 5 or
10, and slow down the polling settings by increasing the scheduler polling
interval and QueuePollInterval to safer values. Update the Hangfire server setup
in CustomWebApplicationFactory so the worker count and polling intervals are
aligned with the DatabaseOptions:ConnectionString pool size and leave enough
headroom for other test operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22bcb084-800c-41c2-8a1b-56dbd54e12a6
📒 Files selected for processing (3)
Test/Altinn.Correspondence.Tests/Helpers/AttachmentHelper.csTest/Altinn.Correspondence.Tests/Helpers/CorrespondenceHelper.csTest/Altinn.Correspondence.Tests/Helpers/CustomWebApplicationFactory.cs
Ceredron
had a problem deploying
to
use-case-test
August 4, 2026 08:26 — with 
GitHub Actions
Failure
GitHub Actions
Failure
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/Altinn.Correspondence.Persistence/Repositories/AttachmentRepository.cs (1)
148-157: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPass
cancellationTokento the attachment lookup.After removing
SaveChangesAsync(cancellationToken),SetStorageProviderno longer observes the cancellation token while awaiting the database query. A cancelled job can continue waiting for theSingleOrDefaultAsynccall to complete.Proposed fix
- var attachment = await _context.Attachments.SingleOrDefaultAsync(a => a.Id == attachmentId); + var attachment = await _context.Attachments.SingleOrDefaultAsync( + a => a.Id == attachmentId, + cancellationToken);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Altinn.Correspondence.Persistence/Repositories/AttachmentRepository.cs` around lines 148 - 157, Update SetStorageProvider to pass its cancellationToken argument to SingleOrDefaultAsync when looking up the attachment, ensuring the database query observes cancellation while preserving the existing missing-attachment handling and field updates.src/Altinn.Correspondence.Application/CreateNotificationOrder/CreateNotificationOrderHandler.cs (2)
26-26: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winUpdate the direct
CreateNotificationOrderHandlerconstruction in tests.
Test/Altinn.Correspondence.Tests/TestingHandler/CreateNotificationOrderHandlerTests.csstill creates the handler with the old signature at line 375.CreateNotificationOrderHandlernow requiresCustomRecipientDeduplicationHelperas the third constructor argument, so the test code must pass the mocked/wired helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Altinn.Correspondence.Application/CreateNotificationOrder/CreateNotificationOrderHandler.cs` at line 26, Update the direct CreateNotificationOrderHandler construction in CreateNotificationOrderHandlerTests to supply a mocked or wired CustomRecipientDeduplicationHelper as the third constructor argument, matching the handler’s current constructor signature while preserving the existing test dependencies.
247-258: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSet
IsReminderon standalone reminder notifications.
CreateNotificationOrderHandler.cspersists every order withIsReminder = false, while existing reminder pathSendNotificationOrderHandler.cspersists reminders withIsReminder = trueand delivery/status users branch on that flag. SinceNotificationOrderRequestV2has noIsReminderfield, set the entity flag from the recipient plan when creating the standalone reminder order.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Altinn.Correspondence.Application/CreateNotificationOrder/CreateNotificationOrderHandler.cs` around lines 247 - 258, Update the standalone reminder creation in CreateNotificationOrderHandler, specifically the NotificationOrderRequestV2 branch guarded by plan.IncludeReminder, so the persisted notification order receives IsReminder = true when its recipient plan represents a reminder. Use the existing recipient-plan reminder information when mapping the request to the entity, without adding IsReminder to NotificationOrderRequestV2 or changing the main notification path.src/Altinn.Correspondence.Application/PublishCorrespondence/PublishCorrespondenceHandler.cs (1)
177-236: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRestore the
ExecuteAsynccall structure.Line 177 closes the
OnDuplicatelambda but does not closeOnDuplicateorExecuteAsync. Lines 179-236 then occur outside a valid method expression. The project does not compile.Keep one transaction delegate. If Lines 101-170 are intended, change Line 177 to
}));and delete Lines 179-236.Proposed fix
- } + }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Altinn.Correspondence.Application/PublishCorrespondence/PublishCorrespondenceHandler.cs` around lines 177 - 236, Restore the ExecuteAsync call structure in PublishCorrespondenceHandler by closing the OnDuplicate transaction delegate at the existing boundary with the required closing syntax. Keep a single transaction delegate, and remove the duplicated status/error handling block after that boundary so the code compiles within one valid method expression.Source: Linters/SAST tools
🧹 Nitpick comments (2)
src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs (2)
238-249: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the
Tasksuccess value with a unit-like type.
ValidateDialogOrTransmissionJobreturnsOneOf<Task, Error>, and the caller at lines 140-144 only checksIsT1. The returnedTask.CompletedTaskis never awaited, soTaskacts as an unused placeholder. ReturnError?orOneOf<Success, Error>to make the contract explicit.♻️ Proposed signature change
- private async Task<OneOf<Task, Error>> ValidateDialogOrTransmissionJob( + private async Task<Error?> ValidateDialogOrTransmissionJob( CorrespondenceEntity correspondence, InitializeCorrespondencesRequest request, CancellationToken cancellationToken) { if (!correspondence.ExternalReferences.Any(er => er.ReferenceType == ReferenceType.DialogportenDialogId)) { - return Task.CompletedTask; + return null; } - - return await initializeCorrespondenceValidationHelper.ValidateTransmissionRequest(correspondence, request, cancellationToken); + + var result = await initializeCorrespondenceValidationHelper.ValidateTransmissionRequest(correspondence, request, cancellationToken); + return result.IsT1 ? result.AsT1 : null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs` around lines 238 - 249, Update ValidateDialogOrTransmissionJob and its caller to replace the unused Task success value with an explicit unit-like success representation, such as Error? or OneOf<Success, Error>. Preserve the existing validation error propagation and ensure the caller’s IsT1 success check is updated to match the new result contract.
89-92: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle failures of post-commit scheduling.
The transaction is already committed when this loop runs. If
scheduleJobs()throws, the correspondence rows stay persisted and no publish, dialog, or notification job exists for them. The exception also stops the remaining actions in the loop, so later correspondences in the same request are never scheduled.Add per-action error isolation with logging, so one scheduling failure does not silently drop the rest. For durability, consider persisting the intent (outbox row) inside the transaction and letting a recurring job pick up unscheduled correspondences.
♻️ Proposed error isolation
- foreach (var scheduleJobs in commitResult.AsT0.PostCommitActions) - { - await scheduleJobs(); - } + foreach (var scheduleJobs in commitResult.AsT0.PostCommitActions) + { + try + { + await scheduleJobs(); + } + catch (Exception ex) + { + logger.LogError(ex, "Failed to schedule post-commit jobs after correspondence initialization"); + throw; + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs` around lines 89 - 92, Update the post-commit action loop in InitializeCorrespondencesHandler to execute each scheduleJobs delegate inside its own error boundary, logging failures with sufficient context while continuing to invoke subsequent actions. Preserve the committed transaction result and, if the existing design supports durable handoff, persist scheduling intent within that transaction for later retry rather than relying solely on post-commit execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/Altinn.Correspondence.Application/ExpireAttachment/ExpireAttachmentHandler.cs`:
- Around line 110-125: The ExpireAttachmentHandler flow currently commits
Expired before executing side effects, allowing failures to suppress retries.
Add a durable outbox or pending-side-effect record within the same transaction
that records attachment purge and AttachmentExpired publication, then process
and retry each independently until completion; ensure the early
Expired/idempotency path still runs unfinished side effects rather than
returning immediately.
In
`@src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs`:
- Around line 160-166: Update the AttachmentIds projection in
InitializeCorrespondencesHandler to provide an empty inner sequence when
c.Content is null, ensuring SelectMany always enumerates a non-null sequence
while preserving the existing distinct ID list behavior.
In `@src/Altinn.Correspondence.Core/Models/Enums/IdempotencyType.cs`:
- Around line 44-45: Add the missing opening `/// <summary>` tag immediately
before the existing Hangfire publish job description for
`SchedulePublishCorrespondence`, keeping the closing `</summary>` and
documentation text properly paired.
- Around line 42-46: The IdempotencyType numeric values are being renumbered
while existing IdempotencyKeys rows store these values as integers. Preserve
SchedulePublishCorrespondence at its historical value 6, or add a data migration
that remaps existing IdempotencyKeys.IdempotencyType values of 6 before
introducing NotificationDeliveryResolution at 6; ensure deployed rows retain
their intended enum meaning.
In `@src/Altinn.Correspondence.Integrations/Azure/AzureResourceManagerService.cs`:
- Around line 99-107: Move the InitializeStorageProvider and SaveChangesAsync
calls in the AzureResourceManagerService provisioning flow to execute only after
Azure resource-group and storage-account creation completes successfully. Keep
the existing storageType, serviceOwner, existingProvider, and deterministic
storage-account-name logic unchanged, and ensure failed provisioning does not
leave a usable provider record persisted.
- Around line 403-409: Update GetDeterministicStorageAccountName to use a
16-character hexadecimal hash suffix instead of 8 characters, preserving the
existing deterministic input and account-name prefix/suffix.
In `@Test/Altinn.Correspondence.Tests/Helpers/CustomWebApplicationFactory.cs`:
- Line 82: Update the Hangfire configuration in CustomWebApplicationFactory to
explicitly set a test-only WorkerCount alongside SchedulePollingInterval, using
the established worker-count value or convention already used by this test
assembly.
---
Outside diff comments:
In
`@src/Altinn.Correspondence.Application/CreateNotificationOrder/CreateNotificationOrderHandler.cs`:
- Line 26: Update the direct CreateNotificationOrderHandler construction in
CreateNotificationOrderHandlerTests to supply a mocked or wired
CustomRecipientDeduplicationHelper as the third constructor argument, matching
the handler’s current constructor signature while preserving the existing test
dependencies.
- Around line 247-258: Update the standalone reminder creation in
CreateNotificationOrderHandler, specifically the NotificationOrderRequestV2
branch guarded by plan.IncludeReminder, so the persisted notification order
receives IsReminder = true when its recipient plan represents a reminder. Use
the existing recipient-plan reminder information when mapping the request to the
entity, without adding IsReminder to NotificationOrderRequestV2 or changing the
main notification path.
In
`@src/Altinn.Correspondence.Application/PublishCorrespondence/PublishCorrespondenceHandler.cs`:
- Around line 177-236: Restore the ExecuteAsync call structure in
PublishCorrespondenceHandler by closing the OnDuplicate transaction delegate at
the existing boundary with the required closing syntax. Keep a single
transaction delegate, and remove the duplicated status/error handling block
after that boundary so the code compiles within one valid method expression.
In `@src/Altinn.Correspondence.Persistence/Repositories/AttachmentRepository.cs`:
- Around line 148-157: Update SetStorageProvider to pass its cancellationToken
argument to SingleOrDefaultAsync when looking up the attachment, ensuring the
database query observes cancellation while preserving the existing
missing-attachment handling and field updates.
---
Nitpick comments:
In
`@src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs`:
- Around line 238-249: Update ValidateDialogOrTransmissionJob and its caller to
replace the unused Task success value with an explicit unit-like success
representation, such as Error? or OneOf<Success, Error>. Preserve the existing
validation error propagation and ensure the caller’s IsT1 success check is
updated to match the new result contract.
- Around line 89-92: Update the post-commit action loop in
InitializeCorrespondencesHandler to execute each scheduleJobs delegate inside
its own error boundary, logging failures with sufficient context while
continuing to invoke subsequent actions. Preserve the committed transaction
result and, if the existing design supports durable handoff, persist scheduling
intent within that transaction for later retry rather than relying solely on
post-commit execution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 585deaec-dfa9-46a3-b85b-b9e3b6693dc8
📒 Files selected for processing (20)
Test/Altinn.Correspondence.Tests/Dialogporten/DialogportenServiceTests.csTest/Altinn.Correspondence.Tests/Helpers/CustomWebApplicationFactory.csTest/Altinn.Correspondence.Tests/TestingController/Correspondence/CorrespondenceInitializationTests.csTest/Altinn.Correspondence.Tests/TestingHandler/CreateNotificationOrderHandlerTests.csTest/Altinn.Correspondence.Tests/TestingHandler/PublishCorrespondenceHandlerTests.csTest/Altinn.Correspondence.Tests/TestingHandler/SendNotificationOrderHandlerTests.csTest/Altinn.Correspondence.Tests/TestingHandler/UnreadConfidentialCorrespondenceReminderHandlerTests.csTest/Altinn.Correspondence.Tests/TestingRepository/AttachmentRepositoryTests.cssrc/Altinn.Correspondence.Application/CreateNotificationOrder/CreateNotificationOrderHandler.cssrc/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/DownloadCorrespondenceAttachmentHandler.cssrc/Altinn.Correspondence.Application/ExpireAttachment/ExpireAttachmentHandler.cssrc/Altinn.Correspondence.Application/Helpers/DatabaseTransactionHelper.cssrc/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cssrc/Altinn.Correspondence.Application/PublishCorrespondence/PublishCorrespondenceHandler.cssrc/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHelper.cssrc/Altinn.Correspondence.Core/Models/Enums/IdempotencyType.cssrc/Altinn.Correspondence.Integrations/Azure/AzureResourceManagerService.cssrc/Altinn.Correspondence.Persistence/Repositories/AttachmentRepository.cssrc/Altinn.Correspondence.Persistence/Repositories/CorrespondenceRepository.cssrc/Altinn.Correspondence.Persistence/Repositories/IdempotencyKeyRepository.cs
💤 Files with no reviewable changes (1)
- src/Altinn.Correspondence.Persistence/Repositories/CorrespondenceRepository.cs
🚧 Files skipped from review as they are similar to previous changes (11)
- Test/Altinn.Correspondence.Tests/TestingHandler/UnreadConfidentialCorrespondenceReminderHandlerTests.cs
- src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/DownloadCorrespondenceAttachmentHandler.cs
- Test/Altinn.Correspondence.Tests/TestingRepository/AttachmentRepositoryTests.cs
- Test/Altinn.Correspondence.Tests/Dialogporten/DialogportenServiceTests.cs
- src/Altinn.Correspondence.Persistence/Repositories/IdempotencyKeyRepository.cs
- Test/Altinn.Correspondence.Tests/TestingController/Correspondence/CorrespondenceInitializationTests.cs
- Test/Altinn.Correspondence.Tests/TestingHandler/CreateNotificationOrderHandlerTests.cs
- Test/Altinn.Correspondence.Tests/TestingHandler/SendNotificationOrderHandlerTests.cs
- src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHelper.cs
- src/Altinn.Correspondence.Application/Helpers/DatabaseTransactionHelper.cs
- Test/Altinn.Correspondence.Tests/TestingHandler/PublishCorrespondenceHandlerTests.cs
Comment on lines
+110
to
+125
| if (!committed) | ||
| { | ||
| return Task.CompletedTask; | ||
| } | ||
|
|
||
| await storageRepository.PurgeAttachment(attachment.Id, attachment.StorageProvider, cancellationToken); | ||
| await storageRepository.PurgeAttachment(attachment.Id, attachment.StorageProvider, cancellationToken); | ||
|
|
||
| backgroundJobClient.Enqueue<IEventBus>((eventBus) => eventBus.Publish( | ||
| AltinnEventType.AttachmentExpired, | ||
| attachment.ResourceId, | ||
| attachment.Id.ToString(), | ||
| "attachment", | ||
| attachment.Sender, | ||
| CancellationToken.None)); | ||
| backgroundJobClient.Enqueue<IEventBus>((eventBus) => eventBus.Publish( | ||
| AltinnEventType.AttachmentExpired, | ||
| attachment.ResourceId, | ||
| attachment.Id.ToString(), | ||
| "attachment", | ||
| attachment.Sender, | ||
| CancellationToken.None)); | ||
|
|
||
| logger.LogInformation("Successfully expired attachment {AttachmentId} with filename {FileName}", attachmentId, attachment.FileName); | ||
| return Task.CompletedTask; | ||
| }, cancellationToken); | ||
| return Task.CompletedTask; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make post-commit side effects durable and retryable.
The transaction commits the Expired status and idempotency key before Line 115. If PurgeAttachment or Enqueue throws, a retry returns at Lines 38-42 because the attachment is already expired. The blob purge or AttachmentExpired event can then remain incomplete.
Write a durable outbox or pending-side-effect record in the transaction. Retry purge and event publication independently until completion. Do not let Expired suppress unfinished side effects.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/Altinn.Correspondence.Application/ExpireAttachment/ExpireAttachmentHandler.cs`
around lines 110 - 125, The ExpireAttachmentHandler flow currently commits
Expired before executing side effects, allowing failures to suppress retries.
Add a durable outbox or pending-side-effect record within the same transaction
that records attachment purge and AttachmentExpired publication, then process
and retry each independently until completion; ensure the early
Expired/idempotency path still runs unfinished side effects rather than
returning immediately.
Comment on lines
+160
to
+166
| return new InitializeCorrespondencesCommitResult( | ||
| new InitializeCorrespondencesResponse() | ||
| { | ||
| Correspondences = initializedCorrespondences, | ||
| AttachmentIds = correspondences.SelectMany(c => c.Content?.Attachments.Select(a => a.AttachmentId)).Distinct().ToList() | ||
| }, | ||
| postCommitActions); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the nullable inner sequence in SelectMany.
c.Content?.Attachments.Select(...) returns null when Content is null. SelectMany then throws while enumerating the null inner sequence. The rest of this file treats Content as nullable (line 187 and line 191), so the same assumption should hold here.
🛡️ Proposed fix
- AttachmentIds = correspondences.SelectMany(c => c.Content?.Attachments.Select(a => a.AttachmentId)).Distinct().ToList()
+ AttachmentIds = correspondences
+ .SelectMany(c => c.Content?.Attachments.Select(a => a.AttachmentId) ?? Enumerable.Empty<Guid>())
+ .Distinct()
+ .ToList()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return new InitializeCorrespondencesCommitResult( | |
| new InitializeCorrespondencesResponse() | |
| { | |
| Correspondences = initializedCorrespondences, | |
| AttachmentIds = correspondences.SelectMany(c => c.Content?.Attachments.Select(a => a.AttachmentId)).Distinct().ToList() | |
| }, | |
| postCommitActions); | |
| return new InitializeCorrespondencesCommitResult( | |
| new InitializeCorrespondencesResponse() | |
| { | |
| Correspondences = initializedCorrespondences, | |
| AttachmentIds = correspondences | |
| .SelectMany(c => c.Content?.Attachments.Select(a => a.AttachmentId) ?? Enumerable.Empty<Guid>()) | |
| .Distinct() | |
| .ToList() | |
| }, | |
| postCommitActions); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs`
around lines 160 - 166, Update the AttachmentIds projection in
InitializeCorrespondencesHandler to provide an empty inner sequence when
c.Content is null, ensuring SelectMany always enumerates a non-null sequence
while preserving the existing distinct ID list behavior.
Comment on lines
42
to
+46
| NotificationDeliveryResolution = 6, | ||
| } No newline at end of file | ||
|
|
||
| /// Indicates that a Hangfire publish job has been scheduled for a correspondence | ||
| /// </summary> | ||
| SchedulePublishCorrespondence = 7, |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
ast-grep outline src/Altinn.Correspondence.Core/Models/Enums/IdempotencyType.cs --items all
rg -n -C 6 --glob '*.cs' \
'\bIdempotencyType\b|IdempotencyKeyEntity|HasConversion|SchedulePublishCorrespondence|NotificationDeliveryResolution' \
src TestRepository: Altinn/altinn-correspondence
Length of output: 50385
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Locate and inspect the enum definition plus configuration/repository usages around IdempotencyType.
fd -a 'IdempotencyType\.cs|IdempotencyKeyEntity\.cs|Idempotency.*Configuration|Idempotency.*Repository' src Test | sed 's#^\./##'
echo '--- IdempotencyType enum ---'
sed -n '1,120p' src/Altinn.Correspondence.Core/Models/Enums/IdempotencyType.cs
echo '--- IdempotencyKeyEntity enum property ---'
fd 'IdempotencyKeyEntity.cs' src | while read -r f; do
echo "FILE: $f"
ast-grep outline "$f" --match IdempotencyKeyEntity --view expanded || true
rg -n -C 4 'IdempotencyType' "$f"
done
echo '--- DbContext/config HasConversion and idempotency migrations ---'
rg -n --glob '*.cs' --glob '*.csproj' \
'\bIdempotencyKeyEntity\b|\bIdempotencyType\b|HasConversion\(\)|Configure\(' src/Altinn.Correspondence src/*/* src/* src/*/*/ | head -n 200
echo '--- migration lines mentioning IdempotencyKeyEntity and conversion (limited) ---'
rg -n -C 3 --glob '*.cs' \
'modelBuilder\.Entity\("Altinn\.Correspondence\.Core\.Models\.Entities\.IdempotencyKeyEntity"|Property<"IdempotencyType".*HasConversion|IdempotencyType.*HasConversion' src/Altinn.Correspondence.Persistence/Migrations | head -n 200Repository: Altinn/altinn-correspondence
Length of output: 3504
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo '--- IdempotencyKeyConfiguration.cs ---'
sed -n '1,160p' src/Altinn.Correspondence.Persistence/Configurations/IdempotencyKeyConfiguration.cs
echo '--- Idempotency migrations files ---'
for f in src/Altinn.Correspondence.Persistence/Migrations/20250408123318_IdempotencyKeyEntity.cs \
src/Altinn.Correspondence.Persistence/Migrations/20250521132251_AddIdempotencyType.cs; do
echo "FILE: $f"
sed -n '1,180p' "$f"
done
echo '--- IdempotencyKeyRepository.cs ---'
ast-grep outline src/Altinn.Correspondence.Persistence/Repositories/IdempotencyKeyRepository.cs --items all || true
sed -n '1,240p' src/Altinn.Correspondence.Persistence/Repositories/IdempotencyKeyRepository.cs
echo '--- targeted usages/new rows with SchedulePublishCorrespondence ---'
rg -n -C 4 --glob '*.cs' \
'IdempotencyType\.SchedulePublishCorrespondence|SchedulePublishCorrespondence|PublishCorrespondence|IdempotencyType\.PublishCorrespondence|IdempotencyType\.NotificationDeliveryResolution' \
src Test
echo '--- diff stat/name status ---'
git diff --stat
git diff --name-statusRepository: Altinn/altinn-correspondence
Length of output: 50384
Add a data migration for the IdempotencyType value change.
IdempotencyType is stored as int in IdempotencyKeys, and this config does not convert it. Existing production rows with value 6 will be read as NotificationDeliveryResolution after NotificationDeliveryResolution takes 6 and SchedulePublishCorrespondence moves to 7. Keep value 6 stable for SchedulePublishCorrespondence, or add a migration that remaps existing IdempotencyKeys.IdempotencyType = 6 before deployment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Altinn.Correspondence.Core/Models/Enums/IdempotencyType.cs` around lines
42 - 46, The IdempotencyType numeric values are being renumbered while existing
IdempotencyKeys rows store these values as integers. Preserve
SchedulePublishCorrespondence at its historical value 6, or add a data migration
that remaps existing IdempotencyKeys.IdempotencyType values of 6 before
introducing NotificationDeliveryResolution at 6; ensure deployed rows retain
their intended enum meaning.
Comment on lines
+44
to
+45
| /// Indicates that a Hangfire publish job has been scheduled for a correspondence | ||
| /// </summary> |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching IdempotencyType.cs:"
fd -a 'IdempotencyType\.cs$' . || true
file="$(fd 'IdempotencyType\.cs$' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
echo
echo "File: $file"
wc -l "$file"
echo
echo "Last 70 lines with line numbers:"
cat -n "$file" | sed -n '1,110p'
fi
echo
echo "XML doc tags in file:"
if [ -n "${file:-}" ]; then
rg -n '^\s*///\s*</?summary|^\s*///\s*</?|Indentations' "$file" || true
fiRepository: Altinn/altinn-correspondence
Length of output: 2735
Add the opening <summary> documentation element.
SchedulePublishCorrespondence has </summary> on line 45 but no matching /// <summary> body text for it.
Proposed fix
+ /// <summary>
/// Indicates that a Hangfire publish job has been scheduled for a correspondence
/// </summary>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Indicates that a Hangfire publish job has been scheduled for a correspondence | |
| /// </summary> | |
| /// <summary> | |
| /// Indicates that a Hangfire publish job has been scheduled for a correspondence | |
| /// </summary> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Altinn.Correspondence.Core/Models/Enums/IdempotencyType.cs` around lines
44 - 45, Add the missing opening `/// <summary>` tag immediately before the
existing Hangfire publish job description for `SchedulePublishCorrespondence`,
keeping the closing `</summary>` and documentation text properly paired.
Comment on lines
+99
to
+107
| var storageType = virusScan ? StorageProviderType.Altinn3Azure : StorageProviderType.Altinn3AzureWithoutVirusScan; | ||
|
|
||
| var serviceOwner = await _serviceOwnerRepository.GetServiceOwnerByOrgNo(serviceOwnerEntity.Id, cancellationToken); | ||
| var existingProvider = serviceOwner?.StorageProviders.FirstOrDefault(sp => sp.Type == storageType); | ||
| var storageAccountName = existingProvider?.StorageResourceName | ||
| ?? GetDeterministicStorageAccountName(serviceOwnerEntity.Id, virusScan); | ||
|
|
||
| await _serviceOwnerRepository.InitializeStorageProvider(serviceOwnerEntity.Id, storageAccountName, storageType); | ||
| await _dbContext.SaveChangesAsync(cancellationToken); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the repository implementation and inspect InitializeStorageProvider.
fd -t f 'ServiceOwnerRepository.cs' | xargs -r ast-grep outline --items all
fd -t f 'ServiceOwnerRepository.cs' | while IFS= read -r f; do
rg -n -C 15 'InitializeStorageProvider' "$f"
done
# Find consumers that resolve storage providers for blob operations.
rg -nP -C 6 '\bStorageProviders\b|GetStorageProvider' --type=cs -g '!**/Test/**'Repository: Altinn/altinn-correspondence
Length of output: 3086
🏁 Script executed:
#!/bin/bash
set -e
echo "== repo outline =="
fd -t f 'ServiceOwnerRepository.cs|AzureResourceManagerService.cs' | while IFS= read -r f; do
echo "--- $f"
wc -l "$f"
done
echo "== ServiceOwnerRepository implementation =="
cat -n src/Altinn.Correspondence.Persistence/Repositories/ServiceOwnerRepository.cs | sed -n '1,120p'
echo "== AzureResourceManagerService relevant section =="
cat -n src/Altinn.Correspondence.Integrations/Azure/AzureResourceManagerService.cs | sed -n '80,155p'
echo "== storage provider entity and index definitions =="
rg -n -C 8 'class StorageProviderEntity|StorageProviders|StorageResourceName|StorageProviderType' src --type=cs -g '!**/Test/**'Repository: Altinn/altinn-correspondence
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -e
echo "== InitializeStorageProvider implementation =="
cat -n src/Altinn.Correspondence.Persistence/Repositories/ServiceOwnerRepository.cs | sed -n '40,75p'
echo "== AzureResourceManagerService provisioning flow =="
cat -n src/Altinn.Correspondence.Integrations/Azure/AzureResourceManagerService.cs | sed -n '80,145p'
echo "== StorageProviderEntity model =="
fd -t f 'StorageProviderEntity.cs' | while IFS= read -r f; do
echo "--- $f"
cat -n "$f"
done
echo "== Deterministic provider lookup occurrences =="
rg -n -C 5 'FirstOrDefault\(sp => sp\.Type == storageType\)|StorageProviders\.FirstOrDefault|GetStorageProvider|GetStorageProviderAsync|StorageProvider' src --type=cs -g '!**/Test/**' -g '!**/Migrations/**' -g '!**/obj/**' -g '!**/bin/**'
echo "== ServiceOwnerEntity StorageProviders model =="
fd -t f 'ServiceOwnerEntity.cs' | while IFS= read -r f; do
echo "--- $f"
rg -n -A 20 -B 5 'StorageProviders' "$f"
doneRepository: Altinn/altinn-correspondence
Length of output: 50384
Create the provider only after provisioning succeeds.
InitializeStorageProvider is already idempotent for the same StorageResourceName and storageType, so the duplicate-row concern does not apply. However, the provider row is saved before Azure creates the resource group/storage account, so consumers of that provider can see a record that points at an Azure storage account that was not created. Save the provider only after creation succeeds, or add a pending/not-ready flag/constraint and guard blob operations by provisioning state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Altinn.Correspondence.Integrations/Azure/AzureResourceManagerService.cs`
around lines 99 - 107, Move the InitializeStorageProvider and SaveChangesAsync
calls in the AzureResourceManagerService provisioning flow to execute only after
Azure resource-group and storage-account creation completes successfully. Keep
the existing storageType, serviceOwner, existingProvider, and deterministic
storage-account-name logic unchanged, and ensure failed provisioning does not
leave a usable provider record persisted.
Comment on lines
+403
to
+409
| private static string GetDeterministicStorageAccountName(string serviceOwnerId, bool virusScan) | ||
| { | ||
| var hashInput = $"{serviceOwnerId}:{virusScan}"; | ||
| var hash = SHA256.HashData(Encoding.UTF8.GetBytes(hashInput)); | ||
| var suffix = Convert.ToHexString(hash)[..8].ToLowerInvariant(); | ||
| return "aicorr" + suffix + "sa"; | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm GenerateStorageAccountName has no remaining callers.
rg -nP -C 3 '\bGenerateStorageAccountName\s*\(' --type=csRepository: Altinn/altinn-correspondence
Length of output: 166
🏁 Script executed:
#!/bin/bash
set -u
echo "== locate file =="
git ls-files | rg 'AzureResourceManagerService\.cs$|src/Altinn\.Correspondence\.Integrations/Azure/AzureResourceManagerService\.cs$' || true
echo "== file outline =="
ast-grep outline src/Altinn.Correspondence.Integrations/Azure/AzureResourceManagerService.cs 2>/dev/null || true
echo "== relevant lines =="
nl -ba src/Altinn.Correspondence.Integrations/Azure/AzureResourceManagerService.cs | sed -n '380,435p'
echo "== all calls/definitions =="
rg -n -C 2 '\b(GetDeterministicStorageAccountName|GenerateStorageAccountName|storageAccountName|Create|storage account|StorageAccount)\b' src/Altinn.Correspondence.Integrations/Azure/AzureResourceManagerService.cs || trueRepository: Altinn/altinn-correspondence
Length of output: 7014
🏁 Script executed:
#!/bin/bash
set -u
echo "== locate file =="
git ls-files | rg 'AzureResourceManagerService\.cs$' || true
echo "== relevant lines =="
nl -ba src/Altinn.Correspondence.Integrations/Azure/AzureResourceManagerService.cs | sed -n '380,445p'
echo "== deterministic storage name definitions/usages =="
rg -n -C 2 'GetDeterministicStorageAccountName|GenerateStorageAccountName' src/Altinn.Correspondence.Integrations/Azure/AzureResourceManagerService.cs || true
echo "== all repo usages =="
rg -n -C 2 'GetDeterministicStorageAccountName|GenerateStorageAccountName' --type=cs || trueRepository: Altinn/altinn-correspondence
Length of output: 1143
🌐 Web query:
Azure storage account naming requirements characters lower case numbers 3-24 globally unique
💡 Result:
Azure storage account names must adhere to the following requirements: - Length: Between 3 and 24 characters [1][2]. - Characters: Must contain only lowercase letters and numbers [1][2]. - Uniqueness: Must be globally unique across all of Azure [2]. If a name includes uppercase letters or special characters, it will result in an invalid account name error [2]. Because the name must be globally unique, no two storage accounts within Azure can share the same name [1][2].
Citations:
- 1: https://learn.microsoft.com/en-us/azure/storage/common/storage-account-overview
- 2: https://learn.microsoft.com/en-us/azure/azure-resource-manager/troubleshooting/error-storage-account-name
Widen the deterministic storage account suffix.
GetDeterministicStorageAccountName uses only 8 hex characters, which gives 32 bits of entropy in the global Azure storage account namespace. A collision is permanent because the name is now deterministic. A 16-character hex suffix remains within the 3- to 24-character Azure storage account name limit and increases collision resistance to 64 bits.
🛡️ Proposed fix
private static string GetDeterministicStorageAccountName(string serviceOwnerId, bool virusScan)
{
var hashInput = $"{serviceOwnerId}:{virusScan}";
var hash = SHA256.HashData(Encoding.UTF8.GetBytes(hashInput));
- var suffix = Convert.ToHexString(hash)[..8].ToLowerInvariant();
+ var suffix = Convert.ToHexString(hash)[..16].ToLowerInvariant();
return "aicorr" + suffix + "sa";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private static string GetDeterministicStorageAccountName(string serviceOwnerId, bool virusScan) | |
| { | |
| var hashInput = $"{serviceOwnerId}:{virusScan}"; | |
| var hash = SHA256.HashData(Encoding.UTF8.GetBytes(hashInput)); | |
| var suffix = Convert.ToHexString(hash)[..8].ToLowerInvariant(); | |
| return "aicorr" + suffix + "sa"; | |
| } | |
| private static string GetDeterministicStorageAccountName(string serviceOwnerId, bool virusScan) | |
| { | |
| var hashInput = $"{serviceOwnerId}:{virusScan}"; | |
| var hash = SHA256.HashData(Encoding.UTF8.GetBytes(hashInput)); | |
| var suffix = Convert.ToHexString(hash)[..16].ToLowerInvariant(); | |
| return "aicorr" + suffix + "sa"; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Altinn.Correspondence.Integrations/Azure/AzureResourceManagerService.cs`
around lines 403 - 409, Update GetDeterministicStorageAccountName to use a
16-character hexadecimal hash suffix instead of 8 characters, preserving the
existing deterministic input and account-name prefix/suffix.
| @@ -79,7 +80,6 @@ protected override void ConfigureWebHost( | |||
| services.AddHangfireServer(options => | |||
| { | |||
| options.SchedulePollingInterval = TimeSpan.FromSeconds(1); | |||
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C 4 \
'WorkerCount|QueuePollInterval|SchedulePollingInterval|WaitFor|WaitUntil|Timeout|TimeSpan\.From' \
Test/Altinn.Correspondence.Tests/Helpers \
Test/Altinn.Correspondence.Tests/TestingHandler \
Test/Altinn.Correspondence.Tests/TestingControllerRepository: Altinn/altinn-correspondence
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate CustomWebApplicationFactory =="
fd -a 'CustomWebApplicationFactory\.cs$' .
echo "== Outline helper files =="
ast-grep outline Test/Altinn.Correspondence.Tests/Helpers/CustomWebApplicationFactory.cs --view expanded || true
echo "== Read CustomWebApplicationFactory =="
cat -n Test/Altinn.Correspondence.Tests/Helpers/CustomWebApplicationFactory.cs
echo "== Search WorkerCount usage globally =="
rg -n 'WorkerCount|UseSimpleAssemblyName|UseSqlStorage|QueuePoll|SchedulePollingInterval|GetDefaultWorkerCount|DefaultWorker' .Repository: Altinn/altinn-correspondence
Length of output: 18915
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Dependency injection paths =="
cat -n src/Altinn.Correspondence.Integrations/Hangfire/DependencyInjection.cs | sed -n '1,120p'
echo "== Search for deterministic wait helpers across tests/helpers =="
rg -n 'WaitFor|WaitUntil|Task\.Delay|Thread\.Sleep|Timeout|Assert\.Wait|Poll|PollUntil|Stopwatch|StopAsync|RunAsync|AddHangfireServer|Hangfire' \
Test/Altinn.Correspondence.Tests \
Test -g '*.cs' \
--glob '!**/obj/**' --glob '!**/bin/**' --max-count 200Repository: Altinn/altinn-correspondence
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== CorrespondenceHelper poll implementation =="
cat -n Test/Altinn.Correspondence.Tests/Helpers/CorrespondenceHelper.cs | sed -n '1,150p'
echo "== AttachmentHelper poll implementation =="
cat -n Test/Altinn.Correspondence.Tests/Helpers/AttachmentHelper.cs | sed -n '1,160p'
echo "== Precise WorkerCount occurrences across cs files =="
rg -n 'WorkerCount|BackgroundJobServerOptions|AddHangfireServer\(' . -g '*.cs' --glob '!**/obj/**' --glob '!**/bin/**'Repository: Altinn/altinn-correspondence
Length of output: 18822
Set a test-only Hangfire worker count.
CustomWebApplicationFactory no longer sets WorkerCount, so the test server now uses Hangfire’s default. This test assembly already uses explicit worker counts where concurrency/ordering is relevant, so add an explicit test-only value for this configuration too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Test/Altinn.Correspondence.Tests/Helpers/CustomWebApplicationFactory.cs` at
line 82, Update the Hangfire configuration in CustomWebApplicationFactory to
explicitly set a test-only WorkerCount alongside SchedulePollingInterval, using
the established worker-count value or convention already used by this test
assembly.
6 tasks
Ceredron
had a problem deploying
to
use-case-test
August 10, 2026 08:34 — with
GitHub Actions
Failure
GitHub Actions
Failure
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Test/Altinn.Correspondence.Tests/TestingHandler/SendNotificationOrderHandlerTests.cs (1)
189-208: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAssert the behavior promised by this test.
CreateAsyncruns before the deferred save, soTimes.AtLeastOnceonly proves that staging was attempted. It does not prove that the unique-violation path skipped the reminder operation. Assert that the reminder operation is not retried and that only the main delivery check is scheduled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Test/Altinn.Correspondence.Tests/TestingHandler/SendNotificationOrderHandlerTests.cs` around lines 189 - 208, Update Process_ShouldSkipReminderPersist_WhenUniqueViolationOnFlush to verify the unique-violation path skips the reminder operation rather than merely staging idempotency creation. Assert that the reminder operation is not retried and that only the main delivery check is scheduled, using the relevant mock verifications for SendNotificationOrderHandler.src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs (1)
809-822: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReuse existing idempotency keys before creating new keys.
CreateCorrespondenceDialogcan retry after an external “already exists” response. Each retry calls this helper again.This helper always stages new open, confirm, and attachment keys. The retry can then create duplicate keys or fail on the unique constraint before it reaches the external duplicate handling. It also loses the stable activity IDs required for idempotency.
Load existing keys first. Create and save only missing keys. Return the existing or newly created IDs. Add a retry test that invokes dialog creation twice for the same correspondence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs` around lines 809 - 822, Update the idempotency-key creation helper used by CreateCorrespondenceDialog to load existing open, confirm, and attachment keys for the correspondence before generating IDs; create and persist only missing keys, preserving existing activity IDs and returning both existing and newly created IDs. Add a retry test that invokes dialog creation twice for the same correspondence and verifies no duplicate keys are created.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs`:
- Around line 809-822: Update the idempotency-key creation helper used by
CreateCorrespondenceDialog to load existing open, confirm, and attachment keys
for the correspondence before generating IDs; create and persist only missing
keys, preserving existing activity IDs and returning both existing and newly
created IDs. Add a retry test that invokes dialog creation twice for the same
correspondence and verifies no duplicate keys are created.
In
`@Test/Altinn.Correspondence.Tests/TestingHandler/SendNotificationOrderHandlerTests.cs`:
- Around line 189-208: Update
Process_ShouldSkipReminderPersist_WhenUniqueViolationOnFlush to verify the
unique-violation path skips the reminder operation rather than merely staging
idempotency creation. Assert that the reminder operation is not retried and that
only the main delivery check is scheduled, using the relevant mock verifications
for SendNotificationOrderHandler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3730ef16-3da1-41c2-9905-280182d7ef52
📒 Files selected for processing (22)
Test/Altinn.Correspondence.Tests/Dialogporten/DialogportenServiceTests.csTest/Altinn.Correspondence.Tests/Helpers/AttachmentHelper.csTest/Altinn.Correspondence.Tests/Helpers/CorrespondenceHelper.csTest/Altinn.Correspondence.Tests/TestingController/Correspondence/CorrespondenceInitializationTests.csTest/Altinn.Correspondence.Tests/TestingHandler/ExpireAttachmentHandlerTests.csTest/Altinn.Correspondence.Tests/TestingHandler/PublishCorrespondenceHandlerTests.csTest/Altinn.Correspondence.Tests/TestingHandler/SendNotificationOrderHandlerTests.csTest/Altinn.Correspondence.Tests/TestingRepository/AttachmentRepositoryTests.cssrc/Altinn.Correspondence.Application/CreateNotificationOrder/CreateNotificationOrderHandler.cssrc/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/DownloadCorrespondenceAttachmentHandler.cssrc/Altinn.Correspondence.Application/Helpers/AttachmentHelper.cssrc/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondenceValidationHelper.cssrc/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cssrc/Altinn.Correspondence.Application/InitializeServiceOwner/InitializeServiceOwnerHandler.cssrc/Altinn.Correspondence.Application/PublishCorrespondence/PublishCorrespondenceHandler.cssrc/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHandler.cssrc/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHelper.cssrc/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cssrc/Altinn.Correspondence.Persistence/Helpers/CorrespondenceNpgsqlRetryingExecutionStrategy.cssrc/Altinn.Correspondence.Persistence/Repositories/AttachmentRepository.cssrc/Altinn.Correspondence.Persistence/Repositories/CorrespondenceForwardingEventRepository.cssrc/Altinn.Correspondence.Persistence/Repositories/CorrespondenceRepository.cs
💤 Files with no reviewable changes (1)
- src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHelper.cs
🚧 Files skipped from review as they are similar to previous changes (18)
- src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHandler.cs
- Test/Altinn.Correspondence.Tests/Helpers/AttachmentHelper.cs
- Test/Altinn.Correspondence.Tests/Dialogporten/DialogportenServiceTests.cs
- src/Altinn.Correspondence.Application/CreateNotificationOrder/CreateNotificationOrderHandler.cs
- src/Altinn.Correspondence.Application/Helpers/AttachmentHelper.cs
- src/Altinn.Correspondence.Persistence/Helpers/CorrespondenceNpgsqlRetryingExecutionStrategy.cs
- Test/Altinn.Correspondence.Tests/TestingHandler/ExpireAttachmentHandlerTests.cs
- src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/DownloadCorrespondenceAttachmentHandler.cs
- src/Altinn.Correspondence.Application/PublishCorrespondence/PublishCorrespondenceHandler.cs
- Test/Altinn.Correspondence.Tests/TestingHandler/PublishCorrespondenceHandlerTests.cs
- Test/Altinn.Correspondence.Tests/TestingRepository/AttachmentRepositoryTests.cs
- src/Altinn.Correspondence.Persistence/Repositories/CorrespondenceRepository.cs
- src/Altinn.Correspondence.Application/InitializeServiceOwner/InitializeServiceOwnerHandler.cs
- Test/Altinn.Correspondence.Tests/TestingController/Correspondence/CorrespondenceInitializationTests.cs
- src/Altinn.Correspondence.Persistence/Repositories/AttachmentRepository.cs
- Test/Altinn.Correspondence.Tests/Helpers/CorrespondenceHelper.cs
- src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondenceValidationHelper.cs
- src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs
Collaborator
Author
|
Resurrected here: |
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When we call SaveChanges mid-transaction a round-trip is made to the server. In most cases this was not necessary. Furthermore, we were very inconsistent with where we used it. Changed the code never perform SaveChanges in a repository method. Instead, repository methods only stage changes for the transaction that is eventually committed or occasionally when needed to ensure duplicate etc.
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Reliability
Bug Fixes