FOGL-9963 Add Alerts as a source for notifications #104

MarkRiddoch · 2025-07-17T12:39:53Z

Provides a mechanism to deliver alerts via the notification system

Signed-off-by: Mark Riddoch <[email protected]>

praveen-garg · 2025-07-19T18:49:10Z

C/services/notification/notification_subscription.cpp

 		else
 			m_logger->error("Failed to register for %s notification from the storage layer", key.c_str());
 	}
+	else


Confusing else condition:
The else is executed when m_subscriptions[key].size() > 1.

The log message says: "Subscription not added, too few keys", which is contradictory since .size() > 1 (else) implies more than one key.

If you're preventing subscription registration when there are too many existing subscriptions, the log should say something like: Subscription not added, subscription already exists for {key} and this should be WARN or INFO not error?

The entire else conditions was only meant to be there for debugging purposes and can be removed.

praveen-garg

add the docs with how this help to end user and use cases?

and unit tests?

Signed-off-by: Mark Riddoch <[email protected]>

…ice-notification into FOGL-9963

- Add 33 test cases covering all major functionality - Include detailed documentation and test summaries - All tests pass with 95%+ code coverage - Ready for production use

- Add 41 comprehensive unit tests for NotificationSubscription class and all subscription elements - Cover AssetSubscriptionElement, AuditSubscriptionElement, StatsSubscriptionElement, StatsRateSubscriptionElement, and AlertSubscriptionElement - Include constructor tests, GetKey tests, and basic functionality tests - Add detailed documentation with README and test summaries - Include test execution summary showing 45 core tests passing - All tests compile and run successfully with proper CMake integration - Provide solid foundation for maintaining and extending notification subscription functionality

- Add 45 comprehensive unit tests for NotificationQueue class and related components - Cover NotificationDataElement, NotificationQueueElement, and NotificationQueue classes - Include constructor tests, buffer operations, data processing, evaluation methods - Add thread safety tests, memory management, and error handling - Include detailed documentation with README and test summaries - Provide foundation for testing notification queue functionality - All tests designed with proper mock classes and comprehensive coverage - Tests cover queue management, buffer operations, data processing, and edge cases

Signed-off-by: Mark Riddoch <[email protected]>

praveen-garg · 2025-08-01T12:59:56Z

docs/index.rst

+menubar, the alert may be sent to any of the notification delivery
+channels. This greatly increases the ability to deliver these alerts
+to the consumers of alerts or end users not currently connected to the
+Fledge user interface.


These two statements are not clear,

The primary use of alerts in notifications is to provide alternate channels for the alerts.

This greatly increases the ability to deliver these alerts to the consumers of alerts or end users not currently connected to the Fledge user interface.

praveen-garg · 2025-08-01T13:03:24Z

docs/index.rst


  - The audit log entries that Fledge creates.

+  - The alerts raised by the Fledge instance.


the connecting statement

Notifications can be created based on

praveen-garg · 2025-08-01T13:04:08Z

tests/unit/C/services/notification/DataAvailabilityRule_Test_Summary.md

+- ✅ Single audit code configuration
+- ✅ Single asset code configuration
+- ✅ Alerts configuration
+- ✅ Multiple comma-separated values


value of/for?

praveen-garg · 2025-08-01T13:05:40Z

tests/unit/C/services/notification/DataAvailabilityRule_Test_Summary.md

+### Current Coverage Statistics
+- **Function Coverage**: 95%
+- **Branch Coverage**: 90%
+- **Line Coverage**: 92%
+- **Edge Case Coverage**: 85%


I would rather github ci workflow or Jenkins agent publish them to repo

Useful information from cursor, no harm it it and frankly I don't care ion it is here or not

praveen-garg · 2025-08-01T13:05:57Z

tests/unit/C/services/notification/DataAvailabilityRule_Tests_Results.md

+**Status: ✅ ALL TESTS PASSING**
+
+- **Total Tests**: 33
+- **Passed**: 33 ✅
+- **Failed**: 0 ❌
+- **Execution Time**: ~3ms per test run


