Add notifications related to currently displayed scenario

[mgoworko]

Describe your changes

The notifications improvements, extracted to separate PR from #7165:

• related to periodic processes, but other processing types too
• before: user did not see any new scenario activities, which were triggered by other user
• now: Nu FE receives all notifications as before, plus notifications related to all activities of currently displayed scenario

Checklist before merge

• Related issue ID is placed at the beginning of PR title in [brackets] (can be GH issue or Nu Jira issue)
• Code is cleaned from temporary changes and commented out lines
• Parts of the code that are not easy to understand are documented in the code
• Changes are covered by automated tests
• Showcase in dev-application.conf added to demonstrate the feature
• Documentation added or updated
• Added entry in Changelog.md describing the change from the perspective of a public distribution user
• Added MigrationGuide.md entry in the appropriate subcategory if introducing a breaking change
• Verify that PR will be squashed during merge

Summary by CodeRabbit

Release Notes

• New Features

  • Enhanced notifications endpoint to support filtering by scenario name.
  • Introduced a new service for fetching scenario activities with improved error handling.
  • Added a new notification type for scenario state updates.

• Improvements

  • Streamlined notification handling by removing unnecessary data types.
  • Updated API specifications to reflect changes in notification structures.

• Bug Fixes

  • Resolved issues with notification refresh scenarios and improved overall reliability.

• Tests

  • Expanded test coverage for notifications and scenario activities, ensuring better validation of functionality.

[gskrobisz]

LGTM

[arkadius]

Minor comments added

[arkadius]

None can mean at least two things:

• notifactions that are not assigned to any scenario
• all notifications (for every scenario)

We can use Option on REST API level because it is more idiomatic, but in scala code IMO we should use ADT with more explicit name for None

[mgoworko]

API without changes, added ADT NotificationsScope (NotificationsForLoggedUserAndScenario/NotificationsForLoggedUser)

[arkadius]

Just to be sure NotificationsForLoggedUser contains NotificationsForLoggedUserAndScenario for every scenario?

[mgoworko]

NotificationsForLoggedUser:

• is exactly as it worked before my changes
• those are notifications related to actions perfomed by user (deploy, cancel, results of those actions) for all scenarios
• fetched by user,. not by scenario
• does not include all scenario events, only those related to actions performed by current user
• that way logged user sees notifications about e.g. failed deploy, even when he is currently on scenario list page

NotificationsForLoggedUserAndScenario:

• in addition to NotificationsForLoggedUser contains also notifications related to all new activities related to current scenario
• those scenario-related notifications are not displayed, they only cause the scenario activities to reload

[arkadius]

let's rename parameter as well

[mgoworko]

done

[arkadius]

Just to be sure NotificationsForLoggedUser contains NotificationsForLoggedUserAndScenario for every scenario?

[arkadius]

LGTM, merge after fixing one comment and replying comment

[coderabbitai]

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)

designer/server/src/main/scala/pl/touk/nussknacker/ui/api/ScenarioActivityApiHttpService.scala (3)

28-28: LGTM! Good architectural improvement.

The change to use FetchScenarioActivityService instead of DeploymentManagerDispatcher improves separation of concerns and follows the Single Responsibility Principle.

Also applies to: 42-42

---

Line range hint 246-262: Consider moving filtering logic to service layer.

The current implementation mixes concerns by having the HTTP service handle complex filtering logic. Consider moving this logic to FetchScenarioActivityService to:

• Keep the HTTP service focused on request/response handling
• Make the filtering logic reusable
• Improve testability

def fetchActivities(
  processIdWithName: ProcessIdWithName
)(implicit loggedUser: LoggedUser): EitherT[Future, ScenarioActivityError, List[Dtos.ScenarioActivity]] = {
  EitherT.right {
    for {
-     combinedActivities <- fetchScenarioActivityService.fetchActivities(processIdWithName, after = None)
-     combinedSuccessfulActivities = combinedActivities.filter {
-       case _: BatchDeploymentRelatedActivity => true
-       case activity: DeploymentRelatedActivity =>
-         activity.result match {
-           case _: DeploymentResult.Success => true
-           case _: DeploymentResult.Failure => false
-         }
-       case _ => true
-     }
-     sortedResult = combinedSuccessfulActivities.map(toDto).toList.sortBy(_.date)
+     activities <- fetchScenarioActivityService.fetchFilteredActivities(processIdWithName, after = None)
+     sortedResult = activities.map(toDto).toList.sortBy(_.date)
    } yield sortedResult
  }
}

