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/11/07 12:47:42 UTC
[GitHub] [fineract] galovics commented on a diff in pull request #2730: FINERACT-1673: Add an endpoint for marking a single notification as read
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org