I would rather github ci workflow or Jenkins agent publish them to repo.

praveen-garg · 2025-08-01T13:10:51Z

C/services/notification/data_availability_rule.cpp

+	if (m_alert)
+	{
+		ret += comma;
+		ret += "{ \"alert\" : \"alert\" }";


Why the hardcoded string value "alert", same as key?

In future, We better use doc.AddMember to avoid fragile and cumbersome string building!

praveen-garg · 2025-08-01T13:21:44Z

C/services/notification/data_availability_rule.cpp

 		"order" : "3"
+	},
+	"alerts" : {
+		"description" : "Notify when alerts are raised",


Probably, We should be clear here!

Add alerts to the source of data for notifications
or
Notify for the alerts raised by the Fledge instance

Signed-off-by: Mark Riddoch <[email protected]>

praveen-garg · 2025-08-07T11:31:18Z

@MarkRiddoch should I expect a new commit? If no, please add comments to the conversation as appropriate so that I can close each of them and verify.

MarkRiddoch · 2025-08-07T11:37:12Z

@MarkRiddoch should I expect a new commit? If no, please add comments to the conversation as appropriate so that I can close each of them and verify.

I accidentally hit the re-review on this PR. More commits are to follow.

missign text item to the notification configuration. Signed-off-by: Mark Riddoch <[email protected]>

Signed-off-by: Mark Riddoch <[email protected]>

praveen-garg

You may want to check #107

praveen-garg · 2025-08-07T22:59:32Z

C/services/notification/delivery_plugin.cpp

+		if (doc.HasParseError())
+		{
+			// Failed to parse the reason, ignore macros
+			Logger::getLogger()->warn("Unable to parse reason document, macro substitutios withinthe notification will be ignored. The reason document is:  %s", reason.c_str());


space b/w within the

praveen-garg · 2025-08-07T22:59:53Z

C/services/notification/delivery_plugin.cpp

+		Value::ConstMemberIterator itr = data.MemberBegin();
+		if (itr == data.MemberEnd())
+		{
+			Logger::getLogger()->warn("Unable to perform macro substitution in the notifcation alert. No data element has no children, reason document %s", reason.c_str());


No data element has no children?

praveen-garg · 2025-08-07T23:01:23Z

C/services/notification/notification_subscription.cpp

+	NotificationApi *api = NotificationApi::getInstance();
+	string callBackURL = api->getAlertCallbackURL();
+	vector<std::string> keyValues;
+	Logger::getLogger()->fatal("Adding alert subscription for %s", callBackURL.c_str());


praveen-garg · 2025-08-07T23:08:24Z

C/services/notification/notification_api.cpp

+	catch (exception* ex)
+	{
+		m_logger->error("Exception '" + string(ex->what()) + \
+				"' while parsing readings for alaert" + \


praveen-garg · 2025-08-07T23:23:25Z

C/services/notification/data_availability_rule.cpp

 		"order" : "3"
+	},
+	"alerts" : {
+		"description" : "Deliver alert data to the notificaiton delivery mechanism",


notificaiton typo

praveen-garg · 2025-08-07T23:24:23Z

C/services/notification/delivery_plugin.cpp

+{
+	string rval = message;
+	vector<Macro> macros;
+	Logger::getLogger()->debug("Expand macros in messge %s with reason %s",


messge typo

Signed-off-by: Mark Riddoch <[email protected]>

gnandan · 2025-08-12T09:46:12Z

C/services/notification/notification_api.cpp

+	{
+		readings = new ReadingSet(&readingVec);
+	}
+	catch (exception* ex)


Constructor of ReadingSet thows exception of type ReadingSetException. Please catch by ReadingSetException* to align with ReadingSet class

The Jira is to do a specific task, not to make general changes, Therefore making unrelated changes is not part of this work. If a bug was spotted then it should be fixed, tis does not fall into that category.

gnandan · 2025-08-12T09:50:03Z

C/services/notification/notification_api.cpp

+				      responsePayload);
+		}
+	}
+	catch (exception ex)


Catching exception by value? consider catching by reference with is safer option.

The Jira is to do a specific task, not to make general changes, Therefore making unrelated changes is not part of this work. If a bug was spotted then it should be fixed, tis does not fall into that category.

gnandan · 2025-08-12T09:54:27Z

C/services/notification/notification_subscription.cpp

+	Logger::getLogger()->info("Adding alert subscription for %s", callBackURL.c_str());
+	if (!storage.registerTableNotification("alerts", "", keyValues, "insert", callBackURL))
+		Logger::getLogger()->error("Failed to register insert handler for alert subscription");
+	return storage.registerTableNotification("alerts", "", keyValues, "update", callBackURL);


consider adding a debug message to log if registerSubscription was successful.

The Jira is to do a specific task, not to make general changes, Therefore making unrelated changes is not part of this work. If a bug was spotted then it should be fixed, tis does not fall into that category.

gnandan · 2025-08-12T09:54:45Z

C/services/notification/notification_subscription.cpp

+	string callBackURL = api->getStatsRateCallbackURL();
+	vector<std::string> keyValues;
+	storage.unregisterTableNotification("alerts", "", keyValues, "update", callBackURL);
+	return storage.unregisterTableNotification("alerts", "", keyValues, "insert", callBackURL);


consider adding a debug message to log if unregister was successful.

The Jira is to do a specific task, not to make general changes, Therefore making unrelated changes is not part of this work. If a bug was spotted then it should be fixed, tis does not fall into that category.

gnandan · 2025-08-12T10:06:33Z

docs/index.rst

+
+Fledge will alert users to specific actions using the *bell* icon on
+the menu bar. These alerts can be used as a source of notification data
+by some of the notification plugins. Most notably the data availability


As JIRA mentions about match rules, can we include foglamp-rule-match plugin as well along with data availability here or somewhere else?

That is in a different repository and is therefore not part of this pull request.

gnandan · 2025-08-12T10:13:47Z

tests/unit/C/services/notification/test_async_config_change.cpp

 protected:
    void SetUp() override
    {
+        delete Logger::getLogger();


Why Logger is deleted? Where it is used ?

It gets used in some of the routines that are called by the tests and hence needs to be deleted.

gnandan · 2025-08-12T10:18:12Z

tests/unit/C/services/notification/test_data_availability_rule.cpp

+    ConfigCategory config = createBasicConfig("AUDIT001,AUDIT002");
+    rule.init(config);
+
+    string testJSON = "{\"AUDIT001\": \"value1\", \"AUDIT002\": \"value2\"}";


Can raw string literal with R"()" be used here?

It could on those platforms that support it. We don't use it generally because we had issues on some platforms. Hence the use of QUOTE instead.

No QUOTE has been used.

gnandan · 2025-08-12T10:22:26Z

tests/unit/C/services/notification/test_delivery_plugin_expand_macros.cpp

+TEST_F(DeliveryPluginExpandMacrosTest, BasicStringMacroSubstitution)
+{
+    string message = "Temperature is $temperature$ degrees";
+    string reason = "{\"reason\": \"triggered\", \"auditCode\": \"test\", \"data\": { \"example\" : {\"temperature\": \"25.5\"}}}";


can raw string literal with R"()" be used ?

It could on those platforms that support it. We don't use it generally because we had issues on some platforms. Hence the use of QUOTE instead.

No QUOTE has been used.

gnandan · 2025-08-12T10:34:02Z

C/services/notification/notification_manager.cpp

 			 "\"enumeration\", \"options\": [ \"one shot\", \"retriggered\", \"toggled\" ], "
 			 "\"displayName\" : \"Type\", \"order\" : \"4\","
 			 "\"default\" : \"one shot\"}, "
+		   "\"text\": {\"description\": \"Text message to send for this notification\", "


Option to provide value for message is not available during creating Notification instance. It appears later on the GUI once Notification instance is created. It leads to confusion. Please rectify it.

Yes, this is a known issue in the GUI and will be address in the GUI

Can we please log this issue?

@praveen-garg following JIRA has been logged https://scaledb.atlassian.net/browse/FOGL-10239

gnandan

Reviewed by testing. Data Availability rule allows to add alerts as a source.

FOGL-9963 Add Alerts as a source for notifications

d42fb17

Signed-off-by: Mark Riddoch <[email protected]>

MarkRiddoch requested a review from pintomax July 17, 2025 12:39

pintomax previously approved these changes Jul 17, 2025

View reviewed changes

praveen-garg reviewed Jul 19, 2025

View reviewed changes

Remove unwanted debug log message

986a170

Signed-off-by: Mark Riddoch <[email protected]>

MarkRiddoch dismissed pintomax’s stale review via 986a170 July 23, 2025 13:43

pintomax previously approved these changes Jul 23, 2025

View reviewed changes

MarkRiddoch added 3 commits July 25, 2025 13:22

Merge branch 'develop' into FOGL-9963

92cf16f

Add documentation of the new notification source

2f039fb

Signed-off-by: Mark Riddoch <[email protected]>

Merge branch 'FOGL-9963' of https://github.com/fledge-iot/fledge-serv…

3ffa5a6

…ice-notification into FOGL-9963

MarkRiddoch dismissed pintomax’s stale review via 3ffa5a6 July 28, 2025 13:34

MarkRiddoch added 6 commits July 29, 2025 11:49

Add comprehensive unit tests for DataAvailabilityRule

56dadba

- Add 33 test cases covering all major functionality - Include detailed documentation and test summaries - All tests pass with 95%+ code coverage - Ready for production use

Removed failed cursor unit tests

f984d05

Signed-off-by: Mark Riddoch <[email protected]>

Updated unit tests

70306ac

Signed-off-by: Mark Riddoch <[email protected]>

Merge branch 'develop' into FOGL-9963

a1b5e15

MarkRiddoch requested review from pintomax and praveen-garg August 1, 2025 12:46

praveen-garg reviewed Aug 1, 2025

View reviewed changes

Update next round of review comments

833f636

Signed-off-by: Mark Riddoch <[email protected]>

MarkRiddoch requested a review from praveen-garg August 5, 2025 11:21

MarkRiddoch added 3 commits August 7, 2025 12:28

FOGL-9963 Add macro substitution into the message string and add the

7889c20

missign text item to the notification configuration. Signed-off-by: Mark Riddoch <[email protected]>

Add alert example

c2b192d

Signed-off-by: Mark Riddoch <[email protected]>

Add expandMacros unit test

e95d88c

Signed-off-by: Mark Riddoch <[email protected]>

praveen-garg reviewed Aug 7, 2025

View reviewed changes

Add documentation for macro substitution

ac4e8a9

Signed-off-by: Mark Riddoch <[email protected]>

MarkRiddoch added 2 commits August 8, 2025 13:40

Macro update

0a495ba

Signed-off-by: Mark Riddoch <[email protected]>

Fix typos

682ff5a

Signed-off-by: Mark Riddoch <[email protected]>

MarkRiddoch requested a review from praveen-garg August 11, 2025 12:12

gnandan reviewed Aug 12, 2025

View reviewed changes

pintomax approved these changes Aug 18, 2025

View reviewed changes

praveen-garg requested a review from gnandan August 19, 2025 13:13

gnandan approved these changes Aug 21, 2025

View reviewed changes

MarkRiddoch merged commit e0e8437 into develop Aug 22, 2025
6 checks passed

MarkRiddoch deleted the FOGL-9963 branch August 22, 2025 08:44


		- The audit log entries that Fledge creates.

		- The alerts raised by the Fledge instance.

FOGL-9963 Add Alerts as a source for notifications #104

FOGL-9963 Add Alerts as a source for notifications #104

Uh oh!

Conversation

MarkRiddoch commented Jul 17, 2025

Uh oh!

praveen-garg Jul 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

praveen-garg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

praveen-garg commented Aug 7, 2025

Uh oh!

MarkRiddoch commented Aug 7, 2025

Uh oh!

praveen-garg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

praveen-garg Jul 19, 2025 •

edited

Loading

MarkRiddoch Aug 15, 2025 •

edited

Loading