Merge pull request #10307 from thingsboard/fix/mobile-notifications

Fix mobile notification sending failures
2024-03-06 17:09:21 +02:00 · 2024-03-06 17:09:21 +02:00 · 439c59e289
commit 439c59e289
parent 8df0cabb52 ff18b8712a
6 changed files with 89 additions and 9 deletions
--- a/application/src/main/java/org/thingsboard/server/service/notification/DefaultNotificationCenter.java
+++ b/application/src/main/java/org/thingsboard/server/service/notification/DefaultNotificationCenter.java
@ -21,6 +21,7 @@ import lombok.extern.slf4j.Slf4j;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Service;
 import org.thingsboard.rule.engine.api.NotificationCenter;
+import org.thingsboard.server.cache.limits.RateLimitService;
 import org.thingsboard.server.common.data.EntityType;
 import org.thingsboard.server.common.data.User;
 import org.thingsboard.server.common.data.id.EntityId;
@ -62,7 +63,6 @@ import org.thingsboard.server.dao.notification.NotificationService;
 import org.thingsboard.server.dao.notification.NotificationSettingsService;
 import org.thingsboard.server.dao.notification.NotificationTargetService;
 import org.thingsboard.server.dao.notification.NotificationTemplateService;
-import org.thingsboard.server.cache.limits.RateLimitService;
 import org.thingsboard.server.gen.transport.TransportProtos;
 import org.thingsboard.server.queue.common.TbProtoQueueMsg;
 import org.thingsboard.server.queue.discovery.TopicService;