---

247-249: Plan to add API filtering support.

The comment indicates that API filtering support is pending. This should be tracked and implemented to provide better control over the returned activities.

Would you like me to create a GitHub issue to track the implementation of API filtering support?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 144ebdc and 3e725e7.

📒 Files selected for processing (2)

• designer/server/src/main/scala/pl/touk/nussknacker/ui/api/ScenarioActivityApiHttpService.scala (4 hunks)
• designer/server/src/main/scala/pl/touk/nussknacker/ui/notifications/NotificationService.scala (2 hunks)

🔇 Additional comments (4)

designer/server/src/main/scala/pl/touk/nussknacker/ui/notifications/NotificationService.scala (4)

27-39: Well-designed ADT for notification scopes

The NotificationsScope ADT effectively models the two distinct notification cases discussed in the previous comments:

1. NotificationsForLoggedUser: For user-specific notifications across all scenarios
2. NotificationsForLoggedUserAndScenario: For combined user and scenario-specific notifications

The sealed trait pattern ensures type safety and exhaustive pattern matching.

---

52-66: Clean implementation of notification scoping

The implementation effectively handles both notification scopes:

• For user-only scope: Returns only user action notifications
• For user+scenario scope: Combines both user actions and scenario activities

The use of for-comprehension for combining futures is idiomatic Scala.

---

101-104: Consider explicit error handling in notificationsForScenarioActivities

The current implementation silently converts errors to empty lists, which might hide underlying issues.

Previous suggestion to add explicit error handling and logging remains valid:

-      allActivities <- fetchScenarioActivityService.fetchActivities(processName, Some(limit))(user).value.map {
-        case Right(activities) => activities
-        case Left(_)           => List.empty
-      }
+      allActivities <- fetchScenarioActivityService.fetchActivities(processName, Some(limit))(user).value.map {
+        case Right(activities) => activities
+        case Left(error) =>
+          logger.warn(s"Failed to fetch activities for scenario $processName: $error")
+          List.empty
+      }

---

19-21: Breaking change in NotificationService trait

The signature change from notifications(implicit user: LoggedUser, ec: ExecutionContext) to notifications(scope: NotificationsScope)(implicit ec: ExecutionContext) is a breaking change that will require updates in all implementing classes. The change appears well-motivated by the need to support scenario-specific notifications.

✅ Verification successful

Breaking change concern is addressed in the same file

The breaking change in the NotificationService trait is properly handled as the only implementation NotificationServiceImpl is updated in the same file to match the new signature. The implementation correctly handles both scopes defined in the NotificationsScope ADT. No other implementations exist in the codebase that would need updating.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Find all implementations of NotificationService to ensure they're updated
ast-grep --pattern 'class $_ extends NotificationService'

Length of output: 59

---

Script:

#!/bin/bash
# Let's try a different pattern to find implementations, including trait mixing
rg "extends.*NotificationService|with.*NotificationService" -l

Length of output: 158

---

Script:

#!/bin/bash
# Also search for class definitions that might implement NotificationService
rg "class.*NotificationServiceImpl" -l

Length of output: 134

---

Script:

#!/bin/bash
# Let's check the actual implementation file content
rg "class.*NotificationServiceImpl" -A 10

Length of output: 1494

---

Script:

#!/bin/bash
# Let's also check if there are any tests that might show implementations
rg "NotificationService" ".*[Tt]est.*\.scala$"

Length of output: 106

---

Script:

#!/bin/bash
# Let's fix the test search by looking in all files
rg "NotificationService.*Test|Test.*NotificationService" --type scala

Length of output: 199

---

Script:

#!/bin/bash
# Let's also check if this is the only implementation by looking at the full content of NotificationService.scala
cat designer/server/src/main/scala/pl/touk/nussknacker/ui/notifications/NotificationService.scala

Length of output: 4380