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 07:39:26 UTC
[GitHub] [fineract] wkigenyi opened a new pull request, #2730: [FINERACT-1673] Add an endpoint for marking a single notification as read
wkigenyi opened a new pull request, #2730:
URL: https://github.com/apache/fineract/pull/2730
## Description
Describe the changes made and why they were made.
Ignore if these details are present on the associated [Apache Fineract JIRA ticket](https://github.com/apache/fineract/pull/1284).
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [ ] Write the commit message as per https://github.com/apache/fineract/#pull-requests
- [ ] Acknowledge that we will not review PRs that are not passing the build _("green")_ - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
- [ ] Create/update unit or integration tests for verifying the changes made.
- [ ] Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
- [ ] Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
- [ ] Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)
FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.
--
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
[GitHub] [fineract] wkigenyi closed pull request #2730: FINERACT-1673: Add an endpoint for marking a single notification as read
Posted by GitBox <gi...@apache.org>.
wkigenyi closed pull request #2730: FINERACT-1673: Add an endpoint for marking a single notification as read
URL: https://github.com/apache/fineract/pull/2730
--
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
[GitHub] [fineract] wkigenyi commented on a diff in pull request #2730: FINERACT-1673: Add an endpoint for marking a single notification as read
Posted by GitBox <gi...@apache.org>.
wkigenyi commented on code in PR #2730:
URL: https://github.com/apache/fineract/pull/2730#discussion_r1015899866
##########
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:
The alternative I thought of was to extract the Object (NotificationMapper) then setIsRead = true, and set other fields too and then save. Another alternative I thought of involved DTO. I thought this maintaining this 2 liner will execute quicker, plus it takes care the ownership issue raised below without writing many lines.
--
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
[GitHub] [fineract] wkigenyi commented on a diff in pull request #2730: FINERACT-1673: Add an endpoint for marking a single notification as read
Posted by GitBox <gi...@apache.org>.
wkigenyi commented on code in PR #2730:
URL: https://github.com/apache/fineract/pull/2730#discussion_r1015870711
##########
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:
Thanks @galovics flushAutomatically is not needed here. It will not be in the next PR.
--
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
[GitHub] [fineract] wkigenyi commented on a diff in pull request #2730: FINERACT-1673: Add an endpoint for marking a single notification as read
Posted by GitBox <gi...@apache.org>.
wkigenyi commented on code in PR #2730:
URL: https://github.com/apache/fineract/pull/2730#discussion_r1015892593
##########
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:
When I leave out Transactional (from org.springframework.transaction.annotation.Transactional) I get the TransactionRequiredException on calling any of the methods where I have used.
--
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
[GitHub] [fineract] wkigenyi commented on pull request #2730: FINERACT-1673: Add an endpoint for marking a single notification as read
Posted by GitBox <gi...@apache.org>.
wkigenyi commented on PR #2730:
URL: https://github.com/apache/fineract/pull/2730#issuecomment-1306842568
@adamsaghy thanks I am changing this PR to draft until I have addressed the change requests.
--
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
[GitHub] [fineract] vidakovic commented on pull request #2730: FINERACT-1673: Add an endpoint for marking a single notification as read
Posted by GitBox <gi...@apache.org>.
vidakovic commented on PR #2730:
URL: https://github.com/apache/fineract/pull/2730#issuecomment-1308162940
Thanks for not getting discouraged by the comments, we mean well 🙂... and be sure that our own PRs get through the same process... and not everyone is making it and get's approved 😉. Looking forward to your next contribution...
--
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
[GitHub] [fineract] adamsaghy commented on pull request #2730: FINERACT-1673: Add an endpoint for marking a single notification as read
Posted by GitBox <gi...@apache.org>.
adamsaghy commented on PR #2730:
URL: https://github.com/apache/fineract/pull/2730#issuecomment-1305862264
@wkigenyi Please dont forget to squash all the 10 commits into 1, before the PR.
--
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
[GitHub] [fineract] wkigenyi commented on pull request #2730: FINERACT-1673: Add an endpoint for marking a single notification as read
Posted by GitBox <gi...@apache.org>.
wkigenyi commented on PR #2730:
URL: https://github.com/apache/fineract/pull/2730#issuecomment-1307693293
@vidakovic @galovics @adamsaghy I am closing this PR and redoing it in a better way
--
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
[GitHub] [fineract] vidakovic commented on a diff in pull request #2730: FINERACT-1673: Add an endpoint for marking a single notification as read
Posted by 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
[GitHub] [fineract] wkigenyi commented on a diff in pull request #2730: FINERACT-1673: Add an endpoint for marking a single notification as read
Posted by GitBox <gi...@apache.org>.
wkigenyi commented on code in PR #2730:
URL: https://github.com/apache/fineract/pull/2730#discussion_r1015684506
##########
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:
In the update query there is an AND which ensures that it indeed for the logged in user.
--
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
[GitHub] [fineract] vidakovic commented on pull request #2730: [FINERACT-1673] Add an endpoint for marking a single notification as read
Posted by GitBox <gi...@apache.org>.
vidakovic commented on PR #2730:
URL: https://github.com/apache/fineract/pull/2730#issuecomment-1305203125
Could you please follow the general formatting rules for PRs? Otherwise they don't get automatically linked to their Jira issues. The correct PR title would be `FINERACT-1673: Add an endpoint for marking a single notification as read` (no square brackets).
Also could you squash your commits please?
Thanks
--
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
[GitHub] [fineract] vidakovic commented on a diff in pull request #2730: FINERACT-1673: Add an endpoint for marking a single notification as read
Posted by GitBox <gi...@apache.org>.
vidakovic commented on code in PR #2730:
URL: https://github.com/apache/fineract/pull/2730#discussion_r1015111162
##########
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();
Review Comment:
Swagger/OpenAPI annotations missing
##########
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
Review Comment:
Swagger/OpenAPI annotations missing
--
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
[GitHub] [fineract] wkigenyi commented on a diff in pull request #2730: FINERACT-1673: Add an endpoint for marking a single notification as read
Posted by GitBox <gi...@apache.org>.
wkigenyi commented on code in PR #2730:
URL: https://github.com/apache/fineract/pull/2730#discussion_r1015296482
##########
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:
Thanks @vidakovic I am fixing this. I am having a problem though, when I updated my local code with the last, on running gradle bootRun, I get this error: "Caused by: java.sql.SQLSyntaxErrorException: (conn=988) Table 'batch_job_instance' already exists". How do I get past it ?
--
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
[GitHub] [fineract] galovics commented on a diff in pull request #2730: FINERACT-1673: Add an endpoint for marking a single notification as read
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [fineract] wkigenyi commented on a diff in pull request #2730: FINERACT-1673: Add an endpoint for marking a single notification as read
Posted by GitBox <gi...@apache.org>.
wkigenyi commented on code in PR #2730:
URL: https://github.com/apache/fineract/pull/2730#discussion_r1015632520
##########
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:
Since notifications are being fetching every after few seconds, it desirable that next batch of unread notifications fetched are indeed only those that are unread.
--
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
[GitHub] [fineract] galovics commented on a diff in pull request #2730: FINERACT-1673: Add an endpoint for marking a single notification as read
Posted by GitBox <gi...@apache.org>.
galovics commented on code in PR #2730:
URL: https://github.com/apache/fineract/pull/2730#discussion_r1015670573
##########
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:
I don't get it @wkigenyi . How does a flush help here? Due to the DB isolation level, there will be visibility issues anyway so there's no point in flushing right away.
--
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