@ -204,6 +204,7 @@ public class DefaultNotificationCenter extends AbstractSubscriptionService imple

    private void processNotificationRequestAsync(NotificationProcessingContext ctx, List<NotificationTarget> targets, FutureCallback<NotificationRequestStats> callback) {
        notificationExecutor.submit(() -> {
+            long startTs = System.currentTimeMillis();
            NotificationRequestId requestId = ctx.getRequest().getId();
            for (NotificationTarget target : targets) {
                try {
@ -219,9 +220,16 @@ public class DefaultNotificationCenter extends AbstractSubscriptionService imple
                    return;
                }
            }
-            log.debug("[{}] Notification request processing is finished", requestId);

            NotificationRequestStats stats = ctx.getStats();
+            long time = System.currentTimeMillis() - startTs;
+            int sent = stats.getTotalSent().get();
+            int errors = stats.getTotalErrors().get();
+            if (errors > 0) {
+                log.info("[{}][{}] Notification request processing finished in {} ms (sent: {}, errors: {})", ctx.getTenantId(), requestId, time, sent, errors);
+            } else {
+                log.info("[{}][{}] Notification request processing finished in {} ms (sent: {})", ctx.getTenantId(), requestId, time, sent);
+            }
            updateRequestStats(ctx, requestId, stats);
            if (callback != null) {
                callback.onSuccess(stats);
--- a/application/src/main/java/org/thingsboard/server/service/notification/channels/MobileAppNotificationChannel.java
+++ b/application/src/main/java/org/thingsboard/server/service/notification/channels/MobileAppNotificationChannel.java
@ -15,6 +15,8 @@
 */
 package org.thingsboard.server.service.notification.channels;

+import com.fasterxml.jackson.databind.JsonNode;
+import com.google.common.base.Strings;
 import com.google.firebase.messaging.FirebaseMessagingException;
 import com.google.firebase.messaging.MessagingErrorCode;
 import lombok.RequiredArgsConstructor;
@ -82,7 +84,7 @@ public class MobileAppNotificationChannel implements NotificationChannel<User, M

    private Map<String, String> getNotificationData(MobileAppDeliveryMethodNotificationTemplate processedTemplate, NotificationProcessingContext ctx) {
        Map<String, String> data = Optional.ofNullable(processedTemplate.getAdditionalConfig())
-                .map(JacksonUtil::toFlatMap).orElseGet(HashMap::new);
+                .filter(JsonNode::isObject).map(JacksonUtil::toFlatMap).orElseGet(HashMap::new);
        NotificationInfo info = ctx.getRequest().getInfo();
        if (info == null) {
            return data;
@ -107,6 +109,7 @@ public class MobileAppNotificationChannel implements NotificationChannel<User, M
                });
                break;
        }
+        data.replaceAll((key, value) -> Strings.nullToEmpty(value));
        return data;
    }

--- a/application/src/test/java/org/thingsboard/server/service/notification/AbstractNotificationApiTest.java
+++ b/application/src/test/java/org/thingsboard/server/service/notification/AbstractNotificationApiTest.java
@ -60,6 +60,7 @@ import org.thingsboard.server.common.data.page.PageLink;
 import org.thingsboard.server.common.data.security.Authority;
 import org.thingsboard.server.controller.AbstractControllerTest;
 import org.thingsboard.server.dao.DaoUtil;
+import org.thingsboard.server.dao.notification.DefaultNotifications;
 import org.thingsboard.server.dao.notification.NotificationRequestService;
 import org.thingsboard.server.dao.notification.NotificationRuleService;
 import org.thingsboard.server.dao.notification.NotificationSettingsService;
@ -99,6 +100,8 @@ public abstract class AbstractNotificationApiTest extends AbstractControllerTest
    protected NotificationSettingsService notificationSettingsService;
    @Autowired
    protected SqlPartitioningRepository partitioningRepository;
+    @Autowired
+    protected DefaultNotifications defaultNotifications;

    public static final String DEFAULT_NOTIFICATION_SUBJECT = "Just a test";
    public static final NotificationType DEFAULT_NOTIFICATION_TYPE = NotificationType.GENERAL;
@ -249,10 +252,14 @@ public abstract class AbstractNotificationApiTest extends AbstractControllerTest
    }

    protected NotificationRule createNotificationRule(NotificationRuleTriggerConfig triggerConfig, String subject, String text, NotificationTargetId... targets) {
-        NotificationTemplate template = createNotificationTemplate(NotificationType.valueOf(triggerConfig.getTriggerType().toString()), subject, text, NotificationDeliveryMethod.WEB);
+        return createNotificationRule(triggerConfig, subject, text, List.of(targets), NotificationDeliveryMethod.WEB);
+    }
+
+    protected NotificationRule createNotificationRule(NotificationRuleTriggerConfig triggerConfig, String subject, String text, List<NotificationTargetId> targets, NotificationDeliveryMethod... deliveryMethods) {
+        NotificationTemplate template = createNotificationTemplate(NotificationType.valueOf(triggerConfig.getTriggerType().toString()), subject, text, deliveryMethods);

        NotificationRule rule = new NotificationRule();
-        rule.setName(triggerConfig.getTriggerType() + " " + Arrays.toString(targets));
+        rule.setName(triggerConfig.getTriggerType() + " " + targets);
        rule.setEnabled(true);
        rule.setTemplateId(template.getId());
        rule.setTriggerType(triggerConfig.getTriggerType());
@ -260,7 +267,7 @@ public abstract class AbstractNotificationApiTest extends AbstractControllerTest

        DefaultNotificationRuleRecipientsConfig recipientsConfig = new DefaultNotificationRuleRecipientsConfig();
        recipientsConfig.setTriggerType(triggerConfig.getTriggerType());
-        recipientsConfig.setTargets(DaoUtil.toUUIDs(List.of(targets)));
+        recipientsConfig.setTargets(DaoUtil.toUUIDs(targets));
        rule.setRecipientsConfig(recipientsConfig);

        return saveNotificationRule(rule);
--- a/application/src/test/java/org/thingsboard/server/service/notification/NotificationApiTest.java
+++ b/application/src/test/java/org/thingsboard/server/service/notification/NotificationApiTest.java
@ -30,8 +30,12 @@ import org.springframework.web.client.RestTemplate;
 import org.thingsboard.common.util.JacksonUtil;
 import org.thingsboard.rule.engine.api.NotificationCenter;
 import org.thingsboard.rule.engine.api.notification.FirebaseService;
+import org.thingsboard.server.common.data.Device;
 import org.thingsboard.server.common.data.EntityType;
 import org.thingsboard.server.common.data.User;
+import org.thingsboard.server.common.data.alarm.Alarm;
+import org.thingsboard.server.common.data.alarm.AlarmComment;
+import org.thingsboard.server.common.data.alarm.AlarmSeverity;
 import org.thingsboard.server.common.data.audit.ActionType;
 import org.thingsboard.server.common.data.id.DeviceId;
 import org.thingsboard.server.common.data.id.NotificationRequestId;
@ -49,6 +53,7 @@ import org.thingsboard.server.common.data.notification.NotificationRequestStats;
 import org.thingsboard.server.common.data.notification.NotificationRequestStatus;
 import org.thingsboard.server.common.data.notification.NotificationType;
 import org.thingsboard.server.common.data.notification.info.EntityActionNotificationInfo;
+import org.thingsboard.server.common.data.notification.rule.trigger.config.AlarmCommentNotificationRuleTriggerConfig;
 import org.thingsboard.server.common.data.notification.settings.MobileAppNotificationDeliveryMethodConfig;
 import org.thingsboard.server.common.data.notification.settings.NotificationSettings;
 import org.thingsboard.server.common.data.notification.settings.SlackNotificationDeliveryMethodConfig;
@ -771,6 +776,55 @@ public class NotificationApiTest extends AbstractNotificationApiTest {
        verifyNoMoreInteractions(firebaseService);
    }

+    @Test
+    public void testMobileAppNotifications_ruleBased() throws Exception {
+        loginSysAdmin();
+        MobileAppNotificationDeliveryMethodConfig config = new MobileAppNotificationDeliveryMethodConfig();
+        config.setFirebaseServiceAccountCredentials("testCredentials");
+        saveNotificationSettings(config);
+
+        loginTenantAdmin();
+        mobileToken = "tenantFcmToken";
+        doPost("/api/user/mobile/session", new MobileSessionInfo()).andExpect(status().isOk());
+
+        createNotificationRule(AlarmCommentNotificationRuleTriggerConfig.builder().onlyUserComments(true).build(),
+                DefaultNotifications.alarmComment.getSubject(), DefaultNotifications.alarmComment.getText(),
+                List.of(createNotificationTarget(tenantAdminUserId).getId()), NotificationDeliveryMethod.MOBILE_APP);
+
+        Device device = createDevice("test", "test");
+        UUID alarmDashboardId = UUID.randomUUID();
+        Alarm alarm = Alarm.builder()
+                .type("test")
+                .tenantId(tenantId)
+                .originator(device.getId())
+                .severity(AlarmSeverity.MAJOR)
+                .details(JacksonUtil.newObjectNode()
+                        .put("dashboardId", alarmDashboardId.toString()))
+                .build();
+        alarm = doPost("/api/alarm", alarm, Alarm.class);
+
+        AlarmComment comment = new AlarmComment();
+        comment.setComment(JacksonUtil.newObjectNode()
+                .put("text", "text"));
+        doPost("/api/alarm/" + alarm.getId() + "/comment", comment, AlarmComment.class);
+
+        ArgumentCaptor<Map<String, String>> msgCaptor = ArgumentCaptor.forClass(Map.class);
+        await().atMost(30, TimeUnit.SECONDS).untilAsserted(() -> {
+            verify(firebaseService).sendMessage(eq(tenantId), eq("testCredentials"),
+                    eq("tenantFcmToken"), eq("Comment on 'test' alarm"),
+                    eq(TENANT_ADMIN_EMAIL + " added comment: text"),
+                    msgCaptor.capture());
+        });
+        Map<String, String> firebaseMessageData = msgCaptor.getValue();
+        assertThat(firebaseMessageData.keySet()).doesNotContainNull().doesNotContain("");
+        assertThat(firebaseMessageData.values()).doesNotContainNull();
+        assertThat(firebaseMessageData.get("info.userEmail")).isEqualTo(TENANT_ADMIN_EMAIL);
+        assertThat(firebaseMessageData.get("info.alarmType")).isEqualTo("test");
+        assertThat(firebaseMessageData.get("onClick.enabled")).isEqualTo("true");
+        assertThat(firebaseMessageData.get("onClick.linkType")).isEqualTo("DASHBOARD");
+        assertThat(firebaseMessageData.get("onClick.dashboardId")).isEqualTo(alarmDashboardId.toString());
+    }
+
    @Test
    public void testMobileSettings_tenantLevel() throws Exception {
        MobileAppNotificationDeliveryMethodConfig config = new MobileAppNotificationDeliveryMethodConfig();
--- a/application/src/test/java/org/thingsboard/server/service/notification/NotificationRuleApiTest.java
+++ b/application/src/test/java/org/thingsboard/server/service/notification/NotificationRuleApiTest.java
@ -26,6 +26,7 @@ import org.springframework.boot.test.mock.mockito.SpyBean;
 import org.springframework.data.util.Pair;
 import org.springframework.test.context.TestPropertySource;
 import org.thingsboard.common.util.JacksonUtil;
+import org.thingsboard.server.cache.limits.RateLimitService;
 import org.thingsboard.server.common.data.DataConstants;
 import org.thingsboard.server.common.data.Device;
 import org.thingsboard.server.common.data.DeviceProfile;
@ -93,7 +94,6 @@ import org.thingsboard.server.dao.notification.DefaultNotifications;
 import org.thingsboard.server.dao.notification.NotificationRequestService;
 import org.thingsboard.server.dao.rule.RuleChainService;
 import org.thingsboard.server.dao.service.DaoSqlTest;
-import org.thingsboard.server.cache.limits.RateLimitService;
 import org.thingsboard.server.queue.notification.DefaultNotificationDeduplicationService;
 import org.thingsboard.server.service.notification.rule.cache.DefaultNotificationRulesCache;
 import org.thingsboard.server.service.state.DeviceStateService;
@ -140,8 +140,6 @@ public class NotificationRuleApiTest extends AbstractNotificationApiTest {
    @Autowired
    private NotificationRuleProcessor notificationRuleProcessor;
    @Autowired
-    private DefaultNotifications defaultNotifications;
-    @Autowired
    private DefaultNotificationRulesCache notificationRulesCache;
    @Autowired
    private DeviceStateService deviceStateService;
--- a/common/data/src/main/java/org/thingsboard/server/common/data/notification/NotificationRequestStats.java
+++ b/common/data/src/main/java/org/thingsboard/server/common/data/notification/NotificationRequestStats.java
@ -31,14 +31,20 @@ import java.util.concurrent.atomic.AtomicInteger;
 public class NotificationRequestStats {

    private final Map<NotificationDeliveryMethod, AtomicInteger> sent;
+    @JsonIgnore
+    private final AtomicInteger totalSent;
    private final Map<NotificationDeliveryMethod, Map<String, String>> errors;
+    @JsonIgnore
+    private final AtomicInteger totalErrors;
    private String error;
    @JsonIgnore
    private final Map<NotificationDeliveryMethod, Set<Object>> processedRecipients;

    public NotificationRequestStats() {
        this.sent = new ConcurrentHashMap<>();
+        this.totalSent = new AtomicInteger();
        this.errors = new ConcurrentHashMap<>();
+        this.totalErrors = new AtomicInteger();
        this.processedRecipients = new ConcurrentHashMap<>();
    }

@ -47,13 +53,16 @@ public class NotificationRequestStats {
                                    @JsonProperty("errors") Map<NotificationDeliveryMethod, Map<String, String>> errors,
                                    @JsonProperty("error") String error) {
        this.sent = sent;
+        this.totalSent = null;
        this.errors = errors;
+        this.totalErrors = null;
        this.error = error;
        this.processedRecipients = Collections.emptyMap();
    }

    public void reportSent(NotificationDeliveryMethod deliveryMethod, NotificationRecipient recipient) {
        sent.computeIfAbsent(deliveryMethod, k -> new AtomicInteger()).incrementAndGet();
+        totalSent.incrementAndGet();
    }

    public void reportError(NotificationDeliveryMethod deliveryMethod, Throwable error, NotificationRecipient recipient) {
@ -65,6 +74,7 @@ public class NotificationRequestStats {
            errorMessage = error.getClass().getSimpleName();
        }
        errors.computeIfAbsent(deliveryMethod, k -> new ConcurrentHashMap<>()).put(recipient.getTitle(), errorMessage);
+        totalErrors.incrementAndGet();
    }

    public void reportProcessed(NotificationDeliveryMethod deliveryMethod, Object recipientId) {