Features for 1.0.0 of package #60

schiwekM · 2024-09-14T16:39:57Z

Hi colleagues,

Inline with cap/dev#1038 this PR includes many desired features for a productive usage of the package.

Own deployment module. Currently cds add would generate a CF app to deploy the types which is basically the srv module again, causing issues with the size, because all dependencies of the main app which uses the plugin are used again. With this PR there is a small deployer module for deploying the types
Proper build step, which builds the notification types with option for apps to specify via cds.env.requires.notifications.build.after or cds.env.requires.notifications.build.after a path to a file modifying the build results. The use case for example is to modify the EmailTemplate property for all notification types
Support for recipient property being an object in alert.notify('Type', {recipient: '', data: {}})
Support for messages being an array in alert.notify('Type', [{recipient: '', data: {}}, {recipients: [], data: {}}])
Sets SaaS dependency to alert-notification for MTX usage.
Adds Notifications cds entity to store Notifications locally.
- alert.emit('deleteNotifications', {}) to delete old notifications. Archiving period can be configured via cds.env.requires.notifications.notifications_deletion_timeout
- index.cds added which has to be imported for the entity to be used / Notifications to be stored
Use cds.context.locale for the Language property of the Properties array in Notifications
Notification types
- Support for MinTimeBetweenNotifications on notification types to specify values like 7d expressing that at least 7 days have to pass for another notification of that type with the same target parameter
- Allow 'ID' instead of 'NotificationTypeKey'
- Allow for 'DeliveryChannels' being an object with the properties 'Mail' or 'Web' to toggle those. For ANS NotificationTypes it is converted to an array
- Allow specify which data properties are TargetParameters via TargetParameters attribute. Array of strings. ID is used as a TargetParameter if none are specified.
- Allow to specify 'Priority' to give a default priority for this NotificationType
- Allows to specify 'Navigation.SemanticObject' and 'Navigation.SemanticAction' to specify the target app of the notification type
- Allows to specify 'Properties' an object with each entry being the property name and for which 'IsSensitive' and 'Type' can be specified. The current implementation uses 'typeof Value' and with that Dates would be Strings
- Support Localisation of 'TemplateSensitive', 'TemplatePublic', 'TemplateGrouped', 'Description', 'Subtitle', 'EmailSubject', 'EmailHtml' and 'EmailText'. For Localisation they can be directly specified in the NotificationType without 'Templates' being specified and it will use the string of that property for i18n lookup at the first cds.i18n.folders and inside it 'notifications.properties'. The Value of those properties can also be an object with 'code' and 'args' as properties. args is an array of strings and specified the order in which properties should be inserted into the i18n placeholders
Placeholder support for global user id. Setting cds.env.features.use_global_user_uuid will set the recipient value to the 'GlobalUserId' property rather than the 'RecipeintId' property.

Open points

'cds.env.features.use_global_user_uuid' fine to use?
Is the simplified Notification type, like the following fine? (@oklemenz2)

{
    "ID": "SimpleType",
    "TemplateSensitive": "I18NKEY",
    "TemplatePublic": {
        "code": "I18NKEY",
        "args": ["property1", "property2"]
    },
    "TemplateGrouped": "I18NKEY",
    "Subtitle": "I18NKEY",
    "Description": "I18NKEY",
    "EmailSubject": "I18NKEY",
    "EmailHtml": "I18NKEY",
    "EmailText": "I18NKEY",
    "DeliveryChannels": {
        "Mail": true,
        "Web": true
    },
    "Priority": "Neutral,Medium,High,Low",
    "Navigation": {
        "SemanticObject": "",
        "SemanticAction": ""
    },
    "MinTimeBetweenNotifications": 0,
    "Properties": {
        "property1": {
            "IsSensitive": true,
            "Type": "String"
        },
        "property2": {
            "IsSensitive": true,
            "Type": "Boolean"
        }
    },
    "TargetParameters": ["property1"]
}

notification-types.json can get quite lengthy and I am thinking about adding support for specifying multiple files. Thoughts? Personally I think this is better than a CSV file with all types. We have this approach currently in our project and it makes things tedious, especially for Properties configuration and I'd say also less clear.
Adding tests. So far I only tested with our app and a variety of cases but still have to write the test cases. Before that, I just wanted to get any feedback to include.

BR,
Marten

oklemenz2 · 2024-09-17T06:22:13Z

Is the simplified Notification type, like the following fine? (@oklemenz2)
Looks fine from my perspective at first glance. @soccermax Anything you want to add?

Still I would find a more a definition more close to the CDS language a good choice as well.
Is multi-tenancy also covered by the deployer? What is the Notification cds entity good for, I think, the persistent outbox should store them anyways, right?

soccermax · 2024-09-17T07:08:03Z

lib/deployer/notificationTypes.js

+const { createLogger } = require("@sap-cloud-sdk/util");
+const LOG = createLogger('notifications');


what is the advantage of using @sap-cloud-sdk/util over cds.log. IMO cds.log should cover all the required features and if not those should be requested

The intention was that the deployer module runs with the bare minimum required dependencies and utils is anyways needed for executeHttpRequest thus @sao/cds as a dependency for the deployer can be avoided.

