fix(notifications): fixed notification severities on snapshot creation (#4604)

app/public/electron.js

@@ -17,7 +17,12 @@ import {
   willLaunchAtStartup,
   refreshWillLaunchAtStartup,
 } from "./auto-launch.js";
-import { setNotificationLevel, getNotificationLevel } from "./notifications.js";
+import {
+  setNotificationLevel,
+  getNotificationLevel,
+  LevelDisabled,
+  LevelWarningsAndErrors,
+} from "./notifications.js";
 import { serverForRepo } from "./server.js";
 import {
   loadConfigs,
@@ -549,21 +554,26 @@ app.on("ready", () => {
 
 function showRepoNotification(e) {
   const nl = getNotificationLevel();
-  if (nl === 0) {
+  if (nl === LevelDisabled) {
     // notifications disabled
     return;
   }
 
-  if (e.severity < 10 && nl === 1) {
-    // non-important notifications disabled.
+  const severity = e.notification.severity;
+  if (severity < 10 && nl === LevelWarningsAndErrors) {
+    log.info(
+      "showRepoNotification",
+      "skipping notification",
+      e.notification.subject,
+    );
     return;
   }
 
   let urgency = "normal";
 
-  if (e.severity < 0) {
+  if (severity < 0) {
     urgency = "low";
-  } else if (e.severity >= 10) {
+  } else if (severity >= 10) {
     // warnings and errors
     urgency = "critical";
   } else {

app/public/notifications.js

@@ -4,9 +4,9 @@ import { configDir } from "./config.js";
 const path = await import("path");
 const fs = await import("fs");
 
-const LevelDisabled = 0;
-const LevelDefault = 1;
-const LevelAll = 2;
+export const LevelDisabled = 0;
+export const LevelWarningsAndErrors = 1;
+export const LevelAll = 2;
 
 let level = -1;
 
@@ -16,7 +16,7 @@ export function getNotificationLevel() {
       const cfg = fs.readFileSync(path.join(configDir(), "notifications.json"));
       return JSON.parse(cfg).level;
     } catch (e) {
-      level = LevelDefault;
+      level = LevelWarningsAndErrors;
     }
   }
 
@@ -26,7 +26,7 @@ export function getNotificationLevel() {
 export function setNotificationLevel(l) {
   level = l;
   if (level < LevelDisabled || level > LevelAll) {
-    level = LevelDefault;
+    level = LevelWarningsAndErrors;
   }
 
   fs.writeFileSync(

cli/command_snapshot_create.go

@@ -150,7 +150,7 @@ func (c *commandSnapshotCreate) run(ctx context.Context, rep repo.RepositoryWrit
 	}
 
 	if c.sendSnapshotReport {
-		notification.Send(ctx, rep, "snapshot-report", st, notification.SeverityReport, c.svc.notificationTemplateOptions())
+		notification.Send(ctx, rep, "snapshot-report", st, c.reportSeverity(st), c.svc.notificationTemplateOptions())
 	}
 
 	// ensure we flush at least once in the session to properly close all pending buffers,
@@ -173,6 +173,17 @@ func (c *commandSnapshotCreate) run(ctx context.Context, rep repo.RepositoryWrit
 	return errors.Errorf("encountered %v errors:\n%v", len(finalErrors), strings.Join(finalErrors, "\n"))
 }
 
+func (c *commandSnapshotCreate) reportSeverity(st notifydata.MultiSnapshotStatus) notification.Severity {
+	switch st.OverallStatusCode() {
+	case notifydata.StatusCodeFatal:
+		return notification.SeverityError
+	case notifydata.StatusCodeWarnings:
+		return notification.SeverityWarning
+	default:
+		return notification.SeverityReport
+	}
+}
+
 func getTags(tagStrings []string) (map[string]string, error) {
 	numberOfPartsInTagString := 2
 	// tagKeyPrefix is the prefix for user defined tag keys.

internal/server/server.go

@@ -572,7 +572,19 @@ func (s *Server) sendSnapshotReport(st notifydata.MultiSnapshotStatus) {
 	// send the notification without blocking if we still have the repository
 	// it's possible that repository was closed in the meantime.
 	if rep != nil {
-		notification.Send(s.rootctx, rep, "snapshot-report", st, notification.SeverityReport, s.notificationTemplateOptions())
+		notification.Send(s.rootctx, rep, "snapshot-report", st, s.reportSeverity(st), s.notificationTemplateOptions())
+	}
+}
+
+func (*Server) reportSeverity(st notifydata.MultiSnapshotStatus) notification.Severity {
+	// TODO - this is a duplicate of the code in command_snapshot_create.go - we should unify this.
+	switch st.OverallStatusCode() {
+	case notifydata.StatusCodeFatal:
+		return notification.SeverityError
+	case notifydata.StatusCodeWarnings:
+		return notification.SeverityWarning
+	default:
+		return notification.SeverityReport
 	}
 }
 

notification/notifydata/multi_snapshot_status.go

@@ -164,6 +164,30 @@ func (m MultiSnapshotStatus) EventArgsType() grpcapi.NotificationEventArgType {
 	return grpcapi.NotificationEventArgType_ARG_TYPE_MULTI_SNAPSHOT_STATUS
 }
 
+// OverallStatusCode returns the overall status of the snapshots (StatusCodeSuccess, StatusCodeWarnings, or StatusCodeFatal).
+func (m MultiSnapshotStatus) OverallStatusCode() string {
+	var hasWarnings, hasErrors bool
+
+	for _, s := range m.Snapshots {
+		switch s.StatusCode() {
+		case StatusCodeFatal:
+			hasErrors = true
+		case StatusCodeWarnings:
+			hasWarnings = true
+		}
+	}
+
+	if hasErrors {
+		return StatusCodeFatal
+	}
+
+	if hasWarnings {
+		return StatusCodeWarnings
+	}
+
+	return StatusCodeSuccess
+}
+
 // OverallStatus returns the overall status of the snapshots.
 func (m MultiSnapshotStatus) OverallStatus() string {
 	var (

notification/notifydata/multi_snapshot_status_test.go

@@ -14,9 +14,10 @@
 
 func TestOverallStatus(t *testing.T) {
 	tests := []struct {
-		name      string
-		snapshots []*notifydata.ManifestWithError
-		expected  string
+		name         string
+		snapshots    []*notifydata.ManifestWithError
+		expected     string
+		expectedCode string
 	}{
 		{
 			name: "one success",
@@ -29,7 +30,8 @@ func TestOverallStatus(t *testing.T) {
 					},
 				}},
 			},
-			expected: "Successfully created a snapshot of /some/path",
+			expected:     "Successfully created a snapshot of /some/path",
+			expectedCode: notifydata.StatusCodeSuccess,
 		},
 		{
 			name: "all success",
@@ -37,7 +39,8 @@ func TestOverallStatus(t *testing.T) {
 				{Manifest: snapshot.Manifest{}},
 				{Manifest: snapshot.Manifest{}},
 			},
-			expected: "Successfully created 2 snapshots",
+			expected:     "Successfully created 2 snapshots",
+			expectedCode: notifydata.StatusCodeSuccess,
 		},
 		{
 			name: "one fatal error",
@@ -45,7 +48,8 @@ func TestOverallStatus(t *testing.T) {
 				{Manifest: snapshot.Manifest{}, Error: "fatal error"},
 				{Manifest: snapshot.Manifest{}},
 			},
-			expected: "Failed to create 1 of 2 snapshots",
+			expected:     "Failed to create 1 of 2 snapshots",
+			expectedCode: notifydata.StatusCodeFatal,
 		},
 		{
 			name: "one fatal error",
@@ -58,7 +62,8 @@ func TestOverallStatus(t *testing.T) {
 					},
 				}, Error: "fatal error"},
 			},
-			expected: "Failed to create a snapshot of /some/path",
+			expected:     "Failed to create a snapshot of /some/path",
+			expectedCode: notifydata.StatusCodeFatal,
 		},
 		{
 			name: "multiple fatal errors",
@@ -66,7 +71,8 @@ func TestOverallStatus(t *testing.T) {
 				{Manifest: snapshot.Manifest{}, Error: "fatal error"},
 				{Manifest: snapshot.Manifest{}, Error: "fatal error"},
 			},
-			expected: "Failed to create 2 of 2 snapshots",
+			expected:     "Failed to create 2 of 2 snapshots",
+			expectedCode: notifydata.StatusCodeFatal,
 		},
 		{
 			name: "one error",
@@ -74,7 +80,8 @@ func TestOverallStatus(t *testing.T) {
 				{Manifest: snapshot.Manifest{RootEntry: &snapshot.DirEntry{DirSummary: &fs.DirectorySummary{IgnoredErrorCount: 1}}}},
 				{Manifest: snapshot.Manifest{}},
 			},
-			expected: "Successfully created 2 snapshots",
+			expected:     "Successfully created 2 snapshots",
+			expectedCode: notifydata.StatusCodeWarnings,
 		},
 		{
 			name: "one fatal error and two errors",
@@ -84,7 +91,8 @@ func TestOverallStatus(t *testing.T) {
 				{Manifest: snapshot.Manifest{RootEntry: &snapshot.DirEntry{DirSummary: &fs.DirectorySummary{IgnoredErrorCount: 1}}}},
 				{Manifest: snapshot.Manifest{RootEntry: &snapshot.DirEntry{DirSummary: &fs.DirectorySummary{IgnoredErrorCount: 1}}}},
 			},
-			expected: "Failed to create 1 of 4 snapshots",
+			expected:     "Failed to create 1 of 4 snapshots",
+			expectedCode: notifydata.StatusCodeFatal,
 		},
 		{
 			name: "one fatal error and one errors",
@@ -93,7 +101,8 @@ func TestOverallStatus(t *testing.T) {
 				{Manifest: snapshot.Manifest{}},
 				{Manifest: snapshot.Manifest{RootEntry: &snapshot.DirEntry{DirSummary: &fs.DirectorySummary{IgnoredErrorCount: 1}}}},
 			},
-			expected: "Failed to create 1 of 3 snapshots",
+			expected:     "Failed to create 1 of 3 snapshots",
+			expectedCode: notifydata.StatusCodeFatal,
 		},
 		{
 			name: "multiple errors",
@@ -101,7 +110,8 @@ func TestOverallStatus(t *testing.T) {
 				{Manifest: snapshot.Manifest{RootEntry: &snapshot.DirEntry{DirSummary: &fs.DirectorySummary{IgnoredErrorCount: 1}}}},
 				{Manifest: snapshot.Manifest{RootEntry: &snapshot.DirEntry{DirSummary: &fs.DirectorySummary{IgnoredErrorCount: 1}}}},
 			},
-			expected: "Successfully created 2 snapshots",
+			expected:     "Successfully created 2 snapshots",
+			expectedCode: notifydata.StatusCodeWarnings,
 		},
 		{
 			name: "incomplete snapshot",
@@ -109,7 +119,8 @@ func TestOverallStatus(t *testing.T) {
 				{Manifest: snapshot.Manifest{IncompleteReason: "incomplete"}},
 				{Manifest: snapshot.Manifest{}},
 			},
-			expected: "Successfully created 2 snapshots",
+			expected:     "Successfully created 2 snapshots",
+			expectedCode: notifydata.StatusCodeSuccess,
 		},
 	}
 
@@ -117,6 +128,7 @@ func TestOverallStatus(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			mss := notifydata.MultiSnapshotStatus{Snapshots: tt.snapshots}
 			require.Equal(t, tt.expected, mss.OverallStatus())
+			require.Equal(t, tt.expectedCode, mss.OverallStatusCode())
 			testRoundTrip(t, &mss)
 		})
 	}