Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(breaking): Move notification check endpoint to /actions #1175

Merged
merged 5 commits into from
Sep 23, 2024
Merged

feat(breaking): Move notification check endpoint to /actions #1175

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e6127a9
remove
oskogstad Sep 23, 2024
cee23e2
??
oskogstad Sep 23, 2024
ea1e158
fix test
oskogstad Sep 23, 2024
3df0ec3
Merge branch 'main' into chore/remove-ignore-query-filter
oskogstad Sep 23, 2024
1337420
Merge branch 'main' into chore/remove-ignore-query-filter
oskogstad Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 78 additions & 78 deletions docs/schema/V1/swagger.verified.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5561,6 +5561,84 @@
]
}
},
"/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"
]
}
},
Comment on lines +5564 to +5641
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

Suggested improvements not implemented

The proposed enhancements in the review comment have not been fully addressed:

  1. 400 Bad Request Response Missing: The new endpoint /api/v1/serviceowner/dialogs/{dialogId}/actions/should-send-notification does not include a 400 Bad Request response for invalid query parameters.
  2. Endpoint Naming: The endpoint naming remains /api/v1/serviceowner/dialogs/{dialogId}/actions/should-send-notification instead of adopting a more RESTful convention like /api/v1/serviceowner/dialogs/{dialogId}/notification-conditions.
  3. 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:

  1. The endpoint is well-documented with a clear description and purpose.
  2. Required query parameters (conditionType, activityType) are properly specified.
  3. The response is a boolean value, which is appropriate for the use case.
  4. 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:

  1. Add more detailed error responses, such as 400 Bad Request for invalid query parameters.
  2. Consider using a more RESTful naming convention for the endpoint, e.g., /api/v1/serviceowner/dialogs/{dialogId}/notification-conditions instead of using an "actions" subfolder.
  3. 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

"/api/v1/serviceowner/dialogs/{dialogId}/activities": {
"get": {
"description": "Gets the list of activities belonging to a dialog",
Expand Down Expand Up @@ -5767,84 +5845,6 @@
]
}
},
"/api/v1/serviceowner/dialogs/{dialogId}/notification-condition": {
"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"
]
}
},
"/api/v1/serviceowner/dialogs/{dialogId}/seenlog": {
"get": {
"description": "Gets all seen log records for a dialog. For more information see the documentation (link TBD).",
Expand Down
21 changes: 6 additions & 15 deletions ...ServiceOwner/DialogActivities/Queries/NotificationCondition/NotificationConditionQuery.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,9 @@ public async Task<NotificationConditionResult> Handle(NotificationConditionQuery
{
var dialog = await _db.Dialogs
.AsNoTracking()
.Include(x => x.Activities)
.IgnoreQueryFilters()
.Include(x => x.Activities
.Where(x => request.TransmissionId == null || x.TransmissionId == request.TransmissionId)
.Where(x => x.TypeId == request.ActivityType))
.FirstOrDefaultAsync(x => x.Id == request.DialogId,
cancellationToken: cancellationToken);

Expand All @@ -48,20 +49,10 @@ public async Task<NotificationConditionResult> Handle(NotificationConditionQuery
return new EntityNotFound<DialogEntity>(request.DialogId);
}

var conditionMet = CheckDialogActivitiesCondition(dialog.Activities, request.ConditionType, request.ActivityType, request.TransmissionId);
var conditionMet = dialog.Activities.Count == 0
? request.ConditionType == NotificationConditionType.NotExists
: request.ConditionType == NotificationConditionType.Exists;
oskogstad marked this conversation as resolved.
Show resolved Hide resolved

return new NotificationConditionDto { SendNotification = conditionMet };
}

private static bool CheckDialogActivitiesCondition(
List<DialogActivity> activities,
NotificationConditionType conditionType,
DialogActivityType.Values activityType,
Guid? transmissionId) =>
activities.Where(
x => x.TypeId == activityType
&& (transmissionId is null || x.TransmissionId == transmissionId)).ToList()
.Count == 0
? conditionType == NotificationConditionType.NotExists
: conditionType == NotificationConditionType.Exists;
}
2 changes: 1 addition & 1 deletion ...s/V1/ServiceOwner/DialogActivities/NotificationCondition/NotificationConditionEndpoint.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public NotificationConditionEndpoint(ISender sender)

public override void Configure()
{
Get("dialogs/{dialogId}/notification-condition");
Get("dialogs/{dialogId}/actions/should-send-notification");
oskogstad marked this conversation as resolved.
Show resolved Hide resolved
Policies(AuthorizationPolicy.NotificationConditionCheck);
Group<ServiceOwnerGroup>();

Expand Down
Loading