Cleanup job for Missing Notification Events in Dialogporten - #2018
Open
RagnarFatland wants to merge 21 commits into
Open
Cleanup job for Missing Notification Events in Dialogporten#2018RagnarFatland wants to merge 21 commits into
RagnarFatland wants to merge 21 commits into
Conversation
RagnarFatland
added
the
kind/chore
Contributor
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
RagnarFatland
temporarily deployed
to
use-case-test
June 18, 2026 14:45 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
added 8 commits
June 23, 2026 14:45
… MigrationController to MaintenanceController.
RagnarFatland
temporarily deployed
to
use-case-test
June 25, 2026 14:58 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
RagnarFatland
temporarily deployed
to
use-case-test
June 25, 2026 15:09 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
added 2 commits
June 25, 2026 17:27
…ltinn/altinn-correspondence into fix/cleanupjobforNotifications
RagnarFatland
temporarily deployed
to
use-case-test
June 25, 2026 15:28 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
![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: 15
🧹 Nitpick comments (7)
src/Altinn.Correspondence.Persistence/Repositories/CorrespondenceNotificationRepository.cs (2)
180-185: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAdd a non-positive
countguard.The sibling method
GetAltinn3NotificationDeliveryRepairCandidatesreturns early whenlimit <= 0(line 118). For consistency and to avoidTake(0)/negative-value surprises, guardcounthere too (return an emptyCorrespondencesWithNotificationsBatch).🤖 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/CorrespondenceNotificationRepository.cs` around lines 180 - 185, Add a non-positive count guard in GetCorrespondencesWithSyncedNotifications, matching the early-return behavior used by GetAltinn3NotificationDeliveryRepairCandidates. Check count at the start of the method and, when it is zero or negative, return an empty CorrespondencesWithNotificationsBatch immediately instead of continuing into the query pipeline.
180-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared cursor filter to avoid divergence.
Lines 186-202 duplicate the exact filter/cursor logic from
GetSyncedNotificationsWithoutDialogActivityBatch(lines 155-171). Extract a privateIQueryable<CorrespondenceNotificationEntity>helper applying the synced predicate + composite cursor so the two methods cannot drift apart. TheNULLNotificationSentconcern noted above applies here as well.🤖 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/CorrespondenceNotificationRepository.cs` around lines 180 - 202, The cursor and synced-status filtering in GetCorrespondencesWithSyncedNotifications duplicates the logic already used by GetSyncedNotificationsWithoutDialogActivityBatch, which risks the two paths drifting apart. Extract the shared synced predicate plus composite cursor condition into a private IQueryable<CorrespondenceNotificationEntity> helper and have both methods call it, keeping the NotificationSent null-handling consistent in that shared place.src/Altinn.Correspondence.Persistence/Migrations/20260623113729_AddNotificationCleanupIndex.cs (1)
27-33: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftBlocking
CREATE INDEXon a ~918M-row table will lock writes during deploy.
migrationBuilder.CreateIndexissues a non-concurrentCREATE INDEX, taking anACCESS EXCLUSIVE/share lock that blocks writes for the duration on this very large table. The comment recommendsCONCURRENTLY, but the migration as written does not do that, andCREATE INDEX CONCURRENTLYcannot run inside the migration's transaction anyway. Recommend creating this index manually withCONCURRENTLYout-of-band (as the comment describes) and making this migration a no-op /IF NOT EXISTS, or documenting that the migration must be applied during a maintenance window. The column order and partial filter correctly match the cursor query — only the rollout mechanism is the concern.🤖 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/Migrations/20260623113729_AddNotificationCleanupIndex.cs` around lines 27 - 33, The new index created in the AddNotificationCleanupIndex migration will block writes because CreateIndex runs non-concurrently on the large CorrespondenceNotifications table. Update the migration to avoid issuing a regular CreateIndex here; instead make the migration a no-op or guarded IF NOT EXISTS path and handle the index creation out-of-band with a manual CONCURRENTLY rollout, or explicitly document that this migration must be applied in a maintenance window. Keep the existing index definition in AddNotificationCleanupIndex aligned with the cleanup query, but change the rollout mechanism only.src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs (1)
1304-1316: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid N+1 notification fetches in the bulk path.
Each notification is loaded with its own DB round-trip inside the loop. For correspondences with many notifications this serializes N queries per group. Consider a single batched lookup (e.g., a
GetNotificationsByIds/WHERE Id IN (...)repository method) to load all notifications for the group at 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 `@src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs` around lines 1304 - 1316, The bulk notification loading in DialogportenService is doing one repository call per notification ID inside the loop, creating an N+1 pattern. Update the notification lookup path around the notificationIds iteration to use a single batched repository call such as a GetNotificationsByIds method or an equivalent WHERE Id IN query, then populate the notifications list from that result set. Keep the existing missing-notification logging behavior by comparing requested IDs against the batched results and logging warnings for any IDs not returned.src/Altinn.Correspondence.Application/CleanupMissingSyncedNotificationsBatch/CleanupMissingSyncedNotificationsBatchHandler.cs (1)
52-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead
ProcessCorrespondenceNotificationsstub.This method only throws
NotImplementedExceptionand, per its own comment, the real work lives inIDialogportenService.AddNotificationActivitiesWithDuplicateCheck(which is what the job actually enqueues). Leaving an[AutomaticRetry(Attempts = 0)]Hangfire-shaped method that always throws is a maintenance hazard—any accidental enqueue would fail hard. Drop it to avoid confusion.🤖 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/CleanupMissingSyncedNotificationsBatch/CleanupMissingSyncedNotificationsBatchHandler.cs` around lines 52 - 63, Remove the dead ProcessCorrespondenceNotifications stub from CleanupMissingSyncedNotificationsBatchHandler; it only throws NotImplementedException and is not the real worker. Delete the unused Hangfire-shaped method and its [AutomaticRetry(Attempts = 0)] attribute, and keep the actual notification handling in IDialogportenService.AddNotificationActivitiesWithDuplicateCheck so there is no accidental enqueue target left behind.src/Altinn.Correspondence.API/Controllers/MaintenanceController.cs (1)
467-467: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
handler.Process(...)returns aTaskthat is neither awaited nor observed.The action method is synchronous (
ActionResult) and discards the returnedTask. TodayProcesscompletes synchronously (enqueue +Task.CompletedTask), so this happens to work, but it silently swallows any exception thrown before the awaitable yields and will become a fire-and-forget bug ifProcessever does real async work. Make the actionasync Task<ActionResult>andawaitit.Proposed change
- public ActionResult CleanupMissingSyncedNotificationEvents( + public async Task<ActionResult> CleanupMissingSyncedNotificationEvents( [FromServices] CleanupMissingSyncedNotificationsBatchHandler handler, [FromQuery] int batchCount = 100, [FromQuery] DateTimeOffset? startDate = null, [FromQuery] Guid? startId = null) { @@ - handler.Process(batchCount, processFromDate, startId); + await handler.Process(batchCount, processFromDate, startId);🤖 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.API/Controllers/MaintenanceController.cs` at line 467, The MaintenanceController action is discarding the Task returned by handler.Process, so exceptions can be missed and future async work will become fire-and-forget. Update the action that calls Process to be async Task<ActionResult>, then await handler.Process(batchCount, processFromDate, startId) instead of calling it synchronously.src/Altinn.Correspondence.Application/CleanupMissingSyncedNotificationsBatch/CleanupMissingSyncedNotificationsBatchJob.cs (1)
26-54: 🚀 Performance & Scalability | 🔵 TrivialRemove redundant database query in
ProcessBatchAsync.The
ChainedBatchJobOrchestratorpasses the items fetched byFetchBatchAsyncdirectly toProcessBatchAsync(seeChainedBatchJobOrchestrator.csline 59). The current implementation ignores this data and executes a duplicate query on lines 50‑54 with identicalrequestparameters. This doubles database load and introduces a race condition where data might change between the initial fetch and the re-fetch. Use theitemsparameter provided by the framework instead of re-querying metadata that should be resolved via state or derived from the list itself.🤖 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/CleanupMissingSyncedNotificationsBatch/CleanupMissingSyncedNotificationsBatchJob.cs` around lines 26 - 54, Remove the duplicate database call in CleanupMissingSyncedNotificationsBatchJob’s ProcessBatchAsync: it currently re-queries GetCorrespondencesWithSyncedNotifications with the same request parameters already used by FetchBatchAsync. Use the items parameter passed into ProcessBatchAsync (from ChainedBatchJobOrchestrator) and derive any needed batch state from that data instead of refetching, while keeping cursor/state handling consistent with the existing batch job flow.
🤖 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 `@docs/notification-cleanup-batch-boundary-solutions.md`:
- Around line 97-104: The enqueue example still references the old batch handler
type, so update the example to use the cleanup-specific maintenance handler name
used by the new cleanup flow. Adjust the `backgroundJobClient.Enqueue` snippet
to point to the correct handler entrypoint so readers are directed to the same
path as `MaintenanceController` rather than
`MigrateNotificationEventsBatchHandler`.
- Around line 53-76: The cursor filter in
GetSyncedNotificationsWithoutDialogActivityBatch should match the nullable Guid
handling used by the repository logic. Update the documentation sample so the
query does not compare n.Id directly to lastProcessedId; instead, add an if
(lastProcessedId.HasValue) branch that applies the timestamp-or-id cursor using
lastProcessedId.Value, and an else branch that filters only by
n.NotificationSent < lastProcessedTimestamp.
In `@docs/notification-cleanup-migration-guide.md`:
- Around line 13-17: The migration guide still refers to the stale batch handler
name, so update the handler reference in this section and the related-files list
to the current cleanup batch handler used by MaintenanceController. Use the
cleanup handler name consistently throughout the runbook so it matches the
current maintenance flow and the symbols in the maintenance endpoint
implementation.
- Around line 66-75: The pg_relation_size lookup in the pg_indexes query is
using an unquoted qualified name, which can break for mixed-case index names.
Update the size expression to build the identifier with quote_ident(schemaname)
and quote_ident(indexname) so the lookup matches the exact index name even when
it was created with double quotes.
In `@docs/notification-cleanup-monitoring.sql`:
- Around line 34-42: The size check in the notification cleanup monitoring query
is building the relation name as plain text, which can fail for the mixed-case
index identifier. Update the query around the pg_relation_size call to use a
quoted regclass reference for the index name so
IX_CorrespondenceNotifications_Cleanup is resolved correctly, and keep the rest
of the pg_indexes filtering intact.
In `@docs/notification-cleanup-quick-reference.md`:
- Around line 33-40: The verification query in the notification cleanup
reference uses a mixed-case relation name without quoting, so update the
`pg_relation_size` reference in the SQL snippet to use a properly quoted
`regclass` target for `IX_CorrespondenceNotifications_Cleanup` and ensure the
schema/table identifier is also quoted if needed. Keep the rest of the
`pg_indexes` check unchanged, and adjust the query so `pg_relation_size`
resolves the exact index name regardless of PostgreSQL’s lowercase folding.
In `@docs/notification-cleanup-risk-assessment.md`:
- Around line 5-26: The Query 13 SQL in notification-cleanup-monitoring.sql can
fail when the matching-row count is zero because the percentage calculation
divides by the subquery COUNT(*). Update the percent_affected expression in the
Query 13 block to guard the denominator with NULLIF(..., 0) so the result
becomes NULL instead of raising a division-by-zero error, keeping the rest of
the duplicates aggregation logic unchanged.
In `@src/Altinn.Correspondence.API/Controllers/MaintenanceController.cs`:
- Around line 452-467: Validate the query-bound batchCount in
CleanupMissingSyncedNotificationEvents before calling handler.Process, since
non-positive values can cause empty batches to be rescheduled indefinitely. Add
a guard near the existing logging/processing path in MaintenanceController that
rejects batchCount values less than or equal to zero and returns a BadRequest
response instead of starting the cleanup.
In
`@src/Altinn.Correspondence.Application/CleanupMissingSyncedNotificationsBatch/CleanupMissingSyncedNotificationsBatchJob.cs`:
- Around line 34-35: The batching loop in
CleanupMissingSyncedNotificationsBatchJob uses an exact size check to decide
whether more work remains; update the hasMoreBatches condition to use a
greater-than-or-equal comparison against request.BatchSize. Keep the logic
centered around the batch.TotalNotificationCount and request.BatchSize symbols
so the loop still continues correctly if the data source ever over-fetches or
the count diverges.
In
`@src/Altinn.Correspondence.Core/Repositories/ICorrespondenceNotificationRepository.cs`:
- Around line 22-31: Remove the unused
GetSyncedNotificationsWithoutDialogActivityBatch member from
ICorrespondenceNotificationRepository and delete its matching implementation
from CorrespondenceNotificationRepository; keep the remaining batch method(s)
like GetCorrespondencesWithSyncedNotifications intact and update any related
references or interface contracts so the repository still compiles cleanly
without the dead API.
In `@src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs`:
- Around line 1258-1287: The PostActivityToDialog flow is swallowing non-422
failures by only logging them and then returning, so the caller in the backfill
loop treats failed activity posts as successful. Update PostActivityToDialog in
DialogportenService to throw on non-HttpStatusCode.UnprocessableEntity responses
(or otherwise propagate the failure) while keeping the existing “already exists”
warning path intact. Make sure the exception bubbles up through the caller that
awaits PostActivityToDialog so the batch/Hangfire job can fail and retry
appropriately.
In
`@src/Altinn.Correspondence.Persistence/Repositories/CorrespondenceNotificationRepository.cs`:
- Around line 155-178: The batch cursor in
CorrespondenceNotificationRepository’s
GetSyncedNotificationsWithoutDialogActivityBatch skips rows with a null
NotificationSent because the filter only compares against
lastProcessedTimestamp; update the cursor logic and ordering to explicitly
include null timestamps and treat them consistently as the oldest records so
they are not permanently missed. Also rename the method to match the actual
query semantics, since it filters CorrespondenceNotificationEntity by
Altinn2NotificationId and SyncedFromAltinn2 rather than any
DialogActivityId-related field; use a name like
GetSyncedAltinn2NotificationsBatch and keep the implementation aligned with that
symbol.
In
`@Test/Altinn.Correspondence.Tests/TestingController/Maintenance/CleanupMissingSyncedNotificationEventsTests.cs`:
- Around line 130-155: The test’s handler setup is mixing manual construction
with scoped DI, which can dispose the underlying DbContext/Npgsql pool during
ExecuteBatch. In CleanupMissingSyncedNotificationEventsTests, change the setup
so CleanupMissingSyncedNotificationsBatchHandler is resolved through the same
active scope as its dependencies, and override the needed test doubles via the
test-host DI container instead of instantiating the handler directly. Keep the
scope alive for the full execution path and avoid combining a manually created
batch job with a real ChainedBatchJobOrchestrator and scoped repository
instances.
In
`@Test/Altinn.Correspondence.Tests/TestingHandler/CleanupMissingSyncedNotificationsBatchHandlerTests.cs`:
- Around line 243-249: The test setup in
CleanupMissingSyncedNotificationsBatchHandlerTests builds each correspondence
batch using only totalNotifications / correspondenceCount, which can drop any
remainder and make TotalNotificationCount differ from the actual generated
NotificationIds. Update the batch construction logic around
notificationsPerCorrespondence so the generated notification list count always
matches totalNotifications, including any remainder distribution across
correspondences, to keep the test data aligned with the asserted totals.
---
Nitpick comments:
In `@src/Altinn.Correspondence.API/Controllers/MaintenanceController.cs`:
- Line 467: The MaintenanceController action is discarding the Task returned by
handler.Process, so exceptions can be missed and future async work will become
fire-and-forget. Update the action that calls Process to be async
Task<ActionResult>, then await handler.Process(batchCount, processFromDate,
startId) instead of calling it synchronously.
In
`@src/Altinn.Correspondence.Application/CleanupMissingSyncedNotificationsBatch/CleanupMissingSyncedNotificationsBatchHandler.cs`:
- Around line 52-63: Remove the dead ProcessCorrespondenceNotifications stub
from CleanupMissingSyncedNotificationsBatchHandler; it only throws
NotImplementedException and is not the real worker. Delete the unused
Hangfire-shaped method and its [AutomaticRetry(Attempts = 0)] attribute, and
keep the actual notification handling in
IDialogportenService.AddNotificationActivitiesWithDuplicateCheck so there is no
accidental enqueue target left behind.
In
`@src/Altinn.Correspondence.Application/CleanupMissingSyncedNotificationsBatch/CleanupMissingSyncedNotificationsBatchJob.cs`:
- Around line 26-54: Remove the duplicate database call in
CleanupMissingSyncedNotificationsBatchJob’s ProcessBatchAsync: it currently
re-queries GetCorrespondencesWithSyncedNotifications with the same request
parameters already used by FetchBatchAsync. Use the items parameter passed into
ProcessBatchAsync (from ChainedBatchJobOrchestrator) and derive any needed batch
state from that data instead of refetching, while keeping cursor/state handling
consistent with the existing batch job flow.
In `@src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs`:
- Around line 1304-1316: The bulk notification loading in DialogportenService is
doing one repository call per notification ID inside the loop, creating an N+1
pattern. Update the notification lookup path around the notificationIds
iteration to use a single batched repository call such as a
GetNotificationsByIds method or an equivalent WHERE Id IN query, then populate
the notifications list from that result set. Keep the existing
missing-notification logging behavior by comparing requested IDs against the
batched results and logging warnings for any IDs not returned.
In
`@src/Altinn.Correspondence.Persistence/Migrations/20260623113729_AddNotificationCleanupIndex.cs`:
- Around line 27-33: The new index created in the AddNotificationCleanupIndex
migration will block writes because CreateIndex runs non-concurrently on the
large CorrespondenceNotifications table. Update the migration to avoid issuing a
regular CreateIndex here; instead make the migration a no-op or guarded IF NOT
EXISTS path and handle the index creation out-of-band with a manual CONCURRENTLY
rollout, or explicitly document that this migration must be applied in a
maintenance window. Keep the existing index definition in
AddNotificationCleanupIndex aligned with the cleanup query, but change the
rollout mechanism only.
In
`@src/Altinn.Correspondence.Persistence/Repositories/CorrespondenceNotificationRepository.cs`:
- Around line 180-185: Add a non-positive count guard in
GetCorrespondencesWithSyncedNotifications, matching the early-return behavior
used by GetAltinn3NotificationDeliveryRepairCandidates. Check count at the start
of the method and, when it is zero or negative, return an empty
CorrespondencesWithNotificationsBatch immediately instead of continuing into the
query pipeline.
- Around line 180-202: The cursor and synced-status filtering in
GetCorrespondencesWithSyncedNotifications duplicates the logic already used by
GetSyncedNotificationsWithoutDialogActivityBatch, which risks the two paths
drifting apart. Extract the shared synced predicate plus composite cursor
condition into a private IQueryable<CorrespondenceNotificationEntity> helper and
have both methods call it, keeping the NotificationSent null-handling consistent
in that shared place.
🪄 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: 19ee58f1-597f-4d14-92d7-b2ee41c22b2b
📒 Files selected for processing (24)
Test/Altinn.Correspondence.Tests/Altinn.Correspondence.Tests.csprojTest/Altinn.Correspondence.Tests/Dialogporten/DialogportenServiceTests.csTest/Altinn.Correspondence.Tests/TestingController/Maintenance/CleanupMissingSyncedNotificationEventsTests.csTest/Altinn.Correspondence.Tests/TestingHandler/CleanupMissingSyncedNotificationsBatchHandlerTests.csdocs/notification-cleanup-batch-boundary-solutions.mddocs/notification-cleanup-migration-guide.mddocs/notification-cleanup-monitoring.sqldocs/notification-cleanup-quick-reference.mddocs/notification-cleanup-risk-assessment.mdsrc/Altinn.Correspondence.API/Controllers/MaintenanceController.cssrc/Altinn.Correspondence.Application/CleanupMissingSyncedNotificationsBatch/CleanupMissingSyncedNotificationsBatchHandler.cssrc/Altinn.Correspondence.Application/CleanupMissingSyncedNotificationsBatch/CleanupMissingSyncedNotificationsBatchJob.cssrc/Altinn.Correspondence.Application/CleanupMissingSyncedNotificationsBatch/CleanupMissingSyncedNotificationsBatchRequest.cssrc/Altinn.Correspondence.Application/DependencyInjection.cssrc/Altinn.Correspondence.Core/Models/Notifications/CorrespondenceWithNotifications.cssrc/Altinn.Correspondence.Core/Repositories/ICorrespondenceNotificationRepository.cssrc/Altinn.Correspondence.Core/Services/IDialogportenService.cssrc/Altinn.Correspondence.Integrations/Dialogporten/DialogportenDevService.cssrc/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cssrc/Altinn.Correspondence.Integrations/Dialogporten/Mappers/CreateDialogRequestMapper.cssrc/Altinn.Correspondence.Persistence/Migrations/20260623113729_AddNotificationCleanupIndex.Designer.cssrc/Altinn.Correspondence.Persistence/Migrations/20260623113729_AddNotificationCleanupIndex.cssrc/Altinn.Correspondence.Persistence/Migrations/ApplicationDbContextModelSnapshot.cssrc/Altinn.Correspondence.Persistence/Repositories/CorrespondenceNotificationRepository.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs (1)
1326-1395: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAssert the notification-to-correspondence contract before posting.
This method treats
notifications.First().Correspondenceas canonical and feeds it intoGetActivityFromAltinn2Notification(...)for every notification.GetNotificationsByIds(...)does not enforce a singleCorrespondenceId, so a mixed ID list would post activities to the wrong dialog with the wrong correspondence payload.Proposed fix
- var correspondence = notifications.First().Correspondence!; + if (notifications.Any(n => n.CorrespondenceId != correspondenceId)) + { + throw new ArgumentException( + $"One or more notifications do not belong to correspondence {correspondenceId}", + nameof(notificationIds)); + } + + var correspondence = notifications.First().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 1326 - 1395, Validate that all notifications in this method belong to the same correspondence before using notifications.First().Correspondence as the source of truth and before calling CreateDialogRequestMapper.GetActivityFromAltinn2Notification for each item. In DialogportenService, add an explicit check that the notification set has one CorrespondenceId and that every notification matches the same Correspondence, then fail fast or skip if mixed IDs are detected. Keep the existing dialog lookup and posting flow unchanged, but only proceed with PostActivityToDialog after the correspondence consistency check passes.
🤖 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.Persistence/Repositories/CorrespondenceNotificationRepository.cs`:
- Around line 58-64: The single-notification retrieval path is missing the eager
load for `Correspondence.ExternalReferences`, which can leave
`AddNotificationActivity(...)` without the dialog reference. Update
`GetNotificationById(...)` in `CorrespondenceNotificationRepository` to match
`GetNotificationsByIds(...)` by including `Correspondence` and then
`ExternalReferences` so the one-item path loads the same data needed for
activity posting.
---
Outside diff comments:
In `@src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs`:
- Around line 1326-1395: Validate that all notifications in this method belong
to the same correspondence before using notifications.First().Correspondence as
the source of truth and before calling
CreateDialogRequestMapper.GetActivityFromAltinn2Notification for each item. In
DialogportenService, add an explicit check that the notification set has one
CorrespondenceId and that every notification matches the same Correspondence,
then fail fast or skip if mixed IDs are detected. Keep the existing dialog
lookup and posting flow unchanged, but only proceed with PostActivityToDialog
after the correspondence consistency check passes.
🪄 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: d9e0b8cf-1553-420f-b129-28835c2b032b
📒 Files selected for processing (8)
Test/Altinn.Correspondence.Tests/TestingController/Maintenance/CleanupMissingSyncedNotificationEventsTests.csTest/Altinn.Correspondence.Tests/TestingHandler/CleanupMissingSyncedNotificationsBatchHandlerTests.cssrc/Altinn.Correspondence.API/Controllers/MaintenanceController.cssrc/Altinn.Correspondence.Application/BatchJobs/ChainedBatchJobOrchestrator.cssrc/Altinn.Correspondence.Application/CleanupMissingSyncedNotificationsBatch/CleanupMissingSyncedNotificationsBatchHandler.cssrc/Altinn.Correspondence.Core/Repositories/ICorrespondenceNotificationRepository.cssrc/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cssrc/Altinn.Correspondence.Persistence/Repositories/CorrespondenceNotificationRepository.cs
💤 Files with no reviewable changes (1)
- src/Altinn.Correspondence.Application/CleanupMissingSyncedNotificationsBatch/CleanupMissingSyncedNotificationsBatchHandler.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- Test/Altinn.Correspondence.Tests/TestingController/Maintenance/CleanupMissingSyncedNotificationEventsTests.cs
- src/Altinn.Correspondence.API/Controllers/MaintenanceController.cs
- Test/Altinn.Correspondence.Tests/TestingHandler/CleanupMissingSyncedNotificationsBatchHandlerTests.cs
RagnarFatland
temporarily deployed
to
use-case-test
June 26, 2026 12:10 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.Persistence/Migrations/20251110150438_IndexIsolatedToA3Correspondences.cs`:
- Around line 14-16: The index creation in the migration uses CREATE INDEX
CONCURRENTLY IF NOT EXISTS, which can hide a pre-existing index with the same
name but a different definition. Update the migration around the A3-only
correspondence index creation to fail fast on mismatches by removing IF NOT
EXISTS, or add an explicit check before issuing CREATE that validates the
existing index definition matches the intended columns, sort order, and
predicate. Use the migration class name
20251110150438_IndexIsolatedToA3Correspondences and the index name
IX_Correspondences_Recipient_RequestedPublishTime_A3Only to locate the change.
🪄 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: e74aba93-8ad8-4199-9c64-f1739c71a70d
📒 Files selected for processing (1)
src/Altinn.Correspondence.Persistence/Migrations/20251110150438_IndexIsolatedToA3Correspondences.cs
RagnarFatland
temporarily deployed
to
use-case-test
June 29, 2026 11:15 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Altinn.Correspondence.Persistence/Repositories/CorrespondenceNotificationRepository.cs (1)
54-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilence the nullable warning on
ThenInclude.CorrespondenceNotificationEntity.Correspondenceis nullable, soThenInclude(c => c.ExternalReferences)still trips nullable analysis. Usec!here or make the navigation non-nullable if the relationship is required.🤖 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/CorrespondenceNotificationRepository.cs` around lines 54 - 55, The nullable warning on the CorrespondenceNotificationRepository query comes from the ThenInclude on the optional Correspondence navigation. Update the Include/ThenInclude chain in the repository query so nullable analysis is silenced, either by using a null-forgiving assertion on the lambda parameter in ThenInclude or by making CorrespondenceNotificationEntity.Correspondence non-nullable if that relationship is required. Use the CorrespondenceNotificationRepository query and the CorrespondenceNotificationEntity.Correspondence navigation as the key symbols to locate the change.Source: Linters/SAST tools
🤖 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
`@src/Altinn.Correspondence.Persistence/Repositories/CorrespondenceNotificationRepository.cs`:
- Around line 54-55: The nullable warning on the
CorrespondenceNotificationRepository query comes from the ThenInclude on the
optional Correspondence navigation. Update the Include/ThenInclude chain in the
repository query so nullable analysis is silenced, either by using a
null-forgiving assertion on the lambda parameter in ThenInclude or by making
CorrespondenceNotificationEntity.Correspondence non-nullable if that
relationship is required. Use the CorrespondenceNotificationRepository query and
the CorrespondenceNotificationEntity.Correspondence navigation as the key
symbols to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 48996535-a842-423b-a4b0-2405d8c32f52
📒 Files selected for processing (2)
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cssrc/Altinn.Correspondence.Persistence/Repositories/CorrespondenceNotificationRepository.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs
RagnarFatland
temporarily deployed
to
use-case-test
June 29, 2026 12:00 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
RagnarFatland
marked this pull request as ready for review
June 29, 2026 14:15
RagnarFatland
temporarily deployed
to
use-case-test
June 29, 2026 19:53 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
…duplication check.
RagnarFatland
temporarily deployed
to
use-case-test
June 30, 2026 09:43 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/notification-cleanup-quick-reference.md (1)
113-115: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the broken documentation link
docs/notification-cleanup-quick-reference.md:115—docs/notification-cleanup-changes-summary.mddoesn’t exist indocs/, so update the reference or remove 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 `@docs/notification-cleanup-quick-reference.md` around lines 113 - 115, The quick reference contains a broken documentation link to a nonexistent changes summary file. Update the “Changes Summary” reference in the notification cleanup quick reference to point to the correct existing document, or remove that bullet if no equivalent doc exists; verify the surrounding links in the same list so they all reference valid docs.
🧹 Nitpick comments (1)
Test/Altinn.Correspondence.Tests/TestingController/Maintenance/CleanupMissingSyncedNotificationEventsTests.cs (1)
182-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for
batchCount <= 0.The controller now rejects non-positive
batchCount, but this suite only covers the missing-startDatepath. A smallBadRequesttest here would lock in the guard that prevents empty cleanup batches from being rescheduled indefinitely.🤖 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/TestingController/Maintenance/CleanupMissingSyncedNotificationEventsTests.cs` around lines 182 - 199, Add a regression test in the CleanupMissingSyncedNotificationEventsTests suite for the new non-positive batchCount validation. Mirror the existing CleanupMissingSyncedNotificationEvents_WithoutStartDate_ReturnsBadRequest pattern, but call the maintenance endpoint with batchCount=0 or a negative value and assert BadRequest plus a response message indicating batchCount must be positive. Use the CleanupMissingSyncedNotificationEvents test method names and MaintenanceControllerBaseUrl to keep the test easy to locate alongside the current startDate validation coverage.
🤖 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 `@docs/notification-cleanup-quick-reference.md`:
- Line 66: The example note and related comment use nonstandard wording by
saying “codefix”; update the phrasing in the referenced markdown example to
“code fix” or “bug fix” consistently. Make the same spelling correction wherever
the same text appears in the doc, including the repeated note mentioned in the
review, so the terminology matches throughout.
In `@Test/Altinn.Correspondence.Tests/Dialogporten/DialogportenServiceTests.cs`:
- Around line 1286-1295: The mocked POST capture in DialogportenServiceTests is
using an async fire-and-forget Callback, so the request body may still be read
and the activity list updated after the assertions run. Update the callback in
the test helper around the POST activity capture to avoid async void behavior by
either reading the request content synchronously like the other helpers or
moving the capture logic into a fully awaited path, and make sure the symbols
activityPostCounter and postedActivities are populated before the test
continues.
---
Outside diff comments:
In `@docs/notification-cleanup-quick-reference.md`:
- Around line 113-115: The quick reference contains a broken documentation link
to a nonexistent changes summary file. Update the “Changes Summary” reference in
the notification cleanup quick reference to point to the correct existing
document, or remove that bullet if no equivalent doc exists; verify the
surrounding links in the same list so they all reference valid docs.
---
Nitpick comments:
In
`@Test/Altinn.Correspondence.Tests/TestingController/Maintenance/CleanupMissingSyncedNotificationEventsTests.cs`:
- Around line 182-199: Add a regression test in the
CleanupMissingSyncedNotificationEventsTests suite for the new non-positive
batchCount validation. Mirror the existing
CleanupMissingSyncedNotificationEvents_WithoutStartDate_ReturnsBadRequest
pattern, but call the maintenance endpoint with batchCount=0 or a negative value
and assert BadRequest plus a response message indicating batchCount must be
positive. Use the CleanupMissingSyncedNotificationEvents test method names and
MaintenanceControllerBaseUrl to keep the test easy to locate alongside the
current startDate validation coverage.
🪄 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: 9ebcff70-d621-4d6e-b900-9a96c7b31e25
📒 Files selected for processing (5)
Test/Altinn.Correspondence.Tests/Dialogporten/DialogportenServiceTests.csTest/Altinn.Correspondence.Tests/TestingController/Maintenance/CleanupMissingSyncedNotificationEventsTests.csTest/Altinn.Correspondence.Tests/TestingHandler/CleanupMissingSyncedNotificationsBatchHandlerTests.csdocs/notification-cleanup-quick-reference.mdsrc/Altinn.Correspondence.API/Controllers/MaintenanceController.cs
… fixed misspelling.
RagnarFatland
temporarily deployed
to
use-case-test
June 30, 2026 10:00 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
9 tasks
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
RagnarFatland
commented
Jun 30, 2026
| { | ||
| migrationBuilder.Sql(@" | ||
| CREATE INDEX CONCURRENTLY IX_Correspondences_Recipient_RequestedPublishTime_A3Only | ||
| CREATE INDEX CONCURRENTLY IF NOT EXISTS IX_Correspondences_Recipient_RequestedPublishTime_A3Only |
Contributor
Author
There was a problem hiding this comment.
Added due to migrations failing locally on my dev image.
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
3 participants
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.
Added a job to update Dialogporten with missing NotificationEvents caused by bug #1794
Description
To avoid duplicates, it will load the dialog and perform a duplicate check on the distinct fields before adding the missing activities.
This is intended as a one-time cleanup job, and should be run with startdate='2026-04-25' which is the date the fix was rolled out.
Related Issue(s)
Verification
Documentation
[x] User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)Summary by CodeRabbit
batchCountplus cursor support (startDateand optionalstartId).