Feature/784 new trigger endpoint #795
Conversation
…784-new-trigger-endpoint
📝 WalkthroughWalkthroughThis pull request updates multiple components of the SMS notification system to support sending time policies. The changes introduce a new parameter, Changes
Possibly related PRs
Suggested reviewers
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/Altinn.Notifications.Persistence/Migration/FunctionsAndProcedures/getsmsstatusnewupdatestatus.sql (1)
25-65: LGTM: SQL function correctly extended with SendingTimePolicy filtering.The new function overload properly implements the filtering based on sending time policy with appropriate handling of NULL values. The detailed comment at the end is helpful for future maintainers.
Note that the original function without parameters is kept, which may be intentional for backward compatibility, but consider adding a deprecation comment if it should no longer be used.
Consider adding a deprecation comment to the original function if it should no longer be used:
+-- This function is kept for backward compatibility and may be removed in future versions. +-- Use notifications.getsms_statusnew_updatestatus(integer) instead. CREATE OR REPLACE FUNCTION notifications.getsms_statusnew_updatestatus()test/Altinn.Notifications.IntegrationTests/Utils/TestdataUtil.cs (2)
39-57: Method name contains a typo.The method name
GeOrderAndSmsAnytimeNotificationis missing a 't' and should beGetOrderAndSmsAnytimeNotification.-public static (NotificationOrder Order, SmsNotification Notification) GeOrderAndSmsAnytimeNotification() +public static (NotificationOrder Order, SmsNotification Notification) GetOrderAndSmsAnytimeNotification()
39-57: Consider refactoring to reduce code duplication.There's significant duplication between
GeOrderAndSmsAnytimeNotificationandGetOrderAndSmsNotification. Consider refactoring to have the first method call the second with appropriate parameters.-public static (NotificationOrder Order, SmsNotification Notification) GeOrderAndSmsAnytimeNotification() +public static (NotificationOrder Order, SmsNotification Notification) GetOrderAndSmsAnytimeNotification() { - NotificationOrder order = NotificationOrder_SmsTemplate_OneRecipient(); - order.Id = Guid.NewGuid(); - var recipient = order.Recipients[0]; - SmsAddressPoint? addressPoint = recipient.AddressInfo.Find(a => a.AddressType == AddressType.Sms) as SmsAddressPoint; - var smsNotification = new SmsNotification() - { - Id = Guid.NewGuid(), - OrderId = order.Id, - RequestedSendTime = DateTime.UtcNow, - Recipient = new() - { - MobileNumber = addressPoint!.MobileNumber, - }, - SendResult = new(SmsNotificationResultType.New, DateTime.UtcNow) - }; - return (order, smsNotification); + var (order, notification) = GetOrderAndSmsNotification(SendingTimePolicy.Anytime); + notification.RequestedSendTime = DateTime.UtcNow; + return (order, notification); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/Altinn.Notifications.Core/Persistence/ISmsNotificationRepository.cs(1 hunks)src/Altinn.Notifications.Core/Services/Interfaces/ISmsNotificationService.cs(2 hunks)src/Altinn.Notifications.Core/Services/SmsNotificationService.cs(1 hunks)src/Altinn.Notifications.Persistence/Migration/FunctionsAndProcedures/getsmsstatusnewupdatestatus.sql(1 hunks)src/Altinn.Notifications.Persistence/Migration/v0.40/01-functions-and-procedures.sql(1 hunks)src/Altinn.Notifications.Persistence/Repository/SmsNotificationRepository.cs(2 hunks)src/Altinn.Notifications/Controllers/TriggerController.cs(1 hunks)test/Altinn.Notifications.IntegrationTests/Notifications.Persistence/SmsNotificationRepositoryTests.cs(1 hunks)test/Altinn.Notifications.IntegrationTests/Utils/PostgreUtil.cs(2 hunks)test/Altinn.Notifications.IntegrationTests/Utils/TestdataUtil.cs(2 hunks)test/Altinn.Notifications.Tests/Notifications.Core/TestingServices/SmsNotificationServiceTests.cs(2 hunks)test/Altinn.Notifications.Tests/Notifications/TestingControllers/TriggerControllerTests.cs(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
test/Altinn.Notifications.IntegrationTests/Utils/TestdataUtil.cs (2)
src/Altinn.Notifications.Core/Models/Orders/NotificationOrder.cs (4)
NotificationOrder(11-133)NotificationOrder(56-80)NotificationOrder(85-88)NotificationOrder(101-104)src/Altinn.Notifications.Core/Models/Notification/SmsNotification.cs (1)
SmsNotification(9-41)
src/Altinn.Notifications.Persistence/Repository/SmsNotificationRepository.cs (2)
src/Altinn.Notifications.Core/Persistence/ISmsNotificationRepository.cs (4)
Task(20-20)Task(27-27)Task(34-34)Task(43-43)src/Altinn.Notifications.Core/Services/SmsNotificationService.cs (4)
Task(44-63)Task(66-77)Task(80-83)Task(94-106)
test/Altinn.Notifications.IntegrationTests/Utils/PostgreUtil.cs (3)
src/Altinn.Notifications.Core/Persistence/ISmsNotificationRepository.cs (4)
Task(20-20)Task(27-27)Task(34-34)Task(43-43)test/Altinn.Notifications.IntegrationTests/Utils/TestdataUtil.cs (6)
NotificationOrder(12-37)NotificationOrder(39-57)NotificationOrder(59-79)NotificationOrder(84-118)NotificationOrder(123-155)TestdataUtil(10-156)src/Altinn.Notifications.Core/Models/Orders/NotificationOrder.cs (4)
NotificationOrder(11-133)NotificationOrder(56-80)NotificationOrder(85-88)NotificationOrder(101-104)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build, test & analyze
🔇 Additional comments (28)
src/Altinn.Notifications.Core/Services/Interfaces/ISmsNotificationService.cs (2)
1-1: Appropriate import addition for the SendingTimePolicy enum.The import statement for the Enums namespace is needed for the SendingTimePolicy parameter.
29-29: Interface method updated to support filtering by sending time policy.The method signature has been updated to include a SendingTimePolicy parameter, which aligns with the PR objective of creating a separate trigger endpoint for SMS notifications with different sending time policies.
test/Altinn.Notifications.Tests/Notifications.Core/TestingServices/SmsNotificationServiceTests.cs (4)
190-191: Updated mock to account for the new SendingTimePolicy parameter.The mock setup correctly uses It.IsAny() to accommodate the interface change.
212-213: Updated mock setup for the SendingTimePolicy parameter.The mock setup correctly uses It.IsAny() to match the updated interface.
225-225: Test method doesn't specify SendingTimePolicy parameter.Similar to the previous test, this calls SendNotifications() without parameters but should work with the default parameter value.
200-200:✅ Verification successful
Test method doesn't specify SendingTimePolicy parameter.
The test calls SendNotifications() without parameters, but should still work as the implementation provides a default value of SendingTimePolicy.Daytime.
🏁 Script executed:
#!/bin/bash # Check SmsNotificationService implementation to confirm default parameter exists grep -A3 "public async Task SendNotifications" src/Altinn.Notifications.Core/Services/SmsNotificationService.csLength of output: 341
Defaults Verified – No Test Change Required
The implementation of
SendNotificationsinSmsNotificationServiceconfirms that the default parameter (SendingTimePolicy.Daytime) is provided. Calling the method without parameters, as done in the test, is correct.src/Altinn.Notifications.Core/Services/SmsNotificationService.cs (2)
66-66: Method implementation updated with SendingTimePolicy parameter and default value.The method now accepts a SendingTimePolicy parameter with a default value of SendingTimePolicy.Daytime, ensuring backward compatibility with existing code calling this method without parameters.
68-68: Repository call updated to pass the SendingTimePolicy parameter.The GetNewNotifications call now correctly passes the sendingTimePolicy parameter to the repository layer.
test/Altinn.Notifications.Tests/Notifications/TestingControllers/TriggerControllerTests.cs (6)
5-5: Added import for SendingTimePolicy enum.The import statement for the Enums namespace is correctly added to support the SendingTimePolicy parameter in the tests.
25-25: Updated mock setup to include SendingTimePolicy parameter.The mock setup for the SmsNotificationService now correctly uses It.IsAny() to match the updated interface.
43-43: Method name changed from Trigger_SendSmsNotifications to Trigger_SendSmsNotificationsDaytime.The test now calls a different controller method that likely filters notifications by daytime policy, which aligns with the PR objective.
46-46: Verification updated to include SendingTimePolicy parameter.The verification now correctly checks the call to SendNotifications with It.IsAny().
58-58: Updated method call to Trigger_SendSmsNotificationsDaytime.Consistent with the change in the first test, this test also calls the updated controller method.
61-61: Verification updated to include SendingTimePolicy parameter.The verification correctly checks the call to SendNotifications with It.IsAny().
test/Altinn.Notifications.IntegrationTests/Utils/PostgreUtil.cs (2)
1-1: LGTM: Required using directive added for SendingTimePolicy enum.The new using directive is correctly added to support the SendingTimePolicy enum parameter in the modified method.
97-99: LGTM: Method correctly enhanced to support SendingTimePolicy filtering.The method signature has been appropriately updated to accept an optional SendingTimePolicy parameter, and the parameter is correctly passed to TestdataUtil.GetOrderAndSmsNotification, allowing tests to create orders with specific sending time policies.
test/Altinn.Notifications.IntegrationTests/Notifications.Persistence/SmsNotificationRepositoryTests.cs (1)
94-110: LGTM: Well-structured test for the new SendingTimePolicy.Anytime functionality.The new test method follows the same pattern as existing tests and properly verifies that the SmsNotificationRepository correctly retrieves notifications when filtering by the SendingTimePolicy.Anytime value.
src/Altinn.Notifications.Core/Persistence/ISmsNotificationRepository.cs (1)
25-27: LGTM: Method signature and documentation updated correctly.The GetNewNotifications method has been properly enhanced to accept a SendingTimePolicy parameter with a sensible default value of SendingTimePolicy.Daytime. The XML documentation clearly explains the parameter's purpose.
src/Altinn.Notifications.Persistence/Repository/SmsNotificationRepository.cs (3)
21-21: SQL constant name accurately represents its purpose.The constant name
_getNewSmsNoticationsSqlWithSendingTimePolicyParameterclearly indicates that it expects a sending time policy parameter. The SQL query itself uses a parameter placeholder ($1) for filtering, which aligns with the PR objectives.
91-97: Well-documented method parameters and purpose.The method documentation clearly explains the purpose of the method, the meaning of the new parameter, and what it returns. This makes the code more maintainable and helps other developers understand how to use the method.
100-102: Proper implementation of parameterized query.The code correctly uses the updated SQL constant and properly adds the sending time policy parameter to the query. This ensures that notifications are filtered based on the specified sending time policy as required.
src/Altinn.Notifications/Controllers/TriggerController.cs (3)
59-67: Route duplication maintains backward compatibility.The method now has two route attributes: the original
sendsmsand the new, more descriptivesendsmsdaytime. This approach ensures backward compatibility while also providing a more descriptive route for new API consumers.
74-76: Explicit sending time policy parameter.The code now explicitly passes
Core.Enums.SendingTimePolicy.Daytimeto the service method, clarifying the intent of this endpoint and implementing the requirement to filter based on sending time policy.
78-89: New endpoint for anytime SMS notifications.The new endpoint
Trigger_SendSmsNotificationsAnytimewith routesendsmsanytimeproperly implements the requirement to provide a separate endpoint for SMS notifications that can be sent at any time. The implementation correctly passesCore.Enums.SendingTimePolicy.Anytimeto the service method.test/Altinn.Notifications.IntegrationTests/Utils/TestdataUtil.cs (1)
12-37: Method enhancement to support sending time policy.The
GetOrderAndSmsNotificationmethod has been correctly enhanced to accept an optional sending time policy parameter, allowing for more flexible test data creation. The implementation properly assigns the policy to the order when provided.src/Altinn.Notifications.Persistence/Migration/v0.40/01-functions-and-procedures.sql (3)
432-464: Well-implemented SQL function to support sending time policy filtering.The SQL function
getsms_statusnew_updatestatushas been properly implemented to accept a sending time policy parameter. The function correctly filters records based on the provided policy and handles the case where the policy is null by treating it as the default Daytime policy (value 2).
466-471: Clear documentation of SQL function behavior.The comment on the function clearly explains its purpose, parameters, and behavior, including important information about how null values are treated. This documentation is valuable for maintaining the code in the future.
582-606: Function overloading provides flexibility while maintaining backward compatibility.The overloaded
insertorderfunction allows for specifying a sending time policy while maintaining backward compatibility with existing code that doesn't provide this parameter. This is a good approach for extending functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/Altinn.Notifications.Persistence/Migration/v0.40/01-functions-and-procedures.sql (3)
407-432: Legacy SMS Status Update innotifications.getsms_statusnew_updatestatus
This legacy function is maintained for backward compatibility. Consider adding clear documentation and possibly a deprecation note if you plan to remove it in a future release.
434-467: New SMS Status Update with Sending Time Policy innotifications.getsms_statusnew_updatestatus(integer)
This new function extends the SMS status update by accepting a_sendingtimepolicyparameter. The logic at line 463—using the condition
WHERE o.sendingtimepolicy = _sendingtimepolicy OR (o.sendingtimepolicy IS NULL AND _sendingtimepolicy = 2)—ensures that if the order’s policy is null, it defaults to the ‘Daytime’ setting (assumed to be represented by 2).Suggestions:
• Add inline documentation (or a comment block) at the beginning of the function to clearly define the expected integer values and their meaning (e.g., which value represents Daytime vs. Anytime).
• Consider whether additional input validation is needed for_sendingtimepolicyto prevent unexpected behavior if an invalid value is passed.
584-608: Overloaded Order Insertion with Sending Time Policy innotifications.insertorder
The new overloaded version now captures an additional_sendingtimepolicyparameter and inserts it into the orders table. This supports the new functionality for filtering notifications by sending time policy.Suggestions:
• Verify that all callers of theinsertorderfunction are updated to supply the_sendingtimepolicyparameter as needed.
• Consider adding input validation or a defaulting mechanism (if applicable) to handle cases where the sending time policy is not explicitly provided.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Altinn.Notifications.Persistence/Migration/FunctionsAndProcedures/getsmsstatusnewupdatestatus.sql(2 hunks)src/Altinn.Notifications.Persistence/Migration/v0.40/01-functions-and-procedures.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Altinn.Notifications.Persistence/Migration/FunctionsAndProcedures/getsmsstatusnewupdatestatus.sql
🔇 Additional comments (16)
src/Altinn.Notifications.Persistence/Migration/v0.40/01-functions-and-procedures.sql (16)
3-64: Cancellation Logic and Order Retrieval innotifications.cancelorder
The function correctly retrieves the order record and handles the “not found” case. However, note the condition on line 48 where the check combines the requested send time and processed status with an OR. Please verify that this logic (i.e. allowing cancellation to be denied when either the order’s send time is too near or its status is not 'Registered') exactly matches the intended business rules.
67-85: Email Recipients Retrieval innotifications.getemailrecipients_v2
This function straightforwardly retrieves email recipient details using a subquery to obtain the internal order id. The implementation is clear and consistent.
87-139: Email Status Update innotifications.getemails_statusnew_updatestatus
The function manages the email timeout and resets it if needed before updating emails marked as ‘New’ to ‘Sending’. The use of a conditional RETURN QUERY and subsequent update is well structured.
142-173: Email Summary Retrieval innotifications.getemailsummary_v2
This function returns a summary of email notifications using a primary query followed by a fallback query if no rows are found. Please double-check that the use of two RETURN QUERY blocks (and the IF NOT FOUND check) behaves as expected in your PL/pgSQL context.
174-205: Metrics Aggregation innotifications.getmetrics
The aggregation of order, email, and SMS metrics is implemented correctly. For optimal performance on large datasets, ensure that appropriate indexes exist on the date/time columns (e.g.requestedsendtime).
207-283: Order Status and Notification Counts innotifications.getorder_includestatus_v4
The function effectively aggregates both email and SMS counts while retrieving order details. Conversion of JSON fields and conditional null handling forIgnoreReservationis appropriate.
285-347: Order Chain Tracking innotifications.get_orders_chain_tracking
The function checks for the existence of a matching order chain before returning tracking details. The error handling (i.e. returning an empty result) is a good practice for idempotency.
369-386: Updating Past Send Time innotifications.getorders_pastsendtime_updatestatus
The query updates orders that have reached or passed their send time (with a one-minute interval) and limits the update to 50 rows. Please confirm that this fixed limit is appropriate and that any remaining orders are handled by subsequent invocations of the trigger.
387-406: SMS Recipients Retrieval innotifications.getsmsrecipients_v2
A simple, clear extraction of SMS recipient information based on the order identifier. The variable naming and use of subquery for order id are consistent with the rest of the codebase.
476-507: SMS Summary Retrieval innotifications.getsmssummary_v2
This function mirrors the approach used in the email summary by first attempting a join and then providing a fallback query if no results are found. The structure is clear and should work as intended provided that the order and creator parameters are correctly set.
508-556: Inserting Email Notifications innotifications.insertemailnotification
The procedure takes all necessary parameters and performs an INSERT into the email notifications table with proper type casting. The use of a subquery to retrieve the order id is consistent with earlier functions.
557-565: Inserting Email Text innotifications.insertemailtext
The implementation is straightforward and correctly inserts email content into the designated table.
568-583: Basic Order Insertion innotifications.insertorder(First Overload)
This function handles order insertion and returns the generated order id. The implementation follows standard practices and appears correct.
611-640: Order Chain Insertion innotifications.insertorderchain
This procedure correctly inserts a new order chain into the database. The column mapping and ordering are correct.
642-689: Inserting SMS Notifications innotifications.insertsmsnotification
The procedure performs an INSERT into the SMS notifications table with proper data type conversions. The implementation is consistent with similar procedures in this script.
691-700: Email Status Update innotifications.updateemailstatus
This procedure is responsible for updating the status, result time, and operation id of email notifications. The SQL is clear and adheres to expected standards.
| /// <summary> | ||
| /// Retrieves and updates status on all sms that have the status new | ||
| /// Calls database function with sendingTimePolicy parameter to filter result based on sending schedule of choice | ||
| /// </summary> | ||
| /// <param name="sendingTimePolicy">Filtered by the sending time policy set on the order related to the notification row</param> | ||
| /// <returns>List of rows read and updated from the database</returns> | ||
| public async Task<List<Sms>> GetNewNotifications(SendingTimePolicy sendingTimePolicy = SendingTimePolicy.Daytime) |
There was a problem hiding this comment.
We use /// <inheritdoc/> to reduce duplication, improve maintainability, keep documentation consistent and reduce code size
| @@ -18,7 +18,7 @@ public class SmsNotificationRepository : ISmsNotificationRepository | |||
| { | |||
| private readonly NpgsqlDataSource _dataSource; | |||
|
|
|||
| private const string _getNewSmsNotificationsSql = "select * from notifications.getsms_statusnew_updatestatus()"; | |||
| private const string _getNewSmsNoticationsSqlWithSendingTimePolicyParameter = "select * from notifications.getsms_statusnew_updatestatus($1)"; // (_sendingtimepolicy) | |||
There was a problem hiding this comment.
This is a verbose name, and we could keep using the name _getNewSmsNotificationsSql. Anyway it is up to you to decide which one to use.
There was a problem hiding this comment.
I did this because I'm now calling an overload function. Which I think is important to keep in mind. But it could have been an extra comment.
| SELECT u.alternateid, | ||
| st.sendernumber, | ||
| u.mobilenumber, | ||
| CASE WHEN u.customizedbody IS NOT NULL AND u.customizedbody <> '' THEN u.customizedbody ELSE st.body END AS body | ||
| FROM updated u | ||
| JOIN notifications.smstexts st ON u._orderid = st._orderid | ||
| JOIN notifications.orders o ON st._orderid = o._id | ||
| -- sendingTimePolicy = 2 is equal to daytime, the default choice when null | ||
| WHERE o.sendingtimepolicy = _sendingtimepolicy OR (o.sendingtimepolicy IS NULL AND _sendingtimepolicy = 2); |
There was a problem hiding this comment.
We can use the following for clearer intent:
SELECT u.alternateid,
st.sendernumber,
u.mobilenumber,
CASE WHEN u.customizedbody IS NOT NULL AND u.customizedbody <> '' THEN u.customizedbody ELSE st.body END AS body
FROM updated u
JOIN notifications.smstexts st ON u._orderid = st._orderid
JOIN notifications.orders o ON st._orderid = o._id
WHERE
CASE
WHEN _sendingtimepolicy = 1 THEN
o.sendingtimepolicy = 1
WHEN _sendingtimepolicy = 2 THEN
(o.sendingtimepolicy = 2 OR o.sendingtimepolicy IS NULL)
END;
...n.Notifications.Persistence/Migration/FunctionsAndProcedures/getsmsstatusnewupdatestatus.sql
Show resolved
Hide resolved
|
This will introduce the new trigger endpoint which should be called via cronjob in Kubernetes, scheduled to be called every minute of the hour, without exclusion.
Description
The database function that gets new sms notifications with status 'New' must now filter its result based on the order's sending time policy set. The existing trigger endpoint will get results having null or Daytime value, while the new endpoint will get sms notifications related to orders with sending time policy anytime
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit