diff --git a/manager/src/main/java/org/openremote/manager/notification/PushNotificationHandler.java b/manager/src/main/java/org/openremote/manager/notification/PushNotificationHandler.java index a6281e349b..f1f773f17c 100644 --- a/manager/src/main/java/org/openremote/manager/notification/PushNotificationHandler.java +++ b/manager/src/main/java/org/openremote/manager/notification/PushNotificationHandler.java @@ -256,39 +256,18 @@ public List getTargets( target -> { Notification.TargetType targetType = target.getType(); String targetId = target.getId(); - AssetQuery assetQuery = - new AssetQuery() - .select(new AssetQuery.Select().excludeAttributes()) - .types(ConsoleAsset.class) - .attributes( - new AttributePredicate( - ConsoleAsset.CONSOLE_PROVIDERS, - new ValueEmptyPredicate().negate(true), - false, - new NameValuePredicate.Path( - PushNotificationMessage.TYPE, "data", "token"))); - - switch (targetType) { - case REALM -> - // Any console assets in the target realm - assetQuery.realm(new RealmPredicate(targetId)); - case USER -> - // Any console assets linked to the target user - assetQuery.userIds(targetId); - case ASSET -> - // Any users linked to this asset and then their linked console assets - assetQuery.userIds( - assetStorageService.findUserAssetLinks(null, null, targetId).stream() - .map(ual -> ual.getId().getUserId()) - .collect(Collectors.toSet()) - .toArray(String[]::new)); - } - + // Prefer consoles that still have an FCM token so a normal send is unchanged. + // #3113: when none do, map FCM consoles anyway so the send records + // "No FCM token found" instead of a missing-targets error. Browser consoles + // (push version "web") stay out of both queries. List consoleAssetIds = - assetStorageService.findAll(assetQuery).stream() - .map(Asset::getId) - .distinct() - .toList(); + consoleIds( + consoleAssetsLinkedTo(targetType, targetId).attributes(nonEmptyFcmToken())); + if (consoleAssetIds.isEmpty()) { + consoleAssetIds = + consoleIds( + consoleAssetsLinkedTo(targetType, targetId).attributes(fcmProviderVersion())); + } if (!consoleAssetIds.isEmpty()) { // Special handling if target type is user (don't need to find all linked users) @@ -386,6 +365,25 @@ public void sendMessage( AbstractNotificationMessage message) throws Exception { + Notification.TargetType targetType = target.getType(); + String targetId = target.getId(); + + if (targetType != Notification.TargetType.ASSET + && targetType != Notification.TargetType.CUSTOM) { + String msg = "Target type not supported: " + targetType; + LOG.warning(msg); + throw new Exception(msg); + } + + // A console without a token must fail even when FCM itself is not configured. Otherwise the + // dev-mode skip below hides the missing token and the API reports success. + String fcmToken = consoleFCMTokenMap.get(targetId); + if (targetType == Notification.TargetType.ASSET && TextUtil.isNullOrEmpty(fcmToken)) { + String msg = "No FCM token found for console: " + targetId; + LOG.finer(msg); + throw new Exception(msg); + } + if (devMode && !valid) { // FCM not configured; log the notification instead of sending so the sent record is still // created @@ -399,19 +397,6 @@ public void sendMessage( return; } - Notification.TargetType targetType = target.getType(); - String targetId = target.getId(); - - if (targetType != Notification.TargetType.ASSET - && targetType != Notification.TargetType.CUSTOM) { - String msg = "Target type not supported: " + targetType; - LOG.warning(msg); - throw new Exception(msg); - } - - // Check this asset has an FCM token (i.e. it is registered for push notifications) - String fcmToken = consoleFCMTokenMap.get(targetId); - if (TextUtil.isNullOrEmpty(fcmToken)) { String msg = "No FCM token found for console: " + targetId; LOG.finer(msg); @@ -598,6 +583,50 @@ protected static Message buildFCMMessage(long id, PushNotificationMessage pushMe return builder.build(); } + private AssetQuery consoleAssetsLinkedTo(Notification.TargetType targetType, String targetId) { + AssetQuery assetQuery = + new AssetQuery() + .select(new AssetQuery.Select().excludeAttributes()) + .types(ConsoleAsset.class); + + switch (targetType) { + case REALM -> + // Any console assets in the target realm + assetQuery.realm(new RealmPredicate(targetId)); + case USER -> + // Any console assets linked to the target user + assetQuery.userIds(targetId); + case ASSET -> + // Any users linked to this asset and then their linked console assets + assetQuery.userIds( + assetStorageService.findUserAssetLinks(null, null, targetId).stream() + .map(ual -> ual.getId().getUserId()) + .collect(Collectors.toSet()) + .toArray(String[]::new)); + } + return assetQuery; + } + + private static AttributePredicate nonEmptyFcmToken() { + return new AttributePredicate( + ConsoleAsset.CONSOLE_PROVIDERS, + new ValueEmptyPredicate().negate(true), + false, + new NameValuePredicate.Path(PushNotificationMessage.TYPE, "data", "token")); + } + + private static AttributePredicate fcmProviderVersion() { + return new AttributePredicate( + ConsoleAsset.CONSOLE_PROVIDERS, + new StringPredicate(FCM_PROVIDER_NAME), + false, + new NameValuePredicate.Path(PushNotificationMessage.TYPE, "version")); + } + + private List consoleIds(AssetQuery assetQuery) { + return assetStorageService.findAll(assetQuery).stream().map(Asset::getId).distinct().toList(); + } + protected static boolean isLinkedToFcmProvider(ConsoleAsset asset) { return asset .getConsoleProviders() diff --git a/test/src/test/groovy/org/openremote/test/notification/NotificationTest.groovy b/test/src/test/groovy/org/openremote/test/notification/NotificationTest.groovy index 31db45e2a0..e9c53ada56 100644 --- a/test/src/test/groovy/org/openremote/test/notification/NotificationTest.groovy +++ b/test/src/test/groovy/org/openremote/test/notification/NotificationTest.groovy @@ -407,6 +407,34 @@ class NotificationTest extends Specification implements ManagerContainerTrait { assert !notificationTargetIds.contains(testuser3Console1.id) } + when: "all linked consoles for a user lack an FCM token" + notificationIds.clear() + notificationTargetIds.clear() + def testUser3Console2Asset = assetStorageService.find(testuser3Console2.id) as ConsoleAsset + testUser3Console2Asset.getConsoleProviders().map { + it.get(PushNotificationMessage.TYPE) + }.get().getData().put("token", null) + assetStorageService.merge(testUser3Console2Asset) + conditions.eventually { + assert pushNotificationHandler.consoleFCMTokenMap.get(testUser3Console2Asset.id) == null + } + notification.targets = [new Notification.Target(Notification.TargetType.USER, keycloakTestSetup.testuser3Id)] + advancePseudoClock(1, TimeUnit.HOURS, container) + adminNotificationResource.sendNotification(null, notification) + + then: "sent notifications record the missing FCM token instead of missing targets" + ex = thrown() + ex.response.withCloseable { r -> + assert r.status == 400 + return true + } + conditions.eventually { + def console1Notifications = adminNotificationResource.getNotifications(null, null, null, null, null, null, null, testuser3Console1.id, null, null, null, null, null) + def console2Notifications = adminNotificationResource.getNotifications(null, null, null, null, null, null, null, testuser3Console2.id, null, null, null, null, null) + assert console1Notifications.any { it.error?.contains("No FCM token found for console") } + assert console2Notifications.any { it.error?.contains("No FCM token found for console") } + } + when: "a notification handler throws an exception" throwPushHandlerException = true advancePseudoClock(1, TimeUnit.HOURS, container) @@ -434,7 +462,7 @@ class NotificationTest extends Specification implements ManagerContainerTrait { conditions.eventually { assert adminNotificationResource.getNotifications(null, null, null, null, null, null, null, testuser2Console.id, null, null, null, null, null).length == 2 notifications = adminNotificationResource.getNotifications(null, null, null, null, null, null, null, testuser3Console1.id, null, null, null, null, null) - assert notifications.length == 5 + assert notifications.length == 7 assert notifications.every {n -> PushNotificationMessage pushMessage = n.message as PushNotificationMessage pushMessage.getTitle() == "Test Action" && @@ -443,7 +471,7 @@ class NotificationTest extends Specification implements ManagerContainerTrait { n.deliveredOn == null && n.acknowledgedOn == null } - assert adminNotificationResource.getNotifications(null, null, null, null, null, null, null, testuser3Console2.id, null, null, null, null, null).length == 7 + assert adminNotificationResource.getNotifications(null, null, null, null, null, null, null, testuser3Console2.id, null, null, null, null, null).length == 8 assert adminNotificationResource.getNotifications(null, null, null, null, null, null, null, anonymousConsole.id, null, null, null, null, null).length == 3 } @@ -455,7 +483,7 @@ class NotificationTest extends Specification implements ManagerContainerTrait { then: "the notification should have been updated" conditions.eventually { notifications = adminNotificationResource.getNotifications(null, null, null, null, null, null, null, testuser3Console1.id, null, null, null, null, null) - assert notifications.length == 5 + assert notifications.length == 7 assert notifications.count {n -> n.deliveredOn != null} == 1 } @@ -467,7 +495,7 @@ class NotificationTest extends Specification implements ManagerContainerTrait { then: "the notification should have been updated" conditions.eventually { notifications = adminNotificationResource.getNotifications(null, null, null, null, null, null, null, testuser3Console1.id, null, null, null, null, null) - assert notifications.length == 5 + assert notifications.length == 7 assert notifications.count {n -> n.deliveredOn != null && n.acknowledgedOn != null && n.acknowledgement == "\"dismissed\"" } == 1 @@ -584,6 +612,19 @@ class NotificationTest extends Specification implements ManagerContainerTrait { when: "a repeat frequency is set and a notification is sent" // Clear exception flag throwPushHandlerException = false + // The missing-token scenario cleared both consoles. Later sends still need a token. + def fcmToken = "23123213ad2313b0897efd" + [testuser3Console1, testuser3Console2].each { console -> + def asset = assetStorageService.find(console.id) as ConsoleAsset + asset.getConsoleProviders().map { + it.get(PushNotificationMessage.TYPE) + }.get().getData().put("token", fcmToken) + assetStorageService.merge(asset) + } + conditions.eventually { + assert pushNotificationHandler.consoleFCMTokenMap.get(testuser3Console1.id) == fcmToken + assert pushNotificationHandler.consoleFCMTokenMap.get(testuser3Console2.id) == fcmToken + } // Move clock to beginning of next hour def advancement = Instant.ofEpochMilli(getClockTimeOf(container)) .truncatedTo(ChronoUnit.HOURS)