Support Daytime sending policy for emails (#1530) - #1533
Draft
olebhansen wants to merge 8 commits into
Draft
Conversation
Adds notifications.orders.emailsendingtimepolicy column (nullable, no backfill, no constraint) and the SQL functions used to claim email batches by policy: - claim_email_batch now filters AND (emailsendingtimepolicy IS NULL OR emailsendingtimepolicy = 1). NULL rows keep current Anytime behavior, so the existing /trigger/sendemail path remains a no-op change. - claim_daytime_email_batch is new and selects only rows with emailsendingtimepolicy = 2. - insertorder gets a new overload with _emailsendingtimepolicy. Refs #1530. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Changes the email sending time policy contract so a missing field stays NULL all the way to the database, instead of defaulting to Anytime client-side. Existing API clients that omit the field see the same behavior as today (the modified claim_email_batch picks up NULL rows). - EmailSendingOptionsExt.SendingTimePolicy is now SendingTimePolicyExt? with no [DefaultValue]; null is preserved. - EmailSendingOptions.SendingTimePolicy is now SendingTimePolicy?. - EmailSendingOptionsValidator allows null and accepts Daytime in addition to Anytime; other values are rejected. - NotificationOrder gets EmailSendingTimePolicy alongside the existing SendingTimePolicy (kept for SMS to avoid a wider rename), and the mapper / OrderRequestService flow the email policy through. - OrderRepository writes the new emailsendingtimepolicy column via the new insertorder overload. Refs #1530. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
GetNewNotificationsAsync gets a SendingTimePolicy parameter (default Anytime) that selects between claim_email_batch and claim_daytime_email_batch. Daytime callers now pull only orders that were explicitly tagged Daytime; Anytime callers (the existing /trigger/sendemail path) keep claiming NULL and Anytime rows. Refs #1530. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the SMS dual-loop pattern for email: - IEmailPublishTaskQueue gains a SendingTimePolicy parameter on TryEnqueue/WaitAsync and a new MarkCompleted, with per-policy channels and duplicate coalescing. - EmailPublishBackgroundService runs two concurrent loops, one for each policy, joined with Task.WhenAll. - IEmailNotificationService.SendNotifications takes a policy (default Anytime), passed through to the repository. - /trigger/sendemail now enqueues explicitly with Anytime — same semantics as before (the new claim_email_batch picks up Anytime and NULL rows), but explicit at the call site. The new /trigger/sendemaildaytime endpoint is added in a follow-up commit alongside the schedule gating. Refs #1530. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
NotificationConfig gets EmailSendWindowStartHour/EndHour with the same 09-17 defaults as the SMS counterparts. Mirroring keeps the change isolated and lets ops tune the windows independently if needed. INotificationScheduleService gets CanSendEmailNow() and GetEmailExpirationDateTime(...). The implementation factors the existing SMS window check and expiry calculation into shared private helpers and applies them to the email window. Refs #1530. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
EmailNotificationService.CreateNotification now takes the expiry from the caller, mirroring the SMS service. EmailOrderProcessingService computes it: Daytime orders use INotificationScheduleService.GetEmail ExpirationDateTime (extending expiry across the next email window so work picked outside hours still has time to be sent), other orders keep RequestedSendTime + 48h. Refs #1530. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors /trigger/sendsmsdaytime: gates on INotificationScheduleService.CanSendEmailNow() and, if inside the configured email window, enqueues the email publish queue with SendingTimePolicy.Daytime. Returns 200 OK regardless of whether work was queued so the cron caller doesn't see noise. The infra cron job that pings this endpoint is added separately in the infra repo. Refs #1530. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Updates the test suite for the email Daytime support: - EmailSendingOptionsValidatorTests: null is allowed; Daytime/Anytime pass; values outside the enum are rejected. - NotificationScheduleServiceTests: cover CanSendEmailNow and GetEmailExpirationDateTime, mirroring the SMS cases. - EmailNotificationRepositoryTests: integration tests that verify the Anytime path claims NULL/Anytime rows and skips Daytime rows, and vice versa for the Daytime path. - Trigger_SendEmailNotificationsTests: gates on CanSendEmailNow, ensures /sendemail does not pick up Daytime orders, and that /sendemaildaytime processes Daytime orders inside the window. - Existing tests adjusted for the new EmailNotificationService signature (caller-supplied expiry), the dual-loop email queue, and the EmailOrderProcessingService schedule-service dependency. Refs #1530. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 tasks
Contributor
📝 WalkthroughWalkthroughThis PR extends daytime-policy sending support from SMS-only to email notifications. It adds email-specific scheduling windows via configuration and scheduling service, implements policy-aware queue coordination, introduces separate database batch-claiming functions for daytime and anytime email orders, and adds a new trigger endpoint for daytime-restricted email processing. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
2 tasks
![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: 5
🧹 Nitpick comments (4)
components/api/src/Altinn.Notifications.Core/Services/EmailPublishBackgroundService.cs (1)
33-39: Consider deriving loops from enum values to avoid policy drift.The current two-loop setup is correct, but hardcoding policies means new enum values could be enqueued without a worker loop.
♻️ Suggested refactor
protected override async Task ExecuteAsync(CancellationToken stoppingToken) { - var anytimeLoop = RunPolicyLoopAsync(SendingTimePolicy.Anytime, stoppingToken); - var daytimeLoop = RunPolicyLoopAsync(SendingTimePolicy.Daytime, stoppingToken); + var loops = Enum + .GetValues<SendingTimePolicy>() + .Select(policy => RunPolicyLoopAsync(policy, stoppingToken)); try { - await Task.WhenAll(anytimeLoop, daytimeLoop); + await Task.WhenAll(loops); } catch (OperationCanceledException) { // Graceful shutdown } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/api/src/Altinn.Notifications.Core/Services/EmailPublishBackgroundService.cs` around lines 33 - 39, The code currently starts two hardcoded loops (calling RunPolicyLoopAsync for SendingTimePolicy.Anytime and .Daytime) which will miss any new SendingTimePolicy enum values; change this to iterate over all SendingTimePolicy enum values (e.g., Enum.GetValues(typeof(SendingTimePolicy))) and start a RunPolicyLoopAsync for each value, collect the returned Tasks and await Task.WhenAll to ensure every policy value spawns a worker loop; update any variable names (e.g., anytimeLoop, daytimeLoop) to a collection like policyLoops to reflect the dynamic set.components/api/test/Altinn.Notifications.Tests/Notifications.Core/TestingServices/EmailOrderProcessingServiceTests.cs (1)
80-87: Consider asserting expiry value explicitly in one happy-path test.Using
It.IsAny<DateTime>()for the new expiry parameter leaves the new expiration logic effectively untested in this test case.Proposed test assertion tightening
serviceMock.Setup(s => s.CreateNotification( It.IsAny<Guid>(), It.Is<DateTime>(d => d.Equals(requested)), - It.IsAny<DateTime>(), + It.Is<DateTime>(d => d.Equals(requested.AddHours(48))), It.Is<List<EmailAddressPoint>>(r => AssertUtils.AreEquivalent(expectedEmailAddressPoints, r)), It.Is<EmailRecipient>(e => AssertUtils.AreEquivalent(expectedEmailRecipient, e)), It.IsAny<bool>()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/api/test/Altinn.Notifications.Tests/Notifications.Core/TestingServices/EmailOrderProcessingServiceTests.cs` around lines 80 - 87, The test uses It.IsAny<DateTime>() for the expiry argument in the serviceMock.Setup call to CreateNotification, so the new expiration logic isn't asserted; update the Setup to assert the expected expiry by replacing It.IsAny<DateTime>() with a stricter matcher referencing the computed expected expiry (e.g. It.Is<DateTime>(d => d.Equals(expectedExpiry)) or a tolerance-based comparison if timing is flaky), ensuring the CreateNotification invocation's expiry parameter is explicitly verified in this happy-path test.components/api/test/Altinn.Notifications.Tests/Notifications.Core/TestingServices/EmailNotificationServiceTests.cs (1)
509-510: Assert the forwarded policy explicitly here.Using
It.IsAny<SendingTimePolicy>()means this test still passes ifSendNotifications()starts fetching the wrong queue. Since policy routing is the point of this change, lock the setup toSendingTimePolicy.Anytimeso the default path is actually verified.Suggested tightening
-repoMock.Setup(r => r.GetNewNotificationsAsync(It.IsAny<int>(), It.IsAny<CancellationToken>(), It.IsAny<SendingTimePolicy>())) - .Callback<int, CancellationToken, SendingTimePolicy>((_, _, _) => cts.Cancel()) +repoMock.Setup(r => r.GetNewNotificationsAsync( + It.IsAny<int>(), + It.IsAny<CancellationToken>(), + It.Is<SendingTimePolicy>(p => p == SendingTimePolicy.Anytime))) + .Callback<int, CancellationToken, SendingTimePolicy>((_, _, _) => cts.Cancel())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/api/test/Altinn.Notifications.Tests/Notifications.Core/TestingServices/EmailNotificationServiceTests.cs` around lines 509 - 510, The test setup for repoMock.GetNewNotificationsAsync uses It.IsAny<SendingTimePolicy>() which allows any policy and doesn't verify routing; update the setup to assert the forwarded policy is SendingTimePolicy.Anytime by replacing the matcher with a specific matcher like It.Is<SendingTimePolicy>(p => p == SendingTimePolicy.Anytime) so the test fails if SendNotifications() requests the wrong queue (refer to repoMock.Setup(... GetNewNotificationsAsync ...) and the SendNotifications() flow in EmailNotificationServiceTests/EmailNotificationService class).components/api/src/Altinn.Notifications.Core/Services/NotificationScheduleService.cs (1)
69-73: Consider boundary behavior.The strict inequality (
>and<) means notifications sent at exactly the window boundaries (e.g., 09:00:00 or 17:00:00) are considered outside the window. This is likely intentional to avoid edge-case timing issues, but worth confirming this matches the business requirement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/api/src/Altinn.Notifications.Core/Services/NotificationScheduleService.cs` around lines 69 - 73, The current IsWithinWindow method uses strict > and < which excludes times exactly equal to windowStart or windowEnd; update the comparison in IsWithinWindow (which uses GetEquivalentDateTimeInNorway(utcNow)) to use inclusive comparisons (>= and <=) if business rules require boundary times to be allowed, or document/rename the method to explicitly indicate exclusive behavior if boundaries must remain excluded; adjust unit tests and any callers of IsWithinWindow accordingly to reflect the chosen boundary semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/api/src/Altinn.Notifications.Persistence/Migration/FunctionsAndProcedures/claimdaytimeemailbatch.sql`:
- Around line 83-85: Update the COMMENT ON FUNCTION for
notifications.claim_daytime_email_batch(integer) to accurately describe the
clamp behavior: explain that NULL _batchsize defaults to 500 via COALESCE and
any value less than 1 is clamped to 1 using GREATEST(1, ...), so the effective
batch size is GREATEST(1, COALESCE(_batchsize, 500)) rather than treating values
<1 as defaulting to 500.
In
`@components/api/src/Altinn.Notifications.Persistence/Migration/v0.80/01-alter-tables.sql`:
- Around line 1-2: Add a DB-level CHECK limiting the new column to NULL, 1, or 2
so unexpected integers can’t be written; when adding emailsendingtimepolicy to
notifications.orders, create the column with (or immediately add) a constraint
that only allows NULL, 1, or 2 (or add a named CHECK constraint like
notifications_orders_emailsendingtimepolicy_chk) to prevent unclaimable values
and ensure the migration fails if invalid defaults are introduced.
In
`@components/api/src/Altinn.Notifications.Persistence/Repository/OrderRepository.cs`:
- Line 587: ReadNotificationOrderWithStatus in OrderRepository currently omits
mapping the emailsendingtimepolicy column back into the model; update the reader
logic in ReadNotificationOrderWithStatus (OrderRepository) to read the
"emailsendingtimepolicy" column, handle DBNull, and assign it to
NotificationOrderWithStatus.EmailSendingTimePolicy (converting the int to the
appropriate enum or nullable enum as used by the property). Ensure you use the
reader.IsDBNull check and the correct enum cast (e.g.,
(EmailSendingTimePolicy)reader.GetInt32(ord)) or set null when DB value is
DBNull.
In
`@components/api/test/Altinn.Notifications.Tests/Notifications.Core/BackgroundQueues/EmailPublishTaskQueueTests.cs`:
- Around line 45-80: The tests rely on fragile wall-clock timing (Task.Delay(50)
and Stopwatch check) which can flake on slow CI; instead use deterministic
cancellation/coordination when calling EmailPublishTaskQueue.WaitAsync and
TryEnqueue. For WaitAsync_CompletesAfterTryEnqueue_WhenWaitingFirst remove
Task.Delay and construct a CancellationTokenSource with a short timeout passed
into WaitAsync so you can call TryEnqueue(SendingTimePolicy.Daytime) and then
await the waitTask with that token (failing if the token cancels). For
WaitAsync_CompletesImmediately_WhenEnqueuedBefore drop the Stopwatch and call
WaitAsync(SendingTimePolicy.Anytime, cancellationToken) with a bounded
CancellationTokenSource (or use Task.WhenAny(waitTask, timeoutToken.AsTask())
and assert the WaitAsync task completed); reference EmailPublishTaskQueue,
WaitAsync and TryEnqueue when applying these changes.
In
`@components/api/test/Altinn.Notifications.Tests/Notifications.Core/TestingServices/NotificationScheduleServiceTests.cs`:
- Around line 132-229: Add explicit boundary tests for 09:00 and 17:00 and make
the schedule logic use an inclusive start boundary. Concretely: add unit tests
calling CanSendEmailNow() with mocked UtcNow at exactly 09:00 (assert True) and
exactly 17:00 (assert False), and add corresponding GetEmailExpirationDateTime
tests for requestedSendTime at 09:00 and 17:00 asserting the expected expiry
behavior; then update the scheduling helper/NotificationScheduleService
comparisons (where it currently uses strict > and <) to use >= sendWindowStart
&& < sendWindowEnd so start is inclusive and end remains exclusive. Ensure tests
reference CanSendEmailNow and GetEmailExpirationDateTime to locate behavior.
---
Nitpick comments:
In
`@components/api/src/Altinn.Notifications.Core/Services/EmailPublishBackgroundService.cs`:
- Around line 33-39: The code currently starts two hardcoded loops (calling
RunPolicyLoopAsync for SendingTimePolicy.Anytime and .Daytime) which will miss
any new SendingTimePolicy enum values; change this to iterate over all
SendingTimePolicy enum values (e.g., Enum.GetValues(typeof(SendingTimePolicy)))
and start a RunPolicyLoopAsync for each value, collect the returned Tasks and
await Task.WhenAll to ensure every policy value spawns a worker loop; update any
variable names (e.g., anytimeLoop, daytimeLoop) to a collection like policyLoops
to reflect the dynamic set.
In
`@components/api/src/Altinn.Notifications.Core/Services/NotificationScheduleService.cs`:
- Around line 69-73: The current IsWithinWindow method uses strict > and < which
excludes times exactly equal to windowStart or windowEnd; update the comparison
in IsWithinWindow (which uses GetEquivalentDateTimeInNorway(utcNow)) to use
inclusive comparisons (>= and <=) if business rules require boundary times to be
allowed, or document/rename the method to explicitly indicate exclusive behavior
if boundaries must remain excluded; adjust unit tests and any callers of
IsWithinWindow accordingly to reflect the chosen boundary semantics.
In
`@components/api/test/Altinn.Notifications.Tests/Notifications.Core/TestingServices/EmailNotificationServiceTests.cs`:
- Around line 509-510: The test setup for repoMock.GetNewNotificationsAsync uses
It.IsAny<SendingTimePolicy>() which allows any policy and doesn't verify
routing; update the setup to assert the forwarded policy is
SendingTimePolicy.Anytime by replacing the matcher with a specific matcher like
It.Is<SendingTimePolicy>(p => p == SendingTimePolicy.Anytime) so the test fails
if SendNotifications() requests the wrong queue (refer to repoMock.Setup(...
GetNewNotificationsAsync ...) and the SendNotifications() flow in
EmailNotificationServiceTests/EmailNotificationService class).
In
`@components/api/test/Altinn.Notifications.Tests/Notifications.Core/TestingServices/EmailOrderProcessingServiceTests.cs`:
- Around line 80-87: The test uses It.IsAny<DateTime>() for the expiry argument
in the serviceMock.Setup call to CreateNotification, so the new expiration logic
isn't asserted; update the Setup to assert the expected expiry by replacing
It.IsAny<DateTime>() with a stricter matcher referencing the computed expected
expiry (e.g. It.Is<DateTime>(d => d.Equals(expectedExpiry)) or a tolerance-based
comparison if timing is flaky), ensuring the CreateNotification invocation's
expiry parameter is explicitly verified in this happy-path test.
🪄 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: bb61f1f1-3193-44ca-8fdf-9ac6e7769707
📒 Files selected for processing (40)
components/api/src/Altinn.Notifications.Core/BackgroundQueue/EmailPublishTaskQueue.cscomponents/api/src/Altinn.Notifications.Core/BackgroundQueue/IEmailPublishTaskQueue.cscomponents/api/src/Altinn.Notifications.Core/Configuration/NotificationConfig.cscomponents/api/src/Altinn.Notifications.Core/Models/Orders/IBaseNotificationOrder.cscomponents/api/src/Altinn.Notifications.Core/Models/Orders/NotificationOrder.cscomponents/api/src/Altinn.Notifications.Core/Models/Orders/NotificationOrderWithStatus.cscomponents/api/src/Altinn.Notifications.Core/Models/RecipientDeliveryDetails.cscomponents/api/src/Altinn.Notifications.Core/Models/Recipients/EmailSendingOptions.cscomponents/api/src/Altinn.Notifications.Core/Persistence/IEmailNotificationRepository.cscomponents/api/src/Altinn.Notifications.Core/Services/EmailNotificationService.cscomponents/api/src/Altinn.Notifications.Core/Services/EmailOrderProcessingService.cscomponents/api/src/Altinn.Notifications.Core/Services/EmailPublishBackgroundService.cscomponents/api/src/Altinn.Notifications.Core/Services/Interfaces/IEmailNotificationService.cscomponents/api/src/Altinn.Notifications.Core/Services/Interfaces/INotificationScheduleService.cscomponents/api/src/Altinn.Notifications.Core/Services/NotificationScheduleService.cscomponents/api/src/Altinn.Notifications.Core/Services/OrderRequestService.cscomponents/api/src/Altinn.Notifications.Persistence/Migration/FunctionsAndProcedures/claimdaytimeemailbatch.sqlcomponents/api/src/Altinn.Notifications.Persistence/Migration/FunctionsAndProcedures/claimemailbatch.sqlcomponents/api/src/Altinn.Notifications.Persistence/Migration/FunctionsAndProcedures/insertorder.sqlcomponents/api/src/Altinn.Notifications.Persistence/Migration/v0.80/01-alter-tables.sqlcomponents/api/src/Altinn.Notifications.Persistence/Migration/v0.80/02-functions-and-procedures.sqlcomponents/api/src/Altinn.Notifications.Persistence/Repository/EmailNotificationRepository.cscomponents/api/src/Altinn.Notifications.Persistence/Repository/OrderRepository.cscomponents/api/src/Altinn.Notifications/Controllers/TriggerController.cscomponents/api/src/Altinn.Notifications/Mappers/NotificationOrderChainMapper.cscomponents/api/src/Altinn.Notifications/Models/Email/EmailSendingOptionsExt.cscomponents/api/src/Altinn.Notifications/Validators/Email/EmailSendingOptionsValidator.cscomponents/api/src/Altinn.Notifications/appsettings.jsoncomponents/api/test/Altinn.Notifications.IntegrationTests/Notifications.Persistence/EmailNotificationRepositoryTests.cscomponents/api/test/Altinn.Notifications.IntegrationTests/Notifications/TriggerController/TriggerControllerTests.cscomponents/api/test/Altinn.Notifications.IntegrationTests/Notifications/TriggerController/Trigger_SendEmailNotificationsTests.cscomponents/api/test/Altinn.Notifications.IntegrationTests/Utils/PostgreUtil.cscomponents/api/test/Altinn.Notifications.IntegrationTests/Utils/TestdataUtil.cscomponents/api/test/Altinn.Notifications.Tests/Notifications.Core/BackgroundQueues/EmailPublishTaskQueueTests.cscomponents/api/test/Altinn.Notifications.Tests/Notifications.Core/TestingServices/EmailNotificationServiceTests.cscomponents/api/test/Altinn.Notifications.Tests/Notifications.Core/TestingServices/EmailOrderProcessingServiceTests.cscomponents/api/test/Altinn.Notifications.Tests/Notifications.Core/TestingServices/NotificationScheduleServiceTests.cscomponents/api/test/Altinn.Notifications.Tests/Notifications.Core/TestingServices/OrderLifecycleStageProcessingTests.cscomponents/api/test/Altinn.Notifications.Tests/Notifications/TestingMappers/NotificationOrderChainMapperTests.cscomponents/api/test/Altinn.Notifications.Tests/Notifications/TestingValidators/EmailSendingOptionsValidatorTests.cs
Comment on lines
+83
to
+85
| COMMENT ON FUNCTION notifications.claim_daytime_email_batch(integer) | ||
| IS 'Claims and returns batches of email notifications restricted to the daytime sending window (emailsendingtimepolicy = 2). | ||
| _batchsize: requested batch size (defaults to 500 if NULL or <1).'; |
Contributor
There was a problem hiding this comment.
Fix the _batchsize comment to match the actual clamp logic.
The function does not default values < 1 to 500; it clamps them to 1 via GREATEST(1, COALESCE(_batchsize, 500)). Leaving the comment as-is is misleading for anyone reading or calling the function directly.
Suggested doc fix
COMMENT ON FUNCTION notifications.claim_daytime_email_batch(integer)
- IS 'Claims and returns batches of email notifications restricted to the daytime sending window (emailsendingtimepolicy = 2).
-_batchsize: requested batch size (defaults to 500 if NULL or <1).';
+ IS 'Claims and returns batches of email notifications restricted to the daytime sending window (emailsendingtimepolicy = 2).
+_batchsize: requested batch size (defaults to 500 if NULL; values <1 are clamped to 1).';📝 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
| COMMENT ON FUNCTION notifications.claim_daytime_email_batch(integer) | |
| IS 'Claims and returns batches of email notifications restricted to the daytime sending window (emailsendingtimepolicy = 2). | |
| _batchsize: requested batch size (defaults to 500 if NULL or <1).'; | |
| COMMENT ON FUNCTION notifications.claim_daytime_email_batch(integer) | |
| IS 'Claims and returns batches of email notifications restricted to the daytime sending window (emailsendingtimepolicy = 2). | |
| _batchsize: requested batch size (defaults to 500 if NULL; values <1 are clamped to 1).'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/api/src/Altinn.Notifications.Persistence/Migration/FunctionsAndProcedures/claimdaytimeemailbatch.sql`
around lines 83 - 85, Update the COMMENT ON FUNCTION for
notifications.claim_daytime_email_batch(integer) to accurately describe the
clamp behavior: explain that NULL _batchsize defaults to 500 via COALESCE and
any value less than 1 is clamped to 1 using GREATEST(1, ...), so the effective
batch size is GREATEST(1, COALESCE(_batchsize, 500)) rather than treating values
<1 as defaulting to 500.
Comment on lines
+1
to
+2
| ALTER TABLE notifications.orders | ||
| ADD COLUMN IF NOT EXISTS emailsendingtimepolicy INTEGER NULL; No newline at end of file |
Contributor
There was a problem hiding this comment.
Constrain emailsendingtimepolicy to the supported values.
The new claimers only handle NULL/1 and 2. Any other integer stored here becomes unclaimable and silently stalls email delivery. Add a DB-level check while introducing the column.
Suggested migration shape
ALTER TABLE notifications.orders
ADD COLUMN IF NOT EXISTS emailsendingtimepolicy INTEGER NULL;
+
+DO $$
+BEGIN
+ IF NOT EXISTS (
+ SELECT 1
+ FROM pg_constraint
+ WHERE conname = 'orders_emailsendingtimepolicy_check'
+ AND conrelid = 'notifications.orders'::regclass
+ ) THEN
+ ALTER TABLE notifications.orders
+ ADD CONSTRAINT orders_emailsendingtimepolicy_check
+ CHECK (emailsendingtimepolicy IS NULL OR emailsendingtimepolicy IN (1, 2));
+ END IF;
+END $$;📝 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
| ALTER TABLE notifications.orders | |
| ADD COLUMN IF NOT EXISTS emailsendingtimepolicy INTEGER NULL; | |
| ALTER TABLE notifications.orders | |
| ADD COLUMN IF NOT EXISTS emailsendingtimepolicy INTEGER NULL; | |
| DO $$ | |
| BEGIN | |
| IF NOT EXISTS ( | |
| SELECT 1 | |
| FROM pg_constraint | |
| WHERE conname = 'orders_emailsendingtimepolicy_check' | |
| AND conrelid = 'notifications.orders'::regclass | |
| ) THEN | |
| ALTER TABLE notifications.orders | |
| ADD CONSTRAINT orders_emailsendingtimepolicy_check | |
| CHECK (emailsendingtimepolicy IS NULL OR emailsendingtimepolicy IN (1, 2)); | |
| END IF; | |
| END $$; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/api/src/Altinn.Notifications.Persistence/Migration/v0.80/01-alter-tables.sql`
around lines 1 - 2, Add a DB-level CHECK limiting the new column to NULL, 1, or
2 so unexpected integers can’t be written; when adding emailsendingtimepolicy to
notifications.orders, create the column with (or immediately add) a constraint
that only allows NULL, 1, or 2 (or add a named CHECK constraint like
notifications_orders_emailsendingtimepolicy_chk) to prevent unclaimable values
and ensure the migration fails if invalid defaults are introduced.
| pgcom.Parameters.AddWithValue(NpgsqlDbType.Integer, (int?)order.SendingTimePolicy ?? (object)DBNull.Value); | ||
| pgcom.Parameters.AddWithValue(NpgsqlDbType.Text, order.Type.ToString()); | ||
| pgcom.Parameters.AddWithValue(NpgsqlDbType.Text, processingStatus.ToString()); | ||
| pgcom.Parameters.AddWithValue(NpgsqlDbType.Integer, (int?)order.EmailSendingTimePolicy ?? (object)DBNull.Value); |
Contributor
There was a problem hiding this comment.
EmailSendingTimePolicy is persisted but not read back into status model.
Line 587 writes the value, but ReadNotificationOrderWithStatus (Line 477–513) never maps emailsendingtimepolicy to NotificationOrderWithStatus.EmailSendingTimePolicy, so order-status reads can silently lose this field.
💡 Proposed fix
private static NotificationOrderWithStatus? ReadNotificationOrderWithStatus(NpgsqlDataReader reader)
{
string? conditionEndpointString = reader.GetValue<string>("conditionendpoint");
Uri? conditionEndpoint = conditionEndpointString == null ? null : new Uri(conditionEndpointString);
NotificationOrderWithStatus order = new(
reader.GetValue<Guid>("alternateid"),
reader.GetValue<string>("sendersreference"),
reader.GetValue<DateTime>("requestedsendtime"),
new Creator(reader.GetValue<string>("creatorname")),
reader.GetValue<DateTime>("created"),
reader.GetValue<NotificationChannel>("notificationchannel"),
reader.GetValue<bool?>("ignorereservation"),
reader.GetValue<string?>("resourceid"),
conditionEndpoint,
new ProcessingStatus(reader.GetValue<OrderProcessingStatus>("processedstatus"), reader.GetValue<DateTime>("processed")),
OrderType.Notification,
reader.GetValue<string?>("resourceaction"));
+
+ order.EmailSendingTimePolicy =
+ reader.GetValue<int?>("emailsendingtimepolicy") is int policyValue
+ ? (SendingTimePolicy)policyValue
+ : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/api/src/Altinn.Notifications.Persistence/Repository/OrderRepository.cs`
at line 587, ReadNotificationOrderWithStatus in OrderRepository currently omits
mapping the emailsendingtimepolicy column back into the model; update the reader
logic in ReadNotificationOrderWithStatus (OrderRepository) to read the
"emailsendingtimepolicy" column, handle DBNull, and assign it to
NotificationOrderWithStatus.EmailSendingTimePolicy (converting the int to the
appropriate enum or nullable enum as used by the property). Ensure you use the
reader.IsDBNull check and the correct enum cast (e.g.,
(EmailSendingTimePolicy)reader.GetInt32(ord)) or set null when DB value is
DBNull.
Comment on lines
+45
to
+80
| public async Task WaitAsync_CompletesAfterTryEnqueue_WhenWaitingFirst() | ||
| { | ||
| // Arrange | ||
| var queue = new EmailPublishTaskQueue(); | ||
| using var cancellationTokenSource = new CancellationTokenSource(_shortTimeout); | ||
|
|
||
| // Act | ||
| var waitTask = queue.WaitAsync(SendingTimePolicy.Daytime, cancellationTokenSource.Token); | ||
|
|
||
| // Small delay to ensure waiter is registered | ||
| await Task.Delay(50, TestContext.Current.CancellationToken); | ||
|
|
||
| var enqueued = queue.TryEnqueue(SendingTimePolicy.Daytime); | ||
|
|
||
| // Assert | ||
| Assert.True(enqueued); | ||
|
|
||
| await waitTask; // should complete | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task WaitAsync_CompletesImmediately_WhenEnqueuedBefore() | ||
| { | ||
| // Arrange | ||
| var queue = new EmailPublishTaskQueue(); | ||
|
|
||
| // Act / Assert | ||
| Assert.True(queue.TryEnqueue(SendingTimePolicy.Anytime)); | ||
|
|
||
| // Since a signal is already in the channel, WaitAsync should complete quickly | ||
| var stopwatch = Stopwatch.StartNew(); | ||
| await queue.WaitAsync(SendingTimePolicy.Anytime, CancellationToken.None); | ||
| stopwatch.Stop(); | ||
|
|
||
| Assert.True(stopwatch.Elapsed < TimeSpan.FromMilliseconds(100)); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid timing-based assertions in these async tests.
Task.Delay(50) and the Elapsed < 100ms check make both cases scheduler-dependent, so they can flap on slower CI workers even when EmailPublishTaskQueue is correct. Prefer asserting completion with explicit coordination or a bounded cancellation token instead of wall-clock timing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/api/test/Altinn.Notifications.Tests/Notifications.Core/BackgroundQueues/EmailPublishTaskQueueTests.cs`
around lines 45 - 80, The tests rely on fragile wall-clock timing
(Task.Delay(50) and Stopwatch check) which can flake on slow CI; instead use
deterministic cancellation/coordination when calling
EmailPublishTaskQueue.WaitAsync and TryEnqueue. For
WaitAsync_CompletesAfterTryEnqueue_WhenWaitingFirst remove Task.Delay and
construct a CancellationTokenSource with a short timeout passed into WaitAsync
so you can call TryEnqueue(SendingTimePolicy.Daytime) and then await the
waitTask with that token (failing if the token cancels). For
WaitAsync_CompletesImmediately_WhenEnqueuedBefore drop the Stopwatch and call
WaitAsync(SendingTimePolicy.Anytime, cancellationToken) with a bounded
CancellationTokenSource (or use Task.WhenAny(waitTask, timeoutToken.AsTask())
and assert the WaitAsync task completed); reference EmailPublishTaskQueue,
WaitAsync and TryEnqueue when applying these changes.
Comment on lines
+132
to
+229
| [Fact] | ||
| public void CanSendEmailNow_WhenCurrentTimeIsWithinSendWindow_ReturnsTrue() | ||
| { | ||
| // Arrange | ||
| var currentDateTime = new DateTime(2022, 1, 1, 10, 0, 0, DateTimeKind.Utc); | ||
| _dateTimeMock.Setup(e => e.UtcNow()).Returns(currentDateTime); | ||
|
|
||
| // Act | ||
| var result = _notificationScheduleService.CanSendEmailNow(); | ||
|
|
||
| // Assert | ||
| Assert.True(result); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void CanSendEmailNow_WhenCurrentTimeIsAfterSendWindow_ReturnsFalse() | ||
| { | ||
| // Arrange | ||
| var currentDateTime = new DateTime(2022, 1, 1, 20, 0, 0, DateTimeKind.Utc); | ||
| _dateTimeMock.Setup(e => e.UtcNow()).Returns(currentDateTime); | ||
|
|
||
| // Act | ||
| var result = _notificationScheduleService.CanSendEmailNow(); | ||
|
|
||
| // Assert | ||
| Assert.False(result); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void CanSendEmailNow_WhenCurrentTimeIsBeforeSendWindow_ReturnsFalse() | ||
| { | ||
| // Arrange | ||
| var currentDateTime = new DateTime(2022, 1, 1, 5, 0, 0, DateTimeKind.Utc); | ||
| _dateTimeMock.Setup(e => e.UtcNow()).Returns(currentDateTime); | ||
|
|
||
| // Act | ||
| var result = _notificationScheduleService.CanSendEmailNow(); | ||
|
|
||
| // Assert | ||
| Assert.False(result); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void GetEmailExpirationDateTime_RequestSendTimeIsWithinSendWindow_ReturnsRequestedPlus48Hours() | ||
| { | ||
| // Arrange | ||
| var requestedSendTime = new DateTime(2025, 08, 25, 10, 0, 0, DateTimeKind.Utc); | ||
|
|
||
| var expectedExpiryDateTime = new DateTime(2025, 08, 27, 10, 0, 0, DateTimeKind.Utc); | ||
|
|
||
| // Act | ||
| var expiryDateTime = _notificationScheduleService.GetEmailExpirationDateTime(requestedSendTime); | ||
|
|
||
| // Assert | ||
| Assert.Equal(expectedExpiryDateTime, expiryDateTime); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void GetEmailExpirationDateTime_WhenReferenceTimeIsAfterSendWindow_ReturnsNextSendWindowStartPlus72Hours() | ||
| { | ||
| // Arrange | ||
| var requestedSendTime = new DateTime(2025, 08, 25, 20, 0, 0, DateTimeKind.Utc); | ||
|
|
||
| var expectedExpiryDateTime = new DateTime(2025, 08, 28, 07, 0, 0, DateTimeKind.Utc); | ||
|
|
||
| // Act | ||
| var expiryDateTime = _notificationScheduleService.GetEmailExpirationDateTime(requestedSendTime); | ||
|
|
||
| // Assert | ||
| Assert.Equal(expectedExpiryDateTime, expiryDateTime); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void GetEmailExpirationDateTime_WhenReferenceTimeIsBeforeSendWindow_ReturnsNextSendWindowStartPlus48Hours() | ||
| { | ||
| // Arrange | ||
| var requestedSendTime = new DateTime(2025, 08, 25, 05, 0, 0, DateTimeKind.Utc); | ||
|
|
||
| var expectedExpiryDateTime = new DateTime(2025, 08, 27, 07, 0, 0, DateTimeKind.Utc); | ||
|
|
||
| // Act | ||
| var expiryDateTime = _notificationScheduleService.GetEmailExpirationDateTime(requestedSendTime); | ||
|
|
||
| // Assert | ||
| Assert.Equal(expectedExpiryDateTime, expiryDateTime); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData(DateTimeKind.Local)] | ||
| [InlineData(DateTimeKind.Unspecified)] | ||
| public void GetEmailExpirationDateTime_WhenReferenceDateTimeIsNotUtc_ThrowsArgumentException(DateTimeKind kind) | ||
| { | ||
| // Arrange | ||
| var nonUtcDateTime = new DateTime(2025, 8, 25, 10, 0, 0, kind); | ||
|
|
||
| // Act & Assert | ||
| Assert.Throws<ArgumentException>(() => _notificationScheduleService.GetEmailExpirationDateTime(nonUtcDateTime)); | ||
| } |
Contributor
There was a problem hiding this comment.
Cover the 09:00/17:00 boundaries explicitly.
The current schedule helper uses strict comparisons, so exactly-on-the-hour times are treated as outside the window. For the new email flow that means a daytime trigger at the window opening can return 200 OK without enqueueing anything. Please add exact-boundary cases here and align the helper if the intended policy is “09–17” with an inclusive start.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/api/test/Altinn.Notifications.Tests/Notifications.Core/TestingServices/NotificationScheduleServiceTests.cs`
around lines 132 - 229, Add explicit boundary tests for 09:00 and 17:00 and make
the schedule logic use an inclusive start boundary. Concretely: add unit tests
calling CanSendEmailNow() with mocked UtcNow at exactly 09:00 (assert True) and
exactly 17:00 (assert False), and add corresponding GetEmailExpirationDateTime
tests for requestedSendTime at 09:00 and 17:00 asserting the expected expiry
behavior; then update the scheduling helper/NotificationScheduleService
comparisons (where it currently uses strict > and <) to use >= sendWindowStart
&& < sendWindowEnd so start is inclusive and end remains exclusive. Ensure tests
reference CanSendEmailNow and GetEmailExpirationDateTime to locate behavior.
|
olebhansen
marked this pull request as draft
July 7, 2026 12:51
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.
Summary
Adds support for
"sendingTimePolicy": "Daytime"on email orders, mirroring the existing SMS Daytime pattern.Closes #1530.
Commits (in review order)
notifications.orders.emailsendingtimepolicy(nullable, no backfill), modifiesclaim_email_batchto filter(NULL OR = 1), addsclaim_daytime_email_batch(= 2), and a newinsertorderoverload.EmailSendingOptions(Ext).SendingTimePolicybecomesSendingTimePolicy?; validator allows null + Daytime + Anytime;NotificationOrder.EmailSendingTimePolicyflows through the mapper /OrderRequestService;OrderRepositorywrites the new column.IEmailNotificationRepository.GetNewNotificationsAsyncgains aSendingTimePolicyparameter (default Anytime) that selects between the two SQL functions.IEmailPublishTaskQueuemirrorsISmsPublishTaskQueuewith per-policy channels;EmailPublishBackgroundServiceruns two concurrent loops;/trigger/sendemailenqueues explicitly with Anytime.EmailSendWindowStart/EndHour(defaults 09–17),INotificationScheduleService.CanSendEmailNow()andGetEmailExpirationDateTime(...).EmailNotificationService.CreateNotificationtakes expiry from caller;EmailOrderProcessingServiceusesGetEmailExpirationDateTimefor Daytime,RequestedSendTime + 48hotherwise.CanSendEmailNow(), then enqueues Daytime; always returns 200 OK.Trigger_SendEmailNotificationsTestsmirroring the SMS variant. 1065 unit tests green (8 added).The commits are intentionally split so individual pieces can be cherry-picked or reverted if anything turns out to need rework.
Backwards compatibility
EmailSendingOptionsExt.SendingTimePolicyis now nullable with no[DefaultValue]; clients that don't send the field stay NULL all the way to the DB.claim_email_batchclaims rows whereemailsendingtimepolicy IS NULL OR = 1, so legacy orders are picked up by the existing/trigger/sendemailcron with no behavior change.notifications.orders.sendingtimepolicycolumn (used by SMS) is intentionally not renamed. The naming asymmetry is accepted; a follow-up rename can be done once Daytime emails are stable in prod.Out of scope
/sendemaildaytimein the infra repo — tracked as a follow-up sub-issue of Support Daytime-policy for emails #1530.sendingtimepolicy→smssendingtimepolicyand considering aSendingTimePolicieswrapper record onNotificationOrder— tracked as a separate cleanup issue.Test plan
dotnet buildclean across all projects (Core, Persistence, Integrations, API, Tests, IntegrationTests, IntegrationTestsASB, Tools)Altinn.Notifications.Tests— 1065/1065 passAltinn.Notifications.IntegrationTests— to run in CI against the test databasesendingTimePolicy: "Daytime"and confirm it is only picked up via/trigger/sendemaildaytimeinside the configured window/trigger/sendemail🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Configuration
EmailSendWindowStartHourandEmailSendWindowEndHoursettings.