feat(breaking): Move notification check endpoint to /actions #1175
Conversation
WalkthroughWalkthroughThe pull request introduces modifications to the notification condition querying process in the application. It updates the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (2)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogActivities/Queries/NotificationCondition/NotificationConditionQuery.cs (2 hunks)
- src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogActivities/NotificationCondition/NotificationConditionEndpoint.cs (1 hunks)
Additional comments not posted (2)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogActivities/Queries/NotificationCondition/NotificationConditionQuery.cs (2)
41-43: Improved query efficiency with filtered IncludeThe changes to the
Includemethod for activities are a significant improvement:
- It now filters activities at the database level, which is more efficient than fetching all activities and filtering in memory.
- The
TransmissionIdfilter is optional, allowing flexibility in the query.- The
ActivityTypefilter ensures only relevant activities are included, further optimizing the query.These changes align well with the PR objective of moving query logic to the database and should improve overall performance.
Line range hint
1-58: Overall improvements to notification condition queryingThe changes in this file significantly improve the notification condition querying process:
- The query logic has been moved to the database level, improving efficiency.
- The implementation now ensures that notifications are not sent for deleted dialogs, aligning with the PR objectives.
- The code has been simplified, making it easier to maintain and understand.
These changes should result in a more reliable and performant notification system. Great work on the refactoring!
...ints/V1/ServiceOwner/DialogActivities/NotificationCondition/NotificationConditionEndpoint.cs
Show resolved
Hide resolved
...V1/ServiceOwner/DialogActivities/Queries/NotificationCondition/NotificationConditionQuery.cs
Show resolved
Hide resolved
| "/api/v1/serviceowner/dialogs/{dialogId}/actions/should-send-notification": { | ||
| "get": { | ||
| "description": "Used by Altinn Notification only. Takes a dialogId and returns a boolean value based on conditions used to determine if a notification is to be sent.", | ||
| "operationId": "GetDialogActivityNotificationConditionSO", | ||
| "parameters": [ | ||
| { | ||
| "in": "path", | ||
| "name": "dialogId", | ||
| "required": true, | ||
| "schema": { | ||
| "format": "guid", | ||
| "type": "string" | ||
| } | ||
| }, | ||
| { | ||
| "in": "query", | ||
| "name": "conditionType", | ||
| "required": true, | ||
| "schema": { | ||
| "allOf": [ | ||
| { | ||
| "$ref": "#/components/schemas/NotificationConditionType" | ||
| } | ||
| ] | ||
| } | ||
| }, | ||
| { | ||
| "in": "query", | ||
| "name": "activityType", | ||
| "required": true, | ||
| "schema": { | ||
| "allOf": [ | ||
| { | ||
| "$ref": "#/components/schemas/DialogActivityType_Values" | ||
| } | ||
| ] | ||
| } | ||
| }, | ||
| { | ||
| "in": "query", | ||
| "name": "transmissionId", | ||
| "schema": { | ||
| "format": "guid", | ||
| "nullable": true, | ||
| "type": "string" | ||
| } | ||
| } | ||
| ], | ||
| "responses": { | ||
| "200": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": {} | ||
| }, | ||
| "text/plain": { | ||
| "schema": {} | ||
| } | ||
| }, | ||
| "description": "Successfully returned the notification determination." | ||
| }, | ||
| "401": { | ||
| "description": "Missing or invalid authentication token. Requires a Maskinporten-token with the scope \u0022altinn:system/notifications.condition.check\u0022." | ||
| }, | ||
| "403": { | ||
| "description": "Forbidden" | ||
| } | ||
| }, | ||
| "security": [ | ||
| { | ||
| "JWTBearerAuth": [] | ||
| } | ||
| ], | ||
| "summary": "Returns a boolean value based on conditions used to determine if a notification is to be sent", | ||
| "tags": [ | ||
| "Serviceowner" | ||
| ] | ||
| } | ||
| }, |
There was a problem hiding this comment.
Tip
Codebase Verification
Suggested improvements not implemented
The proposed enhancements in the review comment have not been fully addressed:
- 400 Bad Request Response Missing: The new endpoint
/api/v1/serviceowner/dialogs/{dialogId}/actions/should-send-notificationdoes not include a400 Bad Requestresponse for invalid query parameters. - Endpoint Naming: The endpoint naming remains
/api/v1/serviceowner/dialogs/{dialogId}/actions/should-send-notificationinstead of adopting a more RESTful convention like/api/v1/serviceowner/dialogs/{dialogId}/notification-conditions. - Response Schema: The response still returns only a boolean value without additional details, such as a reason for the notification decision.
Analysis chain
New endpoint for checking notification conditions added
A new GET endpoint /api/v1/serviceowner/dialogs/{dialogId}/actions/should-send-notification has been added to the API. This endpoint is specifically for Altinn Notification use and determines if a notification should be sent based on certain conditions. The implementation looks good overall, but there are a few points to consider:
- The endpoint is well-documented with a clear description and purpose.
- Required query parameters (conditionType, activityType) are properly specified.
- The response is a boolean value, which is appropriate for the use case.
- The security scheme requires a Maskinporten token with the scope "altinn:system/notifications.condition.check", which is different from other endpoints in this API.
Consider the following suggestions to improve the endpoint:
- Add more detailed error responses, such as 400 Bad Request for invalid query parameters.
- Consider using a more RESTful naming convention for the endpoint, e.g.,
/api/v1/serviceowner/dialogs/{dialogId}/notification-conditionsinstead of using an "actions" subfolder. - If possible, provide more information in the response, such as a reason for the decision, which could be useful for debugging or logging purposes.
Example of a more detailed response schema:
{
"shouldSendNotification": true,
"reason": "Condition met: New activity of type 'Information' added"
}To ensure this endpoint is properly integrated with the rest of the API, let's check for any related models or references:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for references to the new endpoint or its models
grep -rn "should-send-notification" .
grep -rn "NotificationConditionType" .
grep -rn "altinn:system/notifications.condition.check" .
Length of output: 1922
Script:
#!/bin/bash
# Check for additional error responses like 400 Bad Request
grep -rn '"400"' ./docs/schema/V1/swagger.verified.json
# Check if the endpoint naming follows a more RESTful convention
grep -rn "/api/v1/serviceowner/dialogs/{dialogId}/notification-conditions" ./docs/schema/V1/swagger.verified.json
# Inspect the response schema for additional details
grep -rn '"shouldSendNotification"' ./docs/schema/V1/swagger.verified.json
Length of output: 370
|
oskogstad
changed the title
oskogstad
changed the title
🤖 I have created a release *beep* *boop* --- ## [1.19.0](v1.18.1...v1.19.0) (2024-09-24) ### Features * **breaking:** Move notification check endpoint to /actions ([#1175](#1175)) ([e0c1cf2](e0c1cf2)) ### Bug Fixes * **janitor:** ensure Redis is configured correctly ([#1182](#1182)) ([37fe982](37fe982)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
/actions/should-send-notificationSummary by CodeRabbit
New Features
Bug Fixes