[GitHub] [fineract] vidakovic commented on a diff in pull request #2730: FINERACT-1673: Add an endpoint for marking a single notification as read

[GitBox <gi...@apache.org>]

vidakovic commented on code in PR #2730:
URL: https://github.com/apache/fineract/pull/2730#discussion_r1015107141


##########
fineract-provider/src/main/java/org/apache/fineract/notification/service/NotificationReadPlatformServiceImpl.java:
##########
@@ -94,9 +95,8 @@ private boolean createUpdateCacheValue(Long appUserId, Long now,
     }
 
     private boolean checkForUnreadNotifications(Long appUserId) {
-        String sql = "SELECT id, notification_id as notificationId, user_id as userId, is_read as isRead, created_at "
-                + "as createdAt FROM notification_mapper WHERE user_id = ? AND is_read = false";
-        List<NotificationMapperData> notificationMappers = this.jdbcTemplate.query(sql, notificationMapperRow, appUserId);
+
+        Collection<NotificationMapper> notificationMappers = this.notificationMapperRepository.getUnreadNotificationsForAUser(appUserId);

Review Comment:
   This is better done with an `exists` query. No need to load all notifications into memory (note: what happens if you have a really large amount of them... OutOfMemoryException).



##########
fineract-provider/src/main/java/org/apache/fineract/notification/service/NotificationReadPlatformServiceImpl.java:
##########
@@ -94,9 +95,8 @@ private boolean createUpdateCacheValue(Long appUserId, Long now,
     }
 
     private boolean checkForUnreadNotifications(Long appUserId) {
-        String sql = "SELECT id, notification_id as notificationId, user_id as userId, is_read as isRead, created_at "
-                + "as createdAt FROM notification_mapper WHERE user_id = ? AND is_read = false";
-        List<NotificationMapperData> notificationMappers = this.jdbcTemplate.query(sql, notificationMapperRow, appUserId);
+
+        Collection<NotificationMapper> notificationMappers = this.notificationMapperRepository.getUnreadNotificationsForAUser(appUserId);

Review Comment:
   This is better done with an `exists` query. No need to load all notifications into memory (note: what happens if you have a really large amount of them... OutOfMemoryException).



-- 
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