galovics commented on code in PR #2330:
URL: https://github.com/apache/fineract/pull/2330#discussion_r876999955
##########
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:
Same as above mostly. However the transactional context is a good thing you
pointed out.
The original implementation also suffers from this in case the fallback
logic fails somewhere - which frankly is a possibility.
I'll add a simple try catch here to handle potential errors and prevent them
from rolling back the tx. Thanks.
##########
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:
Tried my best. :)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
