galovics commented on code in PR #2730:
URL: https://github.com/apache/fineract/pull/2730#discussion_r1015374250
##########
fineract-provider/src/main/java/org/apache/fineract/notification/domain/NotificationMapperRepository.java:
##########
@@ -18,6 +18,25 @@
*/
package org.apache.fineract.notification.domain;
+import java.util.Collection;
import org.springframework.data.jpa.repository.JpaRepository;
+import org.springframework.data.jpa.repository.Modifying;
+import org.springframework.data.jpa.repository.Query;
+import org.springframework.data.repository.query.Param;
+import org.springframework.transaction.annotation.Transactional;
-public interface NotificationMapperRepository extends
JpaRepository<NotificationMapper, Long> {}
+public interface NotificationMapperRepository extends
JpaRepository<NotificationMapper, Long> {
+
+ @Transactional
+ @Modifying(flushAutomatically = true)
Review Comment:
Why the flushAutomatically?
##########
fineract-provider/src/main/java/org/apache/fineract/notification/domain/NotificationMapperRepository.java:
##########
@@ -18,6 +18,25 @@
*/
package org.apache.fineract.notification.domain;
+import java.util.Collection;
import org.springframework.data.jpa.repository.JpaRepository;
+import org.springframework.data.jpa.repository.Modifying;
+import org.springframework.data.jpa.repository.Query;
+import org.springframework.data.repository.query.Param;
+import org.springframework.transaction.annotation.Transactional;
-public interface NotificationMapperRepository extends
JpaRepository<NotificationMapper, Long> {}
+public interface NotificationMapperRepository extends
JpaRepository<NotificationMapper, Long> {
+
+ @Transactional
Review Comment:
No need for transactional here. Tx management should be around a business
functionality, not a repository call.
##########
fineract-provider/src/main/java/org/apache/fineract/notification/domain/NotificationMapperRepository.java:
##########
@@ -18,6 +18,25 @@
*/
package org.apache.fineract.notification.domain;
+import java.util.Collection;
import org.springframework.data.jpa.repository.JpaRepository;
+import org.springframework.data.jpa.repository.Modifying;
+import org.springframework.data.jpa.repository.Query;
+import org.springframework.data.repository.query.Param;
+import org.springframework.transaction.annotation.Transactional;
-public interface NotificationMapperRepository extends
JpaRepository<NotificationMapper, Long> {}
+public interface NotificationMapperRepository extends
JpaRepository<NotificationMapper, Long> {
+
+ @Transactional
+ @Modifying(flushAutomatically = true)
+ @Query("UPDATE NotificationMapper n SET n.isRead = true WHERE n.userId.id
= :userId")
+ void markAllNotificationsForAUserAsRead(@Param("userId") Long userId);
+
+ @Transactional
+ @Modifying(flushAutomatically = true)
+ @Query("UPDATE NotificationMapper n SET n.isRead = true WHERE n.userId.id
= :userId AND n.notification.id = :notificationId")
+ void markASingleNotificationForAUserAsRead(@Param("userId") Long userId,
@Param("notificationId") Long notificationId);
+
+ @Query("SELECT n FROM NotificationMapper n WHERE n.userId.id = :userId AND
n.isRead=false")
Review Comment:
Why a custom query? You can do this easily with a proper method name.
##########
fineract-provider/src/main/java/org/apache/fineract/notification/service/NotificationMapperWritePlatformService.java:
##########
@@ -23,4 +23,8 @@
public interface NotificationMapperWritePlatformService {
Long create(NotificationMapper notificationMapper);
+
+ void markAllNotificationsForAUserAsRead(Long userId);
+
+ void markASingleNotificationForAUserAsRead(Long userId, Long
notificationId);
Review Comment:
Method name should be markRead
##########
fineract-provider/src/main/java/org/apache/fineract/notification/api/NotificationApiResource.java:
##########
@@ -85,7 +89,16 @@ public String getAllNotifications(@Context final UriInfo
uriInfo,
@Consumes({ MediaType.APPLICATION_JSON })
@Produces({ MediaType.APPLICATION_JSON })
public void update() {
- this.context.authenticatedUser();
- this.notificationReadPlatformService.updateNotificationReadStatus();
+ final AppUser user = this.context.authenticatedUser();
+
this.notificationMapperWritePlatformService.markAllNotificationsForAUserAsRead(user.getId());
+ }
+
+ @PUT
+ @Path("{notificationId}")
Review Comment:
This should be `/{notificationId}` shouldnt it?
##########
fineract-provider/src/main/java/org/apache/fineract/notification/domain/NotificationMapperRepository.java:
##########
@@ -18,6 +18,25 @@
*/
package org.apache.fineract.notification.domain;
+import java.util.Collection;
import org.springframework.data.jpa.repository.JpaRepository;
+import org.springframework.data.jpa.repository.Modifying;
+import org.springframework.data.jpa.repository.Query;
+import org.springframework.data.repository.query.Param;
+import org.springframework.transaction.annotation.Transactional;
-public interface NotificationMapperRepository extends
JpaRepository<NotificationMapper, Long> {}
+public interface NotificationMapperRepository extends
JpaRepository<NotificationMapper, Long> {
+
+ @Transactional
+ @Modifying(flushAutomatically = true)
+ @Query("UPDATE NotificationMapper n SET n.isRead = true WHERE n.userId.id
= :userId")
+ void markAllNotificationsForAUserAsRead(@Param("userId") Long userId);
+
+ @Transactional
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/notification/service/NotificationMapperWritePlatformService.java:
##########
@@ -23,4 +23,8 @@
public interface NotificationMapperWritePlatformService {
Long create(NotificationMapper notificationMapper);
+
+ void markAllNotificationsForAUserAsRead(Long userId);
Review Comment:
Naming is too much here.
Just go easily with markAllAsRead
##########
fineract-provider/src/main/java/org/apache/fineract/notification/service/NotificationMapperWritePlatformServiceImpl.java:
##########
@@ -38,4 +38,14 @@ public Long create(NotificationMapper notificationMapper) {
this.notificationMapperRepository.saveAndFlush(notificationMapper);
return notificationMapper.getId();
}
+
+ @Override
+ public void markAllNotificationsForAUserAsRead(Long userId) {
+
notificationMapperRepository.markAllNotificationsForAUserAsRead(userId);
+ }
+
+ @Override
+ public void markASingleNotificationForAUserAsRead(Long userId, Long
notificationId) {
Review Comment:
I'm missing some validations here around the fact that this way users can
mark other users' notifications read.
Whether a notification corresponds to a particular user shall be checked.
--
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]
