You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2022/05/19 08:00:03 UTC
[GitHub] [fineract] adamsaghy commented on a diff in pull request #2330: Topics cleanup
adamsaghy commented on code in PR #2330:
URL: https://github.com/apache/fineract/pull/2330#discussion_r876374527
##########
fineract-provider/src/main/java/org/apache/fineract/notification/eventandlistener/SpringNotificationEventListener.java:
##########
@@ -0,0 +1,42 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.notification.eventandlistener;
+
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.fineract.notification.data.NotificationData;
+import org.springframework.context.ApplicationListener;
+import org.springframework.context.annotation.Profile;
+import org.springframework.stereotype.Component;
+
+@Component
+@Profile("!activeMqEnabled")
Review Comment:
Even the the activeMq is enabled, still it might not working as expected and as a fallback logic we still need to publish and receive notification events. My recommendation is to remove the profile.
##########
fineract-provider/src/main/java/org/apache/fineract/notification/eventandlistener/ActiveMQNotificationEventPublisher.java:
##########
@@ -18,33 +18,24 @@
*/
package org.apache.fineract.notification.eventandlistener;
-import javax.jms.Destination;
-import javax.jms.JMSException;
-import javax.jms.Message;
-import javax.jms.Session;
+import javax.jms.Queue;
+import lombok.RequiredArgsConstructor;
+import org.apache.activemq.command.ActiveMQQueue;
import org.apache.fineract.notification.data.NotificationData;
-import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.context.annotation.Profile;
import org.springframework.jms.core.JmsTemplate;
-import org.springframework.jms.core.MessageCreator;
import org.springframework.stereotype.Service;
@Service
-public class NotificationEventService {
+@Profile("activeMqEnabled")
+@RequiredArgsConstructor
+public class ActiveMQNotificationEventPublisher implements NotificationEventPublisher {
private final JmsTemplate jmsTemplate;
- @Autowired
- public NotificationEventService(JmsTemplate jmsTemplate) {
- this.jmsTemplate = jmsTemplate;
- }
-
- public void broadcastNotification(final Destination destination, final NotificationData notificationData) {
- this.jmsTemplate.send(destination, new MessageCreator() {
-
- @Override
- public Message createMessage(Session session) throws JMSException {
- return session.createObjectMessage(notificationData);
- }
- });
+ @Override
+ public void broadcastNotification(NotificationData notificationData) {
+ Queue queue = new ActiveMQQueue("NotificationQueue");
Review Comment:
It might have been moved into configuration.
##########
fineract-provider/src/main/java/org/apache/fineract/notification/service/NotificationDomainServiceImpl.java:
##########
@@ -387,39 +379,15 @@ private void buildNotification(String permission, String objectType, Long object
String eventType, Long appUserId, Long officeId) {
String tenantIdentifier = ThreadLocalContextUtil.getTenant().getTenantIdentifier();
- Queue queue = new ActiveMQQueue("NotificationQueue");
- List<Long> userIds = retrieveSubscribers(officeId, permission);
+ Set<Long> userIds = getNotifiableUserIds(officeId, permission);
NotificationData notificationData = new NotificationData(objectType, objectIdentifier, eventType, appUserId, notificationContent,
false, false, tenantIdentifier, officeId, userIds);
- try {
- this.notificationEvent.broadcastNotification(queue, notificationData);
- } catch (Exception e) {
- this.springEventPublisher.broadcastNotification(notificationData);
- }
+ notificationEventPublisher.broadcastNotification(notificationData);
}
- private List<Long> retrieveSubscribers(Long officeId, String permission) {
-
- Set<TopicSubscriberData> topicSubscribers = new HashSet<>();
- List<Long> subscriberIds = new ArrayList<>();
- Long entityId = officeId;
- String entityType = "";
- if (officeRepository.findById(entityId).get().getParent() == null) {
- entityType = "OFFICE";
- } else {
- entityType = "BRANCH";
- }
- List<Role> allRoles = roleRepository.findAll();
- for (Role curRole : allRoles) {
- if (curRole.hasPermissionTo(permission) || curRole.hasPermissionTo("ALL_FUNCTIONS")) {
- String memberType = curRole.getName();
- topicSubscribers.addAll(topicSubscriberReadPlatformService.getSubscribers(entityId, entityType, memberType));
- }
- }
-
- for (TopicSubscriberData topicSubscriber : topicSubscribers) {
- subscriberIds.add(topicSubscriber.getUserId());
- }
- return subscriberIds;
+ private Set<Long> getNotifiableUserIds(Long officeId, String permission) {
+ Collection<AppUser> users = appUserRepository.findByOfficeId(officeId);
+ Collection<AppUser> usersWithPermission = users.stream().filter(aU -> aU.hasAnyPermission(permission, "ALL_FUNCTIONS")).toList();
+ return usersWithPermission.stream().map(AppUser::getId).collect(toSet());
Review Comment:
I have to trust you on this...
##########
fineract-provider/src/main/java/org/apache/fineract/notification/service/NotificationWritePlatformServiceImpl.java:
##########
@@ -64,7 +53,7 @@ public Long notify(Long userId, String objectType, Long objectIdentifier, String
private Long insertIntoNotificationMapper(Long userId, Long generatedNotificationId) {
AppUser appUser = this.appUserRepository.findById(userId).orElse(null);
NotificationMapper notificationMapper = new NotificationMapper(
- this.notificationGeneratorReadRepositoryWrapper.findById(generatedNotificationId), appUser, false, getCurrentDateTime());
+ this.notificationGeneratorReadRepositoryWrapper.findById(generatedNotificationId), appUser, false, new Date());
Review Comment:
Please rather use tenant date...
##########
fineract-provider/src/main/java/org/apache/fineract/notification/eventandlistener/SpringNotificationEventPublisher.java:
##########
@@ -18,18 +18,24 @@
*/
package org.apache.fineract.notification.eventandlistener;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
import org.apache.fineract.notification.data.NotificationData;
-import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.ApplicationEventPublisher;
+import org.springframework.context.annotation.Profile;
import org.springframework.stereotype.Service;
@Service
-public class SpringEventPublisher {
+@Profile("!activeMqEnabled")
Review Comment:
Same as above
##########
fineract-provider/src/main/java/org/apache/fineract/notification/service/NotificationDomainServiceImpl.java:
##########
@@ -387,39 +379,15 @@ private void buildNotification(String permission, String objectType, Long object
String eventType, Long appUserId, Long officeId) {
String tenantIdentifier = ThreadLocalContextUtil.getTenant().getTenantIdentifier();
- Queue queue = new ActiveMQQueue("NotificationQueue");
- List<Long> userIds = retrieveSubscribers(officeId, permission);
+ Set<Long> userIds = getNotifiableUserIds(officeId, permission);
NotificationData notificationData = new NotificationData(objectType, objectIdentifier, eventType, appUserId, notificationContent,
false, false, tenantIdentifier, officeId, userIds);
- try {
- this.notificationEvent.broadcastNotification(queue, notificationData);
- } catch (Exception e) {
- this.springEventPublisher.broadcastNotification(notificationData);
- }
+ notificationEventPublisher.broadcastNotification(notificationData);
Review Comment:
My concern: the notification publishing logic got kind of different.
Priorly: it was sending over JMS and if it failed for any reason we had a fallback logic to send it over internal spring event.
Now: the publisher is decided based on profile, but this way we dont have fallback logic anymore, also the error can bubble up and might triggering a rollback which is an unwanted behaviour.
##########
fineract-provider/src/main/java/org/apache/fineract/notification/service/NotificationWritePlatformServiceImpl.java:
##########
@@ -90,22 +79,15 @@ public Long notify(List<Long> userIds, String objectType, Long objectId, String
return generatedNotificationId;
}
- private List<Long> insertIntoNotificationMapper(List<Long> userIds, Long generatedNotificationId) {
+ private List<Long> insertIntoNotificationMapper(Collection<Long> userIds, Long generatedNotificationId) {
List<Long> mappedIds = new ArrayList<>();
for (Long userId : userIds) {
AppUser appUser = this.appUserRepository.findById(userId).get();
NotificationMapper notificationMapper = new NotificationMapper(
- this.notificationGeneratorReadRepositoryWrapper.findById(generatedNotificationId), appUser, false,
- getCurrentDateTime());
+ this.notificationGeneratorReadRepositoryWrapper.findById(generatedNotificationId), appUser, false, new Date());
Review Comment:
Please rather use tenant date...
##########
fineract-provider/src/main/java/org/apache/fineract/notification/service/NotificationWritePlatformServiceImpl.java:
##########
@@ -74,13 +63,13 @@ private Long insertIntoNotificationGenerator(String objectType, Long objectIdent
String notificationContent, boolean isSystemGenerated) {
Notification notification = new Notification(objectType, objectIdentifier, action, actorId, isSystemGenerated, notificationContent,
- getCurrentDateTime());
+ new Date());
Review Comment:
Please rather use tenant date...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org