soccermax · 2024-09-17T07:10:49Z

lib/utils.js

+    Recipients: recipients?.map(id => {
+      if (cds.env.features.use_global_user_uuid)
+        return { GlobalUserId: id }
+      return { RecipientId: id }
+    })


this should be more dynamic. As not necessarily all users have a global user id. So I think this can't be solved on a static feature

Can you share more details (on teams)? With my naive assumption every solution using IAS+AMS should only have users with a global uuid shouldn't they?

soccermax · 2024-09-17T07:16:16Z

srv/service.js

+      if (req.event === 'deleteNotifications') return next()
+      LOG._debug && LOG.debug('Handling notification event:', req.event);
+      let notificationsToCreate = [];


What is the use case of alert.emit('deleteNotifications', {}). I'm not aware of any reason why we should archiving notifications can you elaborate on this?

Saving notifications is done so a 'MinTimeBetweenNotifications' can be specified avoiding to many notifications. But to do these checks notifications have to be saved. I did not yet find a clean way for scheduling a job to trigger deletion automatically, that is why the event exists so apps can trigger it themselves via a scheduler. I thought about adding a HANA Cloud Task Scheduler, but that would't not work for pg/sqlite.

I have one use case where I send an action to multiple people.
If one person actions it, then I can delete the notification for the rest of the recipients.

Hi @TheoExall, I think the event does not yet allow to delete specific notifications. However please also consider that Notification actions is something that ideally should be avoided. That is also why there likely won't be any support coming for NotificationType actions. The preferred way is to use SAP Task Center and create a task assigned to multiple people, with them receiving the notification for the task.

For task scheduling you can use the event-queue package. This is exactly the purpose of the package.
But however, I still would not add such functionality in a reuse package. For me this seems more application-specific. Because we handle such things in our business coding and would not want additional tables for such things...

@schiwekM
Correct. I have a really small app that is used by IT staff. In my use case there is around 80 notifications per month with around 40 recipients. In this case it's more about cleaning up older notifications, for managing the tasks I agree that Task Center is the best solution

For task scheduling you can use the event-queue package. This is exactly the purpose of the package. But however, I still would not add such functionality in a reuse package. For me this seems more application-specific. Because we handle such things in our business coding and would not want additional tables for such things...

Thanks for mentioning the package - I am just unsure if it is fine to add it as an optional dependency. The tables / functionality is optional. If you do not import the index.cds no table will be added & notifications will be saved.

schiwekM · 2024-09-17T08:46:18Z

Is the simplified Notification type, like the following fine? (@oklemenz2)
Looks fine from my perspective at first glance. @soccermax Anything you want to add?

Still I would find a more a definition more close to the CDS language a good choice as well. Is multi-tenancy also covered by the deployer? What is the Notification cds entity good for, I think, the persistent outbox should store them anyways, right?

Similar to CDS data allow json & csv data? / for localisation just the localised attribute? In our app we have it currently and I have to say it is very clumsy for >50 notifications.

Deployer is not yet multi tenant, e.g. no create/update handler yet registered to push the types, because there are ongoing discussions if onboarding notification types can be simplified for tenants.

schiwekM added 5 commits May 29, 2024 23:06

Use cds.context.locale instead of hardcoded en

d2be26b

Merge branch 'main' into feat-use-context-language

67d59b6

Fixed issue with languages

a8d68ab

Features for 1.0.0

7b59084

Naming changes + fixes for global user id

2b6be1b

schiwekM requested a review from danjoa September 14, 2024 16:39

schiwekM mentioned this pull request Sep 14, 2024

Use cds.context.locale instead of hardcoded en #55

Closed

This was linked to issues Sep 14, 2024

Application crashing when trying to trigger alert #43

Open

Detection of locale not possible with hardcoded property language #53

Open

Emit Failed because NotificationType doesn't exists #58

Open

schiwekM added 2 commits September 14, 2024 19:42

Also store properties

91f62b8

Fix modelling

e5a6d79

soccermax reviewed Sep 17, 2024

View reviewed changes

RamIndia requested a review from simeonPetkov96 September 18, 2024 07:34

schiwekM removed the request for review from danjoa October 31, 2024 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Features for 1.0.0 of package #60

Features for 1.0.0 of package #60

schiwekM commented Sep 14, 2024

oklemenz2 commented Sep 17, 2024

soccermax Sep 17, 2024

schiwekM Sep 17, 2024

soccermax Sep 17, 2024

schiwekM Sep 17, 2024

soccermax Sep 17, 2024

schiwekM Sep 17, 2024

TheoExall Sep 17, 2024

schiwekM Sep 17, 2024

soccermax Sep 17, 2024

TheoExall Sep 17, 2024

schiwekM Sep 17, 2024

schiwekM commented Sep 17, 2024

		const { createLogger } = require("@sap-cloud-sdk/util");
		const LOG = createLogger('notifications');

Features for 1.0.0 of package #60

Are you sure you want to change the base?

Features for 1.0.0 of package #60

Conversation

schiwekM commented Sep 14, 2024

Open points

oklemenz2 commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schiwekM commented Sep 17, 2024