Notification Module #283
Notification Module #283
Conversation
| private String action; | ||
|
|
||
| @Column(name = "actor") | ||
| private Long actorId; |
There was a problem hiding this comment.
actor is text in notification_generator definition. But entity is defined with Long?
There was a problem hiding this comment.
Sorry about that, when the last time I changed the actorId to Long, I forgot to change it in the notification_generator definition.
| import java.util.List; | ||
|
|
||
| @Service | ||
| public class NotificationGeneratorReadPlatformServiceImpl implements NotificationGeneratorReadPlatformService { |
There was a problem hiding this comment.
If you are reading Entity from NotificationRepository, do we need to have separate readplatform service? Why can't caller directly call method on NotificationRepository? Readplatforms are designed to use JDBCTemplate not Repositories.
There was a problem hiding this comment.
Or are you trying as repository wrapper? In such case why the class name is read platform service?
There was a problem hiding this comment.
I am trying it as repository wrapper. What name should this class be named? Would it be NotificationGeneratorReadRepositoryWrapper?
There was a problem hiding this comment.
We follow an approach all read platforms should have jdbc template usage. In your it is not read platform it is just a wrapper. So it should be renamed to NotificationGeneratorReadRepositoryWrapper
| import java.util.List; | ||
|
|
||
| @Service | ||
| public class NotificationMapperReadPlatformServiceImpl implements NotificationMapperReadPlatformService { |
There was a problem hiding this comment.
Do we need this class? Why can't caller directly call methods on NotificationMapperRepository?
Read platform services are designed to use JDBCTemplate
There was a problem hiding this comment.
Or are you trying as repository wrapper? In such case why the class name is read platform service?
There was a problem hiding this comment.
I am trying it as repository wrapper. What name should this class be named? Would it be NotificationMapperReadRepositoryWrapper?
There was a problem hiding this comment.
Rename to NotificationMapperReadRepositoryWrapper
| import javax.jms.Session; | ||
|
|
||
| @Service | ||
| public class NotificationEvent { |
There was a problem hiding this comment.
Is it a event or service?
There was a problem hiding this comment.
It is a service which broadcast the notification event to an external queue.
There was a problem hiding this comment.
Rename this file to NotificationEventService
| private final PaginationHelper<NotificationData> paginationHelper = new PaginationHelper<>(); | ||
| private final NotificationDataRow notificationDataRow = new NotificationDataRow(); | ||
| private final NotificationMapperRow notificationMapperRow = new NotificationMapperRow(); | ||
| private HashMap<Long, CacheNotificationResponseHeader> notificationResponseHeaderCache = new HashMap<>(); |
There was a problem hiding this comment.
Does this cache works in multi tenancy environment? What happens if two tenants have same user id?
There was a problem hiding this comment.
I think I missed this case. Do I need to implement this now or can we add this later in another PR?
There was a problem hiding this comment.
Without this, I don't think this PR is useful. If this cannot be done, then remove this caching.
There was a problem hiding this comment.
It can be done. Just a little bit of work, need to associate tenant with user id's. Will try to complete it as soon as possible 😄
| import javax.jms.Session; | ||
|
|
||
| @Service | ||
| public class NotificationEvent { |
There was a problem hiding this comment.
Rename this file to NotificationEventService
| import java.util.List; | ||
|
|
||
| @Service | ||
| public class NotificationGeneratorReadPlatformServiceImpl implements NotificationGeneratorReadPlatformService { |
There was a problem hiding this comment.
We follow an approach all read platforms should have jdbc template usage. In your it is not read platform it is just a wrapper. So it should be renamed to NotificationGeneratorReadRepositoryWrapper
| import java.util.List; | ||
|
|
||
| @Service | ||
| public class NotificationMapperReadPlatformServiceImpl implements NotificationMapperReadPlatformService { |
There was a problem hiding this comment.
Rename to NotificationMapperReadRepositoryWrapper
|
@ad-os Changes looks ok. Can you resent PR with single commit please. |
|
@ad-os as discussed over slack, You have added a library which is having dependency on BSD licensed third party library. Can you check the current binary licensing please. |
|
@ad-os Please refer LICENSE_RELEASE file. You need to add an entry about this BSD library. |
Added Notification Module Added Notification Module Added Notification Module Added license file for jar analyzer Added entry into LICENSE_RELEASE file Added gradle wrapper
This module will allow developers to integrate notifications with their functionality. Here are some wikis on which you can read more about this module.